fix: check associated types after validating where clause when looking up trait impls, plus some unification fixes by asterite · Pull Request #9265 · noir-lang/noir

asterite · 2025-07-21T17:34:30Z

Description

Problem

Resolves #9245
Resolves #9248

Summary

There are several commits here but the main fixes are:

When looking up matching trait implementations, we now validate the where clause before checking associated types. This guarantees that type bindings needed to reduce, for example, <T as Deserialize>::N> when we have where T: Deserialize, into <Field as Deserialize>::N and eventually, say, "1". Looking at bench-show this change also seems to have improved performance, but I think it's because we now check the object type before everything else too, and maybe that is more frequent to bail out earlier (just a guess).
I noticed that when we unify types we have the current bindings (TypeBindings) but we don't look into these when canonicalizing types. As an example, say we need to unify '5 + '6 - 1 with a named generic X. Just like that it can't be unified. If we knew that '6 resolves to a constant value, say "1", then we'd end up with '5 + 1 - 1, which is simplified into '5, and unifying that with X we'd get '5 <- X. Well, it turns out that even though '6 was sometimes unbound, it was bound in the current bindings, but canonicalize didn't have access to these. Now it does so more cases work.
Another thing I noticed is that we didn't simplify X * 1/X / 1 into X, nor X + 0/X - 0 into X. This was needed for the code in Noir fails to resolve implementation of a trait for an array with generics #9248 which has <T as Deserialize>::N * M where T is Field so we end up with 1 * M, canonicalized to M * 1. Although it might seem that the fix is specifically for that code, in reality it's a generic rule. For example if <T as Deserialize>::N was "2" then at some point in the call site or in some associated constant one would have to write * 2 or at least produce the value "2", and that would naturally unify with the other part that has "* 2" (we have a rule for that).

Now I'm pretty confident (say, 95% 😊) that the changes here a good.

Additional Context

There's a proptest failing now because of the simplification I mention in point 3 but I don't understand how to fix it or what needs to be changed.

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.

…rait implementation

github-actions

⚠️ Performance Alert ⚠️

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

Benchmark suite	Current: `b68ac5a`	Previous: `39a8379`	Ratio
`rollup-root`	`0.005` s	`0.004` s	`1.25`

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

CC: @TomAFrench

jfecher

I think unifying the object type before checking generics is cleaner (and can't remember why it was reversed to begin with but I do think it was done on purpose - maybe just a bad quick fix). So I think that is good but also checking the where clause earlier seems arbitrary to me. You mentioned this is to bind it against a type variable before the same variable gets overwritten with another incorrect type so it sounds like to me the correct solution would be figuring out why this other type variable is being (permanently? temporarily?) bound to an incorrect type.

also checking the where clause earlier seems arbitrary to me

Edit: since we could in theory have the same issue but for the generics if the where clause refers to those instead of the object type

github-actions

ACVM Benchmarks

Details

Benchmark suite	Current: `8ab5b00`	Previous: `4fbd819`	Ratio
`purely_sequential_opcodes`	`252538` ns/iter (`± 451`)	`248544` ns/iter (`± 437`)	`1.02`
`perfectly_parallel_opcodes`	`222424` ns/iter (`± 1895`)	`218725` ns/iter (`± 3861`)	`1.02`
`perfectly_parallel_batch_inversion_opcodes`	`2782282` ns/iter (`± 2321`)	`2778615` ns/iter (`± 1079`)	`1.00`

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

github-actions

Compilation Time

Details

Benchmark suite	Current: `8ab5b00`	Previous: `4fbd819`	Ratio
`semaphore-depth-10`	`0.786` s	`0.762` s	`1.03`
`sha512-100-bytes`	`1.715` s	`1.68` s	`1.02`

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

github-actions

Artifact Size

Details

Benchmark suite	Current: `8ab5b00`	Previous: `4fbd819`	Ratio
`semaphore-depth-10`	`632.4` KB	`632.4` KB	`1`
`sha512-100-bytes`	`525.2` KB	`525.2` KB	`1`

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

github-actions

Opcode count

Details

Benchmark suite	Current: `8ab5b00`	Previous: `4fbd819`	Ratio
`semaphore-depth-10`	`5763` opcodes	`5763` opcodes	`1`
`sha512-100-bytes`	`13173` opcodes	`13173` opcodes	`1`

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

github-actions

Execution Time

Details

Benchmark suite	Current: `8ab5b00`	Previous: `4fbd819`	Ratio
`semaphore-depth-10`	`0.02` s	`0.019` s	`1.05`
`sha512-100-bytes`	`0.102` s	`0.102` s	`1`

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

github-actions

Test Suite Duration

Details

Benchmark suite	Current: `8ab5b00`	Previous: `4fbd819`	Ratio
`test_report_AztecProtocol_aztec-packages_noir-projects_aztec-nr`	`3` s	`5` s	`0.60`
`test_report_AztecProtocol_aztec-packages_noir-projects_noir-contracts`	`30` s	`42` s	`0.71`
`test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_blob`	`2` s	`3` s	`0.67`
`test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_private-kernel-lib`	`1` s	`1` s	`1`
`test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_reset-kernel-lib`	`1` s	`2` s	`0.50`
`test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_rollup-lib`	`2` s	`3` s	`0.67`
`test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_types`	`1` s	`1` s	`1`
`test_report_noir-lang_noir-bignum_`	`374` s	`386` s	`0.97`
`test_report_noir-lang_noir_bigcurve_`	`299` s	`382` s	`0.78`
`test_report_noir-lang_sha512_`	`15` s	`14` s	`1.07`
`test_report_zkpassport_noir-ecdsa_`	`2` s	`2` s	`1`
`test_report_zkpassport_noir_rsa_`	`0` s	`1` s	`0`

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

github-actions

Execution Memory

Details

Benchmark suite	Current: `8ab5b00`	Previous: `4fbd819`	Ratio
`semaphore_depth_10`	`69.31` MB	`69.31` MB	`1`
`sha512_100_bytes`	`54.83` MB	`54.83` MB	`1`

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

github-actions

Compilation Memory

Details

Benchmark suite	Current: `8ab5b00`	Previous: `4fbd819`	Ratio
`semaphore_depth_10`	`104.56` MB	`104.56` MB	`1`
`sha512_100_bytes`	`234.54` MB	`234.59` MB	`1.00`

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

asterite · 2025-07-21T20:45:35Z

@jfecher I asked for a review but I'm still not sure this is the right way to fix this. For example #9248 still fails with this change. Like I commented on the other issue, <T as Deserialize>::N doesn't get the constant value it should have, and maybe it gets that at some point in validate_where_clause but I don't know if calling that method is the way to bind T to Field (in the example code).

github-actions

⚠️ 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: `28f03fc`	Previous: `72b6ec4`	Ratio
`test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_blob`	`3` s	`2` s	`1.50`

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

CC: @TomAFrench

asterite · 2025-07-22T18:45:10Z

@jfecher This is ready for review again, and changes now are more likely to be correct (checking bindings in canonicalize did help 🎉 )

compiler/noirc_frontend/src/hir_def/types/arithmetic.rs

jfecher · 2025-07-23T17:12:42Z

Looks like the failing proptest is asserting on exact equality of the types which is different now. I think we can instead change it to assert that the types are equal after canonicalizing them (again)

jfecher

Great!
The canonicalization change in particular was a big hole in the type system before.

Automated pull of nightly from the [noir](https://github.com/noir-lang/noir) programming language, a dependency of Aztec. BEGIN_COMMIT_OVERRIDE fix: forbid self-referencing type aliases (noir-lang/noir#9103) chore: add a mem2reg test for when all references need to be invalidated (noir-lang/noir#9377) fix(ssa): Do not check ArrayGet/Set as unreachable for Brillig (noir-lang/noir#9376) chore: use SSA parser in all mem2reg tests (noir-lang/noir#9372) fix: trait where clause check fixes (noir-lang/noir#9369) fix: Correct doc comments for SSA passes (noir-lang/noir#9371) fix: prevent `SignedField::from(i128::MIN)` from crashing (noir-lang/noir#9366) fix: allow constants in the type-system to be negative (noir-lang/noir#9360) feat: show circuit output as a value of the program's return type (noir-lang/noir#9364) feat: add `FunctionDefinition::visibility` (noir-lang/noir#9363) chore(docs): Add example for `$crate` in docs (noir-lang/noir#9361) fix: Prevent accidental tuple sharing in comptime code (noir-lang/noir#9313) fix: perserve purities after SSA normalization (noir-lang/noir#9355) fix: modulo overflow in comptime (noir-lang/noir#9348) fix: handle short-syntax for trait constraints on trait generics (noir-lang/noir#9167) chore: enhance trait constraint comment (noir-lang/noir#9358) fix: replace implicitly added named generics with fresh type vars in check_trait_impl_where_clause_matches_trait_where_clause (noir-lang/noir#9352) fix: push definition trait constraints after trait item constraint (noir-lang/noir#9354) chore(ci): Update status of noir_json_parser (noir-lang/noir#9351) fix(ssa): Keep reference count increments for array set values (noir-lang/noir#9344) chore: remove unused `compile_workspace` (noir-lang/noir#9353) chore: try printing byte arrays as strings in the SSA interpreter (noir-lang/noir#9346) feat(lsp): allow opening noir stdlib files (noir-lang/noir#9339) fix: do u128 operations with u128, not i128 (noir-lang/noir#9345) chore(acir): ACIR parser error handling for blackbox inputs/outputs (noir-lang/noir#9342) fix: prevent invalid types in test/fuzz functions (noir-lang/noir#9343) chore(lsp): avoid redundant type checking (noir-lang/noir#9337) feat(acir): Parse ACIR memory and call opcodes (noir-lang/noir#9331) fix(ssa_gen): Add constraint on slice length before popping (noir-lang/noir#9323) chore: impl for u16 conversions (noir-lang/noir#9314) fix: substitute bindings in type before canonicalization (noir-lang/noir#9328) fix(ssa_interpreter): `push_back` and `pop_back` to slices with padding (noir-lang/noir#9320) fix: wildcard type should be allowed in lambda parameter types (noir-lang/noir#9325) chore: graceful handling of SIGPIPE (noir-lang/noir#9075) feat: return unsolvable opcode from `CircuitSimulator` (noir-lang/noir#8943) fix: allow nested fmtstr (noir-lang/noir#9309) feat: Initial ACIR parser (arithmetic exprs and black box functions) (noir-lang/noir#9316) fix(mem2reg): Register aliases when the `IfElse` result in a reference (noir-lang/noir#9305) fix: Make Ssa-gen use existing reference when compiling `&mut foo.bar.baz` (noir-lang/noir#9307) fix: top-level item in dependency isn't always visible (noir-lang/noir#9295) fix(ssa-interpreter): Return error if slice length is 0 during popping (noir-lang/noir#9308) chore: Release Noir(1.0.0-beta.9) (noir-lang/noir#9184) chore(LSP): simplify code lens request handling (noir-lang/noir#9279) chore: add regression tests for #6383 (noir-lang/noir#9302) fix: disallow `_` in signatures and struct members (noir-lang/noir#9301) fix: check associated types after validating where clause when looking up trait impls, plus some unification fixes (noir-lang/noir#9265) chore: Add fmtstr to coercions list (noir-lang/noir#9300) chore: Add a helper function `fmtstr::as_quoted_str` (noir-lang/noir#9293) chore(docs): Copy Type Coercions docs into v1.0.0-beta.8 versioned docs (noir-lang/noir#9298) feat: only inject "out of bounds" checks in brillig (noir-lang/noir#9200) END_COMMIT_OVERRIDE --------- Co-authored-by: AztecBot <tech@aztecprotocol.com> Co-authored-by: Maxim Vezenov <mvezenov@gmail.com>

asterite added 3 commits July 21, 2025 14:33

fix: solve object type and where clause before args when looking up t…

dec49c8

…rait implementation

clippy

e9f35c5

Try validate where clause before, but don't error yet

b68ac5a

github-actions bot reviewed Jul 21, 2025

View reviewed changes

asterite marked this pull request as ready for review July 21, 2025 18:51

asterite added the bench-show Display benchmark results on PR label Jul 21, 2025

jfecher requested changes Jul 21, 2025

View reviewed changes

Only check associated constants after validating where clause

22f54aa

asterite changed the title ~~fix: solve object type and where clause before args when looking up trait impl~~ fix: check associated types after validating where clause when looking up trait impls Jul 21, 2025

github-actions bot reviewed Jul 21, 2025

View reviewed changes

asterite requested a review from jfecher July 21, 2025 20:26

Merge branch 'master' into ab/trait-impl-issue-9245

15a39e8

github-actions bot reviewed Jul 21, 2025

View reviewed changes

asterite added 6 commits July 22, 2025 14:21

Add failing tests

cc07ae4

Let canonicalize and others take bindings too

8abd40b

Simplify mul/div by 1 and add/sub zero

bbd7a47

Merge branch 'ab/more-trait-issues' into ab/trait-impl-issue-9245

33e64b2

Tests

8856f2f

Add a regression test for #9248

f5a1a19

asterite changed the title ~~fix: check associated types after validating where clause when looking up trait impls~~ fix: check associated types after validating where clause when looking up trait impls, plus some unification fixes Jul 22, 2025

asterite commented Jul 22, 2025

View reviewed changes

compiler/noirc_frontend/src/hir_def/types/arithmetic.rs Show resolved Hide resolved

Fix test

28f03fc

jfecher approved these changes Jul 23, 2025

View reviewed changes

asterite added this pull request to the merge queue Jul 23, 2025

jfecher removed this pull request from the merge queue due to a manual request Jul 23, 2025

asterite added 2 commits July 23, 2025 15:35

Merge branch 'master' into ab/trait-impl-issue-9245

ed52a25

Move logic down

8ab5b00

jfecher enabled auto-merge July 23, 2025 18:43

jfecher added this pull request to the merge queue Jul 23, 2025

Merged via the queue into master with commit 6cb4347 Jul 23, 2025
101 checks passed

jfecher deleted the ab/trait-impl-issue-9245 branch July 23, 2025 19:20

noirwhal mentioned this pull request Jul 23, 2025

chore: Release Noir(1.0.0-beta.9) #9184

Merged

AztecBot mentioned this pull request Jul 24, 2025

chore: Bump Noir reference AztecProtocol/aztec-packages#15899

Merged

asterite mentioned this pull request Jul 25, 2025

fix: substitute bindings in type before canonicalization #9328

Merged

5 tasks

AztecBot mentioned this pull request Jul 31, 2025

chore: Bump Noir reference AztecProtocol/aztec-packages#16127

Merged

Conversation

asterite commented Jul 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Problem

Summary

Additional Context

Documentation

PR Checklist

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

⚠️ Performance Alert ⚠️

Uh oh!

jfecher left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

ACVM Benchmarks

Uh oh!

github-actions bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Compilation Time

Uh oh!

github-actions bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Artifact Size

Uh oh!

github-actions bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Opcode count

Uh oh!

github-actions bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Execution Time

Uh oh!

github-actions bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Test Suite Duration

Uh oh!

github-actions bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Execution Memory

Uh oh!

github-actions bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Compilation Memory

Uh oh!

asterite commented Jul 21, 2025

Uh oh!

github-actions bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

⚠️ Performance Alert ⚠️

Uh oh!

asterite commented Jul 22, 2025

Uh oh!

Uh oh!

jfecher commented Jul 23, 2025

Uh oh!

jfecher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

asterite commented Jul 21, 2025 •

edited

Loading

jfecher left a comment •

edited

Loading

github-actions bot left a comment •

edited

Loading

github-actions bot left a comment •

edited

Loading

github-actions bot left a comment •

edited

Loading

github-actions bot left a comment •

edited

Loading

github-actions bot left a comment •

edited

Loading

github-actions bot left a comment •

edited

Loading

github-actions bot left a comment •

edited

Loading

github-actions bot left a comment •

edited

Loading

github-actions bot left a comment •

edited

Loading