From ffb1df491a03f4026298ced048f7a0dcaf4d7011 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Mon, 13 Jan 2025 13:56:27 -0600 Subject: [PATCH 1/2] Prevent overlapping associated types impls --- compiler/noirc_frontend/src/node_interner.rs | 12 +++++++- compiler/noirc_frontend/src/tests/traits.rs | 32 +++++++++++++++++++- 2 files changed, 42 insertions(+), 2 deletions(-) diff --git a/compiler/noirc_frontend/src/node_interner.rs b/compiler/noirc_frontend/src/node_interner.rs index 0ec033a399b..b78850399aa 100644 --- a/compiler/noirc_frontend/src/node_interner.rs +++ b/compiler/noirc_frontend/src/node_interner.rs @@ -1716,8 +1716,18 @@ impl NodeInterner { let instantiated_object_type = object_type.substitute(&substitutions); let trait_generics = &trait_impl.borrow().trait_generics; + + // Replace any associated types with fresh type variables so that we match + // any existing impl regardless of associated types if one already exists. + // E.g. if we already have an `impl Foo for Baz`, we should + // reject `impl Foo for Baz` if it were to be added. let associated_types = self.get_associated_types_for_impl(impl_id); + let associated_types = vecmap(associated_types, |named| { + let typ = self.next_type_variable(); + NamedType { name: named.name.clone(), typ } + }); + // Ignoring overlapping `TraitImplKind::Assumed` impls here is perfectly fine. // It should never happen since impls are defined at global scope, but even // if they were, we should never prevent defining a new impl because a 'where' @@ -1726,7 +1736,7 @@ impl NodeInterner { &instantiated_object_type, trait_id, trait_generics, - associated_types, + &associated_types, ) { let existing_impl = self.get_trait_implementation(existing); let existing_impl = existing_impl.borrow(); diff --git a/compiler/noirc_frontend/src/tests/traits.rs b/compiler/noirc_frontend/src/tests/traits.rs index 3a55fc2b67d..0b774098868 100644 --- a/compiler/noirc_frontend/src/tests/traits.rs +++ b/compiler/noirc_frontend/src/tests/traits.rs @@ -1,10 +1,11 @@ use crate::hir::def_collector::dc_crate::CompilationError; +use crate::hir::def_collector::errors::DefCollectorErrorKind; use crate::hir::resolution::errors::ResolverError; use crate::hir::resolution::import::PathResolutionError; use crate::hir::type_check::TypeCheckError; use crate::tests::{get_program_errors, get_program_with_maybe_parser_errors}; -use super::assert_no_errors; +use super::{assert_no_errors, get_program}; #[test] fn trait_inheritance() { @@ -1236,3 +1237,32 @@ 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 error_on_duplicate_impl_with_associated_type() { + let src = r#" + trait Foo { + type Bar; + } + + impl Foo for i32 { + type Bar = u32; + } + + impl Foo for i32 { + type Bar = u8; + } + + fn main() {} + "#; + + // Expect "Impl for type `i32` overlaps with existing impl" + // and "Previous impl defined here" + let errors = get_program_errors(src); + assert_eq!(errors.len(), 2); + + use CompilationError::DefinitionError; + use DefCollectorErrorKind::*; + assert!(matches!(&errors[0].0, DefinitionError(OverlappingImpl { .. }))); + assert!(matches!(&errors[1].0, DefinitionError(OverlappingImplNote { .. }))); +} From 8c28477109e1d1afc2ed43bec900f51eee6d29ef Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Mon, 13 Jan 2025 13:58:48 -0600 Subject: [PATCH 2/2] Add another test --- compiler/noirc_frontend/src/tests/traits.rs | 31 ++++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-) diff --git a/compiler/noirc_frontend/src/tests/traits.rs b/compiler/noirc_frontend/src/tests/traits.rs index 0b774098868..efdae5b2bee 100644 --- a/compiler/noirc_frontend/src/tests/traits.rs +++ b/compiler/noirc_frontend/src/tests/traits.rs @@ -5,7 +5,7 @@ use crate::hir::resolution::import::PathResolutionError; use crate::hir::type_check::TypeCheckError; use crate::tests::{get_program_errors, get_program_with_maybe_parser_errors}; -use super::{assert_no_errors, get_program}; +use super::assert_no_errors; #[test] fn trait_inheritance() { @@ -1266,3 +1266,32 @@ fn error_on_duplicate_impl_with_associated_type() { assert!(matches!(&errors[0].0, DefinitionError(OverlappingImpl { .. }))); assert!(matches!(&errors[1].0, DefinitionError(OverlappingImplNote { .. }))); } + +#[test] +fn error_on_duplicate_impl_with_associated_constant() { + let src = r#" + trait Foo { + let Bar: u32; + } + + impl Foo for i32 { + let Bar = 5; + } + + impl Foo for i32 { + let Bar = 6; + } + + fn main() {} + "#; + + // Expect "Impl for type `i32` overlaps with existing impl" + // and "Previous impl defined here" + let errors = get_program_errors(src); + assert_eq!(errors.len(), 2); + + use CompilationError::DefinitionError; + use DefCollectorErrorKind::*; + assert!(matches!(&errors[0].0, DefinitionError(OverlappingImpl { .. }))); + assert!(matches!(&errors[1].0, DefinitionError(OverlappingImplNote { .. }))); +}