fix: allow calling trait impl method from struct if multiple impls exist

compiler/noirc_frontend/src/elaborator/mod.rs

@@ -1412,7 +1412,12 @@ impl<'context> Elaborator<'context> {
             self.collect_trait_impl_methods(trait_id, trait_impl, &where_clause);
 
             let span = trait_impl.object_type.span;
-            self.declare_methods_on_struct(Some(trait_id), &mut trait_impl.methods, span);
+            let trait_impl_id = trait_impl.impl_id.expect("TraitImpl impl ID should be set by now");
+            self.declare_methods_on_struct(
+                Some((trait_impl_id, trait_id)),
+                &mut trait_impl.methods,
+                span,
+            );
 
             let trait_visibility = self.interner.get_trait(trait_id).visibility;
 
@@ -1477,7 +1482,7 @@ impl<'context> Elaborator<'context> {
 
     fn declare_methods_on_struct(
         &mut self,
-        trait_id: Option<TraitId>,
+        trait_impl: Option<(TraitImplId, TraitId)>,
         functions: &mut UnresolvedFunctions,
         span: Span,
     ) {
@@ -1491,7 +1496,7 @@ impl<'context> Elaborator<'context> {
             let struct_ref = struct_type.borrow();
 
             // `impl`s are only allowed on types defined within the current crate
-            if trait_id.is_none() && struct_ref.id.krate() != self.crate_id {
+            if trait_impl.is_none() && struct_ref.id.krate() != self.crate_id {
                 let type_name = struct_ref.name.to_string();
                 self.push_err(DefCollectorErrorKind::ForeignImpl { span, type_name });
                 return;
@@ -1508,34 +1513,31 @@ impl<'context> Elaborator<'context> {
                 // object types in each method overlap or not. If they do, we issue an error.
                 // If not, that is specialization which is allowed.
                 let name = method.name_ident().clone();
-                let result = if let Some(trait_id) = trait_id {
-                    module.declare_trait_function(name, *method_id, trait_id)
+                if let Some((trait_impl_id, trait_id)) = trait_impl {
+                    // We don't check the result here because for trait and trait impl functions
+                    // we already checked if a duplicate definition exists
+                    let _ = module.declare_trait_impl_function(
+                        name,
+                        *method_id,
+                        trait_impl_id,
+                        trait_id,
+                    );
                 } else {
-                    module.declare_function(name, method.def.visibility, *method_id)
+                    // For non-trait methods we'll check if there are duplicates below
+                    let _ =
+                        module.declare_function(name.clone(), method.def.visibility, *method_id);
                 };
-                if result.is_err() {
-                    let existing = module.find_func_with_name(method.name_ident()).expect(
-                        "declare_function should only error if there is an existing function",
-                    );
-
-                    // Only remove the existing function from scope if it is from a trait impl as
-                    // well. If it is from a non-trait impl that should override trait impl methods
-                    // anyway so that Foo::bar always resolves to the non-trait impl version.
-                    if self.interner.function_meta(&existing).trait_impl.is_some() {
-                        module.remove_function(method.name_ident());
-                    }
-                }
             }
 
             // Trait impl methods are already declared in NodeInterner::add_trait_implementation
-            if trait_id.is_none() {
+            if trait_impl.is_none() {
                 self.declare_methods(self_type, &function_ids);
             }
         // We can define methods on primitive types only if we're in the stdlib
-        } else if trait_id.is_none() && *self_type != Type::Error {
+        } else if trait_impl.is_none() && *self_type != Type::Error {
             if self.crate_id.is_stdlib() {
                 // Trait impl methods are already declared in NodeInterner::add_trait_implementation
-                if trait_id.is_none() {
+                if trait_impl.is_none() {
                     self.declare_methods(self_type, &function_ids);
                 }
             } else {

compiler/noirc_frontend/src/elaborator/path_resolution.rs

@@ -407,8 +407,8 @@ impl<'context> Elaborator<'context> {
         // Gather a list of items for which their trait is in scope.
         let mut results = Vec::new();
 
-        for (trait_id, item) in values.iter() {
-            let trait_id = trait_id.expect("The None option was already considered before");
+        for (key, item) in values.iter() {
+            let (_, trait_id) = key.expect("The None option was already considered before");
             let trait_ = self.interner.get_trait(trait_id);
             let Some(map) = starting_module.scope().types().get(&trait_.name) else {
                 continue;
@@ -424,15 +424,15 @@ impl<'context> Elaborator<'context> {
         if results.is_empty() {
             if values.len() == 1 {
                 // This is the backwards-compatible case where there's a single trait method but it's not in scope
-                let (trait_id, item) = values.iter().next().expect("Expected an item");
-                let trait_id = trait_id.expect("The None option was already considered before");
+                let (key, item) = values.iter().next().expect("Expected an item");
+                let (_, trait_id) = key.expect("The None option was already considered before");
                 let per_ns = PerNs { types: None, values: Some(*item) };
                 return StructMethodLookupResult::FoundOneTraitMethodButNotInScope(
                     per_ns, trait_id,
                 );
             } else {
                 let trait_ids = vecmap(values, |(trait_id, _)| {
-                    trait_id.expect("The none option was already considered before")
+                    trait_id.expect("The none option was already considered before").1
                 });
                 return StructMethodLookupResult::NotFound(trait_ids);
             }

compiler/noirc_frontend/src/elaborator/types.rs

@@ -41,6 +41,7 @@ use super::{lints, path_resolution::PathResolutionItem, Elaborator, UnsafeBlockS
 
 pub const SELF_TYPE_NAME: &str = "Self";
 
+#[derive(Debug)]
 pub(super) struct TraitPathResolution {
     pub(super) method: TraitMethod,
     pub(super) item: Option<PathResolutionItem>,
@@ -1351,7 +1352,7 @@ impl<'context> Elaborator<'context> {
         object_type: &Type,
         method_name: &str,
         span: Span,
-        has_self_arg: bool,
+        check_self_arg_type: bool,
     ) -> Option<HirMethodReference> {
         match object_type.follow_bindings() {
             // TODO: We should allow method calls on `impl Trait`s eventually.
@@ -1370,7 +1371,7 @@ 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.lookup_method(&element, method_name, span, has_self_arg)
+                self.lookup_method(&element, method_name, span, check_self_arg_type)
             }
 
             // If we fail to resolve the object to a struct type, we have no way of type
@@ -1383,9 +1384,12 @@ impl<'context> Elaborator<'context> {
                 None
             }
 
-            other => {
-                self.lookup_struct_or_primitive_method(&other, method_name, span, has_self_arg)
-            }
+            other => self.lookup_struct_or_primitive_method(
+                &other,
+                method_name,
+                span,
+                check_self_arg_type,
+            ),
         }
     }
 
@@ -1394,18 +1398,18 @@ impl<'context> Elaborator<'context> {
         object_type: &Type,
         method_name: &str,
         span: Span,
-        has_self_arg: bool,
+        check_self_arg_type: bool,
     ) -> Option<HirMethodReference> {
         // 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_arg_type)
         {
             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_arg_type);
 
         // If there's at least one matching trait method we need to see if only one is in scope.
         if !trait_methods.is_empty() {
@@ -1415,7 +1419,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_arg_type);
         if !generic_methods.is_empty() {
             return self.return_trait_method_in_scope(&generic_methods, method_name, span);
         }

compiler/noirc_frontend/src/hir/def_map/item_scope.rs

@@ -1,10 +1,11 @@
 use super::{namespace::PerNs, ModuleDefId, ModuleId};
 use crate::ast::{Ident, ItemVisibility};
-use crate::node_interner::{FuncId, TraitId};
+use crate::node_interner::{FuncId, TraitId, TraitImplId};
 
 use std::collections::{hash_map::Entry, HashMap};
 
-type Scope = HashMap<Option<TraitId>, (ModuleDefId, ItemVisibility, bool /*is_prelude*/)>;
+type Scope =
+    HashMap<Option<(TraitImplId, TraitId)>, (ModuleDefId, ItemVisibility, bool /*is_prelude*/)>;
 
 #[derive(Default, Debug, PartialEq, Eq)]
 pub struct ItemScope {
@@ -20,9 +21,9 @@ impl ItemScope {
         name: Ident,
         visibility: ItemVisibility,
         mod_def: ModuleDefId,
-        trait_id: Option<TraitId>,
+        trait_impl: Option<(TraitImplId, TraitId)>,
     ) -> Result<(), (Ident, Ident)> {
-        self.add_item_to_namespace(name, visibility, mod_def, trait_id, false)?;
+        self.add_item_to_namespace(name, visibility, mod_def, trait_impl, false)?;
         self.defs.push(mod_def);
         Ok(())
     }
@@ -35,13 +36,13 @@ impl ItemScope {
         name: Ident,
         visibility: ItemVisibility,
         mod_def: ModuleDefId,
-        trait_id: Option<TraitId>,
+        trait_impl: Option<(TraitImplId, TraitId)>,
         is_prelude: bool,
     ) -> Result<(), (Ident, Ident)> {
         let add_item = |map: &mut HashMap<Ident, Scope>| {
             if let Entry::Occupied(mut o) = map.entry(name.clone()) {
-                let trait_hashmap = o.get_mut();
-                if let Entry::Occupied(mut n) = trait_hashmap.entry(trait_id) {
+                let trait_impl_hashmap = o.get_mut();
+                if let Entry::Occupied(mut n) = trait_impl_hashmap.entry(trait_impl) {
                     // Generally we want to reject having two of the same ident in the same namespace.
                     // The exception to this is when we're explicitly importing something
                     // which exists in the Noir stdlib prelude.
@@ -56,24 +57,23 @@ impl ItemScope {
                         Err((old_ident.clone(), name))
                     }
                 } else {
-                    trait_hashmap.insert(trait_id, (mod_def, visibility, is_prelude));
+                    trait_impl_hashmap.insert(trait_impl, (mod_def, visibility, is_prelude));
                     Ok(())
                 }
             } else {
                 let mut trait_hashmap = HashMap::new();
-                trait_hashmap.insert(trait_id, (mod_def, visibility, is_prelude));
+                trait_hashmap.insert(trait_impl, (mod_def, visibility, is_prelude));
                 map.insert(name, trait_hashmap);
                 Ok(())
             }
         };
 
         match mod_def {
-            ModuleDefId::ModuleId(_) => add_item(&mut self.types),
-            ModuleDefId::FunctionId(_) => add_item(&mut self.values),
-            ModuleDefId::TypeId(_) => add_item(&mut self.types),
-            ModuleDefId::TypeAliasId(_) => add_item(&mut self.types),
-            ModuleDefId::TraitId(_) => add_item(&mut self.types),
-            ModuleDefId::GlobalId(_) => add_item(&mut self.values),
+            ModuleDefId::ModuleId(_)
+            | ModuleDefId::TypeId(_)
+            | ModuleDefId::TypeAliasId(_)
+            | ModuleDefId::TraitId(_) => add_item(&mut self.types),
+            ModuleDefId::FunctionId(_) | ModuleDefId::GlobalId(_) => add_item(&mut self.values),
         }
     }
 
@@ -109,18 +109,6 @@ impl ItemScope {
         }
     }
 
-    pub fn find_func_with_name_and_trait_id(
-        &self,
-        func_name: &Ident,
-        trait_id: &Option<TraitId>,
-    ) -> Option<FuncId> {
-        let (module_def, _, _) = self.values.get(func_name)?.get(trait_id)?;
-        match module_def {
-            ModuleDefId::FunctionId(id) => Some(*id),
-            _ => None,
-        }
-    }
-
     pub fn find_name(&self, name: &Ident) -> PerNs {
         // Names, not associated with traits are searched first. If not found, we search for name, coming from a trait.
         // If we find only one name from trait, we return it. If there are multiple traits, providing the same name, we return None.
@@ -141,17 +129,6 @@ impl ItemScope {
         PerNs { types: find_name_in(&self.types), values: find_name_in(&self.values) }
     }
 
-    pub fn find_name_for_trait_id(&self, name: &Ident, trait_id: &Option<TraitId>) -> PerNs {
-        PerNs {
-            types: if let Some(t) = self.types.get(name) { t.get(trait_id).cloned() } else { None },
-            values: if let Some(v) = self.values.get(name) {
-                v.get(trait_id).cloned()
-            } else {
-                None
-            },
-        }
-    }
-
     pub fn names(&self) -> impl Iterator<Item = &Ident> {
         self.types.keys().chain(self.values.keys())
     }
@@ -167,9 +144,4 @@ impl ItemScope {
     pub fn values(&self) -> &HashMap<Ident, Scope> {
         &self.values
     }
-
-    pub fn remove_definition(&mut self, name: &Ident) {
-        self.types.remove(name);
-        self.values.remove(name);
-    }
 }

compiler/noirc_frontend/src/hir/def_map/module_data.rs

@@ -4,7 +4,7 @@ use noirc_errors::Location;
 
 use super::{ItemScope, LocalModuleId, ModuleDefId, ModuleId, PerNs};
 use crate::ast::{Ident, ItemVisibility};
-use crate::node_interner::{FuncId, GlobalId, StructId, TraitId, TypeAliasId};
+use crate::node_interner::{FuncId, GlobalId, StructId, TraitId, TraitImplId, TypeAliasId};
 use crate::token::SecondaryAttribute;
 
 /// Contains the actual contents of a module: its parent (if one exists),
@@ -75,17 +75,17 @@ impl ModuleData {
         name: Ident,
         visibility: ItemVisibility,
         item_id: ModuleDefId,
-        trait_id: Option<TraitId>,
+        trait_impl: Option<(TraitImplId, TraitId)>,
     ) -> Result<(), (Ident, Ident)> {
-        self.scope.add_definition(name.clone(), visibility, item_id, trait_id)?;
+        self.scope.add_definition(name.clone(), visibility, item_id, trait_impl)?;
 
         if let ModuleDefId::ModuleId(child) = item_id {
             self.child_declaration_order.push(child.local_id);
         }
 
         // definitions is a subset of self.scope so it is expected if self.scope.define_func_def
         // returns without error, so will self.definitions.define_func_def.
-        self.definitions.add_definition(name, visibility, item_id, trait_id)
+        self.definitions.add_definition(name, visibility, item_id, trait_impl)
     }
 
     pub fn declare_function(
@@ -97,18 +97,14 @@ impl ModuleData {
         self.declare(name, visibility, id.into(), None)
     }
 
-    pub fn declare_trait_function(
+    pub fn declare_trait_impl_function(
         &mut self,
         name: Ident,
         id: FuncId,
+        trait_impl_id: TraitImplId,
         trait_id: TraitId,
     ) -> Result<(), (Ident, Ident)> {
-        self.declare(name, ItemVisibility::Public, id.into(), Some(trait_id))
-    }
-
-    pub fn remove_function(&mut self, name: &Ident) {
-        self.scope.remove_definition(name);
-        self.definitions.remove_definition(name);
+        self.declare(name, ItemVisibility::Public, id.into(), Some((trait_impl_id, trait_id)))
     }
 
     pub fn declare_global(