Fix handling of custom gates named unitary in Split2qUnitaries (Qiskit#14108)

Normally creating a custom gate class that overloads the name of a
Qiskit defined operation is not valid and not allowed. The names have
meaning and are often used as identifiers and this overloading the name
will prevent Qiskit from correctly identifying an operation. However as
was discovered in Qiskit#14103 there are some paths available around
serialization and I/O where Qiskit does this itself. For example, qasm
(both 2 and 3) is a lossy serialization format and qasm2 doesn't have a
representation of a UnitaryGate. So when the qasm2 exporter encounteres
a `UnitaryGate` it is serialized as a custom gate definition with the
name "unitary" in the output qasm2 and the definition is a decomposition
of the unitary from the `UnitaryGate`. When that qasm2 program is
subsequently deserialized by qiskit parser the custom gate named "unitary"
is added as a `_DefinedGate` subclass which includes an `__array__`
implementation which computes the unitary from the definition using
the quantum info Operator class. This makes the custom gate parsed from
qasm2 look like a `UnitaryGate` despite not actually one so this is
typically fine for most use cases. However, if you then qpy serialize
the circuit that contains a custom `_DefinedGate` instance that's named
"unitary", QPY doesn't understand what `_DefinedGate` is and assumes that
it can not fully recreate it because it's outside the standard Qiskit data
model. In these cases qpy serializes the fields from the standard data
model defined in Qiskit which it knows about: name, params, definition, etc
and encodes that as a custom gate. When deserializing that custom gate it
populates the standard fields, but anything defined outside of that is
lost through the serialization because it's defined outside of Qiskit.
This means the custom `__array__` implementation is missing from the
qpy round-tripped, previously qasm2 round-tripped, `UnitaryGate`
instance in a circuit. This was then causing the transpiler to panic
(which is a hard crash) in the Split2QUnitaries pass because when it saw
a gate named "unitary" it rightly assumed it was a `UnitaryGate` and
there is always a matrix available (because a `UnitaryGate` is a gate
defined solely by it's matrix). This is an example of why overloading
gate names is typically not allowed and is considered invalid usage of
Qiskit. However, in this case since the resulting gate came from valid
use of Qiskit's API trying to ensure the circuit doesn't hard crash the
transpiler is correct here because it's a result in internal
inconsistencies in Qiskit. Treating the twice round-tripped UnitaryGate
as the original unitary gate is not possible because of the loss of
information via serialization, but qiskit should at least not have
an internal panic!() because of this usage.

Arguably we could fix this in QPY by adding either a matrix field to the
format specification for custom gates, or adding handling for
`_DefinedGate` to qpy. But either will require a new format version
which is not eligble for backport to 1.4.x. So this commit fixes this
issue by updating the transpiler pass in question to just treat the
matrix of a gate named unitary as fallible, and we just ignore the
custom gate named unitary that's missing a matrix.

This already been fixed on main and stable/2.0 as a side effect of
PR Qiskit#13765 which made the typing around UnitaryGate in rust more
explicit. So the custom deserialized gate from QPY which doesn't have a
matrix defined is never treated as a unitary gate and even through the
qasm2 round trip followed by a qpy round trip we don't encounter any
issues.

Fixes Qiskit#14103

Author: mtreinish

Cargo.lock

@@ -42,10 +42,10 @@ pub fn split_2q_unitaries(
             if qubits.len() != 2 || inst.op.name() != "unitary" {
                 continue;
             }
-            let matrix = inst
-                .op
-                .matrix(inst.params_view())
-                .expect("'unitary' gates should always have a matrix form");
+            let matrix = match inst.op.matrix(inst.params_view()) {
+                Some(mat) => mat,
+                None => continue,
+            };
             let decomp = TwoQubitWeylDecomposition::new_inner(
                 matrix.view(),
                 Some(requested_fidelity),

crates/accelerate/src/split_2q_unitaries.rs

@@ -13,6 +13,7 @@
 """
 Tests for the Split2QUnitaries transpiler pass.
 """
+import io
 from math import pi
 from test import QiskitTestCase
 import numpy as np
@@ -26,6 +27,7 @@
 from qiskit.quantum_info.operators.predicates import matrix_equal
 from qiskit.transpiler.passes import Collect2qBlocks, ConsolidateBlocks
 from qiskit.transpiler.passes.optimization.split_2q_unitaries import Split2QUnitaries
+from qiskit import qpy
 
 
 class TestSplit2QUnitaries(QiskitTestCase):
@@ -266,3 +268,41 @@ def to_matrix(self):
         no_split = Split2QUnitaries()(qc)
 
         self.assertDictEqual({"mygate": 1}, no_split.count_ops())
+
+    def test_overloaded_unitary_name_from_qasm(self):
+        """Test that an otherwise invalid custom gate named unitary created via valid Qiskit
+        API calls doesn't crash the pass
+
+        See: https://github.com/Qiskit/qiskit/issues/14103
+        """
+
+        qasm_str = """OPENQASM 2.0;
+        include "qelib1.inc";
+        gate cx_o0 q0,q1 { x q0; cx q0,q1; x q0; }
+        gate unitary q0,q1 { u(pi/2,0.6763483147328913,0) q0; u(1.6719020266110614,-pi/2,0) q1; cx q0,q1; u(pi,-0.9111063207475532,3.1249343449042435) q0; u(1.6719020266110616,0,-pi/2) q1; }
+        qreg v__0__0_[0];
+        qreg l___0__0___1_[2];
+        qreg l___0__0___2_[2];
+        qreg v__0__1_[0];
+        qreg l___0__1___1_[2];
+        qreg v__1__0_[0];
+        qreg l___1__0___1_[2];
+        qreg l___1__0___2_[2];
+        qreg v__1__1_[0];
+        qreg l___1__1___1_[2];
+        creg meas[12];
+        unitary l___0__0___2_[0],l___0__1___1_[0];
+        """
+        # Parse qasm string to get custom unitary gate that's not a UnitaryGate (but has matrix defined)
+        qc = QuantumCircuit.from_qasm_str(qasm_str)
+        # Roundtrip QPY to lose the custom matrix definition from qasm2
+        # unitary
+        with io.BytesIO() as buf:
+            qpy.dump(qc, buf)
+            # Rewind back to the beginning of the "file".
+            buf.seek(0)
+            qc = qpy.load(buf)[0]
+        # Run split unitaries pass with custom gate named unitary that has
+        # no matrix.
+        res = Split2QUnitaries()(qc)
+        self.assertEqual(res, qc)