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

deprecate GIL refs in function argument #3847

Merged
merged 8 commits into from
Mar 20, 2024

Conversation

davidhewitt
Copy link
Member

This was built on-stream today, the goal is to add a deprecation warning to #[pyfunction] and #[pymethods] arguments which take a GIL ref. The span is wrong but otherwise this technique works. The code is also a bit of a bodge job as I was floundering a bit on-stream trying to figure out how to get it to work 😂

This is not in a reviewable state but I felt like pushing the code anyway so it can be found easily when I get a chance to tidy it up.

@LilyFoote
Copy link
Contributor

Will we get warnings from #[pymodule] with this?

@davidhewitt
Copy link
Member Author

No, and also not for pass_module or slf: &PyCell I think. I will add all those to the tracking issue.

@davidhewitt davidhewitt force-pushed the gil-ref-deprecation branch 2 times, most recently from 548d7d8 to 0854cab Compare March 20, 2024 00:12
@davidhewitt davidhewitt changed the title [WIP] deprecate GIL refs in function argument deprecate GIL refs in function argument Mar 20, 2024
@davidhewitt davidhewitt marked this pull request as ready for review March 20, 2024 00:12
@davidhewitt
Copy link
Member Author

Ok, with luck, this now passes CI!

src/instance.rs Outdated Show resolved Hide resolved
@davidhewitt davidhewitt force-pushed the gil-ref-deprecation branch from 0854cab to 61831cc Compare March 20, 2024 08:49
@davidhewitt davidhewitt force-pushed the gil-ref-deprecation branch from 61831cc to d1dc4ce Compare March 20, 2024 09:25
Copy link
Contributor

@Icxolu Icxolu left a comment

Choose a reason for hiding this comment

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

Very nice 🚀! I really like how Holders turned out, and how it could be reused to handle the self_arg as well. I found a few places to double check on (plus some very minor nits).

pytests/src/pyclasses.rs Outdated Show resolved Hide resolved
tests/test_methods.rs Outdated Show resolved Hide resolved
tests/test_proto_methods.rs Outdated Show resolved Hide resolved
pyo3-macros-backend/src/params.rs Outdated Show resolved Hide resolved
Comment on lines +19 to +31
error[E0271]: type mismatch resolving `<Foo as PyClass>::Frozen == False`
--> tests/ui/invalid_frozen_pyclass_borrow.rs:9:1
|
9 | #[pymethods]
| ^^^^^^^^^^^^ expected `False`, found `True`
|
note: required by a bound in `PyRefMut`
--> src/pycell.rs
|
| pub struct PyRefMut<'p, T: PyClass<Frozen = False>> {
| ^^^^^^^^^^^^^^ required by this bound in `PyRefMut`
= note: this error originates in the attribute macro `pymethods` (in Nightly builds, run with -Z macro-backtrace for more info)

Copy link
Contributor

Choose a reason for hiding this comment

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

Given the corresponding test did not change i'm not sure where this one comes from and if it is expected

Copy link
Member Author

Choose a reason for hiding this comment

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

I think what's happened is that the spans for the code in question are now mixed, so the error has duplicated onto both spans.

It's not really expected but I think it'll also be quite hard to fix without a bit of trial-and-error on spans. I think for now I'm inclined to leave this as-is and hope when we remove these warnings in a release or two it cleans up again. It is a touch ugly though. :(

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, sounds reasonable

tests/ui/invalid_pymethod_enum.stderr Show resolved Hide resolved
@davidhewitt davidhewitt force-pushed the gil-ref-deprecation branch from 2a66640 to 094787e Compare March 20, 2024 19:25
@davidhewitt davidhewitt force-pushed the gil-ref-deprecation branch from 6f89e57 to 81b71f6 Compare March 20, 2024 20:17
@davidhewitt davidhewitt added this pull request to the merge queue Mar 20, 2024
Merged via the queue into PyO3:main with commit 870a4bb Mar 20, 2024
42 checks passed
@davidhewitt davidhewitt deleted the gil-ref-deprecation branch March 20, 2024 23:20
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.

4 participants