diff --git a/compiler/noirc_frontend/src/elaborator/lints.rs b/compiler/noirc_frontend/src/elaborator/lints.rs new file mode 100644 index 00000000000..da3e033cd4c --- /dev/null +++ b/compiler/noirc_frontend/src/elaborator/lints.rs @@ -0,0 +1,221 @@ +use crate::{ + ast::FunctionKind, + graph::CrateId, + hir::{ + resolution::errors::{PubPosition, ResolverError}, + type_check::TypeCheckError, + }, + hir_def::expr::HirIdent, + macros_api::{ + HirExpression, HirLiteral, NodeInterner, NoirFunction, UnaryOp, UnresolvedTypeData, + Visibility, + }, + node_interner::{DefinitionKind, ExprId}, + Type, +}; +use acvm::AcirField; + +use noirc_errors::Span; + +pub(super) fn deprecated_function(interner: &NodeInterner, expr: ExprId) -> Option { + let HirExpression::Ident(HirIdent { location, id, impl_kind: _ }, _) = + interner.expression(&expr) + else { + return None; + }; + + let Some(DefinitionKind::Function(func_id)) = interner.try_definition(id).map(|def| &def.kind) + else { + return None; + }; + + let attributes = interner.function_attributes(func_id); + attributes.get_deprecated_note().map(|note| TypeCheckError::CallDeprecated { + name: interner.definition_name(id).to_string(), + note, + span: location.span, + }) +} + +/// Inline attributes are only relevant for constrained functions +/// as all unconstrained functions are not inlined and so +/// associated attributes are disallowed. +pub(super) fn inlining_attributes(func: &NoirFunction) -> Option { + if !func.def.is_unconstrained { + let attributes = func.attributes().clone(); + + if attributes.is_no_predicates() { + Some(ResolverError::NoPredicatesAttributeOnUnconstrained { + ident: func.name_ident().clone(), + }) + } else if attributes.is_foldable() { + Some(ResolverError::FoldAttributeOnUnconstrained { ident: func.name_ident().clone() }) + } else { + None + } + } else { + None + } +} + +/// Attempting to define new low level (`#[builtin]` or `#[foreign]`) functions outside of the stdlib is disallowed. +pub(super) fn low_level_function_outside_stdlib( + func: &NoirFunction, + crate_id: CrateId, +) -> Option { + let is_low_level_function = + func.attributes().function.as_ref().map_or(false, |func| func.is_low_level()); + if !crate_id.is_stdlib() && is_low_level_function { + Some(ResolverError::LowLevelFunctionOutsideOfStdlib { ident: func.name_ident().clone() }) + } else { + None + } +} + +/// `pub` is required on return types for entry point functions +pub(super) fn missing_pub(func: &NoirFunction, is_entry_point: bool) -> Option { + if is_entry_point + && func.return_type().typ != UnresolvedTypeData::Unit + && func.def.return_visibility == Visibility::Private + { + Some(ResolverError::NecessaryPub { ident: func.name_ident().clone() }) + } else { + None + } +} + +/// `#[recursive]` attribute is only allowed for entry point functions +pub(super) fn recursive_non_entrypoint_function( + func: &NoirFunction, + is_entry_point: bool, +) -> Option { + if !is_entry_point && func.kind == FunctionKind::Recursive { + Some(ResolverError::MisplacedRecursiveAttribute { ident: func.name_ident().clone() }) + } else { + None + } +} + +/// Test functions cannot have arguments in order to be executable. +pub(super) fn test_function_with_args(func: &NoirFunction) -> Option { + if func.attributes().is_test_function() && !func.parameters().is_empty() { + Some(ResolverError::TestFunctionHasParameters { span: func.name_ident().span() }) + } else { + None + } +} + +/// Check that we are not passing a mutable reference from a constrained runtime to an unconstrained runtime. +pub(super) fn unconstrained_function_args( + function_args: &[(Type, ExprId, Span)], +) -> Vec { + function_args + .iter() + .filter_map(|(typ, _, span)| { + if type_contains_mutable_reference(typ) { + Some(TypeCheckError::ConstrainedReferenceToUnconstrained { span: *span }) + } else { + None + } + }) + .collect() +} + +/// Check that we are not passing a slice from an unconstrained runtime to a constrained runtime. +pub(super) fn unconstrained_function_return( + return_type: &Type, + span: Span, +) -> Option { + if return_type.contains_slice() { + Some(TypeCheckError::UnconstrainedSliceReturnToConstrained { span }) + } else if type_contains_mutable_reference(return_type) { + Some(TypeCheckError::UnconstrainedReferenceToConstrained { span }) + } else { + None + } +} + +fn type_contains_mutable_reference(typ: &Type) -> bool { + matches!(&typ.follow_bindings(), Type::MutableReference(_)) +} + +/// Only entrypoint functions require a `pub` visibility modifier applied to their return types. +/// +/// Application of `pub` to other functions is not meaningful and is a mistake. +pub(super) fn unnecessary_pub_return( + func: &NoirFunction, + is_entry_point: bool, +) -> Option { + if !is_entry_point && func.def.return_visibility == Visibility::Public { + Some(ResolverError::UnnecessaryPub { + ident: func.name_ident().clone(), + position: PubPosition::ReturnType, + }) + } else { + None + } +} + +/// Only arguments to entrypoint functions may have a non-private visibility modifier applied to them. +/// +/// Other functions are disallowed from declaring the visibility of their arguments. +pub(super) fn unnecessary_pub_argument( + func: &NoirFunction, + arg_visibility: Visibility, + is_entry_point: bool, +) -> Option { + if arg_visibility == Visibility::Public && !is_entry_point { + Some(ResolverError::UnnecessaryPub { + ident: func.name_ident().clone(), + position: PubPosition::Parameter, + }) + } else { + None + } +} + +/// Check if an assignment is overflowing with respect to `annotated_type` +/// in a declaration statement where `annotated_type` is an unsigned integer +pub(crate) fn overflowing_uint( + interner: &NodeInterner, + rhs_expr: &ExprId, + annotated_type: &Type, +) -> Vec { + let expr = interner.expression(rhs_expr); + let span = interner.expr_span(rhs_expr); + + let mut errors = Vec::with_capacity(2); + match expr { + HirExpression::Literal(HirLiteral::Integer(value, false)) => { + let v = value.to_u128(); + if let Type::Integer(_, bit_count) = annotated_type { + let bit_count: u32 = (*bit_count).into(); + let max = 1 << bit_count; + if v >= max { + errors.push(TypeCheckError::OverflowingAssignment { + expr: value, + ty: annotated_type.clone(), + range: format!("0..={}", max - 1), + span, + }); + }; + }; + } + HirExpression::Prefix(expr) => { + overflowing_uint(interner, &expr.rhs, annotated_type); + if expr.operator == UnaryOp::Minus { + errors.push(TypeCheckError::InvalidUnaryOp { + kind: "annotated_type".to_string(), + span, + }); + } + } + HirExpression::Infix(expr) => { + errors.extend(overflowing_uint(interner, &expr.lhs, annotated_type)); + errors.extend(overflowing_uint(interner, &expr.rhs, annotated_type)); + } + _ => {} + } + + errors +} diff --git a/compiler/noirc_frontend/src/elaborator/mod.rs b/compiler/noirc_frontend/src/elaborator/mod.rs index 594c0690033..24902c395b8 100644 --- a/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/compiler/noirc_frontend/src/elaborator/mod.rs @@ -30,15 +30,12 @@ use crate::{ hir::{ def_collector::{dc_crate::CollectedItems, errors::DefCollectorErrorKind}, def_map::{LocalModuleId, ModuleDefId, ModuleId, MAIN_FUNCTION}, - resolution::{ - errors::PubPosition, import::PathResolution, path_resolver::StandardPathResolver, - }, + resolution::{import::PathResolution, path_resolver::StandardPathResolver}, Context, }, hir_def::function::{FuncMeta, HirFunction}, - macros_api::{Param, Path, UnresolvedType, UnresolvedTypeData, Visibility}, + macros_api::{Param, Path, UnresolvedType, UnresolvedTypeData}, node_interner::TraitImplId, - token::FunctionAttribute, Generics, }; use crate::{ @@ -51,6 +48,7 @@ use crate::{ }; mod expressions; +mod lints; mod patterns; mod scope; mod statements; @@ -295,7 +293,7 @@ impl<'context> Elaborator<'context> { .expect("FuncMetas should be declared before a function is elaborated") .clone(); - // The DefinitionIds for each parameter were already created in define_function_meta + // The `DefinitionId`s for each parameter were already created in `define_function_meta` // so we need to reintroduce the same IDs into scope here. for parameter in &func_meta.parameter_idents { let name = self.interner.definition_name(parameter.id).to_owned(); @@ -429,6 +427,12 @@ impl<'context> Elaborator<'context> { self.errors.push((error.into(), self.file)); } + fn run_lint(&mut self, lint: impl Fn(&Elaborator) -> Option) { + if let Some(error) = lint(self) { + self.push_err(error); + } + } + fn resolve_where_clause(&mut self, clause: &mut [UnresolvedTraitConstraint]) { for bound in clause { if let Some(trait_id) = self.resolve_trait_by_path(bound.trait_bound.trait_path.clone()) @@ -510,6 +514,8 @@ impl<'context> Elaborator<'context> { ) { self.current_function = Some(func_id); self.resolve_where_clause(&mut func.def.where_clause); + // `func` is no longer mutated. + let func = &*func; // Without this, impl methods can accidentally be placed in contracts. See #3254 if self.self_type.is_some() { @@ -523,26 +529,28 @@ impl<'context> Elaborator<'context> { let id = self.interner.function_definition_id(func_id); let name_ident = HirIdent::non_trait_method(id, location); - let attributes = func.attributes().clone(); - let has_no_predicates_attribute = attributes.is_no_predicates(); - let should_fold = attributes.is_foldable(); - if !self.inline_attribute_allowed(func) { - if has_no_predicates_attribute { - self.push_err(ResolverError::NoPredicatesAttributeOnUnconstrained { - ident: func.name_ident().clone(), - }); - } else if should_fold { - self.push_err(ResolverError::FoldAttributeOnUnconstrained { - ident: func.name_ident().clone(), - }); - } - } + let is_entry_point = self.is_entry_point_function(func); + + self.run_lint(|_| lints::inlining_attributes(func).map(Into::into)); + self.run_lint(|_| lints::missing_pub(func, is_entry_point).map(Into::into)); + self.run_lint(|elaborator| { + lints::unnecessary_pub_return(func, elaborator.pub_allowed(func)).map(Into::into) + }); + self.run_lint(|elaborator| { + lints::low_level_function_outside_stdlib(func, elaborator.crate_id).map(Into::into) + }); + self.run_lint(|_| lints::test_function_with_args(func).map(Into::into)); + self.run_lint(|_| { + lints::recursive_non_entrypoint_function(func, is_entry_point).map(Into::into) + }); + // Both the #[fold] and #[no_predicates] alter a function's inline type and code generation in similar ways. // In certain cases such as type checking (for which the following flag will be used) both attributes // indicate we should code generate in the same way. Thus, we unify the attributes into one flag here. + let has_no_predicates_attribute = func.attributes().is_no_predicates(); + let should_fold = func.attributes().is_foldable(); let has_inline_attribute = has_no_predicates_attribute || should_fold; - let is_entry_point = self.is_entry_point_function(func); - + let is_pub_allowed = self.pub_allowed(func); self.add_generics(&func.def.generics); let mut trait_constraints = self.resolve_trait_constraints(&func.def.where_clause); @@ -553,12 +561,9 @@ impl<'context> Elaborator<'context> { let mut parameter_idents = Vec::new(); for Param { visibility, pattern, typ, span: _ } in func.parameters().iter().cloned() { - if visibility == Visibility::Public && !self.pub_allowed(func) { - self.push_err(ResolverError::UnnecessaryPub { - ident: func.name_ident().clone(), - position: PubPosition::Parameter, - }); - } + self.run_lint(|_| { + lints::unnecessary_pub_argument(func, visibility, is_pub_allowed).map(Into::into) + }); let type_span = typ.span.unwrap_or_else(|| pattern.span()); @@ -588,44 +593,6 @@ impl<'context> Elaborator<'context> { let return_type = Box::new(self.resolve_type(func.return_type())); - if !self.pub_allowed(func) && func.def.return_visibility == Visibility::Public { - self.push_err(ResolverError::UnnecessaryPub { - ident: func.name_ident().clone(), - position: PubPosition::ReturnType, - }); - } - - let is_low_level_function = - attributes.function.as_ref().map_or(false, |func| func.is_low_level()); - - if !self.crate_id.is_stdlib() && is_low_level_function { - let error = - ResolverError::LowLevelFunctionOutsideOfStdlib { ident: func.name_ident().clone() }; - self.push_err(error); - } - - // 'pub' is required on return types for entry point functions - if is_entry_point - && return_type.as_ref() != &Type::Unit - && func.def.return_visibility == Visibility::Private - { - self.push_err(ResolverError::NecessaryPub { ident: func.name_ident().clone() }); - } - // '#[recursive]' attribute is only allowed for entry point functions - if !is_entry_point && func.kind == FunctionKind::Recursive { - self.push_err(ResolverError::MisplacedRecursiveAttribute { - ident: func.name_ident().clone(), - }); - } - - if matches!(attributes.function, Some(FunctionAttribute::Test { .. })) - && !parameters.is_empty() - { - self.push_err(ResolverError::TestFunctionHasParameters { - span: func.name_ident().span(), - }); - } - let mut typ = Type::Function(parameter_types, return_type, Box::new(Type::Unit)); if !generics.is_empty() { @@ -682,14 +649,8 @@ impl<'context> Elaborator<'context> { } } - fn inline_attribute_allowed(&self, func: &NoirFunction) -> bool { - // Inline attributes are only relevant for constrained functions - // as all unconstrained functions are not inlined - !func.def.is_unconstrained - } - - /// True if the 'pub' keyword is allowed on parameters in this function - /// 'pub' on function parameters is only allowed for entry point functions + /// True if the `pub` keyword is allowed on parameters in this function + /// `pub` on function parameters is only allowed for entry point functions fn pub_allowed(&self, func: &NoirFunction) -> bool { self.is_entry_point_function(func) || func.attributes().is_foldable() } diff --git a/compiler/noirc_frontend/src/elaborator/statements.rs b/compiler/noirc_frontend/src/elaborator/statements.rs index 40de9a3983a..8f2f4d3911a 100644 --- a/compiler/noirc_frontend/src/elaborator/statements.rs +++ b/compiler/noirc_frontend/src/elaborator/statements.rs @@ -19,7 +19,7 @@ use crate::{ Type, }; -use super::Elaborator; +use super::{lints, Elaborator}; impl<'context> Elaborator<'context> { fn elaborate_statement_value(&mut self, statement: Statement) -> (HirStatement, Type) { @@ -98,7 +98,10 @@ impl<'context> Elaborator<'context> { } }); if annotated_type.is_unsigned() { - self.lint_overflowing_uint(&expression, &annotated_type); + let errors = lints::overflowing_uint(self.interner, &expression, &annotated_type); + for error in errors { + self.push_err(error); + } } annotated_type } else { diff --git a/compiler/noirc_frontend/src/elaborator/types.rs b/compiler/noirc_frontend/src/elaborator/types.rs index 955b4af327a..3d677d8740d 100644 --- a/compiler/noirc_frontend/src/elaborator/types.rs +++ b/compiler/noirc_frontend/src/elaborator/types.rs @@ -33,7 +33,7 @@ use crate::{ Generics, Type, TypeBinding, TypeVariable, TypeVariableKind, }; -use super::Elaborator; +use super::{lints, Elaborator}; impl<'context> Elaborator<'context> { /// Translates an UnresolvedType to a Type @@ -502,44 +502,6 @@ impl<'context> Elaborator<'context> { } } - /// Check if an assignment is overflowing with respect to `annotated_type` - /// in a declaration statement where `annotated_type` is an unsigned integer - pub(super) fn lint_overflowing_uint(&mut self, rhs_expr: &ExprId, annotated_type: &Type) { - let expr = self.interner.expression(rhs_expr); - let span = self.interner.expr_span(rhs_expr); - match expr { - HirExpression::Literal(HirLiteral::Integer(value, false)) => { - let v = value.to_u128(); - if let Type::Integer(_, bit_count) = annotated_type { - let bit_count: u32 = (*bit_count).into(); - let max = 1 << bit_count; - if v >= max { - self.push_err(TypeCheckError::OverflowingAssignment { - expr: value, - ty: annotated_type.clone(), - range: format!("0..={}", max - 1), - span, - }); - }; - }; - } - HirExpression::Prefix(expr) => { - self.lint_overflowing_uint(&expr.rhs, annotated_type); - if matches!(expr.operator, UnaryOp::Minus) { - self.push_err(TypeCheckError::InvalidUnaryOp { - kind: "annotated_type".to_string(), - span, - }); - } - } - HirExpression::Infix(expr) => { - self.lint_overflowing_uint(&expr.lhs, annotated_type); - self.lint_overflowing_uint(&expr.rhs, annotated_type); - } - _ => {} - } - } - pub(super) fn unify( &mut self, actual: &Type, @@ -1182,57 +1144,33 @@ impl<'context> Elaborator<'context> { args: Vec<(Type, ExprId, Span)>, span: Span, ) -> Type { - // Need to setup these flags here as `self` is borrowed mutably to type check the rest of the call expression - // These flags are later used to type check calls to unconstrained functions from constrained functions + self.run_lint(|elaborator| { + lints::deprecated_function(elaborator.interner, call.func).map(Into::into) + }); + let func_mod = self.current_function.map(|func| self.interner.function_modifiers(&func)); let is_current_func_constrained = func_mod.map_or(true, |func_mod| !func_mod.is_unconstrained); - let is_unconstrained_call = self.is_unconstrained_call(call.func); - self.check_if_deprecated(call.func); - - // Check that we are not passing a mutable reference from a constrained runtime to an unconstrained runtime - if is_current_func_constrained && is_unconstrained_call { - for (typ, _, _) in args.iter() { - if matches!(&typ.follow_bindings(), Type::MutableReference(_)) { - self.push_err(TypeCheckError::ConstrainedReferenceToUnconstrained { span }); - } + let crossing_runtime_boundary = is_current_func_constrained && is_unconstrained_call; + if crossing_runtime_boundary { + let errors = lints::unconstrained_function_args(&args); + for error in errors { + self.push_err(error); } } let return_type = self.bind_function_type(func_type, args, span); - // Check that we are not passing a slice from an unconstrained runtime to a constrained runtime - if is_current_func_constrained && is_unconstrained_call { - if return_type.contains_slice() { - self.push_err(TypeCheckError::UnconstrainedSliceReturnToConstrained { span }); - } else if matches!(&return_type.follow_bindings(), Type::MutableReference(_)) { - self.push_err(TypeCheckError::UnconstrainedReferenceToConstrained { span }); - } - }; + if crossing_runtime_boundary { + self.run_lint(|_| { + lints::unconstrained_function_return(&return_type, span).map(Into::into) + }); + } return_type } - fn check_if_deprecated(&mut self, expr: ExprId) { - if let HirExpression::Ident(HirIdent { location, id, impl_kind: _ }, _) = - self.interner.expression(&expr) - { - if let Some(DefinitionKind::Function(func_id)) = - self.interner.try_definition(id).map(|def| &def.kind) - { - let attributes = self.interner.function_attributes(func_id); - if let Some(note) = attributes.get_deprecated_note() { - self.push_err(TypeCheckError::CallDeprecated { - name: self.interner.definition_name(id).to_string(), - note, - span: location.span, - }); - } - } - } - } - fn is_unconstrained_call(&self, expr: ExprId) -> bool { if let HirExpression::Ident(HirIdent { id, .. }, _) = self.interner.expression(&expr) { if let Some(DefinitionKind::Function(func_id)) = diff --git a/cspell.json b/cspell.json index 4497d0bf9da..a71602ac77a 100644 --- a/cspell.json +++ b/cspell.json @@ -57,6 +57,7 @@ "curvegroup", "databus", "debouncer", + "defaultable", "deflater", "deflatten", "deflattened",