fix(elaborator): Fix duplicate methods error (#5225)

# Description

## Problem\*

Resolves "duplicate method" error when declaring trait methods on
different traits with the same method name in the elaborator.

## Summary\*



## Additional Context

This was the last error type in aztec packages for the elaborator.
~~After this, there are still some extra unused variable warnings I will
fix but the entire crate tests successfully.~~ Nevermind, looks like
aztec-nr just naturally has many unused variables

## Documentation\*

Check one:
- [x] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[For Experimental Features]** Documentation to be submitted in a
separate PR.

# PR Checklist\*

- [x] I have tested the changes locally.
- [x] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.

compiler/noirc_frontend/src/elaborator/mod.rs

@@ -232,23 +232,20 @@ impl<'context> Elaborator<'context> {
         // Must resolve structs before we resolve globals.
         this.collect_struct_definitions(items.types);
 
-        // Bind trait impls to their trait. Collect trait functions, that have a
-        // default implementation, which hasn't been overridden.
-        for trait_impl in &mut items.trait_impls {
-            this.collect_trait_impl(trait_impl);
-        }
-
         // Before we resolve any function symbols we must go through our impls and
         // re-collect the methods within into their proper module. This cannot be
         // done during def collection since we need to be able to resolve the type of
         // the impl since that determines the module we should collect into.
-        //
-        // These are resolved after trait impls so that struct methods are chosen
-        // over trait methods if there are name conflicts.
         for ((_self_type, module), impls) in &mut items.impls {
             this.collect_impls(*module, impls);
         }
 
+        // Bind trait impls to their trait. Collect trait functions, that have a
+        // default implementation, which hasn't been overridden.
+        for trait_impl in &mut items.trait_impls {
+            this.collect_trait_impl(trait_impl);
+        }
+
         // We must wait to resolve non-literal globals until after we resolve structs since struct
         // globals will need to reference the struct type they're initialized to to ensure they are valid.
         while let Some((_, global)) = this.unresolved_globals.pop_first() {
@@ -860,9 +857,12 @@ impl<'context> Elaborator<'context> {
         self.self_type = None;
     }
 
-    fn get_module_mut(&mut self, module: ModuleId) -> &mut ModuleData {
+    fn get_module_mut(
+        def_maps: &mut BTreeMap<CrateId, CrateDefMap>,
+        module: ModuleId,
+    ) -> &mut ModuleData {
         let message = "A crate should always be present for a given crate id";
-        &mut self.def_maps.get_mut(&module.krate).expect(message).modules[module.local_id.0]
+        &mut def_maps.get_mut(&module.krate).expect(message).modules[module.local_id.0]
     }
 
     fn declare_methods_on_struct(
@@ -890,7 +890,7 @@ impl<'context> Elaborator<'context> {
             // Grab the module defined by the struct type. Note that impls are a case
             // where the module the methods are added to is not the same as the module
             // they are resolved in.
-            let module = self.get_module_mut(struct_ref.id.module_id());
+            let module = Self::get_module_mut(self.def_maps, struct_ref.id.module_id());
 
             for (_, method_id, method) in &functions.functions {
                 // If this method was already declared, remove it from the module so it cannot
@@ -899,22 +899,37 @@ impl<'context> Elaborator<'context> {
                 // If not, that is specialization which is allowed.
                 let name = method.name_ident().clone();
                 if module.declare_function(name, ItemVisibility::Public, *method_id).is_err() {
-                    module.remove_function(method.name_ident());
+                    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());
+                    }
                 }
             }
 
-            self.declare_struct_methods(self_type, &function_ids);
+            // Trait impl methods are already declared in NodeInterner::add_trait_implementation
+            if !is_trait_impl {
+                self.declare_methods(self_type, &function_ids);
+            }
         // We can define methods on primitive types only if we're in the stdlib
         } else if !is_trait_impl && *self_type != Type::Error {
             if self.crate_id.is_stdlib() {
-                self.declare_struct_methods(self_type, &function_ids);
+                // Trait impl methods are already declared in NodeInterner::add_trait_implementation
+                if !is_trait_impl {
+                    self.declare_methods(self_type, &function_ids);
+                }
             } else {
                 self.push_err(DefCollectorErrorKind::NonStructTypeInImpl { span });
             }
         }
     }
 
-    fn declare_struct_methods(&mut self, self_type: &Type, function_ids: &[FuncId]) {
+    fn declare_methods(&mut self, self_type: &Type, function_ids: &[FuncId]) {
         for method_id in function_ids {
             let method_name = self.interner.function_name(method_id).to_owned();
 

compiler/noirc_frontend/src/node_interner.rs

@@ -1466,6 +1466,7 @@ impl NodeInterner {
         force_type_check: bool,
     ) -> Option<FuncId> {
         let methods = self.struct_methods.get(&(id, method_name.to_owned()));
+
         // If there is only one method, just return it immediately.
         // It will still be typechecked later.
         if !force_type_check {

test_programs/compile_success_empty/no_duplicate_methods/Nargo.toml

@@ -0,0 +1,6 @@
+[package]
+name = "no_duplicate_methods"
+type = "bin"
+authors = [""]
+
+[dependencies]

test_programs/compile_success_empty/no_duplicate_methods/src/main.nr

@@ -0,0 +1,26 @@
+// Test that declaring several methods & trait methods with the same name
+// does not trigger a duplicate method error
+trait ToField {
+    fn to_field(self) -> Field;
+}
+trait ToField2 {
+    fn to_field(self) -> Field;
+}
+
+struct Foo { x: Field }
+
+impl ToField for Foo {
+    fn to_field(self) -> Field { self.x }
+}
+
+impl ToField2 for Foo {
+    fn to_field(self) -> Field { self.x }
+}
+
+impl Foo {
+    fn to_field(self) -> Field {
+        self.x
+    }
+}
+
+fn main() {}