fix: issue duplicate error on impl function without self (#7490)

Co-authored-by: Tom French <[email protected]>

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.

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);
         };

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<HirMethodReference> {
         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<HirMethodReference> {
         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<T> 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);
         }

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 {

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<u32>` and `impl Struct<u64>`
 #[derive(Default, Debug, Clone)]
 pub struct Methods {
-    pub direct: Vec<FuncId>,
+    pub direct: Vec<ImplMethod>,
     pub trait_impl_methods: Vec<TraitImplMethod>,
 }
 
+#[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<Type>,
+    pub typ: Type,
     pub method: FuncId,
     pub trait_id: TraitId,
 }
@@ -1402,16 +1405,17 @@ 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);
                     }
                 }
 
                 // 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<FuncId> {
         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<Type>, trait_id: Option<TraitId>) {
+    fn add_method(&mut self, method: FuncId, typ: Type, trait_id: Option<TraitId>) {
         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<Item = (FuncId, Option<&Type>, Option<TraitId>)> {
+    pub fn iter(&self) -> impl Iterator<Item = (FuncId, &Type, Option<TraitId>)> {
         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<FuncId> {
         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<i32>` was added but
+                    // now we are looking for a method in `Struct<i64>`)
+                    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;
                     }
                 }
             }

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#"