From 1cfba4d4cc88bc861de53f0c3c645f5c9eb7198d Mon Sep 17 00:00:00 2001 From: Icxolu <10486322+Icxolu@users.noreply.github.com> Date: Wed, 6 Mar 2024 23:34:53 +0100 Subject: [PATCH 1/6] deprecate `&PyModule` as `#[pymodule]` argument type --- pyo3-macros-backend/src/module.rs | 25 ++++++++++++++++++-- src/impl_/pymethods.rs | 39 +++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+), 2 deletions(-) diff --git a/pyo3-macros-backend/src/module.rs b/pyo3-macros-backend/src/module.rs index 7173fa8647d..54367138783 100644 --- a/pyo3-macros-backend/src/module.rs +++ b/pyo3-macros-backend/src/module.rs @@ -7,7 +7,7 @@ use crate::{ pyfunction::{impl_wrap_pyfunction, PyFunctionOptions}, }; use proc_macro2::TokenStream; -use quote::quote; +use quote::{quote, quote_spanned}; use syn::{ ext::IdentExt, parse::{Parse, ParseStream}, @@ -295,8 +295,29 @@ pub fn pymodule_function_impl(mut function: syn::ItemFn) -> Result } module_args.push(quote!(::std::convert::Into::into(BoundRef(module)))); + let syn::ItemFn { + attrs, sig, block, .. + } = &function; + + let extractors = sig.inputs.iter().filter_map(|param| { + if let syn::FnArg::Typed(pat_type) = param { + 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); + let _ = e.extract_gil_ref(); + }}); + } + } + None + }); + Ok(quote! { - #function + #(#attrs)* + #vis #sig { + #(#extractors)* + #block + } #vis mod #ident { #initialization } diff --git a/src/impl_/pymethods.rs b/src/impl_/pymethods.rs index df89dba7dbd..c63f3e88055 100644 --- a/src/impl_/pymethods.rs +++ b/src/impl_/pymethods.rs @@ -580,3 +580,42 @@ pub unsafe fn tp_new_impl( .create_class_object_of_type(py, target_type) .map(Bound::into_ptr) } + +pub fn inspect_type(t: T) -> (T, Extractor) { + (t, Extractor::new()) +} + +pub struct Extractor(NotAGilRef); +pub struct NotAGilRef(std::marker::PhantomData); + +pub trait IsGilRef {} + +impl IsGilRef for &'_ T {} + +impl Extractor { + pub fn new() -> Self { + Extractor(NotAGilRef(std::marker::PhantomData)) + } +} + +impl Extractor { + #[cfg_attr( + not(feature = "gil-refs"), + deprecated( + since = "0.21.0", + note = "use `&Bound<'_, T>` instead for this function argument" + ) + )] + pub fn extract_gil_ref(&self) {} +} + +impl NotAGilRef { + pub fn extract_gil_ref(&self) {} +} + +impl std::ops::Deref for Extractor { + type Target = NotAGilRef; + fn deref(&self) -> &Self::Target { + &self.0 + } +} From 34acc5025ad5da48d487f624f606cc3ad301fec0 Mon Sep 17 00:00:00 2001 From: Icxolu <10486322+Icxolu@users.noreply.github.com> Date: Thu, 7 Mar 2024 18:31:54 +0100 Subject: [PATCH 2/6] cleanup --- pyo3-macros-backend/src/module.rs | 46 +++++++++++++++++-------------- src/impl_/pymethods.rs | 1 + 2 files changed, 26 insertions(+), 21 deletions(-) diff --git a/pyo3-macros-backend/src/module.rs b/pyo3-macros-backend/src/module.rs index 54367138783..fb02c074996 100644 --- a/pyo3-macros-backend/src/module.rs +++ b/pyo3-macros-backend/src/module.rs @@ -7,10 +7,11 @@ use crate::{ pyfunction::{impl_wrap_pyfunction, PyFunctionOptions}, }; use proc_macro2::TokenStream; -use quote::{quote, quote_spanned}; +use quote::quote; use syn::{ ext::IdentExt, parse::{Parse, ParseStream}, + parse_quote, parse_quote_spanned, spanned::Spanned, token::Comma, Item, Path, Result, @@ -281,6 +282,7 @@ pub fn pymodule_function_impl(mut function: syn::ItemFn) -> Result let options = PyModuleOptions::from_attrs(&mut function.attrs)?; process_functions_in_module(&options, &mut function)?; let ctx = &Ctx::new(&options.krate); + let stmts = std::mem::take(&mut function.block.stmts); let Ctx { pyo3_path } = ctx; let ident = &function.sig.ident; let vis = &function.vis; @@ -295,29 +297,31 @@ pub fn pymodule_function_impl(mut function: syn::ItemFn) -> Result } module_args.push(quote!(::std::convert::Into::into(BoundRef(module)))); - let syn::ItemFn { - attrs, sig, block, .. - } = &function; - - let extractors = sig.inputs.iter().filter_map(|param| { - if let syn::FnArg::Typed(pat_type) = param { - 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); - let _ = e.extract_gil_ref(); - }}); + let extractors = function + .sig + .inputs + .iter() + .filter_map(|param| { + if let syn::FnArg::Typed(pat_type) = param { + if let syn::Pat::Ident(pat_ident) = &*pat_type.pat { + let ident = &pat_ident.ident; + return Some([ + parse_quote! { let (#ident, e) = #pyo3_path::impl_::pymethods::inspect_type(#ident); }, + parse_quote_spanned! { pat_type.span() => e.extract_gil_ref(); }, + ]); + } } - } - None - }); + None + }) + .flatten(); + + function.block.stmts = extractors.chain(stmts).collect(); + function + .attrs + .push(parse_quote!(#[allow(clippy::used_underscore_binding)])); Ok(quote! { - #(#attrs)* - #vis #sig { - #(#extractors)* - #block - } + #function #vis mod #ident { #initialization } diff --git a/src/impl_/pymethods.rs b/src/impl_/pymethods.rs index c63f3e88055..a7df90b572d 100644 --- a/src/impl_/pymethods.rs +++ b/src/impl_/pymethods.rs @@ -593,6 +593,7 @@ pub trait IsGilRef {} impl IsGilRef for &'_ T {} impl Extractor { + #[allow(clippy::new_without_default)] pub fn new() -> Self { Extractor(NotAGilRef(std::marker::PhantomData)) } From 0d3e76f5fd3c8fa58b2c72a508b19b04f6194358 Mon Sep 17 00:00:00 2001 From: Icxolu <10486322+Icxolu@users.noreply.github.com> Date: Thu, 7 Mar 2024 18:41:49 +0100 Subject: [PATCH 3/6] add ui tests --- tests/ui/deprecations.rs | 35 +++++++++++++++++++++++++++++++++++ tests/ui/deprecations.stderr | 12 ++++++++++++ 2 files changed, 47 insertions(+) diff --git a/tests/ui/deprecations.rs b/tests/ui/deprecations.rs index 4177dd6da22..06bcf8197a7 100644 --- a/tests/ui/deprecations.rs +++ b/tests/ui/deprecations.rs @@ -14,3 +14,38 @@ impl MyClass { } fn main() {} + +#[pyfunction] +fn double(x: usize) -> usize { + x * 2 +} + +#[pymodule] +fn module_gil_ref(m: &PyModule) -> PyResult<()> { + m.add_function(wrap_pyfunction!(double, m)?)?; + Ok(()) +} + +#[pymodule] +fn module_gil_ref_with_explicit_py_arg(_py: Python<'_>, m: &PyModule) -> PyResult<()> { + m.add_function(wrap_pyfunction!(double, m)?)?; + Ok(()) +} + +#[pymodule] +fn module_bound(m: &Bound<'_, PyModule>) -> PyResult<()> { + m.add_function(wrap_pyfunction!(double, m)?)?; + Ok(()) +} + +#[pymodule] +fn module_bound_with_explicit_py_arg(_py: Python<'_>, m: &Bound<'_, PyModule>) -> PyResult<()> { + m.add_function(wrap_pyfunction!(double, m)?)?; + Ok(()) +} + +#[pymodule] +fn module_bound_by_value(m: Bound<'_, PyModule>) -> PyResult<()> { + m.add_function(wrap_pyfunction!(double, &m)?)?; + Ok(()) +} diff --git a/tests/ui/deprecations.stderr b/tests/ui/deprecations.stderr index a857b5ee420..2440f909019 100644 --- a/tests/ui/deprecations.stderr +++ b/tests/ui/deprecations.stderr @@ -9,3 +9,15 @@ note: the lint level is defined here | 1 | #![deny(deprecated)] | ^^^^^^^^^^ + +error: use of deprecated method `pyo3::methods::Extractor::::extract_gil_ref`: use `&Bound<'_, T>` instead for this function argument + --> tests/ui/deprecations.rs:24:19 + | +24 | fn module_gil_ref(m: &PyModule) -> PyResult<()> { + | ^ + +error: use of deprecated method `pyo3::methods::Extractor::::extract_gil_ref`: use `&Bound<'_, T>` instead for this function argument + --> tests/ui/deprecations.rs:30:57 + | +30 | fn module_gil_ref_with_explicit_py_arg(_py: Python<'_>, m: &PyModule) -> PyResult<()> { + | ^ From d48e1eec7e7f98e2992881b184e2755537a0b7b6 Mon Sep 17 00:00:00 2001 From: Icxolu <10486322+Icxolu@users.noreply.github.com> Date: Thu, 7 Mar 2024 19:01:26 +0100 Subject: [PATCH 4/6] fix deprecations in tests --- guide/src/exception.md | 2 +- guide/src/migration.md | 2 +- pytests/src/lib.rs | 2 +- src/tests/hygiene/pymodule.rs | 2 ++ src/types/module.rs | 8 ++++---- tests/test_module.rs | 2 +- tests/test_no_imports.rs | 1 + 7 files changed, 11 insertions(+), 8 deletions(-) diff --git a/guide/src/exception.md b/guide/src/exception.md index 0be44167760..04550bd4b92 100644 --- a/guide/src/exception.md +++ b/guide/src/exception.md @@ -44,7 +44,7 @@ use pyo3::exceptions::PyException; pyo3::create_exception!(mymodule, CustomError, PyException); #[pymodule] -fn mymodule(py: Python<'_>, m: &PyModule) -> PyResult<()> { +fn mymodule(py: Python<'_>, m: &Bound<'_, PyModule>) -> PyResult<()> { // ... other elements added to module ... m.add("CustomError", py.get_type_bound::())?; diff --git a/guide/src/migration.md b/guide/src/migration.md index 709e7a70488..8537f9a1a69 100644 --- a/guide/src/migration.md +++ b/guide/src/migration.md @@ -847,7 +847,7 @@ fn my_module(_py: Python<'_>, m: &PyModule) -> PyResult<()> { To fix it, make the private submodule visible, e.g. with `pub` or `pub(crate)`. -```rust +```rust,ignore mod foo { use pyo3::prelude::*; diff --git a/pytests/src/lib.rs b/pytests/src/lib.rs index bfd80edb719..cbd65c8012c 100644 --- a/pytests/src/lib.rs +++ b/pytests/src/lib.rs @@ -18,7 +18,7 @@ pub mod sequence; pub mod subclassing; #[pymodule] -fn pyo3_pytests(py: Python<'_>, m: &PyModule) -> PyResult<()> { +fn pyo3_pytests(py: Python<'_>, m: &Bound<'_, PyModule>) -> PyResult<()> { m.add_wrapped(wrap_pymodule!(awaitable::awaitable))?; #[cfg(not(Py_LIMITED_API))] m.add_wrapped(wrap_pymodule!(buf_and_str::buf_and_str))?; diff --git a/src/tests/hygiene/pymodule.rs b/src/tests/hygiene/pymodule.rs index bb49d3823c1..6229708dd06 100644 --- a/src/tests/hygiene/pymodule.rs +++ b/src/tests/hygiene/pymodule.rs @@ -7,12 +7,14 @@ fn do_something(x: i32) -> crate::PyResult { ::std::result::Result::Ok(x) } +#[allow(deprecated)] #[crate::pymodule] #[pyo3(crate = "crate")] fn foo(_py: crate::Python<'_>, _m: &crate::types::PyModule) -> crate::PyResult<()> { ::std::result::Result::Ok(()) } +#[allow(deprecated)] #[crate::pymodule] #[pyo3(crate = "crate")] fn my_module(_py: crate::Python<'_>, m: &crate::types::PyModule) -> crate::PyResult<()> { diff --git a/src/types/module.rs b/src/types/module.rs index d6f3acb2567..993060703e6 100644 --- a/src/types/module.rs +++ b/src/types/module.rs @@ -335,11 +335,11 @@ impl PyModule { /// use pyo3::prelude::*; /// /// #[pymodule] - /// fn my_module(py: Python<'_>, module: &PyModule) -> PyResult<()> { + /// fn my_module(py: Python<'_>, module: &Bound<'_, PyModule>) -> PyResult<()> { /// let submodule = PyModule::new_bound(py, "submodule")?; /// submodule.add("super_useful_constant", "important")?; /// - /// module.add_submodule(submodule.as_gil_ref())?; + /// module.add_submodule(&submodule)?; /// Ok(()) /// } /// ``` @@ -530,11 +530,11 @@ pub trait PyModuleMethods<'py>: crate::sealed::Sealed { /// use pyo3::prelude::*; /// /// #[pymodule] - /// fn my_module(py: Python<'_>, module: &PyModule) -> PyResult<()> { + /// fn my_module(py: Python<'_>, module: &Bound<'_, PyModule>) -> PyResult<()> { /// let submodule = PyModule::new_bound(py, "submodule")?; /// submodule.add("super_useful_constant", "important")?; /// - /// module.add_submodule(submodule.as_gil_ref())?; + /// module.add_submodule(&submodule)?; /// Ok(()) /// } /// ``` diff --git a/tests/test_module.rs b/tests/test_module.rs index d4c4acca90f..0eef3c0e2d1 100644 --- a/tests/test_module.rs +++ b/tests/test_module.rs @@ -118,7 +118,7 @@ fn test_module_with_functions() { /// This module uses a legacy two-argument module function. #[pymodule] -fn module_with_explicit_py_arg(_py: Python<'_>, m: &PyModule) -> PyResult<()> { +fn module_with_explicit_py_arg(_py: Python<'_>, m: &Bound<'_, PyModule>) -> PyResult<()> { m.add_function(wrap_pyfunction!(double, m)?)?; Ok(()) } diff --git a/tests/test_no_imports.rs b/tests/test_no_imports.rs index 69f4b6e4651..4c77cc8e2a0 100644 --- a/tests/test_no_imports.rs +++ b/tests/test_no_imports.rs @@ -10,6 +10,7 @@ fn basic_function(py: pyo3::Python<'_>, x: Option) -> pyo3::PyOb x.unwrap_or_else(|| py.None()) } +#[allow(deprecated)] #[pyo3::pymodule] fn basic_module(_py: pyo3::Python<'_>, m: &pyo3::types::PyModule) -> pyo3::PyResult<()> { #[pyfn(m)] From b2009eae5cf1c2022c718ebce632f3d1187df9a6 Mon Sep 17 00:00:00 2001 From: Icxolu <10486322+Icxolu@users.noreply.github.com> Date: Thu, 7 Mar 2024 19:30:54 +0100 Subject: [PATCH 5/6] fix maturin and setuptools-rust starters --- examples/maturin-starter/src/lib.rs | 2 +- examples/setuptools-rust-starter/src/lib.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/examples/maturin-starter/src/lib.rs b/examples/maturin-starter/src/lib.rs index 8b0748f8311..faa147b2a10 100644 --- a/examples/maturin-starter/src/lib.rs +++ b/examples/maturin-starter/src/lib.rs @@ -20,7 +20,7 @@ impl ExampleClass { /// An example module implemented in Rust using PyO3. #[pymodule] -fn maturin_starter(py: Python<'_>, m: &PyModule) -> PyResult<()> { +fn maturin_starter(py: Python<'_>, m: &Bound<'_, PyModule>) -> PyResult<()> { m.add_class::()?; m.add_wrapped(wrap_pymodule!(submodule::submodule))?; diff --git a/examples/setuptools-rust-starter/src/lib.rs b/examples/setuptools-rust-starter/src/lib.rs index 2bcd411c238..d31284be7a3 100644 --- a/examples/setuptools-rust-starter/src/lib.rs +++ b/examples/setuptools-rust-starter/src/lib.rs @@ -20,7 +20,7 @@ impl ExampleClass { /// An example module implemented in Rust using PyO3. #[pymodule] -fn _setuptools_rust_starter(py: Python<'_>, m: &PyModule) -> PyResult<()> { +fn _setuptools_rust_starter(py: Python<'_>, m: &Bound<'_, PyModule>) -> PyResult<()> { m.add_class::()?; m.add_wrapped(wrap_pymodule!(submodule::submodule))?; From a85157e2285feea2a202a7b8b4b904ae4c6650c6 Mon Sep 17 00:00:00 2001 From: Icxolu <10486322+Icxolu@users.noreply.github.com> Date: Thu, 7 Mar 2024 19:31:16 +0100 Subject: [PATCH 6/6] run `deprecated` ui test only when `gil-refs` as disabled --- tests/test_compile_error.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/test_compile_error.rs b/tests/test_compile_error.rs index f5fbec7a533..3f1052c3077 100644 --- a/tests/test_compile_error.rs +++ b/tests/test_compile_error.rs @@ -18,6 +18,7 @@ fn test_compile_errors() { t.compile_fail("tests/ui/invalid_pymethod_names.rs"); t.compile_fail("tests/ui/invalid_pymodule_args.rs"); t.compile_fail("tests/ui/reject_generics.rs"); + #[cfg(not(feature = "gil-refs"))] t.compile_fail("tests/ui/deprecations.rs"); t.compile_fail("tests/ui/invalid_closure.rs"); t.compile_fail("tests/ui/pyclass_send.rs");