feat: Add check for overlapping generic traits (#3307)

compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs

@@ -13,9 +13,7 @@ use crate::hir::resolution::{
 use crate::hir::type_check::{type_check_func, TypeCheckError, TypeChecker};
 use crate::hir::Context;
 use crate::hir_def::traits::{Trait, TraitConstant, TraitFunction, TraitImpl, TraitType};
-use crate::node_interner::{
-    FuncId, NodeInterner, StmtId, StructId, TraitId, TraitImplKey, TypeAliasId,
-};
+use crate::node_interner::{FuncId, NodeInterner, StmtId, StructId, TraitId, TypeAliasId};
 
 use crate::parser::{ParserError, SortedModule};
 use crate::{
@@ -331,7 +329,6 @@ impl DefCollector {
             def_collector.collected_impls,
             &mut errors,
         );
-        // resolve_trait_impls can fill different type of errors, therefore we pass errors by mut ref
         let file_trait_impls_ids = resolve_trait_impls(
             context,
             def_collector.collected_traits_impls,
@@ -930,7 +927,7 @@ fn resolve_impls(
                     let method_name = interner.function_name(method_id).to_owned();
 
                     if let Some(first_fn) =
-                        interner.add_method(&self_type, method_name.clone(), *method_id)
+                        interner.add_method(&self_type, method_name.clone(), *method_id, false)
                     {
                         let error = ResolverError::DuplicateDefinition {
                             name: method_name,
@@ -962,7 +959,6 @@ fn resolve_trait_impls(
         let local_mod_id = trait_impl.module_id;
         let module_id = ModuleId { krate: crate_id, local_id: local_mod_id };
         let path_resolver = StandardPathResolver::new(module_id);
-        let trait_definition_ident = trait_impl.trait_path.last_segment();
 
         let self_type_span = unresolved_type.span;
 
@@ -989,35 +985,40 @@ fn resolve_trait_impls(
             }
         }
 
+        if matches!(self_type, Type::MutableReference(_)) {
+            let span = self_type_span.unwrap_or_else(|| trait_impl.trait_path.span());
+            let error = DefCollectorErrorKind::MutableReferenceInTraitImpl { span };
+            errors.push((error.into(), trait_impl.file_id));
+        }
+
         let mut new_resolver =
             Resolver::new(interner, &path_resolver, &context.def_maps, trait_impl.file_id);
         new_resolver.set_self_type(Some(self_type.clone()));
 
         if let Some(trait_id) = maybe_trait_id {
             check_methods_signatures(&mut new_resolver, &impl_methods, trait_id, errors);
 
-            let key = TraitImplKey { typ: self_type.clone(), trait_id };
-            if let Some(prev_trait_impl_ident) = interner.get_trait_implementation(&key) {
-                let err = DefCollectorErrorKind::Duplicate {
-                    typ: DuplicateType::TraitImplementation,
-                    first_def: prev_trait_impl_ident.borrow().ident.clone(),
-                    second_def: trait_definition_ident.clone(),
-                };
-                errors.push((err.into(), trait_impl.methods.file_id));
-            } else {
-                let resolved_trait_impl = Shared::new(TraitImpl {
-                    ident: trait_impl.trait_path.last_segment().clone(),
+            let resolved_trait_impl = Shared::new(TraitImpl {
+                ident: trait_impl.trait_path.last_segment().clone(),
+                typ: self_type.clone(),
+                trait_id,
+                file: trait_impl.file_id,
+                methods: vecmap(&impl_methods, |(_, func_id)| *func_id),
+            });
+
+            if let Some((prev_span, prev_file)) =
+                interner.add_trait_implementation(self_type.clone(), trait_id, resolved_trait_impl)
+            {
+                let error = DefCollectorErrorKind::OverlappingImpl {
                     typ: self_type.clone(),
-                    trait_id,
-                    methods: vecmap(&impl_methods, |(_, func_id)| *func_id),
-                });
-                if !interner.add_trait_implementation(&key, resolved_trait_impl.clone()) {
-                    let error = DefCollectorErrorKind::TraitImplNotAllowedFor {
-                        trait_path: trait_impl.trait_path.clone(),
-                        span: self_type_span.unwrap_or_else(|| trait_impl.trait_path.span()),
-                    };
-                    errors.push((error.into(), trait_impl.file_id));
-                }
+                    span: self_type_span.unwrap_or_else(|| trait_impl.trait_path.span()),
+                };
+                errors.push((error.into(), trait_impl.file_id));
+
+                // The 'previous impl defined here' note must be a separate error currently
+                // since it may be in a different file and all errors have the same file id.
+                let error = DefCollectorErrorKind::OverlappingImplNote { span: prev_span };
+                errors.push((error.into(), prev_file));
             }
 
             methods.append(&mut impl_methods);

compiler/noirc_frontend/src/hir/def_collector/errors.rs

@@ -33,8 +33,12 @@ pub enum DefCollectorErrorKind {
     PathResolutionError(PathResolutionError),
     #[error("Non-struct type used in impl")]
     NonStructTypeInImpl { span: Span },
-    #[error("Trait implementation is not allowed for this")]
-    TraitImplNotAllowedFor { trait_path: Path, span: Span },
+    #[error("Cannot implement trait on a mutable reference type")]
+    MutableReferenceInTraitImpl { span: Span },
+    #[error("Impl overlaps with existing impl for type {typ}")]
+    OverlappingImpl { span: Span, typ: crate::Type },
+    #[error("Previous impl defined here")]
+    OverlappingImplNote { span: Span },
     #[error("Cannot `impl` a type defined outside the current crate")]
     ForeignImpl { span: Span, type_name: String },
     #[error("Mismatch number of parameters in of trait implementation")]
@@ -130,10 +134,25 @@ impl From<DefCollectorErrorKind> for Diagnostic {
                 "Only struct types may have implementation methods".into(),
                 span,
             ),
-            DefCollectorErrorKind::TraitImplNotAllowedFor { trait_path, span } => {
+            DefCollectorErrorKind::MutableReferenceInTraitImpl { span } => Diagnostic::simple_error(
+                "Trait impls are not allowed on mutable reference types".into(),
+                "Try using a struct type here instead".into(),
+                span,
+            ),
+            DefCollectorErrorKind::OverlappingImpl { span, typ } => {
+                Diagnostic::simple_error(
+                    format!("Impl overlaps with existing impl for type `{typ}`"),
+                    "Overlapping impl".into(),
+                    span,
+                )
+            }
+            DefCollectorErrorKind::OverlappingImplNote { span } => {
+                // This should be a note or part of the previous error eventually.
+                // This must be an error to appear next to the previous OverlappingImpl
+                // error since we sort warnings first.
                 Diagnostic::simple_error(
-                    format!("Only limited types may implement trait `{trait_path}`"),
-                    "Only limited types may implement traits".into(),
+                    "Previous impl defined here".into(),
+                    "Previous impl defined here".into(),
                     span,
                 )
             }

compiler/noirc_frontend/src/hir/type_check/expr.rs

@@ -880,20 +880,14 @@ impl<'interner> TypeChecker<'interner> {
             // This may be a struct or a primitive type.
             Type::MutableReference(element) => self
                 .interner
-                .lookup_mut_primitive_trait_method(element.as_ref(), method_name)
+                .lookup_primitive_trait_method_mut(element.as_ref(), method_name)
                 .map(HirMethodReference::FuncId)
                 .or_else(|| self.lookup_method(element, method_name, expr_id)),
             // If we fail to resolve the object to a struct type, we have no way of type
             // checking its arguments as we can't even resolve the name of the function
             Type::Error => None,
 
-            // In the future we could support methods for non-struct types if we have a context
-            // (in the interner?) essentially resembling HashMap<Type, Methods>
-            other => match self
-                .interner
-                .lookup_primitive_method(other, method_name)
-                .or_else(|| self.interner.lookup_primitive_trait_method(other, method_name))
-            {
+            other => match self.interner.lookup_primitive_method(other, method_name) {
                 Some(method_id) => Some(HirMethodReference::FuncId(method_id)),
                 None => {
                     self.errors.push(TypeCheckError::UnresolvedMethodCall {

compiler/noirc_frontend/src/hir/type_check/mod.rs

@@ -15,7 +15,7 @@ pub use errors::TypeCheckError;
 
 use crate::{
     hir_def::{expr::HirExpression, stmt::HirStatement},
-    node_interner::{ExprId, FuncId, NodeInterner, StmtId, TraitImplKey},
+    node_interner::{ExprId, FuncId, NodeInterner, StmtId},
     Type,
 };
 
@@ -65,8 +65,10 @@ pub fn type_check_func(interner: &mut NodeInterner, func_id: FuncId) -> Vec<Type
         let (expr_span, empty_function) = function_info(interner, function_body_id);
         let func_span = interner.expr_span(function_body_id); // XXX: We could be more specific and return the span of the last stmt, however stmts do not have spans yet
         if let Type::TraitAsType(t) = &declared_return_type {
-            let key = TraitImplKey { typ: function_last_type.follow_bindings(), trait_id: t.id };
-            if interner.get_trait_implementation(&key).is_none() {
+            if interner
+                .lookup_trait_implementation(function_last_type.follow_bindings(), t.id)
+                .is_none()
+            {
                 let error = TypeCheckError::TypeMismatchWithSource {
                     expected: declared_return_type.clone(),
                     actual: function_last_type.clone(),

compiler/noirc_frontend/src/hir_def/traits.rs

@@ -3,6 +3,7 @@ use crate::{
     node_interner::{FuncId, TraitId, TraitMethodId},
     Generics, Ident, NoirFunction, Type, TypeVariable, TypeVariableId,
 };
+use fm::FileId;
 use noirc_errors::Span;
 
 #[derive(Clone, Debug, PartialEq, Eq)]
@@ -62,6 +63,7 @@ pub struct TraitImpl {
     pub ident: Ident,
     pub typ: Type,
     pub trait_id: TraitId,
+    pub file: FileId,
     pub methods: Vec<FuncId>, // methods[i] is the implementation of trait.methods[i] for Type typ
 }
 

compiler/noirc_frontend/src/monomorphization/mod.rs

@@ -24,7 +24,7 @@ use crate::{
         stmt::{HirAssignStatement, HirLValue, HirLetStatement, HirPattern, HirStatement},
         types,
     },
-    node_interner::{self, DefinitionKind, NodeInterner, StmtId, TraitImplKey, TraitMethodId},
+    node_interner::{self, DefinitionKind, NodeInterner, StmtId, TraitMethodId},
     token::FunctionAttribute,
     ContractFunctionType, FunctionKind, Type, TypeBinding, TypeBindings, TypeVariableKind,
     Visibility,
@@ -820,10 +820,7 @@ impl<'interner> Monomorphizer<'interner> {
 
         let trait_impl = self
             .interner
-            .get_trait_implementation(&TraitImplKey {
-                typ: self_type.follow_bindings(),
-                trait_id: method.trait_id,
-            })
+            .lookup_trait_implementation(self_type.follow_bindings(), method.trait_id)
             .expect("ICE: missing trait impl - should be caught during type checking");
 
         let hir_func_id = trait_impl.borrow().methods[method.method_index];