From 52ba6eadecc78bcc9da39c63d26c661f75226e73 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 13 Jan 2025 15:56:15 -0300 Subject: [PATCH 01/10] Don't extend trait function where clause with trait where clause if no body --- compiler/noirc_frontend/src/elaborator/traits.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/traits.rs b/compiler/noirc_frontend/src/elaborator/traits.rs index 09bd639e31e..fd31b2d8cd8 100644 --- a/compiler/noirc_frontend/src/elaborator/traits.rs +++ b/compiler/noirc_frontend/src/elaborator/traits.rs @@ -127,8 +127,11 @@ impl<'context> Elaborator<'context> { let func_id = unresolved_trait.method_ids[&name.0.contents]; let mut where_clause = where_clause.to_vec(); - // Attach any trait constraints on the trait to the function - where_clause.extend(unresolved_trait.trait_def.where_clause.clone()); + // Attach any trait constraints on the trait to the function, + // but only if the body is going to be type-checked + if body.is_some() { + where_clause.extend(unresolved_trait.trait_def.where_clause.clone()); + } this.resolve_trait_function( trait_id, From 00e779945c31917a85ae479a22b7c8ccbfad39f3 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 13 Jan 2025 16:12:22 -0300 Subject: [PATCH 02/10] Add regression test --- .../regression_7038/Nargo.toml | 7 ++++ .../regression_7038/src/main.nr | 40 +++++++++++++++++++ 2 files changed, 47 insertions(+) create mode 100644 test_programs/compile_success_empty/regression_7038/Nargo.toml create mode 100644 test_programs/compile_success_empty/regression_7038/src/main.nr diff --git a/test_programs/compile_success_empty/regression_7038/Nargo.toml b/test_programs/compile_success_empty/regression_7038/Nargo.toml new file mode 100644 index 00000000000..3c874d4b6e8 --- /dev/null +++ b/test_programs/compile_success_empty/regression_7038/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "regression_7038" +type = "bin" +authors = [""] +compiler_version = ">=0.30.0" + +[dependencies] diff --git a/test_programs/compile_success_empty/regression_7038/src/main.nr b/test_programs/compile_success_empty/regression_7038/src/main.nr new file mode 100644 index 00000000000..793a3f60807 --- /dev/null +++ b/test_programs/compile_success_empty/regression_7038/src/main.nr @@ -0,0 +1,40 @@ +trait BigNumTrait {} + +pub struct MyBigNum; + +impl crate::BigNumTrait for MyBigNum {} + +trait CurveParamsTrait +where + BigNum: BigNumTrait, +{ + fn one(); +} + +pub struct BN254Params; +impl CurveParamsTrait for BN254Params { + + fn one() {} +} + +trait BigCurveTrait { + fn two(); +} + +pub struct BigCurve {} + +type BN254 = BigCurve; + +impl BigCurveTrait for BigCurve +where + BigNum: BigNumTrait, + CurveParams: CurveParamsTrait, +{ + fn two() { + let _ = CurveParams::one(); + } +} + +fn main() { + let _ = BN254::two(); +} From 78702cabeb2b07be1b564c5e90a0976c443acc29 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 13 Jan 2025 16:44:06 -0300 Subject: [PATCH 03/10] Add another regression test to prove this is not the right fix --- .../regression_7038 2/Nargo.toml | 7 ++++ .../regression_7038 2/src/main.nr | 37 +++++++++++++++++++ 2 files changed, 44 insertions(+) create mode 100644 test_programs/compile_success_empty/regression_7038 2/Nargo.toml create mode 100644 test_programs/compile_success_empty/regression_7038 2/src/main.nr diff --git a/test_programs/compile_success_empty/regression_7038 2/Nargo.toml b/test_programs/compile_success_empty/regression_7038 2/Nargo.toml new file mode 100644 index 00000000000..f4f23683eb8 --- /dev/null +++ b/test_programs/compile_success_empty/regression_7038 2/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "regression_7038_2" +type = "bin" +authors = [""] +compiler_version = ">=0.30.0" + +[dependencies] diff --git a/test_programs/compile_success_empty/regression_7038 2/src/main.nr b/test_programs/compile_success_empty/regression_7038 2/src/main.nr new file mode 100644 index 00000000000..7183bbabe93 --- /dev/null +++ b/test_programs/compile_success_empty/regression_7038 2/src/main.nr @@ -0,0 +1,37 @@ +trait BigNumTrait {} + +pub struct MyBigNum; + +impl crate::BigNumTrait for MyBigNum {} + +trait CurveParamsTrait +where + BigNum: BigNumTrait, +{ + fn one() {} +} + +pub struct BN254Params; +impl CurveParamsTrait for BN254Params {} + +trait BigCurveTrait { + fn two(); +} + +pub struct BigCurve {} + +type BN254 = BigCurve; + +impl BigCurveTrait for BigCurve +where + BigNum: BigNumTrait, + CurveParams: CurveParamsTrait, +{ + fn two() { + let _ = CurveParams::one(); + } +} + +fn main() { + let _ = BN254::two(); +} From 41f9ff262e3b649d93191ef10ba7d6f24da99738 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 13 Jan 2025 16:51:40 -0300 Subject: [PATCH 04/10] Undo the fix so we can start from scratch --- compiler/noirc_frontend/src/elaborator/traits.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/traits.rs b/compiler/noirc_frontend/src/elaborator/traits.rs index fd31b2d8cd8..e801f7799e2 100644 --- a/compiler/noirc_frontend/src/elaborator/traits.rs +++ b/compiler/noirc_frontend/src/elaborator/traits.rs @@ -128,10 +128,7 @@ impl<'context> Elaborator<'context> { let mut where_clause = where_clause.to_vec(); // Attach any trait constraints on the trait to the function, - // but only if the body is going to be type-checked - if body.is_some() { - where_clause.extend(unresolved_trait.trait_def.where_clause.clone()); - } + where_clause.extend(unresolved_trait.trait_def.where_clause.clone()); this.resolve_trait_function( trait_id, From 4cb2c3871b9dde9d7ee924fb21e149a8832b3bd8 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 13 Jan 2025 16:57:09 -0300 Subject: [PATCH 05/10] Use correct directory name --- .../{regression_7038 2 => regression_7038_2}/Nargo.toml | 0 .../{regression_7038 2 => regression_7038_2}/src/main.nr | 0 2 files changed, 0 insertions(+), 0 deletions(-) rename test_programs/compile_success_empty/{regression_7038 2 => regression_7038_2}/Nargo.toml (100%) rename test_programs/compile_success_empty/{regression_7038 2 => regression_7038_2}/src/main.nr (100%) diff --git a/test_programs/compile_success_empty/regression_7038 2/Nargo.toml b/test_programs/compile_success_empty/regression_7038_2/Nargo.toml similarity index 100% rename from test_programs/compile_success_empty/regression_7038 2/Nargo.toml rename to test_programs/compile_success_empty/regression_7038_2/Nargo.toml diff --git a/test_programs/compile_success_empty/regression_7038 2/src/main.nr b/test_programs/compile_success_empty/regression_7038_2/src/main.nr similarity index 100% rename from test_programs/compile_success_empty/regression_7038 2/src/main.nr rename to test_programs/compile_success_empty/regression_7038_2/src/main.nr From 133bf59d5940094fb7b5173acc74ddde4c9c085a Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Tue, 14 Jan 2025 08:30:47 -0300 Subject: [PATCH 06/10] Just trying something --- .../noirc_frontend/src/elaborator/patterns.rs | 22 +++++++++---------- 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/patterns.rs b/compiler/noirc_frontend/src/elaborator/patterns.rs index 242f5f0b496..961e7225377 100644 --- a/compiler/noirc_frontend/src/elaborator/patterns.rs +++ b/compiler/noirc_frontend/src/elaborator/patterns.rs @@ -744,18 +744,6 @@ impl<'context> Elaborator<'context> { let (typ, bindings) = self.instantiate(t, bindings, generics, function_generic_count, span, location); - // Push any trait constraints required by this definition to the context - // to be checked later when the type of this variable is further constrained. - 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.push_trait_constraint(constraint, expr_id); - } - } - } - if let ImplKind::TraitMethod(mut method) = ident.impl_kind { method.constraint.apply_bindings(&bindings); if method.assumed { @@ -769,6 +757,16 @@ impl<'context> Elaborator<'context> { // that monomorphization can resolve this trait method to the correct impl. self.push_trait_constraint(method.constraint, expr_id); } + } else if let Some(definition) = self.interner.try_definition(ident.id) { + // Push any trait constraints required by this definition to the context + // to be checked later when the type of this variable is further constrained. + 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.push_trait_constraint(constraint, expr_id); + } + } } self.interner.store_instantiation_bindings(expr_id, bindings); From a52a95d4c1343274a1f77c9089bd2c4682e3db24 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Tue, 14 Jan 2025 09:07:18 -0300 Subject: [PATCH 07/10] Revert "Just trying something" This reverts commit 133bf59d5940094fb7b5173acc74ddde4c9c085a. --- .../noirc_frontend/src/elaborator/patterns.rs | 22 ++++++++++--------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/patterns.rs b/compiler/noirc_frontend/src/elaborator/patterns.rs index 961e7225377..242f5f0b496 100644 --- a/compiler/noirc_frontend/src/elaborator/patterns.rs +++ b/compiler/noirc_frontend/src/elaborator/patterns.rs @@ -744,6 +744,18 @@ impl<'context> Elaborator<'context> { let (typ, bindings) = self.instantiate(t, bindings, generics, function_generic_count, span, location); + // Push any trait constraints required by this definition to the context + // to be checked later when the type of this variable is further constrained. + 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.push_trait_constraint(constraint, expr_id); + } + } + } + if let ImplKind::TraitMethod(mut method) = ident.impl_kind { method.constraint.apply_bindings(&bindings); if method.assumed { @@ -757,16 +769,6 @@ impl<'context> Elaborator<'context> { // that monomorphization can resolve this trait method to the correct impl. self.push_trait_constraint(method.constraint, expr_id); } - } else if let Some(definition) = self.interner.try_definition(ident.id) { - // Push any trait constraints required by this definition to the context - // to be checked later when the type of this variable is further constrained. - 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.push_trait_constraint(constraint, expr_id); - } - } } self.interner.store_instantiation_bindings(expr_id, bindings); From 29f163a5b03222004b6c7d025a270258cb352704 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Tue, 14 Jan 2025 09:57:45 -0300 Subject: [PATCH 08/10] Add another regression test --- .../regression_7038_2/src/main.nr | 2 + .../regression_7038_3/Nargo.toml | 7 ++++ .../regression_7038_3/src/main.nr | 40 +++++++++++++++++++ 3 files changed, 49 insertions(+) create mode 100644 test_programs/compile_success_empty/regression_7038_3/Nargo.toml create mode 100644 test_programs/compile_success_empty/regression_7038_3/src/main.nr diff --git a/test_programs/compile_success_empty/regression_7038_2/src/main.nr b/test_programs/compile_success_empty/regression_7038_2/src/main.nr index 7183bbabe93..6a116bb0722 100644 --- a/test_programs/compile_success_empty/regression_7038_2/src/main.nr +++ b/test_programs/compile_success_empty/regression_7038_2/src/main.nr @@ -8,6 +8,8 @@ trait CurveParamsTrait where BigNum: BigNumTrait, { + // The difference between this and regression_7083 is that here + // this is a default method. fn one() {} } diff --git a/test_programs/compile_success_empty/regression_7038_3/Nargo.toml b/test_programs/compile_success_empty/regression_7038_3/Nargo.toml new file mode 100644 index 00000000000..65bc946c559 --- /dev/null +++ b/test_programs/compile_success_empty/regression_7038_3/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "regression_7038_3" +type = "bin" +authors = [""] +compiler_version = ">=0.30.0" + +[dependencies] diff --git a/test_programs/compile_success_empty/regression_7038_3/src/main.nr b/test_programs/compile_success_empty/regression_7038_3/src/main.nr new file mode 100644 index 00000000000..1b6bf0b72d5 --- /dev/null +++ b/test_programs/compile_success_empty/regression_7038_3/src/main.nr @@ -0,0 +1,40 @@ +trait BigNumTrait {} + +pub struct MyBigNum; + +impl crate::BigNumTrait for MyBigNum {} + +trait CurveParamsTrait { + // The difference between this and regression_7038 and regression_7038_2 is that + // here the where clause is on the method, not the trait + fn one() + where + BigNum: BigNumTrait; +} + +pub struct BN254Params; +impl CurveParamsTrait for BN254Params { + fn one() {} +} + +trait BigCurveTrait { + fn two(); +} + +pub struct BigCurve {} + +type BN254 = BigCurve; + +impl BigCurveTrait for BigCurve +where + BigNum: BigNumTrait, + CurveParams: CurveParamsTrait, +{ + fn two() { + let _ = CurveParams::one(); + } +} + +fn main() { + let _ = BN254::two(); +} From 01f46367475eae6ecfaa54efb2655907a148a14b Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Tue, 14 Jan 2025 14:13:25 -0300 Subject: [PATCH 09/10] Add another regression test --- .../regression_7038_4/Nargo.toml | 7 +++++ .../regression_7038_4/src/main.nr | 29 +++++++++++++++++++ 2 files changed, 36 insertions(+) create mode 100644 test_programs/compile_success_empty/regression_7038_4/Nargo.toml create mode 100644 test_programs/compile_success_empty/regression_7038_4/src/main.nr diff --git a/test_programs/compile_success_empty/regression_7038_4/Nargo.toml b/test_programs/compile_success_empty/regression_7038_4/Nargo.toml new file mode 100644 index 00000000000..435c8094c70 --- /dev/null +++ b/test_programs/compile_success_empty/regression_7038_4/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "regression_7038_4" +type = "bin" +authors = [""] +compiler_version = ">=0.30.0" + +[dependencies] diff --git a/test_programs/compile_success_empty/regression_7038_4/src/main.nr b/test_programs/compile_success_empty/regression_7038_4/src/main.nr new file mode 100644 index 00000000000..524f05a5022 --- /dev/null +++ b/test_programs/compile_success_empty/regression_7038_4/src/main.nr @@ -0,0 +1,29 @@ +// This program is a reduction of regression_7038_3 that led to a monomorphizer crash +trait BigNumTrait {} + +trait CurveParamsTrait { + fn one() + where + BigNum: BigNumTrait; +} + +pub struct MyBigNum; + +impl BigNumTrait for MyBigNum {} + +pub struct Params; +impl CurveParamsTrait for Params { + + fn one() {} +} + +fn foo() +where + C: CurveParamsTrait, +{ + let _ = C::one(); +} + +fn main() { + foo::(); +} From f9f5088801b49c8d31f7670062a5c44e33fbb705 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Tue, 14 Jan 2025 14:17:01 -0300 Subject: [PATCH 10/10] fix: don't always select trait impl when verifying trait constraints --- .../noirc_frontend/src/elaborator/expressions.rs | 5 ++++- compiler/noirc_frontend/src/elaborator/mod.rs | 9 +++++++-- .../noirc_frontend/src/elaborator/patterns.rs | 12 ++++++++++-- compiler/noirc_frontend/src/elaborator/types.rs | 15 ++++++++++++--- 4 files changed, 33 insertions(+), 8 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/expressions.rs b/compiler/noirc_frontend/src/elaborator/expressions.rs index 1b3bc645cbc..33af075aebd 100644 --- a/compiler/noirc_frontend/src/elaborator/expressions.rs +++ b/compiler/noirc_frontend/src/elaborator/expressions.rs @@ -775,7 +775,10 @@ impl<'context> Elaborator<'context> { span, }, }; - self.push_trait_constraint(constraint, expr_id); + self.push_trait_constraint( + constraint, expr_id, + true, // this constraint should lead to choosing a trait impl + ); self.type_check_operator_method(expr_id, trait_id, operand_type, span); } typ diff --git a/compiler/noirc_frontend/src/elaborator/mod.rs b/compiler/noirc_frontend/src/elaborator/mod.rs index 0f59de60047..ede5a80bd4b 100644 --- a/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/compiler/noirc_frontend/src/elaborator/mod.rs @@ -197,7 +197,11 @@ struct FunctionContext { /// verified at the end of a function. This is because constraints arise /// on each variable, but it is only until function calls when the types /// needed for the trait constraint may become known. - trait_constraints: Vec<(TraitConstraint, ExprId)>, + /// The `select impl` bool indicates whether, after verifying the trait constraint, + /// the resulting trait impl should be the one used for a call (sometimes trait + /// constraints are verified but there's no call associated with them, like in the + /// case of checking generic arguments) + trait_constraints: Vec<(TraitConstraint, ExprId, bool /* select impl */)>, } impl<'context> Elaborator<'context> { @@ -549,7 +553,7 @@ impl<'context> Elaborator<'context> { } } - for (mut constraint, expr_id) in context.trait_constraints { + for (mut constraint, expr_id, select_impl) in context.trait_constraints { let span = self.interner.expr_span(&expr_id); if matches!(&constraint.typ, Type::MutableReference(_)) { @@ -564,6 +568,7 @@ impl<'context> Elaborator<'context> { &constraint.trait_bound.trait_generics.ordered, &constraint.trait_bound.trait_generics.named, expr_id, + select_impl, span, ); } diff --git a/compiler/noirc_frontend/src/elaborator/patterns.rs b/compiler/noirc_frontend/src/elaborator/patterns.rs index 242f5f0b496..6a672866d7e 100644 --- a/compiler/noirc_frontend/src/elaborator/patterns.rs +++ b/compiler/noirc_frontend/src/elaborator/patterns.rs @@ -751,7 +751,11 @@ impl<'context> Elaborator<'context> { let function = self.interner.function_meta(&function); for mut constraint in function.trait_constraints.clone() { constraint.apply_bindings(&bindings); - self.push_trait_constraint(constraint, expr_id); + + self.push_trait_constraint( + constraint, expr_id, + false, // This constraint shouldn't lead to choosing a trait impl method + ); } } } @@ -767,7 +771,11 @@ impl<'context> Elaborator<'context> { // Currently only one impl can be selected per expr_id, so this // constraint needs to be pushed after any other constraints so // that monomorphization can resolve this trait method to the correct impl. - self.push_trait_constraint(method.constraint, expr_id); + self.push_trait_constraint( + method.constraint, + expr_id, + true, // this constraint should lead to choosing a trait impl method + ); } } diff --git a/compiler/noirc_frontend/src/elaborator/types.rs b/compiler/noirc_frontend/src/elaborator/types.rs index 8d6447d19da..42e6f638339 100644 --- a/compiler/noirc_frontend/src/elaborator/types.rs +++ b/compiler/noirc_frontend/src/elaborator/types.rs @@ -1810,6 +1810,7 @@ impl<'context> Elaborator<'context> { (expr_span, empty_function) } + #[allow(clippy::too_many_arguments)] pub fn verify_trait_constraint( &mut self, object_type: &Type, @@ -1817,6 +1818,7 @@ impl<'context> Elaborator<'context> { trait_generics: &[Type], associated_types: &[NamedType], function_ident_id: ExprId, + select_impl: bool, span: Span, ) { match self.interner.lookup_trait_implementation( @@ -1826,7 +1828,9 @@ impl<'context> Elaborator<'context> { associated_types, ) { Ok(impl_kind) => { - self.interner.select_impl_for_expression(function_ident_id, impl_kind); + if select_impl { + self.interner.select_impl_for_expression(function_ident_id, impl_kind); + } } Err(error) => self.push_trait_constraint_error(object_type, error, span), } @@ -1900,10 +1904,15 @@ impl<'context> Elaborator<'context> { /// Push a trait constraint into the current FunctionContext to be solved if needed /// at the end of the earlier of either the current function or the current comptime scope. - pub fn push_trait_constraint(&mut self, constraint: TraitConstraint, expr_id: ExprId) { + pub fn push_trait_constraint( + &mut self, + constraint: TraitConstraint, + expr_id: ExprId, + select_impl: bool, + ) { let context = self.function_context.last_mut(); let context = context.expect("The function_context stack should always be non-empty"); - context.trait_constraints.push((constraint, expr_id)); + context.trait_constraints.push((constraint, expr_id, select_impl)); } pub fn bind_generics_from_trait_constraint(