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;

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.

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.
• 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.

[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: 82777d50-ed96-4aad-85da-029c0cd980fb

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 92.85714% with 1 line in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/pyqasm/modules/qasm2.py	92.85%	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.