QPY 18: give the register/clbit payload a tag byte instead of a "\x00"-prefixed string

crates/qpy/src/expr.rs

@@ -17,11 +17,11 @@ use crate::formats::{
     ExpressionVarElementPack, ExpressionVarRegisterPack,
 };
 use crate::value::{
-    QPYReadData, QPYWriteData, pack_biguint, pack_duration, unpack_biguint, unpack_duration,
+    QPYReadData, QPYWriteData, clbit_at, clbit_index, creg_by_name, pack_biguint, pack_duration,
+    unpack_biguint, unpack_duration,
 };
 use binrw::{BinRead, BinResult, BinWrite, Endian, Error};
 use num_bigint::BigUint;
-use qiskit_circuit::Clbit;
 use qiskit_circuit::classical::expr::{
     Binary, BinaryOp, Cast, Expr, Index, Unary, UnaryOp, Value, Var,
 };
@@ -109,16 +109,7 @@ pub(crate) fn pack_expression_var(
     let (ty, value_pack) = match var {
         Var::Bit { bit } => (
             &Type::Bool,
-            ExpressionVarElementPack::Clbit(
-                qpy_data
-                    .circuit_data
-                    .clbits()
-                    .find(bit)
-                    .ok_or_else(|| {
-                        QpyError::InvalidBit(format!("Could not find bit {:?} in circuit", bit))
-                    })?
-                    .0,
-            ),
+            ExpressionVarElementPack::Clbit(clbit_index(bit, qpy_data)?),
         ),
         Var::Register { register, ty } => (
             ty,
@@ -152,24 +143,12 @@ pub(crate) fn unpack_expression_var(
     let ty = unpack_expression_type(var_type_pack);
     match var_element_pack {
         ExpressionVarElementPack::Clbit(index) => Ok(Var::Bit {
-            bit: qpy_data
-                .circuit_data
-                .clbits()
-                .get(Clbit(index))
-                .ok_or_else(|| QpyError::InvalidBit("Clbit not found in circuit data".to_string()))?
-                .clone(),
+            bit: clbit_at(index, qpy_data)?,
         }),
         ExpressionVarElementPack::Register(packed_register) => Ok(Var::Register {
-            register: qpy_data
-                .circuit_data
-                .cregs_data()
-                .get(packed_register.name.as_str())
-                .ok_or_else(|| {
-                    QpyError::InvalidRegister("Register not found in circuit data".to_string())
-                })?
-                .clone(),
+            register: creg_by_name(&packed_register.name, qpy_data)?,
             ty,
-        }), // TODO: can we avoid cloning?
+        }),
         ExpressionVarElementPack::Uuid(key) => {
             let var = qpy_data.standalone_vars.get(&key).ok_or_else(|| {
                 QpyError::InvalidParameter("Standalone var not found in qpy data".to_string())

crates/qpy/src/formats.rs

@@ -254,7 +254,8 @@ pub struct RegisterV4Pack {
 // 1) None.
 // 2) Two-tuple: a tuple of the form (register, target) where the register value should be compared with the target.
 // In this case the target is a python int, represented in rust as BigUInt, but in python qpy it was saved using i64 so we keep it for now.
-// Note that we also use (clbit, bool_target) as two tuple, where the clbit is encoded using the "\x00" hack that can be seen in ParamRegisterValue
+// Note that we also use (clbit, bool_target) as two tuple; register and clbit alike are encoded as
+// a register payload, see `ParamRegisterPack` below
 // 3) Expression
 // In the two-tuple representation, the target value is stored in the `value` field and the number of bytes in the serialized registered are stored in the
 // `register_size` fields. Both are unused in the other cases, making the packing and decoding of this struct rather non-uniform.
@@ -286,15 +287,35 @@ impl TryFrom<u8> for ConditionType {
     }
 }
 
-// register SHOULD be a string, but since we encode some registers starting with "\x00" they are rendered illegal
-// we should probably change this in future versions to support magic numbers (TODO: change in QPY18?)
+// The condition's register is carried as an opaque blob because its encoding depends on the QPY
+// version: up to 17 it is the string hack described on `ParamRegisterPack`, from 18 it is that
+// tagged struct.  `value::load_param_register_value` decodes it.
 #[derive(Debug)]
 pub enum ConditionData {
     None,
     Register(Bytes),
     Expression(GenericDataPack),
 }
 
+/// A `Register` payload, which is either a whole `ClassicalRegister` or a single `Clbit`.  It
+#[binrw]
+#[brw(big)]
+#[derive(Debug)]
+pub enum ParamRegisterPack {
+    /// A classical register, identified by name.  The name runs to the end of the payload, whose
+    /// length the enclosing field already carries (`condition_register_size` for a condition, the
+    /// `INSTRUCTION_PARAM` header's `size` for a parameter), so it needs no length of its own.
+    #[brw(magic = 1u8)]
+    Register {
+        #[br(parse_with = binrw::helpers::until_eof, try_map = String::from_utf8)]
+        #[bw(map = |name| name.as_bytes())]
+        name: String,
+    },
+    /// A single clbit, identified by its index in the circuit's clbit list.
+    #[brw(magic = 0u8)]
+    Clbit { index: u32 },
+}
+
 // most of the data here is "virtual" in the sense that is is not stored as-is
 // we use custom reader/writer to enable spreading the data to its relevant places in the qpy instruction
 // in newer versions of qpy it may be better to store all the data consecutively

crates/qpy/src/value.rs

@@ -991,10 +991,10 @@ pub(crate) fn unpack_duration(duration_pack: DurationPack) -> Duration {
 }
 
 // due to historical reasons, the treatment of instructions params which are registers/clbits is a little strange
-// When a register is stored as an instruction param, it is serialized compactly
-// For a classical register its name is saved as a string; for a clbit
-// its index in the full clbit list is converted into a string, with 0x00 appended at the start
-// to differentiate from the register case
+// A `Register` value stored inside an instruction is either a whole classical register or a single
+// clbit.  From QPY 18 the two are told apart by a tag byte (`formats::ParamRegisterPack`); up to
+// QPY 17 they shared one untyped string, a register being its bare name and a clbit being 0x00
+// followed by its index in ASCII digits.
 
 #[derive(Debug, PartialEq, Clone)]
 pub enum ParamRegisterValue {
@@ -1006,17 +1006,22 @@ pub(crate) fn serialize_param_register_value(
     value: &ParamRegisterValue,
     qpy_data: &QPYWriteData,
 ) -> Result<Bytes, QpyError> {
+    if qpy_data.version >= 18 {
+        let pack = match value {
+            ParamRegisterValue::Register(register) => formats::ParamRegisterPack::Register {
+                name: register.name().to_string(),
+            },
+            ParamRegisterValue::ShareableClbit(clbit) => formats::ParamRegisterPack::Clbit {
+                index: clbit_index(clbit, qpy_data)?,
+            },
+        };
+        return serialize(&pack);
+    }
+    // QPY <= 17: the untyped string form.
     match value {
         ParamRegisterValue::Register(register) => Ok(register.name().into()),
         ParamRegisterValue::ShareableClbit(clbit) => {
-            let name = qpy_data
-                .circuit_data
-                .clbits()
-                .find(clbit)
-                .ok_or_else(|| QpyError::InvalidBit("clbit not found".to_string()))?
-                .0
-                .to_string();
-            // this is the part where we get hack-y
+            let name = clbit_index(clbit, qpy_data)?.to_string();
             let mut bytes: Bytes = Bytes(Vec::with_capacity(name.len() + 1));
             bytes.push(0u8);
             bytes.extend_from_slice(name.as_bytes());
@@ -1029,38 +1034,77 @@ pub(crate) fn load_param_register_value(
     bytes: &Bytes,
     qpy_data: &mut QPYReadData,
 ) -> Result<ParamRegisterValue, QpyError> {
-    // If register name prefixed with null character it's a clbit index for single bit condition.
+    if qpy_data.version >= 18 {
+        let (pack, _) = deserialize::<formats::ParamRegisterPack>(bytes)?;
+        return match pack {
+            formats::ParamRegisterPack::Register { name } => {
+                Ok(ParamRegisterValue::Register(creg_by_name(&name, qpy_data)?))
+            }
+            formats::ParamRegisterPack::Clbit { index } => Ok(ParamRegisterValue::ShareableClbit(
+                clbit_at(index, qpy_data)?,
+            )),
+        };
+    }
+    // QPY <= 17: a leading null character means the rest is a clbit index in ASCII digits.
     if bytes.is_empty() {
         return Err(QpyError::InvalidRegister(
             "Failed to load register - name missing".to_string(),
         ));
     }
     if bytes[0] == 0u8 {
-        let index = Clbit(std::str::from_utf8(&bytes[1..])?.parse().map_err(
-            |e: std::num::ParseIntError| {
-                QpyError::ConversionError(format!("Failed to parse clbit index: {}", e))
-            },
-        )?);
-        match qpy_data.circuit_data.clbits().get(index) {
-            Some(shareable_clbit) => {
-                Ok(ParamRegisterValue::ShareableClbit(shareable_clbit.clone()))
-            }
-            None => Err(QpyError::InvalidBit(format!(
-                "Could not find clbit {:?}",
-                index
-            ))),
-        }
+        let index: u32 =
+            std::str::from_utf8(&bytes[1..])?
+                .parse()
+                .map_err(|e: std::num::ParseIntError| {
+                    QpyError::ConversionError(format!("Failed to parse clbit index: {e}"))
+                })?;
+        Ok(ParamRegisterValue::ShareableClbit(clbit_at(
+            index, qpy_data,
+        )?))
     } else {
         // `bytes` has the register name
         let name = std::str::from_utf8(bytes)?;
-        for creg in qpy_data.circuit_data.cregs() {
-            if creg.name() == name {
-                return Ok(ParamRegisterValue::Register(creg.clone()));
-            }
-        }
-        Err(QpyError::InvalidRegister(format!(
-            "Could not find classical register {:?}",
-            name
-        )))
+        Ok(ParamRegisterValue::Register(creg_by_name(name, qpy_data)?))
     }
 }
+
+/// Position of `clbit` in the circuit being written, which is how the format refers to a bit.
+///
+/// A thin wrapper over [`CircuitData::clbit_index`] that turns the `None` into the QPY error, so
+/// every site that needs a bit index reports the same thing.
+pub(crate) fn clbit_index(
+    clbit: &ShareableClbit,
+    qpy_data: &QPYWriteData,
+) -> Result<u32, QpyError> {
+    qpy_data
+        .circuit_data
+        .clbit_index(clbit)
+        .ok_or_else(|| QpyError::InvalidBit(format!("Could not find clbit {clbit:?} in circuit")))
+}
+
+/// The clbit at `index` in the circuit being read.
+pub(crate) fn clbit_at(index: u32, qpy_data: &QPYReadData) -> Result<ShareableClbit, QpyError> {
+    qpy_data
+        .circuit_data
+        .clbits()
+        .get(Clbit(index))
+        .cloned()
+        .ok_or_else(|| QpyError::InvalidBit(format!("Could not find clbit {index} in circuit")))
+}
+
+/// The classical register called `name` in the circuit being read.
+///
+/// Uses the name-keyed [`CircuitData::cregs_data`] map rather than scanning `cregs()`.
+pub(crate) fn creg_by_name(
+    name: &str,
+    qpy_data: &QPYReadData,
+) -> Result<ClassicalRegister, QpyError> {
+    qpy_data
+        .circuit_data
+        .cregs_data()
+        .get(name)
+        .cloned()
+        .ok_or_else(|| {
+            QpyError::InvalidRegister(format!("Could not find classical register {name:?}"))
+        })
+}

qiskit/qpy/__init__.py

@@ -476,6 +476,33 @@ def open(*args):
 to big-endian byte order, consistent with the rest of the QPY specification. In versions
 1–17 these were mistakenly written in little-endian.
 
+New ParamRegisterPack
+~~~~~~~~~~~~~~~~~~~~~
+Version 18 replaces the encoding of a `Register` payload, which stores either a whole
+:class:`.ClassicalRegister` or a single :class:`.Clbit`.  It appears as an instruction's condition
+(see :ref:`qpy_instructions`) and as an ``INSTRUCTION_PARAM`` of type ``'R'``.
+
+Up to :ref:`version 17 <qpy_version_17>` both cases shared one untyped utf8 string: a register was
+its bare name, while a clbit was a null character ``"\\x00"`` followed by the bit's index in the
+circuit *written out as decimal digits*.  From version 18 the payload begins with a tag byte
+identifying which of the two it is:
+
+.. code-block:: c
+
+    struct {                    // classical register, tag == 1
+      uint8_t kind;
+      char    name[];           // to the end of the payload
+    }
+
+    struct {                    // single clbit, tag == 0
+      uint8_t  kind;
+      uint32_t index;           // index of the bit in the circuit
+    }
+
+The register name needs no length of its own because the enclosing field already delimits the
+payload: ``conditional_reg_name_size`` for a condition, and the ``INSTRUCTION_PARAM`` header's
+``size`` for a parameter.
+
 .. _qpy_version_17:
 
 Version 17
@@ -2123,6 +2150,10 @@ class if it's defined in Qiskit. Otherwise it falls back to the custom
 integer representing the classical bit index in the circuit that the condition
 is on.
 
+.. versionchanged:: QPY 18
+    This payload is a tagged struct rather than a utf8 string; see
+    :ref:`qpy_version_18`.
+
 This is immediately followed by the INSTRUCTION_ARG structs for the list of
 arguments of that instruction. These are in the order of all quantum arguments
 (there are num_qargs of these) followed by all classical arguments (num_cargs