fix: emit OpenQASM 2 syntax for classical conditionals - #338
Conversation
Failing tests for the qasm2 serializer emitting OpenQASM 3 syntax for
classical conditionals:
- braced if-blocks instead of `if (creg == int) <statement>`;
- after `unroll()`, the per-bit chain the unroller ravels a register
comparison into (`if (m[0] == true) { if (m[1] == false) { ... } }`),
which additionally uses creg indexing and a `true` literal that
OpenQASM 2 has no syntax for;
- a body writing to the register under test, which cannot be expanded
into one guarded statement per body statement without re-evaluating
the condition mid-body.
`Qasm2Module._qasm_ast_to_str` serialized with the stock openqasm3 printer and patched the result with regexes covering only declarations, so branching statements were emitted verbatim in OpenQASM 3 form under an `OPENQASM 2.0;` header. Downstream QASM 2 parsers reject the result. Adds `Qasm2Printer`, an `openqasm3.printer.Printer` subclass overriding `visit_BranchingStatement` to emit `if (creg == int) <statement>`: - a body that unrolled into several statements becomes one guarded statement each, QASM 2 having no braced blocks; - the nested per-bit chain `unroll()` ravels a register comparison into is walked back into that comparison, inverting the unroller's MSB-first bit ordering so `if(m==2)` round-trips as `if (m == 2)`; - branches QASM 2 cannot express — `else` blocks, nested `if`s, branches spanning registers, partial-register conditions (reachable via `>=` and `<=` unrolling) — raise a `ValidationError`. A multi-statement body is guarded per statement, which re-tests the register before each one. That is only faithful while the body leaves the register alone, so a body assigning to the register under test is rejected rather than silently given new semantics. Output verified against qiskit's OpenQASM 2 parser.
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! |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
🔎 Argus · 9/10 — Cleanly ships OpenQASM 2 classical conditional syntax with solid coverage
🔍 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: Emit valid OpenQASM 2 classical conditional syntax (if (creg == int) ) instead of OpenQASM 3 braced if-blocks.
Not in scope:
- Fix if(m==1) barrier q; input-validation gap (handled separately in #339)
Stated acceptance criteria (from PR/issue — not independently verified): - Branching emits brace-free if (creg == int) QASM 2 syntax
- Unrolled multibit nested per-bit chains collapse back to whole-register comparison
- Multi-statement bodies expand to one guarded conditional per statement
- else blocks, nested ifs, cross-register and partial-register conditions raise errors
- Multi-statement body writing the tested register raises ValidationError
- Emitted programs parse with qiskit.qasm2.loads
- Tests cover issue repro, body expansion, multibit unroll collapse, statement types, and write hazard
✅ Intent delivered
Verdict: This PR correctly emits brace-free if (creg == int) <statement> QASM 2 syntax, collapses multibit unrolls, expands multi-statement bodies, and guards the stated error cases. Ready to merge; the two warnings are minor polish.
🟡 2 P1 · 3 files reviewed
Architecture: Serialization and validation paths stay well separated; the write-hazard check and collapse logic keep semantic risk contained without overreaching scope.
Simulation Results
Tested 1 scenarios, 1 potential issues found:
Scenario: src/pyqasm/modules/qasm2.py: Conditional-body filtering blacklists only barrier instead of whitelisting the three statement kinds the QASM 2 grammar allows as an if body
Verdict: Broken (82% sure)
Why: If-body filter only bans barrier instead of allowing gate, measure, reset.
Fix: Whitelist only QuantumGate, Measure, and Reset as QASM2 if bodies.
Minor notes (1)
Low-confidence or minor observations — safe to ignore.
tests/qasm2/test_branching.py:L50[suggestion] Could we add one skip-guarded test that feeds the emitted program to qiskit.qasm2.loads, so the dialect contract is validated by a real QASM 2 parser and not only by string comparison?
2 findings · 2 inline · 0 folded
🔢 120.7k tokens · $0.9119 total
| Stage | Tokens | Cost |
|---|---|---|
| Intent | 4.7k | $0.0133 |
| Triage | 3.7k | $0.0087 |
| Lead agent | 1.9k | $0.0070 |
| Review · security | 63.7k | $0.5181 |
| Review | 31.1k | $0.3137 |
| Acceptance | 2.3k | $0.0087 |
| Simulation | 9.6k | $0.0322 |
| Scoring | 2.4k | $0.0066 |
| Synthesis | 1.3k | $0.0036 |
Contract: production/full · checked: bug_hunter, security, architecture, regression · review took 17m40s
Dashboard → · React 👎 to dismiss · Reply to any inline comment or use @argus-eye help to chat
Review feedback on #338. The chain walk in _flatten_branch collapses nested branches into one comparison, but nothing checks that the constraints it collects can coexist. Three shapes are silently reduced to whichever constraint is walked last, emitting a valid-looking QASM 2 program that triggers on different values than the source: if(m==1) if(m==0) x q[0]; -> if (m == 0) x q[0]; if(m[0]==1) if(m[0]==0) x q[0]; -> if (m == 0) x q[0]; if(m[0]==2) x q[0]; -> if (m == 1) x q[0]; The first drops the outer condition entirely, so the PR's claim that nested ifs raise is false for a chain of them. Also adds coverage for the four rejection paths the PR does introduce but never exercised -- else blocks, cross-register nesting, partial-register conditions and non-equality operators -- which passed already and now stay pinned.
Review feedback on #338. The chain walk collected each nested branch's constraint but never checked they could coexist, so three shapes were silently reduced to whichever constraint happened to be walked last and emitted as a valid-looking QASM 2 program triggering on different values: if(m==1) if(m==0) x q[0]; -> if (m == 0) x q[0]; if(m[0]==1) if(m[0]==0) x q[0]; -> if (m == 0) x q[0]; if(m[0]==2) x q[0]; -> if (m == 1) x q[0]; The first dropped the outer condition outright, so the guard that was supposed to reject nested ifs never fired for a chain of them -- the walk consumed the nesting before the check could see it. Each is now rejected where the constraint is folded in. The legitimate case the collapse exists for, a per-bit chain from unrolling, is unaffected: those carry distinct bit indices and boolean values. Constraint folding is extracted to _add_chain_constraint to keep _flatten_branch within the branch limit.
There was a problem hiding this comment.
Approved with comments — the core is correct, verified behaviourally rather than by reading. Neither finding is a correctness defect in what the printer emits; both are about where failures surface and how they are described.
Findings
All findings are posted inline on the relevant lines. Checklist:
- M1 — Every new rejection fires at
dumps(), never atvalidate(); #339 does the opposite - M2 —
if(m==1) measure q -> m;over-rejected; message blames the user for pyqasm's broadcast - L1 —
>=diagnostic never mentions the operator the user wrote - L2 — Printer depends on
openqasm3private internals under a<2.0.0pin
How it was tested
- Behavioural equivalence on Aer.
creg m[2]programs for every comparison value 0..3, running the original QASM 2 and the pyqasm-emitted QASM 2 across all four prepared states ofq[0], q[1]— 16/16 count distributions identical. The MSB-first inversion in_flatten_branchround-trips correctly. - Parser-validated. Every emitted program in a ~20-case matrix parses with
qiskit.qasm2.loads(..., custom_instructions=LEGACY_CUSTOM_INSTRUCTIONS): unrolled and non-unrolled repro, 2-bit register at every value, 3-bitm==5, broadcast body,resetbody,measure-to-other-register body, two independent cregs, a decomposedcrzbody expanding to 12 guarded lines,u1/u3parameterised bodies, mixed conditional + top-level statements. - Instrumenting
validate()/unroll()/dumps()separately across all five inexpressible shapes (see M1). if(m==1) barrier q;on this branch alone emitsif (m == 1) barrier q[0], q[1];, which qiskit rejects withQASM2ParseError.- Suite on branch: 653 passed, 4 skipped.
blackclean. Branch 1 commit behindmain(5dcae69). - Combined with #335/#339/#341/#344/#345: merges cleanly, 697 passed.
Next steps
Address M1 and M2 before merge — M2 in particular, since the non-unrolled path already emits the faithful output, so the fix is reachable rather than hypothetical. L1 falls out of M1 naturally; L2 is a one-line test extension.
Land #339 first, or merge the two together. The PR body is right that the conditional-barrier case is an input-validation gap owned by #339 — but the consequence is that #339 is a prerequisite for this PR's output being valid in all cases, not an independent follow-up.
Worth calling out: the test-first commit sequence (278f394 → 5b27fb1) is exactly right, and test_conflicting_chain_constraints_raise is the most valuable thing in the PR. Silently collapsing to the last-walked constraint would have produced valid-looking QASM 2 firing on different values than the source, with if(m==1) if(m==0) x q[0]; dropping the outer condition outright. That class of bug does not announce itself downstream.
| raw_qasm = dumps(qasm_ast, old_measurement=True) | ||
| return self._format_declarations(raw_qasm) | ||
| stream = io.StringIO() | ||
| Qasm2Printer(stream, old_measurement=True, creg_sizes=_creg_sizes(qasm_ast)).visit(qasm_ast) |
There was a problem hiding this comment.
[M1] Every new rejection fires at dumps(), never at validate() — Maintenance · Medium
Rationale: instrumenting the three stages separately on this branch gives an identical result for all five inexpressible shapes:
else validate=OK unroll=OK dumps=RAISE(ValidationError)
cross-register validate=OK unroll=OK dumps=RAISE(ValidationError)
>= operator validate=OK unroll=OK dumps=RAISE(ValidationError)
writes-register validate=OK unroll=OK dumps=RAISE(ValidationError)
nested same-reg validate=OK unroll=OK dumps=RAISE(ValidationError)
So Qasm2Module.validate() reports these programs as valid and then serialization throws. That is the opposite of the convention the sibling PR #339 establishes for this same file: #339 puts its <qop> body check in _filter_statements, so module.validate() rejects. Two PRs landing in the same week, both rejecting QASM 2 conditional shapes, disagreeing on which call raises, is a maintenance liability more than a bug.
It also matters for the stated impact path — the qBraid SDK normalization pass validates and then serializes, so a caller that has already been told the program is fine gets a surprise at the last step, with no opportunity to fall back.
Change requested: hoist the structural checks that do not depend on printing — else, nested if, non-== operator, cross-register nesting — into validation alongside #339's, leaving Qasm2Printer to do only the collapse and the emission. The per-bit chain collapse legitimately belongs here; the "this shape has no QASM 2 form" judgements do not. If that restructuring is too much for this PR, at minimum document on Qasm2Module.dumps() / _qasm_ast_to_str that it can raise ValidationError, so the contract is written down.
There was a problem hiding this comment.
Resolved in 2c153ea — structural checks hoisted into _filter_statements, matching #339's convention.
validate() now rejects else, nested if, non-== operators, and any condition that isn't a whole register against an integer literal. Confirmed on all five shapes:
else validate=RAISE
cross-register validate=RAISE
>= operator validate=RAISE (names '>=')
bit-indexed validate=RAISE
nested same-reg validate=RAISE
Kept the printer checks rather than moving them wholesale: dumps() on a module that was never validated goes straight to _qasm_ast_to_str, so the printer still needs a backstop for anything reaching it — and the per-bit chain collapse has to stay there regardless. _qasm_ast_to_str now documents that it can raise ValidationError, so the contract is written down either way.
The write-hazard check stays at serialization since it depends on the post-unroll body; M2 removed its main trigger.
|
|
||
| # a multi-statement body is emitted as one guarded statement each, which re-tests the | ||
| # register before every statement; that is only faithful while the body leaves it alone | ||
| if len(body) > 1 and any(_writes_to_register(stmt, reg_name) for stmt in body): |
There was a problem hiding this comment.
[M2] if(m==1) measure q -> m; is over-rejected, and the message blames the user for pyqasm's broadcast — Implementation · Medium
Rationale: this is a single source statement and it is valid OpenQASM 2 — <qop> := measure <argument> -> <argument> ; accepts whole-register arguments. qiskit.qasm2.loads accepts if(m==1) measure q -> m; (qiskit expands it to per-bit conditionals itself, incurring exactly the hazard this PR warns about).
The current behaviour on identical source depends on whether unroll() ran:
dumps(loads(src)) # -> if (m == 1) measure q -> m; (emitted, faithful)
m = loads(src); m.unroll(); dumps(m) # -> ValidationErrorThe rejection reports:
Branch on 'm' has a body that writes to 'm' itself. QASM 2.0 guards a single statement per 'if', so the multi-statement body cannot be emitted without re-evaluating the condition mid-body and changing the program's meaning
The user wrote one statement. The multiplicity is pyqasm's own measurement broadcast, which the error attributes to them. And the non-unrolled path already proves the faithful output is expressible — the printer emits it correctly today.
Change requested: preferably (a) preserve the register-level form: when the body came from a single register-level measurement, emit if (m == 1) measure q -> m; unchanged rather than rejecting. That is both expressible and semantically exact, and it removes the unroll-dependent asymmetry. Failing that, (b) at minimum reword the diagnostic to describe what actually happened — that unrolling expanded a single register-level measurement into one statement per bit, and the guard cannot be replicated per statement without re-testing a register the body is writing. The conservative rejection is defensible; attributing a "multi-statement body" to the author is not.
There was a problem hiding this comment.
Resolved in 2c153ea — went with (a), preserving the register-level form.
_collapse_broadcast_measurement reassembles measure qreg -> creg when the body is exactly a full, in-order broadcast over both registers. The asymmetry is gone:
dumps(loads(src)) -> if (m == 1) measure q -> m;
m = loads(src); m.unroll(); dumps(m) -> if (m == 1) measure q -> m;
Both parse with qiskit.qasm2.loads. test_branch_on_register_level_measurement_round_trips asserts the two paths agree.
The hazard check still fires for a genuinely multi-statement body writing the tested register (e.g. { x q[0]; measure q[0] -> m[0]; }), and its message now names the statement count and says the register is the one the branch tests, rather than attributing a "multi-statement body" to the author.
| if set(bit_values) != set(range(size)): | ||
| missing = sorted(set(range(size)) - set(bit_values)) | ||
| raise ValidationError( | ||
| f"Branch on '{reg_name}' constrains only bits {sorted(bit_values)} of a " |
There was a problem hiding this comment.
[L1] The >= diagnostic never mentions the operator — Implementation · Low
Rationale: if(m>=1) x q[0]; on a creg m[2] reaches this branch and reports:
Branch on 'm' constrains only bits [1] of a 2-bit register (bits [0] are unconstrained)
The rejection is correct, but the user wrote >= and the message talks about bit coverage of a chain they never authored. _parse_branch_condition's non-== message is the one that fits, and it only fires when the operator survives to the printer un-unrolled.
Change requested: detect the non-equality operator at the source level — which falls out naturally if the structural checks move into validation per M1 — so the error names the operator the user actually wrote.
There was a problem hiding this comment.
Resolved in 2c153ea — falls out of M1 as you predicted.
if(m>=1) x q[0]; now reports at validate():
Branch condition 'm >= 1', which uses '>=', is not supported in QASM 2.0, which only allows 'if (creg == int)'
unroll() runs _filter_statements too, so the reviewer's path (which required unrolling) gets the operator-naming message rather than the bit-coverage one. Covered by test_inexpressible_branches_rejected_by_validate with >= and < cases asserting the operator appears.
| # a single QASM 2 conditional guards a single statement, so a body that | ||
| # unrolled into several statements becomes one guarded statement each | ||
| for statement in body: | ||
| self._start_line(context) |
There was a problem hiding this comment.
[L2] Printer depends on openqasm3 private internals — Maintenance · Low
Rationale: Qasm2Printer reaches into private printer internals — self._start_line here and PrinterState.skip_next_indent just below — under a >=1.0.0,<2.0.0 pin (pyproject.toml). A 1.x minor bump could change emission silently rather than raising, which is the bad failure mode for a serializer.
Change requested: cheap mitigation rather than a redesign — extend the existing assert "{" not in unrolled from test_branch_body_statement_types to the other emission tests, so a change in upstream indentation behaviour surfaces as a test failure instead of malformed output. A comment noting the coupling would also help.
There was a problem hiding this comment.
Resolved in 2c153ea — brace assertions extended, coupling noted.
assert "{" not in unrolled and "}" not in unrolled now runs in test_branch_emits_qasm2_syntax, test_branch_body_expands_to_one_conditional_per_statement, and test_multibit_branch_survives_unrolling as well, so an upstream indentation change surfaces as a test failure. Added a comment at the _start_line / skip_next_indent call site naming them as openqasm3 internals and pointing at those assertions.
# Conflicts: # CHANGELOG.md
…surement - Qasm2Module._filter_statements now rejects 'else', nested 'if' and non 'creg == int' conditions, so validate() reports them instead of leaving serialization to raise on a program already reported as valid (M1) - the diagnostic names the operator the user wrote, which is only possible before unrolling ravels it into a chain of equality tests (L1) - a branch guarding 'measure qreg -> creg' is reassembled from the per-bit statements unrolling expanded it into, so it is emitted as written instead of rejected; removes the unroll-dependent asymmetry (M2) - _qasm_ast_to_str documents that it can raise ValidationError, and the coupling to openqasm3 printer internals is noted where it is used, with brace assertions extended across the emission tests (L2)
- annotate 'body' before the chain walk so the collapse assignment is not flagged used-before-def - widen _single_index to accept the Optional operand types on QuantumMeasurementStatement
Resolve the qasm2.py conflict by keeping both conditional checks: #339's _filter_branch_body (body <qop> whitelist, now on main) is called before this PR's _filter_branch (branch shape), so the already-merged error messages for barrier/delay/box and global-phase bodies are preserved. CHANGELOG: both entries kept.
TheGupta2012
left a comment
There was a problem hiding this comment.
Conflict resolution holds up; one High opened against _filter_branch that predates the merge. The union at 4f1ca58 is clean: _filter_branch_body is byte-identical to its version on main (#339), _filter_branch is byte-identical to its version at 08957a0, and the only new line is the _filter_branch_body(stmt) call placed ahead of _filter_branch(stmt). Both blocks are correctly closed -- the first raise_qasm3_error(...) now terminates on its own rather than borrowing the trailing paren the two sides shared in the conflict. Nothing was dropped from either CHANGELOG side. All four findings from the 2026-08-05 round are addressed at this head, and #339 landing first makes the two PRs compose exactly as the description predicted.
What the extra scrutiny turned up is separate from the merge: _filter_branch is applied to already-unrolled ASTs as well as to source, and there it rejects programs that Qasm2Printer -- in the same file -- serializes correctly.
Conflict of interest. Commit
4f1ca58was authored and pushed from the reviewer side of this PR, not by @ryanhill1. Keeping both checks, the ordering between them, and the CHANGELOG union are all review-side decisions that nobody else has looked at. They need the author's sign-off before merge.
Findings
Detail is inline on the relevant lines.
- H1 --
_filter_branchrejects the per-bit chainunroll()produces, so a valid conditional fails once any transformation pass has written the unrolled AST back to_statements-- Implementation - High - M3 -- Merge-resolution note: the
_filter_branch_body->_filter_branchordering is load-bearing and reviewer-authored; it also masks one instance of H1 -- Maintenance - Medium
Carried forward from 2026-08-05, all confirmed fixed at 4f1ca58:
| check | result at this head | |
|---|---|---|
| M1 | structural rejections reach validate(), not only dumps() |
all six shapes in test_inexpressible_branches_rejected_by_validate raise from validate(); if(m>=1), if(m[0]==1), else and nested-if confirmed by hand |
| M2 | if(m==1) measure q -> m; emitted as written |
dumps(loads(src)) == dumps(unrolled_module) is True; both give if (m == 1) measure q -> m;, which qiskit accepts |
| L1 | the diagnostic names the operator | >=, >, <, != each produce which uses '<op>' |
| L2 | brace assertions guard the openqasm3 internals |
assert "{" not in unrolled present in four emission tests |
How it was verified
- Merge audit.
_filter_branch_body,_filter_branchand_filter_statementsextracted by AST from7e05f4c,08957a0and4f1ca58and compared: the first two byte-identical to their source side, the third differing by exactly one added line.git diff 7e05f4c 4f1ca58touches onlyCHANGELOG.md,src/pyqasm/modules/qasm2.pyandtests/qasm2/test_branching.py. - Suite at
4f1ca58: 707 passed, 4 skipped (excludingtests/cli, which fails identically onmainin this sandbox -- environment artifact, not this branch).black --checkclean on both changed files. - Three-way behaviour matrix. Twenty public-API sequences on
creg m[1]andcreg m[2]conditionals, run against7e05f4c(main),08957a0(pre-merge PR head) and4f1ca58. This is what surfaced H1; the per-row results are inline. - Parser-validated, qiskit 2.4.1. Eight emitted programs through
qasm2.loads(..., custom_instructions=LEGACY_CUSTOM_INSTRUCTIONS), all parsing: issue repro unrolled and not, 2-bit register atm==2, broadcast body,resetbody, register-levelmeasurebody, acrzbody expanding to guarded lines, and -- new at this head -- the output ofreverse_qubit_order(), which #345 now rewrites inside theifbody and which this PR serializes brace-free. On08957a0that same case emittedif (m == 1) x q[1];with the operand unrewritten, so the two PRs compose into a real improvement. - #339 composition.
if(m==1) barrier q;now raises invalidate()rather than emittingif (m == 1) barrier q[0], q[1];, which qiskit rejected when this branch stood alone. - Security (Level 3): nothing found. No new file or network access, no deserialization of untrusted formats, no new dependency. Error strings embed source expressions through the existing
openqasm3printer and surface only as Python exception messages.
Next steps
- H1 before merge. It is a regression against
mainin substance. This is posted as a comment rather than request-changes so the merge call stays with the maintainer handling the PR. - @ryanhill1: please verify the merge resolution in
4f1ca58yourself. It was authored review-side, not by you -- specifically the decision to keep both_filter_branch_bodyand_filter_branch, the ordering between them, and the CHANGELOG union. Re-running the Aer equivalence check on this head is worth the few minutes, since none of it carries your review. - CI is green on
4f1ca58-- 26/26 checks.[no ci]was deliberately not used: the resolution touchedsrc/pyqasm/modules/qasm2.py, not only the CHANGELOG. - Already tracked, referenced rather than re-raised: #350, #351, #352, #353, #354. H1 rides the same re-filter mechanism #351 documents, but it is a distinct defect -- #351 concerns the diagnostic for something genuinely unrepresentable, H1 is a hard rejection of valid input.
| ) | ||
|
|
||
| def _filter_branch(self, statement: qasm3_ast.BranchingStatement) -> None: | ||
| """Reject the conditional shapes QASM 2.0 has no syntax for, before unrolling. |
There was a problem hiding this comment.
[H1] _filter_branch rejects the per-bit chain unroll() produces, so valid conditionals fail after a transformation pass -- Implementation - High
Rationale: this docstring's premise -- before unrolling -- does not hold. _filter_statements() runs over self._statements, and four base-class passes reassign _statements to the unrolled AST: remove_measurements (modules/base.py:272), remove_barriers (:324), remove_idle_qubits (:551) and reverse_qubit_order (:613). Any later validate() or unroll() therefore re-filters unrolled statements. That mechanism is already documented in #351.
On the unrolled AST a conditional is exactly the shape this method rejects:
if (m[0] == true) {
if (m[1] == false) {
x q[2];
}
}
So both the nested-if check below and the Identifier == IntegerLiteral condition check fire on programs that are valid QASM 2 -- and that Qasm2Printer two hundred lines above serializes correctly, because _flatten_branch collapses that chain instead of rejecting it.
The issue-#337 repro itself:
src = '''OPENQASM 2.0;
include "qelib1.inc";
qreg q[2];
creg m[1];
h q[0];
measure q[0] -> m[0];
if(m==1) x q[1];
'''
m = loads(src)
m.remove_idle_qubits()
m.validate()
# ValidationError: Branch condition 'm[0] == true' is not supported in QASM 2.0,
# which only allows 'if (creg == int)'Sequences run on that program and on its creg m[2] / if(m==2) analogue, at 7e05f4c vs 4f1ca58:
| sequence | main |
this head |
|---|---|---|
validate() / unroll() -> dumps() |
ok | ok |
remove_idle_qubits() |
ok | ok |
reverse_qubit_order() |
ok | ok |
remove_idle_qubits() -> validate() |
ok | raises |
remove_idle_qubits() -> remove_idle_qubits() |
ok | raises |
remove_idle_qubits() -> reverse_qubit_order() |
ok | raises |
reverse_qubit_order() -> remove_idle_qubits() |
ok | raises |
unroll() -> remove_measurements() -> unroll() |
ok | raises |
unroll() -> remove_measurements() -> validate() |
ok | raises |
The 2-bit variant reports Nested 'if' statements are not supported; the 1-bit variant reports the condition-shape message above. main emits invalid QASM 2 for those rows, which is the bug this PR exists to fix -- the point is that dumps() on the very same unrolled AST already emits the correct program on this branch. The printer is not the limitation; the validator is stricter than the serializer it is meant to guard.
Not introduced by the merge -- identical results at 08957a0, so this is a gap in the earlier round's coverage, which exercised validate / unroll / dumps but not transformation-pass sequences. Also worth stating for scope: no such sequence appears in the qbraid SDK paths that consume Qasm2Module (transpiler/conversions/qasm2/qasm2_to_cirq.py:60, interface/circuit_equality.py:133 are both single-pass), so the blast radius today is pyqasm's own public API rather than a live SDK regression.
Second symptom of the same split. _filter_branch and _flatten_branch are two implementations of one rule and they disagree on identical source. This PR's tests pin both answers, in test_inexpressible_branches_rejected_by_validate and test_inexpressible_branches_raise respectively:
source (creg m[2]) |
validate() says |
dumps() says |
|---|---|---|
if(m==1) if(m==0) x q[0]; |
Nested 'if' statements are not supported | Branch on 'm' nests another whole-register comparison |
if(m[0]==1) x q[0]; |
only allows 'if (creg == int)' | constrains only bits [0] ... bits [1] are unconstrained |
Change requested -- two directions, the first preferred:
- Unify. Have
_filter_branchdelegate to_flatten_branch, building the size maps fromself._statementsvia the existing_creg_sizes/_qreg_sizes. One implementation then defines what QASM 2 can express, used at bothvalidate()anddumps(): the unrolled chain collapses instead of being rejected, and the message divergence above disappears. Cost is honest and small -- two expectations intest_inexpressible_branches_rejected_by_validatechange to_flatten_branch's wording (if(m==1) if(m==0)andif(m[0]==1));elseand the>=operator naming, which were the point of M1/L1, are unaffected because_flatten_branchalready produces them. - Narrow. Skip
_filter_branchwhenself._statements is self._unrolled_ast.statements. Two lines and exact today, but it couples correctness to an object-identity relation the base class merely happens to establish; the day a pass copies the list instead of aliasing it, the guard silently stops applying. Acceptable as a stopgap, not as the resting state.
Either way, please add a regression test in tests/qasm2/test_branching.py covering remove_idle_qubits() -> validate() and reverse_qubit_order() -> remove_idle_qubits() on a valid conditional. test_conditional_global_phase_reports_global_phase is currently the only test that touches the re-filter path, and it only covers a shape that should be rejected -- which is why nothing caught this.
| raise ValidationError(f"Statement of type {stmt_type} not supported in QASM 2.0") | ||
| if isinstance(stmt, qasm3_ast.BranchingStatement): | ||
| self._filter_branch_body(stmt) | ||
| self._filter_branch(stmt) |
There was a problem hiding this comment.
[M3] Merge resolution: this ordering is reviewer-authored and load-bearing -- Maintenance - Medium
Conflict of interest first: this line, and the _filter_branch_body(stmt) call above it, come from a reviewer-side conflict resolution (4f1ca58), not from @ryanhill1. Both sides of the merge hooked this same anchor and each defined a method immediately after it, so git could not merge them. The resolution keeps both and calls #339's body whitelist first. None of that carries independent review -- hence this note.
Why body-first, stated so it can be challenged:
- Two cases already on
mainonly pass in this order.test_conditional_barrier_rejected_when_nested(if(m==1) if(m==0) barrier q;must reportbarrier) andtest_conditional_global_phase_reports_global_phaseboth break under shape-first, because this PR's nested-ifand condition-shape rejections would fire before the body walk reaches the barrier or theQuantumPhase. - Stronger than test preservation, and the reason to keep the order for now: on the re-filter path described in H1,
_filter_branch_bodyrunning first is exactly what makes therzz/rxxcase report the tracked gphase emitted for QASM 2 modules: rzz/rxx unroll to a global phase QASM 2 cannot express #351 diagnostic instead of_filter_branch's spurious'm[0] == true' is not supported. The ordering is currently masking one instance of H1. It should not be revisited until H1 is fixed, or that case regresses into a misleading error.
Where the ordering reads slightly wrong, noted rather than requested: _filter_branch_body iterates else_block while _filter_branch rejects any else at all, so if(m==1) x q[0]; else barrier q; reports the barrier. Fixing the barrier then surfaces the else -- two round trips for a program with two independent problems, and the else is the more fundamental of the two. Not worth changing on its own; worth folding into whichever unification H1 leads to.
Change requested: confirm the union and this ordering are what you want, since they are unreviewed review-side edits to your branch. No change to the ordering itself is requested while H1 stands. Also worth a second pair of eyes: the CHANGELOG merge kept both entries and nothing was dropped from either side, verified by diffing each side's block against the result -- but that too was review-side.
Summary of changes
Closes #337.
Qasm2Module._qasm_ast_to_strserialized with the stockopenqasm3printer and then patched the result with regexes that only cover declarations (qubit[n] q→qreg q[n]). Branching statements went through the OpenQASM 3 printer untouched, so a valid QASM 2 conditional round-tripped as a braced block under anOPENQASM 2.0;header:Confirmed rejected by a real QASM 2 parser:
The unrolled path was worse
The issue's repro does not call
unroll(). Once it does,unroll()ravels a register comparison into a nested chain of per-bit tests:That is invalid QASM 2 three times over — braces, creg indexing in the condition, and a
trueliteral. Since the reported impact path is the qBraid SDK normalization pass, which unrolls, fixing only the braces would have left it broken.Fix
Qasm2Printer, anopenqasm3.printer.Printersubclass overridingvisit_BranchingStatementto emit QASM 2'sif (creg == int) <statement>:if(m==2)round-trips asif (m == 2).if(m==1) measure q -> m;is one QASM 2 statement, but unrolling expands it into one statement per bit. The broadcast is reassembled so the branch is emitted as written, rather than as a per-bit guard chain that would re-test the register the body writes.Rejection happens at validation
The branch shapes QASM 2 has no syntax for are properties of the source program, so
Qasm2Module._filter_statementsrejects them andvalidate()reports them:elseblocks, nestedifs, conditions comparing anything other than a whole register against an integer, and non-equality operators. Rejecting at validation is what lets the diagnostic name the operator the user actually wrote — after unrolling,m >= 1is a chain of equality tests and the>=is gone. This matches the convention #339 establishes in the same file.Qasm2Printerre-checks what survives into the unrolled AST, since a caller may serialize a module it never validated or one whose AST it built directly._qasm_ast_to_strdocuments that it can raise.The chain walk also has to reject constraints that cannot coexist in the single comparison QASM 2 allows. Without that, three shapes collapsed to whichever constraint was walked last and emitted a valid-looking program triggering on different values than the source —
if(m==1) if(m==0) x q[0];becameif (m == 0) x q[0];, dropping the outer condition outright. Caught in review; see 65984ac and 14858df.One semantic hazard the per-statement expansion introduces is guarded rather than shipped: guarding each statement re-tests the register before every one, which is only faithful while the body leaves that register alone. A genuinely multi-statement body assigning to the register under test raises a
ValidationErrornaming the register and the statement count.Tests
Written first and committed failing (278f394), then made to pass by the fix (5b27fb1).
test_branch_emits_qasm2_syntax— the issue repro, checked against the full expected program.test_branch_body_expands_to_one_conditional_per_statement— a broadcast body becomes one guarded statement per qubit.test_multibit_branch_survives_unrolling— parametrized over every value of a 2-bit register, asserting the per-bit chain collapses back to the original comparison.test_branch_body_statement_types— gate,reset, andmeasurebodies each emit brace-free.test_branch_on_register_level_measurement_round_trips—if(m==1) measure q -> m;emits identically whether or notunroll()ran.test_branch_body_writing_tested_register_raises— the semantic hazard above is rejected.test_conflicting_chain_constraints_raise— the three shapes that previously collapsed to the wrong condition.test_inexpressible_branches_raise/test_inexpressible_branches_rejected_by_validate— one case per rejection path, at serialization and at validation respectively.Every emitted program in the cases above was additionally verified to parse with
qiskit.qasm2.loads.Related bug, not fixed here
if(m==1) barrier q;is accepted on input but is not valid QASM 2 —barrieris not a<qop>, so it cannot be a conditional body. That is an input-validation gap rather than a serializer gap, so it is fixed separately in #339. Since both PRs add conditional checks to_filter_statements, land #339 first or merge the two together.