Skip to content

Commit

Permalink
fix: Add compiler error message for invalid input types (#3220)
Browse files Browse the repository at this point in the history
  • Loading branch information
jfecher authored Oct 20, 2023
1 parent c96ae48 commit 989e80d
Show file tree
Hide file tree
Showing 5 changed files with 149 additions and 3 deletions.
6 changes: 3 additions & 3 deletions compiler/noirc_frontend/src/hir/def_map/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
})
})
Expand Down
5 changes: 5 additions & 0 deletions compiler/noirc_frontend/src/hir/resolution/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -294,6 +296,9 @@ impl From<ResolverError> 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),
}
}
}
94 changes: 94 additions & 0 deletions compiler/noirc_frontend/src/hir/resolution/resolver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -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
Expand Down
40 changes: 40 additions & 0 deletions compiler/noirc_frontend/src/hir_def/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
7 changes: 7 additions & 0 deletions compiler/noirc_frontend/src/lexer/token.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<Option<String>> {
self.secondary.iter().find_map(|attr| match attr {
Expand Down

0 comments on commit 989e80d

Please sign in to comment.