Skip to content

Fix OverflowError in QuantumCircuit.for_loop for negative integers in list indexset #16432

Open
at264939-ctrl wants to merge 10 commits into
Qiskit:mainfrom
at264939-ctrl:fix-for-loop-negative-indexset
Open

Fix OverflowError in QuantumCircuit.for_loop for negative integers in list indexset #16432
at264939-ctrl wants to merge 10 commits into
Qiskit:mainfrom
at264939-ctrl:fix-for-loop-negative-indexset

Conversation

@at264939-ctrl

@at264939-ctrl at264939-ctrl commented Jun 14, 2026

Copy link
Copy Markdown
Contributor

Summary

This PR fixes an OverflowError that occurs when passing a list containing negative integers as an indexset to QuantumCircuit.for_loop.

The Problem:
The Rust backend represented the for_loop indexset list as Vec<usize> (unsigned). This caused a crash when Python passed negative integers, as they cannot be converted to an unsigned type. PyRange already uses signed integers (isize), making the behavior inconsistent across different iterable forms.

The Fix:

  • Changed ForCollection::List(Vec<usize>) to ForCollection::List(Vec<isize>) in crates/circuit/src/operations.rs.
  • Updated QPY serialization in crates/qpy/src/value.rs to correctly pack and unpack signed integers.
  • Added regression tests to test/python/circuit/test_control_flow.py and test/python/circuit/test_circuit_load_from_qpy.py to ensure correct behavior and QPY compatibility.

Fixes #16412

AI/LLM disclosure

  • I didn't use LLM tooling, or only used it privately.
  • I used the following tool to help write this PR description: antigravity (models: gemini flash 3.5 & sonnet 4.6)
  • I used the following tool to generate or modify code: antigravity (models: gemini flash 3.5 & sonnet 4.6)

@at264939-ctrl at264939-ctrl requested a review from a team as a code owner June 14, 2026 19:52
@at264939-ctrl at264939-ctrl requested a review from gadial June 14, 2026 19:52
@qiskit-bot qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Jun 14, 2026
@qiskit-bot

Copy link
Copy Markdown
Collaborator

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

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

  • @Qiskit/terra-core
  • @mtreinish

@gadial

gadial commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

This PR seems to break backwards compatibility, since QPY files created with older versions would assume the stored value is usize and not isize. However, it seems to be that backwards compatibility was already broken (in the transition from Python to Rust) and we didn't notice it since the backwards compatibility suite did not include a relevant test. We should add one now (in test/qpy_compat/test_qpy.py and observe the failures; if indeed we'll manage to load QPY from 2.3 (Python implementation) an not 2.4 (Rust implementation) it means we had a serious QPY bug which this PR fixes, and will need to manually disable the 2.4 file generation for this test.

If needed, I can help with that.

@coveralls

Copy link
Copy Markdown

Coverage Report for CI Build 27537541812

Coverage decreased (-0.02%) to 87.517%

Details

  • Coverage decreased (-0.02%) from the base build.
  • Patch coverage: 1 of 1 lines across 1 file are fully covered (100%).
  • No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.


Coverage Stats

Coverage Status
Relevant Lines: 127240
Covered Lines: 111356
Line Coverage: 87.52%
Coverage Strength: 956309.04 hits per line

💛 - Coveralls

@at264939-ctrl at264939-ctrl force-pushed the fix-for-loop-negative-indexset branch from f5184cf to 3aa50de Compare June 15, 2026 10:18
@at264939-ctrl

Copy link
Copy Markdown
Contributor Author

This PR seems to break backwards compatibility, since QPY files created with older versions would assume the stored value is usize and not isize. However, it seems to be that backwards compatibility was already broken (in the transition from Python to Rust) and we didn't notice it since the backwards compatibility suite did not include a relevant test. We should add one now (in test/qpy_compat/test_qpy.py and observe the failures; if indeed we'll manage to load QPY from 2.3 (Python implementation) an not 2.4 (Rust implementation) it means we had a serious QPY bug which this PR fixes, and will need to manually disable the 2.4 file generation for this test.

If needed, I can help with that.

Thank you I've spent some time investigating the QPY compatibility as you suggested, and you were spot on.

I've updated

test/qpy_compat/test_qpy.py
to include a test case with a negative integer indexset in

for_loop
. To verify the regression, I ran the compatibility suite across several versions:

Backwards Compatibility (1.x -> Dev): Verified that Qiskit 1.2.4 and 1.3.1 (the Python era) generate these QPY files correctly. My local dev branch with this fix now loads these files without issue.
Current Regression: Confirmed that the current

main
(2.4.x) fails both as a generator (it crashes with the OverflowError) and as a loader for the 1.x files.
Changes in

test_qpy.py
: I've added for_loop_negative.qpy to the generation loop but explicitly skipped it for version 2.4.0 since it crashes there.
This confirms that the OverflowError was indeed a regression introduced during the Rust transition that broke compatibility with older Python-generated QPY files.

I've pushed the updates to

test/qpy_compat/test_qpy.py
and consolidated all the tests

@jakelishman

Copy link
Copy Markdown
Member

We expect all responses to PRs to be from humans and not LLMs. You may use an LLM to assist with writing English, but it's not appropriate to have an LLM generate the entire response.

@at264939-ctrl

Copy link
Copy Markdown
Contributor Author

We expect all responses to PRs to be from humans and not LLMs. You may use an LLM to assist with writing English, but it's not appropriate to have an LLM generate the entire response.

I use ai because English is not my first language

Comment thread test/qpy_compat/test_qpy.py
@at264939-ctrl at264939-ctrl force-pushed the fix-for-loop-negative-indexset branch from c331acd to 0d198ee Compare June 17, 2026 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Community PR PRs from contributors that are not 'members' of the Qiskit repo

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

QuantumCircuit.for_loop raises OverflowError for negative integers in a list indexset

6 participants