From 83b421d6bfc59a5eb33f1b4fad185bbdc54d5814 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 26 Jun 2024 18:06:53 +0000 Subject: [PATCH 01/27] accurately error on no impl found for a trait constraint on a trait impl method --- compiler/noirc_frontend/src/elaborator/mod.rs | 2 +- .../noirc_frontend/src/elaborator/patterns.rs | 1 - .../src/hir/def_collector/dc_mod.rs | 21 +++++++- compiler/noirc_frontend/src/tests.rs | 48 +++++++++++++++++++ 4 files changed, 69 insertions(+), 3 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/mod.rs b/compiler/noirc_frontend/src/elaborator/mod.rs index 46406083a09..dcdc37186b3 100644 --- a/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/compiler/noirc_frontend/src/elaborator/mod.rs @@ -904,7 +904,7 @@ impl<'context> Elaborator<'context> { .where_clause .iter() .flat_map(|item| self.resolve_trait_constraint(item)) - .collect(); + .collect::>(); let trait_generics = trait_impl.resolved_trait_generics.clone(); diff --git a/compiler/noirc_frontend/src/elaborator/patterns.rs b/compiler/noirc_frontend/src/elaborator/patterns.rs index 4f04f5c523c..45713c616ab 100644 --- a/compiler/noirc_frontend/src/elaborator/patterns.rs +++ b/compiler/noirc_frontend/src/elaborator/patterns.rs @@ -513,7 +513,6 @@ impl<'context> Elaborator<'context> { if let Some(definition) = self.interner.try_definition(ident.id) { if let DefinitionKind::Function(function) = definition.kind { let function = self.interner.function_meta(&function); - for mut constraint in function.trait_constraints.clone() { constraint.apply_bindings(&bindings); self.trait_constraints.push((constraint, expr_id)); diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs index 3d8a861b009..ab3724d3ef1 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs @@ -11,6 +11,7 @@ use crate::ast::{ NoirStruct, NoirTrait, NoirTraitImpl, NoirTypeAlias, Pattern, TraitImplItem, TraitItem, TypeImpl, }; +use crate::macros_api::UnresolvedTypeData; use crate::{ graph::CrateId, hir::def_collector::dc_crate::{UnresolvedStruct, UnresolvedTrait}, @@ -169,7 +170,7 @@ impl<'a> ModCollector<'a> { impls: Vec, krate: CrateId, ) { - for trait_impl in impls { + for mut trait_impl in impls { let trait_name = trait_impl.trait_name.clone(); let mut unresolved_functions = @@ -178,7 +179,25 @@ impl<'a> ModCollector<'a> { let module = ModuleId { krate, local_id: self.module_id }; for (_, func_id, noir_function) in &mut unresolved_functions.functions { + // Attach any trait constraints on the impl to the function noir_function.def.where_clause.append(&mut trait_impl.where_clause.clone()); + + // If there are trait constraints on the trait impl methods that reference the impl generics + // they can be viewed as a constraint on the trait impl itself. + for generic in trait_impl.impl_generics.iter() { + let impl_generic_name = &generic.ident().0.contents; + for func_trait_constraint in noir_function.def.where_clause.iter() { + if let UnresolvedTypeData::Named(path, _, _) = + &func_trait_constraint.typ.typ + { + let trait_constraint_name = &path.last_segment().0.contents; + if trait_constraint_name == impl_generic_name { + trait_impl.where_clause.push(func_trait_constraint.clone()); + } + } + } + } + let location = Location::new(noir_function.def.span, self.file_id); context.def_interner.push_function(*func_id, &noir_function.def, module, location); } diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index 9251eb3db6b..69cc032aebc 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -1896,3 +1896,51 @@ fn quote_code_fragments() { use InterpreterError::FailingConstraint; assert!(matches!(&errors[0].0, CompilationError::InterpreterError(FailingConstraint { .. }))); } + +#[test] +fn impl_not_found_for_trait_impl_method_trait_constraint() { + // This test ensures that we still error with that no matching impl was found + // if a where clause referencing an impl generic only exists on a trait impl's + // methods. + let src = r#" + trait Serialize { + fn serialize(self) -> [Field; N]; + } + + trait ToField { + fn to_field(self) -> Field; + } + + fn process_array(array: [Field; N]) -> Field { + array[0] + } + + fn serialize_thing(thing: A) -> [Field; N] where A: Serialize { + thing.serialize() + } + + struct MyType { + a: T, + b: T, + } + + impl Serialize<2> for MyType { + fn serialize(self) -> [Field; 2] where T: ToField { + [ self.a.to_field(), self.b.to_field() ] + } + } + + impl MyType { + fn do_thing_with_serialization_with_extra_steps(self) -> Field { + process_array(serialize_thing(self)) + } + } + "#; + + let errors = get_program_errors(src); + assert_eq!(errors.len(), 1); + assert!(matches!( + &errors[0].0, + CompilationError::TypeError(TypeCheckError::NoMatchingImplFound { .. }) + )); +} From 952be4e4980914f45b6a57845e9445bc3c33deee Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 26 Jun 2024 20:12:31 +0000 Subject: [PATCH 02/27] add impl stricter than trait error --- .../src/elaborator/expressions.rs | 1 + compiler/noirc_frontend/src/elaborator/mod.rs | 46 +++++++++++++--- .../noirc_frontend/src/elaborator/traits.rs | 1 + .../noirc_frontend/src/elaborator/types.rs | 2 + .../src/hir/def_collector/dc_mod.rs | 20 +------ .../src/hir/def_collector/errors.rs | 21 ++++++++ .../src/hir/resolution/resolver.rs | 6 ++- .../src/hir/resolution/traits.rs | 2 + .../noirc_frontend/src/hir/type_check/expr.rs | 2 + compiler/noirc_frontend/src/hir_def/expr.rs | 1 + compiler/noirc_frontend/src/hir_def/traits.rs | 8 +-- compiler/noirc_frontend/src/node_interner.rs | 10 +++- compiler/noirc_frontend/src/tests.rs | 53 +++++++++++++++++-- .../impl_stricter_than_trait/Nargo.toml | 7 +++ .../impl_stricter_than_trait/src/main.nr | 32 +++++++++++ 15 files changed, 175 insertions(+), 37 deletions(-) create mode 100644 test_programs/compile_failure/impl_stricter_than_trait/Nargo.toml create mode 100644 test_programs/compile_failure/impl_stricter_than_trait/src/main.nr diff --git a/compiler/noirc_frontend/src/elaborator/expressions.rs b/compiler/noirc_frontend/src/elaborator/expressions.rs index 02d4225875b..b355c6c9785 100644 --- a/compiler/noirc_frontend/src/elaborator/expressions.rs +++ b/compiler/noirc_frontend/src/elaborator/expressions.rs @@ -546,6 +546,7 @@ impl<'context> Elaborator<'context> { typ: lhs_type.clone(), trait_id: trait_id.trait_id, trait_generics: Vec::new(), + span, }; self.trait_constraints.push((constraint, expr_id)); self.type_check_operator_method(expr_id, trait_id, &lhs_type, span); diff --git a/compiler/noirc_frontend/src/elaborator/mod.rs b/compiler/noirc_frontend/src/elaborator/mod.rs index dcdc37186b3..1655255ab6c 100644 --- a/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/compiler/noirc_frontend/src/elaborator/mod.rs @@ -595,7 +595,7 @@ impl<'context> Elaborator<'context> { }); } - Some(TraitConstraint { typ, trait_id, trait_generics }) + Some(TraitConstraint { typ, trait_id, trait_generics, span }) } /// Extract metadata from a NoirFunction @@ -890,7 +890,14 @@ impl<'context> Elaborator<'context> { if let Some(trait_id) = trait_impl.trait_id { self.generics = trait_impl.resolved_generics.clone(); - self.collect_trait_impl_methods(trait_id, trait_impl); + + let where_clause = trait_impl + .where_clause + .iter() + .flat_map(|item| self.resolve_trait_constraint(item)) + .collect::>(); + + self.collect_trait_impl_methods(trait_id, trait_impl, &where_clause); let span = trait_impl.object_type.span.expect("All trait self types should have spans"); self.declare_methods_on_struct(true, &mut trait_impl.methods, span); @@ -900,12 +907,6 @@ impl<'context> Elaborator<'context> { self.interner.set_function_trait(*func_id, self_type.clone(), trait_id); } - let where_clause = trait_impl - .where_clause - .iter() - .flat_map(|item| self.resolve_trait_constraint(item)) - .collect::>(); - let trait_generics = trait_impl.resolved_trait_generics.clone(); let resolved_trait_impl = Shared::new(TraitImpl { @@ -1040,6 +1041,7 @@ impl<'context> Elaborator<'context> { &mut self, trait_id: TraitId, trait_impl: &mut UnresolvedTraitImpl, + trait_impl_where_clause: &[TraitConstraint], ) { self.local_module = trait_impl.module_id; self.file = trait_impl.file_id; @@ -1096,6 +1098,34 @@ impl<'context> Elaborator<'context> { } } else { for (_, func_id, _) in &overrides { + let func_meta = self.interner.function_meta(func_id); + for override_trait_constraint in func_meta.trait_constraints.clone() { + // We allow where clauses on impls but during definition collection they are included as part of the where + // clause of each function. This check is necessary so we do not error about a + let override_constraint_is_from_impl = + trait_impl_where_clause.iter().any(|impl_constraint| { + impl_constraint.trait_id == override_trait_constraint.trait_id + }); + if override_constraint_is_from_impl { + continue; + } + + let override_constraint_is_on_trait_method = + method.trait_constraints.iter().any(|method_constraint| { + method_constraint.trait_id == override_trait_constraint.trait_id + }); + if !override_constraint_is_on_trait_method { + let the_trait = + self.interner.get_trait(override_trait_constraint.trait_id); + self.push_err(DefCollectorErrorKind::ImplIsStricterThanTrait { + constraint_typ: override_trait_constraint.typ, + constraint_name: the_trait.name.0.contents.clone(), + constraint_span: override_trait_constraint.span, + trait_method_name: method.name.0.contents.clone(), + trait_method_span: method.location.span, + }); + } + } func_ids_in_trait.insert(*func_id); } diff --git a/compiler/noirc_frontend/src/elaborator/traits.rs b/compiler/noirc_frontend/src/elaborator/traits.rs index 77ac8e476f8..7f4c69a7450 100644 --- a/compiler/noirc_frontend/src/elaborator/traits.rs +++ b/compiler/noirc_frontend/src/elaborator/traits.rs @@ -145,6 +145,7 @@ impl<'context> Elaborator<'context> { location: Location::new(name.span(), unresolved_trait.file_id), default_impl, default_impl_module_id: unresolved_trait.module_id, + trait_constraints: func_meta.trait_constraints.clone(), }); }); } diff --git a/compiler/noirc_frontend/src/elaborator/types.rs b/compiler/noirc_frontend/src/elaborator/types.rs index 06f7b0f9b6c..386359bbafe 100644 --- a/compiler/noirc_frontend/src/elaborator/types.rs +++ b/compiler/noirc_frontend/src/elaborator/types.rs @@ -401,6 +401,7 @@ impl<'context> Elaborator<'context> { generic.type_var.clone() })), trait_id, + span: path.span(), }; return Some((method, constraint, false)); @@ -435,6 +436,7 @@ impl<'context> Elaborator<'context> { generic.type_var.clone() })), trait_id, + span: path.span(), }; return Some((method, constraint, false)); } diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs index ab3724d3ef1..7998a6b5e70 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs @@ -11,7 +11,6 @@ use crate::ast::{ NoirStruct, NoirTrait, NoirTraitImpl, NoirTypeAlias, Pattern, TraitImplItem, TraitItem, TypeImpl, }; -use crate::macros_api::UnresolvedTypeData; use crate::{ graph::CrateId, hir::def_collector::dc_crate::{UnresolvedStruct, UnresolvedTrait}, @@ -170,7 +169,7 @@ impl<'a> ModCollector<'a> { impls: Vec, krate: CrateId, ) { - for mut trait_impl in impls { + for trait_impl in impls { let trait_name = trait_impl.trait_name.clone(); let mut unresolved_functions = @@ -181,23 +180,6 @@ impl<'a> ModCollector<'a> { for (_, func_id, noir_function) in &mut unresolved_functions.functions { // Attach any trait constraints on the impl to the function noir_function.def.where_clause.append(&mut trait_impl.where_clause.clone()); - - // If there are trait constraints on the trait impl methods that reference the impl generics - // they can be viewed as a constraint on the trait impl itself. - for generic in trait_impl.impl_generics.iter() { - let impl_generic_name = &generic.ident().0.contents; - for func_trait_constraint in noir_function.def.where_clause.iter() { - if let UnresolvedTypeData::Named(path, _, _) = - &func_trait_constraint.typ.typ - { - let trait_constraint_name = &path.last_segment().0.contents; - if trait_constraint_name == impl_generic_name { - trait_impl.where_clause.push(func_trait_constraint.clone()); - } - } - } - } - let location = Location::new(noir_function.def.span, self.file_id); context.def_interner.push_function(*func_id, &noir_function.def, module, location); } diff --git a/compiler/noirc_frontend/src/hir/def_collector/errors.rs b/compiler/noirc_frontend/src/hir/def_collector/errors.rs index af2264c3843..a92b9ec2b05 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/errors.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/errors.rs @@ -68,6 +68,14 @@ pub enum DefCollectorErrorKind { MacroError(MacroError), #[error("The only supported types of numeric generics are integers, fields, and booleans")] UnsupportedNumericGenericType { ident: Ident, typ: UnresolvedTypeData }, + #[error("impl has stricer requirements than trait")] + ImplIsStricterThanTrait { + constraint_typ: crate::Type, + constraint_name: String, + constraint_span: Span, + trait_method_name: String, + trait_method_span: Span, + }, } /// An error struct that macro processors can return. @@ -239,6 +247,19 @@ impl<'a> From<&'a DefCollectorErrorKind> for Diagnostic { ident.0.span(), ) } + DefCollectorErrorKind::ImplIsStricterThanTrait { constraint_typ, constraint_name: constraint, constraint_span, trait_method_name, trait_method_span } => { + // let constraint_name = &constraint_name.0.contents; + // let bound_name = bound_name; + + let mut diag = Diagnostic::simple_error( + "impl has stricter requirements than trait".to_string(), + format!("impl has extra requirement `{constraint_typ}: {constraint}`"), + *constraint_span, + ); + // diag.add_note(message) + diag.add_secondary(format!("definition of {trait_method_name} from trait"), *trait_method_span); + diag + } } } } diff --git a/compiler/noirc_frontend/src/hir/resolution/resolver.rs b/compiler/noirc_frontend/src/hir/resolution/resolver.rs index ebb3f7290ea..4005f079050 100644 --- a/compiler/noirc_frontend/src/hir/resolution/resolver.rs +++ b/compiler/noirc_frontend/src/hir/resolution/resolver.rs @@ -544,7 +544,7 @@ impl<'a> Resolver<'a> { }); } - Some(TraitConstraint { typ, trait_id, trait_generics }) + Some(TraitConstraint { typ, trait_id, trait_generics, span }) } /// Translates an UnresolvedType into a Type and appends any @@ -1921,6 +1921,7 @@ impl<'a> Resolver<'a> { generic.type_var.clone() })), trait_id, + span: path.span(), }; return Some((method, constraint, false)); } @@ -1938,6 +1939,7 @@ impl<'a> Resolver<'a> { let mut trait_path = path.clone(); trait_path.pop(); + let span = trait_path.span(); let trait_id = self.lookup(trait_path).ok()?; let the_trait = self.interner.get_trait(trait_id); @@ -1951,6 +1953,7 @@ impl<'a> Resolver<'a> { generic.type_var.clone() })), trait_id, + span, }; return Some((method, constraint, false)); } @@ -1992,6 +1995,7 @@ impl<'a> Resolver<'a> { trait_generics: vecmap(trait_bound.trait_generics, |typ| { self.resolve_type(typ) }), + span: trait_bound.trait_path.span(), }; return Some((method, constraint, true)); } diff --git a/compiler/noirc_frontend/src/hir/resolution/traits.rs b/compiler/noirc_frontend/src/hir/resolution/traits.rs index a6b732439b0..801706bc758 100644 --- a/compiler/noirc_frontend/src/hir/resolution/traits.rs +++ b/compiler/noirc_frontend/src/hir/resolution/traits.rs @@ -180,6 +180,8 @@ fn resolve_trait_methods( location: Location::new(name.span(), unresolved_trait.file_id), default_impl, default_impl_module_id: unresolved_trait.module_id, + // This field is only used by the elaborator + trait_constraints: vec![], }); let errors = resolver.take_errors().into_iter(); diff --git a/compiler/noirc_frontend/src/hir/type_check/expr.rs b/compiler/noirc_frontend/src/hir/type_check/expr.rs index 77861a6d8f8..5b67c547c07 100644 --- a/compiler/noirc_frontend/src/hir/type_check/expr.rs +++ b/compiler/noirc_frontend/src/hir/type_check/expr.rs @@ -163,6 +163,8 @@ impl<'interner> TypeChecker<'interner> { typ: lhs_type.clone(), trait_id: id.trait_id, trait_generics: Vec::new(), + // This field is used by the elaborator + span: Span::default(), }; self.trait_constraints.push((constraint, *expr_id)); self.typecheck_operator_method(*expr_id, id, &lhs_type, span); diff --git a/compiler/noirc_frontend/src/hir_def/expr.rs b/compiler/noirc_frontend/src/hir_def/expr.rs index 8de4f118774..655d537ffde 100644 --- a/compiler/noirc_frontend/src/hir_def/expr.rs +++ b/compiler/noirc_frontend/src/hir_def/expr.rs @@ -227,6 +227,7 @@ impl HirMethodCallExpression { typ: object_type, trait_id: method_id.trait_id, trait_generics: generics.clone(), + span: location.span, }; (id, ImplKind::TraitMethod(*method_id, constraint, false)) } diff --git a/compiler/noirc_frontend/src/hir_def/traits.rs b/compiler/noirc_frontend/src/hir_def/traits.rs index e4959cb3dd9..68b5cce577d 100644 --- a/compiler/noirc_frontend/src/hir_def/traits.rs +++ b/compiler/noirc_frontend/src/hir_def/traits.rs @@ -16,6 +16,7 @@ pub struct TraitFunction { pub location: Location, pub default_impl: Option>, pub default_impl_module_id: crate::hir::def_map::LocalModuleId, + pub trait_constraints: Vec, } #[derive(Clone, Debug, PartialEq, Eq)] @@ -82,16 +83,17 @@ pub struct TraitImpl { pub where_clause: Vec, } -#[derive(Debug, Clone)] +#[derive(Debug, Clone, PartialEq, Eq)] pub struct TraitConstraint { pub typ: Type, pub trait_id: TraitId, pub trait_generics: Vec, + pub span: Span, } impl TraitConstraint { - pub fn new(typ: Type, trait_id: TraitId, trait_generics: Vec) -> Self { - Self { typ, trait_id, trait_generics } + pub fn new(typ: Type, trait_id: TraitId, trait_generics: Vec, span: Span) -> Self { + Self { typ, trait_id, trait_generics, span } } pub fn apply_bindings(&mut self, type_bindings: &TypeBindings) { diff --git a/compiler/noirc_frontend/src/node_interner.rs b/compiler/noirc_frontend/src/node_interner.rs index c7bbd6ad044..f284bc19f00 100644 --- a/compiler/noirc_frontend/src/node_interner.rs +++ b/compiler/noirc_frontend/src/node_interner.rs @@ -1258,8 +1258,14 @@ impl NodeInterner { type_bindings: &mut TypeBindings, recursion_limit: u32, ) -> Result> { - let make_constraint = - || TraitConstraint::new(object_type.clone(), trait_id, trait_generics.to_vec()); + let make_constraint = || { + TraitConstraint::new( + object_type.clone(), + trait_id, + trait_generics.to_vec(), + Span::default(), + ) + }; // Prevent infinite recursion when looking for impls if recursion_limit == 0 { diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index 69cc032aebc..dd7c647ec10 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -1898,10 +1898,9 @@ fn quote_code_fragments() { } #[test] -fn impl_not_found_for_trait_impl_method_trait_constraint() { - // This test ensures that we still error with that no matching impl was found - // if a where clause referencing an impl generic only exists on a trait impl's - // methods. +fn impl_stricter_than_trait() { + // This test ensures that the error we get from the where clause on the trait impl method + // is a `DefCollectorErrorKind::ImplIsStricterThanTrait` error. let src = r#" trait Serialize { fn serialize(self) -> [Field; N]; @@ -1937,6 +1936,52 @@ fn impl_not_found_for_trait_impl_method_trait_constraint() { } "#; + let errors = get_program_errors(src); + assert_eq!(errors.len(), 1); + assert!(matches!( + &errors[0].0, + CompilationError::DefinitionError(DefCollectorErrorKind::ImplIsStricterThanTrait { .. }) + )); +} + +#[test] +fn impl_not_found_for_inner_impl() { + // We want to guarantee that we get a no impl found error + let src = r#" + trait Serialize { + fn serialize(self) -> [Field; N]; + } + + trait ToField { + fn to_field(self) -> Field; + } + + fn process_array(array: [Field; N]) -> Field { + array[0] + } + + fn serialize_thing(thing: A) -> [Field; N] where A: Serialize { + thing.serialize() + } + + struct MyType { + a: T, + b: T, + } + + impl Serialize<2> for MyType where T: ToField { + fn serialize(self) -> [Field; 2] { + [ self.a.to_field(), self.b.to_field() ] + } + } + + impl MyType { + fn do_thing_with_serialization_with_extra_steps(self) -> Field { + process_array(serialize_thing(self)) + } + } + "#; + let errors = get_program_errors(src); assert_eq!(errors.len(), 1); assert!(matches!( diff --git a/test_programs/compile_failure/impl_stricter_than_trait/Nargo.toml b/test_programs/compile_failure/impl_stricter_than_trait/Nargo.toml new file mode 100644 index 00000000000..00d63cd46e0 --- /dev/null +++ b/test_programs/compile_failure/impl_stricter_than_trait/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "impl_stricter_than_trait" +type = "bin" +authors = [""] +compiler_version = ">=0.31.0" + +[dependencies] \ No newline at end of file diff --git a/test_programs/compile_failure/impl_stricter_than_trait/src/main.nr b/test_programs/compile_failure/impl_stricter_than_trait/src/main.nr new file mode 100644 index 00000000000..6d2660af450 --- /dev/null +++ b/test_programs/compile_failure/impl_stricter_than_trait/src/main.nr @@ -0,0 +1,32 @@ +trait Serialize { + fn serialize(self) -> [Field; N]; +} + +trait ToField { + fn to_field(self) -> Field; +} + +fn process_array(array: [Field; N]) -> Field { + array[0] +} + +fn serialize_thing(thing: A) -> [Field; N] where A: Serialize { + thing.serialize() +} + +struct MyType { + a: T, + b: T, +} + +impl Serialize<2> for MyType { + fn serialize(self) -> [Field; 2] where T: ToField { + [ self.a.to_field(), self.b.to_field() ] + } + } + +impl MyType { + fn do_thing_with_serialization_with_extra_steps(self) -> Field { + process_array(serialize_thing(self)) + } +} From 1ba38f5c30f65ef3548d16449bdfa409a3d0972c Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 26 Jun 2024 20:24:35 +0000 Subject: [PATCH 03/27] master merge --- Cargo.lock | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 0511d4e75ce..d2afe025c69 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3416,9 +3416,9 @@ dependencies = [ [[package]] name = "proptest" -version = "1.3.1" +version = "1.4.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "7c003ac8c77cb07bb74f5f198bce836a689bcd5a42574612bf14d17bfd08c20e" +checksum = "31b476131c3c86cb68032fdc5cb6d5a1045e3e42d96b69fa599fd77701e1f5bf" dependencies = [ "bit-set", "bit-vec", @@ -3428,7 +3428,7 @@ dependencies = [ "rand 0.8.5", "rand_chacha 0.3.1", "rand_xorshift", - "regex-syntax 0.7.4", + "regex-syntax 0.8.2", "rusty-fork", "tempfile", "unarray", @@ -3676,12 +3676,6 @@ version = "0.6.29" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f162c6dd7b008981e4d40210aca20b4bd0f9b60ca9271061b07f78537722f2e1" -[[package]] -name = "regex-syntax" -version = "0.7.4" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e5ea92a5b6195c6ef2a0295ea818b312502c6fc94dde986c5553242e18fd4ce2" - [[package]] name = "regex-syntax" version = "0.8.2" From 762ea5850fae1551f1243bf2869ea5c9f34257a7 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 26 Jun 2024 20:25:59 +0000 Subject: [PATCH 04/27] nargo fmt --- .../impl_stricter_than_trait/src/main.nr | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test_programs/compile_failure/impl_stricter_than_trait/src/main.nr b/test_programs/compile_failure/impl_stricter_than_trait/src/main.nr index 6d2660af450..81a9884e324 100644 --- a/test_programs/compile_failure/impl_stricter_than_trait/src/main.nr +++ b/test_programs/compile_failure/impl_stricter_than_trait/src/main.nr @@ -20,13 +20,13 @@ struct MyType { } impl Serialize<2> for MyType { - fn serialize(self) -> [Field; 2] where T: ToField { - [ self.a.to_field(), self.b.to_field() ] - } + fn serialize(self) -> [Field; 2] where T: ToField { + [ self.a.to_field(), self.b.to_field() ] } +} impl MyType { - fn do_thing_with_serialization_with_extra_steps(self) -> Field { + fn do_thing_with_serialization_with_extra_steps(self) -> Field { process_array(serialize_thing(self)) } -} +} \ No newline at end of file From 6f5ef1eb805db86752a08bf5251f4f6efaa8cec5 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 26 Jun 2024 20:26:13 +0000 Subject: [PATCH 05/27] reduce test --- compiler/noirc_frontend/src/tests.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index dd7c647ec10..cb5ad2d4919 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -1930,7 +1930,7 @@ fn impl_stricter_than_trait() { } impl MyType { - fn do_thing_with_serialization_with_extra_steps(self) -> Field { + fn do_thing_with_serialization_with_extra_steps(self) -> Field { process_array(serialize_thing(self)) } } @@ -1976,7 +1976,7 @@ fn impl_not_found_for_inner_impl() { } impl MyType { - fn do_thing_with_serialization_with_extra_steps(self) -> Field { + fn do_thing_with_serialization_with_extra_steps(self) -> Field { process_array(serialize_thing(self)) } } From 2dcc646307ec28275eff469ee0ea35df828ec829 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 26 Jun 2024 20:26:57 +0000 Subject: [PATCH 06/27] improve error and fix typo --- compiler/noirc_frontend/src/hir/def_collector/errors.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/compiler/noirc_frontend/src/hir/def_collector/errors.rs b/compiler/noirc_frontend/src/hir/def_collector/errors.rs index a92b9ec2b05..56b693780ff 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/errors.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/errors.rs @@ -68,7 +68,7 @@ pub enum DefCollectorErrorKind { MacroError(MacroError), #[error("The only supported types of numeric generics are integers, fields, and booleans")] UnsupportedNumericGenericType { ident: Ident, typ: UnresolvedTypeData }, - #[error("impl has stricer requirements than trait")] + #[error("impl has stricter requirements than trait")] ImplIsStricterThanTrait { constraint_typ: crate::Type, constraint_name: String, @@ -257,7 +257,7 @@ impl<'a> From<&'a DefCollectorErrorKind> for Diagnostic { *constraint_span, ); // diag.add_note(message) - diag.add_secondary(format!("definition of {trait_method_name} from trait"), *trait_method_span); + diag.add_secondary(format!("definition of `{trait_method_name}` from trait"), *trait_method_span); diag } } From 86ea7b51061d3e5daf6de10d72195ae5366f326d Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 26 Jun 2024 20:29:20 +0000 Subject: [PATCH 07/27] cleanup comment --- compiler/noirc_frontend/src/elaborator/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_frontend/src/elaborator/mod.rs b/compiler/noirc_frontend/src/elaborator/mod.rs index 3f303bb1cff..0056079b669 100644 --- a/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/compiler/noirc_frontend/src/elaborator/mod.rs @@ -1086,7 +1086,7 @@ impl<'context> Elaborator<'context> { let func_meta = self.interner.function_meta(func_id); for override_trait_constraint in func_meta.trait_constraints.clone() { // We allow where clauses on impls but during definition collection they are included as part of the where - // clause of each function. This check is necessary so we do not error about a + // clause of each function. This check is necessary so we do not unnecessary error on trait impl where clauses. let override_constraint_is_from_impl = trait_impl_where_clause.iter().any(|impl_constraint| { impl_constraint.trait_id == override_trait_constraint.trait_id From d9df0af6f70bdfb33d67badbc15dd0c1b9f8ee36 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 26 Jun 2024 20:29:55 +0000 Subject: [PATCH 08/27] more cleanup --- compiler/noirc_frontend/src/hir/def_collector/errors.rs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/compiler/noirc_frontend/src/hir/def_collector/errors.rs b/compiler/noirc_frontend/src/hir/def_collector/errors.rs index 56b693780ff..b160bf9c761 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/errors.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/errors.rs @@ -248,15 +248,11 @@ impl<'a> From<&'a DefCollectorErrorKind> for Diagnostic { ) } DefCollectorErrorKind::ImplIsStricterThanTrait { constraint_typ, constraint_name: constraint, constraint_span, trait_method_name, trait_method_span } => { - // let constraint_name = &constraint_name.0.contents; - // let bound_name = bound_name; - let mut diag = Diagnostic::simple_error( "impl has stricter requirements than trait".to_string(), format!("impl has extra requirement `{constraint_typ}: {constraint}`"), *constraint_span, ); - // diag.add_note(message) diag.add_secondary(format!("definition of `{trait_method_name}` from trait"), *trait_method_span); diag } From dea35f4185bb9d564c608778490fc1a310b34a88 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 26 Jun 2024 20:50:18 +0000 Subject: [PATCH 09/27] fix regression_4635 --- .../compile_success_empty/regression_4635/src/main.nr | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test_programs/compile_success_empty/regression_4635/src/main.nr b/test_programs/compile_success_empty/regression_4635/src/main.nr index 350b60ba3f7..75188f797dd 100644 --- a/test_programs/compile_success_empty/regression_4635/src/main.nr +++ b/test_programs/compile_success_empty/regression_4635/src/main.nr @@ -42,8 +42,8 @@ struct MyStruct { a: T } -impl Deserialize<1> for MyStruct { - fn deserialize(fields: [Field; 1]) -> Self where T: FromField { +impl Deserialize<1> for MyStruct where T: FromField { + fn deserialize(fields: [Field; 1]) -> Self { Self{ a: FromField::from_field(fields[0]) } } } From 18aad5d0bcd6e9afe756f4869177877764422616 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Tue, 9 Jul 2024 11:11:53 -0400 Subject: [PATCH 10/27] Update compiler/noirc_frontend/src/elaborator/mod.rs --- compiler/noirc_frontend/src/elaborator/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_frontend/src/elaborator/mod.rs b/compiler/noirc_frontend/src/elaborator/mod.rs index 4431458ce06..1f54b23ef46 100644 --- a/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/compiler/noirc_frontend/src/elaborator/mod.rs @@ -1109,7 +1109,7 @@ impl<'context> Elaborator<'context> { let func_meta = self.interner.function_meta(func_id); for override_trait_constraint in func_meta.trait_constraints.clone() { // We allow where clauses on impls but during definition collection they are included as part of the where - // clause of each function. This check is necessary so we do not unnecessary error on trait impl where clauses. + // clause of each function. This check is necessary so we do not unnecessarily error on trait impl where clauses. let override_constraint_is_from_impl = trait_impl_where_clause.iter().any(|impl_constraint| { impl_constraint.trait_id == override_trait_constraint.trait_id From 4abf257f04d2ca631941c73788919ca0df72ce98 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 10 Jul 2024 15:52:44 +0000 Subject: [PATCH 11/27] initial work to determine if impl is stricter than trait for different generics --- compiler/noirc_frontend/src/elaborator/mod.rs | 158 ++++++++++++++---- .../noirc_frontend/src/elaborator/traits.rs | 7 +- .../src/hir/def_collector/dc_crate.rs | 4 +- .../src/hir/resolution/traits.rs | 3 +- compiler/noirc_frontend/src/hir_def/traits.rs | 1 + compiler/noirc_frontend/src/tests.rs | 24 +++ .../impl_stricter_than_trait/src/main.nr | 14 ++ 7 files changed, 177 insertions(+), 34 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/mod.rs b/compiler/noirc_frontend/src/elaborator/mod.rs index bd82bfec6f7..fdcfaf218f3 100644 --- a/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/compiler/noirc_frontend/src/elaborator/mod.rs @@ -22,7 +22,7 @@ use crate::{ hir_def::{ expr::HirIdent, function::{FunctionBody, Parameters}, - traits::TraitConstraint, + traits::{TraitConstraint, TraitFunction}, types::{Generics, Kind, ResolvedGeneric}, }, macros_api::{ @@ -1068,7 +1068,8 @@ impl<'context> Elaborator<'context> { // while also mutating the interner let the_trait = self.interner.get_trait_mut(trait_id); let methods = std::mem::take(&mut the_trait.methods); - + // dbg!(the_trait.generics.clone()); + // dbg!(trait_impl.resolved_generics.clone()); for method in &methods { let overrides: Vec<_> = trait_impl .methods @@ -1106,34 +1107,8 @@ impl<'context> Elaborator<'context> { } } else { for (_, func_id, _) in &overrides { - let func_meta = self.interner.function_meta(func_id); - for override_trait_constraint in func_meta.trait_constraints.clone() { - // We allow where clauses on impls but during definition collection they are included as part of the where - // clause of each function. This check is necessary so we do not unnecessarily error on trait impl where clauses. - let override_constraint_is_from_impl = - trait_impl_where_clause.iter().any(|impl_constraint| { - impl_constraint.trait_id == override_trait_constraint.trait_id - }); - if override_constraint_is_from_impl { - continue; - } + self.check_where_clause_against_trait(func_id, method, trait_impl_where_clause, &trait_impl.resolved_generics); - let override_constraint_is_on_trait_method = - method.trait_constraints.iter().any(|method_constraint| { - method_constraint.trait_id == override_trait_constraint.trait_id - }); - if !override_constraint_is_on_trait_method { - let the_trait = - self.interner.get_trait(override_trait_constraint.trait_id); - self.push_err(DefCollectorErrorKind::ImplIsStricterThanTrait { - constraint_typ: override_trait_constraint.typ, - constraint_name: the_trait.name.0.contents.clone(), - constraint_span: override_trait_constraint.span, - trait_method_name: method.name.0.contents.clone(), - trait_method_span: method.location.span, - }); - } - } func_ids_in_trait.insert(*func_id); } @@ -1168,6 +1143,131 @@ impl<'context> Elaborator<'context> { trait_impl.methods.trait_id = Some(trait_id); } + /// Issue an error if the impl is stricter than the trait. + /// + /// # Example + /// + /// ``` + /// trait MyTrait { } + /// trait Foo { + /// fn foo(); + /// } + /// impl Foo for () { + /// // Error issued here as `foo` does not have the `MyTrait` constraint + /// fn foo() where B: MyTrait {} + /// } + /// ``` + fn check_where_clause_against_trait( + &mut self, + func_id: &FuncId, + method: &TraitFunction, + trait_impl_where_clause: &[TraitConstraint], + trait_impl_generics: &Generics, + ) { + let func_meta = self.interner.function_meta(func_id); + // dbg!(func_meta.trait_constraints.clone()); + // dbg!(trait_impl_where_clause); + // dbg!(method.typ.clone()); + let override_generics = func_meta.all_generics.clone(); + // dbg!(override_generics.clone()); + // dbg!(func_meta.direct_generics.clone()); + // dbg!(method.trait_constraints.clone()); + // dbg!(method.all_generics.clone()); + let method_generics = method.all_generics.clone(); + if self.interner.function_name(func_id) == "hash" && override_generics.len() == 2 { + dbg!(override_generics.clone()); + dbg!(method.all_generics.clone()); + dbg!(trait_impl_generics.clone()); + } + + for override_trait_constraint in func_meta.trait_constraints.clone() { + match &override_trait_constraint.typ { + Type::NamedGeneric(_, name, _) => { + // dbg!(name); + } + _ => { + dbg!(override_trait_constraint.typ.clone()); + } + } + // self.unify(actual, expected, make_error) + // We allow where clauses on impls but during definition collection they are included as part of the where + // clause of each function. This check is necessary so we do not unnecessarily error on trait impl where clauses. + let override_constraint_is_from_impl = + trait_impl_where_clause.iter().any(|impl_constraint| { + impl_constraint.trait_id == override_trait_constraint.trait_id + }); + + // dbg!(override_constraint_is_from_impl); + if override_constraint_is_from_impl { + continue; + } + + let override_constraint_is_on_trait_method = + method.trait_constraints.iter().any(|method_constraint| { + method_constraint.trait_id == override_trait_constraint.trait_id + }); + + let method_constraint_typ_name = method.trait_constraints.iter().find(|method_constraint| { + method_constraint.trait_id == override_trait_constraint.trait_id + }).map(|found_constraint| { + let typ_as_string = found_constraint.typ.to_string(); + // dbg!(typ_as_string.clone()); + // let index = override_generics.iter().position(|generic| generic.name.as_str() == typ_as_string.as_str()); + // dbg!(index) + typ_as_string + }); + let override_constraint_typ_name = override_trait_constraint.typ.to_string(); + // dbg!(override_constraint_typ_name.clone()); + let override_generic_index = override_generics.iter().position(|generic| generic.name.as_str() == override_constraint_typ_name.as_str()); + if let (Some(method_constraint_typ_name), Some(override_generic_index)) = (method_constraint_typ_name, override_generic_index) { + // dbg!(method_constraint_typ_name.clone()); + dbg!(override_generic_index); + let mut method_generic_index = None; + let mut index = 0; + for method_generic in method_generics.iter() { + if method_generic.name.as_str() == "Self" { + continue; + } + + if method_generic.name.as_str() == method_constraint_typ_name.as_str() { + method_generic_index = Some(index + trait_impl_generics.len()); + break; + } + + index += 1; + } + dbg!(method_generic_index.clone()); + let method_generic_index = method_generic_index.expect("Should have a method generic index"); + if method_generic_index != override_generic_index { + // dbg!() + let the_trait = + self.interner.get_trait(override_trait_constraint.trait_id); + self.push_err(DefCollectorErrorKind::ImplIsStricterThanTrait { + constraint_typ: override_trait_constraint.typ, + constraint_name: the_trait.name.0.contents.clone(), + constraint_span: override_trait_constraint.span, + trait_method_name: method.name.0.contents.clone(), + trait_method_span: method.location.span, + }); + } + } + // dbg!(override_constraint_typ_name.clone()); + + // dbg!(override_constraint_is_on_trait_method); + // if !override_constraint_is_on_trait_method { + // let the_trait = + // self.interner.get_trait(override_trait_constraint.trait_id); + // self.push_err(DefCollectorErrorKind::ImplIsStricterThanTrait { + // constraint_typ: override_trait_constraint.typ, + // constraint_name: the_trait.name.0.contents.clone(), + // constraint_span: override_trait_constraint.span, + // trait_method_name: method.name.0.contents.clone(), + // trait_method_span: method.location.span, + // }); + // } + } + } + fn check_trait_impl_crate_coherence( &mut self, trait_id: TraitId, diff --git a/compiler/noirc_frontend/src/elaborator/traits.rs b/compiler/noirc_frontend/src/elaborator/traits.rs index 7f4c69a7450..d1309176c3f 100644 --- a/compiler/noirc_frontend/src/elaborator/traits.rs +++ b/compiler/noirc_frontend/src/elaborator/traits.rs @@ -119,8 +119,10 @@ impl<'context> Elaborator<'context> { let arguments = vecmap(&func_meta.parameters.0, |(_, typ, _)| typ.clone()); let return_type = func_meta.return_type().clone(); - - let generics = vecmap(&this.generics, |generic| generic.type_var.clone()); + // dbg!(this.generics.clone()); + // dbg!(func_meta.all_generics.clone()); + // dbg!(func_meta.direct_generics.clone()); + let generics = vecmap(&this.generics.clone(), |generic| generic.type_var.clone()); let default_impl_list: Vec<_> = unresolved_trait .fns_with_default_impl @@ -146,6 +148,7 @@ impl<'context> Elaborator<'context> { default_impl, default_impl_module_id: unresolved_trait.module_id, trait_constraints: func_meta.trait_constraints.clone(), + all_generics: func_meta.all_generics.clone(), }); }); } diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs index 390f6528592..03fe1a2777b 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs @@ -5,7 +5,7 @@ use crate::graph::CrateId; use crate::hir::comptime::{Interpreter, InterpreterError}; use crate::hir::def_map::{CrateDefMap, LocalModuleId, ModuleId}; use crate::hir::resolution::errors::ResolverError; -use crate::{ResolvedGeneric, Type}; +use crate::{Generics, ResolvedGeneric, Type}; use crate::hir::resolution::import::{resolve_import, ImportDirective, PathResolution}; use crate::hir::resolution::{ @@ -126,7 +126,7 @@ pub struct UnresolvedTraitImpl { pub trait_id: Option, pub impl_id: Option, pub resolved_object_type: Option, - pub resolved_generics: Vec, + pub resolved_generics: Generics, // The resolved generic on the trait itself. E.g. it is the `` in // `impl Foo for Bar { ... }` diff --git a/compiler/noirc_frontend/src/hir/resolution/traits.rs b/compiler/noirc_frontend/src/hir/resolution/traits.rs index f358b7f9f37..63c962fffb5 100644 --- a/compiler/noirc_frontend/src/hir/resolution/traits.rs +++ b/compiler/noirc_frontend/src/hir/resolution/traits.rs @@ -178,8 +178,9 @@ fn resolve_trait_methods( location: Location::new(name.span(), unresolved_trait.file_id), default_impl, default_impl_module_id: unresolved_trait.module_id, - // This field is only used by the elaborator + // These fields are only used by the elaborator trait_constraints: vec![], + all_generics: vec![], }); let errors = resolver.take_errors().into_iter(); diff --git a/compiler/noirc_frontend/src/hir_def/traits.rs b/compiler/noirc_frontend/src/hir_def/traits.rs index a0f1081d998..b4a44947bb9 100644 --- a/compiler/noirc_frontend/src/hir_def/traits.rs +++ b/compiler/noirc_frontend/src/hir_def/traits.rs @@ -17,6 +17,7 @@ pub struct TraitFunction { pub default_impl: Option>, pub default_impl_module_id: crate::hir::def_map::LocalModuleId, pub trait_constraints: Vec, + pub all_generics: Generics, } #[derive(Clone, Debug, PartialEq, Eq)] diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index 26d56014670..2bbf249481e 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -1944,6 +1944,30 @@ fn impl_stricter_than_trait() { )); } +#[test] +fn impl_stricter_than_trait_different_generics() { + let src = r#" + trait Default { } + + // Object type differs + trait Foo { + fn foo() where T: Default; + // fn foo(); + } + + impl Foo for () { + fn foo() where B: Default {} + // fn foo() where A: Default {} + + // fn foo() {} + } + "#; + let errors = get_program_errors(src); + dbg!(errors.clone()); + + assert_eq!(errors.len(), 1); +} + #[test] fn impl_not_found_for_inner_impl() { // We want to guarantee that we get a no impl found error diff --git a/test_programs/compile_failure/impl_stricter_than_trait/src/main.nr b/test_programs/compile_failure/impl_stricter_than_trait/src/main.nr index 81a9884e324..587c22da65e 100644 --- a/test_programs/compile_failure/impl_stricter_than_trait/src/main.nr +++ b/test_programs/compile_failure/impl_stricter_than_trait/src/main.nr @@ -29,4 +29,18 @@ impl MyType { fn do_thing_with_serialization_with_extra_steps(self) -> Field { process_array(serialize_thing(self)) } +} + +trait MyTrait { } + +// Object type differs +trait Foo { + fn foo() where U: MyTrait; + // fn foo(); +} + +impl Foo for () { + fn foo() where B: MyTrait {} + // fn foo() where A: Default {} + // fn foo() {} } \ No newline at end of file From b99e6c900fa6565fd52a65a8b47a3deb4e31928e Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 11 Jul 2024 03:32:16 +0000 Subject: [PATCH 12/27] new strategy using substitute of new type bindings, only works for named generics need to generalize it for all types --- compiler/noirc_frontend/src/elaborator/mod.rs | 343 ++++++++++++++---- .../noirc_frontend/src/elaborator/traits.rs | 6 +- compiler/noirc_frontend/src/hir_def/types.rs | 2 +- compiler/noirc_frontend/src/tests.rs | 80 +++- .../impl_stricter_than_trait/src/main.nr | 30 +- 5 files changed, 379 insertions(+), 82 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/mod.rs b/compiler/noirc_frontend/src/elaborator/mod.rs index fdcfaf218f3..2092ee9df4d 100644 --- a/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/compiler/noirc_frontend/src/elaborator/mod.rs @@ -1,5 +1,5 @@ use std::{ - collections::{BTreeMap, BTreeSet}, + collections::{BTreeMap, BTreeSet, HashMap as TypeBindingsMap}, rc::Rc, }; @@ -34,7 +34,7 @@ use crate::{ TypeAliasId, }, parser::TopLevelStatement, - Shared, Type, TypeBindings, TypeVariable, + Shared, Type, TypeBindings, TypeVariable, TypeVariableId, }; use crate::{ ast::{TraitBound, UnresolvedGeneric, UnresolvedGenerics}, @@ -1107,7 +1107,18 @@ impl<'context> Elaborator<'context> { } } else { for (_, func_id, _) in &overrides { - self.check_where_clause_against_trait(func_id, method, trait_impl_where_clause, &trait_impl.resolved_generics); + self.check_where_clause_against_trait( + func_id, + method, + trait_impl_where_clause, + &trait_impl.resolved_generics, + ); + self.check_where_clause_against_trait_new( + func_id, + method, + trait_impl_where_clause, + &trait_impl.resolved_generics, + ); func_ids_in_trait.insert(*func_id); } @@ -1143,10 +1154,10 @@ impl<'context> Elaborator<'context> { trait_impl.methods.trait_id = Some(trait_id); } - /// Issue an error if the impl is stricter than the trait. - /// + /// Issue an error if the impl is stricter than the trait. + /// /// # Example - /// + /// /// ``` /// trait MyTrait { } /// trait Foo { @@ -1174,29 +1185,19 @@ impl<'context> Elaborator<'context> { // dbg!(method.trait_constraints.clone()); // dbg!(method.all_generics.clone()); let method_generics = method.all_generics.clone(); - if self.interner.function_name(func_id) == "hash" && override_generics.len() == 2 { - dbg!(override_generics.clone()); - dbg!(method.all_generics.clone()); - dbg!(trait_impl_generics.clone()); - } + // if self.interner.function_name(func_id) == "hash" && (override_generics.len() == 1 || override_generics.len() == 2) { + // dbg!(override_generics.clone()); + // dbg!(method.all_generics.clone()); + // dbg!(trait_impl_generics.clone()); + // } for override_trait_constraint in func_meta.trait_constraints.clone() { - match &override_trait_constraint.typ { - Type::NamedGeneric(_, name, _) => { - // dbg!(name); - } - _ => { - dbg!(override_trait_constraint.typ.clone()); - } - } - // self.unify(actual, expected, make_error) // We allow where clauses on impls but during definition collection they are included as part of the where // clause of each function. This check is necessary so we do not unnecessarily error on trait impl where clauses. let override_constraint_is_from_impl = trait_impl_where_clause.iter().any(|impl_constraint| { impl_constraint.trait_id == override_trait_constraint.trait_id }); - // dbg!(override_constraint_is_from_impl); if override_constraint_is_from_impl { continue; @@ -1207,41 +1208,146 @@ impl<'context> Elaborator<'context> { method_constraint.trait_id == override_trait_constraint.trait_id }); - let method_constraint_typ_name = method.trait_constraints.iter().find(|method_constraint| { - method_constraint.trait_id == override_trait_constraint.trait_id - }).map(|found_constraint| { - let typ_as_string = found_constraint.typ.to_string(); - // dbg!(typ_as_string.clone()); - // let index = override_generics.iter().position(|generic| generic.name.as_str() == typ_as_string.as_str()); - // dbg!(index) - typ_as_string - }); + let method_generic_index = method + .trait_constraints + .iter() + .find(|method_constraint| { + // TODO: need to change how this check is done as we want to check whether the type is the same + // and the trait is different as well + method_constraint.trait_id == override_trait_constraint.trait_id + }) + .map(|method_constraint| { + dbg!(method_constraint.clone()); + // TODO: this needs to be made a recursive check + match (&method_constraint.typ, &override_trait_constraint.typ) { + ( + Type::NamedGeneric(_, found_name, _), + Type::NamedGeneric(_, override_name, _), + ) => { + dbg!(found_name); + dbg!(override_name); + } + ( + Type::Struct(method_struct, method_struct_generics), + Type::Struct(override_struct, override_struct_generics), + ) => { + dbg!(method_struct.borrow().id); + dbg!(override_struct.borrow().id); + if method_struct.borrow().id == override_struct.borrow().id { + // for method_generic in method_struct_generics.iter() { + // dbg!(method_generic.to_string()); + // } + // for override_generic in override_struct_generics.iter() { + // dbg!(override_generic.to_string()); + // } + for (method_struct_generic, override_struct_generic) in + method_struct_generics.iter().zip(override_struct_generics) + { + let method_constraint_typ_name = + method_struct_generic.to_string(); + let mut method_generic_index = None; + let mut index = 0; + for method_generic in method_generics.iter() { + if method_generic.name.as_str() == "Self" { + continue; + } + + if method_generic.name.as_str() + == method_constraint_typ_name.as_str() + { + method_generic_index = Some(index); + break; + } + + index += 1; + } + + let override_struct_generic_name = + override_struct_generic.to_string(); + dbg!(override_struct_generic_name.clone()); + let override_generic_index = + override_generics.iter().position(|generic| { + generic.name.as_str() + == override_struct_generic_name.as_str() + }); + dbg!(method_generic_index); + dbg!(override_generic_index); + if let ( + Some(method_generic_index), + Some(override_generic_index), + ) = (method_generic_index, override_generic_index) + { + dbg!(override_generic_index); + + if method_generic_index != override_generic_index { + let the_trait = self + .interner + .get_trait(override_trait_constraint.trait_id); + self.push_err( + DefCollectorErrorKind::ImplIsStricterThanTrait { + constraint_typ: override_trait_constraint + .typ + .clone(), + constraint_name: the_trait + .name + .0 + .contents + .clone(), + constraint_span: override_trait_constraint.span, + trait_method_name: method + .name + .0 + .contents + .clone(), + trait_method_span: method.location.span, + }, + ); + } + } + } + } + } + _ => { + dbg!(method_constraint.typ.clone()); + dbg!(override_trait_constraint.typ.clone()); + } + } + + let method_constraint_typ_name = method_constraint.typ.to_string(); + // dbg!(typ_as_string.clone()); + // let index = override_generics.iter().position(|generic| generic.name.as_str() == typ_as_string.as_str()); + // dbg!(index) + // method_constraint_typ_name + let mut method_generic_index = None; + let mut index = 0; + for method_generic in method_generics.iter() { + if method_generic.name.as_str() == "Self" { + continue; + } + + if method_generic.name.as_str() == method_constraint_typ_name.as_str() { + method_generic_index = Some(index); + break; + } + + index += 1; + } + method_generic_index + }) + .flatten(); + let override_constraint_typ_name = override_trait_constraint.typ.to_string(); // dbg!(override_constraint_typ_name.clone()); - let override_generic_index = override_generics.iter().position(|generic| generic.name.as_str() == override_constraint_typ_name.as_str()); - if let (Some(method_constraint_typ_name), Some(override_generic_index)) = (method_constraint_typ_name, override_generic_index) { - // dbg!(method_constraint_typ_name.clone()); + let override_generic_index = override_generics + .iter() + .position(|generic| generic.name.as_str() == override_constraint_typ_name.as_str()); + if let (Some(method_generic_index), Some(override_generic_index)) = + (method_generic_index, override_generic_index) + { dbg!(override_generic_index); - let mut method_generic_index = None; - let mut index = 0; - for method_generic in method_generics.iter() { - if method_generic.name.as_str() == "Self" { - continue; - } - - if method_generic.name.as_str() == method_constraint_typ_name.as_str() { - method_generic_index = Some(index + trait_impl_generics.len()); - break; - } - index += 1; - } - dbg!(method_generic_index.clone()); - let method_generic_index = method_generic_index.expect("Should have a method generic index"); if method_generic_index != override_generic_index { - // dbg!() - let the_trait = - self.interner.get_trait(override_trait_constraint.trait_id); + let the_trait = self.interner.get_trait(override_trait_constraint.trait_id); self.push_err(DefCollectorErrorKind::ImplIsStricterThanTrait { constraint_typ: override_trait_constraint.typ, constraint_name: the_trait.name.0.contents.clone(), @@ -1249,23 +1355,130 @@ impl<'context> Elaborator<'context> { trait_method_name: method.name.0.contents.clone(), trait_method_span: method.location.span, }); - } + } + } + } + } + + fn check_where_clause_against_trait_new( + &mut self, + func_id: &FuncId, + method: &TraitFunction, + trait_impl_where_clause: &[TraitConstraint], + trait_impl_generics: &Generics, + ) { + let func_meta = self.interner.function_meta(func_id); + let override_generics = func_meta.all_generics.clone(); + let method_generics = method.all_generics.clone(); + + // TODO: this only will work with Type;:NamedGeneric get it working with types that themselves contain generics + let mut method_constraint_ids = HashSet::default(); + let mut test_set = HashSet::default(); + let mut substituted_method_ids = HashSet::default(); + // let mut substituted_method_ids_new = HashSet::default(); + for method_constraint in method.trait_constraints.iter() { + let method_generic_index = + Self::find_generic_index(&method_constraint.typ.to_string(), &method_generics); + dbg!(method_generic_index); + test_set.insert(method_constraint.typ.clone()); + if let Some(method_generic_index) = method_generic_index { + method_constraint_ids.insert((method_generic_index, method_constraint.trait_id)); + + // TODO: possible new strategy + // substitute the bindings with the generic indices + // then just compare the types using Eq + // only works on a type var and named generic + let mut bindings: TypeBindings = TypeBindingsMap::new(); + let current_type_var = + method_constraint.typ.type_variable_id().expect("currently expect a type var"); + bindings.insert( + current_type_var, + ( + method_generics[method_generic_index].type_var.clone(), + Type::NamedGeneric( + TypeVariable::unbound(TypeVariableId(method_generic_index)), + method_generics[method_generic_index].name.clone(), + method_generics[method_generic_index].kind.clone(), + ), + ), + ); + substituted_method_ids.insert((method_constraint.typ.substitute(&bindings), method_constraint.trait_id)); + } + } + dbg!(test_set.clone()); + dbg!(substituted_method_ids.clone()); + + let mut override_test_set = HashSet::default(); + for override_trait_constraint in func_meta.trait_constraints.clone() { + override_test_set.insert(override_trait_constraint.typ.clone()); + + let override_generic_index = Self::find_generic_index( + &override_trait_constraint.typ.to_string(), + &override_generics, + ); + dbg!(override_generic_index); + + if override_generic_index.is_none() { + continue; + } + let override_generic_index = override_generic_index.expect("Should have generic index"); + + if !method_constraint_ids + .contains(&(override_generic_index, override_trait_constraint.trait_id)) + { + // TODO + dbg!("ISSUE ERROR IN NEW METHOD"); + } + + // NEW STRATEGY + let mut bindings: TypeBindings = TypeBindingsMap::new(); + let current_type_var = + override_trait_constraint.typ.type_variable_id().expect("currently expect a type var"); + bindings.insert( + current_type_var, + ( + override_generics[override_generic_index].type_var.clone(), + Type::NamedGeneric( + TypeVariable::unbound(TypeVariableId(override_generic_index)), + override_generics[override_generic_index].name.clone(), + override_generics[override_generic_index].kind.clone(), + ), + ), + ); + let substituted_constraint_type = override_trait_constraint.typ.substitute(&bindings); + dbg!(substituted_constraint_type.clone()); + if !substituted_method_ids.contains(&(substituted_constraint_type, override_trait_constraint.trait_id)) { + dbg!("NEW STRATEGY ERR"); + + let the_trait = self.interner.get_trait(override_trait_constraint.trait_id); + self.push_err(DefCollectorErrorKind::ImplIsStricterThanTrait { + constraint_typ: override_trait_constraint.typ, + constraint_name: the_trait.name.0.contents.clone(), + constraint_span: override_trait_constraint.span, + trait_method_name: method.name.0.contents.clone(), + trait_method_span: method.location.span, + }); + } + } + dbg!(override_test_set.clone()); + } + + fn find_generic_index(target_name: &str, generics: &Generics) -> Option { + let mut generic_index = None; + let mut index = 0; + for generic in generics.iter() { + if generic.name.as_str() == "Self" { + continue; + } + + if generic.name.as_str() == target_name { + generic_index = Some(index); + break; } - // dbg!(override_constraint_typ_name.clone()); - // dbg!(override_constraint_is_on_trait_method); - // if !override_constraint_is_on_trait_method { - // let the_trait = - // self.interner.get_trait(override_trait_constraint.trait_id); - // self.push_err(DefCollectorErrorKind::ImplIsStricterThanTrait { - // constraint_typ: override_trait_constraint.typ, - // constraint_name: the_trait.name.0.contents.clone(), - // constraint_span: override_trait_constraint.span, - // trait_method_name: method.name.0.contents.clone(), - // trait_method_span: method.location.span, - // }); - // } + index += 1; } + generic_index } fn check_trait_impl_crate_coherence( diff --git a/compiler/noirc_frontend/src/elaborator/traits.rs b/compiler/noirc_frontend/src/elaborator/traits.rs index d1309176c3f..70e025e2e3a 100644 --- a/compiler/noirc_frontend/src/elaborator/traits.rs +++ b/compiler/noirc_frontend/src/elaborator/traits.rs @@ -122,7 +122,8 @@ impl<'context> Elaborator<'context> { // dbg!(this.generics.clone()); // dbg!(func_meta.all_generics.clone()); // dbg!(func_meta.direct_generics.clone()); - let generics = vecmap(&this.generics.clone(), |generic| generic.type_var.clone()); + let generics = + vecmap(&this.generics.clone(), |generic| generic.type_var.clone()); let default_impl_list: Vec<_> = unresolved_trait .fns_with_default_impl @@ -148,7 +149,8 @@ impl<'context> Elaborator<'context> { default_impl, default_impl_module_id: unresolved_trait.module_id, trait_constraints: func_meta.trait_constraints.clone(), - all_generics: func_meta.all_generics.clone(), + // all_generics: func_meta.all_generics.clone(), + all_generics: this.generics.clone(), }); }); } diff --git a/compiler/noirc_frontend/src/hir_def/types.rs b/compiler/noirc_frontend/src/hir_def/types.rs index 6777c0d1c79..9a1d1c50710 100644 --- a/compiler/noirc_frontend/src/hir_def/types.rs +++ b/compiler/noirc_frontend/src/hir_def/types.rs @@ -1746,7 +1746,7 @@ impl Type { } } - fn type_variable_id(&self) -> Option { + pub(crate) fn type_variable_id(&self) -> Option { match self { Type::TypeVariable(variable, _) | Type::NamedGeneric(variable, _, _) => { Some(variable.0) diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index 2bbf249481e..4ac22807eae 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -1949,22 +1949,90 @@ fn impl_stricter_than_trait_different_generics() { let src = r#" trait Default { } - // Object type differs + // Object type of the trait constraint differs trait Foo { - fn foo() where T: Default; - // fn foo(); + fn foo_good() where T: Default; + + fn foo_bad() where T: Default; } impl Foo for () { - fn foo() where B: Default {} - // fn foo() where A: Default {} + fn foo_good() where A: Default {} + + fn foo_bad() where B: Default {} + } + "#; + let errors = get_program_errors(src); + dbg!(errors.clone()); + + assert_eq!(errors.len(), 1); + if let CompilationError::DefinitionError(DefCollectorErrorKind::ImplIsStricterThanTrait { + constraint_typ, + .. + }) = &errors[0].0 + { + assert!(matches!(constraint_typ.to_string().as_str(), "B")); + } else { + panic!("Expected DefCollectorErrorKind::ImplIsStricterThanTrait but got {:?}", errors[0].0); + } +} + +#[test] +fn impl_stricter_than_trait_different_object_generics() { + let src = r#" + trait Default { } + + struct Option { + inner: T + } + + // Object type differs by + trait Bar { + fn bar() where Option: Default; + } - // fn foo() {} + impl Bar for () { + fn bar() where Option: Default {} } "#; + let errors = get_program_errors(src); dbg!(errors.clone()); + assert_eq!(errors.len(), 1); + if let CompilationError::DefinitionError(DefCollectorErrorKind::ImplIsStricterThanTrait { + constraint_typ, + .. + }) = &errors[0].0 + { + assert!(matches!(constraint_typ.to_string().as_str(), "Option")); + } else { + panic!("Expected DefCollectorErrorKind::ImplIsStricterThanTrait but got {:?}", errors[0].0); + } +} + +#[test] +fn impl_stricter_than_trait_different_trait() { + let src = r#" + trait Default { } + trait OtherDefault { } + + struct Option { + inner: T + } + + // Object type differs by + trait Bar { + fn bar() where Option: Default; + } + + impl Bar for () { + fn bar() where Option: OtherDefault {} + } + "#; + + let errors = get_program_errors(src); + dbg!(errors.clone()); assert_eq!(errors.len(), 1); } diff --git a/test_programs/compile_failure/impl_stricter_than_trait/src/main.nr b/test_programs/compile_failure/impl_stricter_than_trait/src/main.nr index 587c22da65e..c6c2b33fd99 100644 --- a/test_programs/compile_failure/impl_stricter_than_trait/src/main.nr +++ b/test_programs/compile_failure/impl_stricter_than_trait/src/main.nr @@ -33,14 +33,28 @@ impl MyType { trait MyTrait { } -// Object type differs -trait Foo { - fn foo() where U: MyTrait; - // fn foo(); +// // Object type differs +// trait Foo { +// fn foo() where T: MyTrait; +// // fn foo(); +// } + +// impl Foo for () { +// fn foo() where B: MyTrait {} +// // fn foo() where A: Default {} +// // fn foo() {} +// } + +struct MyStruct { + inner: T, +} + +trait OtherTrait {} + +trait Bar { + fn bar() where MyStruct: MyTrait; } -impl Foo for () { - fn foo() where B: MyTrait {} - // fn foo() where A: Default {} - // fn foo() {} +impl Bar for () { + fn bar() where MyStruct: OtherTrait {} } \ No newline at end of file From 5f8d756cd8f6af99eabdecf05484c9b5a7a41c62 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Tue, 16 Jul 2024 18:04:11 +0000 Subject: [PATCH 13/27] impl stricter than trait generalized for object types --- compiler/noirc_frontend/src/elaborator/mod.rs | 397 ++++++------------ .../noirc_frontend/src/elaborator/traits.rs | 10 +- .../src/hir/def_collector/dc_crate.rs | 2 +- compiler/noirc_frontend/src/tests.rs | 127 +++++- .../impl_stricter_than_trait/Nargo.toml | 7 - .../impl_stricter_than_trait/src/main.nr | 60 --- 6 files changed, 251 insertions(+), 352 deletions(-) delete mode 100644 test_programs/compile_failure/impl_stricter_than_trait/Nargo.toml delete mode 100644 test_programs/compile_failure/impl_stricter_than_trait/src/main.nr diff --git a/compiler/noirc_frontend/src/elaborator/mod.rs b/compiler/noirc_frontend/src/elaborator/mod.rs index 2092ee9df4d..6541f4f8588 100644 --- a/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/compiler/noirc_frontend/src/elaborator/mod.rs @@ -1111,13 +1111,8 @@ impl<'context> Elaborator<'context> { func_id, method, trait_impl_where_clause, - &trait_impl.resolved_generics, - ); - self.check_where_clause_against_trait_new( - func_id, - method, - trait_impl_where_clause, - &trait_impl.resolved_generics, + trait_impl.resolved_generics.len(), + trait_impl.resolved_trait_generics.len(), ); func_ids_in_trait.insert(*func_id); @@ -1173,283 +1168,52 @@ impl<'context> Elaborator<'context> { func_id: &FuncId, method: &TraitFunction, trait_impl_where_clause: &[TraitConstraint], - trait_impl_generics: &Generics, + trait_impl_generics_len: usize, + trait_generics_len: usize, ) { let func_meta = self.interner.function_meta(func_id); - // dbg!(func_meta.trait_constraints.clone()); - // dbg!(trait_impl_where_clause); - // dbg!(method.typ.clone()); let override_generics = func_meta.all_generics.clone(); - // dbg!(override_generics.clone()); - // dbg!(func_meta.direct_generics.clone()); - // dbg!(method.trait_constraints.clone()); - // dbg!(method.all_generics.clone()); - let method_generics = method.all_generics.clone(); - // if self.interner.function_name(func_id) == "hash" && (override_generics.len() == 1 || override_generics.len() == 2) { - // dbg!(override_generics.clone()); - // dbg!(method.all_generics.clone()); - // dbg!(trait_impl_generics.clone()); - // } + let method_generics: Vec<_> = method + .all_generics + .clone() + .into_iter() + .filter(|generic| generic.name.as_str() != "Self") + .collect(); + + let mut substituted_method_ids = HashSet::default(); + for method_constraint in method.trait_constraints.iter() { + let substituted_constraint_type = Self::reset_generics_on_constraint_type( + &method_constraint.typ, + &method_generics, + &method_generics, + 0, + 0, + ); + substituted_method_ids + .insert((substituted_constraint_type, method_constraint.trait_id)); + } for override_trait_constraint in func_meta.trait_constraints.clone() { - // We allow where clauses on impls but during definition collection they are included as part of the where - // clause of each function. This check is necessary so we do not unnecessarily error on trait impl where clauses. let override_constraint_is_from_impl = trait_impl_where_clause.iter().any(|impl_constraint| { impl_constraint.trait_id == override_trait_constraint.trait_id }); - // dbg!(override_constraint_is_from_impl); if override_constraint_is_from_impl { continue; } - let override_constraint_is_on_trait_method = - method.trait_constraints.iter().any(|method_constraint| { - method_constraint.trait_id == override_trait_constraint.trait_id - }); - - let method_generic_index = method - .trait_constraints - .iter() - .find(|method_constraint| { - // TODO: need to change how this check is done as we want to check whether the type is the same - // and the trait is different as well - method_constraint.trait_id == override_trait_constraint.trait_id - }) - .map(|method_constraint| { - dbg!(method_constraint.clone()); - // TODO: this needs to be made a recursive check - match (&method_constraint.typ, &override_trait_constraint.typ) { - ( - Type::NamedGeneric(_, found_name, _), - Type::NamedGeneric(_, override_name, _), - ) => { - dbg!(found_name); - dbg!(override_name); - } - ( - Type::Struct(method_struct, method_struct_generics), - Type::Struct(override_struct, override_struct_generics), - ) => { - dbg!(method_struct.borrow().id); - dbg!(override_struct.borrow().id); - if method_struct.borrow().id == override_struct.borrow().id { - // for method_generic in method_struct_generics.iter() { - // dbg!(method_generic.to_string()); - // } - // for override_generic in override_struct_generics.iter() { - // dbg!(override_generic.to_string()); - // } - for (method_struct_generic, override_struct_generic) in - method_struct_generics.iter().zip(override_struct_generics) - { - let method_constraint_typ_name = - method_struct_generic.to_string(); - let mut method_generic_index = None; - let mut index = 0; - for method_generic in method_generics.iter() { - if method_generic.name.as_str() == "Self" { - continue; - } - - if method_generic.name.as_str() - == method_constraint_typ_name.as_str() - { - method_generic_index = Some(index); - break; - } - - index += 1; - } - - let override_struct_generic_name = - override_struct_generic.to_string(); - dbg!(override_struct_generic_name.clone()); - let override_generic_index = - override_generics.iter().position(|generic| { - generic.name.as_str() - == override_struct_generic_name.as_str() - }); - dbg!(method_generic_index); - dbg!(override_generic_index); - if let ( - Some(method_generic_index), - Some(override_generic_index), - ) = (method_generic_index, override_generic_index) - { - dbg!(override_generic_index); - - if method_generic_index != override_generic_index { - let the_trait = self - .interner - .get_trait(override_trait_constraint.trait_id); - self.push_err( - DefCollectorErrorKind::ImplIsStricterThanTrait { - constraint_typ: override_trait_constraint - .typ - .clone(), - constraint_name: the_trait - .name - .0 - .contents - .clone(), - constraint_span: override_trait_constraint.span, - trait_method_name: method - .name - .0 - .contents - .clone(), - trait_method_span: method.location.span, - }, - ); - } - } - } - } - } - _ => { - dbg!(method_constraint.typ.clone()); - dbg!(override_trait_constraint.typ.clone()); - } - } - - let method_constraint_typ_name = method_constraint.typ.to_string(); - // dbg!(typ_as_string.clone()); - // let index = override_generics.iter().position(|generic| generic.name.as_str() == typ_as_string.as_str()); - // dbg!(index) - // method_constraint_typ_name - let mut method_generic_index = None; - let mut index = 0; - for method_generic in method_generics.iter() { - if method_generic.name.as_str() == "Self" { - continue; - } - - if method_generic.name.as_str() == method_constraint_typ_name.as_str() { - method_generic_index = Some(index); - break; - } - - index += 1; - } - method_generic_index - }) - .flatten(); - - let override_constraint_typ_name = override_trait_constraint.typ.to_string(); - // dbg!(override_constraint_typ_name.clone()); - let override_generic_index = override_generics - .iter() - .position(|generic| generic.name.as_str() == override_constraint_typ_name.as_str()); - if let (Some(method_generic_index), Some(override_generic_index)) = - (method_generic_index, override_generic_index) - { - dbg!(override_generic_index); - - if method_generic_index != override_generic_index { - let the_trait = self.interner.get_trait(override_trait_constraint.trait_id); - self.push_err(DefCollectorErrorKind::ImplIsStricterThanTrait { - constraint_typ: override_trait_constraint.typ, - constraint_name: the_trait.name.0.contents.clone(), - constraint_span: override_trait_constraint.span, - trait_method_name: method.name.0.contents.clone(), - trait_method_span: method.location.span, - }); - } - } - } - } - - fn check_where_clause_against_trait_new( - &mut self, - func_id: &FuncId, - method: &TraitFunction, - trait_impl_where_clause: &[TraitConstraint], - trait_impl_generics: &Generics, - ) { - let func_meta = self.interner.function_meta(func_id); - let override_generics = func_meta.all_generics.clone(); - let method_generics = method.all_generics.clone(); - - // TODO: this only will work with Type;:NamedGeneric get it working with types that themselves contain generics - let mut method_constraint_ids = HashSet::default(); - let mut test_set = HashSet::default(); - let mut substituted_method_ids = HashSet::default(); - // let mut substituted_method_ids_new = HashSet::default(); - for method_constraint in method.trait_constraints.iter() { - let method_generic_index = - Self::find_generic_index(&method_constraint.typ.to_string(), &method_generics); - dbg!(method_generic_index); - test_set.insert(method_constraint.typ.clone()); - if let Some(method_generic_index) = method_generic_index { - method_constraint_ids.insert((method_generic_index, method_constraint.trait_id)); - - // TODO: possible new strategy - // substitute the bindings with the generic indices - // then just compare the types using Eq - // only works on a type var and named generic - let mut bindings: TypeBindings = TypeBindingsMap::new(); - let current_type_var = - method_constraint.typ.type_variable_id().expect("currently expect a type var"); - bindings.insert( - current_type_var, - ( - method_generics[method_generic_index].type_var.clone(), - Type::NamedGeneric( - TypeVariable::unbound(TypeVariableId(method_generic_index)), - method_generics[method_generic_index].name.clone(), - method_generics[method_generic_index].kind.clone(), - ), - ), - ); - substituted_method_ids.insert((method_constraint.typ.substitute(&bindings), method_constraint.trait_id)); - } - } - dbg!(test_set.clone()); - dbg!(substituted_method_ids.clone()); - - let mut override_test_set = HashSet::default(); - for override_trait_constraint in func_meta.trait_constraints.clone() { - override_test_set.insert(override_trait_constraint.typ.clone()); - - let override_generic_index = Self::find_generic_index( - &override_trait_constraint.typ.to_string(), + let substituted_constraint_type = Self::reset_generics_on_constraint_type( + &override_trait_constraint.typ, &override_generics, + &method_generics, + trait_impl_generics_len, + trait_generics_len, ); - dbg!(override_generic_index); - - if override_generic_index.is_none() { - continue; - } - let override_generic_index = override_generic_index.expect("Should have generic index"); - - if !method_constraint_ids - .contains(&(override_generic_index, override_trait_constraint.trait_id)) - { - // TODO - dbg!("ISSUE ERROR IN NEW METHOD"); - } - // NEW STRATEGY - let mut bindings: TypeBindings = TypeBindingsMap::new(); - let current_type_var = - override_trait_constraint.typ.type_variable_id().expect("currently expect a type var"); - bindings.insert( - current_type_var, - ( - override_generics[override_generic_index].type_var.clone(), - Type::NamedGeneric( - TypeVariable::unbound(TypeVariableId(override_generic_index)), - override_generics[override_generic_index].name.clone(), - override_generics[override_generic_index].kind.clone(), - ), - ), - ); - let substituted_constraint_type = override_trait_constraint.typ.substitute(&bindings); - dbg!(substituted_constraint_type.clone()); - if !substituted_method_ids.contains(&(substituted_constraint_type, override_trait_constraint.trait_id)) { - dbg!("NEW STRATEGY ERR"); - + if !substituted_method_ids.contains(&( + substituted_constraint_type.clone(), + override_trait_constraint.trait_id, + )) { let the_trait = self.interner.get_trait(override_trait_constraint.trait_id); self.push_err(DefCollectorErrorKind::ImplIsStricterThanTrait { constraint_typ: override_trait_constraint.typ, @@ -1460,7 +1224,102 @@ impl<'context> Elaborator<'context> { }); } } - dbg!(override_test_set.clone()); + } + + fn reset_generics_on_constraint_type( + typ: &Type, + override_generics: &Generics, + method_generics: &Generics, + trait_impl_generics_len: usize, + trait_generics_len: usize, + ) -> Type { + let recur_generics_reset = |typ: &Type| { + Self::reset_generics_on_constraint_type( + typ, + override_generics, + method_generics, + trait_impl_generics_len, + trait_generics_len, + ) + }; + + match &typ { + Type::NamedGeneric(type_var, name, _) => { + let generic_index = Self::find_generic_index(name, override_generics); + + let mut bindings: TypeBindings = TypeBindingsMap::new(); + if let Some(mut method_generic_index) = generic_index { + // Override generics from a trait impl can possibly contain generics + // as part of the trait impl that are not part of the trait method generics. + // We need to account for this by checking against the length of the lists for trait impl generics and trait generics. + // It is important to note that trait impl generics are expected to also contain the trait generics, + // and that is why we add the trait generics list length before subtracting the trait impl generics length. + if trait_impl_generics_len > trait_generics_len { + method_generic_index = + method_generic_index + trait_generics_len - trait_impl_generics_len; + } + + // To accurately match against the trait function's constraint types, we must + // replace the name of any generics in the override function with the respective + // name from the original trait. + // This substitution is why we also must recompute the method generic index above. + bindings.insert( + type_var.id(), + ( + method_generics[method_generic_index].type_var.clone(), + Type::NamedGeneric( + TypeVariable::unbound(TypeVariableId(method_generic_index)), + method_generics[method_generic_index].name.clone(), + method_generics[method_generic_index].kind.clone(), + ), + ), + ); + } + + typ.substitute(&bindings) + } + Type::Struct(struct_type, generics) => { + let reset_generics = generics.iter().map(recur_generics_reset).collect(); + Type::Struct(struct_type.clone(), reset_generics) + } + Type::Alias(type_alias, generics) => { + let reset_generics = generics.iter().map(recur_generics_reset).collect(); + Type::Alias(type_alias.clone(), reset_generics) + } + Type::Array(size, element_type) => { + let size = recur_generics_reset(size.as_ref()); + let element_type = recur_generics_reset(element_type.as_ref()); + Type::Array(Box::new(size), Box::new(element_type)) + } + Type::Slice(element_type) => { + let element_type = recur_generics_reset(element_type.as_ref()); + Type::Slice(Box::new(element_type)) + } + Type::String(size) => { + let size = recur_generics_reset(size.as_ref()); + Type::String(Box::new(size)) + } + Type::FmtString(size, element_types) => { + let size = recur_generics_reset(size.as_ref()); + let element_types = recur_generics_reset(element_types.as_ref()); + Type::FmtString(Box::new(size), Box::new(element_types)) + } + Type::Tuple(types) => { + let reset_types = types.iter().map(recur_generics_reset).collect(); + Type::Tuple(reset_types) + } + Type::Function(arguments, return_type, environment) => { + let arguments = arguments.iter().map(recur_generics_reset).collect(); + let return_type = recur_generics_reset(return_type.as_ref()); + let environment = recur_generics_reset(environment.as_ref()); + Type::Function(arguments, Box::new(return_type), Box::new(environment)) + } + Type::MutableReference(typ) => { + let typ = recur_generics_reset(typ.as_ref()); + Type::MutableReference(Box::new(typ)) + } + _ => typ.clone(), + } } fn find_generic_index(target_name: &str, generics: &Generics) -> Option { diff --git a/compiler/noirc_frontend/src/elaborator/traits.rs b/compiler/noirc_frontend/src/elaborator/traits.rs index 70e025e2e3a..17337656d0a 100644 --- a/compiler/noirc_frontend/src/elaborator/traits.rs +++ b/compiler/noirc_frontend/src/elaborator/traits.rs @@ -106,6 +106,9 @@ impl<'context> Elaborator<'context> { let func_id = unresolved_trait.method_ids[&name.0.contents]; + // this.recover_generics(|this| { + + // }); this.resolve_trait_function( name, generics, @@ -119,9 +122,7 @@ impl<'context> Elaborator<'context> { let arguments = vecmap(&func_meta.parameters.0, |(_, typ, _)| typ.clone()); let return_type = func_meta.return_type().clone(); - // dbg!(this.generics.clone()); - // dbg!(func_meta.all_generics.clone()); - // dbg!(func_meta.direct_generics.clone()); + let generics = vecmap(&this.generics.clone(), |generic| generic.type_var.clone()); @@ -149,8 +150,7 @@ impl<'context> Elaborator<'context> { default_impl, default_impl_module_id: unresolved_trait.module_id, trait_constraints: func_meta.trait_constraints.clone(), - // all_generics: func_meta.all_generics.clone(), - all_generics: this.generics.clone(), + all_generics: func_meta.all_generics.clone(), }); }); } diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs index 03fe1a2777b..8059e885db7 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs @@ -5,7 +5,7 @@ use crate::graph::CrateId; use crate::hir::comptime::{Interpreter, InterpreterError}; use crate::hir::def_map::{CrateDefMap, LocalModuleId, ModuleId}; use crate::hir::resolution::errors::ResolverError; -use crate::{Generics, ResolvedGeneric, Type}; +use crate::{Generics, Type}; use crate::hir::resolution::import::{resolve_import, ImportDirective, PathResolution}; use crate::hir::resolution::{ diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index 4ac22807eae..942ae8b674c 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -1898,11 +1898,13 @@ fn quote_code_fragments() { } #[test] -fn impl_stricter_than_trait() { +fn impl_stricter_than_trait_no_trait_method_constraints() { // This test ensures that the error we get from the where clause on the trait impl method // is a `DefCollectorErrorKind::ImplIsStricterThanTrait` error. let src = r#" trait Serialize { + // We want to make sure we trigger the error when override a trait method + // which itself has no trait constraints. fn serialize(self) -> [Field; N]; } @@ -1962,9 +1964,8 @@ fn impl_stricter_than_trait_different_generics() { fn foo_bad() where B: Default {} } "#; - let errors = get_program_errors(src); - dbg!(errors.clone()); + let errors = get_program_errors(src); assert_eq!(errors.len(), 1); if let CompilationError::DefinitionError(DefCollectorErrorKind::ImplIsStricterThanTrait { constraint_typ, @@ -1980,31 +1981,89 @@ fn impl_stricter_than_trait_different_generics() { #[test] fn impl_stricter_than_trait_different_object_generics() { let src = r#" - trait Default { } + trait MyTrait { } + + trait OtherTrait {} struct Option { inner: T } - // Object type differs by + struct OtherOption { + inner: Option, + } + trait Bar { - fn bar() where Option: Default; + fn bar_good() where Option: MyTrait, OtherOption>: OtherTrait; + + fn bar_bad() where Option: MyTrait, OtherOption>: OtherTrait; + + fn array_good() where [T; 8]: MyTrait; + + fn array_bad() where [T; 8]: MyTrait; + + fn tuple_good() where (Option, Option): MyTrait; + + fn tuple_bad() where (Option, Option): MyTrait; } impl Bar for () { - fn bar() where Option: Default {} + fn bar_good() + where + OtherOption>: OtherTrait, + Option: MyTrait { } + + fn bar_bad() + where + OtherOption>: OtherTrait, + Option: MyTrait { } + + fn array_good() where [A; 8]: MyTrait { } + + fn array_bad() where [B; 8]: MyTrait { } + + fn tuple_good() where (Option, Option): MyTrait { } + + fn tuple_bad() where (Option, Option): MyTrait { } } "#; let errors = get_program_errors(src); dbg!(errors.clone()); - assert_eq!(errors.len(), 1); + assert_eq!(errors.len(), 3); if let CompilationError::DefinitionError(DefCollectorErrorKind::ImplIsStricterThanTrait { constraint_typ, + constraint_name, .. }) = &errors[0].0 { assert!(matches!(constraint_typ.to_string().as_str(), "Option")); + assert!(matches!(constraint_name.as_str(), "MyTrait")); + } else { + panic!("Expected DefCollectorErrorKind::ImplIsStricterThanTrait but got {:?}", errors[0].0); + } + + if let CompilationError::DefinitionError(DefCollectorErrorKind::ImplIsStricterThanTrait { + constraint_typ, + constraint_name, + .. + }) = &errors[1].0 + { + assert!(matches!(constraint_typ.to_string().as_str(), "[B; 8]")); + assert!(matches!(constraint_name.as_str(), "MyTrait")); + } else { + panic!("Expected DefCollectorErrorKind::ImplIsStricterThanTrait but got {:?}", errors[0].0); + } + + if let CompilationError::DefinitionError(DefCollectorErrorKind::ImplIsStricterThanTrait { + constraint_typ, + constraint_name, + .. + }) = &errors[2].0 + { + dbg!(constraint_typ.to_string().as_str()); + assert!(matches!(constraint_typ.to_string().as_str(), "(Option, Option)")); + assert!(matches!(constraint_name.as_str(), "MyTrait")); } else { panic!("Expected DefCollectorErrorKind::ImplIsStricterThanTrait but got {:?}", errors[0].0); } @@ -2021,19 +2080,67 @@ fn impl_stricter_than_trait_different_trait() { inner: T } - // Object type differs by trait Bar { fn bar() where Option: Default; } impl Bar for () { + // Trait constraint differs due to the trait even though the constraint + // types are the same. fn bar() where Option: OtherDefault {} } "#; let errors = get_program_errors(src); - dbg!(errors.clone()); assert_eq!(errors.len(), 1); + if let CompilationError::DefinitionError(DefCollectorErrorKind::ImplIsStricterThanTrait { + constraint_typ, + constraint_name, + .. + }) = &errors[0].0 + { + assert!(matches!(constraint_typ.to_string().as_str(), "Option")); + assert!(matches!(constraint_name.as_str(), "OtherDefault")); + } else { + panic!("Expected DefCollectorErrorKind::ImplIsStricterThanTrait but got {:?}", errors[0].0); + } +} + +#[test] +fn trait_impl_where_clause_stricter_pass() { + let src = r#" + trait MyTrait { + fn good_foo() where H: OtherTrait; + + fn bad_foo() where H: OtherTrait; + } + + trait OtherTrait {} + + struct Option { + inner: T + } + + impl MyTrait for [T] where Option: MyTrait { + fn good_foo() where B: OtherTrait { } + + fn bad_foo() where A: OtherTrait { } + } + "#; + + let errors = get_program_errors(src); + assert_eq!(errors.len(), 1); + if let CompilationError::DefinitionError(DefCollectorErrorKind::ImplIsStricterThanTrait { + constraint_typ, + constraint_name, + .. + }) = &errors[0].0 + { + assert!(matches!(constraint_typ.to_string().as_str(), "A")); + assert!(matches!(constraint_name.as_str(), "OtherTrait")); + } else { + panic!("Expected DefCollectorErrorKind::ImplIsStricterThanTrait but got {:?}", errors[0].0); + } } #[test] diff --git a/test_programs/compile_failure/impl_stricter_than_trait/Nargo.toml b/test_programs/compile_failure/impl_stricter_than_trait/Nargo.toml deleted file mode 100644 index 00d63cd46e0..00000000000 --- a/test_programs/compile_failure/impl_stricter_than_trait/Nargo.toml +++ /dev/null @@ -1,7 +0,0 @@ -[package] -name = "impl_stricter_than_trait" -type = "bin" -authors = [""] -compiler_version = ">=0.31.0" - -[dependencies] \ No newline at end of file diff --git a/test_programs/compile_failure/impl_stricter_than_trait/src/main.nr b/test_programs/compile_failure/impl_stricter_than_trait/src/main.nr deleted file mode 100644 index c6c2b33fd99..00000000000 --- a/test_programs/compile_failure/impl_stricter_than_trait/src/main.nr +++ /dev/null @@ -1,60 +0,0 @@ -trait Serialize { - fn serialize(self) -> [Field; N]; -} - -trait ToField { - fn to_field(self) -> Field; -} - -fn process_array(array: [Field; N]) -> Field { - array[0] -} - -fn serialize_thing(thing: A) -> [Field; N] where A: Serialize { - thing.serialize() -} - -struct MyType { - a: T, - b: T, -} - -impl Serialize<2> for MyType { - fn serialize(self) -> [Field; 2] where T: ToField { - [ self.a.to_field(), self.b.to_field() ] - } -} - -impl MyType { - fn do_thing_with_serialization_with_extra_steps(self) -> Field { - process_array(serialize_thing(self)) - } -} - -trait MyTrait { } - -// // Object type differs -// trait Foo { -// fn foo() where T: MyTrait; -// // fn foo(); -// } - -// impl Foo for () { -// fn foo() where B: MyTrait {} -// // fn foo() where A: Default {} -// // fn foo() {} -// } - -struct MyStruct { - inner: T, -} - -trait OtherTrait {} - -trait Bar { - fn bar() where MyStruct: MyTrait; -} - -impl Bar for () { - fn bar() where MyStruct: OtherTrait {} -} \ No newline at end of file From e8e977a1f0b1d4f5f6c0d7e9e26b4b41bf8d5084 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Tue, 16 Jul 2024 18:25:26 +0000 Subject: [PATCH 14/27] cleanup mod.rs in elabroator package make separate trait_impls module? --- compiler/noirc_frontend/src/elaborator/mod.rs | 326 +---------------- .../src/elaborator/trait_impls.rs | 336 ++++++++++++++++++ 2 files changed, 341 insertions(+), 321 deletions(-) create mode 100644 compiler/noirc_frontend/src/elaborator/trait_impls.rs diff --git a/compiler/noirc_frontend/src/elaborator/mod.rs b/compiler/noirc_frontend/src/elaborator/mod.rs index 3b15e35f89b..4a921739066 100644 --- a/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/compiler/noirc_frontend/src/elaborator/mod.rs @@ -1,5 +1,5 @@ use std::{ - collections::{BTreeMap, BTreeSet, HashMap as TypeBindingsMap}, + collections::{BTreeMap, BTreeSet}, fmt::Display, rc::Rc, }; @@ -14,7 +14,6 @@ use crate::{ UnresolvedStruct, UnresolvedTypeAlias, }, dc_mod, - errors::DuplicateType, }, resolution::{errors::ResolverError, path_resolver::PathResolver, resolver::LambdaContext}, scope::ScopeForest as GenericScopeForest, @@ -23,7 +22,7 @@ use crate::{ hir_def::{ expr::HirIdent, function::{FunctionBody, Parameters}, - traits::{TraitConstraint, TraitFunction}, + traits::TraitConstraint, types::{Generics, Kind, ResolvedGeneric}, }, lexer::Lexer, @@ -37,7 +36,7 @@ use crate::{ }, parser::TopLevelStatement, token::Tokens, - Shared, Type, TypeBindings, TypeVariable, TypeVariableId, + Shared, Type, TypeBindings, TypeVariable, }; use crate::{ ast::{TraitBound, UnresolvedGeneric, UnresolvedGenerics}, @@ -66,6 +65,7 @@ mod lints; mod patterns; mod scope; mod statements; +mod trait_impls; mod traits; mod types; mod unquote; @@ -73,7 +73,7 @@ mod unquote; use fm::FileId; use iter_extended::vecmap; use noirc_errors::{Location, Span}; -use rustc_hash::{FxHashMap as HashMap, FxHashSet as HashSet}; +use rustc_hash::FxHashMap as HashMap; /// ResolverMetas are tagged onto each definition to track how many times they are used #[derive(Debug, PartialEq, Eq)] @@ -1067,322 +1067,6 @@ impl<'context> Elaborator<'context> { } } - fn collect_trait_impl_methods( - &mut self, - trait_id: TraitId, - trait_impl: &mut UnresolvedTraitImpl, - trait_impl_where_clause: &[TraitConstraint], - ) { - self.local_module = trait_impl.module_id; - self.file = trait_impl.file_id; - - // In this Vec methods[i] corresponds to trait.methods[i]. If the impl has no implementation - // for a particular method, the default implementation will be added at that slot. - let mut ordered_methods = Vec::new(); - - // check whether the trait implementation is in the same crate as either the trait or the type - self.check_trait_impl_crate_coherence(trait_id, trait_impl); - - // set of function ids that have a corresponding method in the trait - let mut func_ids_in_trait = HashSet::default(); - - // Temporarily take ownership of the trait's methods so we can iterate over them - // while also mutating the interner - let the_trait = self.interner.get_trait_mut(trait_id); - let methods = std::mem::take(&mut the_trait.methods); - // dbg!(the_trait.generics.clone()); - // dbg!(trait_impl.resolved_generics.clone()); - for method in &methods { - let overrides: Vec<_> = trait_impl - .methods - .functions - .iter() - .filter(|(_, _, f)| f.name() == method.name.0.contents) - .collect(); - - if overrides.is_empty() { - if let Some(default_impl) = &method.default_impl { - // copy 'where' clause from unresolved trait impl - let mut default_impl_clone = default_impl.clone(); - default_impl_clone.def.where_clause.extend(trait_impl.where_clause.clone()); - - let func_id = self.interner.push_empty_fn(); - let module = self.module_id(); - let location = Location::new(default_impl.def.span, trait_impl.file_id); - self.interner.push_function(func_id, &default_impl.def, module, location); - self.define_function_meta(&mut default_impl_clone, func_id, false); - func_ids_in_trait.insert(func_id); - ordered_methods.push(( - method.default_impl_module_id, - func_id, - *default_impl_clone, - )); - } else { - self.push_err(DefCollectorErrorKind::TraitMissingMethod { - trait_name: self.interner.get_trait(trait_id).name.clone(), - method_name: method.name.clone(), - trait_impl_span: trait_impl - .object_type - .span - .expect("type must have a span"), - }); - } - } else { - for (_, func_id, _) in &overrides { - self.check_where_clause_against_trait( - func_id, - method, - trait_impl_where_clause, - trait_impl.resolved_generics.len(), - trait_impl.resolved_trait_generics.len(), - ); - - func_ids_in_trait.insert(*func_id); - } - - if overrides.len() > 1 { - self.push_err(DefCollectorErrorKind::Duplicate { - typ: DuplicateType::TraitAssociatedFunction, - first_def: overrides[0].2.name_ident().clone(), - second_def: overrides[1].2.name_ident().clone(), - }); - } - - ordered_methods.push(overrides[0].clone()); - } - } - - // Restore the methods that were taken before the for loop - let the_trait = self.interner.get_trait_mut(trait_id); - the_trait.set_methods(methods); - - // Emit MethodNotInTrait error for methods in the impl block that - // don't have a corresponding method signature defined in the trait - for (_, func_id, func) in &trait_impl.methods.functions { - if !func_ids_in_trait.contains(func_id) { - let trait_name = the_trait.name.clone(); - let impl_method = func.name_ident().clone(); - let error = DefCollectorErrorKind::MethodNotInTrait { trait_name, impl_method }; - self.errors.push((error.into(), self.file)); - } - } - - trait_impl.methods.functions = ordered_methods; - trait_impl.methods.trait_id = Some(trait_id); - } - - /// Issue an error if the impl is stricter than the trait. - /// - /// # Example - /// - /// ``` - /// trait MyTrait { } - /// trait Foo { - /// fn foo(); - /// } - /// impl Foo for () { - /// // Error issued here as `foo` does not have the `MyTrait` constraint - /// fn foo() where B: MyTrait {} - /// } - /// ``` - fn check_where_clause_against_trait( - &mut self, - func_id: &FuncId, - method: &TraitFunction, - trait_impl_where_clause: &[TraitConstraint], - trait_impl_generics_len: usize, - trait_generics_len: usize, - ) { - let func_meta = self.interner.function_meta(func_id); - let override_generics = func_meta.all_generics.clone(); - let method_generics: Vec<_> = method - .all_generics - .clone() - .into_iter() - .filter(|generic| generic.name.as_str() != "Self") - .collect(); - - let mut substituted_method_ids = HashSet::default(); - for method_constraint in method.trait_constraints.iter() { - let substituted_constraint_type = Self::reset_generics_on_constraint_type( - &method_constraint.typ, - &method_generics, - &method_generics, - 0, - 0, - ); - substituted_method_ids - .insert((substituted_constraint_type, method_constraint.trait_id)); - } - - for override_trait_constraint in func_meta.trait_constraints.clone() { - let override_constraint_is_from_impl = - trait_impl_where_clause.iter().any(|impl_constraint| { - impl_constraint.trait_id == override_trait_constraint.trait_id - }); - if override_constraint_is_from_impl { - continue; - } - - let substituted_constraint_type = Self::reset_generics_on_constraint_type( - &override_trait_constraint.typ, - &override_generics, - &method_generics, - trait_impl_generics_len, - trait_generics_len, - ); - - if !substituted_method_ids.contains(&( - substituted_constraint_type.clone(), - override_trait_constraint.trait_id, - )) { - let the_trait = self.interner.get_trait(override_trait_constraint.trait_id); - self.push_err(DefCollectorErrorKind::ImplIsStricterThanTrait { - constraint_typ: override_trait_constraint.typ, - constraint_name: the_trait.name.0.contents.clone(), - constraint_span: override_trait_constraint.span, - trait_method_name: method.name.0.contents.clone(), - trait_method_span: method.location.span, - }); - } - } - } - - fn reset_generics_on_constraint_type( - typ: &Type, - override_generics: &Generics, - method_generics: &Generics, - trait_impl_generics_len: usize, - trait_generics_len: usize, - ) -> Type { - let recur_generics_reset = |typ: &Type| { - Self::reset_generics_on_constraint_type( - typ, - override_generics, - method_generics, - trait_impl_generics_len, - trait_generics_len, - ) - }; - - match &typ { - Type::NamedGeneric(type_var, name, _) => { - let generic_index = Self::find_generic_index(name, override_generics); - - let mut bindings: TypeBindings = TypeBindingsMap::new(); - if let Some(mut method_generic_index) = generic_index { - // Override generics from a trait impl can possibly contain generics - // as part of the trait impl that are not part of the trait method generics. - // We need to account for this by checking against the length of the lists for trait impl generics and trait generics. - // It is important to note that trait impl generics are expected to also contain the trait generics, - // and that is why we add the trait generics list length before subtracting the trait impl generics length. - if trait_impl_generics_len > trait_generics_len { - method_generic_index = - method_generic_index + trait_generics_len - trait_impl_generics_len; - } - - // To accurately match against the trait function's constraint types, we must - // replace the name of any generics in the override function with the respective - // name from the original trait. - // This substitution is why we also must recompute the method generic index above. - bindings.insert( - type_var.id(), - ( - method_generics[method_generic_index].type_var.clone(), - Type::NamedGeneric( - TypeVariable::unbound(TypeVariableId(method_generic_index)), - method_generics[method_generic_index].name.clone(), - method_generics[method_generic_index].kind.clone(), - ), - ), - ); - } - - typ.substitute(&bindings) - } - Type::Struct(struct_type, generics) => { - let reset_generics = generics.iter().map(recur_generics_reset).collect(); - Type::Struct(struct_type.clone(), reset_generics) - } - Type::Alias(type_alias, generics) => { - let reset_generics = generics.iter().map(recur_generics_reset).collect(); - Type::Alias(type_alias.clone(), reset_generics) - } - Type::Array(size, element_type) => { - let size = recur_generics_reset(size.as_ref()); - let element_type = recur_generics_reset(element_type.as_ref()); - Type::Array(Box::new(size), Box::new(element_type)) - } - Type::Slice(element_type) => { - let element_type = recur_generics_reset(element_type.as_ref()); - Type::Slice(Box::new(element_type)) - } - Type::String(size) => { - let size = recur_generics_reset(size.as_ref()); - Type::String(Box::new(size)) - } - Type::FmtString(size, element_types) => { - let size = recur_generics_reset(size.as_ref()); - let element_types = recur_generics_reset(element_types.as_ref()); - Type::FmtString(Box::new(size), Box::new(element_types)) - } - Type::Tuple(types) => { - let reset_types = types.iter().map(recur_generics_reset).collect(); - Type::Tuple(reset_types) - } - Type::Function(arguments, return_type, environment) => { - let arguments = arguments.iter().map(recur_generics_reset).collect(); - let return_type = recur_generics_reset(return_type.as_ref()); - let environment = recur_generics_reset(environment.as_ref()); - Type::Function(arguments, Box::new(return_type), Box::new(environment)) - } - Type::MutableReference(typ) => { - let typ = recur_generics_reset(typ.as_ref()); - Type::MutableReference(Box::new(typ)) - } - _ => typ.clone(), - } - } - - fn find_generic_index(target_name: &str, generics: &Generics) -> Option { - let mut generic_index = None; - let mut index = 0; - for generic in generics.iter() { - if generic.name.as_str() == "Self" { - continue; - } - - if generic.name.as_str() == target_name { - generic_index = Some(index); - break; - } - - index += 1; - } - generic_index - } - - fn check_trait_impl_crate_coherence( - &mut self, - trait_id: TraitId, - trait_impl: &UnresolvedTraitImpl, - ) { - self.local_module = trait_impl.module_id; - self.file = trait_impl.file_id; - - let object_crate = match &trait_impl.resolved_object_type { - Some(Type::Struct(struct_type, _)) => struct_type.borrow().id.krate(), - _ => CrateId::Dummy, - }; - - let the_trait = self.interner.get_trait(trait_id); - if self.crate_id != the_trait.crate_id && self.crate_id != object_crate { - self.push_err(DefCollectorErrorKind::TraitImplOrphaned { - span: trait_impl.object_type.span.expect("object type must have a span"), - }); - } - } - fn define_type_alias(&mut self, alias_id: TypeAliasId, alias: UnresolvedTypeAlias) { self.file = alias.file_id; self.local_module = alias.module_id; diff --git a/compiler/noirc_frontend/src/elaborator/trait_impls.rs b/compiler/noirc_frontend/src/elaborator/trait_impls.rs new file mode 100644 index 00000000000..abb84662a3b --- /dev/null +++ b/compiler/noirc_frontend/src/elaborator/trait_impls.rs @@ -0,0 +1,336 @@ +use std::collections::HashMap as TypeBindingsMap; + +use crate::{ + graph::CrateId, + hir::def_collector::{dc_crate::UnresolvedTraitImpl, errors::DefCollectorErrorKind}, +}; +use crate::{ + hir::def_collector::errors::DuplicateType, + hir_def::{ + traits::{TraitConstraint, TraitFunction}, + types::Generics, + }, + node_interner::{FuncId, TraitId}, + Type, TypeBindings, TypeVariable, TypeVariableId, +}; + +use noirc_errors::Location; +use rustc_hash::FxHashSet as HashSet; + +use super::Elaborator; + +impl<'context> Elaborator<'context> { + pub(super) fn collect_trait_impl_methods( + &mut self, + trait_id: TraitId, + trait_impl: &mut UnresolvedTraitImpl, + trait_impl_where_clause: &[TraitConstraint], + ) { + self.local_module = trait_impl.module_id; + self.file = trait_impl.file_id; + + // In this Vec methods[i] corresponds to trait.methods[i]. If the impl has no implementation + // for a particular method, the default implementation will be added at that slot. + let mut ordered_methods = Vec::new(); + + // check whether the trait implementation is in the same crate as either the trait or the type + self.check_trait_impl_crate_coherence(trait_id, trait_impl); + + // set of function ids that have a corresponding method in the trait + let mut func_ids_in_trait = HashSet::default(); + + // Temporarily take ownership of the trait's methods so we can iterate over them + // while also mutating the interner + let the_trait = self.interner.get_trait_mut(trait_id); + let methods = std::mem::take(&mut the_trait.methods); + for method in &methods { + let overrides: Vec<_> = trait_impl + .methods + .functions + .iter() + .filter(|(_, _, f)| f.name() == method.name.0.contents) + .collect(); + + if overrides.is_empty() { + if let Some(default_impl) = &method.default_impl { + // copy 'where' clause from unresolved trait impl + let mut default_impl_clone = default_impl.clone(); + default_impl_clone.def.where_clause.extend(trait_impl.where_clause.clone()); + + let func_id = self.interner.push_empty_fn(); + let module = self.module_id(); + let location = Location::new(default_impl.def.span, trait_impl.file_id); + self.interner.push_function(func_id, &default_impl.def, module, location); + self.define_function_meta(&mut default_impl_clone, func_id, false); + func_ids_in_trait.insert(func_id); + ordered_methods.push(( + method.default_impl_module_id, + func_id, + *default_impl_clone, + )); + } else { + self.push_err(DefCollectorErrorKind::TraitMissingMethod { + trait_name: self.interner.get_trait(trait_id).name.clone(), + method_name: method.name.clone(), + trait_impl_span: trait_impl + .object_type + .span + .expect("type must have a span"), + }); + } + } else { + for (_, func_id, _) in &overrides { + self.check_where_clause_against_trait( + func_id, + method, + trait_impl_where_clause, + trait_impl.resolved_generics.len(), + trait_impl.resolved_trait_generics.len(), + ); + + func_ids_in_trait.insert(*func_id); + } + + if overrides.len() > 1 { + self.push_err(DefCollectorErrorKind::Duplicate { + typ: DuplicateType::TraitAssociatedFunction, + first_def: overrides[0].2.name_ident().clone(), + second_def: overrides[1].2.name_ident().clone(), + }); + } + + ordered_methods.push(overrides[0].clone()); + } + } + + // Restore the methods that were taken before the for loop + let the_trait = self.interner.get_trait_mut(trait_id); + the_trait.set_methods(methods); + + // Emit MethodNotInTrait error for methods in the impl block that + // don't have a corresponding method signature defined in the trait + for (_, func_id, func) in &trait_impl.methods.functions { + if !func_ids_in_trait.contains(func_id) { + let trait_name = the_trait.name.clone(); + let impl_method = func.name_ident().clone(); + let error = DefCollectorErrorKind::MethodNotInTrait { trait_name, impl_method }; + self.errors.push((error.into(), self.file)); + } + } + + trait_impl.methods.functions = ordered_methods; + trait_impl.methods.trait_id = Some(trait_id); + } + + /// Issue an error if the impl is stricter than the trait. + /// + /// # Example + /// + /// ``` + /// trait MyTrait { } + /// trait Foo { + /// fn foo(); + /// } + /// impl Foo for () { + /// // Error issued here as `foo` does not have the `MyTrait` constraint + /// fn foo() where B: MyTrait {} + /// } + /// ``` + fn check_where_clause_against_trait( + &mut self, + func_id: &FuncId, + method: &TraitFunction, + trait_impl_where_clause: &[TraitConstraint], + trait_impl_generics_len: usize, + trait_generics_len: usize, + ) { + let func_meta = self.interner.function_meta(func_id); + let override_generics = func_meta.all_generics.clone(); + let method_generics: Vec<_> = method + .all_generics + .clone() + .into_iter() + .filter(|generic| generic.name.as_str() != "Self") + .collect(); + + let mut substituted_method_ids = HashSet::default(); + for method_constraint in method.trait_constraints.iter() { + let substituted_constraint_type = Self::reset_generics_on_constraint_type( + &method_constraint.typ, + &method_generics, + &method_generics, + 0, + 0, + ); + substituted_method_ids + .insert((substituted_constraint_type, method_constraint.trait_id)); + } + + for override_trait_constraint in func_meta.trait_constraints.clone() { + let override_constraint_is_from_impl = + trait_impl_where_clause.iter().any(|impl_constraint| { + impl_constraint.trait_id == override_trait_constraint.trait_id + }); + if override_constraint_is_from_impl { + continue; + } + + let substituted_constraint_type = Self::reset_generics_on_constraint_type( + &override_trait_constraint.typ, + &override_generics, + &method_generics, + trait_impl_generics_len, + trait_generics_len, + ); + + if !substituted_method_ids.contains(&( + substituted_constraint_type.clone(), + override_trait_constraint.trait_id, + )) { + let the_trait = self.interner.get_trait(override_trait_constraint.trait_id); + self.push_err(DefCollectorErrorKind::ImplIsStricterThanTrait { + constraint_typ: override_trait_constraint.typ, + constraint_name: the_trait.name.0.contents.clone(), + constraint_span: override_trait_constraint.span, + trait_method_name: method.name.0.contents.clone(), + trait_method_span: method.location.span, + }); + } + } + } + + fn reset_generics_on_constraint_type( + typ: &Type, + override_generics: &Generics, + method_generics: &Generics, + trait_impl_generics_len: usize, + trait_generics_len: usize, + ) -> Type { + let recur_generics_reset = |typ: &Type| { + Self::reset_generics_on_constraint_type( + typ, + override_generics, + method_generics, + trait_impl_generics_len, + trait_generics_len, + ) + }; + + match &typ { + Type::NamedGeneric(type_var, name, _) => { + let generic_index = Self::find_generic_index(name, override_generics); + + let mut bindings: TypeBindings = TypeBindingsMap::new(); + if let Some(mut method_generic_index) = generic_index { + // Override generics from a trait impl can possibly contain generics + // as part of the trait impl that are not part of the trait method generics. + // We need to account for this by checking against the length of the lists for trait impl generics and trait generics. + // It is important to note that trait impl generics are expected to also contain the trait generics, + // and that is why we add the trait generics list length before subtracting the trait impl generics length. + if trait_impl_generics_len > trait_generics_len { + method_generic_index = + method_generic_index + trait_generics_len - trait_impl_generics_len; + } + + // To accurately match against the trait function's constraint types, we must + // replace the name of any generics in the override function with the respective + // name from the original trait. + // This substitution is why we also must recompute the method generic index above. + bindings.insert( + type_var.id(), + ( + method_generics[method_generic_index].type_var.clone(), + Type::NamedGeneric( + TypeVariable::unbound(TypeVariableId(method_generic_index)), + method_generics[method_generic_index].name.clone(), + method_generics[method_generic_index].kind.clone(), + ), + ), + ); + } + + typ.substitute(&bindings) + } + Type::Struct(struct_type, generics) => { + let reset_generics = generics.iter().map(recur_generics_reset).collect(); + Type::Struct(struct_type.clone(), reset_generics) + } + Type::Alias(type_alias, generics) => { + let reset_generics = generics.iter().map(recur_generics_reset).collect(); + Type::Alias(type_alias.clone(), reset_generics) + } + Type::Array(size, element_type) => { + let size = recur_generics_reset(size.as_ref()); + let element_type = recur_generics_reset(element_type.as_ref()); + Type::Array(Box::new(size), Box::new(element_type)) + } + Type::Slice(element_type) => { + let element_type = recur_generics_reset(element_type.as_ref()); + Type::Slice(Box::new(element_type)) + } + Type::String(size) => { + let size = recur_generics_reset(size.as_ref()); + Type::String(Box::new(size)) + } + Type::FmtString(size, element_types) => { + let size = recur_generics_reset(size.as_ref()); + let element_types = recur_generics_reset(element_types.as_ref()); + Type::FmtString(Box::new(size), Box::new(element_types)) + } + Type::Tuple(types) => { + let reset_types = types.iter().map(recur_generics_reset).collect(); + Type::Tuple(reset_types) + } + Type::Function(arguments, return_type, environment) => { + let arguments = arguments.iter().map(recur_generics_reset).collect(); + let return_type = recur_generics_reset(return_type.as_ref()); + let environment = recur_generics_reset(environment.as_ref()); + Type::Function(arguments, Box::new(return_type), Box::new(environment)) + } + Type::MutableReference(typ) => { + let typ = recur_generics_reset(typ.as_ref()); + Type::MutableReference(Box::new(typ)) + } + _ => typ.clone(), + } + } + + fn find_generic_index(target_name: &str, generics: &Generics) -> Option { + let mut generic_index = None; + let mut index = 0; + for generic in generics.iter() { + if generic.name.as_str() == "Self" { + continue; + } + + if generic.name.as_str() == target_name { + generic_index = Some(index); + break; + } + + index += 1; + } + generic_index + } + + fn check_trait_impl_crate_coherence( + &mut self, + trait_id: TraitId, + trait_impl: &UnresolvedTraitImpl, + ) { + self.local_module = trait_impl.module_id; + self.file = trait_impl.file_id; + + let object_crate = match &trait_impl.resolved_object_type { + Some(Type::Struct(struct_type, _)) => struct_type.borrow().id.krate(), + _ => CrateId::Dummy, + }; + + let the_trait = self.interner.get_trait(trait_id); + if self.crate_id != the_trait.crate_id && self.crate_id != object_crate { + self.push_err(DefCollectorErrorKind::TraitImplOrphaned { + span: trait_impl.object_type.span.expect("object type must have a span"), + }); + } + } +} From b069bd2741c910efd04f407ae3a4590f7f64450a Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Tue, 16 Jul 2024 18:29:44 +0000 Subject: [PATCH 15/27] cleanup --- compiler/noirc_frontend/src/elaborator/trait_impls.rs | 6 +++--- compiler/noirc_frontend/src/elaborator/traits.rs | 3 --- compiler/noirc_frontend/src/hir_def/types.rs | 2 +- 3 files changed, 4 insertions(+), 7 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/trait_impls.rs b/compiler/noirc_frontend/src/elaborator/trait_impls.rs index abb84662a3b..ce59b937fc4 100644 --- a/compiler/noirc_frontend/src/elaborator/trait_impls.rs +++ b/compiler/noirc_frontend/src/elaborator/trait_impls.rs @@ -299,9 +299,9 @@ impl<'context> Elaborator<'context> { let mut generic_index = None; let mut index = 0; for generic in generics.iter() { - if generic.name.as_str() == "Self" { - continue; - } + // if generic.name.as_str() == "Self" { + // continue; + // } if generic.name.as_str() == target_name { generic_index = Some(index); diff --git a/compiler/noirc_frontend/src/elaborator/traits.rs b/compiler/noirc_frontend/src/elaborator/traits.rs index cb063ecfa1a..e33a2ab1697 100644 --- a/compiler/noirc_frontend/src/elaborator/traits.rs +++ b/compiler/noirc_frontend/src/elaborator/traits.rs @@ -116,9 +116,6 @@ impl<'context> Elaborator<'context> { let func_id = unresolved_trait.method_ids[&name.0.contents]; - // this.recover_generics(|this| { - - // }); this.resolve_trait_function( name, generics, diff --git a/compiler/noirc_frontend/src/hir_def/types.rs b/compiler/noirc_frontend/src/hir_def/types.rs index b0ed9f6dbdb..91a374a0787 100644 --- a/compiler/noirc_frontend/src/hir_def/types.rs +++ b/compiler/noirc_frontend/src/hir_def/types.rs @@ -1753,7 +1753,7 @@ impl Type { } } - pub(crate) fn type_variable_id(&self) -> Option { + fn type_variable_id(&self) -> Option { match self { Type::TypeVariable(variable, _) | Type::NamedGeneric(variable, _, _) => { Some(variable.0) From 8f0af0853f1cda329cd77653ebadf46fb06952a0 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Tue, 16 Jul 2024 18:44:10 +0000 Subject: [PATCH 16/27] add comments to reset_generics_on_constraint_type --- .../src/elaborator/trait_impls.rs | 45 ++++++++++++------- compiler/noirc_frontend/src/tests.rs | 2 - 2 files changed, 30 insertions(+), 17 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/trait_impls.rs b/compiler/noirc_frontend/src/elaborator/trait_impls.rs index ce59b937fc4..9f094de8192 100644 --- a/compiler/noirc_frontend/src/elaborator/trait_impls.rs +++ b/compiler/noirc_frontend/src/elaborator/trait_impls.rs @@ -199,6 +199,35 @@ impl<'context> Elaborator<'context> { } } + /// Resets the generics of a trait impl's trait constraint type. + /// This helps us match the trait constraint type from a trait itself and its trait impl. + /// If a type contains named generics, this method will reset the type variable ids + /// of those generics based off its position in the actual trait definition's generics. + /// + /// Example + /// + /// ``` + /// trait MyTrait { } + /// trait Foo { + /// fn foo_good() where T: MyTrait; + /// + /// fn foo_bad() where T: MyTrait; + /// } + /// impl Foo for () { + /// // Error issued here as `foo` does not have the `MyTrait` constraint + /// fn foo_good() where A: MyTrait {} + /// + /// fn foo_bad() where B: MyTrait {} + /// ``` + /// `A` in `A: MyTrait` will have an actual type variable id based upon the global interner current type variable id. + /// In the example, above we will have A -> '2. However, we want to compare again T -> '0. + /// We can find `A`'s position among the trait generics and reset it to be T -> '0. + /// + /// On the flip side, `B` in `B: MyTrait` will be reset to T -> '1. + /// This will not match T -> '0 and we can mark that these types are unequal and our impl is stricter than the trait. + /// + /// The last two fields `trait_impl_generics_len` and `trait_generics_len` are only necessary to account + /// for extra impl generics when indexing into the trait definition's generics. fn reset_generics_on_constraint_type( typ: &Type, override_generics: &Generics, @@ -296,21 +325,7 @@ impl<'context> Elaborator<'context> { } fn find_generic_index(target_name: &str, generics: &Generics) -> Option { - let mut generic_index = None; - let mut index = 0; - for generic in generics.iter() { - // if generic.name.as_str() == "Self" { - // continue; - // } - - if generic.name.as_str() == target_name { - generic_index = Some(index); - break; - } - - index += 1; - } - generic_index + generics.iter().position(|generic| generic.name.as_str() == target_name) } fn check_trait_impl_crate_coherence( diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index 4a9bbe6bcd4..e753f2902c3 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -2088,7 +2088,6 @@ fn impl_stricter_than_trait_different_object_generics() { "#; let errors = get_program_errors(src); - dbg!(errors.clone()); assert_eq!(errors.len(), 3); if let CompilationError::DefinitionError(DefCollectorErrorKind::ImplIsStricterThanTrait { constraint_typ, @@ -2120,7 +2119,6 @@ fn impl_stricter_than_trait_different_object_generics() { .. }) = &errors[2].0 { - dbg!(constraint_typ.to_string().as_str()); assert!(matches!(constraint_typ.to_string().as_str(), "(Option, Option)")); assert!(matches!(constraint_name.as_str(), "MyTrait")); } else { From 19840eac64f85a9b48f9bd2221dd5349df041812 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Tue, 16 Jul 2024 19:18:46 +0000 Subject: [PATCH 17/27] fix up method_generic_index reassignment --- compiler/noirc_frontend/src/elaborator/trait_impls.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_frontend/src/elaborator/trait_impls.rs b/compiler/noirc_frontend/src/elaborator/trait_impls.rs index 9f094de8192..e60e72fda3b 100644 --- a/compiler/noirc_frontend/src/elaborator/trait_impls.rs +++ b/compiler/noirc_frontend/src/elaborator/trait_impls.rs @@ -256,7 +256,7 @@ impl<'context> Elaborator<'context> { // We need to account for this by checking against the length of the lists for trait impl generics and trait generics. // It is important to note that trait impl generics are expected to also contain the trait generics, // and that is why we add the trait generics list length before subtracting the trait impl generics length. - if trait_impl_generics_len > trait_generics_len { + if trait_impl_generics_len > trait_generics_len && (method_generic_index + trait_generics_len) >= trait_impl_generics_len { method_generic_index = method_generic_index + trait_generics_len - trait_impl_generics_len; } From baa0561b6993bea6d918eef3cb793f1b09b7c1d6 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Tue, 16 Jul 2024 19:21:06 +0000 Subject: [PATCH 18/27] remove unnecessary dbg --- compiler/noirc_frontend/src/hir/def_map/module_data.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 62ece65c6de..488ccc476d7 100644 --- a/compiler/noirc_frontend/src/hir/def_map/module_data.rs +++ b/compiler/noirc_frontend/src/hir/def_map/module_data.rs @@ -108,7 +108,7 @@ impl ModuleData { } pub fn find_func_with_name(&self, name: &Ident) -> Option { - dbg!(&self.scope).find_func_with_name(name) + self.scope.find_func_with_name(name) } pub fn import( From b39c76b095dca0294ccce5cf95e243126ad84d29 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Tue, 16 Jul 2024 19:47:51 +0000 Subject: [PATCH 19/27] try a hacK --- compiler/noirc_frontend/src/elaborator/trait_impls.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/compiler/noirc_frontend/src/elaborator/trait_impls.rs b/compiler/noirc_frontend/src/elaborator/trait_impls.rs index e60e72fda3b..5035ff117ef 100644 --- a/compiler/noirc_frontend/src/elaborator/trait_impls.rs +++ b/compiler/noirc_frontend/src/elaborator/trait_impls.rs @@ -256,7 +256,11 @@ impl<'context> Elaborator<'context> { // We need to account for this by checking against the length of the lists for trait impl generics and trait generics. // It is important to note that trait impl generics are expected to also contain the trait generics, // and that is why we add the trait generics list length before subtracting the trait impl generics length. - if trait_impl_generics_len > trait_generics_len && (method_generic_index + trait_generics_len) >= trait_impl_generics_len { + if (method_generic_index + trait_generics_len) >= trait_impl_generics_len { + return typ.clone(); + } + + if trait_impl_generics_len > trait_generics_len { method_generic_index = method_generic_index + trait_generics_len - trait_impl_generics_len; } From 66328d8db5db06b34d04f61d787300dc587a22ad Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Tue, 16 Jul 2024 19:59:33 +0000 Subject: [PATCH 20/27] fix comparison direction --- compiler/noirc_frontend/src/elaborator/trait_impls.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_frontend/src/elaborator/trait_impls.rs b/compiler/noirc_frontend/src/elaborator/trait_impls.rs index 5035ff117ef..6d894f9305a 100644 --- a/compiler/noirc_frontend/src/elaborator/trait_impls.rs +++ b/compiler/noirc_frontend/src/elaborator/trait_impls.rs @@ -256,7 +256,7 @@ impl<'context> Elaborator<'context> { // We need to account for this by checking against the length of the lists for trait impl generics and trait generics. // It is important to note that trait impl generics are expected to also contain the trait generics, // and that is why we add the trait generics list length before subtracting the trait impl generics length. - if (method_generic_index + trait_generics_len) >= trait_impl_generics_len { + if (method_generic_index + trait_generics_len) < trait_impl_generics_len { return typ.clone(); } From 74693fdb38e8d7288e97566f73014229a0f3f4ac Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Tue, 16 Jul 2024 20:04:30 +0000 Subject: [PATCH 21/27] cargo fmt --- compiler/noirc_frontend/src/elaborator/trait_impls.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_frontend/src/elaborator/trait_impls.rs b/compiler/noirc_frontend/src/elaborator/trait_impls.rs index 6d894f9305a..592319c9839 100644 --- a/compiler/noirc_frontend/src/elaborator/trait_impls.rs +++ b/compiler/noirc_frontend/src/elaborator/trait_impls.rs @@ -259,7 +259,7 @@ impl<'context> Elaborator<'context> { if (method_generic_index + trait_generics_len) < trait_impl_generics_len { return typ.clone(); } - + if trait_impl_generics_len > trait_generics_len { method_generic_index = method_generic_index + trait_generics_len - trait_impl_generics_len; From 014643a60033555200202d5cfd4cdb5937528577 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 17 Jul 2024 13:54:28 +0000 Subject: [PATCH 22/27] simpler strategy using just substitute --- .../src/elaborator/trait_impls.rs | 200 +++--------------- .../noirc_frontend/src/elaborator/traits.rs | 1 + compiler/noirc_frontend/src/hir_def/traits.rs | 1 + compiler/noirc_frontend/src/tests.rs | 1 + 4 files changed, 36 insertions(+), 167 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/trait_impls.rs b/compiler/noirc_frontend/src/elaborator/trait_impls.rs index 592319c9839..431df69f254 100644 --- a/compiler/noirc_frontend/src/elaborator/trait_impls.rs +++ b/compiler/noirc_frontend/src/elaborator/trait_impls.rs @@ -1,8 +1,7 @@ -use std::collections::HashMap as TypeBindingsMap; - use crate::{ graph::CrateId, hir::def_collector::{dc_crate::UnresolvedTraitImpl, errors::DefCollectorErrorKind}, + ResolvedGeneric, }; use crate::{ hir::def_collector::errors::DuplicateType, @@ -11,7 +10,7 @@ use crate::{ types::Generics, }, node_interner::{FuncId, TraitId}, - Type, TypeBindings, TypeVariable, TypeVariableId, + Type, TypeBindings, }; use noirc_errors::Location; @@ -39,6 +38,8 @@ impl<'context> Elaborator<'context> { // set of function ids that have a corresponding method in the trait let mut func_ids_in_trait = HashSet::default(); + let trait_generics = &self.interner.get_trait(trait_id).generics.clone(); + // let trait_generics = &self.interner.get_trait(trait_id).generics; // Temporarily take ownership of the trait's methods so we can iterate over them // while also mutating the interner let the_trait = self.interner.get_trait_mut(trait_id); @@ -84,8 +85,8 @@ impl<'context> Elaborator<'context> { func_id, method, trait_impl_where_clause, - trait_impl.resolved_generics.len(), - trait_impl.resolved_trait_generics.len(), + &trait_impl.resolved_trait_generics, + trait_generics, ); func_ids_in_trait.insert(*func_id); @@ -126,7 +127,7 @@ impl<'context> Elaborator<'context> { /// /// # Example /// - /// ``` + /// ```compile_fail /// trait MyTrait { } /// trait Foo { /// fn foo(); @@ -141,32 +142,38 @@ impl<'context> Elaborator<'context> { func_id: &FuncId, method: &TraitFunction, trait_impl_where_clause: &[TraitConstraint], - trait_impl_generics_len: usize, - trait_generics_len: usize, + impl_trait_generics: &[Type], + trait_generics: &Generics, ) { - let func_meta = self.interner.function_meta(func_id); - let override_generics = func_meta.all_generics.clone(); - let method_generics: Vec<_> = method - .all_generics - .clone() - .into_iter() - .filter(|generic| generic.name.as_str() != "Self") - .collect(); + let mut bindings = TypeBindings::new(); + for (trait_generic, impl_trait_generic) in trait_generics.iter().zip(impl_trait_generics) { + bindings.insert( + trait_generic.type_var.id(), + (trait_generic.type_var.clone(), impl_trait_generic.clone()), + ); + } + + let override_meta = self.interner.function_meta(func_id); + // Substitute each generic on the trait function with the corresponding generic on the impl function + for ( + ResolvedGeneric { type_var: trait_fn_generic, .. }, + ResolvedGeneric { name, type_var: impl_fn_generic, kind, .. }, + ) in method.direct_generics.iter().zip(&override_meta.direct_generics) + { + let arg = Type::NamedGeneric(impl_fn_generic.clone(), name.clone(), kind.clone()); + bindings.insert(trait_fn_generic.id(), (trait_fn_generic.clone(), arg)); + } + // dbg!(bindings.clone()); + // dbg!(method.trait_constraints.clone()); let mut substituted_method_ids = HashSet::default(); for method_constraint in method.trait_constraints.iter() { - let substituted_constraint_type = Self::reset_generics_on_constraint_type( - &method_constraint.typ, - &method_generics, - &method_generics, - 0, - 0, - ); + let substituted_constraint_type = method_constraint.typ.substitute(&bindings); substituted_method_ids .insert((substituted_constraint_type, method_constraint.trait_id)); } - - for override_trait_constraint in func_meta.trait_constraints.clone() { + // dbg!(substituted_method_ids.clone()); + for override_trait_constraint in override_meta.trait_constraints.clone() { let override_constraint_is_from_impl = trait_impl_where_clause.iter().any(|impl_constraint| { impl_constraint.trait_id == override_trait_constraint.trait_id @@ -175,16 +182,8 @@ impl<'context> Elaborator<'context> { continue; } - let substituted_constraint_type = Self::reset_generics_on_constraint_type( - &override_trait_constraint.typ, - &override_generics, - &method_generics, - trait_impl_generics_len, - trait_generics_len, - ); - if !substituted_method_ids.contains(&( - substituted_constraint_type.clone(), + override_trait_constraint.typ.clone(), override_trait_constraint.trait_id, )) { let the_trait = self.interner.get_trait(override_trait_constraint.trait_id); @@ -199,139 +198,6 @@ impl<'context> Elaborator<'context> { } } - /// Resets the generics of a trait impl's trait constraint type. - /// This helps us match the trait constraint type from a trait itself and its trait impl. - /// If a type contains named generics, this method will reset the type variable ids - /// of those generics based off its position in the actual trait definition's generics. - /// - /// Example - /// - /// ``` - /// trait MyTrait { } - /// trait Foo { - /// fn foo_good() where T: MyTrait; - /// - /// fn foo_bad() where T: MyTrait; - /// } - /// impl Foo for () { - /// // Error issued here as `foo` does not have the `MyTrait` constraint - /// fn foo_good() where A: MyTrait {} - /// - /// fn foo_bad() where B: MyTrait {} - /// ``` - /// `A` in `A: MyTrait` will have an actual type variable id based upon the global interner current type variable id. - /// In the example, above we will have A -> '2. However, we want to compare again T -> '0. - /// We can find `A`'s position among the trait generics and reset it to be T -> '0. - /// - /// On the flip side, `B` in `B: MyTrait` will be reset to T -> '1. - /// This will not match T -> '0 and we can mark that these types are unequal and our impl is stricter than the trait. - /// - /// The last two fields `trait_impl_generics_len` and `trait_generics_len` are only necessary to account - /// for extra impl generics when indexing into the trait definition's generics. - fn reset_generics_on_constraint_type( - typ: &Type, - override_generics: &Generics, - method_generics: &Generics, - trait_impl_generics_len: usize, - trait_generics_len: usize, - ) -> Type { - let recur_generics_reset = |typ: &Type| { - Self::reset_generics_on_constraint_type( - typ, - override_generics, - method_generics, - trait_impl_generics_len, - trait_generics_len, - ) - }; - - match &typ { - Type::NamedGeneric(type_var, name, _) => { - let generic_index = Self::find_generic_index(name, override_generics); - - let mut bindings: TypeBindings = TypeBindingsMap::new(); - if let Some(mut method_generic_index) = generic_index { - // Override generics from a trait impl can possibly contain generics - // as part of the trait impl that are not part of the trait method generics. - // We need to account for this by checking against the length of the lists for trait impl generics and trait generics. - // It is important to note that trait impl generics are expected to also contain the trait generics, - // and that is why we add the trait generics list length before subtracting the trait impl generics length. - if (method_generic_index + trait_generics_len) < trait_impl_generics_len { - return typ.clone(); - } - - if trait_impl_generics_len > trait_generics_len { - method_generic_index = - method_generic_index + trait_generics_len - trait_impl_generics_len; - } - - // To accurately match against the trait function's constraint types, we must - // replace the name of any generics in the override function with the respective - // name from the original trait. - // This substitution is why we also must recompute the method generic index above. - bindings.insert( - type_var.id(), - ( - method_generics[method_generic_index].type_var.clone(), - Type::NamedGeneric( - TypeVariable::unbound(TypeVariableId(method_generic_index)), - method_generics[method_generic_index].name.clone(), - method_generics[method_generic_index].kind.clone(), - ), - ), - ); - } - - typ.substitute(&bindings) - } - Type::Struct(struct_type, generics) => { - let reset_generics = generics.iter().map(recur_generics_reset).collect(); - Type::Struct(struct_type.clone(), reset_generics) - } - Type::Alias(type_alias, generics) => { - let reset_generics = generics.iter().map(recur_generics_reset).collect(); - Type::Alias(type_alias.clone(), reset_generics) - } - Type::Array(size, element_type) => { - let size = recur_generics_reset(size.as_ref()); - let element_type = recur_generics_reset(element_type.as_ref()); - Type::Array(Box::new(size), Box::new(element_type)) - } - Type::Slice(element_type) => { - let element_type = recur_generics_reset(element_type.as_ref()); - Type::Slice(Box::new(element_type)) - } - Type::String(size) => { - let size = recur_generics_reset(size.as_ref()); - Type::String(Box::new(size)) - } - Type::FmtString(size, element_types) => { - let size = recur_generics_reset(size.as_ref()); - let element_types = recur_generics_reset(element_types.as_ref()); - Type::FmtString(Box::new(size), Box::new(element_types)) - } - Type::Tuple(types) => { - let reset_types = types.iter().map(recur_generics_reset).collect(); - Type::Tuple(reset_types) - } - Type::Function(arguments, return_type, environment) => { - let arguments = arguments.iter().map(recur_generics_reset).collect(); - let return_type = recur_generics_reset(return_type.as_ref()); - let environment = recur_generics_reset(environment.as_ref()); - Type::Function(arguments, Box::new(return_type), Box::new(environment)) - } - Type::MutableReference(typ) => { - let typ = recur_generics_reset(typ.as_ref()); - Type::MutableReference(Box::new(typ)) - } - _ => typ.clone(), - } - } - - fn find_generic_index(target_name: &str, generics: &Generics) -> Option { - generics.iter().position(|generic| generic.name.as_str() == target_name) - } - fn check_trait_impl_crate_coherence( &mut self, trait_id: TraitId, diff --git a/compiler/noirc_frontend/src/elaborator/traits.rs b/compiler/noirc_frontend/src/elaborator/traits.rs index 522f8f55576..367e12c950e 100644 --- a/compiler/noirc_frontend/src/elaborator/traits.rs +++ b/compiler/noirc_frontend/src/elaborator/traits.rs @@ -164,6 +164,7 @@ impl<'context> Elaborator<'context> { default_impl_module_id: unresolved_trait.module_id, trait_constraints: func_meta.trait_constraints.clone(), all_generics: func_meta.all_generics.clone(), + direct_generics: func_meta.direct_generics.clone(), }); }); } diff --git a/compiler/noirc_frontend/src/hir_def/traits.rs b/compiler/noirc_frontend/src/hir_def/traits.rs index b4a44947bb9..1cc6ab536dd 100644 --- a/compiler/noirc_frontend/src/hir_def/traits.rs +++ b/compiler/noirc_frontend/src/hir_def/traits.rs @@ -18,6 +18,7 @@ pub struct TraitFunction { pub default_impl_module_id: crate::hir::def_map::LocalModuleId, pub trait_constraints: Vec, pub all_generics: Generics, + pub direct_generics: Generics, } #[derive(Clone, Debug, PartialEq, Eq)] diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index e753f2902c3..b48797d946f 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -2025,6 +2025,7 @@ fn impl_stricter_than_trait_different_generics() { "#; let errors = get_program_errors(src); + dbg!(errors.clone()); assert_eq!(errors.len(), 1); if let CompilationError::DefinitionError(DefCollectorErrorKind::ImplIsStricterThanTrait { constraint_typ, From 2eebf76d9597976aeca019106cf724cf56313db4 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 17 Jul 2024 13:55:29 +0000 Subject: [PATCH 23/27] remove dbgs --- compiler/noirc_frontend/src/elaborator/trait_impls.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/trait_impls.rs b/compiler/noirc_frontend/src/elaborator/trait_impls.rs index 431df69f254..2d2e193b48b 100644 --- a/compiler/noirc_frontend/src/elaborator/trait_impls.rs +++ b/compiler/noirc_frontend/src/elaborator/trait_impls.rs @@ -164,15 +164,13 @@ impl<'context> Elaborator<'context> { bindings.insert(trait_fn_generic.id(), (trait_fn_generic.clone(), arg)); } - // dbg!(bindings.clone()); - // dbg!(method.trait_constraints.clone()); let mut substituted_method_ids = HashSet::default(); for method_constraint in method.trait_constraints.iter() { let substituted_constraint_type = method_constraint.typ.substitute(&bindings); substituted_method_ids .insert((substituted_constraint_type, method_constraint.trait_id)); } - // dbg!(substituted_method_ids.clone()); + for override_trait_constraint in override_meta.trait_constraints.clone() { let override_constraint_is_from_impl = trait_impl_where_clause.iter().any(|impl_constraint| { From bfa8dddf43dd1b449c4bc376a81646d867f28072 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 17 Jul 2024 13:55:46 +0000 Subject: [PATCH 24/27] one more leftover dbg --- compiler/noirc_frontend/src/tests.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index b48797d946f..e753f2902c3 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -2025,7 +2025,6 @@ fn impl_stricter_than_trait_different_generics() { "#; let errors = get_program_errors(src); - dbg!(errors.clone()); assert_eq!(errors.len(), 1); if let CompilationError::DefinitionError(DefCollectorErrorKind::ImplIsStricterThanTrait { constraint_typ, From 8897de81c088983d8c1f5a2c610c19301f0bdded Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 18 Jul 2024 16:13:15 +0000 Subject: [PATCH 25/27] cleanup, remove unnecessary field from TraitFunction --- compiler/noirc_frontend/src/elaborator/traits.rs | 1 - compiler/noirc_frontend/src/hir_def/traits.rs | 1 - 2 files changed, 2 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/traits.rs b/compiler/noirc_frontend/src/elaborator/traits.rs index 367e12c950e..3ae0e9e0e00 100644 --- a/compiler/noirc_frontend/src/elaborator/traits.rs +++ b/compiler/noirc_frontend/src/elaborator/traits.rs @@ -163,7 +163,6 @@ impl<'context> Elaborator<'context> { default_impl, default_impl_module_id: unresolved_trait.module_id, trait_constraints: func_meta.trait_constraints.clone(), - all_generics: func_meta.all_generics.clone(), direct_generics: func_meta.direct_generics.clone(), }); }); diff --git a/compiler/noirc_frontend/src/hir_def/traits.rs b/compiler/noirc_frontend/src/hir_def/traits.rs index 1cc6ab536dd..099c9ea78f7 100644 --- a/compiler/noirc_frontend/src/hir_def/traits.rs +++ b/compiler/noirc_frontend/src/hir_def/traits.rs @@ -17,7 +17,6 @@ pub struct TraitFunction { pub default_impl: Option>, pub default_impl_module_id: crate::hir::def_map::LocalModuleId, pub trait_constraints: Vec, - pub all_generics: Generics, pub direct_generics: Generics, } From 3ce5628e11cb1a37332feac5b5619d41df1b3cd7 Mon Sep 17 00:00:00 2001 From: jfecher Date: Thu, 18 Jul 2024 12:52:40 -0500 Subject: [PATCH 26/27] Update compiler/noirc_frontend/src/elaborator/trait_impls.rs --- compiler/noirc_frontend/src/elaborator/trait_impls.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/compiler/noirc_frontend/src/elaborator/trait_impls.rs b/compiler/noirc_frontend/src/elaborator/trait_impls.rs index 2d2e193b48b..afe82e070d2 100644 --- a/compiler/noirc_frontend/src/elaborator/trait_impls.rs +++ b/compiler/noirc_frontend/src/elaborator/trait_impls.rs @@ -39,7 +39,6 @@ impl<'context> Elaborator<'context> { let mut func_ids_in_trait = HashSet::default(); let trait_generics = &self.interner.get_trait(trait_id).generics.clone(); - // let trait_generics = &self.interner.get_trait(trait_id).generics; // Temporarily take ownership of the trait's methods so we can iterate over them // while also mutating the interner let the_trait = self.interner.get_trait_mut(trait_id); From aebb9755dbca4ce310a5605f46279c41f85dfc67 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 18 Jul 2024 18:17:39 +0000 Subject: [PATCH 27/27] fmt and clippy --- .../src/elaborator/trait_impls.rs | 14 ++++++-- .../src/hir/def_collector/errors.rs | 14 ++++++-- compiler/noirc_frontend/src/tests.rs | 33 +++++++++++++++++++ 3 files changed, 57 insertions(+), 4 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/trait_impls.rs b/compiler/noirc_frontend/src/elaborator/trait_impls.rs index 2d2e193b48b..47fa4ce7723 100644 --- a/compiler/noirc_frontend/src/elaborator/trait_impls.rs +++ b/compiler/noirc_frontend/src/elaborator/trait_impls.rs @@ -167,8 +167,16 @@ impl<'context> Elaborator<'context> { let mut substituted_method_ids = HashSet::default(); for method_constraint in method.trait_constraints.iter() { let substituted_constraint_type = method_constraint.typ.substitute(&bindings); - substituted_method_ids - .insert((substituted_constraint_type, method_constraint.trait_id)); + let substituted_trait_generics = method_constraint + .trait_generics + .iter() + .map(|generic| generic.substitute(&bindings)) + .collect::>(); + substituted_method_ids.insert(( + substituted_constraint_type, + method_constraint.trait_id, + substituted_trait_generics, + )); } for override_trait_constraint in override_meta.trait_constraints.clone() { @@ -183,11 +191,13 @@ impl<'context> Elaborator<'context> { if !substituted_method_ids.contains(&( override_trait_constraint.typ.clone(), override_trait_constraint.trait_id, + override_trait_constraint.trait_generics.clone(), )) { let the_trait = self.interner.get_trait(override_trait_constraint.trait_id); self.push_err(DefCollectorErrorKind::ImplIsStricterThanTrait { constraint_typ: override_trait_constraint.typ, constraint_name: the_trait.name.0.contents.clone(), + constraint_generics: override_trait_constraint.trait_generics, constraint_span: override_trait_constraint.span, trait_method_name: method.name.0.contents.clone(), trait_method_span: method.location.span, diff --git a/compiler/noirc_frontend/src/hir/def_collector/errors.rs b/compiler/noirc_frontend/src/hir/def_collector/errors.rs index 07264bddf69..1ccf8dd4792 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/errors.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/errors.rs @@ -74,6 +74,7 @@ pub enum DefCollectorErrorKind { ImplIsStricterThanTrait { constraint_typ: crate::Type, constraint_name: String, + constraint_generics: Vec, constraint_span: Span, trait_method_name: String, trait_method_span: Span, @@ -259,10 +260,19 @@ impl<'a> From<&'a DefCollectorErrorKind> for Diagnostic { ident.0.span(), ) } - DefCollectorErrorKind::ImplIsStricterThanTrait { constraint_typ, constraint_name: constraint, constraint_span, trait_method_name, trait_method_span } => { + DefCollectorErrorKind::ImplIsStricterThanTrait { constraint_typ, constraint_name, constraint_generics, constraint_span, trait_method_name, trait_method_span } => { + let mut constraint_name_with_generics = constraint_name.to_owned(); + if !constraint_generics.is_empty() { + constraint_name_with_generics.push('<'); + for generic in constraint_generics.iter() { + constraint_name_with_generics.push_str(generic.to_string().as_str()); + } + constraint_name_with_generics.push('>'); + } + let mut diag = Diagnostic::simple_error( "impl has stricter requirements than trait".to_string(), - format!("impl has extra requirement `{constraint_typ}: {constraint}`"), + format!("impl has extra requirement `{constraint_typ}: {constraint_name_with_generics}`"), *constraint_span, ); diag.add_secondary(format!("definition of `{trait_method_name}` from trait"), *trait_method_span); diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index 314e26ecff1..b4f17489ff7 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -2200,6 +2200,39 @@ fn trait_impl_where_clause_stricter_pass() { } } +#[test] +fn impl_stricter_than_trait_different_trait_generics() { + let src = r#" + trait Foo { + fn foo() where T: T2; + } + + impl Foo for () { + // Should be A: T2 + fn foo() where A: T2 {} + } + + trait T2 {} + "#; + + let errors = get_program_errors(src); + assert_eq!(errors.len(), 1); + if let CompilationError::DefinitionError(DefCollectorErrorKind::ImplIsStricterThanTrait { + constraint_typ, + constraint_name, + constraint_generics, + .. + }) = &errors[0].0 + { + dbg!(constraint_name.as_str()); + assert!(matches!(constraint_typ.to_string().as_str(), "A")); + assert!(matches!(constraint_name.as_str(), "T2")); + assert!(matches!(constraint_generics[0].to_string().as_str(), "B")); + } else { + panic!("Expected DefCollectorErrorKind::ImplIsStricterThanTrait but got {:?}", errors[0].0); + } +} + #[test] fn impl_not_found_for_inner_impl() { // We want to guarantee that we get a no impl found error