Fix OpenQASM 2 gate definitions following a shadowed built-in gate (

#13340) (#13352)

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 9a1d8d3)

Co-authored-by: Jake Lishman <[email protected]>

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),

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 <https://github.com/Qiskit/qiskit/issues/13339>`__ for more detail.

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):