Retain all deprecated Bit properties in QPY roundtrip (#9525) (#9603)

* Retain more deprecated Bit properties in QPY roundtrip

A major bugfix for QPY's handling of loose circuit bits in c0ac5fb
(gh-9095) caused the deprecated bit properties `index` and `register` to
be lost after the QPY roundtrip.  As far as Terra's data model is
concerned, the two circuits would still compare equal after this loss;
the output registers would still be equal to the inputs, and bit
equality is not considered directly since it general bit instances are
not expected to be equal between circuits.

Losing this information caused some downstream issues for the IBM
runtime, which was still relying on `Bit.index` working in some cases,
despite it issuing a deprecating warning since Terra 0.17 (April 2021).
While this can be corrected downstream, QPY can still do a better job of
roundtripping the deprecated information while it is still present.

The QPY format does not currently store enough information to
_completely_ roundtrip this information in cases that some but not all
owned bits from a register are present in the circuit.  (This partial
data is a decent part of the cause of the bugs that gh-9095 fixed.)
Since this is just in the support of deprecated functionality that
Terra's data model does not even require for circuit equality (QPY's
goal), it seems not worth it to produce a new QPY binary format version
to store this, when the deprecated properties being removed would
obsolete the format again immediately.

* Fix lint

* Correct deprecated bit information in QPY

This allows complete round-tripping of all the deprecated register+index
information in the `Bit` instances through QPY, restoring us to the
_intended_ behaviour before gh-9095.  The behaviour in the dumper before
that did not allow full reconstruction, because some of the information
was lost for bits that were encountered in more than one register.

This fixes the dumper to always output all the indexing information for
all bits, not to use `-1` whenever a bit _is_ in the circuit but has
previously been encountered.  The `standalone` field on a register is
sufficient to tell whether the bits contained in it should have their
"owner" information set; it's not possible (in valid Qiskit code) to
have a register that owns only _some_ of its bits.  To accomodate this,
the register reader now needs to be two-pass.

* Add deprecated-bit checks to backwards compatibility tests

* Rewrite release note

* Improve internal documentation

---------

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit b6aba59)

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

qiskit/qpy/__init__.py

@@ -796,10 +796,10 @@
 the register ``qr`` would be a standalone register. While something like::
 
     bits = [Qubit(), Qubit()]
-    qr = QuantumRegister(bits=bits)
-    qc = QuantumCircuit(bits=bits)
+    qr2 = QuantumRegister(bits=bits)
+    qc = QuantumCircuit(qr2)
 
-``qr`` would have ``standalone`` set to ``False``.
+``qr2`` would have ``standalone`` set to ``False``.
 
 
 .. _qpy_custom_definition:

qiskit/qpy/binary_io/circuits.py

@@ -701,7 +701,6 @@ def _write_calibrations(file_obj, calibrations, metadata_serializer):
 
 def _write_registers(file_obj, in_circ_regs, full_bits):
     bitmap = {bit: index for index, bit in enumerate(full_bits)}
-    processed_indices = set()
 
     out_circ_regs = set()
     for bit in full_bits:
@@ -727,12 +726,7 @@ def _write_registers(file_obj, in_circ_regs, full_bits):
             REGISTER_ARRAY_PACK = "!%sq" % reg.size
             bit_indices = []
             for bit in reg:
-                bit_index = bitmap.get(bit, -1)
-                if bit_index in processed_indices:
-                    bit_index = -1
-                if bit_index >= 0:
-                    processed_indices.add(bit_index)
-                bit_indices.append(bit_index)
+                bit_indices.append(bitmap.get(bit, -1))
             file_obj.write(struct.pack(REGISTER_ARRAY_PACK, *bit_indices))
 
     return len(in_circ_regs) + len(out_circ_regs)
@@ -842,30 +836,70 @@ def read_circuit(file_obj, version, metadata_deserializer=None):
     num_clbits = header["num_clbits"]
     num_registers = header["num_registers"]
     num_instructions = header["num_instructions"]
+    # `out_registers` is two "name: registter" maps segregated by type for the rest of QPY, and
+    # `all_registers` is the complete ordered list used to construct the `QuantumCircuit`.
     out_registers = {"q": {}, "c": {}}
-    circ = QuantumCircuit(
-        [Qubit() for _ in [None] * num_qubits],
-        [Clbit() for _ in [None] * num_clbits],
-        name=name,
-        global_phase=global_phase,
-        metadata=metadata,
-    )
+    all_registers = []
+    out_bits = {"q": [None] * num_qubits, "c": [None] * num_clbits}
     if num_registers > 0:
         if version < 4:
             registers = _read_registers(file_obj, num_registers)
         else:
             registers = _read_registers_v4(file_obj, num_registers)
-
-        for bit_type_label, reg_type in [("q", QuantumRegister), ("c", ClassicalRegister)]:
-            # Add quantum registers and bits
-            circuit_bits = {"q": circ.qubits, "c": circ.clbits}[bit_type_label]
-            for register_name, (_, indices, in_circuit) in registers[bit_type_label].items():
-                register = reg_type(
-                    name=register_name, bits=[circuit_bits[x] for x in indices if x >= 0]
-                )
-                if in_circuit:
-                    circ.add_register(register)
+        for bit_type_label, bit_type, reg_type in [
+            ("q", Qubit, QuantumRegister),
+            ("c", Clbit, ClassicalRegister),
+        ]:
+            # This does two passes through the registers. In the first, we're actually just
+            # constructing the `Bit` instances: any register that is `standalone` "owns" all its
+            # bits in the old Qiskit data model, so we have to construct those by creating the
+            # register and taking the bits from them.  That's the case even if that register isn't
+            # actually in the circuit, which is why we stored them (with `in_circuit=False`) in QPY.
+            #
+            # Since there's no guarantees in QPY about the ordering of registers, we have to pass
+            # through all registers to create the bits first, because we can't reliably know if a
+            # non-standalone register contains bits from a standalone one until we've seen all
+            # standalone registers.
+            typed_bits = out_bits[bit_type_label]
+            typed_registers = registers[bit_type_label]
+            for register_name, (standalone, indices, _incircuit) in typed_registers.items():
+                if not standalone:
+                    continue
+                register = reg_type(len(indices), register_name)
                 out_registers[bit_type_label][register_name] = register
+                for owned, index in zip(register, indices):
+                    # Negative indices are for bits that aren't in the circuit.
+                    if index >= 0:
+                        typed_bits[index] = owned
+            # Any remaining unset bits aren't owned, so we can construct them in the standard way.
+            typed_bits = [bit if bit is not None else bit_type() for bit in typed_bits]
+            # Finally _properly_ construct all the registers.  Bits can be in more than one
+            # register, including bits that are old-style "owned" by a register.
+            for register_name, (standalone, indices, in_circuit) in typed_registers.items():
+                if standalone:
+                    register = out_registers[bit_type_label][register_name]
+                else:
+                    register = reg_type(
+                        name=register_name,
+                        bits=[typed_bits[x] if x >= 0 else bit_type() for x in indices],
+                    )
+                    out_registers[bit_type_label][register_name] = register
+                if in_circuit:
+                    all_registers.append(register)
+            out_bits[bit_type_label] = typed_bits
+    else:
+        out_bits = {
+            "q": [Qubit() for _ in out_bits["q"]],
+            "c": [Clbit() for _ in out_bits["c"]],
+        }
+    circ = QuantumCircuit(
+        out_bits["q"],
+        out_bits["c"],
+        *all_registers,
+        name=name,
+        global_phase=global_phase,
+        metadata=metadata,
+    )
     custom_operations = _read_custom_operations(file_obj, version, vectors)
     for _instruction in range(num_instructions):
         _read_instruction(file_obj, circ, out_registers, custom_operations, version, vectors)

releasenotes/notes/fix-deprecated-bit-qpy-roundtrip-9a23a795aa677c71.yaml

@@ -0,0 +1,13 @@
+---
+fixes:
+  - |
+    The deprecated :class:`.Qubit` and :class:`.Clbit` properties :attr:`~.Qubit.register` and
+    :attr:`~.Qubit.index` will now be correctly round-tripped by QPY (:mod:`qiskit.qpy`) in all
+    valid usages of :class:`.QuantumRegister` and :class:`.ClassicalRegister`.  In earlier releases
+    in the Terra 0.23 series, this information would be lost.  In versions before 0.23.0, this
+    information was partially reconstructed but could be incorrect or produce invalid circuits for
+    certain register configurations.
+
+    The correct way to retrieve the index of a bit within a circuit, and any registers in that
+    circuit the bit is contained within is to call :meth:`.QuantumCircuit.find_bit`.  This method
+    will return the correct information in all versions of Terra since its addition in version 0.19.