From 7a2483694549ffa7740743286b57f2b816e21247 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Tue, 11 Jun 2024 11:10:24 -0500 Subject: [PATCH 1/3] Fix duplicate methods error --- compiler/noirc_frontend/src/elaborator/mod.rs | 24 ++++++++++------- .../no_duplicate_methods/Nargo.toml | 6 +++++ .../no_duplicate_methods/Prover.toml | 0 .../no_duplicate_methods/src/main.nr | 26 +++++++++++++++++++ 4 files changed, 47 insertions(+), 9 deletions(-) create mode 100644 test_programs/compile_success_empty/no_duplicate_methods/Nargo.toml create mode 100644 test_programs/compile_success_empty/no_duplicate_methods/Prover.toml create mode 100644 test_programs/compile_success_empty/no_duplicate_methods/src/main.nr diff --git a/compiler/noirc_frontend/src/elaborator/mod.rs b/compiler/noirc_frontend/src/elaborator/mod.rs index 37ad74b78b0..99c466e14e4 100644 --- a/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/compiler/noirc_frontend/src/elaborator/mod.rs @@ -232,12 +232,6 @@ 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 @@ -249,6 +243,12 @@ impl<'context> Elaborator<'context> { 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() { @@ -903,18 +903,24 @@ impl<'context> Elaborator<'context> { } } - 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(); diff --git a/test_programs/compile_success_empty/no_duplicate_methods/Nargo.toml b/test_programs/compile_success_empty/no_duplicate_methods/Nargo.toml new file mode 100644 index 00000000000..2125d475530 --- /dev/null +++ b/test_programs/compile_success_empty/no_duplicate_methods/Nargo.toml @@ -0,0 +1,6 @@ +[package] +name = "no_duplicate_methods" +type = "bin" +authors = [""] + +[dependencies] diff --git a/test_programs/compile_success_empty/no_duplicate_methods/Prover.toml b/test_programs/compile_success_empty/no_duplicate_methods/Prover.toml new file mode 100644 index 00000000000..e69de29bb2d diff --git a/test_programs/compile_success_empty/no_duplicate_methods/src/main.nr b/test_programs/compile_success_empty/no_duplicate_methods/src/main.nr new file mode 100644 index 00000000000..2be1d3fa11e --- /dev/null +++ b/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() {} From b2aa1a59cff944f4d4d3b089f935e81a0c096c5f Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Tue, 11 Jun 2024 11:50:05 -0500 Subject: [PATCH 2/3] Fix method resolution --- compiler/noirc_frontend/src/elaborator/mod.rs | 32 +++++++++++++------ compiler/noirc_frontend/src/node_interner.rs | 1 + 2 files changed, 23 insertions(+), 10 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/mod.rs b/compiler/noirc_frontend/src/elaborator/mod.rs index 99c466e14e4..723cf84c31f 100644 --- a/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/compiler/noirc_frontend/src/elaborator/mod.rs @@ -232,6 +232,12 @@ 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 @@ -243,12 +249,6 @@ impl<'context> Elaborator<'context> { 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 +860,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, + 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 +893,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,7 +902,16 @@ 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()); + } } } diff --git a/compiler/noirc_frontend/src/node_interner.rs b/compiler/noirc_frontend/src/node_interner.rs index cef49332b00..6c5f9b6bbcb 100644 --- a/compiler/noirc_frontend/src/node_interner.rs +++ b/compiler/noirc_frontend/src/node_interner.rs @@ -1466,6 +1466,7 @@ impl NodeInterner { force_type_check: bool, ) -> Option { 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 { From 2cb42438e189cb6f4b9650cfb8ce7912e459fa01 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Tue, 11 Jun 2024 11:54:55 -0500 Subject: [PATCH 3/3] Reorder collect resolution --- compiler/noirc_frontend/src/elaborator/mod.rs | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/mod.rs b/compiler/noirc_frontend/src/elaborator/mod.rs index 723cf84c31f..aee3b5c5807 100644 --- a/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/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() {