From 908e661237a4bc31939473f80047b67a7d129bda Mon Sep 17 00:00:00 2001 From: Icxolu <10486322+Icxolu@users.noreply.github.com> Date: Sat, 9 Mar 2024 10:52:12 +0100 Subject: [PATCH] deprecate gil-refs in "self" position (#3943) * deprecate gil-refs in "self" position * feature gate explicit gil-ref tests * fix MSRV * adjust bracketing --------- Co-authored-by: David Hewitt --- pyo3-macros-backend/src/method.rs | 72 ++++++++++++++++++++++++++----- tests/test_class_basics.rs | 8 +++- tests/test_methods.rs | 1 + tests/test_module.rs | 8 +++- tests/ui/deprecations.rs | 23 ++++++++++ tests/ui/deprecations.stderr | 38 +++++++++++++--- 6 files changed, 129 insertions(+), 21 deletions(-) diff --git a/pyo3-macros-backend/src/method.rs b/pyo3-macros-backend/src/method.rs index 9a8cb828..3cc2a96e 100644 --- a/pyo3-macros-backend/src/method.rs +++ b/pyo3-macros-backend/src/method.rs @@ -128,24 +128,24 @@ impl FnType { } FnType::FnClass(span) | FnType::FnNewClass(span) => { let py = syn::Ident::new("py", Span::call_site()); - let slf: Ident = syn::Ident::new("_slf", Span::call_site()); + let slf: Ident = syn::Ident::new("_slf_ref", Span::call_site()); let pyo3_path = pyo3_path.to_tokens_spanned(*span); quote_spanned! { *span => #[allow(clippy::useless_conversion)] ::std::convert::Into::into( - #pyo3_path::impl_::pymethods::BoundRef::ref_from_ptr(#py, &#slf.cast()) + #pyo3_path::impl_::pymethods::BoundRef::ref_from_ptr(#py, &*(#slf as *const _ as *const *mut _)) .downcast_unchecked::<#pyo3_path::types::PyType>() ), } } FnType::FnModule(span) => { let py = syn::Ident::new("py", Span::call_site()); - let slf: Ident = syn::Ident::new("_slf", Span::call_site()); + let slf: Ident = syn::Ident::new("_slf_ref", Span::call_site()); let pyo3_path = pyo3_path.to_tokens_spanned(*span); quote_spanned! { *span => #[allow(clippy::useless_conversion)] ::std::convert::Into::into( - #pyo3_path::impl_::pymethods::BoundRef::ref_from_ptr(#py, &#slf.cast()) + #pyo3_path::impl_::pymethods::BoundRef::ref_from_ptr(#py, &*(#slf as *const _ as *const *mut _)) .downcast_unchecked::<#pyo3_path::types::PyModule>() ), } @@ -519,7 +519,9 @@ impl<'a> FnSpec<'a> { ); } - let rust_call = |args: Vec, holders: &mut Vec| { + let rust_call = |args: Vec, + self_e: &syn::Ident, + holders: &mut Vec| { let self_arg = self.tp.self_arg(cls, ExtractErrorMode::Raise, holders, ctx); let call = if self.asyncness.is_some() { @@ -538,6 +540,7 @@ impl<'a> FnSpec<'a> { holders.pop().unwrap(); // does not actually use holder created by `self_arg` quote! {{ + #self_e = #pyo3_path::impl_::pymethods::Extractor::<()>::new(); let __guard = #pyo3_path::impl_::coroutine::RefGuard::<#cls>::new(&#pyo3_path::impl_::pymethods::BoundRef::ref_from_ptr(py, &_slf))?; async move { function(&__guard, #(#args),*).await } }} @@ -546,11 +549,25 @@ impl<'a> FnSpec<'a> { holders.pop().unwrap(); // does not actually use holder created by `self_arg` quote! {{ + #self_e = #pyo3_path::impl_::pymethods::Extractor::<()>::new(); let mut __guard = #pyo3_path::impl_::coroutine::RefMutGuard::<#cls>::new(&#pyo3_path::impl_::pymethods::BoundRef::ref_from_ptr(py, &_slf))?; async move { function(&mut __guard, #(#args),*).await } }} } - _ => quote! { function(#self_arg #(#args),*) }, + _ => { + if self_arg.is_empty() { + quote! {{ + #self_e = #pyo3_path::impl_::pymethods::Extractor::<()>::new(); + function(#(#args),*) + }} + } else { + quote! { function({ + let (self_arg, e) = #pyo3_path::impl_::pymethods::inspect_type(#self_arg); + #self_e = e; + self_arg + }, #(#args),*) } + } + } }; let mut call = quote! {{ let future = #future; @@ -569,10 +586,24 @@ impl<'a> FnSpec<'a> { }}; } call + } else if self_arg.is_empty() { + quote! {{ + #self_e = #pyo3_path::impl_::pymethods::Extractor::<()>::new(); + function(#(#args),*) + }} } else { - quote! { function(#self_arg #(#args),*) } + quote! { + function({ + let (self_arg, e) = #pyo3_path::impl_::pymethods::inspect_type(#self_arg); + #self_e = e; + self_arg + }, #(#args),*) + } }; - quotes::map_result_into_ptr(quotes::ok_wrap(call, ctx), ctx) + ( + quotes::map_result_into_ptr(quotes::ok_wrap(call, ctx), ctx), + self_arg.span(), + ) }; let func_name = &self.name; @@ -582,6 +613,7 @@ impl<'a> FnSpec<'a> { quote!(#func_name) }; + let self_e = syn::Ident::new("self_e", Span::call_site()); Ok(match self.convention { CallingConvention::Noargs => { let mut holders = Vec::new(); @@ -599,16 +631,21 @@ impl<'a> FnSpec<'a> { } }) .collect(); - let call = rust_call(args, &mut holders); + let (call, self_arg_span) = rust_call(args, &self_e, &mut holders); + let extract_gil_ref = + quote_spanned! { self_arg_span => #self_e.extract_gil_ref(); }; quote! { unsafe fn #ident<'py>( py: #pyo3_path::Python<'py>, _slf: *mut #pyo3_path::ffi::PyObject, ) -> #pyo3_path::PyResult<*mut #pyo3_path::ffi::PyObject> { + let _slf_ref = &_slf; let function = #rust_name; // Shadow the function name to avoid #3017 + let #self_e; #( #holders )* let result = #call; + #extract_gil_ref result } } @@ -616,7 +653,10 @@ impl<'a> FnSpec<'a> { CallingConvention::Fastcall => { let mut holders = Vec::new(); let (arg_convert, args) = impl_arg_params(self, cls, true, &mut holders, ctx)?; - let call = rust_call(args, &mut holders); + let (call, self_arg_span) = rust_call(args, &self_e, &mut holders); + let extract_gil_ref = + quote_spanned! { self_arg_span => #self_e.extract_gil_ref(); }; + quote! { unsafe fn #ident<'py>( py: #pyo3_path::Python<'py>, @@ -625,10 +665,13 @@ impl<'a> FnSpec<'a> { _nargs: #pyo3_path::ffi::Py_ssize_t, _kwnames: *mut #pyo3_path::ffi::PyObject ) -> #pyo3_path::PyResult<*mut #pyo3_path::ffi::PyObject> { + let _slf_ref = &_slf; let function = #rust_name; // Shadow the function name to avoid #3017 + let #self_e; #arg_convert #( #holders )* let result = #call; + #extract_gil_ref result } } @@ -636,7 +679,10 @@ impl<'a> FnSpec<'a> { CallingConvention::Varargs => { let mut holders = Vec::new(); let (arg_convert, args) = impl_arg_params(self, cls, false, &mut holders, ctx)?; - let call = rust_call(args, &mut holders); + let (call, self_arg_span) = rust_call(args, &self_e, &mut holders); + let extract_gil_ref = + quote_spanned! { self_arg_span => #self_e.extract_gil_ref(); }; + quote! { unsafe fn #ident<'py>( py: #pyo3_path::Python<'py>, @@ -644,10 +690,13 @@ impl<'a> FnSpec<'a> { _args: *mut #pyo3_path::ffi::PyObject, _kwargs: *mut #pyo3_path::ffi::PyObject ) -> #pyo3_path::PyResult<*mut #pyo3_path::ffi::PyObject> { + let _slf_ref = &_slf; let function = #rust_name; // Shadow the function name to avoid #3017 + let #self_e; #arg_convert #( #holders )* let result = #call; + #extract_gil_ref result } } @@ -667,6 +716,7 @@ impl<'a> FnSpec<'a> { _kwargs: *mut #pyo3_path::ffi::PyObject ) -> #pyo3_path::PyResult<*mut #pyo3_path::ffi::PyObject> { use #pyo3_path::callback::IntoPyCallbackOutput; + let _slf_ref = &_slf; let function = #rust_name; // Shadow the function name to avoid #3017 #arg_convert #( #holders )* diff --git a/tests/test_class_basics.rs b/tests/test_class_basics.rs index 21ccc4a6..8c6a1c04 100644 --- a/tests/test_class_basics.rs +++ b/tests/test_class_basics.rs @@ -307,6 +307,7 @@ impl ClassWithFromPyWithMethods { } #[classmethod] + #[cfg(feature = "gil-refs")] fn classmethod_gil_ref( _cls: &PyType, #[pyo3(from_py_with = "PyAny::len")] argument: usize, @@ -324,16 +325,19 @@ impl ClassWithFromPyWithMethods { fn test_pymethods_from_py_with() { Python::with_gil(|py| { let instance = Py::new(py, ClassWithFromPyWithMethods {}).unwrap(); + let has_gil_refs = cfg!(feature = "gil-refs"); py_run!( py, - instance, + instance + has_gil_refs, r#" arg = {1: 1, 2: 3} assert instance.instance_method(arg) == 2 assert instance.classmethod(arg) == 2 - assert instance.classmethod_gil_ref(arg) == 2 + if has_gil_refs: + assert instance.classmethod_gil_ref(arg) == 2 assert instance.staticmethod(arg) == 2 "# ); diff --git a/tests/test_methods.rs b/tests/test_methods.rs index 68a004bd..a9e7d37d 100644 --- a/tests/test_methods.rs +++ b/tests/test_methods.rs @@ -78,6 +78,7 @@ impl ClassMethod { #[classmethod] /// Test class method. + #[cfg(feature = "gil-refs")] fn method_gil_ref(cls: &PyType) -> PyResult { Ok(format!("{}.method()!", cls.qualname()?)) } diff --git a/tests/test_module.rs b/tests/test_module.rs index 0eef3c0e..5760c3eb 100644 --- a/tests/test_module.rs +++ b/tests/test_module.rs @@ -45,7 +45,7 @@ fn module_with_functions(m: &Bound<'_, PyModule>) -> PyResult<()> { #[pyfn(m)] #[pyo3(pass_module)] - fn with_module(module: &PyModule) -> PyResult<&str> { + fn with_module<'py>(module: &Bound<'py, PyModule>) -> PyResult> { module.name() } @@ -373,6 +373,7 @@ fn pyfunction_with_module<'py>(module: &Bound<'py, PyModule>) -> PyResult PyResult<&str> { module.name() } @@ -427,6 +428,7 @@ fn pyfunction_with_module_and_args_kwargs<'py>( #[pyfunction] #[pyo3(pass_module)] +#[cfg(feature = "gil-refs")] fn pyfunction_with_pass_module_in_attribute(module: &PyModule) -> PyResult<&str> { module.name() } @@ -434,12 +436,14 @@ fn pyfunction_with_pass_module_in_attribute(module: &PyModule) -> PyResult<&str> #[pymodule] fn module_with_functions_with_module(m: &Bound<'_, PyModule>) -> PyResult<()> { m.add_function(wrap_pyfunction!(pyfunction_with_module, m)?)?; + #[cfg(feature = "gil-refs")] m.add_function(wrap_pyfunction!(pyfunction_with_module_gil_ref, m)?)?; m.add_function(wrap_pyfunction!(pyfunction_with_module_owned, m)?)?; m.add_function(wrap_pyfunction!(pyfunction_with_module_and_py, m)?)?; m.add_function(wrap_pyfunction!(pyfunction_with_module_and_arg, m)?)?; m.add_function(wrap_pyfunction!(pyfunction_with_module_and_default_arg, m)?)?; m.add_function(wrap_pyfunction!(pyfunction_with_module_and_args_kwargs, m)?)?; + #[cfg(feature = "gil-refs")] m.add_function(wrap_pyfunction!( pyfunction_with_pass_module_in_attribute, m @@ -457,6 +461,7 @@ fn test_module_functions_with_module() { m, "m.pyfunction_with_module() == 'module_with_functions_with_module'" ); + #[cfg(feature = "gil-refs")] py_assert!( py, m, @@ -484,6 +489,7 @@ fn test_module_functions_with_module() { "m.pyfunction_with_module_and_args_kwargs(1, x=1, y=2) \ == ('module_with_functions_with_module', 1, 2)" ); + #[cfg(feature = "gil-refs")] py_assert!( py, m, diff --git a/tests/ui/deprecations.rs b/tests/ui/deprecations.rs index 06bcf819..c062cead 100644 --- a/tests/ui/deprecations.rs +++ b/tests/ui/deprecations.rs @@ -1,6 +1,7 @@ #![deny(deprecated)] use pyo3::prelude::*; +use pyo3::types::{PyString, PyType}; #[pyclass] struct MyClass; @@ -11,10 +12,32 @@ impl MyClass { fn new() -> Self { Self } + + #[classmethod] + fn cls_method_gil_ref(_cls: &PyType) {} + + #[classmethod] + fn cls_method_bound(_cls: &Bound<'_, PyType>) {} + + fn method_gil_ref(_slf: &PyCell) {} + + fn method_bound(_slf: &Bound<'_, Self>) {} } fn main() {} +#[pyfunction] +#[pyo3(pass_module)] +fn pyfunction_with_module<'py>(module: &Bound<'py, PyModule>) -> PyResult> { + module.name() +} + +#[pyfunction] +#[pyo3(pass_module)] +fn pyfunction_with_module_gil_ref(module: &PyModule) -> PyResult<&str> { + module.name() +} + #[pyfunction] fn double(x: usize) -> usize { x * 2 diff --git a/tests/ui/deprecations.stderr b/tests/ui/deprecations.stderr index 2440f909..edb74f97 100644 --- a/tests/ui/deprecations.stderr +++ b/tests/ui/deprecations.stderr @@ -1,7 +1,7 @@ error: use of deprecated constant `pyo3::impl_::deprecations::PYMETHODS_NEW_DEPRECATED_FORM`: use `#[new]` instead of `#[__new__]` - --> tests/ui/deprecations.rs:10:7 + --> tests/ui/deprecations.rs:11:7 | -10 | #[__new__] +11 | #[__new__] | ^^^^^^^ | note: the lint level is defined here @@ -10,14 +10,38 @@ 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 +error: use of deprecated struct `pyo3::PyCell`: `PyCell` was merged into `Bound`, use that instead; see the migration guide for more info + --> tests/ui/deprecations.rs:22:30 | -24 | fn module_gil_ref(m: &PyModule) -> PyResult<()> { +22 | fn method_gil_ref(_slf: &PyCell) {} + | ^^^^^^ + +error: use of deprecated method `pyo3::methods::Extractor::::extract_gil_ref`: use `&Bound<'_, T>` instead for this function argument + --> tests/ui/deprecations.rs:17:33 + | +17 | fn cls_method_gil_ref(_cls: &PyType) {} + | ^ + +error: use of deprecated method `pyo3::methods::Extractor::::extract_gil_ref`: use `&Bound<'_, T>` instead for this function argument + --> tests/ui/deprecations.rs:22:29 + | +22 | fn method_gil_ref(_slf: &PyCell) {} + | ^ + +error: use of deprecated method `pyo3::methods::Extractor::::extract_gil_ref`: use `&Bound<'_, T>` instead for this function argument + --> tests/ui/deprecations.rs:37:43 + | +37 | fn pyfunction_with_module_gil_ref(module: &PyModule) -> PyResult<&str> { + | ^ + +error: use of deprecated method `pyo3::methods::Extractor::::extract_gil_ref`: use `&Bound<'_, T>` instead for this function argument + --> tests/ui/deprecations.rs:47:19 + | +47 | 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 + --> tests/ui/deprecations.rs:53:57 | -30 | fn module_gil_ref_with_explicit_py_arg(_py: Python<'_>, m: &PyModule) -> PyResult<()> { +53 | fn module_gil_ref_with_explicit_py_arg(_py: Python<'_>, m: &PyModule) -> PyResult<()> { | ^