From 48c12637e8ef5220e130543b5c7d10f69581bd5b Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 27 Oct 2023 14:07:42 -0500 Subject: [PATCH 1/3] Fix overlapping type check --- .../src/hir/def_collector/errors.rs | 4 +- compiler/noirc_frontend/src/hir_def/types.rs | 69 +++++++++++++++++++ compiler/noirc_frontend/src/node_interner.rs | 22 +++--- .../overlapping_generic_impls/src/main.nr | 3 +- 4 files changed, 85 insertions(+), 13 deletions(-) diff --git a/compiler/noirc_frontend/src/hir/def_collector/errors.rs b/compiler/noirc_frontend/src/hir/def_collector/errors.rs index 8658acb0864..2abacbc64da 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/errors.rs +++ b/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 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, ) diff --git a/compiler/noirc_frontend/src/hir_def/types.rs b/compiler/noirc_frontend/src/hir_def/types.rs index 110334f331e..25059193429 100644 --- a/compiler/noirc_frontend/src/hir_def/types.rs +++ b/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) + } + + /// Returns a list of substituions from each unbound type variable in the current type + /// 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. diff --git a/compiler/noirc_frontend/src/node_interner.rs b/compiler/noirc_frontend/src/node_interner.rs index 10d5ece2edc..ef5f72df4a2 100644 --- a/compiler/noirc_frontend/src/node_interner.rs +++ b/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 overlaps with S + 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)); + } } } diff --git a/tooling/nargo_cli/tests/compile_failure/overlapping_generic_impls/src/main.nr b/tooling/nargo_cli/tests/compile_failure/overlapping_generic_impls/src/main.nr index a835670e0f8..abf905ad71b 100644 --- a/tooling/nargo_cli/tests/compile_failure/overlapping_generic_impls/src/main.nr +++ b/tooling/nargo_cli/tests/compile_failure/overlapping_generic_impls/src/main.nr @@ -2,7 +2,6 @@ trait Trait { fn t(self); } impl Trait for T { fn t(self){} } - -impl Trait for T { fn t(self){} } +impl Trait for u32 { fn t(self){} } fn main() {} From 93a9be46a94c76b2ea5732f033b1ffcaef1881d3 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 27 Oct 2023 14:27:45 -0500 Subject: [PATCH 2/3] Amend comment --- compiler/noirc_frontend/src/hir_def/types.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/compiler/noirc_frontend/src/hir_def/types.rs b/compiler/noirc_frontend/src/hir_def/types.rs index 25059193429..98171b0a5d1 100644 --- a/compiler/noirc_frontend/src/hir_def/types.rs +++ b/compiler/noirc_frontend/src/hir_def/types.rs @@ -1076,8 +1076,8 @@ impl Type { self.substitute(&substitutions) } - /// Returns a list of substituions from each unbound type variable in the current type - /// to a fresh type variable + /// 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, From 7404da51b60aa165bbf027cd9d29c988e2a56642 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 27 Oct 2023 14:42:27 -0500 Subject: [PATCH 3/3] Cargo fmt made clippy complain --- compiler/noirc_frontend/src/hir_def/types.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_frontend/src/hir_def/types.rs b/compiler/noirc_frontend/src/hir_def/types.rs index 98171b0a5d1..59404c5f298 100644 --- a/compiler/noirc_frontend/src/hir_def/types.rs +++ b/compiler/noirc_frontend/src/hir_def/types.rs @@ -1119,7 +1119,7 @@ impl Type { env.find_all_unbound_type_variables(interner, bindings); } Type::MutableReference(elem) => { - elem.find_all_unbound_type_variables(interner, bindings) + 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, _) => {