Skip to content

fix: implicitly added named generics are equal between each other#8184

Merged
asterite merged 6 commits intomasterfrom
ab/trait-where-clause-associated-number-fix
Apr 25, 2025
Merged

fix: implicitly added named generics are equal between each other#8184
asterite merged 6 commits intomasterfrom
ab/trait-where-clause-associated-number-fix

Conversation

@asterite
Copy link
Collaborator

Description

Problem

Resolves #8177

Summary

In a code like this:

trait Bar {
    let N: Field;
}

impl Bar for Field {
    let N: Field = 42;
}

trait Foo {
    fn foo<B>(b: B) where B: Bar; 
}

impl Foo for Field{
    fn foo<B>(_: B) where B: Bar {} 
}

in the past, the compiler would fail to compile the above just because B: Bar would error saying that it's missing N from Bar's generics (that is, Bar<N = ...>). So in #7026 this was now allowed, and N was added implicitly as an unbound named generic.

When comparing where clauses, these are compared by equality (after binding substitution). The issue was that the implicitly added named generic in the first foo ended up with a type variable that's different than the one used for the second foo. Then, they aren't equal.

What's done in this PR, and I don't know if it's the correct way to solve this, was to mark these implicitly added named generics as such. Then, two unbound implicitly added named generics are considered equal, regardless of their type variable ID. For this I extracted a new NamedGeneric type instead of adding another argument to NamedGeneric(..., /* implicit /* bool) for clarity.

Additional Context

Documentation

Check one:

  • No documentation needed.
  • Documentation included in this PR.
  • [For Experimental Features] Documentation to be submitted in a separate PR.

PR Checklist

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

@asterite asterite requested a review from a team April 23, 2025 15:10
@asterite asterite added this pull request to the merge queue Apr 25, 2025
Merged via the queue into master with commit ef57db0 Apr 25, 2025
114 checks passed
@asterite asterite deleted the ab/trait-where-clause-associated-number-fix branch April 25, 2025 12:13
github-merge-queue bot pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Apr 28, 2025
Automated pull of nightly from the
[noir](https://github.com/noir-lang/noir) programming language, a
dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
feat: let `nargo test` use the new greybox fuzzer
(noir-lang/noir#8234)
chore(docs): add fixed bugs to list
(noir-lang/noir#8201)
fix(parser): do not use `Ident::default()`
(noir-lang/noir#8224)
feat(fuzz): Add experimental comptime code generator
(noir-lang/noir#8207)
fix: let SSA parser parse calls to print
(noir-lang/noir#8232)
feat: added 1, -1 case for inversions in brillig
(noir-lang/noir#8225)
chore: clippy (noir-lang/noir#8220)
fix: don't take implicitly added named generics when checking where
clauses (noir-lang/noir#8184)
chore(docs): Defunctionalization and some minor cleanup
(noir-lang/noir#8217)
feat: add `--no-fuzz` and `--only-fuzz` to `nargo test`
(noir-lang/noir#8213)
chore: add regression test for panic encountered in bigcurve library.
(noir-lang/noir#8211)
fix: validate numeric types when passing them to a FunctionBuilder
(noir-lang/noir#8215)
chore: refactor constrainedness checks on trait impls
(noir-lang/noir#8170)
chore(test): Add smoke test for AST generation
(noir-lang/noir#8048)
chore: bump more dependencies
(noir-lang/noir#8208)
chore: Don't use `i1` in the AST fuzzer
(noir-lang/noir#8204)
fix: Do not panic if RHS constant in division has more bits than the
operand (noir-lang/noir#8197)
chore: bump cargo deps (noir-lang/noir#8205)
fix: Handle truncating constants to 128 bits
(noir-lang/noir#8180)
fix!: disallow the `i1` type
(noir-lang/noir#8172)
chore(fuzz): Make sure `main` makes at least one call
(noir-lang/noir#8202)
fix: Use `get_local_or_global_instruction` in
`try_optimize_array_set_from_previous_get`
(noir-lang/noir#8200)
fix: returns 0 for right shift overflow
(noir-lang/noir#8189)
feat: avoid overflow check when subtracting from a value that is the
maximum for its bitsize (noir-lang/noir#8190)
feat: optimize `checked_to_unchecked` to take into account
multiplications (noir-lang/noir#8188)
chore: document ssa `inline_simple_functions`
(noir-lang/noir#8169)
chore(docs): remove_unreachable SSA pass
(noir-lang/noir#8196)
chore: Change AST fuzzer recursion limit
(noir-lang/noir#8173)
chore(acir): Unify how we display witness indices
(noir-lang/noir#8192)
fix: Detect ABI change of fuzzing harnesses
(noir-lang/noir#8193)
chore(acir): Test that `BLACKBOX::RANGE` display the number of bits
(noir-lang/noir#8191)
fix(debugger): send idle at loop end + fix test
(noir-lang/noir#8187)
fix: Use `get_local_or_global_instruction` when simplifying `IfElse`
(noir-lang/noir#8185)
fix(parser): avoid using `Location::dummy()`
(noir-lang/noir#8178)
chore: Push a bug list (noir-lang/noir#8186)
feat(greybox_fuzzer): Should_fail and should_fail_with
(noir-lang/noir#8118)
chore: move unsigned overflow check from acir/brillig to ssa
(noir-lang/noir#8163)
feat: location tree for debug_info
(noir-lang/noir#7034)
fix: Use `IntegerConstant` for loop boundaries in `unrolling`
(noir-lang/noir#8094)
fix(ssa): Recursive shared Brillig entry points
(noir-lang/noir#8099)
chore: enable '--pedantic-solving' on more tests
(noir-lang/noir#7701)
chore(readme): Update `acvm-repo` READMEs
(noir-lang/noir#8150)
chore(test): add `test_programs` dir for expected-panic tests
(noir-lang/noir#8147)
chore(docs): minor updates in solidity doc
(noir-lang/noir#8160)
feat(debugger): debug test functions
(noir-lang/noir#7958)
chore: migrate recursive proof test to ultrahonk
(noir-lang/noir#8038)
chore: bump glob package (noir-lang/noir#8159)
chore: bump external pinned commits
(noir-lang/noir#8139)
chore(docs): patch web app tutorial in 1.0.0-beta3 versioned docs
(noir-lang/noir#8158)
END_COMMIT_OVERRIDE

---------

Co-authored-by: AztecBot <tech@aztecprotocol.com>
Co-authored-by: Ary Borenszweig <asterite@gmail.com>
Co-authored-by: TomAFrench <tom@tomfren.ch>
github-merge-queue bot pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Apr 28, 2025
Automated pull of nightly from the
[noir](https://github.com/noir-lang/noir) programming language, a
dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
feat: let `nargo test` use the new greybox fuzzer
(noir-lang/noir#8234)
chore(docs): add fixed bugs to list
(noir-lang/noir#8201)
fix(parser): do not use `Ident::default()`
(noir-lang/noir#8224)
feat(fuzz): Add experimental comptime code generator
(noir-lang/noir#8207)
fix: let SSA parser parse calls to print
(noir-lang/noir#8232)
feat: added 1, -1 case for inversions in brillig
(noir-lang/noir#8225)
chore: clippy (noir-lang/noir#8220)
fix: don't take implicitly added named generics when checking where
clauses (noir-lang/noir#8184)
chore(docs): Defunctionalization and some minor cleanup
(noir-lang/noir#8217)
feat: add `--no-fuzz` and `--only-fuzz` to `nargo test`
(noir-lang/noir#8213)
chore: add regression test for panic encountered in bigcurve library.
(noir-lang/noir#8211)
fix: validate numeric types when passing them to a FunctionBuilder
(noir-lang/noir#8215)
chore: refactor constrainedness checks on trait impls
(noir-lang/noir#8170)
chore(test): Add smoke test for AST generation
(noir-lang/noir#8048)
chore: bump more dependencies
(noir-lang/noir#8208)
chore: Don't use `i1` in the AST fuzzer
(noir-lang/noir#8204)
fix: Do not panic if RHS constant in division has more bits than the
operand (noir-lang/noir#8197)
chore: bump cargo deps (noir-lang/noir#8205)
fix: Handle truncating constants to 128 bits
(noir-lang/noir#8180)
fix!: disallow the `i1` type
(noir-lang/noir#8172)
chore(fuzz): Make sure `main` makes at least one call
(noir-lang/noir#8202)
fix: Use `get_local_or_global_instruction` in
`try_optimize_array_set_from_previous_get`
(noir-lang/noir#8200)
fix: returns 0 for right shift overflow
(noir-lang/noir#8189)
feat: avoid overflow check when subtracting from a value that is the
maximum for its bitsize (noir-lang/noir#8190)
feat: optimize `checked_to_unchecked` to take into account
multiplications (noir-lang/noir#8188)
chore: document ssa `inline_simple_functions`
(noir-lang/noir#8169)
chore(docs): remove_unreachable SSA pass
(noir-lang/noir#8196)
chore: Change AST fuzzer recursion limit
(noir-lang/noir#8173)
chore(acir): Unify how we display witness indices
(noir-lang/noir#8192)
fix: Detect ABI change of fuzzing harnesses
(noir-lang/noir#8193)
chore(acir): Test that `BLACKBOX::RANGE` display the number of bits
(noir-lang/noir#8191)
fix(debugger): send idle at loop end + fix test
(noir-lang/noir#8187)
fix: Use `get_local_or_global_instruction` when simplifying `IfElse`
(noir-lang/noir#8185)
fix(parser): avoid using `Location::dummy()`
(noir-lang/noir#8178)
chore: Push a bug list (noir-lang/noir#8186)
feat(greybox_fuzzer): Should_fail and should_fail_with
(noir-lang/noir#8118)
chore: move unsigned overflow check from acir/brillig to ssa
(noir-lang/noir#8163)
feat: location tree for debug_info
(noir-lang/noir#7034)
fix: Use `IntegerConstant` for loop boundaries in `unrolling`
(noir-lang/noir#8094)
fix(ssa): Recursive shared Brillig entry points
(noir-lang/noir#8099)
chore: enable '--pedantic-solving' on more tests
(noir-lang/noir#7701)
chore(readme): Update `acvm-repo` READMEs
(noir-lang/noir#8150)
chore(test): add `test_programs` dir for expected-panic tests
(noir-lang/noir#8147)
chore(docs): minor updates in solidity doc
(noir-lang/noir#8160)
feat(debugger): debug test functions
(noir-lang/noir#7958)
chore: migrate recursive proof test to ultrahonk
(noir-lang/noir#8038)
chore: bump glob package (noir-lang/noir#8159)
chore: bump external pinned commits
(noir-lang/noir#8139)
chore(docs): patch web app tutorial in 1.0.0-beta3 versioned docs
(noir-lang/noir#8158)
END_COMMIT_OVERRIDE

---------

Co-authored-by: AztecBot <tech@aztecprotocol.com>
Co-authored-by: Ary Borenszweig <asterite@gmail.com>
Co-authored-by: TomAFrench <tom@tomfren.ch>
github-merge-queue bot pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Apr 28, 2025
Automated pull of nightly from the
[noir](https://github.com/noir-lang/noir) programming language, a
dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
feat: let `nargo test` use the new greybox fuzzer
(noir-lang/noir#8234)
chore(docs): add fixed bugs to list
(noir-lang/noir#8201)
fix(parser): do not use `Ident::default()`
(noir-lang/noir#8224)
feat(fuzz): Add experimental comptime code generator
(noir-lang/noir#8207)
fix: let SSA parser parse calls to print
(noir-lang/noir#8232)
feat: added 1, -1 case for inversions in brillig
(noir-lang/noir#8225)
chore: clippy (noir-lang/noir#8220)
fix: don't take implicitly added named generics when checking where
clauses (noir-lang/noir#8184)
chore(docs): Defunctionalization and some minor cleanup
(noir-lang/noir#8217)
feat: add `--no-fuzz` and `--only-fuzz` to `nargo test`
(noir-lang/noir#8213)
chore: add regression test for panic encountered in bigcurve library.
(noir-lang/noir#8211)
fix: validate numeric types when passing them to a FunctionBuilder
(noir-lang/noir#8215)
chore: refactor constrainedness checks on trait impls
(noir-lang/noir#8170)
chore(test): Add smoke test for AST generation
(noir-lang/noir#8048)
chore: bump more dependencies
(noir-lang/noir#8208)
chore: Don't use `i1` in the AST fuzzer
(noir-lang/noir#8204)
fix: Do not panic if RHS constant in division has more bits than the
operand (noir-lang/noir#8197)
chore: bump cargo deps (noir-lang/noir#8205)
fix: Handle truncating constants to 128 bits
(noir-lang/noir#8180)
fix!: disallow the `i1` type
(noir-lang/noir#8172)
chore(fuzz): Make sure `main` makes at least one call
(noir-lang/noir#8202)
fix: Use `get_local_or_global_instruction` in
`try_optimize_array_set_from_previous_get`
(noir-lang/noir#8200)
fix: returns 0 for right shift overflow
(noir-lang/noir#8189)
feat: avoid overflow check when subtracting from a value that is the
maximum for its bitsize (noir-lang/noir#8190)
feat: optimize `checked_to_unchecked` to take into account
multiplications (noir-lang/noir#8188)
chore: document ssa `inline_simple_functions`
(noir-lang/noir#8169)
chore(docs): remove_unreachable SSA pass
(noir-lang/noir#8196)
chore: Change AST fuzzer recursion limit
(noir-lang/noir#8173)
chore(acir): Unify how we display witness indices
(noir-lang/noir#8192)
fix: Detect ABI change of fuzzing harnesses
(noir-lang/noir#8193)
chore(acir): Test that `BLACKBOX::RANGE` display the number of bits
(noir-lang/noir#8191)
fix(debugger): send idle at loop end + fix test
(noir-lang/noir#8187)
fix: Use `get_local_or_global_instruction` when simplifying `IfElse`
(noir-lang/noir#8185)
fix(parser): avoid using `Location::dummy()`
(noir-lang/noir#8178)
chore: Push a bug list (noir-lang/noir#8186)
feat(greybox_fuzzer): Should_fail and should_fail_with
(noir-lang/noir#8118)
chore: move unsigned overflow check from acir/brillig to ssa
(noir-lang/noir#8163)
feat: location tree for debug_info
(noir-lang/noir#7034)
fix: Use `IntegerConstant` for loop boundaries in `unrolling`
(noir-lang/noir#8094)
fix(ssa): Recursive shared Brillig entry points
(noir-lang/noir#8099)
chore: enable '--pedantic-solving' on more tests
(noir-lang/noir#7701)
chore(readme): Update `acvm-repo` READMEs
(noir-lang/noir#8150)
chore(test): add `test_programs` dir for expected-panic tests
(noir-lang/noir#8147)
chore(docs): minor updates in solidity doc
(noir-lang/noir#8160)
feat(debugger): debug test functions
(noir-lang/noir#7958)
chore: migrate recursive proof test to ultrahonk
(noir-lang/noir#8038)
chore: bump glob package (noir-lang/noir#8159)
chore: bump external pinned commits
(noir-lang/noir#8139)
chore(docs): patch web app tutorial in 1.0.0-beta3 versioned docs
(noir-lang/noir#8158)
END_COMMIT_OVERRIDE

---------

Co-authored-by: AztecBot <tech@aztecprotocol.com>
Co-authored-by: Ary Borenszweig <asterite@gmail.com>
Co-authored-by: TomAFrench <tom@tomfren.ch>
@asterite asterite changed the title fix: don't take implicitly added named generics when checking where clauses fix: implicitly added named generics are equal between each other Apr 30, 2025
Comment on lines +3194 to +3198
// An implicitly added unbound named generic's hash must be the same as any other
// implicitly added unbound named generic's hash.
if !type_var.borrow().is_unbound() {
type_var.hash(state);
}
Copy link
Contributor

@jfecher jfecher Apr 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Must it be the same though?
We issue implicit NamedGenerics for associated types so this seems like it'd consider the generics from two separate associated types as equal.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point.

Originally I fixed this issue in a different way: when comparing a method where clause in a impl against its trait, I removed all implicit generics from the comparison (there's an equality comparison somewhere there). But then I thought I could also fix it by changing the equality... maybe I should have done the former?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Inclusion of traits with associated constants in trait constraints on trait methods results in impl has stricter requirements than trait error

3 participants