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

asterite · 2025-01-13T18:57:14Z

Description

Problem

Resolves #7038

Summary

Here's a comment that helped me track down what was going on.

I think what was happening is that for a same ExprId we were selecting trait impls multiple times. In particular, when verifying trait constraints of a function call by verifying the where clause on that call, we were selecting trait impls always on the same ExprId for types that weren't self in the call (they are generic type parameters).

In this PR I changed the code so that when verifying these where clauses we don't associate trait impls to them, but we still check that we can find a trait impl.

I'm not sure if this is the correct fix, though, so let me know!

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.

…o body

TomAFrench · 2025-01-13T19:00:22Z

Can you add a regression test?

github-actions · 2025-01-13T19:04:27Z

Compilation Memory Report

Program	Peak Memory
keccak256	77.72M
workspace	123.82M
regression_4709	424.09M
ram_blowup_regression	1.58G
rollup-base-public	2.51G
rollup-base-private	901.10M
private-kernel-tail	207.04M
private-kernel-reset	669.12M
private-kernel-inner	294.26M

github-actions · 2025-01-13T19:04:29Z

Execution Memory Report

Program	Peak Memory
keccak256	74.67M
workspace	123.40M
regression_4709	316.00M
ram_blowup_regression	512.61M
rollup-base-public	478.87M
rollup-base-private	325.48M
private-kernel-tail	180.33M
private-kernel-reset	245.02M
private-kernel-inner	208.41M

github-actions · 2025-01-13T19:04:41Z

Compilation Report

Program	Compilation Time	%
sha256_regression	1.090s	9%
regression_4709	0.833s	0%
ram_blowup_regression	16.500s	1%
rollup-root	3.616s	9%
rollup-merge	1.934s	7%
rollup-block-merge	3.402s	0%
rollup-base-public	29.900s	-6%
rollup-base-private	10.580s	0%
private-kernel-tail	0.963s	-3%
private-kernel-reset	6.054s	-4%
private-kernel-inner	1.958s	-1%

github-actions · 2025-01-13T19:04:46Z

Execution Report

Program	Execution Time	%
sha256_regression	0.054s	3%
regression_4709	0.001s	0%
ram_blowup_regression	0.604s	0%
rollup-root	0.105s	0%
rollup-merge	0.007s	16%
rollup-block-merge	0.104s	0%
rollup-base-public	1.222s	0%
rollup-base-private	0.452s	0%
private-kernel-tail	0.019s	0%
private-kernel-reset	0.311s	-1%
private-kernel-inner	0.068s	0%

test_programs/compile_success_empty/regression_7038/src/main.nr

This reverts commit 133bf59.

jfecher

This sounds like the right solution

…oir-lang/noir#7066) feat!: Handle generic fields in `StructDefinition::fields` and move old functionality to `StructDefinition::fields_as_written` (noir-lang/noir#7067) chore(ci): Check various inliner aggressiveness setttings in Brillig reports (noir-lang/noir#7049) chore: reenable reports on rollup root circuits (noir-lang/noir#7061) fix: don't always select trait impl when verifying trait constraints (noir-lang/noir#7041) chore: mark some critical libraries as good again (noir-lang/noir#7065) fix: Reduce memory usage in mem2reg (noir-lang/noir#7053) feat: Allow associated types to be ellided from trait constraints (noir-lang/noir#7026) chore(perf): try using vec-collections's VecSet in AliasSet (noir-lang/noir#7058) chore: reduce number of iterations of `acvm::compiler::compile` (noir-lang/noir#7050) chore: add `noir_check_shuffle` as a critical library (noir-lang/noir#7056) chore: clippy warning fix (noir-lang/noir#7051) chore(ci): Unify compilation/execution report jobs that take averages with single runs (noir-lang/noir#7048) fix(nargo_fmt): let doc comment could come after regular comment (noir-lang/noir#7046) fix(nargo_fmt): don't consider identifiers the same if they are equal… (noir-lang/noir#7043) feat: auto-import traits when suggesting trait methods (noir-lang/noir#7037) feat: avoid inserting `inc_rc` instructions into ACIR (noir-lang/noir#7036) fix(lsp): suggest all possible trait methods, but only visible ones (noir-lang/noir#7027) chore: add more protocol circuits to reports (noir-lang/noir#6977) feat: avoid generating a new witness when checking if linear expression is zero (noir-lang/noir#7031) feat: skip codegen of zero iteration loops (noir-lang/noir#7030)

…ir#7066) feat!: Handle generic fields in `StructDefinition::fields` and move old functionality to `StructDefinition::fields_as_written` (noir-lang/noir#7067) chore(ci): Check various inliner aggressiveness setttings in Brillig reports (noir-lang/noir#7049) chore: reenable reports on rollup root circuits (noir-lang/noir#7061) fix: don't always select trait impl when verifying trait constraints (noir-lang/noir#7041) chore: mark some critical libraries as good again (noir-lang/noir#7065) fix: Reduce memory usage in mem2reg (noir-lang/noir#7053) feat: Allow associated types to be ellided from trait constraints (noir-lang/noir#7026) chore(perf): try using vec-collections's VecSet in AliasSet (noir-lang/noir#7058) chore: reduce number of iterations of `acvm::compiler::compile` (noir-lang/noir#7050) chore: add `noir_check_shuffle` as a critical library (noir-lang/noir#7056) chore: clippy warning fix (noir-lang/noir#7051) chore(ci): Unify compilation/execution report jobs that take averages with single runs (noir-lang/noir#7048) fix(nargo_fmt): let doc comment could come after regular comment (noir-lang/noir#7046) fix(nargo_fmt): don't consider identifiers the same if they are equal… (noir-lang/noir#7043) feat: auto-import traits when suggesting trait methods (noir-lang/noir#7037) feat: avoid inserting `inc_rc` instructions into ACIR (noir-lang/noir#7036) fix(lsp): suggest all possible trait methods, but only visible ones (noir-lang/noir#7027) chore: add more protocol circuits to reports (noir-lang/noir#6977) feat: avoid generating a new witness when checking if linear expression is zero (noir-lang/noir#7031) feat: skip codegen of zero iteration loops (noir-lang/noir#7030)

Don't extend trait function where clause with trait where clause if n…

52ba6ea

…o body

asterite added 4 commits January 13, 2025 16:12

Add regression test

00e7799

Add another regression test to prove this is not the right fix

78702ca

Undo the fix so we can start from scratch

41f9ff2

Use correct directory name

4cb2c38

asterite changed the title ~~Don't extend trait function where clause with trait where clause if n…~~ Add a couple of regression tests for #7038 Jan 13, 2025

TomAFrench reviewed Jan 13, 2025

View reviewed changes

test_programs/compile_success_empty/regression_7038/src/main.nr Show resolved Hide resolved

asterite added 3 commits January 14, 2025 08:44

Just trying something

133bf59

Revert "Just trying something"

a52a95d

This reverts commit 133bf59.

Add another regression test

29f163a

asterite changed the title ~~Add a couple of regression tests for #7038~~ Add a few regression tests for #7038 Jan 14, 2025

asterite added 3 commits January 14, 2025 11:18

Merge branch 'master' into ab/trait-where-clause-bug

da7a79f

Add another regression test

01f4636

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

f9f5088

asterite changed the title ~~Add a few regression tests for #7038~~ fix: don't always select trait impl when verifying trait constraints Jan 14, 2025

asterite changed the title ~~fix: don't always select trait impl when verifying trait constraints~~ fix: don't always select trait impl when verifying trait constraints Jan 14, 2025

asterite requested a review from a team January 14, 2025 17:44

jfecher approved these changes Jan 14, 2025

View reviewed changes

jfecher added this pull request to the merge queue Jan 14, 2025

Merged via the queue into master with commit ba07336 Jan 14, 2025
94 of 97 checks passed

jfecher deleted the ab/trait-where-clause-bug branch January 14, 2025 19:27

noirwhal mentioned this pull request Jan 14, 2025

chore: Release Noir(1.0.0-beta.2) #6914

Open

AztecBot mentioned this pull request Jan 14, 2025

feat: Sync from noir AztecProtocol/aztec-packages#11196

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

asterite commented Jan 13, 2025 •

edited

Loading

TomAFrench commented Jan 13, 2025

github-actions bot commented Jan 13, 2025 •

edited

Loading

github-actions bot commented Jan 13, 2025 •

edited

Loading

github-actions bot commented Jan 13, 2025 •

edited

Loading

github-actions bot commented Jan 13, 2025 •

edited

Loading

jfecher left a comment

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

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

Conversation

asterite commented Jan 13, 2025 • edited Loading

Description

Problem

Summary

Additional Context

Documentation

PR Checklist

TomAFrench commented Jan 13, 2025

github-actions bot commented Jan 13, 2025 • edited Loading

Compilation Memory Report

github-actions bot commented Jan 13, 2025 • edited Loading

Execution Memory Report

github-actions bot commented Jan 13, 2025 • edited Loading

Compilation Report

github-actions bot commented Jan 13, 2025 • edited Loading

Execution Report

jfecher left a comment

Choose a reason for hiding this comment

asterite commented Jan 13, 2025 •

edited

Loading

github-actions bot commented Jan 13, 2025 •

edited

Loading

github-actions bot commented Jan 13, 2025 •

edited

Loading

github-actions bot commented Jan 13, 2025 •

edited

Loading

github-actions bot commented Jan 13, 2025 •

edited

Loading