-
Notifications
You must be signed in to change notification settings - Fork 779
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 &PyModule
as #[pymodule]
argument type
#3936
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, nice solution! I think that's a workable approach.
A compile error test would be sufficient. Maybe add to tests/ui/deprecations.rs
?
pyo3-macros-backend/src/module.rs
Outdated
#(#attrs)* | ||
#vis #sig { | ||
#(#extractors)* | ||
#block | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than splitting up the function it might be possible to insert these into the front of function.block.stmts
.
pyo3-macros-backend/src/module.rs
Outdated
if let syn::Pat::Ident(pat_ident) = &*pat_type.pat { | ||
let ident = &pat_ident.ident; | ||
return Some(quote_spanned! { pat_type.span() => { | ||
let (_, e) = #pyo3_path::impl_::pymethods::inspect_type(#ident); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might need to assign the argument back out:
let (_, e) = #pyo3_path::impl_::pymethods::inspect_type(#ident); | |
let (#ident, e) = #pyo3_path::impl_::pymethods::inspect_type(#ident); |
(I think this would matter for edge cases where the module is using Bound<'_, PyModule>
or Py<PyModule>
for some reason, and inspect_type
would transfer ownership rather than copy.)
I think this is ready now, and it actually found a few placed that we haven't updated yet, including two doc tests 🎉 |
b40e530
to
a85157e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, many thanks!
I'm sure there are quite a few more GIL Refs sneaking around various edges of the codebase... Hopefully we can eliminate a few more from the docs before 0.21 final. The road to 0.22 is littered with removing most of the rest of them PyO3's test suite, I think 😂
This uses the framework of #3847 to deprecate
&PyModule
as#[pymodule]
argument type.This is done by modifying the original module function on macro expansion and injection of the gil-ref extractor with deref specialization. This can (and should) be reverted once the gil-refs are fully removed.
I assume this will need some tests, I will add those if we want go with this