fix: walk nested box and if bodies in module transformation passes - #345
fix: walk nested box and if bodies in module transformation passes#345ryanhill1 wants to merge 3 commits into
Conversation
remove_idle_qubits and reverse_qubit_order only looked at top-level statements, so operands inside box and if blocks kept stale indices -- silently addressing the wrong qubit, or naming a qubit outside the shrunken register, which no longer parses. Both passes now walk nested bodies, as do the measurement and barrier removal passes. A qubit touched only inside a branch is recorded as used so it is not mistaken for idle, and operands that are physical qubits are skipped by the remap instead of tripping an assert.
Argus reviewAuto-review is off for this repo. Tick the box below to run a review on this PR. Running Argus review... 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[bot]](https://avatars.github.com/in/2969822?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔎 Argus · 9/10 — Argus found 3 issues (0 critical, 1 warnings) across 7 files.
🔍 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: Walk nested box and if bodies in module transformation passes so qubit remapping and measurement/barrier handling work correctly.
Not in scope:
- Fix shared AST nodes in remove_measurements(in_place=False); nested filtering retains the pre-existing mutation behavior.
- Change branch qubit depth values; depth is still settled when the branch closes.
Stated acceptance criteria (from PR/issue — not independently verified): - Nested quantum statements have operand indices remapped by remove_idle_qubits and reverse_qubit_order.
- Measurements and barriers inside box and if bodies are found and removed by the corresponding passes.
- A qubit used only inside a branch is not classified as idle or removed.
- Physical qubit operands are skipped during remapping.
- Empty boxes are dropped after removing measurements or barriers, while empty if blocks are retained.
✅ Intent delivered
Argus found 3 issues (0 critical, 1 warnings) across 7 files. Key concerns: bug, testing.
Simulation Results
Tested 1 scenarios, 1 potential issues found:
Scenario: src/pyqasm/visitor.py: A trailing pragma inside a box leaks verbatim state into the enclosing scope
Verdict: Broken (90% sure)
Why: The PR does not reset pragma verbatim state when leaving a box scope.
Fix: Restore the prior verbatim state after visiting a box body, including trailing pragmas.
1 additional findings on lines outside the diff
- 💡
tests/qasm3/test_transformations.py:L362[suggestion] The branch assertion cannot detect whether branch operands are remappedA future or existing traversal failure can silently leave branch operands unchanged while CI reports the transformation as correct.
3 findings · 2 inline · 1 folded
🔢 407.8k tokens · $1.4666 total
| Stage | Tokens | Cost |
|---|---|---|
| Intent | 3.4k | $0.0024 |
| Triage | 5.2k | $0.0009 |
| Lead agent | 4.0k | $0.0098 |
| Review · bug_hunter | 88.2k | $0.3393 |
| Review · security | 85.8k | $0.2563 |
| Review · architecture | 87.3k | $0.3385 |
| Review · regression | 88.1k | $0.2904 |
| Review | 32.3k | $0.2030 |
| Acceptance | 1.9k | $0.0042 |
| Simulation | 8.7k | $0.0116 |
| Scoring | 2.9k | $0.0102 |
Contract: production/full · checked: bug_hunter, security, architecture, regression · review took 2m24s
Dashboard → · React 👎 to dismiss · Reply to any inline comment or use @argus-eye help to chat
| for qubit_subset in [op_qubits] + [ctrls]: | ||
| for qubit in qubit_subset: | ||
| qubit_name, qubit_idx = QasmVisitor._get_qubit_name_and_id(qubit) | ||
| self._is_branch_qubits.add((qubit_name, qubit_idx)) | ||
| self._mark_branch_qubit(qubit_name, qubit_idx) |
There was a problem hiding this comment.
🟡 **P1 (7/10) · Bug:** Custom gates double-count branch operations after expansion (medium confidence)
Operation metadata becomes inaccurate for every custom gate used in a branch, affecting consumers of QubitDepthNode._total_ops().
| for qubit_subset in [op_qubits] + [ctrls]: | |
| for qubit in qubit_subset: | |
| qubit_name, qubit_idx = QasmVisitor._get_qubit_name_and_id(qubit) | |
| self._is_branch_qubits.add((qubit_name, qubit_idx)) | |
| self._mark_branch_qubit(qubit_name, qubit_idx) | |
| for qubit_subset in [op_qubits] + [ctrls]: | |
| for qubit in qubit_subset: | |
| qubit_name, qubit_idx = QasmVisitor._get_qubit_name_and_id(qubit) | |
| self._is_branch_qubits.add((qubit_name, qubit_idx)) |
React 👎 to dismiss · Argus learns from feedback
- remove_measurements/remove_barriers(in_place=False) took the copy after filtering, and the nested filter rewrites box and if bodies in place, so the original lost its nested measurements and barriers too - branch operations are now flagged rather than counted: a branch body can be visited more than once and a custom gate marks both its own operands and those of its expansion, so the count was never meaningful - reverse-order test used a branch operand that maps to itself, which could not tell a walked branch from a skipped one
|
Both findings fixed in ea621d6. Out-of-place removal mutating the original — confirmed: Custom gates double-counting branch operations — the count was wrong, but narrowing it to the custom-gate site would not have fixed it: a branch body is visited more than once, so Folded finding on the branch assertion — also valid. |
There was a problem hiding this comment.
Approved with comments — this converts three classes of silently wrong qubit operands into correct output. None of the three notes below is blocking.
Findings
All findings are posted inline on the relevant lines. Checklist:
- L1 —
_total_ops()gains a non-operation term; second reader exists inprinter.py - L2 — PR description's
in_place=Falsealiasing caveat is contradicted by this PR's own tests - L3 — Non-unrolled path still misses
for/while/switchbodies
How it was tested
Side-by-side on main vs this branch. The first two rows are the dangerous ones — nothing raises, the program still parses, it just addresses different qubits than the author wrote:
| Program | main |
this branch |
|---|---|---|
qubit[3] q; bit[1] c; c[0]=measure q[0]; if (c[0]) { x q[2]; } + remove_idle_qubits() |
emits qubit[1] q; while the body still reads x q[2]; — reloading raises Index 2 out of range for register of size 1 |
qubit[2] q; with x q[1];, round-trips |
qubit[3] q; h q[0]; box { cx q[0], q[1]; } + reverse_qubit_order() |
h q[2]; at top level, box { cx q[0], q[1]; } untouched |
h q[2]; box { cx q[2], q[1]; } |
has_measurements() / has_barriers() with the occurrence inside a box or if |
False |
True |
remove_measurements() / remove_barriers(), same shape |
no-op | filtered, output re-validates |
qubit[3] q; h q[0]; h $1; + remove_idle_qubits() / reverse_qubit_order() |
AssertionError / AttributeError: 'str' object has no attribute 'name' |
both correct |
module.depth()identical on both branches across four programs (2 / 2 / 1 / 3) — the description's claim holds for the public API and the.depthfield.- Container containment:
switch,whileandforfully unroll and do not reach_unrolled_astwith bodies intact (aswitchoverx q[2]unrolls to a barex q[2]), soBoxandBranchingStatementare the only two the walk must handle.boxinside anifandelse_blockboth covered. - Empty-container asymmetry:
boxemptied by a removal is dropped,ifis kept asif (c[0] == true) { }; both re-load and validate. used_in_branchstaleness afterremove_measurements()— does not bite, becauseremove_idle_qubits()re-unroll()s and rebuilds the depth map.- Physical qubits inside a branch register a
('$1', 1)key, so_mark_branch_qubit's direct index does notKeyError. - Reverting only the
curr_module = self if in_place else self.copy()hoist failstest_remove_measurement_not_in_place_leaves_the_original_alone— confirming that reordering is load-bearing, not cosmetic. - Suite on branch: 645 passed, 4 skipped (excluding
tests/cli, which fails identically onmainin this environment).
Next steps
Merge when ready. L2 is a one-paragraph edit to the PR description and worth doing before merge so it does not leave a future reader hunting a phantom aliasing bug. L1 is attached as an applicable suggestion and worth taking while the code is fresh; L3 is fine as a follow-up issue.
| """ | ||
| # copy first: the filtering rewrites nested box and if bodies in place, so it | ||
| # has to run on the module that is being returned | ||
| curr_module = self if in_place else self.copy() |
There was a problem hiding this comment.
[L2] The description's aliasing caveat is contradicted by this line — Maintenance · Low
Rationale: the PR description's last paragraph states that remove_measurements(in_place=False) "shares AST nodes between the original and the copy (pre-existing); the nested filtering mutates stmt.body in place, so it inherits that behaviour rather than fixing it."
This hoisted copy() is what rules that out. QasmModule.copy() is return deepcopy(self) (src/pyqasm/modules/base.py:803-805), so the returned module shares nothing with the original. Testing for exactly the aliasing the caveat predicts — box { h q[0]; c[0] = measure q[0]; barrier q; }, calling remove_measurements(in_place=False) and remove_barriers(in_place=False), on both the unrolled (_unrolled_ast.statements) and non-unrolled (_statements) paths — the original is byte-identical before and after in all four combinations, and only the returned copy is filtered. Same for remove_idle_qubits(in_place=False) with a box.
This PR's own tests assert it too: test_remove_measurement_not_in_place_leaves_the_original_alone and test_remove_barriers_not_in_place_leaves_the_original_alone. The caveat reads as a note written before the copy was hoisted and not revisited afterwards.
Change requested: drop that paragraph from the description before merge, so it does not leave a future reader hunting for a latent aliasing bug that this hoist already rules out.
There was a problem hiding this comment.
Resolved — paragraph dropped from the description.
You are right that the hoisted copy() rules it out: QasmModule.copy() is deepcopy, so the returned module shares nothing. The caveat was written before the hoist and not revisited. The description now states the opposite where it matters — that hoisting the copy above the filtering is what makes in_place=False safe, and that reverting only the hoist fails test_remove_measurement_not_in_place_leaves_the_original_alone.
| elif isinstance(stmt, BranchingStatement): | ||
| yield from iter_quantum_statements(stmt.if_block) | ||
| yield from iter_quantum_statements(stmt.else_block) | ||
| elif isinstance(stmt, QUANTUM_STATEMENTS): |
There was a problem hiding this comment.
[L3] Non-unrolled path still misses loop and switch bodies — Implementation · Low
Rationale: the walk closes the gap on the unrolled path, but has_measurements / remove_measurements / has_barriers / remove_barriers fall back to self._statements when the module has not been unrolled, and that list can still contain for / while / switch bodies this walker does not descend into. Verified on this branch:
qubit[2] q; bit[2] c;
for int i in [0:1] { c[i] = measure q[i]; barrier q; }Without unroll(), has_measurements() and has_barriers() both return False and remove_measurements() is a no-op; after unroll() both return True and removal works.
This is pre-existing — main behaves identically — and the PR strictly improves the situation, so it is not worth holding the merge on. Flagged because this walker is the natural home for the remainder, and it is easy to read the PR as having closed the class of bug entirely.
Change requested: either extend iter_quantum_statements to loop and switch bodies for the non-unrolled path, or document on those four methods that they are only meaningful after unroll(). A follow-up issue is fine.
There was a problem hiding this comment.
Follow-up filed as #354 and referenced in the description, per your note that an issue is fine here.
Reproduced your case on this branch: for int i in [0:1] { c[i] = measure q[i]; barrier q; } gives has_measurements() == has_barriers() == False without unroll(), True after. #354 records both remedies you offered (extend iter_quantum_statements, or document the four methods as post-unroll() only).
The description now has a "Not closed here" section naming the gap, and carries your finding that for/while/switch never reach _unrolled_ast with bodies intact — so the unrolled path really is complete with Box and BranchingStatement, and the remainder is non-unrolled only.
| return ( | ||
| self.num_resets | ||
| + self.num_measurements | ||
| + self.num_gates | ||
| + self.num_barriers | ||
| + int(self.used_in_branch) | ||
| ) | ||
|
|
||
| def is_idle(self) -> bool: | ||
| return self._total_ops() == 0 |
There was a problem hiding this comment.
[L1] _total_ops() gains a term that is not an operation count — Maintenance · Low
Rationale: int(self.used_in_branch) was folded in so that is_idle() picks it up, but _total_ops() has a second consumer at src/pyqasm/printer.py:181. Measured per-qubit values for qubit[3] q; bit[1] c; h q[0]; c[0]=measure q[0]; if (c[0]) { x q[1]; }:
main:{('q',0): 2, ('q',1): 0, ('q',2): 0}- this branch:
{('q',0): 2, ('q',1): 1, ('q',2): 0}
To be fair to the change, the impact was chased and there is none today: the value feeds max_depth in _compute_line_nums, which computes it (printer.py:167, 182) but returns only line_nums, sizes (printer.py:186). It is dead code, and pre-existing dead code this PR does not touch — so there is no rendering regression to fix.
The concern is narrower. _total_ops() no longer means "number of operations on this qubit", it means "operations, plus one if the qubit appears in a branch", and the one place that reads it for a non-idleness purpose looks like unfinished intent someone may well wire up later.
Change requested: keep the flag out of the arithmetic and express it in is_idle(), where it is the only thing it was introduced to affect. This also restores _total_ops() to the exact body it has on main.
| return ( | |
| self.num_resets | |
| + self.num_measurements | |
| + self.num_gates | |
| + self.num_barriers | |
| + int(self.used_in_branch) | |
| ) | |
| def is_idle(self) -> bool: | |
| return self._total_ops() == 0 | |
| return self.num_resets + self.num_measurements + self.num_gates + self.num_barriers | |
| def is_idle(self) -> bool: | |
| return self._total_ops() == 0 and not self.used_in_branch |
There was a problem hiding this comment.
Resolved in 8b1f2ac — suggestion applied verbatim.
_total_ops() is now byte-identical to main, and the flag is read by is_idle() alone. Confirmed your measurement flips back:
main {('q',0): 2, ('q',1): 0, ('q',2): 0}
this branch {('q',0): 2, ('q',1): 0, ('q',2): 0} <- was 1 for ('q',1)
with idleness still correct — q[1] is kept (used in branch), q[2] removed. Good catch on the second reader in printer.py; even dead, it is exactly the kind of thing someone wires up later against the wrong meaning.
_total_ops() means 'operations on this qubit' again -- byte-identical to main -- and the branch flag is expressed in is_idle(), the only thing it was introduced to affect. printer.py reads _total_ops() for max_depth, so the term did not belong in the arithmetic (L1).
3 participants
Summary of changes
Closes #342.
Problem
remove_idle_qubits()andreverse_qubit_order()iterate_unrolled_ast.statementsat the top level only.BoxandBranchingStatementare not inQUANTUM_STATEMENTSand their bodies were never descended into, so nested operands kept their old indices while top-level ones were rewritten — the output silently addressed the wrong qubits, and where a nested index fell outside the shrunken register it no longer parsed at all.has_measurements/remove_measurements/has_barriers/remove_barriershad the same blind spot.Two things the walk forced into scope
Branch qubits are not idle. Gates and measurements applied inside an
ifblock never incremented anyQubitDepthNodecounter, andis_idle()is_total_ops() == 0— so a qubit used only in a branch was removed as idle. Left alone, walkingifbodies would turn that silent breakage into aKeyErrorin the remap. Branch use is now recorded in aused_in_branchflag, kept out of the operation counters because a branch body's depth is only settled when the branch closes and the body may be visited more than once. The flag is read byis_idle()alone, so_total_ops()still means "operations on this qubit" and is byte-identical tomain. Depth values are unchanged.Physical qubits.
_remap_qubitsasserted every operand was anIndexedIdentifier, soremove_idle_qubits()raisedAssertionErroron a program mixing$1with a declared register. Physical operands are now skipped — they belong to no register, so there is nothing to remap.Worth a look
A
boxemptied byremove_measurements/remove_barriersis dropped rather than left behind, because pyqasm's own validator rejects a box with no statements and the output would not round-trip. Anifblock emptied the same way is kept —if (c) {}parses fine, and dropping it would discard the condition.Not closed here
The walk covers the unrolled path.
has_measurements/remove_measurements/has_barriers/remove_barriersfall back to_statementswhen the module has not been unrolled, and that list can still holdfor/while/switchbodies this walker does not descend into — so an occurrence inside a loop is invisible untilunroll()runs. Pre-existing and identical onmain; tracked in #354. On the unrolled path there is nothing left to cover: those three containers are fully unrolled and never reach_unrolled_astwith bodies intact, soBoxandBranchingStatementreally are the only two.Tests
Side-by-side against
main, the dangerous shapes being the ones where nothing raises and the program still parses — it just addresses different qubits than the author wrote:mainqubit[3] q; bit[1] c; c[0]=measure q[0]; if (c[0]) { x q[2]; }+remove_idle_qubits()qubit[1] q;while the body still readsx q[2];— reloading raisesIndex 2 out of range for register of size 1qubit[2] q;withx q[1];, round-tripsqubit[3] q; h q[0]; box { cx q[0], q[1]; }+reverse_qubit_order()h q[2];at top level,box { cx q[0], q[1]; }untouchedh q[2]; box { cx q[2], q[1]; }has_measurements()/has_barriers()with the occurrence inside aboxorifFalseTrueremove_measurements()/remove_barriers(), same shapequbit[3] q; h q[0]; h $1;+remove_idle_qubits()/reverse_qubit_order()AssertionError/AttributeErrormodule.depth()is identical on both branches across four programs, and_total_ops()per qubit matchesmainexactly.test_remove_measurement_not_in_place_leaves_the_original_aloneandtest_remove_barriers_not_in_place_leaves_the_original_alonecoverin_place=False:QasmModule.copy()is adeepcopy, and hoisting it above the filtering means the original shares nothing with the returned copy. Reverting only that hoist fails the first of those tests.