deprecate gil-refs in `from_py_with` (#3967)

* deprecate gil-refs in `from_py_with`

* review feedback davidhewitt

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::<TokenStream>();
+
     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::<Result<_>>()?;
         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::<Result<_>>()?;
 
     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<TokenStream>,
@@ -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(_) -> _,
                 )?
             }
         }

src/impl_/pymethods.rs

@@ -585,6 +585,10 @@ pub fn inspect_type<T>(t: T) -> (T, Extractor<T>) {
     (t, Extractor::new())
 }
 
+pub fn inspect_fn<A, T>(f: fn(A) -> PyResult<T>, _: &Extractor<A>) -> fn(A) -> PyResult<T> {
+    f
+}
+
 pub struct Extractor<T>(NotAGilRef<T>);
 pub struct NotAGilRef<T>(std::marker::PhantomData<T>);
 
@@ -616,10 +620,19 @@ impl<T: IsGilRef> Extractor<T> {
         )
     )]
     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<T> NotAGilRef<T> {
     pub fn extract_gil_ref(&self) {}
+    pub fn extract_from_py_with(&self) {}
     pub fn is_python(&self) {}
 }
 

tests/test_methods.rs

@@ -1146,7 +1146,7 @@ fn test_issue_2988() {
         _data: Vec<i32>,
         // 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<i32>,
+        #[pyo3(from_py_with = "<Bound<'_, _> as PyAnyMethods>::extract")] _data2: Vec<i32>,
     ) {
     }
 }

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<i64> {
+fn datetime_to_timestamp(dt: &Bound<'_, PyAny>) -> PyResult<i64> {
-    let dt: &PyDateTime = dt.extract()?;
+    let dt = dt.downcast::<PyDateTime>()?;
     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<Option<i32>> {
+    fn optional_int(x: &Bound<'_, PyAny>) -> PyResult<Option<i32>> {
         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 = "<Bound<'_, _> as PyAnyMethods>::len")] len: usize,
+    ) -> usize {
         len
     }
 

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<i32> {
+    obj.extract()
+}
+
+fn extract_bound(obj: &Bound<'_, PyAny>) -> PyResult<i32> {
+    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);

tests/ui/deprecations.stderr

@@ -46,10 +46,16 @@ error: use of deprecated method `pyo3::methods::Extractor::<T>::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::<pyo3::Python<'_>>::is_python`: use `wrap_pyfunction_bound!` instead
+error: use of deprecated method `pyo3::methods::Extractor::<T>::extract_from_py_with`: use `&Bound<'_, PyAny>` as the argument for this `from_py_with` extractor
-  --> tests/ui/deprecations.rs:79:13
+  --> 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::<pyo3::Python<'_>>::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)