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

Create a secure internal path for custom prefixes in registers. #14005

Merged
merged 6 commits into from
Mar 18, 2025

Conversation

raynelfss
Copy link
Contributor

Summary

The following commits add the internal method _new_with_prefix, which allows for the one-time replacement of the set prefix name and use of the same instance counter.

Details and comments

Prior implementations would replace the Register's prefix attribute in place which is an unsafe operation. The following commits add a secure path for a provisional replacement of a register's prefix name to fix changed unsafe behavior from #13860 and attempts to fix #14003.

Prior implementations would replace the Register's prefix attribute inplace which is an unsafe operation. The following commits add a secure path for a provisional replacement of a register's prefix name to fix changed unsafe behavior from Qiskit#13860.
@raynelfss raynelfss added the Changelog: None Do not include in changelog label Mar 12, 2025
@raynelfss raynelfss added this to the 2.0.0 milestone Mar 12, 2025
@raynelfss raynelfss requested a review from a team as a code owner March 12, 2025 15:17
@qiskit-bot
Copy link
Collaborator

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

  • @Qiskit/terra-core

@raynelfss raynelfss added the bug Something isn't working label Mar 12, 2025
@coveralls
Copy link

coveralls commented Mar 12, 2025

Pull Request Test Coverage Report for Build 13925424331

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 16 of 17 (94.12%) changed or added relevant lines in 3 files are covered.
  • 961 unchanged lines in 19 files lost coverage.
  • Overall coverage decreased (-0.06%) to 88.061%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/circuit/quantumcircuit.py 1 2 50.0%
Files with Coverage Reduction New Missed Lines %
crates/accelerate/src/gate_metrics.rs 1 97.06%
crates/accelerate/src/unitary_synthesis.rs 1 94.69%
qiskit/transpiler/passes/optimization/template_matching/template_substitution.py 1 93.27%
crates/accelerate/src/synthesis/clifford/bm_synthesis.rs 2 99.22%
crates/accelerate/src/synthesis/clifford/greedy_synthesis.rs 3 97.92%
crates/qasm2/src/lex.rs 3 92.73%
crates/accelerate/src/remove_identity_equiv.rs 5 94.44%
crates/accelerate/src/gate_direction.rs 7 96.95%
qiskit/circuit/tools/pi_check.py 9 91.3%
qiskit/circuit/parameterexpression.py 10 96.74%
Totals Coverage Status
Change from base Build 13813281684: -0.06%
Covered Lines: 72592
Relevant Lines: 82434

💛 - Coveralls

Copy link
Contributor

@ElePT ElePT left a comment

Choose a reason for hiding this comment

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

It would be nice to have a unit test for this path. The reproducing code that I found looks quite hacky, and this is probably why the issue hadn't surfaced until now. It's a test for the Aer estimator primitive that builds a circuit, adds measurements (via circuit.measure_all()), and then calls a part of the aer estimator code that does:

  circuit = self._circuits[i].copy()
  circuit.measure_all()
  .... # stuff
  circuit.remove_final_measurements()

Calling measure_all() twice after a shallow copy doesn't look like a great thing to do, but I guess we shouldn't hit this error if someone does it. I can see if I can isolate this into a more reasonable example.

Copy link
Contributor

@kevinhartman kevinhartman left a comment

Choose a reason for hiding this comment

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

Looks good to me after @ElePT's comment is addressed!

Copy link
Contributor

@ElePT ElePT left a comment

Choose a reason for hiding this comment

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

Thanks a lot! The PR looks good to me too. Here is the minimal reproducible code for the issue, I can't add it as a direct suggestion but I think it's enough to test this fix:

from qiskit import QuantumCircuit

qc = QuantumCircuit(1)
qc.measure_all()
qc.measure_all()

@ElePT ElePT added the stable backport potential The bug might be minimal and/or import enough to be port to stable label Mar 17, 2025
Copy link
Contributor

@kevinhartman kevinhartman left a comment

Choose a reason for hiding this comment

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

Looks like you've got a linter error in CI as well 🙂

ElePT
ElePT previously approved these changes Mar 18, 2025
Copy link
Contributor

@ElePT ElePT left a comment

Choose a reason for hiding this comment

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

LGTM, I found a tiny typo in the test comment but it's not important.

@ElePT ElePT enabled auto-merge March 18, 2025 15:20
@ElePT ElePT added this pull request to the merge queue Mar 18, 2025
Merged via the queue into Qiskit:main with commit 56a16ab Mar 18, 2025
20 checks passed
mergify bot pushed a commit that referenced this pull request Mar 18, 2025
* FIx: Create an internal path for custom prefixes in registers.

Prior implementations would replace the Register's prefix attribute inplace which is an unsafe operation. The following commits add a secure path for a provisional replacement of a register's prefix name to fix changed unsafe behavior from #13860.

* Fix: Address review comments
- Add test-case

* Apply suggestions from code review

Co-authored-by: Kevin Hartman <[email protected]>

* Fix: Address more review comments

* Fix: Lint error

* Update test/python/circuit/test_circuit_operations.py

Co-authored-by: Elena Peña Tapia <[email protected]>

---------

Co-authored-by: Kevin Hartman <[email protected]>
Co-authored-by: Elena Peña Tapia <[email protected]>
(cherry picked from commit 56a16ab)
github-merge-queue bot pushed a commit that referenced this pull request Mar 18, 2025
…) (#14048)

* FIx: Create an internal path for custom prefixes in registers.

Prior implementations would replace the Register's prefix attribute inplace which is an unsafe operation. The following commits add a secure path for a provisional replacement of a register's prefix name to fix changed unsafe behavior from #13860.

* Fix: Address review comments
- Add test-case

* Apply suggestions from code review

Co-authored-by: Kevin Hartman <[email protected]>

* Fix: Address more review comments

* Fix: Lint error

* Update test/python/circuit/test_circuit_operations.py

Co-authored-by: Elena Peña Tapia <[email protected]>

---------

Co-authored-by: Kevin Hartman <[email protected]>
Co-authored-by: Elena Peña Tapia <[email protected]>
(cherry picked from commit 56a16ab)

Co-authored-by: Raynel Sanchez <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Changelog: None Do not include in changelog stable backport potential The bug might be minimal and/or import enough to be port to stable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug in QuantumCircuit accessing ClassicalRegister attribute
5 participants