feat: Expand trait impl overlap check to cover generic types (#3320)

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

@@ -35,7 +35,7 @@ pub enum DefCollectorErrorKind {
     NonStructTypeInImpl { span: Span },
     #[error("Cannot implement trait on a mutable reference type")]
     MutableReferenceInTraitImpl { span: Span },
-    #[error("Impl overlaps with existing impl for type {typ}")]
+    #[error("Impl for type `{typ}` overlaps with existing impl")]
     OverlappingImpl { span: Span, typ: crate::Type },
     #[error("Previous impl defined here")]
     OverlappingImplNote { span: Span },
@@ -141,7 +141,7 @@ impl From<DefCollectorErrorKind> for Diagnostic {
             ),
             DefCollectorErrorKind::OverlappingImpl { span, typ } => {
                 Diagnostic::simple_error(
-                    format!("Impl overlaps with existing impl for type `{typ}`"),
+                    format!("Impl for type `{typ}` overlaps with existing impl"),
                     "Overlapping impl".into(),
                     span,
                 )

compiler/noirc_frontend/src/hir_def/types.rs

@@ -1069,6 +1069,75 @@ impl Type {
         }
     }
 
+    /// Replace each NamedGeneric (and TypeVariable) in this type with a fresh type variable
+    pub(crate) fn instantiate_named_generics(&self, interner: &NodeInterner) -> Type {
+        let mut substitutions = HashMap::new();
+        self.find_all_unbound_type_variables(interner, &mut substitutions);
+        self.substitute(&substitutions)
+    }
+
+    /// For each unbound type variable in the current type, add a type binding to the given list
+    /// to bind the unbound type variable to a fresh type variable.
+    fn find_all_unbound_type_variables(
+        &self,
+        interner: &NodeInterner,
+        bindings: &mut TypeBindings,
+    ) {
+        match self {
+            Type::FieldElement
+            | Type::Integer(_, _)
+            | Type::Bool
+            | Type::Unit
+            | Type::TraitAsType(_)
+            | Type::Constant(_)
+            | Type::NotConstant
+            | Type::Error => (),
+            Type::Array(length, elem) => {
+                length.find_all_unbound_type_variables(interner, bindings);
+                elem.find_all_unbound_type_variables(interner, bindings);
+            }
+            Type::String(length) => length.find_all_unbound_type_variables(interner, bindings),
+            Type::FmtString(length, env) => {
+                length.find_all_unbound_type_variables(interner, bindings);
+                env.find_all_unbound_type_variables(interner, bindings);
+            }
+            Type::Struct(_, generics) => {
+                for generic in generics {
+                    generic.find_all_unbound_type_variables(interner, bindings);
+                }
+            }
+            Type::Tuple(fields) => {
+                for field in fields {
+                    field.find_all_unbound_type_variables(interner, bindings);
+                }
+            }
+            Type::Function(args, ret, env) => {
+                for arg in args {
+                    arg.find_all_unbound_type_variables(interner, bindings);
+                }
+                ret.find_all_unbound_type_variables(interner, bindings);
+                env.find_all_unbound_type_variables(interner, bindings);
+            }
+            Type::MutableReference(elem) => {
+                elem.find_all_unbound_type_variables(interner, bindings);
+            }
+            Type::Forall(_, typ) => typ.find_all_unbound_type_variables(interner, bindings),
+            Type::TypeVariable(type_variable, _) | Type::NamedGeneric(type_variable, _) => {
+                match &*type_variable.borrow() {
+                    TypeBinding::Bound(binding) => {
+                        binding.find_all_unbound_type_variables(interner, bindings);
+                    }
+                    TypeBinding::Unbound(id) => {
+                        if !bindings.contains_key(id) {
+                            let fresh_type_variable = interner.next_type_variable();
+                            bindings.insert(*id, (type_variable.clone(), fresh_type_variable));
+                        }
+                    }
+                }
+            }
+        }
+    }
+
     /// Substitute any type variables found within this type with the
     /// given bindings if found. If a type variable is not found within
     /// the given TypeBindings, it is unchanged.

compiler/noirc_frontend/src/node_interner.rs

@@ -970,15 +970,19 @@ impl NodeInterner {
 
         self.trait_implementations.push(trait_impl.clone());
 
-        let entries = self.trait_implementation_map.entry(trait_id).or_default();
-
-        // Check that this new impl does not overlap with any existing impls first
-        for (existing_object_type, existing_impl_id) in entries {
-            if object_type.try_unify(existing_object_type).is_ok() {
-                // Overlapping impl
-                let existing_impl = &self.trait_implementations[existing_impl_id.0];
-                let existing_impl = existing_impl.borrow();
-                return Some((existing_impl.ident.span(), existing_impl.file));
+        if let Some(entries) = self.trait_implementation_map.get(&trait_id) {
+            // Check that this new impl does not overlap with any existing impls first
+            for (existing_object_type, existing_impl_id) in entries {
+                // Instantiate named generics so that S<T> overlaps with S<u32>
+                let object_type = object_type.instantiate_named_generics(self);
+                let existing_object_type = existing_object_type.instantiate_named_generics(self);
+
+                if object_type.try_unify(&existing_object_type).is_ok() {
+                    // Overlapping impl
+                    let existing_impl = &self.trait_implementations[existing_impl_id.0];
+                    let existing_impl = existing_impl.borrow();
+                    return Some((existing_impl.ident.span(), existing_impl.file));
+                }
             }
         }
 

tooling/nargo_cli/tests/compile_failure/overlapping_generic_impls/src/main.nr

@@ -2,7 +2,6 @@
 trait Trait { fn t(self); }
 
 impl<T> Trait for T { fn t(self){} }
-
-impl<T> Trait for T { fn t(self){} }
+impl Trait for u32 { fn t(self){} }
 
 fn main() {}