Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: don't always select trait impl when verifying trait constraints #7041

Merged
merged 11 commits into from
Jan 14, 2025
5 changes: 4 additions & 1 deletion compiler/noirc_frontend/src/elaborator/expressions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
9 changes: 7 additions & 2 deletions compiler/noirc_frontend/src/elaborator/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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> {
Expand Down Expand Up @@ -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(_)) {
Expand All @@ -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,
);
}
Expand Down
12 changes: 10 additions & 2 deletions compiler/noirc_frontend/src/elaborator/patterns.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
);
}
}
}
Expand All @@ -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
);
}
}

Expand Down
2 changes: 1 addition & 1 deletion compiler/noirc_frontend/src/elaborator/traits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ 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
// Attach any trait constraints on the trait to the function,
where_clause.extend(unresolved_trait.trait_def.where_clause.clone());

this.resolve_trait_function(
Expand Down
15 changes: 12 additions & 3 deletions compiler/noirc_frontend/src/elaborator/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1810,13 +1810,15 @@ impl<'context> Elaborator<'context> {
(expr_span, empty_function)
}

#[allow(clippy::too_many_arguments)]
pub fn verify_trait_constraint(
&mut self,
object_type: &Type,
trait_id: TraitId,
trait_generics: &[Type],
associated_types: &[NamedType],
function_ident_id: ExprId,
select_impl: bool,
span: Span,
) {
match self.interner.lookup_trait_implementation(
Expand All @@ -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),
}
Expand Down Expand Up @@ -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(
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[package]
name = "regression_7038"
type = "bin"
authors = [""]
compiler_version = ">=0.30.0"

[dependencies]
40 changes: 40 additions & 0 deletions test_programs/compile_success_empty/regression_7038/src/main.nr
TomAFrench marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
trait BigNumTrait {}

pub struct MyBigNum;

impl crate::BigNumTrait for MyBigNum {}

trait CurveParamsTrait<BigNum>
where
BigNum: BigNumTrait,
{
fn one();
}

pub struct BN254Params;
impl CurveParamsTrait<MyBigNum> for BN254Params {

fn one() {}
}

trait BigCurveTrait {
fn two();
}

pub struct BigCurve<BigNum, CurveParams> {}

type BN254 = BigCurve<MyBigNum, BN254Params>;

impl<BigNum, CurveParams> BigCurveTrait for BigCurve<BigNum, CurveParams>
where
BigNum: BigNumTrait,
CurveParams: CurveParamsTrait<BigNum>,
{
fn two() {
let _ = CurveParams::one();
}
}

fn main() {
let _ = BN254::two();
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[package]
name = "regression_7038_2"
type = "bin"
authors = [""]
compiler_version = ">=0.30.0"

[dependencies]
39 changes: 39 additions & 0 deletions test_programs/compile_success_empty/regression_7038_2/src/main.nr
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
trait BigNumTrait {}

pub struct MyBigNum;

impl crate::BigNumTrait for MyBigNum {}

trait CurveParamsTrait<BigNum>
where
BigNum: BigNumTrait,
{
// The difference between this and regression_7083 is that here
// this is a default method.
fn one() {}
}

pub struct BN254Params;
impl CurveParamsTrait<MyBigNum> for BN254Params {}

trait BigCurveTrait {
fn two();
}

pub struct BigCurve<BigNum, CurveParams> {}

type BN254 = BigCurve<MyBigNum, BN254Params>;

impl<BigNum, CurveParams> BigCurveTrait for BigCurve<BigNum, CurveParams>
where
BigNum: BigNumTrait,
CurveParams: CurveParamsTrait<BigNum>,
{
fn two() {
let _ = CurveParams::one();
}
}

fn main() {
let _ = BN254::two();
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[package]
name = "regression_7038_3"
type = "bin"
authors = [""]
compiler_version = ">=0.30.0"

[dependencies]
40 changes: 40 additions & 0 deletions test_programs/compile_success_empty/regression_7038_3/src/main.nr
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
trait BigNumTrait {}

pub struct MyBigNum;

impl crate::BigNumTrait for MyBigNum {}

trait CurveParamsTrait<BigNum> {
// 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<MyBigNum> for BN254Params {
fn one() {}
}

trait BigCurveTrait {
fn two();
}

pub struct BigCurve<BigNum, CurveParams> {}

type BN254 = BigCurve<MyBigNum, BN254Params>;

impl<BigNum, CurveParams> BigCurveTrait for BigCurve<BigNum, CurveParams>
where
BigNum: BigNumTrait,
CurveParams: CurveParamsTrait<BigNum>,
{
fn two() {
let _ = CurveParams::one();
}
}

fn main() {
let _ = BN254::two();
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[package]
name = "regression_7038_4"
type = "bin"
authors = [""]
compiler_version = ">=0.30.0"

[dependencies]
29 changes: 29 additions & 0 deletions test_programs/compile_success_empty/regression_7038_4/src/main.nr
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
// This program is a reduction of regression_7038_3 that led to a monomorphizer crash
trait BigNumTrait {}

trait CurveParamsTrait<BigNum> {
fn one()
where
BigNum: BigNumTrait;
}

pub struct MyBigNum;

impl BigNumTrait for MyBigNum {}

pub struct Params;
impl CurveParamsTrait<MyBigNum> for Params {

fn one() {}
}

fn foo<C>()
where
C: CurveParamsTrait<MyBigNum>,
{
let _ = C::one();
}

fn main() {
foo::<Params>();
}
Loading