fix: walk loop and switch bodies in module statement passes - #360
fix: walk loop and switch bodies in module statement passes#360ryanhill1 wants to merge 2 commits into
Conversation
has_measurements(), remove_measurements(), has_barriers() and remove_barriers() fall back to _statements when the module has not been unrolled, and that list can still hold for/while/switch bodies the walker did not descend into — so an occurrence inside a loop or switch was invisible and removal was a no-op. iter_quantum_statements and drop_statements now descend into loop blocks and switch cases. Fixes #354
Argus reviewAuto-review is off for this repo. Tick the box below to run a review on this PR.
Estimated cost
Tip: you can also comment |
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
@Argus-Eye review |
This comment has been minimized.
This comment has been minimized.
![argus-eye[bot]](https://avatars.githubusercontent.com/in/2969822?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔎 Argus · 10/10 — Correctly extends statement passes into loop and switch bodies
🔍 PR intent vs diff (LLM analysis)
Argus read the diff against the stated intent. This is not an execution log — reviewer still needs to test behavior.
Goal: Make module statement passes detect and remove measurements and barriers inside for, while, and switch bodies before unrolling.
Not in scope:
- The unrolled path is unchanged; loops and switches are fully expanded by unroll() and do not reach _unrolled_ast with their bodies intact.
Stated acceptance criteria (from PR/issue — not independently verified): - iter_quantum_statements descends into for and while loop blocks and switch cases/default.
- drop_statements descends into for and while loop blocks and switch cases/default.
- has_measurements(), remove_measurements(), has_barriers(), and remove_barriers() handle occurrences inside non-unrolled loop and switch bodies.
✅ Intent delivered
Verdict: This PR adds the intended traversal and removal support for measurements and barriers in non-unrolled loop and switch bodies. It is ready to merge.
💡 2 P2 · 4 files reviewed
Architecture: The recursive traversal keeps module-level statement passes consistent with nested AST structure; broader nested-control-flow coverage would further protect this behavior.
2 findings · 2 inline · 0 folded
🔢 118.9k tokens · $0.2737 total
| Stage | Tokens | Cost |
|---|---|---|
| Intent | 2.6k | $0.0000 |
| Triage | 2.8k | $0.0000 |
| Lead agent | 1.5k | $0.0000 |
| Review · bug_hunter | 20.9k | $0.0674 |
| Review · security | 21.0k | $0.0665 |
| Review · architecture | 20.8k | $0.0707 |
| Review · regression | 21.1k | $0.0692 |
| Review | 24.9k | $0.0000 |
| Acceptance | 1.2k | $0.0000 |
| Scoring | 1.0k | $0.0000 |
| Synthesis | 932 | $0.0000 |
Contract: production/full · checked: bug_hunter, security, architecture, regression · review took 1m32s
Dashboard → · React 👎 to dismiss · Reply to any inline comment or use @argus-eye help to chat
- while-loop measurement removal now asserted, not just detection - barrier removal test asserts the loop's gates survive the pass
TheGupta2012
left a comment
TheGupta2012
left a comment
There was a problem hiding this comment.
Verdict
Approve with comments. The fix is correct, minimal, and lands exactly where issue #354 said it hurt. Every behaviour the PR claims was reproduced on this branch and confirmed absent on origin/main. Nothing here blocks merge. Four follow-up items are raised inline; three are Low and one is a Medium worth a short docstring or a small change before this pattern spreads further.
One correction to the PR description, and it is the most useful thing in this review: the description states "the unrolled path is unchanged: loops and switches are fully expanded by unroll() and never reach _unrolled_ast." That is not quite true. remove_measurements() / remove_barriers() end by assigning curr_module._unrolled_ast.statements = <filtered raw statements>, so calling either on a non-unrolled module writes loop nodes straight into _unrolled_ast. Measured directly after remove_barriers() on a module holding a for loop:
_unrolled_ast statement types: ['Include', 'QubitDeclaration', 'ForInLoop']
That matters because iter_quantum_statements has seven call sites, not four. Besides the four has_* / remove_* methods, it is also used by the operand-rewriting passes _remap_qubits (base.py:446) and reverse_qubit_order (base.py:602 and base.py:619), both of which read _unrolled_ast.statements. Those passes now descend into loop and switch bodies too. This was traced end-to-end and found safe today — both passes call unroll() first, which rebuilds _unrolled_ast from _statements with loops expanded. reverse_qubit_order(), remove_idle_qubits(), populate_idle_qubits() and depth() were each exercised after a pre-unroll remove_barriers() and none regressed. The residual hazard is noted inline.
Findings checklist
| # | Type | Severity | Item |
|---|---|---|---|
| 1 | Maintenance | Medium | In-place removal leaves original_program half-stripped |
| 2 | Implementation | Low | Emptied loop/switch bodies are kept, while an emptied box is dropped |
| 3 | Implementation | Low | Broadened walker now reaches loop bodies from the operand-rewriting passes |
| 4 | Maintenance | Low | Docstring should record the new pre-unroll over-approximation |
Security: no findings. The change adds pure AST traversal, introduces no I/O, no external input handling, no new dependency, and no eval/exec surface. Recursion depth follows program nesting depth, which the parser already bounds.
Minor polish, no action strictly required: the new switch test in tests/qasm3/test_measurement.py places the measurement in case 1 only, so the stmt.default is not None branch is walked but never actually removes anything. A second assertion with the measurement in default would close that gap; the equivalent was verified manually and passes.
What worked — verified, not merely read
Every row below was run on the PR worktree at b58c87c9 and again on a read-only origin/main worktree, with openqasm3 1.0.1.
AST shape confirmed against openqasm3 1.0.1 (a wrong attribute name would only raise on the untested path):
ForInLoop.block,WhileLoop.block— present.SwitchStatement.cases—list[tuple[list[Expression], CompoundStatement]], so thefor _, case_block in stmt.cases/case_block.statementsunpacking is right.SwitchStatement.default—CompoundStatement | None, so theis not Noneguard is right.QUANTUM_STATEMENTSis(QuantumGate, QuantumBarrier, QuantumReset, QuantumMeasurementStatement). None of those is also a container type, so the newelifbranches cannot shadow the finalelif isinstance(stmt, QUANTUM_STATEMENTS). Ordering is safe.
The #354 repro, all four methods, base vs PR. Target placed inside a for, a while, a switch case, and a switch default:
| Placement | has_* on main |
has_* on PR |
remove_* on main |
remove_* on PR |
|---|---|---|---|---|
for body |
False |
True |
no-op | removed |
while body |
False |
True |
no-op | removed |
switch case |
False |
True |
no-op | removed |
switch default |
False |
True |
no-op | removed |
The old behaviour was worse than a plain no-op: remove_measurements() set _has_measurements = False unconditionally, so has_measurements() reported False while dumps() still emitted measure. That silent lie is gone.
Nesting combinations — all reached, box handling from #345 still intact: for inside if; if inside a switch case; while inside a box; for inside for. In each case detection flipped False -> True and removal became effective. (The while-inside-box sample fails unroll() with Global variable 'i' must be a constant to use it in a local scope — reproduced identically on origin/main, unrelated to this PR.)
Round-trip after removal. Every filtered program was re-serialized with dumps() and re-parsed with loads(); all succeeded, including the degenerate cases where a body was emptied. See finding 2 for the cosmetic residue.
Pre-unroll / post-unroll consistency. For all eight sample programs, has_measurements() before unroll() and after unroll() now agree, where they disagreed on origin/main. The one remaining class of disagreement is a safe over-approximation and is covered by finding 4.
Mutation semantics. remove_barriers(in_place=False) was confirmed to leave a caller-held Program completely untouched — the copy()-first ordering introduced by #345 holds for the new loop and switch branches too. remove_barriers() followed by unroll() produces the correct expanded program and does not double-apply.
How it was tested
- Detached worktree at PR head
b58c87c9, read-only baseline worktree atorigin/main(7c31308), same interpreter and sameopenqasm31.0.1 for both. - Purpose-built probe scripts, run against both worktrees, covering: the four-method x four-placement matrix, eight nesting combinations, empty-body round-tripping, caller-AST mutation for both
in_placevalues,_unrolled_astcontents after a pre-unroll removal, and the downstream passesreverse_qubit_order(),remove_idle_qubits(),populate_idle_qubits(),depth(),num_qubits. - Full suite on the PR worktree: 717 passed, 4 skipped, 2 failed. Both failures are
tests/cli/test_cli_commands.py::test_validate_qasm_with_invalid_fileand::test_validate_command_with_invalid_file; both fail identically on theorigin/mainbaseline worktree, so they are environmental and not attributable to this PR. black --check src tests-> clean, 93 files unchanged.gh pr checks 360->Changelogpass,CodeRabbitskipped.
Could not be verified
pylintandisortare not installed in the available environment, so the repo's full lint gate was not reproduced locally.blackwas.- No test workflow appears in
gh pr checks 360— only the changelog check ran on CI, so CI has not independently exercised the new tests. - Qiskit interop was not exercised (
qiskitnot installed); no qiskit-facing surface is touched by this diff.
Next steps
- Rebase on
main. The PR is currentlyCONFLICTING, but only inCHANGELOG.md. There is no code conflict —src/pyqasm/modules/base.pymerges cleanly. A rebase and a re-stated changelog entry clears it. - Consider finding 1 before merge, or file it as a follow-up covering
boxandifas well, since the root cause predates this PR. - Findings 2, 3 and 4 are safe to defer.
Nice, tightly scoped fix — the walker was already the right abstraction, and extending it beat adding a parallel traversal.
| rewrites qubit operands has to reach the statements inside them too. Loop and switch | ||
| bodies only exist before unrolling, but walking them lets the same pass work on a | ||
| module that has not been unrolled (issue #354). |
There was a problem hiding this comment.
Type: Maintenance
Severity: Low
Rationale: Walking loop and switch bodies makes the pre-unroll answer conservative rather than exact, and the docstrings elsewhere still claim the unrolled AST is simply "a better indicator". The pre-unroll check now reports a measurement that the unrolled program provably never executes. Measured on this branch:
| Program | pre-unroll has_measurements() |
post-unroll |
|---|---|---|
measure in a non-taken switch case (const int i = 5; case 1 { measure }) |
True |
False |
measure in a zero-iteration loop (for int i in [0:-1]) |
True |
False |
measure in if (false) { ... } |
True |
False |
The if (false) row predates this PR (#345); the loop and switch rows are new. The direction is the safe one — a false positive is far better than the false negative that #354 reported — but a caller that branches on has_measurements() to decide whether to append its own measurements will now take the wrong branch on these programs, and nothing in the code says so.
Change Requested: Extend this docstring, and ideally the has_measurements / has_barriers docstrings, to state that the pre-unroll answer is an over-approximation: it reports occurrences that are syntactically present even when the branch or iteration never executes. One sentence is enough; no behaviour change is wanted here.
| elif isinstance(stmt, (qasm3_ast.ForInLoop, qasm3_ast.WhileLoop)): | ||
| yield from iter_quantum_statements(stmt.block) | ||
| elif isinstance(stmt, qasm3_ast.SwitchStatement): | ||
| for _, case_block in stmt.cases: | ||
| yield from iter_quantum_statements(case_block.statements) | ||
| if stmt.default is not None: | ||
| yield from iter_quantum_statements(stmt.default.statements) |
There was a problem hiding this comment.
Type: Implementation
Severity: Low
Rationale: This helper has seven call sites, not four. Beyond the has_* / remove_* methods it also backs _remap_qubits (base.py:446) and reverse_qubit_order (base.py:602, base.py:619), which rewrite qubit operands in _unrolled_ast.statements. Those passes now descend into loop and switch bodies as well.
The PR description says loops "never reach _unrolled_ast". They can. remove_barriers() / remove_measurements() end with curr_module._unrolled_ast.statements = <filtered raw statements>, so on a non-unrolled module the raw loop nodes land in _unrolled_ast. Verified directly after remove_barriers() on a module holding a for loop:
_unrolled_ast statement types: ['Include', 'QubitDeclaration', 'ForInLoop']
This was traced end-to-end and is not exploitable today: _remap_qubits and reverse_qubit_order both call unroll() first, which rebuilds _unrolled_ast from _statements with loops expanded. reverse_qubit_order(), remove_idle_qubits(), populate_idle_qubits() and depth() were each run after a pre-unroll remove_barriers() on a loop-bearing program and all behaved correctly.
The hazard is latent rather than live. If any future operand pass reads _unrolled_ast without unrolling first, it will now reach a q[i] inside a for body, where the index node is an Identifier and bit.indices[0][0].value raises AttributeError. Before this PR the loop was invisible, so that pass silently skipped it.
Change Requested: No code change is required. Please correct the "never reach _unrolled_ast" claim in the PR description, and consider a short comment here noting that the operand-rewriting callers rely on unroll() having expanded loops first — that invariant is currently implicit and easy to break.
| stmt.if_block = drop_statements(stmt.if_block, unwanted) | ||
| stmt.else_block = drop_statements(stmt.else_block, unwanted) | ||
| elif isinstance(stmt, (qasm3_ast.ForInLoop, qasm3_ast.WhileLoop)): | ||
| stmt.block = drop_statements(stmt.block, unwanted) |
There was a problem hiding this comment.
Type: Implementation
Severity: Low
Rationale: The Box branch a few lines above drops a box whose body was emptied (if not stmt.body: continue), but the loop and switch branches keep an emptied container. Removing the only statement from a loop or a case leaves dead scaffolding in the output:
// after remove_barriers() on: for int i in [0:1] { barrier q; }
for int i in [0:1] {
}
// after remove_barriers() on: switch (i) { case 1 { barrier q; } default { x q; } }
switch (i) {
case 1 {
}
default {
x q;
}
}This was checked carefully and is cosmetic, not a correctness bug: both forms re-serialize with dumps(), re-parse with loads(), and unroll() cleanly to nothing. Severity stays Low for that reason.
Change Requested: Consider mirroring the Box treatment so an emptied ForInLoop / WhileLoop / case body is dropped, which would also let the docstring above stop singling out box. Deliberately no suggestion block: dropping an emptied while changes runtime behaviour (an emptied non-terminating while becomes a no-op rather than a hang), so the call belongs to the author. Leaving this as-is and noting it in the docstring is an equally acceptable resolution.
| elif isinstance(stmt, (qasm3_ast.ForInLoop, qasm3_ast.WhileLoop)): | ||
| stmt.block = drop_statements(stmt.block, unwanted) | ||
| elif isinstance(stmt, qasm3_ast.SwitchStatement): | ||
| for _, case_block in stmt.cases: | ||
| case_block.statements = drop_statements(case_block.statements, unwanted) | ||
| if stmt.default is not None: | ||
| stmt.default.statements = drop_statements(stmt.default.statements, unwanted) |
There was a problem hiding this comment.
Type: Maintenance
Severity: Medium
Rationale: These branches filter nested bodies in place, so an in-place removal now mutates the module's own _original_program, which stays reachable through the public original_program property. The top level is rebuilt into a fresh list, but the nested bodies are edited on the shared nodes, so the exposed "original" ends up being neither the original nor the filtered result. Reproduced on this branch:
m = loads('OPENQASM 3.0; include "stdgates.inc"; qubit[2] q; barrier q; for int i in [0:1] { h q[i]; barrier q; }')
m.remove_barriers()
# module.original_program -> keeps `barrier q;` at top level, but the barrier
# INSIDE the for body is gone
# dumps(module) -> both barriers gone (correct)The design comes from #345 and already applies to box and if; this PR widens its reach to loops and switches, which is where it becomes easy to hit. To be precise about the blast radius: __str__ is not affected after a removal, because _unrolled_ast.statements is non-empty by then and __str__ takes the unrolled branch. The exposure is the original_program property, plus any caller that constructed the module from a Program it still holds.
The in_place=False path is sound — copy() runs before filtering, and a caller-held Program was confirmed byte-identical afterwards. Only in_place=True (the default) is affected.
Change Requested: Either deep-copy the nested bodies before filtering them so _original_program stays pristine, or document on remove_measurements / remove_barriers that in_place=True also rewrites nested bodies of the original program and that original_program is not a faithful pre-removal snapshot afterwards. Since the root cause predates this PR, splitting this into a follow-up that covers box and if at the same time is a perfectly reasonable answer.
3 participants
Fixes #354
has_measurements()/remove_measurements()/has_barriers()/remove_barriers()fall back toself._statementswhen the module has not been unrolled, and the statement walker did not descend intofor/while/switchbodies — so an occurrence inside one was invisible and removal was a no-op.iter_quantum_statementsanddrop_statements(the walker #345 introduced forbox/if) now descend into loop blocks and switch cases/default. The unrolled path is unchanged: loops and switches are fully expanded byunroll()and never reach_unrolled_ast.Also in this PR:
Nested Statement Removal Flow
Auto-enriched by Argus