From d1cd43dc3aee70713f047004754bf7fad7b639d1 Mon Sep 17 00:00:00 2001 From: Jake Lishman Date: Tue, 22 Oct 2024 13:59:17 +0100 Subject: [PATCH] Fix OpenQASM 2 `gate` definitions following a shadowed built-in `gate` (#13340) Previously, when given an OpenQASM 2 program such as OPENQASM 2.0; gate builtin a { U(0, 0, 0) a; } gate not_builtin a { U(pi, pi, pi) a; } qreg q[1]; not_builtin q[0]; and a set of `custom_instructions` including a `builtin=True` definition for `builtin` (but not for `not_builtin`), the Rust and Python sides would get themselves out-of-sync and output a gate that matched a prior definition, rather than `not_builtin`. This was because the Rust side was still issuing `DefineGate` bytecode instructions, even for gates whose OpenQASM 2 definitions should have been ignored, so Python-space thought there were more gates than Rust-space thought there were. (cherry picked from commit 9a1d8d3aeaffff7e75dd3106b0317150e838aa53) --- crates/qasm2/src/parse.rs | 23 ++++++++--- .../qasm2-builtin-gate-d80c2868cdf5f958.yaml | 7 ++++ test/python/qasm2/test_structure.py | 41 +++++++++++++++++++ 3 files changed, 66 insertions(+), 5 deletions(-) create mode 100644 releasenotes/notes/qasm2-builtin-gate-d80c2868cdf5f958.yaml diff --git a/crates/qasm2/src/parse.rs b/crates/qasm2/src/parse.rs index 36e2a49f669c..ec8c61152bab 100644 --- a/crates/qasm2/src/parse.rs +++ b/crates/qasm2/src/parse.rs @@ -827,8 +827,16 @@ impl State { } bc.push(Some(InternalBytecode::EndDeclareGate {})); self.gate_symbols.clear(); - self.define_gate(Some(&gate_token), name, num_params, num_qubits)?; - Ok(statements + 2) + let num_bytecode = statements + 2; + if self.define_gate(Some(&gate_token), name, num_params, num_qubits)? { + Ok(num_bytecode) + } else { + // The gate was built-in, so we don't actually need to emit the bytecode. This is + // uncommon, so it doesn't matter too much that we throw away allocation work we did - + // it still helps that we verified that the gate body was valid OpenQASM 2. + bc.truncate(bc.len() - num_bytecode); + Ok(0) + } } /// Parse an `opaque` statement. This assumes that the `opaque` token is still in the token @@ -1634,6 +1642,9 @@ impl State { /// bytecode because not all gate definitions need something passing to Python. For example, /// the Python parser initializes its state including the built-in gates `U` and `CX`, and /// handles the `qelib1.inc` include specially as well. + /// + /// Returns whether the gate needs to be defined in Python space (`true`) or if it was some sort + /// of built-in that doesn't need the definition (`false`). fn define_gate( &mut self, owner: Option<&Token>, @@ -1685,12 +1696,14 @@ impl State { } match self.symbols.get(&name) { None => { + // The gate wasn't a built-in, so we need to move the symbol in, but we don't + // need to increment the number of gates because it's already got a gate ID + // assigned. self.symbols.insert(name, symbol.into()); - self.num_gates += 1; - Ok(true) + Ok(false) } Some(GlobalSymbol::Gate { .. }) => { - self.symbols.insert(name, symbol.into()); + // The gate was built-in and we can ignore the new definition (it's the same). Ok(false) } _ => already_defined(self, name), diff --git a/releasenotes/notes/qasm2-builtin-gate-d80c2868cdf5f958.yaml b/releasenotes/notes/qasm2-builtin-gate-d80c2868cdf5f958.yaml new file mode 100644 index 000000000000..bfcf867f2a3b --- /dev/null +++ b/releasenotes/notes/qasm2-builtin-gate-d80c2868cdf5f958.yaml @@ -0,0 +1,7 @@ +--- +fixes: + - | + The OpenQASM 2 importer previously would output incorrect :class:`.Gate` instances for gate + calls referring to a ``gate`` definition that followed a prior ``gate`` definition that was + being treated as a built-in operation by a :class:`~.qasm2.CustomInstruction`. See + `#13339 `__ for more detail. diff --git a/test/python/qasm2/test_structure.py b/test/python/qasm2/test_structure.py index d963eea7e255..a5e5bbd77329 100644 --- a/test/python/qasm2/test_structure.py +++ b/test/python/qasm2/test_structure.py @@ -1648,6 +1648,47 @@ def __init__(self, a): qc.append(MyGate(0.5), [0]) self.assertEqual(parsed, qc) + def test_compatible_definition_of_builtin_is_ignored(self): + program = """ + qreg q[1]; + gate my_gate a { U(0, 0, 0) a; } + my_gate q[0]; + """ + + class MyGate(Gate): + def __init__(self): + super().__init__("my_gate", 1, []) + + def _define(self): + self._definition = QuantumCircuit(1) + self._definition.z(0) + + parsed = qiskit.qasm2.loads( + program, custom_instructions=[qiskit.qasm2.CustomInstruction("my_gate", 0, 1, MyGate)] + ) + self.assertEqual(parsed.data[0].operation.definition, MyGate().definition) + + def test_gates_defined_after_a_builtin_align(self): + """It's easy to get out of sync between the Rust-space and Python-space components when + ``builtin=True``. See https://github.com/Qiskit/qiskit/issues/13339.""" + program = """ + OPENQASM 2.0; + gate first a { U(0, 0, 0) a; } + gate second a { U(pi, pi, pi) a; } + + qreg q[1]; + first q[0]; + second q[0]; + """ + custom = qiskit.qasm2.CustomInstruction("first", 0, 1, lib.XGate, builtin=True) + parsed = qiskit.qasm2.loads(program, custom_instructions=[custom]) + # Provided definitions for built-in gates are ignored, so it should be an XGate directly. + self.assertEqual(parsed.data[0].operation, lib.XGate()) + self.assertEqual(parsed.data[1].operation.name, "second") + defn = parsed.data[1].operation.definition.copy_empty_like() + defn.u(math.pi, math.pi, math.pi, 0) + self.assertEqual(parsed.data[1].operation.definition, defn) + class TestCustomClassical(QiskitTestCase): def test_qiskit_extensions(self):