fix: Fix evaluation order of assignments with side-effects in their rhs#8342
fix: Fix evaluation order of assignments with side-effects in their rhs#8342
Conversation
vezenovm
left a comment
There was a problem hiding this comment.
Sounds like we still have a failure for the case you posted in the description?
a.0[{ a = ([1,2], 3); 1 }] = [4, 5];Can we add a failing test for this before merging with a link to a new issue? Happy to translate my comments on #8262 into an issue as well
Changes to number of Brillig opcodes executed
🧾 Summary (10% most significant diffs)
Full diff report 👇
|
Changes to Brillig bytecode sizes
🧾 Summary (10% most significant diffs)
Full diff report 👇
|
038bdd3 to
a9599e8
Compare
There was a problem hiding this comment.
⚠️ 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: 185ed45 | Previous: b8d04d6 | Ratio |
|---|---|---|---|
test_report_noir-lang_noir_bigcurve_ |
289 s |
233 s |
1.24 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
Automated pull of nightly from the [noir](https://github.com/noir-lang/noir) programming language, a dependency of Aztec. BEGIN_COMMIT_OVERRIDE fix: always type-check turbofish, and error when it's not allowed (noir-lang/noir#8437) chore: Release Noir(1.0.0-beta.5) (noir-lang/noir#7955) feat(greybox_fuzzer): Parallel fuzz tests (noir-lang/noir#8432) fix(ssa): Mislabeled instructions with side effects in EnableSideEffectsIf removal pass (noir-lang/noir#8355) feat: SSA pass impact report (noir-lang/noir#8393) chore: bump external pinned commits (noir-lang/noir#8433) chore: separate benchmarking from github actions more (noir-lang/noir#7943) chore(fuzz): Break up the AST fuzzer `compare` module (noir-lang/noir#8431) chore(fuzz): Rename `init_vs_final` to `min_vs_full` (noir-lang/noir#8430) fix!: error on tuple mismatch (noir-lang/noir#8424) chore: bump external pinned commits (noir-lang/noir#8429) chore(acir): Test whether the predicate has an effect on slice intrinsics (noir-lang/noir#8421) feat(ssa): Mark transitively dead parameters during DIE (noir-lang/noir#8254) fix(ssa_gen): Do not code gen fetching of empty arrays when initializing the data bus (noir-lang/noir#8426) chore: remove `.aztec-sync-commit` (noir-lang/noir#8415) chore(test): Add more unit tests for `inline_functions_with_at_most_one_instruction` (noir-lang/noir#8418) chore: add minor docs for interpreter (noir-lang/noir#8397) fix: print slice composite types surrounded by parentheses (noir-lang/noir#8412) feat: Skip SSA passes that contain any of the given messages (noir-lang/noir#8416) fix: disable range constraints using the predicate (noir-lang/noir#8396) chore: bumping external libraries (noir-lang/noir#8406) chore: redo typo PR by shystrui1199 (noir-lang/noir#8405) feat(test): add `nargo_fuzz_target` (noir-lang/noir#8308) fix: allow names to collide in the values/types namespaces (noir-lang/noir#8286) fix: Fix sequencing of side-effects in lvalue (noir-lang/noir#8384) feat(greybox_fuzzer): Maximum executions parameter added (noir-lang/noir#8390) fix: warn on and discard unreachable statements after break and continue (noir-lang/noir#8382) fix: add handling for u128 infix ops in interpreter (noir-lang/noir#8392) chore: move acirgen tests into separate file (noir-lang/noir#8376) feat(fuzz): initial version of comptime vs brillig target for AST fuzzer (noir-lang/noir#8335) chore: apply lints to `ast_fuzzer` (noir-lang/noir#8386) chore: add note on AI generated PRs in `CONTRIBUTING.md` (noir-lang/noir#8385) chore: document flattening pass (noir-lang/noir#8312) fix: comptime shift-right overflow is zero (noir-lang/noir#8380) feat: let static_assert accept any type for its message (noir-lang/noir#8322) fix(expand): output safety comment before statements (noir-lang/noir#8378) chore: avoid need to rebuild after running tests (noir-lang/noir#8379) chore: bump dependencies (noir-lang/noir#8372) chore: Add GITHUB_TOKEN to cross build (noir-lang/noir#8370) chore: redo typo PR by GarmashAlex (noir-lang/noir#8364) chore: remove unsafe code from greybox fuzzer (noir-lang/noir#8315) feat: add `--fuzz-timeout` to `nargo test` options (noir-lang/noir#8326) chore: bump external pinned commits (noir-lang/noir#8334) fix(expand): try to use "Self" in function calls (noir-lang/noir#8353) fix: Fix evaluation order of assignments with side-effects in their rhs (noir-lang/noir#8342) fix: let comptime Field value carry the field's sign (noir-lang/noir#8343) fix: Ordering of items in callstacks (noir-lang/noir#8338) chore: add snapshosts for nargo expand tests (noir-lang/noir#8318) fix(ownership): Clone global arrays (noir-lang/noir#8328) chore: Replace all SSA interpreter panics with error variants (noir-lang/noir#8311) feat: Metamorphic AST fuzzing (noir-lang/noir#8299) fix: fix some Display implementations for AST nodes (noir-lang/noir#8316) chore: remove leftover file (noir-lang/noir#8313) fix: uses non-zero points with ec-add-unsafe (noir-lang/noir#8248) END_COMMIT_OVERRIDE --------- Co-authored-by: AztecBot <[email protected]> Co-authored-by: Tom French <[email protected]>
Automated pull of nightly from the [noir](https://github.com/noir-lang/noir) programming language, a dependency of Aztec. BEGIN_COMMIT_OVERRIDE fix: always type-check turbofish, and error when it's not allowed (noir-lang/noir#8437) chore: Release Noir(1.0.0-beta.5) (noir-lang/noir#7955) feat(greybox_fuzzer): Parallel fuzz tests (noir-lang/noir#8432) fix(ssa): Mislabeled instructions with side effects in EnableSideEffectsIf removal pass (noir-lang/noir#8355) feat: SSA pass impact report (noir-lang/noir#8393) chore: bump external pinned commits (noir-lang/noir#8433) chore: separate benchmarking from github actions more (noir-lang/noir#7943) chore(fuzz): Break up the AST fuzzer `compare` module (noir-lang/noir#8431) chore(fuzz): Rename `init_vs_final` to `min_vs_full` (noir-lang/noir#8430) fix!: error on tuple mismatch (noir-lang/noir#8424) chore: bump external pinned commits (noir-lang/noir#8429) chore(acir): Test whether the predicate has an effect on slice intrinsics (noir-lang/noir#8421) feat(ssa): Mark transitively dead parameters during DIE (noir-lang/noir#8254) fix(ssa_gen): Do not code gen fetching of empty arrays when initializing the data bus (noir-lang/noir#8426) chore: remove `.aztec-sync-commit` (noir-lang/noir#8415) chore(test): Add more unit tests for `inline_functions_with_at_most_one_instruction` (noir-lang/noir#8418) chore: add minor docs for interpreter (noir-lang/noir#8397) fix: print slice composite types surrounded by parentheses (noir-lang/noir#8412) feat: Skip SSA passes that contain any of the given messages (noir-lang/noir#8416) fix: disable range constraints using the predicate (noir-lang/noir#8396) chore: bumping external libraries (noir-lang/noir#8406) chore: redo typo PR by shystrui1199 (noir-lang/noir#8405) feat(test): add `nargo_fuzz_target` (noir-lang/noir#8308) fix: allow names to collide in the values/types namespaces (noir-lang/noir#8286) fix: Fix sequencing of side-effects in lvalue (noir-lang/noir#8384) feat(greybox_fuzzer): Maximum executions parameter added (noir-lang/noir#8390) fix: warn on and discard unreachable statements after break and continue (noir-lang/noir#8382) fix: add handling for u128 infix ops in interpreter (noir-lang/noir#8392) chore: move acirgen tests into separate file (noir-lang/noir#8376) feat(fuzz): initial version of comptime vs brillig target for AST fuzzer (noir-lang/noir#8335) chore: apply lints to `ast_fuzzer` (noir-lang/noir#8386) chore: add note on AI generated PRs in `CONTRIBUTING.md` (noir-lang/noir#8385) chore: document flattening pass (noir-lang/noir#8312) fix: comptime shift-right overflow is zero (noir-lang/noir#8380) feat: let static_assert accept any type for its message (noir-lang/noir#8322) fix(expand): output safety comment before statements (noir-lang/noir#8378) chore: avoid need to rebuild after running tests (noir-lang/noir#8379) chore: bump dependencies (noir-lang/noir#8372) chore: Add GITHUB_TOKEN to cross build (noir-lang/noir#8370) chore: redo typo PR by GarmashAlex (noir-lang/noir#8364) chore: remove unsafe code from greybox fuzzer (noir-lang/noir#8315) feat: add `--fuzz-timeout` to `nargo test` options (noir-lang/noir#8326) chore: bump external pinned commits (noir-lang/noir#8334) fix(expand): try to use "Self" in function calls (noir-lang/noir#8353) fix: Fix evaluation order of assignments with side-effects in their rhs (noir-lang/noir#8342) fix: let comptime Field value carry the field's sign (noir-lang/noir#8343) fix: Ordering of items in callstacks (noir-lang/noir#8338) chore: add snapshosts for nargo expand tests (noir-lang/noir#8318) fix(ownership): Clone global arrays (noir-lang/noir#8328) chore: Replace all SSA interpreter panics with error variants (noir-lang/noir#8311) feat: Metamorphic AST fuzzing (noir-lang/noir#8299) fix: fix some Display implementations for AST nodes (noir-lang/noir#8316) chore: remove leftover file (noir-lang/noir#8313) fix: uses non-zero points with ec-add-unsafe (noir-lang/noir#8248) END_COMMIT_OVERRIDE --------- Co-authored-by: AztecBot <[email protected]> Co-authored-by: Tom French <[email protected]>
Description
Problem*
Resolves #8262
Resolves #8337
Summary*
Changed evaluation order of assign-subexpressions to match the comptime interpreter.
Additional Context
@vezenovm mentioned here #8262 (comment) that we could also desugar the blocks to remove any internal statements beforehand. This would still be beneficial since we can still get confusing results by doing:
ie. assigning within the lvalue will now assign after the
[4, 5]assignment instead of before. This is probably unexpected though it should be internally consistent now at least. That being said, changing this to remove internal statements beforehand to remove all sub-expressions requires an entire compiler pass which would be a larger pass so I thought I'd get this out first.Edit: We're still not completely consistent here. There are differences between the comptime interpreter and SSA/brillig in compiling the above example but both are incorrect.
Documentation*
Check one:
PR Checklist*
cargo fmton default settings.