fix: reject barrier as an OpenQASM 2 conditional body

[ryanhill1]

Summary of changes

Found while fixing #337 (see #338). Independent of that PR — this one is about what gets accepted, not how it is printed.

The OpenQASM 2 grammar admits only a <qop> as the body of an if:

<if>  := if ( <id> == <nninteger> ) <qop>
<qop> := <uop> | measure <argument> -> <argument> ; | reset <argument> ;

barrier is a separate production, so if(m==1) barrier q; is not a valid QASM 2 program — and delay/box have no QASM 2 syntax at all, yet both are accepted in a conditional body too (delay is even rejected by the existing top-level whitelist, so a conditional was the one place it slipped through). Qasm2Module._filter_statements checked only the type of each top-level statement, so nothing ever inspected what a BranchingStatement carried in its body — the conditional barrier validated cleanly and was serialized verbatim:

if (m == 1) barrier q[0], q[1];

which downstream parsers reject:

qiskit.qasm2.QASM2ParseError: '<input>:5,12: needed a gate application, measurement or reset, but instead saw barrier'

So the invalid construct entered unflagged and left as invalid output.

Fix

_filter_statements now recurses into conditional bodies — both if_block and else_block, at any nesting depth — and checks each statement against the <qop> production itself (QuantumGate, QuantumMeasurementStatement, QuantumReset) rather than blacklisting barrier alone, which would let through whatever kinds it had not enumerated:

ValidationError: 'barrier' is not supported as the body of an 'if' in QASM 2.0,
which allows only a gate, measurement or reset there

reported through raise_qasm3_error so the message carries the source span and the offending statement:

Error at line 6, column 9 in QASM file

 >>>>>> barrier q;

Statements are named by the keyword the user wrote — barrier, delay, box — rather than by the AST class they parsed into.

This is a behavior change: programs that previously validated now raise. That is the point — they were being turned into invalid QASM 2, and failing at validation with a located error beats failing in a downstream parser with a syntax error.

Global phase

rzz/rxx decompose to a body containing a QuantumPhase, which QASM 2 has no syntax for at all. That gap is pre-existing and tracked in #351 — the unroller emits gphase(...) for a Qasm2Module whether or not a conditional is involved, so fixing it belongs there, not here. What this PR does add is an honest message for it: the QuantumPhase case names global phase and the rzz/rxx decomposition that introduces it, rather than reporting statement of type QuantumPhase for a program the user wrote as rzz.

Tests

Written first and committed failing (ddb79e4), then made to pass by the fix (3ca979d).

• test_conditional_barrier_rejected — parametrized over broadcast, single-qubit, and multi-qubit barrier forms.
• test_conditional_barrier_rejected_when_nested — a barrier reached only through a nested conditional, verified to fail when the recursion is removed.
• test_conditional_non_qop_rejected — delay and box bodies, the wider hole behind the barrier case, asserting each is named by its keyword.
• test_conditional_global_phase_reports_global_phase — the rzz case from gphase emitted for QASM 2 modules: rzz/rxx unroll to a global phase QASM 2 cannot express #351 reports global phase rather than an AST class name.
• test_conditional_qop_accepted — every operation that is a <qop> (gate, two-qubit gate, reset, measure) still validates, guarding against over-rejection.
• test_unconditional_barrier_accepted — barriers outside conditionals are untouched.

The last two passed before the fix as well, which is what makes them useful: they pin the boundary the fix must not cross.

Merge order

Land this before #338. #338's printer emits QASM 2 output for exactly the construct this PR rejects, so merging in that order means the invalid case never reaches the new printer. Both PRs add conditional checks to _filter_statements, so expect a small conflict there.

[argus-eye]

Argus review

Auto-review is off for this repo. Tick the box below to run a review on this PR.

Running Argus review...

Estimated cost

• Files changed: 3
• Diff lines (±): 92

Tip: you can also comment @argus-eye review at any time.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 2f49bf8f-c69e-4409-8ace-625868471f7c

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[codecov-commenter]

Codecov Report

❌ Patch coverage is 94.11765% with 1 line in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/pyqasm/modules/qasm2.py	94.11%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

[argus-eye]

🔎 Argus · 9/10 — Cleanly rejects barrier in OpenQASM 2 conditionals with solid nested 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: Reject barrier as an OpenQASM 2 conditional body, since the grammar allows only a qop (gate, measure, or reset) there.
Not in scope:

• Independent of #338 (printing/serialization of conditionals)
  Stated acceptance criteria (from PR/issue — not independently verified):
• Recurse into conditional if_block and else_block at any nesting depth and reject barrier there
• Raise ValidationError via raise_qasm3_error with source span for conditional barrier
• Accept conditional qops: gate, two-qubit gate, reset, measure
• Accept unconditional barriers outside conditionals
• Tests: test_conditional_barrier_rejected, test_conditional_barrier_rejected_when_nested, test_conditional_qop_accepted, test_unconditional_barrier_accepted

✅ Intent delivered

Verdict: This PR correctly enforces the qop-only grammar for OpenQASM 2 conditional bodies, including nested if/else, with clear ValidationError spans and good acceptance tests. Ready to merge; the two warnings are minor polish.

🟡 2 P1 · 3 files reviewed

Architecture: Validation is scoped tightly to conditional bodies without disturbing unconditional barriers or legitimate qops, which keeps the change easy to reason about.

2 findings · 2 inline · 0 folded

_{🔢 186.1k tokens · $1.2897 total}

Stage	Tokens	Cost
Intent	4.1k	$0.0126
Triage	3.3k	$0.0087
Lead agent	1.8k	$0.0063
Review · bug_hunter	31.2k	$0.1673
Review · security	32.8k	$0.1885
Review · architecture	31.7k	$0.2841
Review · regression	58.9k	$0.4644
Review	18.7k	$0.1467
Scoring	2.4k	$0.0072
Synthesis	1.4k	$0.0039

---

<sub>Contract: production/full · checked: bug_hunter, security, architecture, regression · review took 14m4s

Dashboard → · React 👎 to dismiss · Reply to any inline comment or use @argus-eye help to chat</sub>

[ryanhill1]

@TheGupta2012 flagging this one for a deliberate look before merge — it is a breaking behaviour change, and the scope grew during review, so I would rather you sign off on it than have it slip through as a routine bug fix.

What breaks

Programs that validate today will start raising ValidationError:

if(m==1) barrier q;         // was: emitted as-is
if(m==1) delay[10ns] q;     // was: emitted as-is
if(m==1) box { x q[0]; }    // was: emitted as-is

Why I think it is right

None of these are expressible in OpenQASM 2. The grammar admits only a <qop> as the body of an if:

<if>  := if ( <id> == <nninteger> ) <qop>
<qop> := <uop> | measure <argument> -> <argument> ; | reset <argument> ;

barrier is a separate production; delay and box have no QASM 2 syntax at all. So today these are accepted on input and emitted as output that QASM 2 parsers reject:

qiskit.qasm2.QASM2ParseError: '<input>:5,12: needed a gate application,
measurement or reset, but instead saw barrier'

The program was already broken — this only moves the failure from a downstream parser's syntax error to a validation error with a source span and the offending statement named. delay is the sharpest illustration: DelayInstruction is already rejected by the top-level whitelist in _filter_statements, so a conditional body was the one place it could slip through the module unflagged. Rejecting it there is consistency, not new policy.

The scope growth you should be aware of

The PR originally rejected barrier only. Argus pointed out that a blacklist lets through whatever it has not enumerated, and it was right — I found delay and box doing exactly that. So I switched to whitelisting the <qop> production itself (785b20f). That is the more correct fix, but it widens the blast radius from one statement kind to "anything that is not a gate, measurement or reset", which is a bigger call than the PR title suggests.

If you would rather not break users

Two smaller options, happy to switch to either:

1. Warn instead of raise — log a warning and keep emitting, so nothing breaks but the problem is visible.
2. Narrow it back to barrier — the originally scoped fix, leaving the delay/box holes open for a follow-up.

My recommendation is to keep it as-is: silently emitting invalid QASM 2 is the worst of the three, since the failure surfaces far from its cause. But this is a maintainer call, not mine.

The sibling PR #338 (the #337 serializer fix) has no such concern — it only changes how conditionals are printed, and every program that validated before still validates.

[TheGupta2012]

Approved with comments — tightly scoped, correctly placed, well tested. Ready to merge; the one Medium is a pre-existing gap this PR brings into focus, not a defect in the diff.

Findings

All findings are posted inline on the relevant lines. Checklist:

• M1 — QuantumPhase reaches conditional bodies via rzz/rxx; whitelist does not cover it
• L1 — _qop_statements is instance state for a constant
• L2 — Non-barrier cases report an AST class name, not a QASM keyword

How it was tested

• Rejection stage. Instrumented: loads = OK, then RAISE ValidationError at validate(), before any unrolling — accept() calls _filter_statements() before visit_basic_block.
• Coverage. Broadcast barrier q;, single barrier q[0];, a barrier reached only through a nested if, and a barrier in an else block all raise with the located message.
• Over-rejection guard. Single- and two-qubit gate bodies, reset, measure into another register, and gates with multi-statement decompositions (crz → 12 statements, u3 → 5 rotations) all still validate, unroll and serialize. Top-level barriers untouched.
• Whitelist sweep. ~45 qelib1/pyqasm gate shapes as conditional bodies, inspecting unrolled if_block node kinds — every gate but rzz/rxx produced only QuantumGate.
• Reachable public-API sequences diffed against main to separate the M1 regression from pre-existing failures.
• Suite on branch: 647 passed, 4 skipped (excluding tests/cli, which fails identically on main in this environment). Branch 1 commit behind main; merges cleanly with #335/#338/#341/#344/#345 (CHANGELOG only), combined 697 passed.

Next steps

Merge when ready — none of the three blocks. File the follow-up requested in M1 and reference it here; L1 and L2 are small and worth folding in now while the file is open. L2 is attached as an applicable suggestion.

Land this before #338. #338's printer emits QASM 2 output for exactly the construct this PR rejects, so merging in that order means the invalid case never reaches the new printer.

The failing-test-first commit ordering (ddb79e4 → 3ca979d) and the follow-up that made the nested test actually nest are both good discipline. test_conditional_qop_accepted and test_unconditional_barrier_accepted passing before the fix is what makes them valuable — they pin the boundary the fix must not cross.

[TheGupta2012]

[M1] QuantumPhase can reach a conditional body — this whitelist does not cover it — Implementation · Medium (pre-existing gap; not a blocker)

Rationale: the whitelist is the right shape, but the <qop> set is not quite the set of things that actually arrive here. Two legal qelib1 gates decompose to a body containing a QuantumPhase node:

if(m==1) rzz(0.3) q[0],q[1];
if(m==1) rxx(0.3) q[0],q[1];

Sweeping ~45 qelib1/pyqasm gate shapes as conditional bodies and inspecting the unrolled if_block node kinds: every other gate produced only QuantumGate, but these two produce ['QuantumPhase', 'QuantumGate', ...]. Two consequences on this branch:

1. The output-level hole this PR closes for barrier is still open here. After unroll(), dumps() emits:

if (m[0] == true) {
  gphase(-0.15) q[0], q[1];
  cx q[0], q[1];
  ...
}

gphase has no QASM 2 syntax at all — the same accept-then-emit-invalid pattern this PR is fixing, arriving from the unroller rather than from the source program. (The m[0] == true half of that line is #338's territory.)

2. A narrow regression. _filter_statements runs over self._statements, and remove_idle_qubits() / reverse_qubit_order() both reassign _statements = _unrolled_ast.statements. Since unroll() has no re-entry guard, a second pass filters the unrolled body and raises:

ValidationError: statement of type QuantumPhase is not supported as the body of an 'if'
in QASM 2.0, which allows only a gate, measurement or reset

naming an AST class for a program the user wrote as rzz. Diffing the reachable public-API sequences against main: reverse_qubit_order() then remove_idle_qubits() passes on main and raises here. The other sequences (remove_idle_qubits() then unroll(), etc.) already fail on main for an unrelated pre-existing reason (Index 1 out of range for register of size 1), and remove_idle_qubits() then dumps() is fine on both.

Attribution: the underlying gap is pre-existing, not introduced here. QuantumPhase is absent from _whitelist_statements too, so unconditional rzz(0.3) q[0],q[1]; followed by remove_idle_qubits() + unroll() already raises Statement of type <class 'openqasm3.ast.QuantumPhase'> not supported in QASM 2.0 on main. This PR extends the same gap into conditional bodies, which is consistent — it just widens where it surfaces by one construct.

Change requested: not a blocker, and not worth growing the diff for. Please file a follow-up for QASM 2 global-phase handling — either the unroller should not emit gphase for a Qasm2Module, or _qasm_ast_to_str should fold/drop it — and reference it here. As a cheap in-PR mitigation, giving QuantumPhase its own message (global phase is not representable in QASM 2, rather than statement of type QuantumPhase) would keep the user-facing error honest in the meantime.

[ryanhill1]

Follow-up filed as #351, referenced here and in the code. Kept the diff out of the underlying gap as you asked.

Reproduced first: if(m==1) rzz(0.3) q[0],q[1]; unrolls to ['QuantumPhase', 'QuantumGate', ...] and emits gphase(-0.15) q[0], q[1];, and reverse_qubit_order() then remove_idle_qubits() raises here where it passes on main. #351 covers both routes to a fix (unroller not emitting gphase for a Qasm2Module, or _qasm_ast_to_str folding it) and notes the _whitelist_statements half.

Took the cheap in-PR mitigation you suggested — QuantumPhase now gets its own message:

Global phase is not representable in QASM 2.0, so it cannot appear in a conditional body; it is introduced by unrolling gates such as 'rzz' and 'rxx'

test_conditional_global_phase_reports_global_phase pins it via the reverse_qubit_order() + remove_idle_qubits() sequence you found.

[TheGupta2012]

[L1] Instance state for a constant — Maintenance · Low

Rationale: this tuple is rebuilt in __init__ on every module construction but never varies per instance. It is a fixed grammar production, and reads more clearly as one.

Change requested: hoist to a module-level _QOP_STATEMENTS tuple, alongside where the existing whitelist lives. Same for _whitelist_statements if worthwhile, though that is outside this PR's scope. Left as prose rather than a suggestion because the change spans two locations — the removal here and the new constant at module scope — so it is not applicable as a single patch.

[ryanhill1]

Resolved in ae8fedc — hoisted to a module-level _QOP_STATEMENTS tuple with the grammar comment attached to it.

Left _whitelist_statements where it is, per your note that it is outside this PR's scope.

[TheGupta2012]

[L2] Non-barrier cases report an AST class name — Implementation · Low

Rationale: barrier gets a friendly name; everything else falls back to statement of type {type(inner_stmt).__name__}, so a delay body reports statement of type DelayInstruction and a box reports statement of type Box. Since delay and box are called out in the description as the wider hole behind the barrier case, they are worth naming properly — the user wrote a QASM keyword, not an AST class.

Change requested:

Suggested change

	name = "barrier" if isinstance(inner_stmt, qasm3_ast.QuantumBarrier) else None
	described = f"'{name}'" if name else f"statement of type {type(inner_stmt).__name__}"
	keywords = {
	qasm3_ast.QuantumBarrier: "barrier",
	qasm3_ast.DelayInstruction: "delay",
	qasm3_ast.Box: "box",
	}
	name = keywords.get(type(inner_stmt))
	described = f"'{name}'" if name else f"statement of type {type(inner_stmt).__name__}"

If L1 is taken too, the mapping reads better hoisted to module scope next to _QOP_STATEMENTS rather than rebuilt per loop iteration.

[ryanhill1]

Resolved in ae8fedc — suggestion applied, hoisted to module scope as _NON_QOP_KEYWORDS alongside _QOP_STATEMENTS since L1 was taken, rather than rebuilt per loop iteration.

test_conditional_non_qop_rejected now asserts the keyword rather than just body of an 'if', so delay and box are pinned by name.