fix: reject mixed declared/physical programs under consolidate_qubits by ryanhill1 · Pull Request #362 · qBraid/pyqasm

ryanhill1 · 2026-08-07T13:25:36Z

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 a ValidationError naming the physical qubits, rather than emitting a consolidated register plus as-written $n references — 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 by device_qubits or 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:

Updates the changelog to document the consolidated-qubit validation behavior.

Consolidated Qubit Validation Flow

sequenceDiagram
  participant Test as tests/qasm3/test_device_qubits.py
  participant Visitor as src/pyqasm/visitor.py ⚠️
  Test->>Visitor: unroll(consolidate_qubits=True)
  Visitor->>Visitor: inspect declared and physical qubits
  Visitor-->>Test: raise ValidationError or emit unrolled program

_{Auto-enriched by Argus}

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-eye · 2026-08-07T13:25:39Z

Argus review

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

Trigger Argus review

Estimated cost

Files changed: 3
Diff lines (±): 81
Historical avg: ~318.9k tokens · ~$1.35 · across last 6 review(s)

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

coderabbitai · 2026-08-07T13:28:56Z

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: 55875e8c-d40b-4572-877f-fc6595e0f93c

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

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

codecov-commenter · 2026-08-07T13:31:22Z

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

ryanhill1 · 2026-08-07T16:26:15Z

@Argus-Eye review

ryanhill1 · 2026-08-07T16:56:48Z

@Argus-Eye review

argus-eye

🔎 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}

argus-eye · 2026-08-07T16:58:39Z

+
+def test_mixed_declared_and_physical_still_unrolls_without_consolidation():
+    """The mixed-program rejection applies only under consolidate_qubits=True."""


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

Suggested change

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

ryanhill1 requested a review from TheGupta2012 as a code owner August 7, 2026 13:25

Merge branch 'main' into fix-consolidate-mixed-physical

33be273

This comment has been minimized.

Sign in to view

argus-eye Bot reviewed Aug 7, 2026

View reviewed changes

ryanhill1 added 2 commits August 7, 2026 12:00

fix: detect declared registers by presence, not capacity (Argus P1)

3e1b60f

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

Merge remote-tracking branch 'origin/fix-consolidate-mixed-physical' …

c18b624

…into fix-consolidate-mixed-physical

Navigation Menu

Uh oh!

fix: reject mixed declared/physical programs under consolidate_qubits - #362

fix: reject mixed declared/physical programs under consolidate_qubits#362
ryanhill1 wants to merge 4 commits into
mainfrom
fix-consolidate-mixed-physical

ryanhill1 commented Aug 7, 2026 •

edited by argus-eye Bot

Loading

Uh oh!

argus-eye Bot commented Aug 7, 2026

Uh oh!

coderabbitai Bot commented Aug 7, 2026 •

edited

Loading

Review skipped

Uh oh!

codecov-commenter commented Aug 7, 2026

Uh oh!

ryanhill1 commented Aug 7, 2026

Uh oh!

ryanhill1 commented Aug 7, 2026

Uh oh!

This comment has been minimized.

argus-eye Bot left a comment

Uh oh!

Uh oh!

argus-eye Bot Aug 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants


		def test_mixed_declared_and_physical_still_unrolls_without_consolidation():
		"""The mixed-program rejection applies only under consolidate_qubits=True."""

-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

Uh oh!

Conversation

ryanhill1 commented Aug 7, 2026 • edited by argus-eye Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

argus-eye Bot commented Aug 7, 2026

Argus review

Uh oh!

coderabbitai Bot commented Aug 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

codecov-commenter commented Aug 7, 2026

Codecov Report

Uh oh!

ryanhill1 commented Aug 7, 2026

Uh oh!

ryanhill1 commented Aug 7, 2026

Uh oh!

This comment has been minimized.

argus-eye Bot left a comment

Choose a reason for 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)

✅ Intent delivered

Simulation Results

Uh oh!

Uh oh!

argus-eye Bot Aug 7, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ryanhill1 commented Aug 7, 2026 •

edited by argus-eye Bot

Loading

coderabbitai Bot commented Aug 7, 2026 •

edited

Loading