Skip to content

chore(elaborator): Ensure that push_location and push_type cannot be forgotten#10374

Merged
aakoshh merged 7 commits intomasterfrom
af/elaborator-pusher
Nov 5, 2025
Merged

chore(elaborator): Ensure that push_location and push_type cannot be forgotten#10374
aakoshh merged 7 commits intomasterfrom
af/elaborator-pusher

Conversation

@aakoshh
Copy link
Contributor

@aakoshh aakoshh commented Nov 5, 2025

Description

Problem*

Resolves a potential for forgetting to set the location or type of an Expression or Statement. Makes it easier to review the code knowing that it cannot happen.

Summary*

Adds a PushedExpr construct which wrap an ExprId and show the status of what has been pushed in the type as well as with flags. If not all pieces are present when the variable goes out of scope, it panics. The fact that we have to unwrap or dereference means the compiler tells us when we need to push the type information.

The new type also makes it possible to demand that location has been pushed, which is used in at least one method that produces the Type: if we ask for PushedExpr<HasLocation> then the compiler ensures that there shouldn't be a panic when querying the interner for the location of the ID.

The only place I found the type was missing was in make_error.

Additional Context

Originally I added support for pushing the type first, and then the location, or vice versa, but in practice location is always immediately available, so I changed all call sites to use either push_expr_full, or intern_expr followed by a new intern_expr_type. Since we never do it the other way, I removed the option to start with push_type, thinking that since all of these are pub, should we add another necessary pushable thing later, I don't want to support 3*2 paths to completion, if it can be restricted to go in a linear fashion.

For statements it turns out that the location was always available, and so I just used a new push_stmt_full everywhere, but still added support for pushing separately. Originally a PushedStmt was added, but it was removed as it wasn't used, along with push_stmt.

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.

@aakoshh aakoshh added this to the Group 7 Audited milestone Nov 5, 2025
@aakoshh aakoshh requested a review from a team November 5, 2025 12:51
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: 687015a Previous: 27c9f67 Ratio
test_report_zkpassport_noir-ecdsa_ 3 s 1 s 3

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

CC: @TomAFrench

Copy link
Contributor

@jfecher jfecher left a comment

Choose a reason for hiding this comment

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

LGTM. Some nits

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 'Execution Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite Current: 5e9af29 Previous: dec5ef2 Ratio
rollup-block-root-single-tx 0.003 s 0.002 s 1.50

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

CC: @TomAFrench

@aakoshh aakoshh enabled auto-merge November 5, 2025 14:34
@aakoshh aakoshh added this pull request to the merge queue Nov 5, 2025
auto-merge was automatically disabled November 5, 2025 15:11

Pull Request is not mergeable

Merged via the queue into master with commit 9c554df Nov 5, 2025
133 checks passed
@aakoshh aakoshh deleted the af/elaborator-pusher branch November 5, 2025 15:26
AztecBot added a commit to AztecProtocol/aztec-packages that referenced this pull request Nov 6, 2025
Automated pull of nightly from the [noir](https://github.com/noir-lang/noir) programming language, a dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
chore(frontend): Loop control flow unit tests (noir-lang/noir#10387)
chore: Release Noir(1.0.0-beta.15) (noir-lang/noir#10125)
feat(doc): crate name, version, and dark mode (noir-lang/noir#10378)
chore(elaborator): Add `LoopStatement` (noir-lang/noir#10377)
chore(elaborator): Ensure that `push_location` and `push_type` cannot be forgotten (noir-lang/noir#10374)
feat: nargo doc (noir-lang/noir#10314)
chore: Add remaining doc comments to interpreter (noir-lang/noir#10368)
chore: green light for bn254_blackbox_solver audit (noir-lang/noir#10371)
chore: elaborator types.rs nits (noir-lang/noir#10375)
fix: do not replace return for databus (noir-lang/noir#10355)
END_COMMIT_OVERRIDE

Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>
AztecBot added a commit to AztecProtocol/aztec-packages that referenced this pull request Nov 6, 2025
Automated pull of nightly from the [noir](https://github.com/noir-lang/noir) programming language, a dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
chore(frontend): Loop control flow unit tests (noir-lang/noir#10387)
chore: Release Noir(1.0.0-beta.15) (noir-lang/noir#10125)
feat(doc): crate name, version, and dark mode (noir-lang/noir#10378)
chore(elaborator): Add `LoopStatement` (noir-lang/noir#10377)
chore(elaborator): Ensure that `push_location` and `push_type` cannot be forgotten (noir-lang/noir#10374)
feat: nargo doc (noir-lang/noir#10314)
chore: Add remaining doc comments to interpreter (noir-lang/noir#10368)
chore: green light for bn254_blackbox_solver audit (noir-lang/noir#10371)
chore: elaborator types.rs nits (noir-lang/noir#10375)
fix: do not replace return for databus (noir-lang/noir#10355)
END_COMMIT_OVERRIDE
github-merge-queue bot pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Nov 6, 2025
Automated pull of nightly from the
[noir](https://github.com/noir-lang/noir) programming language, a
dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
chore(frontend): Loop control flow unit tests
(noir-lang/noir#10387)
chore: Release Noir(1.0.0-beta.15)
(noir-lang/noir#10125)
feat(doc): crate name, version, and dark mode
(noir-lang/noir#10378)
chore(elaborator): Add `LoopStatement`
(noir-lang/noir#10377)
chore(elaborator): Ensure that `push_location` and `push_type` cannot be
forgotten (noir-lang/noir#10374)
feat: nargo doc (noir-lang/noir#10314)
chore: Add remaining doc comments to interpreter
(noir-lang/noir#10368)
chore: green light for bn254_blackbox_solver audit
(noir-lang/noir#10371)
chore: elaborator types.rs nits
(noir-lang/noir#10375)
fix: do not replace return for databus
(noir-lang/noir#10355)
END_COMMIT_OVERRIDE
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.

2 participants