From 989e80d4ea62e68cfab69a1cd16d481cbccc6c02 Mon Sep 17 00:00:00 2001 From: jfecher Date: Fri, 20 Oct 2023 15:48:01 -0500 Subject: [PATCH] fix: Add compiler error message for invalid input types (#3220) --- .../noirc_frontend/src/hir/def_map/mod.rs | 6 +- .../src/hir/resolution/errors.rs | 5 + .../src/hir/resolution/resolver.rs | 94 +++++++++++++++++++ compiler/noirc_frontend/src/hir_def/types.rs | 40 ++++++++ compiler/noirc_frontend/src/lexer/token.rs | 7 ++ 5 files changed, 149 insertions(+), 3 deletions(-) diff --git a/compiler/noirc_frontend/src/hir/def_map/mod.rs b/compiler/noirc_frontend/src/hir/def_map/mod.rs index 006e00d2d07..345e5447bf5 100644 --- a/compiler/noirc_frontend/src/hir/def_map/mod.rs +++ b/compiler/noirc_frontend/src/hir/def_map/mod.rs @@ -173,9 +173,9 @@ impl CrateDefMap { .value_definitions() .filter_map(|id| { id.as_function().map(|function_id| { - let attributes = interner.function_attributes(&function_id); - let is_entry_point = !attributes.has_contract_library_method() - && !attributes.is_test_function(); + let is_entry_point = interner + .function_attributes(&function_id) + .is_contract_entry_point(); ContractFunctionMeta { function_id, is_entry_point } }) }) diff --git a/compiler/noirc_frontend/src/hir/resolution/errors.rs b/compiler/noirc_frontend/src/hir/resolution/errors.rs index 80a0b557465..afdf65e5c5c 100644 --- a/compiler/noirc_frontend/src/hir/resolution/errors.rs +++ b/compiler/noirc_frontend/src/hir/resolution/errors.rs @@ -78,6 +78,8 @@ pub enum ResolverError { InvalidClosureEnvironment { typ: Type, span: Span }, #[error("{name} is private and not visible from the current module")] PrivateFunctionCalled { name: String, span: Span }, + #[error("Only sized types may be used in the entry point to a program")] + InvalidTypeForEntryPoint { span: Span }, } impl ResolverError { @@ -294,6 +296,9 @@ impl From for Diagnostic { ResolverError::PrivateFunctionCalled { span, name } => Diagnostic::simple_warning( format!("{name} is private and not visible from the current module"), format!("{name} is private"), span), + ResolverError::InvalidTypeForEntryPoint { span } => Diagnostic::simple_error( + "Only sized types may be used in the entry point to a program".to_string(), + "Slices, references, or any type containing them may not be used in main or a contract function".to_string(), span), } } } diff --git a/compiler/noirc_frontend/src/hir/resolution/resolver.rs b/compiler/noirc_frontend/src/hir/resolution/resolver.rs index ca30b31e78d..a4a85d85929 100644 --- a/compiler/noirc_frontend/src/hir/resolution/resolver.rs +++ b/compiler/noirc_frontend/src/hir/resolution/resolver.rs @@ -736,6 +736,10 @@ impl<'a> Resolver<'a> { }); } + if self.is_entry_point_function(func) { + self.verify_type_valid_for_program_input(&typ); + } + let pattern = self.resolve_pattern(pattern, DefinitionKind::Local(None)); let typ = self.resolve_type_inner(typ, &mut generics); @@ -810,6 +814,14 @@ impl<'a> Resolver<'a> { } } + fn is_entry_point_function(&self, func: &NoirFunction) -> bool { + if self.in_contract() { + func.attributes().is_contract_entry_point() + } else { + func.name() == MAIN_FUNCTION + } + } + /// True if the `distinct` keyword is allowed on a function's return type fn distinct_allowed(&self, func: &NoirFunction) -> bool { if self.in_contract() { @@ -1627,6 +1639,88 @@ impl<'a> Resolver<'a> { } HirLiteral::FmtStr(str, fmt_str_idents) } + + /// Only sized types are valid to be used as main's parameters or the parameters to a contract + /// function. If the given type is not sized (e.g. contains a slice or NamedGeneric type), an + /// error is issued. + fn verify_type_valid_for_program_input(&mut self, typ: &UnresolvedType) { + match &typ.typ { + UnresolvedTypeData::FieldElement + | UnresolvedTypeData::Integer(_, _) + | UnresolvedTypeData::Bool + | UnresolvedTypeData::Unit + | UnresolvedTypeData::Error => (), + + UnresolvedTypeData::MutableReference(_) + | UnresolvedTypeData::Function(_, _, _) + | UnresolvedTypeData::FormatString(_, _) + | UnresolvedTypeData::TraitAsType(..) + | UnresolvedTypeData::Unspecified => { + let span = typ.span.expect("Function parameters should always have spans"); + self.push_err(ResolverError::InvalidTypeForEntryPoint { span }); + } + + UnresolvedTypeData::Array(length, element) => { + if let Some(length) = length { + self.verify_type_expression_valid_for_program_input(length); + } else { + let span = typ.span.expect("Function parameters should always have spans"); + self.push_err(ResolverError::InvalidTypeForEntryPoint { span }); + } + self.verify_type_valid_for_program_input(element); + } + UnresolvedTypeData::Expression(expression) => { + self.verify_type_expression_valid_for_program_input(expression); + } + UnresolvedTypeData::String(length) => { + if let Some(length) = length { + self.verify_type_expression_valid_for_program_input(length); + } else { + let span = typ.span.expect("Function parameters should always have spans"); + self.push_err(ResolverError::InvalidTypeForEntryPoint { span }); + } + } + UnresolvedTypeData::Named(path, generics) => { + // Since the type is named, we need to resolve it to see what it actually refers to + // in order to check whether it is valid. Since resolving it may lead to a + // resolution error, we have to truncate our error count to the previous count just + // in case. This is to ensure resolution errors are not issued twice when this type + // is later resolved properly. + let error_count = self.errors.len(); + let resolved = self.resolve_named_type(path.clone(), generics.clone(), &mut vec![]); + self.errors.truncate(error_count); + + if !resolved.is_valid_for_program_input() { + let span = typ.span.expect("Function parameters should always have spans"); + self.push_err(ResolverError::InvalidTypeForEntryPoint { span }); + } + } + UnresolvedTypeData::Tuple(elements) => { + for element in elements { + self.verify_type_valid_for_program_input(element); + } + } + } + } + + fn verify_type_expression_valid_for_program_input(&mut self, expr: &UnresolvedTypeExpression) { + match expr { + UnresolvedTypeExpression::Constant(_, _) => (), + UnresolvedTypeExpression::Variable(path) => { + let error_count = self.errors.len(); + let resolved = self.resolve_named_type(path.clone(), vec![], &mut vec![]); + self.errors.truncate(error_count); + + if !resolved.is_valid_for_program_input() { + self.push_err(ResolverError::InvalidTypeForEntryPoint { span: path.span() }); + } + } + UnresolvedTypeExpression::BinaryOperation(lhs, _, rhs, _) => { + self.verify_type_expression_valid_for_program_input(lhs); + self.verify_type_expression_valid_for_program_input(rhs); + } + } + } } /// Gives an error if a user tries to create a mutable reference diff --git a/compiler/noirc_frontend/src/hir_def/types.rs b/compiler/noirc_frontend/src/hir_def/types.rs index 1f7baa0afaf..110334f331e 100644 --- a/compiler/noirc_frontend/src/hir_def/types.rs +++ b/compiler/noirc_frontend/src/hir_def/types.rs @@ -520,6 +520,46 @@ impl Type { } } } + + /// True if this type can be used as a parameter to `main` or a contract function. + /// This is only false for unsized types like slices or slices that do not make sense + /// as a program input such as named generics or mutable references. + /// + /// This function should match the same check done in `create_value_from_type` in acir_gen. + /// If this function does not catch a case where a type should be valid, it will later lead to a + /// panic in that function instead of a user-facing compiler error message. + pub(crate) fn is_valid_for_program_input(&self) -> bool { + match self { + // Type::Error is allowed as usual since it indicates an error was already issued and + // we don't need to issue further errors about this likely unresolved type + Type::FieldElement + | Type::Integer(_, _) + | Type::Bool + | Type::Unit + | Type::Constant(_) + | Type::Error => true, + + Type::FmtString(_, _) + | Type::TypeVariable(_, _) + | Type::NamedGeneric(_, _) + | Type::Function(_, _, _) + | Type::MutableReference(_) + | Type::Forall(_, _) + | Type::TraitAsType(..) + | Type::NotConstant => false, + + Type::Array(length, element) => { + length.is_valid_for_program_input() && element.is_valid_for_program_input() + } + Type::String(length) => length.is_valid_for_program_input(), + Type::Tuple(elements) => elements.iter().all(|elem| elem.is_valid_for_program_input()), + Type::Struct(definition, generics) => definition + .borrow() + .get_fields(generics) + .into_iter() + .all(|(_, field)| field.is_valid_for_program_input()), + } + } } impl std::fmt::Display for Type { diff --git a/compiler/noirc_frontend/src/lexer/token.rs b/compiler/noirc_frontend/src/lexer/token.rs index c91d1610f48..fe3e9476318 100644 --- a/compiler/noirc_frontend/src/lexer/token.rs +++ b/compiler/noirc_frontend/src/lexer/token.rs @@ -398,6 +398,13 @@ impl Attributes { matches!(self.function, Some(FunctionAttribute::Test(_))) } + /// True if these attributes mean the given function is an entry point function if it was + /// defined within a contract. Note that this does not check if the function is actually part + /// of a contract. + pub fn is_contract_entry_point(&self) -> bool { + !self.has_contract_library_method() && !self.is_test_function() + } + /// Returns note if a deprecated secondary attribute is found pub fn get_deprecated_note(&self) -> Option> { self.secondary.iter().find_map(|attr| match attr {