Preserve Python `range` objects in for loops #15404

jakelishman · 2025-12-01T14:13:13Z

The ForLoopOp always preserved Python range objects in the structured form, and had special-purpose visualisation and OQ3 export capabilities to ensure that these were handled in a structured way end-to-end. While the initial Rust port preserved the Python type hints (range is an Iterable[int]), it didn't preserve all the intended behaviour.

This commit includes the setup to allow the for-loop collection to be more dynamically typed as well.

Summary

Details and comments

Fix #15380.

No additional tests because the existing tests would already have caught this, except that we don't run them because of #15402. No release note because the bug isn't released.

The `ForLoopOp` always preserved Python `range` objects in the structured form, and had special-purpose visualisation and OQ3 export capabilities to ensure that these were handled in a structured way end-to-end. While the initial Rust port preserved the Python type hints (`range` _is_ an `Iterable[int]`), it didn't preserve all the intended behaviour. This commit includes the setup to allow the for-loop collection to be more dynamically typed as well.

qiskit-bot · 2025-12-01T14:13:18Z

One or more of the following people are relevant to this code:

@Qiskit/terra-core

coveralls · 2025-12-01T17:15:36Z

Pull Request Test Coverage Report for Build 19829846885

Details

46 of 69 (66.67%) changed or added relevant lines in 5 files are covered.
9 unchanged lines in 3 files lost coverage.
Overall coverage decreased (-0.009%) to 88.381%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
crates/circuit/src/circuit_instruction.rs	8	14	57.14%
crates/circuit/src/operations.rs	25	42	59.52%

Files with Coverage Reduction	New Missed Lines	%
crates/circuit/src/parameter/symbol_expr.rs	1	72.94%
crates/qasm2/src/lex.rs	2	92.29%
crates/qasm2/src/parse.rs	6	97.56%

Totals
Change from base Build 19827713394:	-0.009%
Covered Lines:	95763
Relevant Lines:	108353

💛 - Coveralls

raynelfss

I haven't fully looked at the code but there are two questions on my mind as I'm looking.

raynelfss · 2025-12-01T20:27:31Z

crates/circuit/src/operations.rs

+/// A literal Python range extracted to a Rust object.
+///
+/// This is separate to `PyO3`'s `PyRange` type, since that keeps everything internally safe for
+/// subclassing and modification the Python heap.
+#[derive(Clone, Copy, Debug, PartialEq, Eq)]
+pub struct PyRange {
+    pub start: isize,
+    pub stop: isize,
+    pub step: NonZero<isize>,
+}
+impl PyRange {
+    pub fn is_empty(&self) -> bool {
+        let step = self.step.unsigned_abs().get();
+        let diff = self.start.abs_diff(self.stop);
+        (self.step.get() > 0 && self.start < self.stop
+            || (self.step.get() < 0 && self.start > self.stop))
+            && diff >= step
+    }
+    pub fn len(&self) -> usize {
+        let step = self.step.unsigned_abs().get();
+        let diff = self.start.abs_diff(self.stop);
+        if (self.step.get() > 0 && self.start < self.stop)
+            || (self.step.get() < 0 && self.start > self.stop)
+        {
+            // The `diff-1` is guaranteed safe because the `start < stop` or `start > stop`
+            // conditions guarantee that `diff` is at least 1.
+            1 + (diff - 1) / step
+        } else {
+            0
+        }
+    }
+}


Hmmm... This struct looks very similar to parts of SequenceIndex an enum struct you designed during #12669. Would you be able to repurpose that struct instead of creating a new one for this? Or if not, should you repurpose that one to use the one offered here?

Not really - a slice and a range are different Python objects, although they share some passing similarities. Slices can have None in various places, and have different semantics on negative indices.

I understand that, in which case since SequenceIndex uses Ranges and this type consolidates them into one. Should we replace the PosRange and NegRange variants to use this instead??

It doesn't feel to me that unifying PosRange and NegRange into PyRange would actually make things easier; the reason PosRange and NegRange are already split from each other is that handling steps that can be either positive or negative is already very awkward. The semantics of the ranges within SliceIndex are slightly different too, because of how the end-points work (particularly with regard to negative steps).

Fwiw, I don't expect this PyRange object to live forever - we will almost certainly rework how for-loop collections work at some point.

raynelfss · 2025-12-01T20:31:31Z

crates/circuit/src/operations.rs

+/// Possible specifications of the "collection" that a for loop iterates over.
+#[derive(Clone, Debug, IntoPyObject, IntoPyObjectRef, FromPyObject, PartialEq, Eq)]
+pub enum ForCollection {
+    /// A literal Python `range` object extracted to Rust.
+    PyRange(PyRange),
+    /// Some ordered collection of integers.
+    List(Vec<usize>),
+}


This here looks very similar to PySequenceIndex which you introduced in #12669. The only difference I see here is that you're not using the int variant? Or would we be able to repurpose it to work wit this one.

debasmita2102 · 2025-12-02T06:29:25Z

I’m a bit confused about should ForCollection::len() is meant to line up always with the actual number of loop iterations. For both the PyRange and List cases, should I always expect len() to equal the number of times the body will run, even for larger or nested loops? If that’s the intended invariant, should we add tests that build ForLoops with both PyRange and List collections and check that len() matches the actual iteration count when the loop is expanded.

raynelfss

I don't have any strong feelings on this addition. Although, I believe the naming here is a bit misleading and we should consider renaming it to something more rust friendly.

raynelfss · 2025-12-02T14:26:09Z

crates/circuit/src/operations.rs

+/// A literal Python range extracted to a Rust object.
+///
+/// This is separate to `PyO3`'s `PyRange` type, since that keeps everything internally safe for
+/// subclassing and modification the Python heap.
+#[derive(Clone, Copy, Debug, PartialEq, Eq)]
+pub struct PyRange {
+    pub start: isize,
+    pub stop: isize,
+    pub step: NonZero<isize>,
+}
+impl PyRange {
+    pub fn is_empty(&self) -> bool {
+        let step = self.step.unsigned_abs().get();
+        let diff = self.start.abs_diff(self.stop);
+        (self.step.get() > 0 && self.start < self.stop
+            || (self.step.get() < 0 && self.start > self.stop))
+            && diff >= step
+    }
+    pub fn len(&self) -> usize {
+        let step = self.step.unsigned_abs().get();
+        let diff = self.start.abs_diff(self.stop);
+        if (self.step.get() > 0 && self.start < self.stop)
+            || (self.step.get() < 0 && self.start > self.stop)
+        {
+            // The `diff-1` is guaranteed safe because the `start < stop` or `start > stop`
+            // conditions guarantee that `diff` is at least 1.
+            1 + (diff - 1) / step
+        } else {
+            0
+        }
+    }
+}


I understand that, in which case since SequenceIndex uses Ranges and this type consolidates them into one. Should we replace the PosRange and NegRange variants to use this instead??

raynelfss · 2025-12-02T14:47:52Z

crates/circuit/src/operations.rs

+/// This is separate to `PyO3`'s `PyRange` type, since that keeps everything internally safe for
+/// subclassing and modification the Python heap.
+#[derive(Clone, Copy, Debug, PartialEq, Eq)]
+pub struct PyRange {


Should we consider changing the name of the struct to something that doesn't have Py in the name? even if this struct comes from Python, it doesn't keep any Python tokens or pointers. If anything we can change the name to something along the lines of:

pub struct IndexRange {

Or something like that.

This object here is PyRange because it's a very literal description of a Python range object. We expect that the collection of a ForLoop will expand to involve more runtime ranges / collections, which actually would be pure Rust and unrelated to Python built-ins, including something like Range { start: Expr, stop: Expr, step: Option<Expr> }, which is why I was leaving naming space for it.

jakelishman · 2025-12-03T08:50:15Z

I’m a bit confused about should ForCollection::len() is meant to line up always with the actual number of loop iterations. For both the PyRange and List cases, should I always expect len() to equal the number of times the body will run, even for larger or nested loops? If that’s the intended invariant, should we add tests that build ForLoops with both PyRange and List collections and check that len() matches the actual iteration count when the loop is expanded.

Debasmita: sorry, I didn't reply to your comment properly before. ForCollection::len should return the number of elements that will be iterated over. It could have been a good idea to add tests of the PyRange::len method (the List variant is trivial) - it should line up exactly with Python's range object. If you wanted to make a PR adding Rust-space tests to this file that test the length, I can review it.

jakelishman added this to the 2.3.0 milestone Dec 1, 2025

jakelishman requested a review from a team as a code owner December 1, 2025 14:13

jakelishman added Rust This PR or issue is related to Rust code in the repository mod: circuit Related to the core of the `QuantumCircuit` class or the circuit library labels Dec 1, 2025

github-project-automation bot added this to Qiskit 2.3 Dec 1, 2025

github-project-automation bot moved this to Ready in Qiskit 2.3 Dec 1, 2025

jakelishman assigned raynelfss Dec 1, 2025

jakelishman force-pushed the cf/for-loop-range branch from 180fe7f to 0b2a36c Compare December 1, 2025 15:55

Fix clippy complaints

7d0fba2

jakelishman force-pushed the cf/for-loop-range branch from 0b2a36c to 7d0fba2 Compare December 1, 2025 16:29

raynelfss reviewed Dec 1, 2025

View reviewed changes

raynelfss reviewed Dec 2, 2025

View reviewed changes

raynelfss approved these changes Dec 2, 2025

View reviewed changes

raynelfss added this pull request to the merge queue Dec 2, 2025

Merged via the queue into Qiskit:main with commit 4b6dd22 Dec 2, 2025
23 checks passed

github-project-automation bot moved this from Ready to Done in Qiskit 2.3 Dec 2, 2025

jakelishman deleted the cf/for-loop-range branch December 2, 2025 19:36

arthurostrauss mentioned this pull request Dec 3, 2025

New classical expr.Range specification #14618

Open

Preserve Python range objects in for loops #15404

Preserve Python range objects in for loops #15404

Uh oh!

Conversation

jakelishman commented Dec 1, 2025

Summary

Details and comments

Uh oh!

qiskit-bot commented Dec 1, 2025

Uh oh!

coveralls commented Dec 1, 2025

Pull Request Test Coverage Report for Build 19829846885

Details

💛 - Coveralls

Uh oh!

raynelfss left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

debasmita2102 commented Dec 2, 2025

Uh oh!

raynelfss left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jakelishman commented Dec 3, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Preserve Python `range` objects in for loops #15404

Preserve Python `range` objects in for loops #15404