fix: reject mixed declared/physical programs under consolidate_qubits - #362
fix: reject mixed declared/physical programs under consolidate_qubits#362ryanhill1 wants to merge 4 commits into
Conversation
A program mixing declared registers with physical qubits consolidated into two address spaces the output could not relate: a virtual __PYQASM_QUBITS__ register plus absolute $n references, with num_qubits conflating the two. Such programs now raise a ValidationError naming the physical qubits. A program using only physical qubits no longer receives an internal register declaration nothing references. Fixes #353
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✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
@Argus-Eye review |
1 similar comment
|
@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 · 9/10 — Handles mixed qubit address spaces correctly, with one validation edge case to tighten
🔍 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: Reject mixed declared-register and physical-qubit programs under consolidate_qubits=True and suppress unused consolidated declarations for physical-only programs.
Stated acceptance criteria (from PR/issue — not independently verified):
- A program mixing declared registers with physical qubits under unroll(consolidate_qubits=True) raises a ValidationError naming the physical qubits.
- Pure-physical programs preserve physical qubits as written and do not receive an unreferenced PYQASM_QUBITS declaration.
- Update test_physical_qubits_are_not_consolidated and test_physical_qubits_only to reflect the new semantics.
✅ Intent delivered
Verdict: This implements the requested rejection and physical-only declaration behavior cleanly. It is close to merge-ready; addressing the remaining validation warning would make the semantics more robust.
🟡 1 P1 · 💡 1 P2 · 3 files reviewed
Architecture: The validation and allocation behavior is well scoped in the visitor, while tests clearly document the distinct declared-register and physical-qubit paths.
Simulation Results
Tested 2 scenarios, 2 potential issues found:
Scenario: src/pyqasm/visitor.py: Custom gates double-count branch operations after expansion
Verdict: Broken (93% sure)
Why: This change does not address custom gate expansion counting operations inside branches twice.
Fix: Update custom gate expansion to visit each branch body once when collecting or emitting operations.
Scenario: src/pyqasm/visitor.py: A trailing pragma inside a box leaks verbatim state into the enclosing scope
Verdict: Broken (97% sure)
Why: The change only alters qubit consolidation and does not restore scope after a pragma inside a box.
Fix: Save and restore verbatim pragma state when entering and leaving box scopes.
2 findings · 2 inline · 0 folded
🔢 186.1k tokens · $0.6403 total
| Stage | Tokens | Cost |
|---|---|---|
| Intent | 3.2k | $0.0000 |
| Triage | 2.5k | $0.0000 |
| Lead agent | 1.3k | $0.0000 |
| Review · bug_hunter | 41.6k | $0.1604 |
| Review · security | 41.2k | $0.1418 |
| Review · architecture | 41.4k | $0.1676 |
| Review · regression | 42.2k | $0.1705 |
| Acceptance | 994 | $0.0000 |
| Simulation | 9.5k | $0.0000 |
| Scoring | 1.2k | $0.0000 |
| Synthesis | 962 | $0.0000 |
Contract: production/full · checked: bug_hunter, security, architecture, regression · review took 1m26s
Dashboard → · React 👎 to dismiss · Reply to any inline comment or use @argus-eye help to chat
|
|
||
| def test_mixed_declared_and_physical_still_unrolls_without_consolidation(): | ||
| """The mixed-program rejection applies only under consolidate_qubits=True.""" |
There was a problem hiding this comment.
💡 P2 (5/10) · Testing: The regression test does not verify that the ValidationError names every physical qubit
Users could receive incomplete diagnostics for mixed programs while CI still reports the acceptance criterion as satisfied.
| def test_mixed_declared_and_physical_still_unrolls_without_consolidation(): | |
| """The mixed-program rejection applies only under consolidate_qubits=True.""" | |
| with pytest.raises(ValidationError) as err: | |
| result.unroll(consolidate_qubits=True) | |
| message = str(err.value) | |
| assert "mixes declared registers with physical qubits" in message | |
| assert "$2" in message |
A zero-sized declared register still declares a second address space, so qubit[0] q; h $1; now raises under consolidate_qubits=True. Also assert the error names the physical qubit (Argus P2).
…into fix-consolidate-mixed-physical
TheGupta2012
left a comment
TheGupta2012
left a comment
There was a problem hiding this comment.
Verdict: approve with minor comments
Settling #353 as option 2 is a defensible call, and the implementation is placed correctly: the check sits in _qubit_register_consolidation after the device_qubits capacity check, so the existing "total qubits exceed device qubits" error still wins when both apply. The scan over _qubit_depths turns out to be complete — every path that can put a physical qubit into a program registers it. No blocking issues. Four comments below: one Medium about a validation that the early return skips (and which the docstring in this PR still promises), three Low.
Findings
- Medium — the
return unrolled_stmtson the pure-physical path skips the reserved-name check, contradicting the docstring edited in this same PR.int __PYQASM_QUBITS__ = 3; h $1;raisesVariable '__PYQASM_QUBITS__' is already definedonorigin/mainand succeeds silently here. Hoisting the loop above the new block also restores the precise message for the qreg shape. - Low —
sorted()on$nstrings orders$10before$2in the error message. - Low — the error names the offending qubits but does not say what to do next, and no source span is available at
finalizetime, so the message text is all the user gets. - Low —
test_mixed_declared_and_physical_still_unrolls_without_consolidationasserts onlynum_qubits; the deleted test pinned the emitted QASM, so nothing now pins #344's "left as written" rule for a mixed program.
How it was tested
Behaviour was compared between a worktree at c18b624 and one at origin/main (7c31308), running every claim rather than reading for it.
Coverage of the physical-qubit scan (complete). A physical qubit reaching the program only through measure, reset, barrier, an if body, a box body, or a custom gate call site all land in _qubit_depths — _get_op_bits registers $n for every operand shape. All six now raise; all six emitted the two-address-space output on base.
No false positive on the pure-physical path. _global_qreg_size_map holds only user-declared registers at that point, so h $1; cz $2, $1; with consolidate_qubits=True returns cleanly, num_qubits == 3, no __PYQASM_QUBITS__ — with and without device_qubits.
device_qubits ordering is right. h $9; under device_qubits=3 still raises Total qubits '(10)' exceed device qubits '(3)'. — the capacity check fires before the new block, for physical-only and mixed programs alike.
"Presence, not capacity" holds. qubit[0] q; does create a _global_qreg_size_map entry, so the truthiness test is correct. (qreg q[0]; is unreachable — the QASM 2 parser rejects a zero-size qreg outright, so the qasm3 spelling is the only one that can exercise this.)
No regression with the flag off. The mixed program unrolls unchanged under plain unroll(). A corpus of 333 declared-registers-only programs harvested from tests/ dumps byte-identically on both worktrees under consolidate_qubits=True; the only two diffs are set-iteration order inside an unrelated "Unsupported OpenQASM version" message.
OpenPulse does not trip the new check. Worth recording, because defcalgrammar "openpulse" forces _consolidate_qubits = True at visitor.py:3328 regardless of what the caller passed. Under OpenPulse, gate, measure and reset operands are renamed to __PYQASM_QUBITS__[n] before registration, so no $n key ever reaches _qubit_depths and plain unroll() on an OpenPulse program behaves identically on both worktrees.
Suite and CI. 720 passed / 714 on base (the 6 new tests), same 2 pre-existing tests/cli failures on both worktrees, so nothing is attributable to this change. black --check clean on both touched files; all GitHub checks green. isort and pylint are absent from the local venv and were not run — CI covers them.
Blast radius. An org-wide code search finds no consumer of consolidate_qubits outside pyqasm itself and the docs, so the breaking change lands narrowly. That search does not see private repositories.
Next steps
- Rebase onto
origin/mainand resolve thesrc/pyqasm/visitor.pyconflict against #346's typing and docstring changes. #357 and #359 touch other regions of the same file. qBraid/docsv2/pyqasm/user-guide/advanced-features.mdxdocumentsconsolidate_qubitswithout the new rejection. Its examples use declared registers only, so they still hold, but the page should gain a line about mixed programs.- Two pre-existing behaviours surfaced while testing, identical on
origin/mainand therefore not findings against this PR, but each worth its own issue:- Under OpenPulse,
barrier $0is the one physical reference left unrenamed while every other one becomes__PYQASM_QUBITS__[n], so the output carries both spellings. Index-identity makes it unambiguous in practice, only inconsistent. - Unrolling a module twice resets
num_qubitsfrom 3 to 2 for a mixed program, dropping the physical qubit from the count. This PR makes the path easy to reach: catching the new error and re-runningunroll()without the flag is the natural remedy, and it lands on the stale count.
- Under OpenPulse,
| ) | ||
| # only physical qubits: nothing to consolidate, so do not declare an | ||
| # internal register nothing would reference | ||
| return unrolled_stmts |
There was a problem hiding this comment.
Type: Implementation
Severity: Medium
Rationale: This early return skips the INTERNAL_QUBIT_REGISTER reserved-name loop directly below, and the check is reachable with that name already declared. Verified against origin/main:
| program | origin/main |
this PR |
|---|---|---|
int __PYQASM_QUBITS__ = 3; h $1; |
ValidationError: Variable '__PYQASM_QUBITS__' is already defined |
succeeds silently |
qubit[2] __PYQASM_QUBITS__; h $1; |
same reserved-name error | ...mixes declared registers with physical qubits ($1) |
Neither outcome corrupts the output — no internal register is emitted on this path, so nothing actually collides. The concern is the contract: the docstring edited a few lines above still promises a raise "if the reserved register '__PYQASM_QUBITS__' is already declared", and on this path it no longer does. The second row is also a diagnostic downgrade: the user's real problem is the reserved name, but they are told about physical qubits instead.
Change Requested: Move the global_scope reserved-name loop (lines 526-532) above the new physical-qubit block, so it runs before either exit. One move fixes both rows and keeps the docstring honest. If the intent is instead that the guard should not apply when no register is emitted, narrow the docstring to say so.
| physical_qubits = sorted( | ||
| name for name, _ in self._module._qubit_depths if name.startswith("$") | ||
| ) |
There was a problem hiding this comment.
Type: Implementation
Severity: Low
Rationale: sorted() on the raw $n strings sorts lexicographically. A program using $2 and $10 produces ...physical qubits ($10, $2), verified at this commit. On a device with more than ten qubits the list reads as unordered, which makes it harder to scan for the offending references.
Change Requested: Sort by the numeric index.
| physical_qubits = sorted( | |
| name for name, _ in self._module._qubit_depths if name.startswith("$") | |
| ) | |
| physical_qubits = sorted( | |
| (name for name, _ in self._module._qubit_depths if name.startswith("$")), | |
| key=lambda name: int(name[1:]), | |
| ) |
The parentheses around the generator are required once key= is added. Every name reaching this point has already passed the reg_name[1:].isdigit() guard in _get_op_bits, so int() is safe.
| "Cannot consolidate qubit registers: the program mixes declared " | ||
| f"registers with physical qubits ({', '.join(physical_qubits)})", |
There was a problem hiding this comment.
Type: Maintenance
Severity: Low
Rationale: This is a new hard failure on a public flag: programs that unrolled successfully before now raise. The message names the physical qubits but does not tell the user how to proceed, and raise_qasm3_error is called without error_node/span here — no statement node is available at finalize time — so there is no line number either. The message text is the entire diagnostic.
Change Requested: Name the two ways out.
| "Cannot consolidate qubit registers: the program mixes declared " | |
| f"registers with physical qubits ({', '.join(physical_qubits)})", | |
| "Cannot consolidate qubit registers: the program mixes declared " | |
| f"registers with physical qubits ({', '.join(physical_qubits)}). " | |
| "Unroll without 'consolidate_qubits=True', or rewrite the physical " | |
| "qubits as operands of a declared register.", |
| # two consolidated slots plus physical $2, which sizes the count to its own index + 1. | ||
| # neither number is the declared qubit[5], which comes from device_qubits (see #353) | ||
| result.unroll() | ||
| assert result.num_qubits == 3 |
There was a problem hiding this comment.
Type: Implementation
Severity: Low
Rationale: The test_physical_qubits_are_not_consolidated this replaces asserted the full unrolled text, which is what pinned #344's rule that a physical qubit survives unrolling as written. This test asserts only the count, and num_qubits == 3 would still pass if cz $2, q[1] came out rewritten or dropped. test_physical_qubits_only still pins the text for the pure-physical case, so the gap is specifically the mixed shape.
Change Requested: Add the output assertion. Verified against this commit — check_unrolled_qasm and dumps are already imported in this file.
| assert result.num_qubits == 3 | |
| result.unroll() | |
| expected_qasm = """OPENQASM 3.0; | |
| include "stdgates.inc"; | |
| qubit[2] q; | |
| h q[0]; | |
| cz $2, q[1]; | |
| """ | |
| check_unrolled_qasm(dumps(result), expected_qasm) | |
| assert result.num_qubits == 3 |
3 participants
Fixes #353
Settles the semantics question from the issue as option 2: a program mixing declared registers with physical qubits under
unroll(consolidate_qubits=True)raises aValidationErrornaming the physical qubits, rather than emitting a consolidated register plus as-written$nreferences — two address spaces the output never relates. This preserves #344's "physical qubits are left as written" rule for pure-physical programs while refusing to emit an ambiguous program.Falling out of the same check: a program using only physical qubits no longer receives a
__PYQASM_QUBITS__declaration nothing references (previously sized bydevice_qubitsor the highest physical index).The two #344 tests that pinned the old counts (
test_physical_qubits_are_not_consolidated,test_physical_qubits_only) are deliberately edited, as the issue anticipated.Also in this PR:
Consolidated Qubit Validation Flow
Auto-enriched by Argus