From 69e901f8f4e8c88c9964d832f331f881b70d2d35 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 24 Feb 2025 15:39:18 -0300 Subject: [PATCH] fix: issue duplicate error on impl function without self (#7490) Co-authored-by: Tom French --- .../src/elaborator/expressions.rs | 3 +- .../noirc_frontend/src/elaborator/patterns.rs | 5 +- .../noirc_frontend/src/elaborator/types.rs | 21 +++--- .../src/hir/comptime/interpreter.rs | 3 +- compiler/noirc_frontend/src/node_interner.rs | 72 ++++++++++--------- compiler/noirc_frontend/src/tests.rs | 21 ++++++ 6 files changed, 79 insertions(+), 46 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/expressions.rs b/compiler/noirc_frontend/src/elaborator/expressions.rs index 7954dc135b3..8f49c47a058 100644 --- a/compiler/noirc_frontend/src/elaborator/expressions.rs +++ b/compiler/noirc_frontend/src/elaborator/expressions.rs @@ -515,7 +515,8 @@ impl<'context> Elaborator<'context> { let method_name_location = method_call.method_name.location(); let method_name = method_call.method_name.0.contents.as_str(); - match self.lookup_method(&object_type, method_name, location, true) { + let check_self_param = true; + match self.lookup_method(&object_type, method_name, location, check_self_param) { Some(method_ref) => { // Automatically add `&mut` if the method expects a mutable reference and // the object is not already one. diff --git a/compiler/noirc_frontend/src/elaborator/patterns.rs b/compiler/noirc_frontend/src/elaborator/patterns.rs index 11529764b6e..1bb7889a9a1 100644 --- a/compiler/noirc_frontend/src/elaborator/patterns.rs +++ b/compiler/noirc_frontend/src/elaborator/patterns.rs @@ -885,8 +885,11 @@ impl<'context> Elaborator<'context> { pub(super) fn elaborate_type_path(&mut self, path: TypePath) -> (ExprId, Type) { let location = path.item.location(); let typ = self.resolve_type(path.typ); + let check_self_param = false; - let Some(method) = self.lookup_method(&typ, &path.item.0.contents, location, false) else { + let Some(method) = + self.lookup_method(&typ, &path.item.0.contents, location, check_self_param) + else { let error = Expression::new(ExpressionKind::Error, location); return self.elaborate_expression(error); }; diff --git a/compiler/noirc_frontend/src/elaborator/types.rs b/compiler/noirc_frontend/src/elaborator/types.rs index bdb434b8f3e..1bcc7b3d47c 100644 --- a/compiler/noirc_frontend/src/elaborator/types.rs +++ b/compiler/noirc_frontend/src/elaborator/types.rs @@ -1478,7 +1478,7 @@ impl<'context> Elaborator<'context> { object_type: &Type, method_name: &str, location: Location, - has_self_arg: bool, + check_self_param: bool, ) -> Option { let span = location.span; let file = location.file; @@ -1502,7 +1502,7 @@ impl<'context> Elaborator<'context> { // Mutable references to another type should resolve to methods of their element type. // This may be a data type or a primitive type. Type::MutableReference(element) => { - self.lookup_method(&element, method_name, location, has_self_arg) + self.lookup_method(&element, method_name, location, check_self_param) } // If we fail to resolve the object to a data type, we have no way of type @@ -1515,9 +1515,12 @@ impl<'context> Elaborator<'context> { None } - other => { - self.lookup_type_or_primitive_method(&other, method_name, location, has_self_arg) - } + other => self.lookup_type_or_primitive_method( + &other, + method_name, + location, + check_self_param, + ), } } @@ -1526,21 +1529,21 @@ impl<'context> Elaborator<'context> { object_type: &Type, method_name: &str, location: Location, - has_self_arg: bool, + check_self_param: bool, ) -> Option { let span = location.span; let file = location.file; // First search in the type methods. If there is one, that's the one. if let Some(method_id) = - self.interner.lookup_direct_method(object_type, method_name, has_self_arg) + self.interner.lookup_direct_method(object_type, method_name, check_self_param) { return Some(HirMethodReference::FuncId(method_id)); } // Next lookup all matching trait methods. let trait_methods = - self.interner.lookup_trait_methods(object_type, method_name, has_self_arg); + self.interner.lookup_trait_methods(object_type, method_name, check_self_param); // If there's at least one matching trait method we need to see if only one is in scope. if !trait_methods.is_empty() { @@ -1550,7 +1553,7 @@ impl<'context> Elaborator<'context> { // If we couldn't find any trait methods, search in // impls for all types `T`, e.g. `impl Foo for T` let generic_methods = - self.interner.lookup_generic_methods(object_type, method_name, has_self_arg); + self.interner.lookup_generic_methods(object_type, method_name, check_self_param); if !generic_methods.is_empty() { return self.return_trait_method_in_scope(&generic_methods, method_name, location); } diff --git a/compiler/noirc_frontend/src/hir/comptime/interpreter.rs b/compiler/noirc_frontend/src/hir/comptime/interpreter.rs index 67f170c1600..12db16764be 100644 --- a/compiler/noirc_frontend/src/hir/comptime/interpreter.rs +++ b/compiler/noirc_frontend/src/hir/comptime/interpreter.rs @@ -1406,10 +1406,11 @@ impl<'local, 'interner> Interpreter<'local, 'interner> { let typ = object.get_type().follow_bindings(); let method_name = &call.method.0.contents; + let check_self_param = true; let method = self .elaborator - .lookup_method(&typ, method_name, location, true) + .lookup_method(&typ, method_name, location, check_self_param) .and_then(|method| method.func_id(self.elaborator.interner)); if let Some(method) = method { diff --git a/compiler/noirc_frontend/src/node_interner.rs b/compiler/noirc_frontend/src/node_interner.rs index 84c234bfac3..a3d2441d93b 100644 --- a/compiler/noirc_frontend/src/node_interner.rs +++ b/compiler/noirc_frontend/src/node_interner.rs @@ -359,16 +359,19 @@ pub enum ImplSearchErrorKind { /// as long as these specialized impls do not overlap. E.g. `impl Struct` and `impl Struct` #[derive(Default, Debug, Clone)] pub struct Methods { - pub direct: Vec, + pub direct: Vec, pub trait_impl_methods: Vec, } +#[derive(Debug, Clone)] +pub struct ImplMethod { + pub typ: Type, + pub method: FuncId, +} + #[derive(Debug, Clone)] pub struct TraitImplMethod { - // This type is only stored for primitive types to be able to - // select the correct static methods between multiple options keyed - // under TypeMethodKey::FieldOrInt - pub typ: Option, + pub typ: Type, pub method: FuncId, pub trait_id: TraitId, } @@ -1402,7 +1405,9 @@ impl NodeInterner { }); if trait_id.is_none() && matches!(self_type, Type::DataType(..)) { - if let Some(existing) = self.lookup_direct_method(self_type, &method_name, true) + let check_self_param = false; + if let Some(existing) = + self.lookup_direct_method(self_type, &method_name, check_self_param) { return Some(existing); } @@ -1410,8 +1415,7 @@ impl NodeInterner { // 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 }; + let typ = self_type.clone(); self.methods .entry(key) .or_default() @@ -1778,18 +1782,20 @@ impl NodeInterner { } /// Looks up a method that's directly defined in the given type. + /// If `check_self_param` is `true`, only a method that has a `self` parameter with a type + /// that unifies with `typ` will be returned. pub fn lookup_direct_method( &self, typ: &Type, method_name: &str, - has_self_arg: bool, + check_self_param: bool, ) -> Option { 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)) + .and_then(|methods| methods.find_direct_method(typ, check_self_param, self)) } /// Looks up a methods that apply to the given type but are defined in traits. @@ -2311,20 +2317,21 @@ impl NodeInterner { } impl Methods { - fn add_method(&mut self, method: FuncId, typ: Option, trait_id: Option) { + fn add_method(&mut self, method: FuncId, typ: Type, trait_id: Option) { if let Some(trait_id) = trait_id { let trait_impl_method = TraitImplMethod { typ, method, trait_id }; self.trait_impl_methods.push(trait_impl_method); } else { - self.direct.push(method); + let impl_method = ImplMethod { typ, method }; + self.direct.push(impl_method); } } /// Iterate through each method, starting with the direct methods - pub fn iter(&self) -> impl Iterator, Option)> { + pub fn iter(&self) -> impl Iterator)> { let trait_impl_methods = - self.trait_impl_methods.iter().map(|m| (m.method, m.typ.as_ref(), Some(m.trait_id))); - let direct = self.direct.iter().copied().map(|func_id| (func_id, None, None)); + self.trait_impl_methods.iter().map(|m| (m.method, &m.typ, Some(m.trait_id))); + let direct = self.direct.iter().map(|method| (method.method, &method.typ, None)); direct.chain(trait_impl_methods) } @@ -2346,12 +2353,12 @@ impl Methods { pub fn find_direct_method( &self, typ: &Type, - has_self_param: bool, + check_self_param: bool, interner: &NodeInterner, ) -> Option { for method in &self.direct { - if Self::method_matches(typ, has_self_param, *method, None, interner) { - return Some(*method); + if Self::method_matches(typ, check_self_param, method.method, &method.typ, interner) { + return Some(method.method); } } @@ -2368,7 +2375,7 @@ impl Methods { for trait_impl_method in &self.trait_impl_methods { let method = trait_impl_method.method; - let method_type = trait_impl_method.typ.as_ref(); + let method_type = &trait_impl_method.typ; let trait_id = trait_impl_method.trait_id; if Self::method_matches(typ, has_self_param, method, method_type, interner) { @@ -2381,14 +2388,14 @@ impl Methods { fn method_matches( typ: &Type, - has_self_param: bool, + check_self_param: bool, method: FuncId, - method_type: Option<&Type>, + method_type: &Type, interner: &NodeInterner, ) -> bool { match interner.function_meta(&method).typ.instantiate(interner).0 { Type::Function(args, _, _, _) => { - if has_self_param { + if check_self_param { if let Some(object) = args.first() { if object.unify(typ).is_ok() { return true; @@ -2402,21 +2409,18 @@ impl Methods { } } } else { - // If we recorded the concrete type this trait impl method belongs to, - // and it matches typ, it's an exact match and we return that. - if let Some(method_type) = method_type { + // We still need to make sure the method is for the given type + // (this might be false if for example a method for `Struct` was added but + // now we are looking for a method in `Struct`) + if method_type.unify(typ).is_ok() { + return true; + } + + // Handle auto-dereferencing `&mut T` into `T` + if let Type::MutableReference(method_type) = method_type { if method_type.unify(typ).is_ok() { return true; } - - // Handle auto-dereferencing `&mut T` into `T` - if let Type::MutableReference(method_type) = method_type { - if method_type.unify(typ).is_ok() { - return true; - } - } - } else { - return true; } } } diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index a238d7d6bb5..d719e437140 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -4495,6 +4495,27 @@ fn errors_on_repeated_match_variables_in_pattern() { )); } +#[test] +fn check_impl_duplicate_method_without_self() { + let src = " + pub struct Foo {} + + impl Foo { + fn foo() {} + fn foo() {} + } + + fn main() {} + "; + let errors = get_program_errors(src); + assert_eq!(errors.len(), 1); + + assert!(matches!( + errors[0].0, + CompilationError::ResolverError(ResolverError::DuplicateDefinition { .. }) + )); +} + #[test] fn duplicate_field_in_match_struct_pattern() { let src = r#"