Skip to content

change return type of intern! macro to &Bound<PyString>#3781

Merged
davidhewitt merged 3 commits intoPyO3:mainfrom
davidhewitt:intern-bound
Jan 30, 2024
Merged

change return type of intern! macro to &Bound<PyString>#3781
davidhewitt merged 3 commits intoPyO3:mainfrom
davidhewitt:intern-bound

Conversation

@davidhewitt
Copy link
Member

Split from #3606

Primarily, this PR adds intern_bound! macro and deprecates current intern! form, to move that macro off the GIL Refs API. There are a number of locations where internal code is adjusted for this. Documentation is also updated, and in some cases reads not so nicely any more (e.g. "example: intern_bound!ing the attribute name"), but I think that I'd rather not make adjustments to the docs further in this PR, especially as we can revert those adjustments in 0.23.

This is built on top of the first commit which adds PyString::intern_bound and PyString::from_object_bound, as they were not added in #3774.

@davidhewitt davidhewitt added the CI-skip-changelog Skip checking changelog entry label Jan 30, 2024
Copy link
Member

@mejrs mejrs left a comment

Choose a reason for hiding this comment

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

Do we plan to remove intern! and rename intern_bound! to intern! eventually? If so, given it doesn't seem to break much code maybe it makes sense to just change intern! now?

@davidhewitt
Copy link
Member Author

Do we plan to remove intern! and rename intern_bound! to intern! eventually? If so, given it doesn't seem to break much code maybe it makes sense to just change intern! now?

I considered this. I think it's definitely the case that most user code will have smaller diffs by just renaming this straight away, at the cost of a few possible type errors in positions where intern! isn't directly inside of a getattr.

We could certainly start with the rename and wait for user feedback.

I'm feeling like it's worth creating a 0.21 beta release, so this would certainly be something we could correct for the final release if the feedback from the beta is that changing intern! return type is annoying.

If I don't hear any reason otherwise, maybe I'll adjust this PR to just change the return type of intern! in the next day or so and then merge this.

@alex
Copy link
Contributor

alex commented Jan 30, 2024

If pyca/cryptography is any indication, 99% of our intern!() invocations are immediately passed as an argument to call_method or getattr, so I think just changing the return type is best.

@davidhewitt
Copy link
Member Author

Ok, pushed that as a separate commit so it's easy to revert if needed later, will merge & move forward now.

@davidhewitt davidhewitt enabled auto-merge January 30, 2024 13:29
@davidhewitt davidhewitt changed the title add intern_bound! macro change return type of intern! macro to &Bound<PyString> Jan 30, 2024
@davidhewitt davidhewitt added this pull request to the merge queue Jan 30, 2024
Merged via the queue into PyO3:main with commit 6040d93 Jan 30, 2024
@davidhewitt davidhewitt deleted the intern-bound branch January 30, 2024 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI-skip-changelog Skip checking changelog entry

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants