From cb573d769aa86d0d7437a31b518873dc7d988fac Mon Sep 17 00:00:00 2001 From: Ryan Hill Date: Fri, 7 Aug 2026 08:25:33 -0500 Subject: [PATCH 1/4] fix: reject mixed declared/physical programs under consolidate_qubits 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 --- CHANGELOG.md | 1 + src/pyqasm/visitor.py | 20 +++++++++-- tests/qasm3/test_device_qubits.py | 60 +++++++++++++++++++++---------- 3 files changed, 60 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bcc32f06..5c0706d6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,7 @@ Types of changes: ### Removed ### Fixed +- Fixed `unroll(consolidate_qubits=True)` emitting two unrelated address spaces for a program mixing declared registers with physical qubits — a consolidated register plus as-written `$n` references. Such programs now raise a `ValidationError` naming the physical qubits, and a program using only physical qubits no longer receives an internal register declaration nothing references. ([#353](https://github.com/qBraid/pyqasm/issues/353)) - Fixed `remove_idle_qubits()` and `reverse_qubit_order()` ignoring statements nested inside `box` and `if` blocks. Top-level operands were rewritten while nested ones kept their old indices, so the result silently addressed the wrong qubits — and when a nested index fell outside the shrunken register, the output was not a loadable program at all. Both passes now walk nested bodies, as do `has_measurements()` / `remove_measurements()` and `has_barriers()` / `remove_barriers()`; a box left empty by a removal is dropped, since pyqasm rejects a box with no statements. Two consequences of the same blind spot are fixed alongside: a qubit operated on only inside an `if` block no longer counts as idle, and `remove_idle_qubits()` no longer raises `AssertionError` on a program that mixes physical qubits with declared registers. ([#345](https://github.com/qBraid/pyqasm/pull/345)) - Fixed `unroll(consolidate_qubits=True)` raising `AttributeError: 'str' object has no attribute 'name'` for any gate applied to a physical qubit, e.g. `h $1;`. Consolidation assumed every gate operand was an `IndexedIdentifier`, but a physical qubit survives unrolling as `Identifier("$1")`. Physical qubits are absolute hardware indices belonging to no declared register, so they are now left as written — matching how `measure`, `reset` and `barrier` already treat them. ([#344](https://github.com/qBraid/pyqasm/pull/344)) - Fixed `unroll()` and `rebase()` emitting statements that share operand AST nodes: gate decompositions passed the same `IndexedIdentifier` objects into every statement they emitted, so transformations that rewrite qubit indices in place mutated a shared node once per referencing statement. This crashed `reverse_qubit_order()` (`KeyError: -1`) and `remove_idle_qubits()` (`KeyError`, [#331](https://github.com/qBraid/pyqasm/issues/331)) on any decomposed gate (e.g. `crz`) whenever the remap was not the identity. Statement constructors in `maps/gates.py` and `Decomposer` now copy their qubit operands so every emitted statement owns its nodes. ([#333](https://github.com/qBraid/pyqasm/issues/333)) diff --git a/src/pyqasm/visitor.py b/src/pyqasm/visitor.py index ea9f5f38..ff62d4ca 100644 --- a/src/pyqasm/visitor.py +++ b/src/pyqasm/visitor.py @@ -496,8 +496,9 @@ def _qubit_register_consolidation( Raises: ValidationError: If the total number of qubits exceeds the available device qubits, - or if the reserved register '__PYQASM_QUBITS__' is already declared - in the original QASM program. + if the reserved register '__PYQASM_QUBITS__' is already declared + in the original QASM program, or if the program mixes declared + registers with physical qubits. """ if total_qubits > self._module._device_qubits: # type: ignore raise_qasm3_error( @@ -505,6 +506,21 @@ def _qubit_register_consolidation( f"Total qubits '({total_qubits})' exceed device qubits '({self._module._device_qubits})'.", ) + # physical qubits are kept as written, so consolidating around them would emit + # two address spaces the output cannot relate (issue #353) + physical_qubits = sorted( + name for name, _ in self._module._qubit_depths if name.startswith("$") + ) + if physical_qubits: + if sum(self._global_qreg_size_map.values()) > 0: + raise_qasm3_error( + "Cannot consolidate qubit registers: the program mixes declared " + f"registers with physical qubits ({', '.join(physical_qubits)})", + ) + # only physical qubits: nothing to consolidate, so do not declare an + # internal register nothing would reference + return unrolled_stmts + global_scope = self._scope_manager.get_global_scope() for var, val in global_scope.items(): if var == INTERNAL_QUBIT_REGISTER: diff --git a/tests/qasm3/test_device_qubits.py b/tests/qasm3/test_device_qubits.py index 7389633c..09615712 100644 --- a/tests/qasm3/test_device_qubits.py +++ b/tests/qasm3/test_device_qubits.py @@ -331,41 +331,52 @@ def test_incorrect_qubit_reg(qasm_code, error_message, error_span, caplog): assert error_span in caplog.text -def test_physical_qubits_are_not_consolidated(): - """Physical qubits are absolute hardware indices and belong to no declared register, - so consolidation must leave them alone instead of raising (see #343).""" - qasm = """OPENQASM 3.0; +@pytest.mark.parametrize( + "operation", + [ + "cz $2, q[1];", + "c = measure $2;", + "reset $2;", + "barrier $2;", + ], +) +def test_mixed_declared_and_physical_rejected_when_consolidating(operation): + """A program mixing declared registers with physical qubits would consolidate into + two address spaces the output cannot relate, so it is rejected (issue #353).""" + qasm = f"""OPENQASM 3.0; include "stdgates.inc"; qubit[2] q; bit c; h q[0]; - cz $2, q[1]; - c = measure $2; + {operation} """ - expected_qasm = """OPENQASM 3.0; - qubit[5] __PYQASM_QUBITS__; + result = loads(qasm, device_qubits=5) + with pytest.raises(ValidationError, match=r"mixes declared registers with physical qubits"): + result.unroll(consolidate_qubits=True) + + +def test_mixed_declared_and_physical_still_unrolls_without_consolidation(): + """The mixed-program rejection applies only under consolidate_qubits=True.""" + qasm = """OPENQASM 3.0; include "stdgates.inc"; - bit[1] c; - h __PYQASM_QUBITS__[0]; - cz $2, __PYQASM_QUBITS__[1]; - c = measure $2; + qubit[2] q; + h q[0]; + cz $2, q[1]; """ result = loads(qasm, device_qubits=5) - result.unroll(consolidate_qubits=True) - check_unrolled_qasm(dumps(result), expected_qasm) - # 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 def test_physical_qubits_only(): + """With nothing to consolidate, no internal register is declared: the program + keeps speaking the physical address space alone (issue #353).""" qasm = """OPENQASM 3.0; include "stdgates.inc"; h $1; cz $2, $1; """ expected_qasm = """OPENQASM 3.0; - qubit[5] __PYQASM_QUBITS__; include "stdgates.inc"; h $1; cz $2, $1; @@ -373,6 +384,17 @@ def test_physical_qubits_only(): result = loads(qasm, device_qubits=5) result.unroll(consolidate_qubits=True) check_unrolled_qasm(dumps(result), expected_qasm) - # nothing was consolidated, so the count comes entirely from the physical indices - # while the emitted declaration is sized by device_qubits (see #353) + # the count comes entirely from the physical indices assert result.num_qubits == 3 + + +def test_physical_qubits_only_without_device_qubits(): + """The unreferenced declaration is suppressed with or without device_qubits set.""" + qasm = """OPENQASM 3.0; + include "stdgates.inc"; + h $1; + """ + result = loads(qasm) + result.unroll(consolidate_qubits=True) + assert "__PYQASM_QUBITS__" not in dumps(result) + assert result.num_qubits == 2 From 3e1b60f6f7533122203413cd101eaeb49afd588c Mon Sep 17 00:00:00 2001 From: Ryan Hill Date: Fri, 7 Aug 2026 12:00:02 -0500 Subject: [PATCH 2/4] fix: detect declared registers by presence, not capacity (Argus P1) 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). --- src/pyqasm/visitor.py | 4 +++- tests/qasm3/test_device_qubits.py | 14 ++++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/src/pyqasm/visitor.py b/src/pyqasm/visitor.py index ff62d4ca..592a724a 100644 --- a/src/pyqasm/visitor.py +++ b/src/pyqasm/visitor.py @@ -512,7 +512,9 @@ def _qubit_register_consolidation( name for name, _ in self._module._qubit_depths if name.startswith("$") ) if physical_qubits: - if sum(self._global_qreg_size_map.values()) > 0: + # presence, not capacity: a zero-sized declared register still declares + # a second address space + if self._global_qreg_size_map: raise_qasm3_error( "Cannot consolidate qubit registers: the program mixes declared " f"registers with physical qubits ({', '.join(physical_qubits)})", diff --git a/tests/qasm3/test_device_qubits.py b/tests/qasm3/test_device_qubits.py index 09615712..3a975695 100644 --- a/tests/qasm3/test_device_qubits.py +++ b/tests/qasm3/test_device_qubits.py @@ -351,6 +351,20 @@ def test_mixed_declared_and_physical_rejected_when_consolidating(operation): {operation} """ result = loads(qasm, device_qubits=5) + with pytest.raises( + ValidationError, match=r"mixes declared registers with physical qubits \(\$2\)" + ): + result.unroll(consolidate_qubits=True) + + +def test_zero_sized_register_still_counts_as_declared(): + """A zero-sized declared register is still a second address space (Argus P1).""" + qasm = """OPENQASM 3.0; + include "stdgates.inc"; + qubit[0] q; + h $1; + """ + result = loads(qasm) with pytest.raises(ValidationError, match=r"mixes declared registers with physical qubits"): result.unroll(consolidate_qubits=True) From f4fcfc6cbceb559457d5144c5c1a971734a3391c Mon Sep 17 00:00:00 2001 From: Ryan Hill Date: Thu, 13 Aug 2026 15:26:13 -0500 Subject: [PATCH 3/4] fix: check reserved name first, sort physical qubits numerically Address review on #362: - Move the INTERNAL_QUBIT_REGISTER reserved-name loop above the new physical-qubit block. The early return skipped it entirely, so 'int __PYQASM_QUBITS__ = 3; h $1;' succeeded silently where main raised, and the qubit-register form reported the physical-qubit problem instead of the reserved name the user actually hit. Keeps the docstring's promise honest. - Sort the reported physical qubits by numeric index; a lexicographic sort produced '($10, $2)' on any device with ten or more qubits. - Name the two ways out in the message. No statement node exists at finalize time, so there is no span or line number to fall back on and the text is the entire diagnostic. - Assert the unrolled text in test_physical_qubits_are_not_consolidated, not just the count -- num_qubits == 3 would still pass if 'cz $2, q[1]' came out rewritten or dropped. --- src/pyqasm/visitor.py | 25 ++++++++------ tests/qasm3/test_device_qubits.py | 54 +++++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+), 10 deletions(-) diff --git a/src/pyqasm/visitor.py b/src/pyqasm/visitor.py index 592a724a..efc77e10 100644 --- a/src/pyqasm/visitor.py +++ b/src/pyqasm/visitor.py @@ -506,10 +506,21 @@ def _qubit_register_consolidation( f"Total qubits '({total_qubits})' exceed device qubits '({self._module._device_qubits})'.", ) + # checked before the physical-qubit exits below, so a program that declares the + # reserved name is told about that rather than about whatever it does next + global_scope = self._scope_manager.get_global_scope() + for var, val in global_scope.items(): + if var == INTERNAL_QUBIT_REGISTER: + raise_qasm3_error( + f"Variable '{INTERNAL_QUBIT_REGISTER}' is already defined", + span=val.span, + ) + # physical qubits are kept as written, so consolidating around them would emit # two address spaces the output cannot relate (issue #353) physical_qubits = sorted( - name for name, _ in self._module._qubit_depths if name.startswith("$") + (name for name, _ in self._module._qubit_depths if name.startswith("$")), + key=lambda name: int(name[1:]), ) if physical_qubits: # presence, not capacity: a zero-sized declared register still declares @@ -517,20 +528,14 @@ def _qubit_register_consolidation( if self._global_qreg_size_map: raise_qasm3_error( "Cannot consolidate qubit registers: the program mixes declared " - f"registers with physical qubits ({', '.join(physical_qubits)})", + f"registers with physical qubits ({', '.join(physical_qubits)}). " + "Unroll without 'consolidate_qubits=True', or rewrite the physical " + "qubits as operands of a declared register.", ) # only physical qubits: nothing to consolidate, so do not declare an # internal register nothing would reference return unrolled_stmts - global_scope = self._scope_manager.get_global_scope() - for var, val in global_scope.items(): - if var == INTERNAL_QUBIT_REGISTER: - raise_qasm3_error( - f"Variable '{INTERNAL_QUBIT_REGISTER}' is already defined", - span=val.span, - ) - pyqasm_reg_id = qasm3_ast.Identifier(INTERNAL_QUBIT_REGISTER) pyqasm_reg_size = qasm3_ast.IntegerLiteral(self._module._device_qubits) # type: ignore pyqasm_reg_stmt = qasm3_ast.QubitDeclaration(pyqasm_reg_id, pyqasm_reg_size) diff --git a/tests/qasm3/test_device_qubits.py b/tests/qasm3/test_device_qubits.py index 3a975695..1a87cba5 100644 --- a/tests/qasm3/test_device_qubits.py +++ b/tests/qasm3/test_device_qubits.py @@ -357,6 +357,53 @@ def test_mixed_declared_and_physical_rejected_when_consolidating(operation): result.unroll(consolidate_qubits=True) +def test_mixed_error_lists_physical_qubits_in_numeric_order(): + """A lexicographic sort would report ($10, $2) once a device has ten or more + qubits, which reads as unordered when scanning for the offending references.""" + qasm = """OPENQASM 3.0; + include "stdgates.inc"; + qubit[1] q; + h q[0]; + h $2; + h $10; + """ + result = loads(qasm, device_qubits=16) + with pytest.raises( + ValidationError, match=r"mixes declared registers with physical qubits \(\$2, \$10\)" + ): + result.unroll(consolidate_qubits=True) + + +def test_mixed_error_names_the_way_out(): + """The message is the whole diagnostic here -- no statement node is available at + finalize time, so there is no line number to fall back on.""" + qasm = """OPENQASM 3.0; + include "stdgates.inc"; + qubit[1] q; + h q[0]; + h $2; + """ + result = loads(qasm, device_qubits=5) + with pytest.raises(ValidationError, match=r"Unroll without 'consolidate_qubits=True'"): + result.unroll(consolidate_qubits=True) + + +@pytest.mark.parametrize( + "declaration", ["int __PYQASM_QUBITS__ = 3;", "qubit[2] __PYQASM_QUBITS__;"] +) +def test_reserved_name_is_reported_before_physical_qubit_exits(declaration): + """Declaring the reserved name must raise even when the program also uses physical + qubits, which would otherwise return early or report the wrong problem.""" + qasm = f"""OPENQASM 3.0; + include "stdgates.inc"; + {declaration} + h $1; + """ + result = loads(qasm, device_qubits=5) + with pytest.raises(ValidationError, match=r"'__PYQASM_QUBITS__' is already defined"): + result.unroll(consolidate_qubits=True) + + def test_zero_sized_register_still_counts_as_declared(): """A zero-sized declared register is still a second address space (Argus P1).""" qasm = """OPENQASM 3.0; @@ -379,6 +426,13 @@ def test_mixed_declared_and_physical_still_unrolls_without_consolidation(): """ result = loads(qasm, device_qubits=5) 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 From 53dbc76a8cc0e3e54d41f0b4ccc38f4dee43103d Mon Sep 17 00:00:00 2001 From: TheGupta2012 Date: Mon, 17 Aug 2026 16:23:07 +0530 Subject: [PATCH 4/4] refactor: recognise physical qubits through a shared helper The "$n" test was written out at seven call sites across visitor.py and the pulse module, and the consolidation check spelled it a eighth way, matching on the prefix alone and then parsing the index back out of the name. elements.py gains is_physical_qubit alongside is_internal_qubit_register, and the call sites use it. The consolidation check now orders on the index already held in the _qubit_depths key rather than re-deriving it. The declaration path keeps its own prefix test: it has to recognise a malformed "$foo" to report it, which a well-formedness helper cannot express. Co-Authored-By: Claude Opus 5 (1M context) --- src/pyqasm/elements.py | 20 ++++++++++++++++++++ src/pyqasm/pulse/utils.py | 4 ++-- src/pyqasm/pulse/visitor.py | 6 ++---- src/pyqasm/visitor.py | 28 ++++++++++++++++++---------- 4 files changed, 42 insertions(+), 16 deletions(-) diff --git a/src/pyqasm/elements.py b/src/pyqasm/elements.py index b7dd303c..3950da36 100644 --- a/src/pyqasm/elements.py +++ b/src/pyqasm/elements.py @@ -47,6 +47,26 @@ def is_internal_qubit_register(qubit_name: str) -> bool: ) +PHYSICAL_QUBIT_PREFIX = "$" +"""Prefix marking a physical qubit: an absolute hardware index that belongs to no +declared register.""" + + +def is_physical_qubit(qubit_name: str) -> bool: + """Check whether an identifier refers to a physical qubit ("$0", "$12"). + + The index must be a non-negative integer, so neither the bare prefix nor a name + such as "$foo" is a physical qubit. + + Args: + qubit_name (str): The identifier name to check. + + Returns: + bool: True if the identifier refers to a physical qubit. + """ + return qubit_name.startswith(PHYSICAL_QUBIT_PREFIX) and qubit_name[1:].isdigit() + + class InversionOp(Enum): """ Enum for specifying the inversion action of a gate. diff --git a/src/pyqasm/pulse/utils.py b/src/pyqasm/pulse/utils.py index 3469c0c9..694aff93 100644 --- a/src/pyqasm/pulse/utils.py +++ b/src/pyqasm/pulse/utils.py @@ -22,7 +22,7 @@ import openqasm3.ast as qasm3_ast -from pyqasm.elements import INTERNAL_QUBIT_REGISTER +from pyqasm.elements import INTERNAL_QUBIT_REGISTER, is_physical_qubit from pyqasm.exceptions import raise_qasm3_error @@ -75,7 +75,7 @@ def process_qubits_for_openpulse_gate( # pylint: disable=too-many-arguments _qubit_set = set() for i, qubit in enumerate(operation.qubits): qubit_id = qubit.name.name if hasattr(qubit.name, "name") else qubit.name - if qubit_id.startswith("$") and qubit_id[1:].isdigit(): + if is_physical_qubit(qubit_id): if ( gate_op not in openpulse_qubit_map or qubit_id not in openpulse_qubit_map[gate_op] diff --git a/src/pyqasm/pulse/visitor.py b/src/pyqasm/pulse/visitor.py index 65075525..6128aa69 100644 --- a/src/pyqasm/pulse/visitor.py +++ b/src/pyqasm/pulse/visitor.py @@ -25,7 +25,7 @@ import openqasm3.ast as qasm3_ast -from pyqasm.elements import Capture, Frame, Variable, Waveform +from pyqasm.elements import Capture, Frame, Variable, Waveform, is_physical_qubit from pyqasm.exceptions import ( raise_qasm3_error, ) @@ -385,9 +385,7 @@ def _visit_barrier(self, barrier: qasm3_ast.QuantumBarrier) -> list[qasm3_ast.Qu """ if barrier.qubits: for qubit in barrier.qubits: - if isinstance(qubit, qasm3_ast.Identifier) and not ( - qubit.name.startswith("$") and qubit.name[1:].isdigit() - ): + if isinstance(qubit, qasm3_ast.Identifier) and not is_physical_qubit(qubit.name): frame = self._openpulse_scope_manager.get_from_global_scope(qubit.name) if frame is None: raise_qasm3_error( diff --git a/src/pyqasm/visitor.py b/src/pyqasm/visitor.py index 737c9628..0eca3962 100644 --- a/src/pyqasm/visitor.py +++ b/src/pyqasm/visitor.py @@ -35,6 +35,7 @@ from pyqasm.analyzer import Qasm3Analyzer from pyqasm.elements import ( INTERNAL_QUBIT_REGISTER, + PHYSICAL_QUBIT_PREFIX, Capture, ClbitDepthNode, Context, @@ -44,6 +45,7 @@ Variable, Waveform, is_internal_qubit_register, + is_physical_qubit, ) from pyqasm.exceptions import ( BreakSignal, @@ -330,8 +332,9 @@ def _get_op_bits( else: reg_name = bit.name - if qubits and reg_name.startswith("$"): - # Physical qubit reference (e.g. $0, $1). + if qubits and reg_name.startswith(PHYSICAL_QUBIT_PREFIX): + # Physical qubit reference (e.g. $0, $1). Matched on the prefix alone so + # that a malformed index is reported rather than read as a register name. if not reg_name[1:].isdigit(): raise_qasm3_error( f"Invalid physical qubit identifier '{reg_name}': " @@ -524,10 +527,15 @@ def _qubit_register_consolidation( # physical qubits are kept as written, so consolidating around them would emit # two address spaces the output cannot relate (issue #353) - physical_qubits = sorted( - (name for name, _ in self._module._qubit_depths if name.startswith("$")), - key=lambda name: int(name[1:]), - ) + # _qubit_depths is keyed (name, index), so the hardware index is already + # there to order on -- no need to parse it back out of the name + physical_qubits = [ + name + for name, _ in sorted( + (key for key in self._module._qubit_depths if is_physical_qubit(key[0])), + key=lambda key: key[1], + ) + ] if physical_qubits: # presence, not capacity: a zero-sized declared register still declares # a second address space @@ -615,7 +623,7 @@ def _visit_measurement( # pylint: disable=too-many-locals,too-many-branches,too target = statement.target if isinstance(source, qasm3_ast.Identifier): is_pulse_gate = False - if source.name.startswith("$") and source.name[1:].isdigit(): + if is_physical_qubit(source.name): if self._openpulse_grammar_declared: # OpenPulse program: rename to the internal virtual register used by the # pulse visitor, and validate the index is in range. @@ -761,7 +769,7 @@ def _resolve_unindexed_reset_qubit(self, statement: qasm3_ast.QuantumReset) -> b return False qubit_name = statement.qubits.name - if qubit_name.startswith("$") and qubit_name[1:].isdigit(): + if is_physical_qubit(qubit_name): if self._openpulse_grammar_declared: # OpenPulse program: rename to the internal virtual register used by the # pulse visitor. @@ -893,7 +901,7 @@ def _visit_barrier( # pylint: disable=too-many-locals, too-many-branches valid_open_pulse_qubits = False for op_qubit in barrier.qubits: if isinstance(op_qubit, qasm3_ast.Identifier): - if op_qubit.name.startswith("$") and op_qubit.name[1:].isdigit(): + if is_physical_qubit(op_qubit.name): phys_idx = int(op_qubit.name[1:]) # In an OpenPulse program all physical qubits are declared up-front via # defcal; validate that the index is within the known range. @@ -3267,7 +3275,7 @@ def _visit_calibration_definition( if not isinstance(qubit, qasm3_ast.Identifier): continue name = qubit.name - if name.startswith("$") and name[1:].isdigit(): + if is_physical_qubit(name): _qubit_set.add(int(name[1:])) self._openpulse_qubit_map[statement.name.name].add(name) self._total_pulse_qubits = max(self._total_pulse_qubits, int(name[1:]) + 1)