Fix overriding qelib1.inc with custom gates

The bytecode interpreter assumed that all gates `qelib1.inc` would
always need to be appended to the gate list.  The inclusion of custom
instructions broke this assumption.  It was masked if _all_ the gates
were overridden, and no further user gates were defined, but failed in
other situations.

Author: jakelishman

docs/changelog.rst

@@ -8,6 +8,10 @@ Unreleased
 * Added some missing gates to :data:`.QISKIT_CUSTOM_INSTRUCTIONS`; Qiskit's legacy importer
   made rather a lot of changes to the file as presented in the paper!
 
+* Fixed incorrect gate creations when a strict subset of the ``qelib1.inc`` gates were overridden
+  with custom constructors, or if any user gates were defined after all every gate in that include
+  file was overridden.
+
 0.3.1 (2023-01-20)
 ==================
 

src-python/qiskit_qasm2/parse.py

@@ -236,7 +236,9 @@ def from_bytecode(bytecode, custom_instructions: Iterable[CustomInstruction]):
             # Including `qelib1.inc` is pretty much universal, and we treat its gates as having
             # special relationships to the Qiskit ones, so we don't actually parse it; we just
             # short-circuit to add its pre-calculated content to our state.
-            gates += QELIB1
+            (indices,) = op.operands
+            for index in indices:
+                gates.append(QELIB1[index])
         elif opcode == OpCode.DeclareGate:
             name, n_qubits = op.operands
             # This inner loop advances the iterator of the outer loop as well, since `bc` is a

src-rust/bytecode.rs

@@ -182,7 +182,7 @@ pub enum InternalBytecode {
         n_qubits: usize,
     },
     SpecialInclude {
-        name: String,
+        indices: Vec<usize>,
     },
 }
 
@@ -264,9 +264,9 @@ impl IntoPy<Bytecode> for InternalBytecode {
                 opcode: OpCode::DeclareOpaque,
                 operands: (name, n_qubits).into_py(py),
             },
-            InternalBytecode::SpecialInclude { name } => Bytecode {
+            InternalBytecode::SpecialInclude { indices } => Bytecode {
                 opcode: OpCode::SpecialInclude,
-                operands: (name,).into_py(py),
+                operands: (indices,).into_py(py),
             },
         }
     }

src-rust/parse.rs

@@ -13,11 +13,33 @@ use crate::CustomInstruction;
 
 /// The number of gates that are built in to the OpenQASM 2 language.  This is U and CX.
 const N_BUILTIN_GATES: usize = 2;
-/// The number of gates that 'qelib1.inc' defines.  For efficiency, this parser doesn't actually
-/// parse the include file itself, since its contents are fixed by the OpenQASM 2 arXiv paper,
-/// which this parser follows to the letter.  Instead, we just have it hardcoded as a special
-/// import that happens quickly when the relevant 'include' statement is seen.
-const N_QELIB1_GATES: usize = 23;
+/// The "qelib1.inc" special include.  The elements of the tuple are the gate name, the number of
+/// parameters it takes, and the number of qubits it acts on.
+const QELIB1: [(&str, usize, usize); 23] = [
+    ("u3", 3, 1),
+    ("u2", 2, 1),
+    ("u1", 1, 1),
+    ("cx", 0, 2),
+    ("id", 0, 1),
+    ("x", 0, 1),
+    ("y", 0, 1),
+    ("z", 0, 1),
+    ("h", 0, 1),
+    ("s", 0, 1),
+    ("sdg", 0, 1),
+    ("t", 0, 1),
+    ("tdg", 0, 1),
+    ("rx", 1, 1),
+    ("ry", 1, 1),
+    ("rz", 1, 1),
+    ("cz", 0, 2),
+    ("cy", 0, 2),
+    ("ch", 0, 2),
+    ("ccx", 0, 3),
+    ("crz", 1, 2),
+    ("cu1", 1, 2),
+    ("cu3", 3, 2),
+];
 
 /// A symbol in the global symbol table.  Parameters and individual qubits can't be in the global
 /// symbol table, as there is no way for them to be defined.
@@ -132,7 +154,7 @@ impl State {
             // custom instructions, but this is small-potatoes allocation and we'd rather not have
             // to reallocate.
             symbols: HashMap::with_capacity(
-                N_BUILTIN_GATES + N_QELIB1_GATES + custom_instructions.len(),
+                N_BUILTIN_GATES + QELIB1.len() + custom_instructions.len(),
             ),
             gate_symbols: HashMap::new(),
             n_qubits: 0,
@@ -1279,8 +1301,14 @@ impl State {
         self.expect(TokenType::Semicolon, "';'", &include_token)?;
         let filename = filename_token.filename(&self.context);
         if filename == "qelib1.inc" {
-            self.include_qelib1(&include_token)?;
-            bc.push(Some(InternalBytecode::SpecialInclude { name: filename }));
+            self.symbols.reserve(QELIB1.len());
+            let mut indices = Vec::with_capacity(QELIB1.len());
+            for (i, (name, n_params, n_qubits)) in QELIB1.iter().enumerate() {
+                if self.define_gate(&include_token, name.to_string(), *n_params, *n_qubits)? {
+                    indices.push(i);
+                }
+            }
+            bc.push(Some(InternalBytecode::SpecialInclude { indices }));
             Ok(1)
         } else {
             let base_filename = std::path::PathBuf::from(&filename);
@@ -1308,36 +1336,6 @@ impl State {
         }
     }
 
-    /// Update the parser state with the built-in version of the `qelib1.inc` include file.  This
-    /// is precisely as the file is described in the arXiv paper.
-    fn include_qelib1(&mut self, include: &Token) -> Result<(), String> {
-        self.symbols.reserve(N_QELIB1_GATES);
-        self.define_gate(include, "u3".to_owned(), 3, 1)?;
-        self.define_gate(include, "u2".to_owned(), 2, 1)?;
-        self.define_gate(include, "u1".to_owned(), 1, 1)?;
-        self.define_gate(include, "cx".to_owned(), 0, 2)?;
-        self.define_gate(include, "id".to_owned(), 0, 1)?;
-        self.define_gate(include, "x".to_owned(), 0, 1)?;
-        self.define_gate(include, "y".to_owned(), 0, 1)?;
-        self.define_gate(include, "z".to_owned(), 0, 1)?;
-        self.define_gate(include, "h".to_owned(), 0, 1)?;
-        self.define_gate(include, "s".to_owned(), 0, 1)?;
-        self.define_gate(include, "sdg".to_owned(), 0, 1)?;
-        self.define_gate(include, "t".to_owned(), 0, 1)?;
-        self.define_gate(include, "tdg".to_owned(), 0, 1)?;
-        self.define_gate(include, "rx".to_owned(), 1, 1)?;
-        self.define_gate(include, "ry".to_owned(), 1, 1)?;
-        self.define_gate(include, "rz".to_owned(), 1, 1)?;
-        self.define_gate(include, "cz".to_owned(), 0, 2)?;
-        self.define_gate(include, "cy".to_owned(), 0, 2)?;
-        self.define_gate(include, "ch".to_owned(), 0, 2)?;
-        self.define_gate(include, "ccx".to_owned(), 0, 3)?;
-        self.define_gate(include, "crz".to_owned(), 1, 2)?;
-        self.define_gate(include, "cu1".to_owned(), 1, 2)?;
-        self.define_gate(include, "cu3".to_owned(), 3, 2)?;
-        Ok(())
-    }
-
     /// Update the parser state with the definition of a particular gate.  This does not emit any
     /// bytecode because not all gate definitions need something passing to Python.  For example,
     /// the Python parser initialises its state including the built-in gates `U` and `CX`, and
@@ -1348,7 +1346,7 @@ impl State {
         name: String,
         n_params: usize,
         n_qubits: usize,
-    ) -> Result<(), String> {
+    ) -> Result<bool, String> {
         match self.symbols.get_mut(&name) {
             None => {
                 self.symbols.insert(
@@ -1362,7 +1360,7 @@ impl State {
                     },
                 );
                 self.n_gates += 1;
-                Ok(())
+                Ok(true)
             }
             Some(GlobalSymbol::Gate {
                 n_params: defined_n_params,
@@ -1402,7 +1400,7 @@ impl State {
                     ))
                 } else {
                     *defined = true;
-                    Ok(())
+                    Ok(false)
                 }
             }
             _ => Err(message_from_token(

tests/test_structure.py

@@ -1195,6 +1195,85 @@ def test_qelib1_include_overridden(self):
         from_qiskit = QuantumCircuit.from_qasm_str(program)
         assert parsed == from_qiskit
 
+    def test_qelib1_sparse_overrides(self):
+        """Test that the qelib1 special import still works as expected when a couple of gates in the
+        middle of it are custom.  As long as qelib1 is handled specially, there is a risk that this
+        handling will break in weird ways when custom instructions overlap it."""
+        program = """
+            include "qelib1.inc";
+            qreg q[3];
+            u3(0.5, 0.25, 0.125) q[0];
+            u2(0.5, 0.25) q[0];
+            u1(0.5) q[0];
+            cx q[0], q[1];
+            id q[0];
+            x q[0];
+            y q[0];
+            z q[0];
+            h q[0];
+            s q[0];
+            sdg q[0];
+            t q[0];
+            tdg q[0];
+            rx(0.5) q[0];
+            ry(0.5) q[0];
+            rz(0.5) q[0];
+            cz q[0], q[1];
+            cy q[0], q[1];
+            ch q[0], q[1];
+            ccx q[0], q[1], q[2];
+            crz(0.5) q[0], q[1];
+            cu1(0.5) q[0], q[1];
+            cu3(0.5, 0.25, 0.125) q[0], q[1];
+        """
+        parsed = qiskit_qasm2.loads(
+            program,
+            custom_instructions=[
+                qiskit_qasm2.CustomInstruction("id", 0, 1, lib.IGate),
+                qiskit_qasm2.CustomInstruction("h", 0, 1, lib.HGate),
+                qiskit_qasm2.CustomInstruction("crz", 1, 2, lib.CRZGate),
+            ],
+        )
+        qc = QuantumCircuit(QuantumRegister(3, "q"))
+        qc.append(lib.U3Gate(0.5, 0.25, 0.125), [0])
+        qc.append(lib.U2Gate(0.5, 0.25), [0])
+        qc.append(lib.U1Gate(0.5), [0])
+        qc.append(lib.CXGate(), [0, 1])
+        qc.append(lib.IGate(), [0])
+        qc.append(lib.XGate(), [0])
+        qc.append(lib.YGate(), [0])
+        qc.append(lib.ZGate(), [0])
+        qc.append(lib.HGate(), [0])
+        qc.append(lib.SGate(), [0])
+        qc.append(lib.SdgGate(), [0])
+        qc.append(lib.TGate(), [0])
+        qc.append(lib.TdgGate(), [0])
+        qc.append(lib.RXGate(0.5), [0])
+        qc.append(lib.RYGate(0.5), [0])
+        qc.append(lib.RZGate(0.5), [0])
+        qc.append(lib.CZGate(), [0, 1])
+        qc.append(lib.CYGate(), [0, 1])
+        qc.append(lib.CHGate(), [0, 1])
+        qc.append(lib.CCXGate(), [0, 1, 2])
+        qc.append(lib.CRZGate(0.5), [0, 1])
+        qc.append(lib.CU1Gate(0.5), [0, 1])
+        qc.append(lib.CU3Gate(0.5, 0.25, 0.125), [0, 1])
+        assert parsed == qc
+
+    def test_user_gate_after_overidden_qelib1(self):
+        program = """
+            include "qelib1.inc";
+            qreg q[1];
+            opaque my_gate q;
+            my_gate q[0];
+        """
+        parsed = qiskit_qasm2.loads(
+            program, custom_instructions=qiskit_qasm2.QISKIT_CUSTOM_INSTRUCTIONS
+        )
+        qc = QuantumCircuit(QuantumRegister(1, "q"))
+        qc.append(Gate("my_gate", 1, []), [0])
+        assert parsed == qc
+
     def test_qiskit_extra_builtins(self):
         program = """
             qreg q[5];
@@ -1245,7 +1324,7 @@ def test_qiskit_extra_builtins(self):
         # is and it has no Qiskit equivalent, so we'll just test that it using it doesn't produce an
         # error.
         parsed = qiskit_qasm2.loads(
-            'qreg q[1]; u0(1) q[0];', custom_instructions=qiskit_qasm2.QISKIT_CUSTOM_INSTRUCTIONS
+            "qreg q[1]; u0(1) q[0];", custom_instructions=qiskit_qasm2.QISKIT_CUSTOM_INSTRUCTIONS
         )
         assert parsed.data[0].operation.name == "u0"