Skip to content

fix: Fix nested trait dispatch with associated types#8440

Merged
jfecher merged 13 commits intomasterfrom
jf/implicit-generics-unification
May 12, 2025
Merged

fix: Fix nested trait dispatch with associated types#8440
jfecher merged 13 commits intomasterfrom
jf/implicit-generics-unification

Conversation

@jfecher
Copy link
Contributor

@jfecher jfecher commented May 9, 2025

Description

Problem*

Resolves #8252

Summary*

This ended up being a case of doing something almost but not quite right. We were throwing away instantiation bindings during impl search which ends up being fine most of the time when you already have an impl (by using method syntax which looks inside the struct type to find the exact impl method beforehand). If you don't however, you'll only have the instantiation bindings from instantiating your current trait - not the trait impl! So when doing Eq::eq(a, b) we'd instantiate the object types since Eq requires us instantiate Self, but we don't get the additional type variables on the specific Eq impl which requires the implicit generics from associated types from other traits (in the case of the bugged program at least).

I adjusted impl search slightly to return the instantiation bindings of the selected impl as well, and store them if select_impl is true. If we store them without select_impl being true we can accidentally override other type bindings - I made a note of this in the code.

Additional Context

Dobby is a free elf now!

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.

@jfecher jfecher requested a review from a team May 9, 2025 20:35
@jfecher
Copy link
Contributor Author

jfecher commented May 9, 2025

@asterite do you know where the blacklist is for the nargo expand test?
Looks like it produces invalid syntax for the regression example preventing the expanded regression from compiling.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Test Suite Duration'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite Current: cd5dfbc Previous: f8c6943 Ratio
test_report_noir-lang_noir_bigcurve_ 275 s 229 s 1.20

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

@asterite
Copy link
Collaborator

asterite commented May 9, 2025

It's in build.rs inside nargo_cli. There are many constants for all the tests we ignore.

Copy link
Collaborator

@asterite asterite left a comment

Choose a reason for hiding this comment

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

Looks good!

What do you think if we also add the tests from #8279 ?

@TomAFrench
Copy link
Member

I've ignored the nargo expand test as it's unrelated to the issue which this PR is fixing. I've opened an issue here: #8449

@TomAFrench TomAFrench enabled auto-merge May 12, 2025 13:23
@TomAFrench
Copy link
Member

I've tested that this fixes the failures in noir-lang/noir_bigcurve#46. Miranda is going to be blocked by this very soon so merging so this ends up in the next nightly.

@TomAFrench
Copy link
Member

:huh:

@jfecher jfecher disabled auto-merge May 12, 2025 13:30
@jfecher
Copy link
Contributor Author

jfecher commented May 12, 2025

Disabled auto-merge, I'm adding the extra failing test from #8279. The others should be covered by the existing test.

@jfecher jfecher enabled auto-merge May 12, 2025 13:33
@jfecher jfecher added this pull request to the merge queue May 12, 2025
Merged via the queue into master with commit 2d727b1 May 12, 2025
115 of 116 checks passed
@jfecher jfecher deleted the jf/implicit-generics-unification branch May 12, 2025 14:07
github-merge-queue bot pushed a commit to AztecProtocol/aztec-packages that referenced this pull request May 13, 2025
Automated pull of nightly from the
[noir](https://github.com/noir-lang/noir) programming language, a
dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
fix: sign extend in signed cast
(noir-lang/noir#8264)
chore(fuzz): Do not use zero length types in the main input output
(noir-lang/noir#8465)
chore: fix visibility issues in test suite
(noir-lang/noir#8454)
chore: blackbox functions for ssa intepreter
(noir-lang/noir#8375)
feat: improve bitshift codegen
(noir-lang/noir#8442)
fix(ssa): Mark mutually recursive simple functions
(noir-lang/noir#8447)
fix: Fix nested trait dispatch with associated types
(noir-lang/noir#8440)
chore: carry visibilities in monomorphized AST
(noir-lang/noir#8439)
chore(tests): Add regression for now passing test
(noir-lang/noir#8441)
chore: use human-readable bytecode in snapshots
(noir-lang/noir#8164)
chore: bump external pinned commits
(noir-lang/noir#8445)
END_COMMIT_OVERRIDE

---------

Co-authored-by: AztecBot <tech@aztecprotocol.com>
Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>
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.

Using traits with associated constants in trait bounds in trait impls results in No matching impl found

3 participants