deprecate GIL refs in function argument

PyO3 · Feb 16, 2024 · 2daa644 · 2daa644
1 parent e308c8d
commit 2daa644
Show file tree

Hide file tree

Showing 7 changed files with 119 additions and 39 deletions.
diff --git a/pyo3-macros-backend/src/method.rs b/pyo3-macros-backend/src/method.rs
@@ -107,7 +107,7 @@ impl FnType {
         &self,
         cls: Option<&syn::Type>,
         error_mode: ExtractErrorMode,
-        holders: &mut Vec<TokenStream>,
+        holders: &mut Vec<(TokenStream, syn::Ident)>,
     ) -> TokenStream {
         match self {
             FnType::Getter(st) | FnType::Setter(st) | FnType::Fn(st) => {
@@ -171,7 +171,7 @@ impl SelfType {
         &self,
         cls: &syn::Type,
         error_mode: ExtractErrorMode,
-        holders: &mut Vec<TokenStream>,
+        holders: &mut Vec<(TokenStream, syn::Ident)>,
     ) -> TokenStream {
         // Due to use of quote_spanned in this function, need to bind these idents to the
         // main macro callsite.
@@ -185,10 +185,12 @@ impl SelfType {
                     syn::Ident::new("extract_pyclass_ref", *span)
                 };
                 let holder = syn::Ident::new(&format!("holder_{}", holders.len()), *span);
-                holders.push(quote_spanned! { *span =>
+                let extractor =
+                    syn::Ident::new(&format!("extractor_{}", holders.len()), Span::call_site());
+                holders.push((quote_spanned! { *span =>
                     #[allow(clippy::let_unit_value)]
                     let mut #holder = _pyo3::impl_::extract_argument::FunctionArgumentHolder::INIT;
-                });
+                }, extractor));
                 error_mode.handle_error(quote_spanned! { *span =>
                     _pyo3::impl_::extract_argument::#method::<#cls>(
                         #py.from_borrowed_ptr::<_pyo3::PyAny>(#slf),
@@ -486,7 +488,7 @@ impl<'a> FnSpec<'a> {
             }
         }
 
-        let rust_call = |args: Vec<TokenStream>, holders: &mut Vec<TokenStream>| {
+        let rust_call = |args: Vec<TokenStream>, holders: &mut Vec<(TokenStream, syn::Ident)>| {
             let self_arg = self.tp.self_arg(cls, ExtractErrorMode::Raise, holders);
 
             let call = if self.asyncness.is_some() {
@@ -539,7 +541,7 @@ impl<'a> FnSpec<'a> {
             } else {
                 quote! { function(#self_arg #(#args),*) }
             };
-            quotes::map_result_into_ptr(quotes::ok_wrap(call))
+            quotes::map_result_into_ptr(quotes::ok_wrap(call), &holders)
         };
 
         let func_name = &self.name;
@@ -567,7 +569,7 @@ impl<'a> FnSpec<'a> {
                     })
                     .collect();
                 let call = rust_call(args, &mut holders);
-
+                let holders = holders.iter().map(|(holder, _)| holder);
                 quote! {
                     unsafe fn #ident<'py>(
                         py: _pyo3::Python<'py>,
@@ -583,6 +585,7 @@ impl<'a> FnSpec<'a> {
                 let mut holders = Vec::new();
                 let (arg_convert, args) = impl_arg_params(self, cls, true, &mut holders)?;
                 let call = rust_call(args, &mut holders);
+                let holders = holders.iter().map(|(holder, _)| holder);
                 quote! {
                     unsafe fn #ident<'py>(
                         py: _pyo3::Python<'py>,
@@ -602,6 +605,7 @@ impl<'a> FnSpec<'a> {
                 let mut holders = Vec::new();
                 let (arg_convert, args) = impl_arg_params(self, cls, false, &mut holders)?;
                 let call = rust_call(args, &mut holders);
+                let holders = holders.iter().map(|(holder, _)| holder);
                 quote! {
                     unsafe fn #ident<'py>(
                         py: _pyo3::Python<'py>,
@@ -621,6 +625,7 @@ impl<'a> FnSpec<'a> {
                 let (arg_convert, args) = impl_arg_params(self, cls, false, &mut holders)?;
                 let self_arg = self.tp.self_arg(cls, ExtractErrorMode::Raise, &mut holders);
                 let call = quote! { #rust_name(#self_arg #(#args),*) };
+                let holders = holders.iter().map(|(holder, _)| holder);
                 quote! {
                     unsafe fn #ident(
                         py: _pyo3::Python<'_>,

diff --git a/pyo3-macros-backend/src/params.rs b/pyo3-macros-backend/src/params.rs
@@ -29,7 +29,7 @@ pub fn impl_arg_params(
     spec: &FnSpec<'_>,
     self_: Option<&syn::Type>,
     fastcall: bool,
-    holders: &mut Vec<TokenStream>,
+    holders: &mut Vec<(TokenStream, syn::Ident)>,
 ) -> Result<(TokenStream, Vec<TokenStream>)> {
     let args_array = syn::Ident::new("output", Span::call_site());
 
@@ -144,7 +144,7 @@ fn impl_arg_param(
     arg: &FnArg<'_>,
     option_pos: &mut usize,
     args_array: &syn::Ident,
-    holders: &mut Vec<TokenStream>,
+    holders: &mut Vec<(TokenStream, syn::Ident)>,
 ) -> Result<TokenStream> {
     // Use this macro inside this function, to ensure that all code generated here is associated
     // with the function argument
@@ -165,19 +165,27 @@ fn impl_arg_param(
 
     let mut push_holder = || {
         let holder = syn::Ident::new(&format!("holder_{}", holders.len()), arg.ty.span());
-        holders.push(quote_arg_span! {
-            #[allow(clippy::let_unit_value)]
-            let mut #holder = _pyo3::impl_::extract_argument::FunctionArgumentHolder::INIT;
-        });
-        holder
+        let extractor = syn::Ident::new(&format!("extractor_{}", holders.len()), Span::call_site());
+        holders.push((
+            quote_arg_span! {
+                #[allow(clippy::let_unit_value)]
+                let mut #holder = _pyo3::impl_::extract_argument::FunctionArgumentHolder::INIT;
+                let #extractor;
+            },
+            extractor.clone(),
+        ));
+        (holder, extractor)
     };
 
+    let mut extractor = None;
+
     if arg.is_varargs {
         ensure_spanned!(
             arg.optional.is_none(),
             arg.name.span() => "args cannot be optional"
         );
-        let holder = push_holder();
+        let (holder, e) = push_holder();
+        extractor = Some(e);
         return Ok(quote_arg_span! {
             _pyo3::impl_::extract_argument::extract_argument(
                 _args,
@@ -190,7 +198,8 @@ fn impl_arg_param(
             arg.optional.is_some(),
             arg.name.span() => "kwargs must be Option<_>"
         );
-        let holder = push_holder();
+        let (holder, e) = push_holder();
+        extractor = Some(e);
         return Ok(quote_arg_span! {
             _pyo3::impl_::extract_argument::extract_optional_argument(
                 _kwargs.map(::std::convert::AsRef::as_ref),
@@ -213,6 +222,8 @@ fn impl_arg_param(
     }
 
     let tokens = if let Some(expr_path) = arg.attrs.from_py_with.as_ref().map(|attr| &attr.value) {
+        // let (holder, e) = push_holder();
+        // extractor = Some(e);
         if let Some(default) = default {
             quote_arg_span! {
                 #[allow(clippy::redundant_closure)]
@@ -233,7 +244,8 @@ fn impl_arg_param(
             }
         }
     } else if arg.optional.is_some() {
-        let holder = push_holder();
+        let (holder, e) = push_holder();
+        extractor = Some(e);
         quote_arg_span! {
             #[allow(clippy::redundant_closure)]
             _pyo3::impl_::extract_argument::extract_optional_argument(
@@ -244,7 +256,8 @@ fn impl_arg_param(
             )?
         }
     } else if let Some(default) = default {
-        let holder = push_holder();
+        let (holder, e) = push_holder();
+        extractor = Some(e);
         quote_arg_span! {
             #[allow(clippy::redundant_closure)]
             _pyo3::impl_::extract_argument::extract_argument_with_default(
@@ -255,7 +268,8 @@ fn impl_arg_param(
             )?
         }
     } else {
-        let holder = push_holder();
+        let (holder, e) = push_holder();
+        extractor = Some(e);
         quote_arg_span! {
             _pyo3::impl_::extract_argument::extract_argument(
                 _pyo3::impl_::extract_argument::unwrap_required_argument(#arg_value),
@@ -264,5 +278,15 @@ fn impl_arg_param(
             )?
         }
     };
+
+    let tokens = quote! {
+        {
+            let extracted = #tokens;
+            let (extracted, e) = _pyo3::impl_::pymethods::inspect_type(extracted);
+            #extractor = e;
+            extracted
+        }
+    };
+
     Ok(tokens)
 }
diff --git a/pyo3-macros-backend/src/pymethod.rs b/pyo3-macros-backend/src/pymethod.rs
@@ -482,7 +482,7 @@ fn impl_call_setter(
     cls: &syn::Type,
     spec: &FnSpec<'_>,
     self_type: &SelfType,
-    holders: &mut Vec<TokenStream>,
+    holders: &mut Vec<(TokenStream, syn::Ident)>,
 ) -> syn::Result<TokenStream> {
     let (py_arg, args) = split_off_python_arg(&spec.signature.arguments);
     let slf = self_type.receiver(cls, ExtractErrorMode::Raise, holders);
@@ -565,6 +565,7 @@ pub fn impl_py_setter_def(
         }
     }
 
+    let holders = holders.iter().map(|(holder, _)| holder);
     let associated_method = quote! {
         #cfg_attrs
         unsafe fn #wrapper_ident(
@@ -604,7 +605,7 @@ fn impl_call_getter(
     cls: &syn::Type,
     spec: &FnSpec<'_>,
     self_type: &SelfType,
-    holders: &mut Vec<TokenStream>,
+    holders: &mut Vec<(TokenStream, syn::Ident)>,
 ) -> syn::Result<TokenStream> {
     let (py_arg, args) = split_off_python_arg(&spec.signature.arguments);
     let slf = self_type.receiver(cls, ExtractErrorMode::Raise, holders);
@@ -648,9 +649,12 @@ pub fn impl_py_getter_def(
                 // tuple struct field
                 syn::Index::from(field_index).to_token_stream()
             };
-            quotes::map_result_into_ptr(quotes::ok_wrap(quote! {
-                ::std::clone::Clone::clone(&(#slf.#field_token))
-            }))
+            quotes::map_result_into_ptr(
+                quotes::ok_wrap(quote! {
+                    ::std::clone::Clone::clone(&(#slf.#field_token))
+                }),
+                &holders,
+            )
         }
         // Forward to `IntoPyCallbackOutput`, to handle `#[getter]`s returning results.
         PropertyType::Function {
@@ -691,6 +695,7 @@ pub fn impl_py_getter_def(
         }
     }
 
+    let holders = holders.iter().map(|(holder, _)| holder);
     let associated_method = quote! {
         #cfg_attrs
         unsafe fn #wrapper_ident(
@@ -922,7 +927,7 @@ impl Ty {
         ident: &syn::Ident,
         arg: &FnArg<'_>,
         extract_error_mode: ExtractErrorMode,
-        holders: &mut Vec<TokenStream>,
+        holders: &mut Vec<(TokenStream, syn::Ident)>,
     ) -> TokenStream {
         let name_str = arg.name.unraw().to_string();
         match self {
@@ -986,15 +991,19 @@ impl Ty {
 
 fn extract_object(
     extract_error_mode: ExtractErrorMode,
-    holders: &mut Vec<TokenStream>,
+    holders: &mut Vec<(TokenStream, syn::Ident)>,
     name: &str,
     source: TokenStream,
 ) -> TokenStream {
     let holder = syn::Ident::new(&format!("holder_{}", holders.len()), Span::call_site());
-    holders.push(quote! {
-        #[allow(clippy::let_unit_value)]
-        let mut #holder = _pyo3::impl_::extract_argument::FunctionArgumentHolder::INIT;
-    });
+    let extractor = syn::Ident::new(&format!("extractor_{}", holders.len()), Span::call_site());
+    holders.push((
+        quote! {
+            #[allow(clippy::let_unit_value)]
+            let mut #holder = _pyo3::impl_::extract_argument::FunctionArgumentHolder::INIT;
+        },
+        extractor,
+    ));
     extract_error_mode.handle_error(quote! {
         _pyo3::impl_::extract_argument::extract_argument(
             #source,
@@ -1133,6 +1142,7 @@ impl SlotDef {
             return_mode.as_ref(),
         )?;
         let name = spec.name;
+        let holders = holders.iter().map(|(holder, _)| holder);
         let associated_method = quote! {
             unsafe fn #wrapper_ident(
                 py: _pyo3::Python<'_>,
@@ -1175,7 +1185,7 @@ fn generate_method_body(
     spec: &FnSpec<'_>,
     arguments: &[Ty],
     extract_error_mode: ExtractErrorMode,
-    holders: &mut Vec<TokenStream>,
+    holders: &mut Vec<(TokenStream, syn::Ident)>,
     return_mode: Option<&ReturnMode>,
 ) -> Result<TokenStream> {
     let self_arg = spec.tp.self_arg(Some(cls), extract_error_mode, holders);
@@ -1240,6 +1250,7 @@ impl SlotFragmentDef {
             None,
         )?;
         let ret_ty = ret_ty.ffi_type();
+        let holders = holders.iter().map(|(holder, _)| holder);
         Ok(quote! {
             impl _pyo3::impl_::pyclass::#fragment_trait<#cls> for _pyo3::impl_::pyclass::PyClassImplCollector<#cls> {
 
@@ -1346,7 +1357,7 @@ fn extract_proto_arguments(
     spec: &FnSpec<'_>,
     proto_args: &[Ty],
     extract_error_mode: ExtractErrorMode,
-    holders: &mut Vec<TokenStream>,
+    holders: &mut Vec<(TokenStream, syn::Ident)>,
 ) -> Result<Vec<TokenStream>> {
     let mut args = Vec::with_capacity(spec.signature.arguments.len());
     let mut non_python_args = 0;

diff --git a/pyo3-macros-backend/src/quotes.rs b/pyo3-macros-backend/src/quotes.rs
@@ -1,4 +1,4 @@
-use proc_macro2::TokenStream;
+use proc_macro2::{Span, TokenStream};
 use quote::quote;
 
 pub(crate) fn some_wrap(obj: TokenStream) -> TokenStream {
@@ -14,8 +14,14 @@ pub(crate) fn ok_wrap(obj: TokenStream) -> TokenStream {
     }
 }
 
-pub(crate) fn map_result_into_ptr(result: TokenStream) -> TokenStream {
+pub(crate) fn map_result_into_ptr(
+    result: TokenStream,
+    holders: &Vec<(TokenStream, syn::Ident)>,
+) -> TokenStream {
+    let extractors = holders.iter().map(|(_, extractor)| extractor);
     quote! {
-        _pyo3::impl_::wrap::map_result_into_ptr(py, #result)
+        let result = _pyo3::impl_::wrap::map_result_into_ptr(py, #result);
+        #(#extractors.extract_gil_ref();)*
+        result
     }
 }
diff --git a/src/impl_.rs b/src/impl_.rs
@@ -6,8 +6,8 @@
 //! APIs may may change at any time without documentation in the CHANGELOG and without
 //! breaking semver guarantees.
 
-#[cfg(feature = "macros")]
-pub mod coroutine;
+// #[cfg(feature = "macros")]
+// pub mod coroutine;
 pub mod deprecations;
 pub mod extract_argument;
 pub mod freelist;

diff --git a/src/impl_/pymethods.rs b/src/impl_/pymethods.rs
@@ -9,6 +9,7 @@ use crate::{
 use std::borrow::Cow;
 use std::ffi::CStr;
 use std::fmt;
+use std::marker::PhantomData;
 use std::os::raw::{c_int, c_void};
 use std::panic::{catch_unwind, AssertUnwindSafe};
 use std::ptr::null_mut;
@@ -466,3 +467,36 @@ pub trait AsyncIterResultOptionKind {
 }
 
 impl<Value, Error> AsyncIterResultOptionKind for Result<Option<Value>, Error> {}
+
+pub fn inspect_type<T>(t: T) -> (T, Extractor<T>) {
+    (t, Extractor::new())
+}
+
+pub struct Extractor<T>(NotAGilRef<T>);
+pub struct NotAGilRef<T>(PhantomData<T>);
+
+pub trait IsGilRef {}
+
+impl IsGilRef for &'_ PyAny {}
+
+impl<T> Extractor<T> {
+    pub fn new() -> Self {
+        Extractor(NotAGilRef(PhantomData))
+    }
+}
+
+impl<T: IsGilRef> Extractor<T> {
+    #[deprecated]
+    pub fn extract_gil_ref(&self) {}
+}
+
+impl<T> NotAGilRef<T> {
+    pub fn extract_gil_ref(&self) {}
+}
+
+impl<T> std::ops::Deref for Extractor<T> {
+    type Target = NotAGilRef<T>;
+    fn deref(&self) -> &Self::Target {
+        &self.0
+    }
+}