Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Stretch] Support Duration type in classical expressions. #13844

Merged
merged 96 commits into from
Mar 3, 2025
Merged
Show file tree
Hide file tree
Changes from 93 commits
Commits
Show all changes
96 commits
Select commit Hold shift + click to select a range
af4ba52
WIP
kevinhartman Feb 7, 2025
e423bf9
Add try_const to lift.
kevinhartman Feb 7, 2025
0a5917b
Try multiple singletons, new one for const.
kevinhartman Feb 7, 2025
a97434d
Revert "Try multiple singletons, new one for const."
kevinhartman Feb 7, 2025
1afc965
Remove Bool singleton test.
kevinhartman Feb 7, 2025
86655f1
Add const handling for stores, fix test bugs.
kevinhartman Feb 7, 2025
aaeae9b
Fix formatting.
kevinhartman Feb 7, 2025
a2a444b
Remove Duration and Stretch for now.
kevinhartman Feb 7, 2025
8ac2dc3
Cleanup, fix const bug in index.
kevinhartman Feb 7, 2025
9f8313c
Fix ordering issue for types with differing const-ness.
kevinhartman Feb 9, 2025
db9d9cb
Fix QPY serialization.
kevinhartman Feb 10, 2025
71b7e7a
Make expr.Lift default to non-const.
kevinhartman Feb 11, 2025
2091557
Revert to old test_expr_constructors.py.
kevinhartman Feb 11, 2025
7307be9
Make binary_logical lift independent again.
kevinhartman Feb 11, 2025
9b30284
Update tests, handle a few edge cases.
kevinhartman Feb 12, 2025
ce1faf1
Fix docstring.
kevinhartman Feb 12, 2025
4fee48f
Remove now redundant arg from tests.
kevinhartman Feb 12, 2025
88ab046
Add const testing for ordering.
kevinhartman Feb 12, 2025
c5a230f
Add const tests for shifts.
kevinhartman Feb 12, 2025
7c88d88
Add release note.
kevinhartman Feb 12, 2025
c58a7b8
Add const store tests.
kevinhartman Feb 12, 2025
d9e9a8c
Address lint, minor cleanup.
kevinhartman Feb 13, 2025
4a56150
Add Float type to classical expressions.
kevinhartman Feb 12, 2025
23b5961
Allow DANGEROUS conversion from Float to Bool.
kevinhartman Feb 12, 2025
8bf2e4f
Test Float ordering.
kevinhartman Feb 12, 2025
111eb32
Improve error messages for using Float with logical operators.
kevinhartman Feb 12, 2025
a839d51
Float tests for constructors.
kevinhartman Feb 12, 2025
a19b39a
Add release note.
kevinhartman Feb 12, 2025
7f02e56
Add Duration and Stretch classical types.
kevinhartman Feb 13, 2025
55af327
Add Duration type to qiskit.circuit.
kevinhartman Feb 13, 2025
86b7af9
Block Stretch from use in binary relations.
kevinhartman Feb 13, 2025
9b3c821
Add type ordering tests for Duration and Stretch.
kevinhartman Feb 13, 2025
69eab91
Test expr constructors for Duration and Stretch.
kevinhartman Feb 13, 2025
971cc26
Fix lint.
kevinhartman Feb 13, 2025
25508bf
Reject const vars in add_var and add_input.
kevinhartman Feb 17, 2025
2c8ce43
Merge branch 'main' of github.com:Qiskit/qiskit into const-expr
kevinhartman Feb 17, 2025
ccf9441
Implement QPY support for const-typed expressions.
kevinhartman Feb 18, 2025
c6eab02
Remove invalid test.
kevinhartman Feb 18, 2025
edd7806
Update QPY version 14 desc.
kevinhartman Feb 18, 2025
8afa92e
Fix lint.
kevinhartman Feb 18, 2025
4e0f2df
Add serialization testing.
kevinhartman Feb 18, 2025
15ba943
Merge branch 'main' of github.com:Qiskit/qiskit into const-expr
kevinhartman Feb 18, 2025
81c5833
Merge branch 'const-expr' into expr-float
kevinhartman Feb 18, 2025
50c31d3
Test pre-v14 QPY rejects const-typed exprs.
kevinhartman Feb 18, 2025
7e43322
Merge branch 'main' of github.com:Qiskit/qiskit into const-expr
kevinhartman Feb 18, 2025
2449ee6
Merge branch 'const-expr' into expr-float
kevinhartman Feb 18, 2025
e55e189
QASM export for floats.
kevinhartman Feb 18, 2025
1d51022
QPY support for floats.
kevinhartman Feb 18, 2025
eb8f150
Fix lint.
kevinhartman Feb 18, 2025
415f62e
Merge branch 'expr-float' into expr-timing
kevinhartman Feb 18, 2025
d38f3e9
Settle on Duration circuit core type.
kevinhartman Feb 19, 2025
fd66c1d
QPY serialization for durations and stretches.
kevinhartman Feb 19, 2025
e9b6d97
Add QPY testing.
kevinhartman Feb 19, 2025
52da3cc
QASM support for stretch and duration.
kevinhartman Feb 19, 2025
a3be688
Fix lint.
kevinhartman Feb 19, 2025
07771f1
Merge branch 'main' of github.com:Qiskit/qiskit into const-expr
kevinhartman Feb 20, 2025
9332ed3
Merge branch 'const-expr' into expr-float
kevinhartman Feb 20, 2025
17241c6
Merge branch 'expr-float' into expr-timing
kevinhartman Feb 20, 2025
ca91bfd
Don't use match since we still support Python 3.9.
kevinhartman Feb 20, 2025
ad87e78
Fix enum match.
kevinhartman Feb 20, 2025
d3bca5f
Merge branch 'main' of github.com:Qiskit/qiskit into const-expr
kevinhartman Feb 24, 2025
50deb93
Revert visitors.py.
kevinhartman Feb 24, 2025
f1dc1a1
Address review comments.
kevinhartman Feb 24, 2025
1f439a0
Merge branch 'main' of github.com:Qiskit/qiskit into const-expr
kevinhartman Feb 25, 2025
166d7b2
Improve type docs.
kevinhartman Feb 26, 2025
10b2c8d
Merge branch 'main' of github.com:Qiskit/qiskit into const-expr
kevinhartman Feb 26, 2025
9d6cf39
Revert QPY, since the old format can support constexprs.
kevinhartman Feb 26, 2025
8021e00
Move const-ness from Type to Expr.
kevinhartman Feb 26, 2025
a15141b
Revert QPY testing, no longer needed.
kevinhartman Feb 26, 2025
ca2785d
Add explicit validation of const expr.
kevinhartman Feb 26, 2025
e902009
Revert stuff I didn't need to touch.
kevinhartman Feb 26, 2025
16475c3
Update release note.
kevinhartman Feb 26, 2025
6b5930d
A few finishing touches.
kevinhartman Feb 27, 2025
be73180
Merge branch 'const-expr-expr' into expr-float-expr
kevinhartman Feb 27, 2025
25f1693
Fix-up after merge.
kevinhartman Feb 27, 2025
ecc343b
Merge branch 'expr-float-expr' into expr-timing-expr
kevinhartman Feb 27, 2025
b32e6f0
Fix-ups after merge.
kevinhartman Feb 27, 2025
864506d
Fix lint.
kevinhartman Feb 27, 2025
6bc728a
Fix comment and release note.
kevinhartman Feb 27, 2025
2d38178
Merge branch 'expr-float' into expr-timing
kevinhartman Feb 27, 2025
42258b8
Merge branch 'main' of github.com:Qiskit/qiskit into expr-float
kevinhartman Feb 28, 2025
481700a
Merge branch 'expr-float' into expr-timing
kevinhartman Feb 28, 2025
e09762f
Special-case Var const-ness for Stretch type.
kevinhartman Feb 28, 2025
cbd55db
Address review comments.
kevinhartman Feb 28, 2025
f12afb4
Update release note.
kevinhartman Feb 28, 2025
8d059cd
Merge branch 'expr-float' into expr-timing
kevinhartman Feb 28, 2025
e05d9ff
Update docs.
kevinhartman Feb 28, 2025
0f14cca
Address review comments.
kevinhartman Mar 1, 2025
7db8222
Merge branch 'expr-float' into expr-timing
kevinhartman Mar 1, 2025
34e615b
Merge branch 'main' of github.com:Qiskit/qiskit into expr-timing
kevinhartman Mar 1, 2025
bbe67a6
Remove Stretch type.
kevinhartman Mar 2, 2025
0acc450
Remove a few more mentions of the binned stretch type.
kevinhartman Mar 2, 2025
7fea216
Add docstring for Duration.
kevinhartman Mar 2, 2025
78b2951
Address review comments.
kevinhartman Mar 3, 2025
97bbeba
Remove unused import.
kevinhartman Mar 3, 2025
49f2af8
Remove visitor short circuit.
kevinhartman Mar 3, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 60 additions & 0 deletions crates/circuit/src/duration.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
// This code is part of Qiskit.
//
// (C) Copyright IBM 2025
//
// This code is licensed under the Apache License, Version 2.0. You may
// obtain a copy of this license in the LICENSE.txt file in the root directory
// of this source tree or at http://www.apache.org/licenses/LICENSE-2.0.
//
// Any modifications or derivative works of this code must retain this
// copyright notice, and modified files need to carry a notice indicating
// that they have been altered from the originals.

use pyo3::prelude::*;

#[pyclass(eq, module = "qiskit._accelerate.circuit")]
#[derive(PartialEq, Clone, Copy, Debug)]
#[allow(non_camel_case_types)]
/// A length of time used to express circuit timing.
///
/// It defines a group of classes which are all subclasses of itself (functionally, an
/// enumeration carrying data).
///
/// In Python 3.10+, you can use it in a match statement::
///
/// match duration:
/// case Duration.dt(dt):
/// return dt
/// case Duration.s(seconds):
/// return seconds / 5e-7
/// case _:
/// raise ValueError("expected dt or seconds")
///
/// And in Python 3.9, you can use ``isinstance`` to determine which variant
/// is populated::
///
/// if isinstance(duration, Duration.dt):
/// return duration[0]
/// elif isinstance(duration, Duration.s):
/// return duration[0] / 5e-7
/// else:
/// raise ValueError("expected dt or seconds")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit of a nuisance in Python 3.9. Perhaps cleaner to add a unit pyfunction that returns the string name, so you can dispatch on it? E.g.

#[pymethods]
impl Duration {
  /// The corresponding ``unit`` of the duration.
  fn unit(&self) -> &'static str {
    match Self {
      Duration::ns(_) => "ns",
      // ...
    }
  }
}

then Python-space has a cheaper way of doing the dispatch programmatically, and it maybe provides a slightly easier link with the legacy (value, unit) 2-tuple.

I'd say that ^that particular one is a candidate for doing in this PR (if you think it's worthwhile), but ones that would certainly be follow-ups if we wanted them to be: this class is a sensible place to add methods like as_seconds(self) -> f64, as_dt(self, dt_in_seconds) -> u64, etc, which would be useful if/when we swap to using this more in the internals.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 78b2951.

I also added a py_value method to get the int or float (exposed in Python as Duration.value())

pub enum Duration {
dt(u64),
ns(f64),
us(f64),
ms(f64),
s(f64),
}

impl Duration {
fn __repr__(&self) -> String {
match self {
Duration::ns(t) => format!("Duration.ns({})", t),
Duration::us(t) => format!("Duration.us({})", t),
Duration::ms(t) => format!("Duration.ms({})", t),
Duration::s(t) => format!("Duration.s({})", t),
Duration::dt(t) => format!("Duration.dt({})", t),
}
}
}
2 changes: 2 additions & 0 deletions crates/circuit/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ pub mod converters;
pub mod dag_circuit;
pub mod dag_node;
mod dot_utils;
pub mod duration;
pub mod error;
pub mod gate_matrix;
pub mod imports;
Expand Down Expand Up @@ -157,6 +158,7 @@ macro_rules! impl_intopyobject_for_copy_pyclass {
}

pub fn circuit(m: &Bound<PyModule>) -> PyResult<()> {
m.add_class::<duration::Duration>()?;
m.add_class::<circuit_data::CircuitData>()?;
m.add_class::<circuit_instruction::CircuitInstruction>()?;
m.add_class::<dag_circuit::DAGCircuit>()?;
Expand Down
2 changes: 2 additions & 0 deletions qiskit/circuit/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -1282,6 +1282,8 @@ def __array__(self, dtype=None, copy=None):
\end{pmatrix}
"""

from qiskit._accelerate.circuit import Duration # pylint: disable=unused-import

from .exceptions import CircuitError
from . import _utils
from .quantumcircuit import QuantumCircuit
Expand Down
5 changes: 4 additions & 1 deletion qiskit/circuit/classical/expr/constructors.py
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ def lift(value: typing.Any, /, type: types.Type | None = None) -> Expr:
if type is not None:
raise ValueError("use 'cast' to cast existing expressions, not 'lift'")
return value
from qiskit.circuit import Clbit, ClassicalRegister # pylint: disable=cyclic-import
from qiskit.circuit import Clbit, ClassicalRegister, Duration # pylint: disable=cyclic-import

inferred: types.Type
if value is True or value is False or isinstance(value, Clbit):
Expand All @@ -126,6 +126,9 @@ def lift(value: typing.Any, /, type: types.Type | None = None) -> Expr:
elif isinstance(value, float):
inferred = types.Float()
constructor = Value
elif isinstance(value, Duration):
inferred = types.Duration()
constructor = Value
else:
raise TypeError(f"failed to infer a type for '{value}'")
if type is None:
Expand Down
5 changes: 5 additions & 0 deletions qiskit/circuit/classical/expr/visitors.py
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,8 @@ def is_lvalue(node: expr.Expr, /) -> bool:
the scope that attempts to write to it. This would be an access property of the containing
program, however, and not an inherent property of the expression system.

A constant expression is never an lvalue.

Examples:
Literal values are never l-values; there's no memory location associated with (for example)
the constant ``1``::
Expand Down Expand Up @@ -297,4 +299,7 @@ def is_lvalue(node: expr.Expr, /) -> bool:
>>> expr.is_lvalue(expr.bit_and(a, b))
False
"""
if node.const:
# A constant expression is _never_ an lvalue.
return False
return node.accept(_IS_LVALUE)
13 changes: 6 additions & 7 deletions qiskit/circuit/classical/types/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,16 +34,14 @@
singleton instances to facilitate this.

The :class:`Bool` type represents :class:`.Clbit` and the literals ``True`` and ``False``, the
:class:`Uint` type represents :class:`.ClassicalRegister` and Python integers, and the
:class:`Float` type represents Python floats.
:class:`Uint` type represents :class:`.ClassicalRegister` and Python integers, the :class:`Float`
type represents Python floats, and the :class:`Duration` type represents a duration for use in
timing-aware circuit operations.

.. autoclass:: Bool
.. autoclass:: Uint
.. autoclass:: Float

Note that :class:`Uint` defines a family of types parametrized by their width; it is not one single
type, which may be slightly different to the 'classical' programming languages you are used to.

.. autoclass:: Duration

Working with types
==================
Expand Down Expand Up @@ -99,6 +97,7 @@
__all__ = [
"Type",
"Bool",
"Duration",
"Float",
"Uint",
"Ordering",
Expand All @@ -110,5 +109,5 @@
"cast_kind",
]

from .types import Type, Bool, Float, Uint
from .types import Type, Bool, Duration, Float, Uint
from .ordering import Ordering, order, is_subtype, is_supertype, greater, CastKind, cast_kind
4 changes: 3 additions & 1 deletion qiskit/circuit/classical/types/ordering.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@

import enum

from .types import Type, Bool, Float, Uint
from .types import Type, Bool, Duration, Float, Uint


# While the type system is simple, it's overkill to represent the complete partial ordering graph of
Expand Down Expand Up @@ -67,6 +67,7 @@ def _order_uint_uint(left: Uint, right: Uint, /) -> Ordering:
(Bool, Bool): lambda _a, _b, /: Ordering.EQUAL,
(Uint, Uint): _order_uint_uint,
(Float, Float): lambda _a, _b, /: Ordering.EQUAL,
(Duration, Duration): lambda _a, _b, /: Ordering.EQUAL,
}


Expand Down Expand Up @@ -199,6 +200,7 @@ def _uint_cast(from_: Uint, to_: Uint, /) -> CastKind:
(Float, Float): lambda _a, _b, /: CastKind.EQUAL,
(Float, Uint): lambda _a, _b, /: CastKind.DANGEROUS,
(Float, Bool): lambda _a, _b, /: CastKind.DANGEROUS,
(Duration, Duration): lambda _a, _b, /: CastKind.EQUAL,
}


Expand Down
17 changes: 17 additions & 0 deletions qiskit/circuit/classical/types/types.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
__all__ = [
"Type",
"Bool",
"Duration",
"Float",
"Uint",
]
Expand Down Expand Up @@ -134,3 +135,19 @@ def __hash__(self):

def __eq__(self, other):
return isinstance(other, Float)


@typing.final
class Duration(Type, metaclass=_Singleton):
"""A length of time, possibly negative."""

__slots__ = ()

def __repr__(self):
return "Duration()"

def __hash__(self):
return hash(self.__class__)

def __eq__(self, other):
return isinstance(other, Duration)
12 changes: 12 additions & 0 deletions qiskit/qasm3/ast.py
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,18 @@ class BitType(ClassicalType):
__slots__ = ()


class DurationType(ClassicalType):
"""Type information for a duration."""

__slots__ = ()


class StretchType(ClassicalType):
"""Type information for a stretch."""

__slots__ = ()


Comment on lines +182 to +187
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe just semantics: my reading was we give-or-take decided to interpret the OQ3 spec as saying that stretch x; didn't involve a type stretch so much as just being a special keyword for a duration-typed declaration. I'm also fine to leave it the way we have (heaven knows this AST is a jumble), but in another world: would it make more sense to handle stretch x (= Expr)?; as a

class StretchDeclaration(Statement):
    identifier: Identifier
    bound: Optional[Expr]

rather than a type?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah, that makes a ton more sense. We don't use this in this PR anyway, so if it's alright, I'll leave it for now and make the switch in the PR that actually does.

class BitArrayType(ClassicalType):
"""Type information for a sized number of classical bits."""

Expand Down
15 changes: 15 additions & 0 deletions qiskit/qasm3/exporter.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
CircuitInstruction,
Clbit,
Gate,
Duration,
Measure,
Parameter,
ParameterExpression,
Expand Down Expand Up @@ -1255,6 +1256,8 @@ def _build_ast_type(type_: types.Type) -> ast.ClassicalType:
return ast.UintType(type_.width)
if type_.kind is types.Float:
return ast.FloatType.DOUBLE
if type_.kind is types.Duration:
return ast.DurationType()
raise RuntimeError(f"unhandled expr type '{type_}'")


Expand All @@ -1271,13 +1274,25 @@ def __init__(self, lookup):
def visit_var(self, node, /):
return self.lookup(node) if node.standalone else self.lookup(node.var)

# pylint: disable=too-many-return-statements
def visit_value(self, node, /):
if node.type.kind is types.Bool:
return ast.BooleanLiteral(node.value)
if node.type.kind is types.Uint:
return ast.IntegerLiteral(node.value)
if node.type.kind is types.Float:
return ast.FloatLiteral(node.value)
if node.type.kind is types.Duration:
if isinstance(node.value, Duration.dt):
return ast.DurationLiteral(node.value[0], ast.DurationUnit.SAMPLE)
if isinstance(node.value, Duration.ns):
return ast.DurationLiteral(node.value[0], ast.DurationUnit.NANOSECOND)
if isinstance(node.value, Duration.us):
return ast.DurationLiteral(node.value[0], ast.DurationUnit.MICROSECOND)
if isinstance(node.value, Duration.ms):
return ast.DurationLiteral(node.value[0], ast.DurationUnit.MILLISECOND)
if isinstance(node.value, Duration.s):
return ast.DurationLiteral(node.value[0], ast.DurationUnit.SECOND)
raise RuntimeError(f"unhandled Value type '{node}'")

def visit_cast(self, node, /):
Expand Down
6 changes: 6 additions & 0 deletions qiskit/qasm3/printer.py
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,12 @@ def _visit_FloatType(self, node: ast.FloatType) -> None:
def _visit_BoolType(self, _node: ast.BoolType) -> None:
self.stream.write("bool")

def _visit_DurationType(self, _node: ast.DurationType) -> None:
self.stream.write("duration")

def _visit_StretchType(self, _node: ast.StretchType) -> None:
self.stream.write("stretch")

def _visit_IntType(self, node: ast.IntType) -> None:
self.stream.write("int")
if node.size is not None:
Expand Down
49 changes: 38 additions & 11 deletions qiskit/qpy/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -382,31 +382,58 @@ def open(*args):
Version 14
----------

Version 14 adds support for additional :class:`~.types.Type` classes.
Version 14 adds a new core DURATION type, as well as support for additional :class:`~.types.Type`
classes.

DURATION
~~~~~~~~

A :class:`~.circuit.Duration` is encoded by a single-byte ASCII ``char`` that encodes the kind of
type, followed by a payload that varies depending on the type. The defined codes are:

============================== ========= =========================================================
Qiskit class Type code Payload
============================== ========= =========================================================
:class:`~.circuit.Duration.dt` ``t`` One ``unsigned long long value``.

:class:`~.circuit.Duration.ns` ``n`` One ``double value``.

:class:`~.circuit.Duration.us` ``u`` One ``double value``.

:class:`~.circuit.Duration.ms` ``m`` One ``double value``.

:class:`~.circuit.Duration.s` ``s`` One ``double value``.

============================== ========= =========================================================

Changes to EXPR_TYPE
~~~~~~~~~~~~~~~~~~~~

The following table shows the new type classes added in the version:

====================== ========= =================================================================
Qiskit class Type code Payload
====================== ========= =================================================================
:class:`~.types.Float` ``f`` None.
====================== ========= =================================================================
========================= ========= ==============================================================
Qiskit class Type code Payload
========================= ========= ==============================================================
:class:`~.types.Float` ``f`` None.

:class:`~.types.Duration` ``d`` None.

========================= ========= ==============================================================

Changes to EXPR_VALUE
~~~~~~~~~~~~~~~~~~~~~

The classical expression's type system now supports new encoding types for value literals, in
addition to the existing encodings for int and bool. The new value type encodings are below:

=========== ========= ============================================================================
Python type Type code Payload
=========== ========= ============================================================================
``float`` ``f`` One ``double value``.
=========================== ========= ============================================================
Python type / class Type code Payload
=========================== ========= ============================================================
``float`` ``f`` One ``double value``.

=========== ========= ============================================================================
:class:`~.circuit.Duration` ``t`` One ``DURATION``.

=========================== ========= ============================================================
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

circuit.Duration is still a Python type, tbf

I think there's a risk with using t: I think that type code is already used elsewhere to represent "tuple", and there's a risk of a clash (despite the organisational structure on type_keys.py, the enums in there are not necessarily separate namespaces in QPY land). That said, I think EXPR_VALUE is localised enough that it can never hit the clash, but PARAM_TYPE would. Up to you if you think it's worth changing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might not be understanding. Are you saying that we should try to have unique characters across all of type_keys.py in case we introduce a bug that reads/writes a byte in the wrong place? My understanding of these is that we're manually reading / writing a byte that can only be one of e.g. TypeKeys.ExprType.<KEYS> in the place that we're reading / writing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't remember the exact circumstances, but I remember having trouble with QPY at one point because there were locations where type keys from more than one TypeKeys.<Thing> were valid choices - I think the abstraction is leaky.

That said, given that I added the ExprType ones after that, I'm relatively sure that it's not a problem here, so it's ok to leave.


.. _qpy_version_13:

Expand Down
Loading