fix: emit fresh operand nodes from gate decompositions

CHANGELOG.md

@@ -27,6 +27,7 @@ Types of changes:
 ### Fixed
 - 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))
 - Fixed `remove_idle_qubits(in_place=False)` updating the qubit count on the wrong module: the original module's `num_qubits` was decremented while the returned copy kept the stale pre-removal count. The copy's AST was already correct; only the counters were swapped. ([#336](https://github.com/qBraid/pyqasm/pull/336))
 - Fixed `remove_idle_qubits()` raising `KeyError` when the unrolled AST contains operand nodes shared across multiple statements (e.g. the `crz` decomposition) and an idle lower-indexed qubit shifts the register indices. `_remap_qubits` now remaps each operand node exactly once instead of once per statement that references it. ([#332](https://github.com/qBraid/pyqasm/pull/332))
 - Fixed `box` duration validation summing `delay` durations across all qubits instead of tracking each qubit's timeline. Delays on disjoint qubits run in parallel, so `box[300ns] { delay[200ns] q[0]; delay[200ns] q[1]; }` was rejected while the identical schedule written as a broadcast delay (`delay[200ns] q;`) was accepted. Delays are now accumulated per qubit and the box is validated against the busiest single timeline; the error message names the offending qubit. Nested boxes now also contribute their declared duration to the enclosing box's timelines (previously the accumulator was reset when an inner box closed, dropping all inner delay accounting). ([#330](https://github.com/qBraid/pyqasm/pull/330))

src/pyqasm/decomposer.py

@@ -21,7 +21,7 @@
 
 from pyqasm.exceptions import RebaseError
 from pyqasm.maps.decomposition_rules import DECOMPOSITION_RULES, AppliedQubit
-from pyqasm.maps.gates import BASIS_GATE_MAP
+from pyqasm.maps.gates import BASIS_GATE_MAP, fresh_qubits
 
 
 class Decomposer:
@@ -133,7 +133,9 @@ def _get_decomposed_gates(cls, decomposition_rules, statement, gate):
                 modifiers=[],
                 name=qasm3_ast.Identifier(name=rule["gate"]),
                 arguments=arguments,
-                qubits=qubits,
+                # copy so the emitted gates do not share operand nodes with each
+                # other or with the source statement (see #333)
+                qubits=fresh_qubits(*qubits),
             )
 
             decomposed_gates.append(new_gate)

src/pyqasm/maps/gates.py

@@ -19,16 +19,66 @@
 
 """
 
+from copy import deepcopy
 from typing import Callable
 
 import numpy as np
-from openqasm3.ast import FloatLiteral, Identifier, IndexedIdentifier, QuantumGate, QuantumPhase
+from openqasm3.ast import (
+    FloatLiteral,
+    Identifier,
+    IndexedIdentifier,
+    IntegerLiteral,
+    QuantumGate,
+    QuantumPhase,
+)
 
 from pyqasm.elements import BasisSet, InversionOp
 from pyqasm.exceptions import ValidationError, raise_qasm3_error
 from pyqasm.linalg import kak_decomposition_angles
 from pyqasm.maps.expressions import CONSTANTS_MAP
 
+QubitOperand = IndexedIdentifier | Identifier
+
+
+def _copy_qubit(qubit: QubitOperand) -> QubitOperand:
+    """Return a copy of a single qubit operand that shares no nodes with it.
+
+    A shallow ``copy`` is not enough: transforms rewrite the index in place as
+    ``bit.indices[0][0].value = ...``, so the nested ``IntegerLiteral`` has to
+    be a distinct object. Rebuilding the (small, fully known) node tree is an
+    order of magnitude cheaper than ``deepcopy``, so the general case is only
+    used as a fallback for operands that are not a plain ``reg[int]``.
+    """
+    fresh: QubitOperand
+    if isinstance(qubit, Identifier):
+        fresh = Identifier(name=qubit.name)
+    else:
+        indices = qubit.indices
+        if not (
+            len(indices) == 1
+            and isinstance(indices[0], list)
+            and len(indices[0]) == 1
+            and isinstance(indices[0][0], IntegerLiteral)
+        ):
+            return deepcopy(qubit)
+        fresh = IndexedIdentifier(
+            name=Identifier(name=qubit.name.name),
+            indices=[[IntegerLiteral(value=indices[0][0].value)]],
+        )
+    fresh.span = qubit.span
+    return fresh
+
+
+def fresh_qubits(*qubits: QubitOperand) -> list[QubitOperand]:
+    """Copy qubit operands so every emitted statement owns its nodes.
+
+    Decomposition functions pass the same operand nodes to each statement they
+    emit; without copies, transforms that later rewrite qubit indices in place
+    (e.g. ``remove_idle_qubits``, ``reverse_qubit_order``) would mutate a
+    shared node once per referencing statement.
+    """
+    return [_copy_qubit(qubit) for qubit in qubits]
+
 
 def u3_gate(
     theta: int | float,
@@ -113,7 +163,9 @@ def global_phase_gate(theta: float, qubit_list: list[IndexedIdentifier]) -> list
     """
     return [
         QuantumPhase(
-            argument=FloatLiteral(value=theta), qubits=qubit_list, modifiers=[]  # type: ignore
+            argument=FloatLiteral(value=theta),
+            qubits=fresh_qubits(*qubit_list),  # type: ignore
+            modifiers=[],
         )
     ]
 
@@ -702,7 +754,7 @@ def ccx_gate_op(
             modifiers=[],
             name=Identifier(name="ccx"),
             arguments=[],
-            qubits=[qubit0, qubit1, qubit2],
+            qubits=fresh_qubits(qubit0, qubit1, qubit2),
         )
     ]
 
@@ -905,7 +957,7 @@ def one_qubit_gate_op(gate_name: str, qubit_id: IndexedIdentifier) -> list[Quant
             modifiers=[],
             name=Identifier(name=gate_name),
             arguments=[],
-            qubits=[qubit_id],
+            qubits=fresh_qubits(qubit_id),
         )
     ]
 
@@ -918,7 +970,7 @@ def one_qubit_rotation_op(
             modifiers=[],
             name=Identifier(name=gate_name),
             arguments=[FloatLiteral(value=rotation)],
-            qubits=[qubit_id],
+            qubits=fresh_qubits(qubit_id),
         )
     ]
 
@@ -931,7 +983,7 @@ def two_qubit_gate_op(
             modifiers=[],
             name=Identifier(name=gate_name.lower()),
             arguments=[],
-            qubits=[qubit_id1, qubit_id2],
+            qubits=fresh_qubits(qubit_id1, qubit_id2),
         )
     ]
 

src/pyqasm/modules/base.py

@@ -423,8 +423,9 @@ def _remap_qubits(self, reg_name: str, size: int, idle_indices: list[int]):
             del self._qubit_depths[(reg_name, idx)]
 
         # update the operations that use the qubits
-        # gate decompositions can reuse the same operand node across multiple statements,
-        # so track visited nodes to avoid remapping a shared node more than once
+        # the negctrl expansion in the visitor re-inserts the same QuantumGate object before
+        # and after the controlled gate, so a single index node can be reached more than once;
+        # track visited nodes so it is remapped exactly once
         visited_node_ids = set()
         for operation in iter_quantum_statements(self._unrolled_ast.statements):
             bit_list = Qasm3Analyzer.get_op_bit_list(operation)

tests/qasm3/test_transformations.py

@@ -17,7 +17,14 @@
 
 """
 
+import pytest
+from openqasm3.ast import DiscreteSet, Identifier, IndexedIdentifier, IntegerLiteral, Span
+
+from pyqasm.analyzer import Qasm3Analyzer
+from pyqasm.elements import BasisSet
 from pyqasm.entrypoint import dumps, loads
+from pyqasm.maps import QUANTUM_STATEMENTS
+from pyqasm.maps.gates import fresh_qubits
 from tests.utils import check_unrolled_qasm
 
 
@@ -156,6 +163,139 @@ def test_reverse_qubit_order_qasm3():
     check_unrolled_qasm(dumps(module), expected_qasm3_str)
 
 
+def test_reverse_qubit_order_gate_decomposition():
+    """Test reverse_qubit_order on a decomposed gate whose statements previously
+    shared operand nodes (issue #333)"""
+    qasm3_str = """
+    OPENQASM 3.0;
+    include "stdgates.inc";
+    qubit[3] q;
+    crz(0.5) q[1], q[2];
+    """
+
+    expected_qasm3_str = """
+    OPENQASM 3.0;
+    include "stdgates.inc";
+    qubit[3] q;
+    rz(0.25) q[0];
+    rx(1.5707963267948966) q[0];
+    rz(3.141592653589793) q[0];
+    rx(1.5707963267948966) q[0];
+    rz(3.141592653589793) q[0];
+    cx q[1], q[0];
+    rz(-0.25) q[0];
+    rx(1.5707963267948966) q[0];
+    rz(3.141592653589793) q[0];
+    rx(1.5707963267948966) q[0];
+    rz(3.141592653589793) q[0];
+    cx q[1], q[0];
+    """
+
+    module = loads(qasm3_str)
+    module.unroll()
+    module.reverse_qubit_order()
+    check_unrolled_qasm(dumps(module), expected_qasm3_str)
+
+
+def _assert_no_shared_operand_nodes(module):
+    """Assert no two quantum statements in the unrolled AST share an operand node"""
+    seen_bits: set[int] = set()
+    seen_indices: set[int] = set()
+    for statement in module._unrolled_ast.statements:
+        if isinstance(statement, QUANTUM_STATEMENTS):
+            for bit in Qasm3Analyzer.get_op_bit_list(statement):
+                assert id(bit) not in seen_bits, f"operand node shared: {bit}"
+                seen_bits.add(id(bit))
+                # physical qubits are bare Identifiers, and an index may be a DiscreteSet
+                # or multi-dimensional; only plain reg[int] operands have an index node
+                if not isinstance(bit, IndexedIdentifier):
+                    continue
+                indices = bit.indices
+                if not (
+                    len(indices) == 1 and isinstance(indices[0], list) and len(indices[0]) == 1
+                ):
+                    continue
+                index_node = indices[0][0]
+                assert id(index_node) not in seen_indices, f"index node shared: {bit}"
+                seen_indices.add(id(index_node))
+
+
+@pytest.mark.parametrize(
+    "operation",
+    [
+        "crz(0.5) q[1], q[2];",
+        "crx(0.5) q[0], q[1];",
+        "c4x q[0], q[1], q[2], q[3], q[4];",
+        "ecr q[0], q[1];",
+        "inv @ crz(0.5) q[1], q[2];",
+    ],
+)
+def test_unroll_emits_fresh_operand_nodes(operation):
+    """Test that unroll() never emits statements sharing operand nodes, so in-place
+    transformations remap each operand exactly once (issues #331, #333)"""
+    qasm3_str = f"""
+    OPENQASM 3.0;
+    include "stdgates.inc";
+    qubit[5] q;
+    {operation}
+    """
+    module = loads(qasm3_str)
+    module.unroll()
+    _assert_no_shared_operand_nodes(module)
+
+
+@pytest.mark.parametrize(
+    "operation", ["crz(0.5) q[1], q[2];", "swap q[0], q[2];", "cz q[1], q[2];"]
+)
+def test_rebase_emits_fresh_operand_nodes(operation):
+    """Test that rebase() never emits statements sharing operand nodes (issue #333)
+
+    ``swap`` and ``cz`` are in DECOMPOSITION_RULES so they exercise
+    Decomposer._get_decomposed_gates; ``crz`` is handled by maps/gates.py instead.
+    """
+    qasm3_str = f"""
+    OPENQASM 3.0;
+    include "stdgates.inc";
+    qubit[3] q;
+    {operation}
+    """
+    module = loads(qasm3_str).rebase(BasisSet.ROTATIONAL_CX)
+    _assert_no_shared_operand_nodes(module)
+
+
+@pytest.mark.parametrize(
+    "qubit",
+    [
+        # plain reg[int] operand, rebuilt node by node
+        IndexedIdentifier(Identifier("q"), [[IntegerLiteral(2)]]),
+        # physical qubit, kept as a bare Identifier
+        Identifier("$0"),
+        # anything else falls back to a deep copy
+        IndexedIdentifier(Identifier("q"), [DiscreteSet([IntegerLiteral(0), IntegerLiteral(1)])]),
+        IndexedIdentifier(Identifier("q"), [[IntegerLiteral(0)], [IntegerLiteral(1)]]),
+    ],
+)
+def test_fresh_qubits_shares_no_nodes(qubit):
+    """Test that fresh_qubits returns an equal operand that shares no node with the original"""
+    qubit.span = Span(1, 0, 1, 4)
+
+    (copied,) = fresh_qubits(qubit)
+
+    assert copied == qubit
+    assert copied.span == qubit.span
+    assert copied is not qubit
+    if isinstance(qubit, IndexedIdentifier):
+        assert copied.name is not qubit.name
+        for copied_dim, original_dim in zip(copied.indices, qubit.indices):
+            assert copied_dim is not original_dim
+            copied_exprs = copied_dim.values if isinstance(copied_dim, DiscreteSet) else copied_dim
+            original_exprs = (
+                original_dim.values if isinstance(original_dim, DiscreteSet) else original_dim
+            )
+            for copied_index, original_index in zip(copied_exprs, original_exprs):
+                assert copied_index is not original_index
+
+
 def test_populate_idle_qubits_qasm3():
     """Test the populate idle qubits function for qasm3 string"""