From c2963689b98b71679284d27471c9df67ae90a782 Mon Sep 17 00:00:00 2001 From: Ryan Hill Date: Fri, 24 Jul 2026 12:36:07 -0400 Subject: [PATCH 1/3] fix: emit fresh operand nodes from gate decompositions Gate decomposition functions passed the same IndexedIdentifier objects into every statement they emitted, so in-place index rewrites in remove_idle_qubits() and reverse_qubit_order() mutated a shared node once per referencing statement, raising KeyError whenever the remap was not the identity. Statement constructors in maps/gates.py and Decomposer now deep-copy their qubit operands so every emitted statement owns its nodes. Fixes #333 --- CHANGELOG.md | 1 + src/pyqasm/decomposer.py | 6 +- src/pyqasm/maps/gates.py | 24 ++++++-- tests/qasm3/test_transformations.py | 89 +++++++++++++++++++++++++++++ 4 files changed, 114 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 63fac09e..01615b0a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,7 @@ Types of changes: ### Removed ### Fixed +- 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 deep-copy their qubit operands so every emitted statement owns its nodes. ([#333](https://github.com/qBraid/pyqasm/issues/333)) - 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)) - Fixed `reset` on a physical qubit rewriting the operand to the internal pulse register, e.g. `reset $2;` unrolled to `reset __PYQASM_QUBITS__[2];`. That names a register the program never declares, so the unrolled output did not round-trip through `dumps()`/`loads()`, and the qubit was never registered (a program whose only operation was `reset $3;` reported `num_qubits == 0`). Physical qubits are now kept as-is in plain QASM programs, matching how gate and measurement operands already treat them; the rename is still applied for OpenPulse programs, where the pulse visitor expects it. ([#325](https://github.com/qBraid/pyqasm/pull/325)) - Fixed `measure` and `reset` on a user register whose name merely starts with the reserved internal register name (e.g. `__PYQASM_QUBITS__foo`) being mistaken for the internal register itself. Such statements were short-circuited out of unrolling and emitted verbatim, so `c = measure __PYQASM_QUBITS__foo;` was never expanded per qubit and `reset __PYQASM_QUBITS__foo;` silently reset nothing. The register is now matched on its exact name, or on the name followed by an index or slice. ([#325](https://github.com/qBraid/pyqasm/pull/325)) diff --git a/src/pyqasm/decomposer.py b/src/pyqasm/decomposer.py index d302f938..9c41bf89 100644 --- a/src/pyqasm/decomposer.py +++ b/src/pyqasm/decomposer.py @@ -16,6 +16,8 @@ Definition of the Decomposer class """ +from copy import deepcopy + import openqasm3.ast as qasm3_ast from openqasm3.ast import BranchingStatement, QuantumGate @@ -133,7 +135,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=[deepcopy(qubit) for qubit in qubits], ) decomposed_gates.append(new_gate) diff --git a/src/pyqasm/maps/gates.py b/src/pyqasm/maps/gates.py index 0d074e43..7d02f1de 100644 --- a/src/pyqasm/maps/gates.py +++ b/src/pyqasm/maps/gates.py @@ -19,6 +19,7 @@ """ +from copy import deepcopy from typing import Callable import numpy as np @@ -30,6 +31,17 @@ from pyqasm.maps.expressions import CONSTANTS_MAP +def _fresh_qubits(*qubits: IndexedIdentifier) -> list[IndexedIdentifier]: + """Deep-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 [deepcopy(qubit) for qubit in qubits] + + def u3_gate( theta: int | float, phi: int | float, @@ -113,7 +125,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 +716,7 @@ def ccx_gate_op( modifiers=[], name=Identifier(name="ccx"), arguments=[], - qubits=[qubit0, qubit1, qubit2], + qubits=_fresh_qubits(qubit0, qubit1, qubit2), ) ] @@ -905,7 +919,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 +932,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 +945,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), ) ] diff --git a/tests/qasm3/test_transformations.py b/tests/qasm3/test_transformations.py index a8c0eabe..18205a3c 100644 --- a/tests/qasm3/test_transformations.py +++ b/tests/qasm3/test_transformations.py @@ -17,7 +17,12 @@ """ +import pytest + +from pyqasm.analyzer import Qasm3Analyzer +from pyqasm.elements import BasisSet from pyqasm.entrypoint import dumps, loads +from pyqasm.maps import QUANTUM_STATEMENTS from tests.utils import check_unrolled_qasm @@ -135,6 +140,90 @@ 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)) + index_node = bit.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) + + +def test_rebase_emits_fresh_operand_nodes(): + """Test that rebase() never emits statements sharing operand nodes (issue #333)""" + qasm3_str = """ + OPENQASM 3.0; + include "stdgates.inc"; + qubit[3] q; + crz(0.5) q[1], q[2]; + """ + module = loads(qasm3_str).rebase(BasisSet.ROTATIONAL_CX) + _assert_no_shared_operand_nodes(module) + + def test_populate_idle_qubits_qasm3(): """Test the populate idle qubits function for qasm3 string""" From e4293d6f655cff941e9bbc5f826482d11bc73aa6 Mon Sep 17 00:00:00 2001 From: Ryan Hill Date: Fri, 24 Jul 2026 12:39:15 -0400 Subject: [PATCH 2/3] fix mypy: widen _fresh_qubits return type for QuantumGate.qubits invariance --- src/pyqasm/maps/gates.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pyqasm/maps/gates.py b/src/pyqasm/maps/gates.py index 7d02f1de..63fd6602 100644 --- a/src/pyqasm/maps/gates.py +++ b/src/pyqasm/maps/gates.py @@ -31,7 +31,7 @@ from pyqasm.maps.expressions import CONSTANTS_MAP -def _fresh_qubits(*qubits: IndexedIdentifier) -> list[IndexedIdentifier]: +def _fresh_qubits(*qubits: IndexedIdentifier) -> list[IndexedIdentifier | Identifier]: """Deep-copy qubit operands so every emitted statement owns its nodes. Decomposition functions pass the same operand nodes to each statement they From 87f41d5167f03cf46c65b2d60e9305497ed2780f Mon Sep 17 00:00:00 2001 From: Ryan Hill Date: Fri, 31 Jul 2026 12:19:02 -0500 Subject: [PATCH 3/3] review: replace deepcopy with structural operand copy, share helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses review feedback on #335: - A shallow `copy` cannot replace `deepcopy` here: transforms rewrite the index in place as `bit.indices[0][0].value = ...`, so the nested `IntegerLiteral` must be a distinct object, which `copy` does not give. Instead, `_copy_qubit` rebuilds the (small, fully known) `reg[int]` node tree directly — ~14x faster than `deepcopy` in a microbenchmark, and faster than `copy.copy` as well — falling back to `deepcopy` only for operand shapes that are not a plain `reg[int]`. - `Decomposer._get_decomposed_gates` now uses the shared `fresh_qubits` helper (renamed from `_fresh_qubits` now that it crosses modules) instead of its own inline `deepcopy` comprehension. - Added `test_fresh_qubits_shares_no_nodes` covering the plain, physical qubit, and `deepcopy` fallback paths. --- CHANGELOG.md | 2 +- src/pyqasm/decomposer.py | 6 ++-- src/pyqasm/maps/gates.py | 56 ++++++++++++++++++++++++----- tests/qasm3/test_transformations.py | 35 ++++++++++++++++++ 4 files changed, 85 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 33f2f26d..4d909bb6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,7 +25,7 @@ Types of changes: ### Removed ### Fixed -- 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 deep-copy their qubit operands so every emitted statement owns its nodes. ([#333](https://github.com/qBraid/pyqasm/issues/333)) +- 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()` 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)) - Fixed `reset` on a physical qubit rewriting the operand to the internal pulse register, e.g. `reset $2;` unrolled to `reset __PYQASM_QUBITS__[2];`. That names a register the program never declares, so the unrolled output did not round-trip through `dumps()`/`loads()`, and the qubit was never registered (a program whose only operation was `reset $3;` reported `num_qubits == 0`). Physical qubits are now kept as-is in plain QASM programs, matching how gate and measurement operands already treat them; the rename is still applied for OpenPulse programs, where the pulse visitor expects it. ([#325](https://github.com/qBraid/pyqasm/pull/325)) diff --git a/src/pyqasm/decomposer.py b/src/pyqasm/decomposer.py index 9c41bf89..147cee07 100644 --- a/src/pyqasm/decomposer.py +++ b/src/pyqasm/decomposer.py @@ -16,14 +16,12 @@ Definition of the Decomposer class """ -from copy import deepcopy - import openqasm3.ast as qasm3_ast from openqasm3.ast import BranchingStatement, QuantumGate 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: @@ -137,7 +135,7 @@ def _get_decomposed_gates(cls, decomposition_rules, statement, gate): arguments=arguments, # copy so the emitted gates do not share operand nodes with each # other or with the source statement (see #333) - qubits=[deepcopy(qubit) for qubit in qubits], + qubits=fresh_qubits(*qubits), ) decomposed_gates.append(new_gate) diff --git a/src/pyqasm/maps/gates.py b/src/pyqasm/maps/gates.py index 63fd6602..7115da06 100644 --- a/src/pyqasm/maps/gates.py +++ b/src/pyqasm/maps/gates.py @@ -23,23 +23,61 @@ 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: IndexedIdentifier) -> list[IndexedIdentifier | Identifier]: - """Deep-copy qubit operands so every emitted statement owns its nodes. +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 [deepcopy(qubit) for qubit in qubits] + return [_copy_qubit(qubit) for qubit in qubits] def u3_gate( @@ -126,7 +164,7 @@ def global_phase_gate(theta: float, qubit_list: list[IndexedIdentifier]) -> list return [ QuantumPhase( argument=FloatLiteral(value=theta), - qubits=_fresh_qubits(*qubit_list), # type: ignore + qubits=fresh_qubits(*qubit_list), # type: ignore modifiers=[], ) ] @@ -716,7 +754,7 @@ def ccx_gate_op( modifiers=[], name=Identifier(name="ccx"), arguments=[], - qubits=_fresh_qubits(qubit0, qubit1, qubit2), + qubits=fresh_qubits(qubit0, qubit1, qubit2), ) ] @@ -919,7 +957,7 @@ def one_qubit_gate_op(gate_name: str, qubit_id: IndexedIdentifier) -> list[Quant modifiers=[], name=Identifier(name=gate_name), arguments=[], - qubits=_fresh_qubits(qubit_id), + qubits=fresh_qubits(qubit_id), ) ] @@ -932,7 +970,7 @@ def one_qubit_rotation_op( modifiers=[], name=Identifier(name=gate_name), arguments=[FloatLiteral(value=rotation)], - qubits=_fresh_qubits(qubit_id), + qubits=fresh_qubits(qubit_id), ) ] @@ -945,7 +983,7 @@ def two_qubit_gate_op( modifiers=[], name=Identifier(name=gate_name.lower()), arguments=[], - qubits=_fresh_qubits(qubit_id1, qubit_id2), + qubits=fresh_qubits(qubit_id1, qubit_id2), ) ] diff --git a/tests/qasm3/test_transformations.py b/tests/qasm3/test_transformations.py index c335b777..3f552533 100644 --- a/tests/qasm3/test_transformations.py +++ b/tests/qasm3/test_transformations.py @@ -18,11 +18,13 @@ """ 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 @@ -243,6 +245,39 @@ def test_rebase_emits_fresh_operand_nodes(): _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"""