diff --git a/compiler/noirc_frontend/src/elaborator/mod.rs b/compiler/noirc_frontend/src/elaborator/mod.rs index 10e56364ed3..64eafa73112 100644 --- a/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/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, + 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 { diff --git a/compiler/noirc_frontend/src/elaborator/path_resolution.rs b/compiler/noirc_frontend/src/elaborator/path_resolution.rs index 0d0b153b6b6..f3dc1c62473 100644 --- a/compiler/noirc_frontend/src/elaborator/path_resolution.rs +++ b/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); } diff --git a/compiler/noirc_frontend/src/elaborator/types.rs b/compiler/noirc_frontend/src/elaborator/types.rs index 8fa0b210605..17d62f67190 100644 --- a/compiler/noirc_frontend/src/elaborator/types.rs +++ b/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, @@ -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 { 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 { // 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 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); } diff --git a/compiler/noirc_frontend/src/hir/def_map/item_scope.rs b/compiler/noirc_frontend/src/hir/def_map/item_scope.rs index 3ca89e56bbc..ff6acef4098 100644 --- a/compiler/noirc_frontend/src/hir/def_map/item_scope.rs +++ b/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, (ModuleDefId, ItemVisibility, bool /*is_prelude*/)>; +type Scope = + HashMap, (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, + 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, + trait_impl: Option<(TraitImplId, TraitId)>, is_prelude: bool, ) -> Result<(), (Ident, Ident)> { let add_item = |map: &mut HashMap| { 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, - ) -> Option { - 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) -> 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 { self.types.keys().chain(self.values.keys()) } @@ -167,9 +144,4 @@ impl ItemScope { pub fn values(&self) -> &HashMap { &self.values } - - pub fn remove_definition(&mut self, name: &Ident) { - self.types.remove(name); - self.values.remove(name); - } } diff --git a/compiler/noirc_frontend/src/hir/def_map/module_data.rs b/compiler/noirc_frontend/src/hir/def_map/module_data.rs index 06188f3920b..330139083fd 100644 --- a/compiler/noirc_frontend/src/hir/def_map/module_data.rs +++ b/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,9 +75,9 @@ impl ModuleData { name: Ident, visibility: ItemVisibility, item_id: ModuleDefId, - trait_id: Option, + 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); @@ -85,7 +85,7 @@ impl ModuleData { // 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( diff --git a/compiler/noirc_frontend/src/node_interner.rs b/compiler/noirc_frontend/src/node_interner.rs index 0ec033a399b..4519c36cfcc 100644 --- a/compiler/noirc_frontend/src/node_interner.rs +++ b/compiler/noirc_frontend/src/node_interner.rs @@ -1382,7 +1382,8 @@ impl NodeInterner { }); if trait_id.is_none() && matches!(self_type, Type::Struct(..)) { - if let Some(existing) = self.lookup_direct_method(self_type, &method_name, true) + if let Some(existing) = + self.lookup_direct_method(self_type, &method_name, false) { return Some(existing); } @@ -1752,14 +1753,14 @@ impl NodeInterner { &self, typ: &Type, method_name: &str, - has_self_arg: bool, + check_self_arg_type: 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_arg_type, self)) } /// Looks up a methods that apply to the given type but are defined in traits. @@ -1767,14 +1768,14 @@ impl NodeInterner { &self, typ: &Type, method_name: &str, - has_self_arg: bool, + check_self_arg_type: bool, ) -> Vec<(FuncId, TraitId)> { let key = get_type_method_key(typ); if let Some(key) = key { self.methods .get(&key) .and_then(|h| h.get(method_name)) - .map(|methods| methods.find_trait_methods(typ, has_self_arg, self)) + .map(|methods| methods.find_trait_methods(typ, check_self_arg_type, self)) .unwrap_or_default() } else { Vec::new() @@ -1786,12 +1787,12 @@ impl NodeInterner { &self, typ: &Type, method_name: &str, - has_self_arg: bool, + check_self_arg_type: bool, ) -> Vec<(FuncId, TraitId)> { self.methods .get(&TypeMethodKey::Generic) .and_then(|h| h.get(method_name)) - .map(|methods| methods.find_trait_methods(typ, has_self_arg, self)) + .map(|methods| methods.find_trait_methods(typ, check_self_arg_type, self)) .unwrap_or_default() } diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index 637b15e7197..5d18862df9f 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -2832,6 +2832,50 @@ fn duplicate_struct_field() { assert_eq!(second_def.span().start(), 46); } +#[test] +fn duplicate_struct_method_without_self() { + let src = r#" + pub struct Foo { + } + + impl Foo { + fn bar() {} + fn bar() {} + } + + fn main() {} + "#; + let errors = get_program_errors(src); + assert_eq!(errors.len(), 1); + + let CompilationError::ResolverError(ResolverError::DuplicateDefinition { .. }) = &errors[0].0 + else { + panic!("Expected a duplicate error, got {:?}", errors[0].0); + }; +} + +#[test] +fn duplicate_struct_method_with_self() { + let src = r#" + pub struct Foo { + } + + impl Foo { + fn bar(self) { let _ = self; } + fn bar(self) { let _ = self; } + } + + fn main() {} + "#; + let errors = get_program_errors(src); + assert_eq!(errors.len(), 1); + + let CompilationError::ResolverError(ResolverError::DuplicateDefinition { .. }) = &errors[0].0 + else { + panic!("Expected a duplicate error, got {:?}", errors[0].0); + }; +} + #[test] fn trait_constraint_on_tuple_type() { let src = r#" diff --git a/compiler/noirc_frontend/src/tests/traits.rs b/compiler/noirc_frontend/src/tests/traits.rs index 3a55fc2b67d..50576823c03 100644 --- a/compiler/noirc_frontend/src/tests/traits.rs +++ b/compiler/noirc_frontend/src/tests/traits.rs @@ -1236,3 +1236,59 @@ fn warns_if_trait_is_not_in_scope_for_generic_function_call_and_there_is_only_on assert_eq!(ident.to_string(), "foo"); assert_eq!(trait_name, "private_mod::Foo"); } + +#[test] +fn calls_trait_method_using_struct_name_when_multiple_impls_exist_first_one() { + let src = r#" + trait From2 { + fn from2(input: T) -> Self; + } + + struct U60Repr {} + + impl From2<[Field; 3]> for U60Repr { + fn from2(_: [Field; 3]) -> Self { + U60Repr {} + } + } + + impl From2 for U60Repr { + fn from2(_: Field) -> Self { + U60Repr {} + } + } + + fn main() { + let _ = U60Repr::from2([1, 2, 3]); + } + "#; + assert_no_errors(src); +} + +#[test] +fn calls_trait_method_using_struct_name_when_multiple_impls_exist_second_one() { + let src = r#" + trait From2 { + fn from2(input: T) -> Self; + } + + struct U60Repr {} + + impl From2<[Field; 3]> for U60Repr { + fn from2(_: [Field; 3]) -> Self { + U60Repr {} + } + } + + impl From2 for U60Repr { + fn from2(_: Field) -> Self { + U60Repr {} + } + } + + fn main() { + let _ = U60Repr::from2(1); + } + "#; + assert_no_errors(src); +}