From b06e95727b1bda03a5352f162bb90fa483cc1a8b Mon Sep 17 00:00:00 2001 From: Icxolu <10486322+Icxolu@users.noreply.github.com> Date: Tue, 19 Mar 2024 09:58:41 +0100 Subject: [PATCH] deprecate gil-refs in `from_py_with` (#3967) * deprecate gil-refs in `from_py_with` * review feedback davidhewitt --- pyo3-macros-backend/src/params.rs | 39 +++++++++++++++++++++++++++---- src/impl_/pymethods.rs | 13 +++++++++++ tests/test_methods.rs | 2 +- tests/test_pyfunction.rs | 10 ++++---- tests/ui/deprecations.rs | 15 ++++++++++++ tests/ui/deprecations.stderr | 12 +++++++--- 6 files changed, 78 insertions(+), 13 deletions(-) diff --git a/pyo3-macros-backend/src/params.rs b/pyo3-macros-backend/src/params.rs index 7260362f..74b3eba4 100644 --- a/pyo3-macros-backend/src/params.rs +++ b/pyo3-macros-backend/src/params.rs @@ -36,6 +36,23 @@ pub fn impl_arg_params( let args_array = syn::Ident::new("output", Span::call_site()); let Ctx { pyo3_path } = ctx; + let from_py_with = spec + .signature + .arguments + .iter() + .enumerate() + .filter_map(|(i, arg)| { + let from_py_with = &arg.attrs.from_py_with.as_ref()?.value; + let from_py_with_holder = + syn::Ident::new(&format!("from_py_with_{}", i), Span::call_site()); + Some(quote_spanned! { from_py_with.span() => + let e = #pyo3_path::impl_::pymethods::Extractor::new(); + let #from_py_with_holder = #pyo3_path::impl_::pymethods::inspect_fn(#from_py_with, &e); + e.extract_from_py_with(); + }) + }) + .collect::(); + if !fastcall && is_forwarded_args(&spec.signature) { // In the varargs convention, we can just pass though if the signature // is (*args, **kwds). @@ -43,12 +60,14 @@ pub fn impl_arg_params( .signature .arguments .iter() - .map(|arg| impl_arg_param(arg, &mut 0, &args_array, holders, ctx)) + .enumerate() + .map(|(i, arg)| impl_arg_param(arg, i, &mut 0, &args_array, holders, ctx)) .collect::>()?; return Ok(( quote! { let _args = #pyo3_path::impl_::pymethods::BoundRef::ref_from_ptr(py, &_args); let _kwargs = #pyo3_path::impl_::pymethods::BoundRef::ref_from_ptr_or_opt(py, &_kwargs); + #from_py_with }, arg_convert, )); @@ -81,7 +100,8 @@ pub fn impl_arg_params( .signature .arguments .iter() - .map(|arg| impl_arg_param(arg, &mut option_pos, &args_array, holders, ctx)) + .enumerate() + .map(|(i, arg)| impl_arg_param(arg, i, &mut option_pos, &args_array, holders, ctx)) .collect::>()?; let args_handler = if spec.signature.python_signature.varargs.is_some() { @@ -136,6 +156,7 @@ pub fn impl_arg_params( }; let mut #args_array = [::std::option::Option::None; #num_params]; let (_args, _kwargs) = #extract_expression; + #from_py_with }, param_conversion, )) @@ -145,6 +166,7 @@ pub fn impl_arg_params( /// index and the index in option diverge when using py: Python fn impl_arg_param( arg: &FnArg<'_>, + pos: usize, option_pos: &mut usize, args_array: &syn::Ident, holders: &mut Vec, @@ -222,14 +244,21 @@ fn impl_arg_param( )); } - let tokens = if let Some(expr_path) = arg.attrs.from_py_with.as_ref().map(|attr| &attr.value) { + let tokens = if arg + .attrs + .from_py_with + .as_ref() + .map(|attr| &attr.value) + .is_some() + { + let from_py_with = syn::Ident::new(&format!("from_py_with_{}", pos), Span::call_site()); if let Some(default) = default { quote_arg_span! { #[allow(clippy::redundant_closure)] #pyo3_path::impl_::extract_argument::from_py_with_with_default( #arg_value.as_deref(), #name_str, - #expr_path as fn(_) -> _, + #from_py_with as fn(_) -> _, || #default )? } @@ -238,7 +267,7 @@ fn impl_arg_param( #pyo3_path::impl_::extract_argument::from_py_with( &#pyo3_path::impl_::extract_argument::unwrap_required_argument(#arg_value), #name_str, - #expr_path as fn(_) -> _, + #from_py_with as fn(_) -> _, )? } } diff --git a/src/impl_/pymethods.rs b/src/impl_/pymethods.rs index bc1125f9..ca3f0a06 100644 --- a/src/impl_/pymethods.rs +++ b/src/impl_/pymethods.rs @@ -585,6 +585,10 @@ pub fn inspect_type(t: T) -> (T, Extractor) { (t, Extractor::new()) } +pub fn inspect_fn(f: fn(A) -> PyResult, _: &Extractor) -> fn(A) -> PyResult { + f +} + pub struct Extractor(NotAGilRef); pub struct NotAGilRef(std::marker::PhantomData); @@ -616,10 +620,19 @@ impl Extractor { ) )] pub fn extract_gil_ref(&self) {} + #[cfg_attr( + not(feature = "gil-refs"), + deprecated( + since = "0.21.0", + note = "use `&Bound<'_, PyAny>` as the argument for this `from_py_with` extractor" + ) + )] + pub fn extract_from_py_with(&self) {} } impl NotAGilRef { pub fn extract_gil_ref(&self) {} + pub fn extract_from_py_with(&self) {} pub fn is_python(&self) {} } diff --git a/tests/test_methods.rs b/tests/test_methods.rs index e7eacf26..7ca06b27 100644 --- a/tests/test_methods.rs +++ b/tests/test_methods.rs @@ -1146,7 +1146,7 @@ fn test_issue_2988() { _data: Vec, // The from_py_with here looks a little odd, we just need some way // to encourage the macro to expand the from_py_with default path too - #[pyo3(from_py_with = "PyAny::extract")] _data2: Vec, + #[pyo3(from_py_with = " as PyAnyMethods>::extract")] _data2: Vec, ) { } } diff --git a/tests/test_pyfunction.rs b/tests/test_pyfunction.rs index 5c435046..5221b0f8 100644 --- a/tests/test_pyfunction.rs +++ b/tests/test_pyfunction.rs @@ -118,8 +118,8 @@ fn test_functions_with_function_args() { } #[cfg(not(Py_LIMITED_API))] -fn datetime_to_timestamp(dt: &PyAny) -> PyResult { - let dt: &PyDateTime = dt.extract()?; +fn datetime_to_timestamp(dt: &Bound<'_, PyAny>) -> PyResult { + let dt = dt.downcast::()?; let ts: f64 = dt.call_method0("timestamp")?.extract()?; Ok(ts as i64) @@ -170,7 +170,7 @@ fn test_function_with_custom_conversion_error() { #[test] fn test_from_py_with_defaults() { - fn optional_int(x: &PyAny) -> PyResult> { + fn optional_int(x: &Bound<'_, PyAny>) -> PyResult> { if x.is_none() { Ok(None) } else { @@ -185,7 +185,9 @@ fn test_from_py_with_defaults() { } #[pyfunction(signature = (len=0))] - fn from_py_with_default(#[pyo3(from_py_with = "PyAny::len")] len: usize) -> usize { + fn from_py_with_default( + #[pyo3(from_py_with = " as PyAnyMethods>::len")] len: usize, + ) -> usize { len } diff --git a/tests/ui/deprecations.rs b/tests/ui/deprecations.rs index d82c406f..1dcc673a 100644 --- a/tests/ui/deprecations.rs +++ b/tests/ui/deprecations.rs @@ -74,6 +74,21 @@ fn module_bound_by_value(m: Bound<'_, PyModule>) -> PyResult<()> { Ok(()) } +fn extract_gil_ref(obj: &PyAny) -> PyResult { + obj.extract() +} + +fn extract_bound(obj: &Bound<'_, PyAny>) -> PyResult { + obj.extract() +} + +#[pyfunction] +fn pyfunction_from_py_with( + #[pyo3(from_py_with = "extract_gil_ref")] _gil_ref: i32, + #[pyo3(from_py_with = "extract_bound")] _bound: i32, +) { +} + fn test_wrap_pyfunction(py: Python<'_>, m: &Bound<'_, PyModule>) { // should lint let _ = wrap_pyfunction!(double, py); diff --git a/tests/ui/deprecations.stderr b/tests/ui/deprecations.stderr index e54e1a4e..10a21d3a 100644 --- a/tests/ui/deprecations.stderr +++ b/tests/ui/deprecations.stderr @@ -46,10 +46,16 @@ error: use of deprecated method `pyo3::methods::Extractor::::extract_gil_ref` 54 | fn module_gil_ref_with_explicit_py_arg(_py: Python<'_>, m: &PyModule) -> PyResult<()> { | ^ -error: use of deprecated method `pyo3::methods::Extractor::>::is_python`: use `wrap_pyfunction_bound!` instead - --> tests/ui/deprecations.rs:79:13 +error: use of deprecated method `pyo3::methods::Extractor::::extract_from_py_with`: use `&Bound<'_, PyAny>` as the argument for this `from_py_with` extractor + --> tests/ui/deprecations.rs:87:27 | -79 | let _ = wrap_pyfunction!(double, py); +87 | #[pyo3(from_py_with = "extract_gil_ref")] _gil_ref: i32, + | ^^^^^^^^^^^^^^^^^ + +error: use of deprecated method `pyo3::methods::Extractor::>::is_python`: use `wrap_pyfunction_bound!` instead + --> tests/ui/deprecations.rs:94:13 + | +94 | let _ = wrap_pyfunction!(double, py); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: this error originates in the macro `wrap_pyfunction` (in Nightly builds, run with -Z macro-backtrace for more info)