From 56c931a8e59e9e67e8bd5aae4a114e85fe40ff98 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 9 Jan 2025 15:56:42 -0300 Subject: [PATCH] feat!: require trait primitive functions/calls to have their trait in scope (#6901) Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com> --- .../noirc_frontend/src/elaborator/types.rs | 77 ++++++------- compiler/noirc_frontend/src/hir_def/types.rs | 2 +- compiler/noirc_frontend/src/node_interner.rs | 106 ++++++------------ compiler/noirc_frontend/src/tests/traits.rs | 65 +++++++++++ noir_stdlib/src/meta/ctstring.nr | 2 + noir_stdlib/src/meta/mod.nr | 1 + noir_stdlib/src/meta/op.nr | 2 + .../src/main.nr | 2 + .../comptime_trait_constraint/src/main.nr | 2 +- .../ctstring/src/main.nr | 2 + .../regression_5428/src/main.nr | 2 + 11 files changed, 144 insertions(+), 119 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/types.rs b/compiler/noirc_frontend/src/elaborator/types.rs index 98b08266f67..9b3df4631b4 100644 --- a/compiler/noirc_frontend/src/elaborator/types.rs +++ b/compiler/noirc_frontend/src/elaborator/types.rs @@ -34,8 +34,7 @@ use crate::{ TraitImplKind, TraitMethodId, }, token::SecondaryAttribute, - Generics, Kind, ResolvedGeneric, Shared, StructType, Type, TypeBinding, TypeBindings, - UnificationError, + Generics, Kind, ResolvedGeneric, Type, TypeBinding, TypeBindings, UnificationError, }; use super::{lints, path_resolution::PathResolutionItem, Elaborator, UnsafeBlockStatus}; @@ -1320,9 +1319,6 @@ impl<'context> Elaborator<'context> { has_self_arg: bool, ) -> Option { match object_type.follow_bindings() { - Type::Struct(typ, _args) => { - self.lookup_struct_method(object_type, method_name, span, has_self_arg, typ) - } // TODO: We should allow method calls on `impl Trait`s eventually. // For now it is fine since they are only allowed on return types. Type::TraitAsType(..) => { @@ -1338,11 +1334,9 @@ impl<'context> Elaborator<'context> { } // Mutable references to another type should resolve to methods of their element type. // This may be a struct or a primitive type. - Type::MutableReference(element) => self - .interner - .lookup_primitive_trait_method_mut(element.as_ref(), method_name, has_self_arg) - .map(HirMethodReference::FuncId) - .or_else(|| self.lookup_method(&element, method_name, span, has_self_arg)), + Type::MutableReference(element) => { + self.lookup_method(&element, method_name, span, has_self_arg) + } // If we fail to resolve the object to a struct type, we have no way of type // checking its arguments as we can't even resolve the name of the function @@ -1354,39 +1348,29 @@ impl<'context> Elaborator<'context> { None } - other => match self.interner.lookup_primitive_method(&other, method_name, has_self_arg) - { - Some(method_id) => Some(HirMethodReference::FuncId(method_id)), - None => { - // It could be that this type is a composite type that is bound to a trait, - // for example `x: (T, U) ... where (T, U): SomeTrait` - // (so this case is a generalization of the NamedGeneric case) - self.lookup_method_in_trait_constraints(object_type, method_name, span) - } - }, + other => { + self.lookup_struct_or_primitive_method(&other, method_name, span, has_self_arg) + } } } - fn lookup_struct_method( + fn lookup_struct_or_primitive_method( &mut self, object_type: &Type, method_name: &str, span: Span, has_self_arg: bool, - typ: Shared, ) -> Option { - let id = typ.borrow().id; - // First search in the struct methods if let Some(method_id) = - self.interner.lookup_direct_method(object_type, id, method_name, has_self_arg) + self.interner.lookup_direct_method(object_type, method_name, has_self_arg) { return Some(HirMethodReference::FuncId(method_id)); } // Next lookup all matching trait methods. let trait_methods = - self.interner.lookup_trait_methods(object_type, id, method_name, has_self_arg); + self.interner.lookup_trait_methods(object_type, method_name, has_self_arg); if trait_methods.is_empty() { // If we couldn't find any trait methods, search in @@ -1397,26 +1381,31 @@ impl<'context> Elaborator<'context> { return Some(HirMethodReference::FuncId(func_id)); } - // Otherwise it's an error - let has_field_with_function_type = typ - .borrow() - .get_fields_as_written() - .into_iter() - .any(|field| field.name.0.contents == method_name && field.typ.is_function()); - if has_field_with_function_type { - self.push_err(TypeCheckError::CannotInvokeStructFieldFunctionType { - method_name: method_name.to_string(), - object_type: object_type.clone(), - span, - }); + if let Type::Struct(struct_type, _) = object_type { + let has_field_with_function_type = + struct_type.borrow().get_fields_as_written().into_iter().any(|field| { + field.name.0.contents == method_name && field.typ.is_function() + }); + if has_field_with_function_type { + self.push_err(TypeCheckError::CannotInvokeStructFieldFunctionType { + method_name: method_name.to_string(), + object_type: object_type.clone(), + span, + }); + } else { + self.push_err(TypeCheckError::UnresolvedMethodCall { + method_name: method_name.to_string(), + object_type: object_type.clone(), + span, + }); + } + return None; } else { - self.push_err(TypeCheckError::UnresolvedMethodCall { - method_name: method_name.to_string(), - object_type: object_type.clone(), - span, - }); + // It could be that this type is a composite type that is bound to a trait, + // for example `x: (T, U) ... where (T, U): SomeTrait` + // (so this case is a generalization of the NamedGeneric case) + return self.lookup_method_in_trait_constraints(object_type, method_name, span); } - return None; } // We found some trait methods... but is only one of them currently in scope? diff --git a/compiler/noirc_frontend/src/hir_def/types.rs b/compiler/noirc_frontend/src/hir_def/types.rs index 0ed6399296b..c0dbf6f9500 100644 --- a/compiler/noirc_frontend/src/hir_def/types.rs +++ b/compiler/noirc_frontend/src/hir_def/types.rs @@ -1862,7 +1862,7 @@ impl Type { if let (Type::Array(_size, element1), Type::Slice(element2)) = (&this, &target) { // We can only do the coercion if the `as_slice` method exists. // This is usually true, but some tests don't have access to the standard library. - if let Some(as_slice) = interner.lookup_primitive_method(&this, "as_slice", true) { + if let Some(as_slice) = interner.lookup_direct_method(&this, "as_slice", true) { // Still have to ensure the element types match. // Don't need to issue an error here if not, it will be done in unify_with_coercions let mut bindings = TypeBindings::new(); diff --git a/compiler/noirc_frontend/src/node_interner.rs b/compiler/noirc_frontend/src/node_interner.rs index 599558bb91a..c1a405535f2 100644 --- a/compiler/noirc_frontend/src/node_interner.rs +++ b/compiler/noirc_frontend/src/node_interner.rs @@ -183,16 +183,13 @@ pub struct NodeInterner { next_type_variable_id: std::cell::Cell, - /// A map from a struct type and method name to a function id for the method. + /// A map from a type and method name to a function id for the method. /// This can resolve to potentially multiple methods if the same method name is /// specialized for different generics on the same type. E.g. for `Struct`, we /// may have both `impl Struct { fn foo(){} }` and `impl Struct { fn foo(){} }`. /// If this happens, the returned Vec will have 2 entries and we'll need to further /// disambiguate them by checking the type of each function. - struct_methods: HashMap>, - - /// Methods on primitive types defined in the stdlib. - primitive_methods: HashMap>, + methods: HashMap>, // For trait implementation functions, this is their self type and trait they belong to func_id_to_trait: HashMap, @@ -666,8 +663,7 @@ impl Default for NodeInterner { next_type_variable_id: std::cell::Cell::new(0), globals: Vec::new(), global_attributes: HashMap::default(), - struct_methods: HashMap::default(), - primitive_methods: HashMap::default(), + methods: HashMap::default(), type_alias_ref: Vec::new(), type_ref_locations: Vec::new(), quoted_types: Default::default(), @@ -1214,27 +1210,8 @@ impl NodeInterner { self.structs[&id].clone() } - pub fn get_struct_methods(&self, id: StructId) -> Option<&HashMap> { - self.struct_methods.get(&id) - } - - fn get_primitive_methods(&self, key: TypeMethodKey) -> Option<&HashMap> { - self.primitive_methods.get(&key) - } - pub fn get_type_methods(&self, typ: &Type) -> Option<&HashMap> { - match typ { - Type::Struct(struct_type, _) => { - let struct_type = struct_type.borrow(); - self.get_struct_methods(struct_type.id) - } - Type::Alias(type_alias, generics) => { - let type_alias = type_alias.borrow(); - let typ = type_alias.get_type(generics); - self.get_type_methods(&typ) - } - _ => get_type_method_key(typ).and_then(|key| self.get_primitive_methods(key)), - } + get_type_method_key(typ).and_then(|key| self.methods.get(&key)) } pub fn get_trait(&self, id: TraitId) -> &Trait { @@ -1394,39 +1371,27 @@ impl NodeInterner { trait_id: Option, ) -> Option { match self_type { - Type::Struct(struct_type, _generics) => { - let id = struct_type.borrow().id; - - if trait_id.is_none() { - if let Some(existing) = - self.lookup_direct_method(self_type, id, &method_name, true) - { - return Some(existing); - } - } - - self.struct_methods - .entry(id) - .or_default() - .entry(method_name) - .or_default() - .add_method(method_id, None, trait_id); - None - } Type::Error => None, Type::MutableReference(element) => { self.add_method(element, method_name, method_id, trait_id) } - - other => { + _ => { let key = get_type_method_key(self_type).unwrap_or_else(|| { - unreachable!("Cannot add a method to the unsupported type '{}'", other) + unreachable!("Cannot add a method to the unsupported type '{}'", self_type) }); + + if trait_id.is_none() && matches!(self_type, Type::Struct(..)) { + if let Some(existing) = self.lookup_direct_method(self_type, &method_name, true) + { + return Some(existing); + } + } + // Only remember the actual type if it's FieldOrInt, // so later we can disambiguate on calls like `u32::call`. let typ = if key == TypeMethodKey::FieldOrInt { Some(self_type.clone()) } else { None }; - self.primitive_methods + self.methods .entry(key) .or_default() .entry(method_name) @@ -1785,12 +1750,13 @@ impl NodeInterner { pub fn lookup_direct_method( &self, typ: &Type, - id: StructId, method_name: &str, has_self_arg: bool, ) -> Option { - self.struct_methods - .get(&id) + let key = get_type_method_key(typ)?; + + self.methods + .get(&key) .and_then(|h| h.get(method_name)) .and_then(|methods| methods.find_direct_method(typ, has_self_arg, self)) } @@ -1799,15 +1765,19 @@ impl NodeInterner { pub fn lookup_trait_methods( &self, typ: &Type, - id: StructId, method_name: &str, has_self_arg: bool, ) -> Vec<(FuncId, TraitId)> { - self.struct_methods - .get(&id) - .and_then(|h| h.get(method_name)) - .map(|methods| methods.find_trait_methods(typ, has_self_arg, self)) - .unwrap_or_default() + let key = get_type_method_key(typ); + if let Some(key) = key { + self.methods + .get(&key) + .and_then(|h| h.get(method_name)) + .map(|methods| methods.find_trait_methods(typ, has_self_arg, self)) + .unwrap_or_default() + } else { + Vec::new() + } } /// Select the 1 matching method with an object type matching `typ` @@ -1833,8 +1803,7 @@ impl NodeInterner { method_name: &str, has_self_arg: bool, ) -> Option { - let global_methods = - self.primitive_methods.get(&TypeMethodKey::Generic)?.get(method_name)?; + let global_methods = self.methods.get(&TypeMethodKey::Generic)?.get(method_name)?; global_methods.find_matching_method(typ, has_self_arg, self) } @@ -1846,20 +1815,10 @@ impl NodeInterner { has_self_arg: bool, ) -> Option { let key = get_type_method_key(typ)?; - let methods = self.primitive_methods.get(&key)?.get(method_name)?; + let methods = self.methods.get(&key)?.get(method_name)?; self.find_matching_method(typ, Some(methods), method_name, has_self_arg) } - pub fn lookup_primitive_trait_method_mut( - &self, - typ: &Type, - method_name: &str, - has_self_arg: bool, - ) -> Option { - let typ = Type::MutableReference(Box::new(typ.clone())); - self.lookup_primitive_method(&typ, method_name, has_self_arg) - } - /// Returns what the next trait impl id is expected to be. pub fn next_trait_impl_id(&mut self) -> TraitImplId { let next_id = self.next_trait_implementation_id; @@ -2462,6 +2421,7 @@ enum TypeMethodKey { Function, Generic, Quoted(QuotedType), + Struct(StructId), } fn get_type_method_key(typ: &Type) -> Option { @@ -2489,12 +2449,12 @@ fn get_type_method_key(typ: &Type) -> Option { Type::Quoted(quoted) => Some(Quoted(*quoted)), Type::MutableReference(element) => get_type_method_key(element), Type::Alias(alias, _) => get_type_method_key(&alias.borrow().typ), + Type::Struct(struct_type, _) => Some(Struct(struct_type.borrow().id)), // We do not support adding methods to these types Type::Forall(_, _) | Type::Constant(..) | Type::Error - | Type::Struct(_, _) | Type::InfixExpr(..) | Type::CheckedCast { .. } | Type::TraitAsType(..) => None, diff --git a/compiler/noirc_frontend/src/tests/traits.rs b/compiler/noirc_frontend/src/tests/traits.rs index faacd9aeee3..f5730451311 100644 --- a/compiler/noirc_frontend/src/tests/traits.rs +++ b/compiler/noirc_frontend/src/tests/traits.rs @@ -1101,3 +1101,68 @@ fn type_checks_trait_default_method_and_does_not_error_using_self() { "#; assert_no_errors(src); } + +#[test] +fn warns_if_trait_is_not_in_scope_for_primitive_function_call_and_there_is_only_one_trait_method() { + let src = r#" + fn main() { + let _ = Field::foo(); + } + + mod private_mod { + pub trait Foo { + fn foo() -> i32; + } + + impl Foo for Field { + fn foo() -> i32 { + 42 + } + } + } + "#; + let errors = get_program_errors(src); + assert_eq!(errors.len(), 1); + + let CompilationError::ResolverError(ResolverError::PathResolutionError( + PathResolutionError::TraitMethodNotInScope { ident, trait_name }, + )) = &errors[0].0 + else { + panic!("Expected a 'trait method not in scope' error"); + }; + assert_eq!(ident.to_string(), "foo"); + assert_eq!(trait_name, "private_mod::Foo"); +} + +#[test] +fn warns_if_trait_is_not_in_scope_for_primitive_method_call_and_there_is_only_one_trait_method() { + let src = r#" + fn main() { + let x: Field = 1; + let _ = x.foo(); + } + + mod private_mod { + pub trait Foo { + fn foo(self) -> i32; + } + + impl Foo for Field { + fn foo(self) -> i32 { + self as i32 + } + } + } + "#; + let errors = get_program_errors(src); + assert_eq!(errors.len(), 1); + + let CompilationError::ResolverError(ResolverError::PathResolutionError( + PathResolutionError::TraitMethodNotInScope { ident, trait_name }, + )) = &errors[0].0 + else { + panic!("Expected a 'trait method not in scope' error"); + }; + assert_eq!(ident.to_string(), "foo"); + assert_eq!(trait_name, "private_mod::Foo"); +} diff --git a/noir_stdlib/src/meta/ctstring.nr b/noir_stdlib/src/meta/ctstring.nr index b414b3418d9..e23567ece7d 100644 --- a/noir_stdlib/src/meta/ctstring.nr +++ b/noir_stdlib/src/meta/ctstring.nr @@ -86,6 +86,8 @@ comptime fn ctstring_eq(_first: CtString, _second: CtString) -> bool {} comptime fn ctstring_hash(_string: CtString) -> Field {} mod test { + use super::AsCtString; + #[test] fn as_quoted_str_example() { comptime { diff --git a/noir_stdlib/src/meta/mod.nr b/noir_stdlib/src/meta/mod.nr index 4edda9a3120..f6a1f4838ee 100644 --- a/noir_stdlib/src/meta/mod.nr +++ b/noir_stdlib/src/meta/mod.nr @@ -117,6 +117,7 @@ pub comptime fn make_trait_impl( } mod tests { + use crate::meta::ctstring::AsCtString; use crate::meta::derive_via; // docs:start:quote-example diff --git a/noir_stdlib/src/meta/op.nr b/noir_stdlib/src/meta/op.nr index afe230429f8..67c9b4cdb64 100644 --- a/noir_stdlib/src/meta/op.nr +++ b/noir_stdlib/src/meta/op.nr @@ -1,3 +1,5 @@ +use crate::hash::Hash; + pub struct UnaryOp { op: Field, } diff --git a/test_programs/compile_success_empty/comptime_change_type_each_iteration/src/main.nr b/test_programs/compile_success_empty/comptime_change_type_each_iteration/src/main.nr index 976987fec9e..153eb308911 100644 --- a/test_programs/compile_success_empty/comptime_change_type_each_iteration/src/main.nr +++ b/test_programs/compile_success_empty/comptime_change_type_each_iteration/src/main.nr @@ -1,3 +1,5 @@ +use std::meta::ctstring::AsCtString; + fn main() { comptime { for i in 9..11 { diff --git a/test_programs/compile_success_empty/comptime_trait_constraint/src/main.nr b/test_programs/compile_success_empty/comptime_trait_constraint/src/main.nr index 2f24675e8c7..39ac1b5cde1 100644 --- a/test_programs/compile_success_empty/comptime_trait_constraint/src/main.nr +++ b/test_programs/compile_success_empty/comptime_trait_constraint/src/main.nr @@ -1,4 +1,4 @@ -use std::hash::Hasher; +use std::hash::{Hash, Hasher}; trait TraitWithGenerics { fn foo(self) -> (A, B); diff --git a/test_programs/compile_success_empty/ctstring/src/main.nr b/test_programs/compile_success_empty/ctstring/src/main.nr index 61cd848a436..818ae7f94e8 100644 --- a/test_programs/compile_success_empty/ctstring/src/main.nr +++ b/test_programs/compile_success_empty/ctstring/src/main.nr @@ -1,3 +1,5 @@ +use std::meta::ctstring::AsCtString; + fn main() { comptime { let msg1 = "msg1"; diff --git a/test_programs/compile_success_empty/regression_5428/src/main.nr b/test_programs/compile_success_empty/regression_5428/src/main.nr index f01b89cbea4..e9e6c8a5dc8 100644 --- a/test_programs/compile_success_empty/regression_5428/src/main.nr +++ b/test_programs/compile_success_empty/regression_5428/src/main.nr @@ -1,3 +1,5 @@ +use std::append::Append; + fn main() { assert_true!(); }