fix: reject mixed declared/physical programs under consolidate_qubits

CHANGELOG.md

@@ -28,6 +28,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 external and verbatim-box gates counting the depth of the decomposition they skipped: `unroll(external_gates=["crz"])` on a single `crz` reported `depth() == 12` while emitting one statement. An external gate now records its own depth, matching how a single-level external custom gate is already handled. ([#352](https://github.com/qBraid/pyqasm/issues/352))
 - Fixed `has_measurements()` / `remove_measurements()` and `has_barriers()` / `remove_barriers()` missing occurrences inside `for` / `while` / `switch` bodies on a module that has not been unrolled — e.g. a measurement inside a `for` loop was invisible and `remove_measurements()` was a no-op. The statement walker now descends into loop and switch bodies. ([#354](https://github.com/qBraid/pyqasm/issues/354))
 - Fixed unrolling of `rzz`/`rxx` in an OpenQASM 2 program emitting an invalid `gphase(...)` statement — syntax QASM 2 does not have — so the output was not a loadable QASM 2 program. Global phase is unobservable, so unroll-emitted phases are now dropped for a QASM 2 target; a user-written `gphase` is still rejected. A conditional left with no body by the drop is removed too, since QASM 2 has no form for an `if` without a `qop`. ([#351](https://github.com/qBraid/pyqasm/issues/351))

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.

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]

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(

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}': "
@@ -502,15 +505,18 @@ 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(
                 # pylint: disable-next=line-too-long
                 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:
@@ -519,6 +525,31 @@ def _qubit_register_consolidation(
                     span=val.span,
                 )
 
+        # physical qubits are kept as written, so consolidating around them would emit
+        # two address spaces the output cannot relate (issue #353)
+        # _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
+            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)}). "
+                    "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
+
         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)
@@ -592,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.
@@ -738,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.
@@ -870,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.
@@ -3244,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)

tests/qasm3/test_device_qubits.py

@@ -331,48 +331,138 @@ 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];
+    {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_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;
+    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)
+
+
+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";
+    qubit[2] q;
+    h q[0];
     cz $2, q[1];
-    c = measure $2;
     """
+    result = loads(qasm, device_qubits=5)
+    result.unroll()
     expected_qasm = """OPENQASM 3.0;
-    qubit[5] __PYQASM_QUBITS__;
     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)
     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;
     """
     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