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

Remove need for mutable reference to static (backport #13705) #13712

Merged
merged 3 commits into from
Jan 22, 2025

Conversation

mergify[bot]
Copy link
Contributor

@mergify mergify bot commented Jan 21, 2025

Mutable references to static data are inherently unsafe and typically unsound in Rust, because statics are implicitly shared between threads, and the borrow checker can only enforce the shared/exclusive reference limitations within a single thread here.

This just moves the thread-exclusion logic into the individual elements of the static, where the GILOnceCell can correctly handle the runtime exclusion of multiple threads. (The GIL is no longer suitable for thread exclusion if we were doing a freethreaded Python build, but that's a problem we have all over Qiskit, and would need to change to OnceLock or the like.)

Summary

Details and comments

Minor conflict with the update to PyO3 0.23, but only a small syntactic one - the import_bound becomes import. The logic still works.


This is an automatic backport of pull request #13705 done by Mergify.

Mutable references to static data are inherently unsafe and typically
unsound in Rust, because statics are implicitly shared between threads,
and the borrow checker can only enforce the shared/exclusive reference
limitations within a single thread here.

This just moves the thread-exclusion logic into the individual elements
of the `static`, where the `GILOnceCell` can correctly handle the
runtime exclusion of multiple threads. (The GIL is no longer suitable
for thread exclusion if we were doing a freethreaded Python build, but
that's a problem we have all over Qiskit, and would need to change to
`OnceLock` or the like.)

(cherry picked from commit 7b0b6fc)

# Conflicts:
#	crates/circuit/src/imports.rs
@mergify mergify bot requested a review from a team as a code owner January 21, 2025 22:19
@mergify mergify bot added the conflicts used by mergify when there are conflicts in a port label Jan 21, 2025
Copy link
Contributor Author

mergify bot commented Jan 21, 2025

Cherry-pick of 7b0b6fc has failed:

On branch mergify/bp/stable/1.3/pr-13705
Your branch is up to date with 'origin/stable/1.3'.

You are currently cherry-picking commit 7b0b6fcd.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	modified:   crates/circuit/src/operations.rs

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   crates/circuit/src/imports.rs

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

@qiskit-bot
Copy link
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

@github-actions github-actions bot added type: qa Issues and PRs that relate to testing and code quality Changelog: None Do not include in changelog Rust This PR or issue is related to Rust code in the repository labels Jan 21, 2025
@ElePT ElePT removed the conflicts used by mergify when there are conflicts in a port label Jan 22, 2025
ElePT
ElePT previously approved these changes Jan 22, 2025
@ElePT ElePT enabled auto-merge January 22, 2025 08:19
@coveralls
Copy link

Pull Request Test Coverage Report for Build 12904198417

Details

  • 6 of 6 (100.0%) changed or added relevant lines in 2 files are covered.
  • 11 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+0.001%) to 88.916%

Files with Coverage Reduction New Missed Lines %
crates/accelerate/src/unitary_synthesis.rs 1 93.21%
crates/qasm2/src/lex.rs 4 92.73%
crates/qasm2/src/parse.rs 6 96.69%
Totals Coverage Status
Change from base Build 12896571875: 0.001%
Covered Lines: 79156
Relevant Lines: 89023

💛 - Coveralls

@ElePT ElePT added this pull request to the merge queue Jan 22, 2025
Merged via the queue into stable/1.3 with commit 48a2e5b Jan 22, 2025
19 checks passed
@mtreinish mtreinish deleted the mergify/bp/stable/1.3/pr-13705 branch February 19, 2025 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: None Do not include in changelog Rust This PR or issue is related to Rust code in the repository type: qa Issues and PRs that relate to testing and code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants