Skip to content

Commit

Permalink
refactor #[setter] argument extraction (#4002)
Browse files Browse the repository at this point in the history
  • Loading branch information
@Icxolu
Icxolu authored Apr 1, 2024
1 parent 63ba371 commit 8f87b86
Show file tree
Hide file tree
Showing 5 changed files with 97 additions and 70 deletions.
4 changes: 4 additions & 0 deletions pyo3-macros-backend/src/method.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,10 @@ impl<'a> FnArg<'a> {
}
}
}

pub fn is_regular(&self) -> bool {
!self.py && !self.is_cancel_handle && !self.is_kwargs && !self.is_varargs
}
}

fn handle_argument_error(pat: &syn::Pat) -> syn::Error {
Expand Down
39 changes: 22 additions & 17 deletions pyo3-macros-backend/src/params.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ pub fn is_forwarded_args(signature: &FunctionSignature<'_>) -> bool {
)
}

fn check_arg_for_gil_refs(
pub(crate) fn check_arg_for_gil_refs(
tokens: TokenStream,
gil_refs_checker: syn::Ident,
ctx: &Ctx,
Expand Down Expand Up @@ -120,7 +120,11 @@ pub fn impl_arg_params(
.iter()
.enumerate()
.map(|(i, arg)| {
impl_arg_param(arg, i, &mut 0, &args_array, holders, ctx).map(|tokens| {
let from_py_with =
syn::Ident::new(&format!("from_py_with_{}", i), Span::call_site());
let arg_value = quote!(#args_array[0].as_deref());

impl_arg_param(arg, from_py_with, arg_value, holders, ctx).map(|tokens| {
check_arg_for_gil_refs(
tokens,
holders.push_gil_refs_checker(arg.ty.span()),
Expand Down Expand Up @@ -161,14 +165,20 @@ pub fn impl_arg_params(

let num_params = positional_parameter_names.len() + keyword_only_parameters.len();

let mut option_pos = 0;
let mut option_pos = 0usize;
let param_conversion = spec
.signature
.arguments
.iter()
.enumerate()
.map(|(i, arg)| {
impl_arg_param(arg, i, &mut option_pos, &args_array, holders, ctx).map(|tokens| {
let from_py_with = syn::Ident::new(&format!("from_py_with_{}", i), Span::call_site());
let arg_value = quote!(#args_array[#option_pos].as_deref());
if arg.is_regular() {
option_pos += 1;
}

impl_arg_param(arg, from_py_with, arg_value, holders, ctx).map(|tokens| {
check_arg_for_gil_refs(tokens, holders.push_gil_refs_checker(arg.ty.span()), ctx)
})
})
Expand Down Expand Up @@ -234,11 +244,10 @@ pub fn impl_arg_params(

/// Re option_pos: The option slice doesn't contain the py: Python argument, so the argument
/// index and the index in option diverge when using py: Python
fn impl_arg_param(
pub(crate) fn impl_arg_param(
arg: &FnArg<'_>,
pos: usize,
option_pos: &mut usize,
args_array: &syn::Ident,
from_py_with: syn::Ident,
arg_value: TokenStream, // expected type: Option<&'a Bound<'py, PyAny>>
holders: &mut Holders,
ctx: &Ctx,
) -> Result<TokenStream> {
Expand Down Expand Up @@ -291,9 +300,6 @@ fn impl_arg_param(
});
}

let arg_value = quote_arg_span!(#args_array[#option_pos]);
*option_pos += 1;

let mut default = arg.default.as_ref().map(|expr| quote!(#expr));

// Option<T> arguments have special treatment: the default should be specified _without_ the
Expand All @@ -312,11 +318,10 @@ fn impl_arg_param(
.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! {
#pyo3_path::impl_::extract_argument::from_py_with_with_default(
#arg_value.as_deref(),
#arg_value,
#name_str,
#from_py_with as fn(_) -> _,
#[allow(clippy::redundant_closure)]
Expand All @@ -328,7 +333,7 @@ fn impl_arg_param(
} else {
quote_arg_span! {
#pyo3_path::impl_::extract_argument::from_py_with(
&#pyo3_path::impl_::extract_argument::unwrap_required_argument(#arg_value),
#pyo3_path::impl_::extract_argument::unwrap_required_argument(#arg_value),
#name_str,
#from_py_with as fn(_) -> _,
)?
Expand All @@ -338,7 +343,7 @@ fn impl_arg_param(
let holder = holders.push_holder(arg.name.span());
quote_arg_span! {
#pyo3_path::impl_::extract_argument::extract_optional_argument(
#arg_value.as_deref(),
#arg_value,
&mut #holder,
#name_str,
#[allow(clippy::redundant_closure)]
Expand All @@ -351,7 +356,7 @@ fn impl_arg_param(
let holder = holders.push_holder(arg.name.span());
quote_arg_span! {
#pyo3_path::impl_::extract_argument::extract_argument_with_default(
#arg_value.as_deref(),
#arg_value,
&mut #holder,
#name_str,
#[allow(clippy::redundant_closure)]
Expand All @@ -364,7 +369,7 @@ fn impl_arg_param(
let holder = holders.push_holder(arg.name.span());
quote_arg_span! {
#pyo3_path::impl_::extract_argument::extract_argument(
&#pyo3_path::impl_::extract_argument::unwrap_required_argument(#arg_value),
#pyo3_path::impl_::extract_argument::unwrap_required_argument(#arg_value),
&mut #holder,
#name_str
)?
Expand Down
97 changes: 56 additions & 41 deletions pyo3-macros-backend/src/pymethod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use std::borrow::Cow;

use crate::attributes::{NameAttribute, RenamingRule};
use crate::method::{CallingConvention, ExtractErrorMode};
use crate::params::Holders;
use crate::params::{check_arg_for_gil_refs, impl_arg_param, Holders};
use crate::utils::Ctx;
use crate::utils::PythonDoc;
use crate::{
Expand Down Expand Up @@ -586,48 +586,63 @@ pub fn impl_py_setter_def(
}
};

// TODO: rework this to make use of `impl_::params::impl_arg_param` which
// handles all these cases already.
let extract = if let PropertyType::Function { spec, .. } = &property_type {
Some(spec)
} else {
None
}
.and_then(|spec| {
let (_, args) = split_off_python_arg(&spec.signature.arguments);
let value_arg = &args[0];
let from_py_with = &value_arg.attrs.from_py_with.as_ref()?.value;
let name = value_arg.name.to_string();

Some(quote_spanned! { from_py_with.span() =>
let e = #pyo3_path::impl_::deprecations::GilRefs::new();
let from_py_with = #pyo3_path::impl_::deprecations::inspect_fn(#from_py_with, &e);
e.from_py_with_arg();
let _val = #pyo3_path::impl_::extract_argument::from_py_with(
&_value.into(),
#name,
from_py_with as fn(_) -> _,
)?;
})
})
.unwrap_or_else(|| {
let (span, name) = match &property_type {
PropertyType::Descriptor { field, .. } => (field.ty.span(), field.ident.as_ref().map(|i|i.to_string()).unwrap_or_default()),
PropertyType::Function { spec, .. } => {
let (_, args) = split_off_python_arg(&spec.signature.arguments);
(args[0].ty.span(), args[0].name.to_string())
}
};
let extract = match &property_type {
PropertyType::Function { spec, .. } => {
let (_, args) = split_off_python_arg(&spec.signature.arguments);
let value_arg = &args[0];
let (from_py_with, ident) = if let Some(from_py_with) =
&value_arg.attrs.from_py_with.as_ref().map(|f| &f.value)
{
let ident = syn::Ident::new("from_py_with", from_py_with.span());
(
quote_spanned! { from_py_with.span() =>
let e = #pyo3_path::impl_::deprecations::GilRefs::new();
let #ident = #pyo3_path::impl_::deprecations::inspect_fn(#from_py_with, &e);
e.from_py_with_arg();
},
ident,
)
} else {
(quote!(), syn::Ident::new("dummy", Span::call_site()))
};

let holder = holders.push_holder(span);
let gil_refs_checker = holders.push_gil_refs_checker(span);
quote! {
let _val = #pyo3_path::impl_::deprecations::inspect_type(
#pyo3_path::impl_::extract_argument::extract_argument(_value.into(), &mut #holder, #name)?,
&#gil_refs_checker
);
let extract = impl_arg_param(
&args[0],
ident,
quote!(::std::option::Option::Some(_value.into())),
&mut holders,
ctx,
)
.map(|tokens| {
check_arg_for_gil_refs(
tokens,
holders.push_gil_refs_checker(value_arg.ty.span()),
ctx,
)
})?;
quote! {
#from_py_with
let _val = #extract;
}
}
PropertyType::Descriptor { field, .. } => {
let span = field.ty.span();
let name = field
.ident
.as_ref()
.map(|i| i.to_string())
.unwrap_or_default();

let holder = holders.push_holder(span);
let gil_refs_checker = holders.push_gil_refs_checker(span);
quote! {
let _val = #pyo3_path::impl_::deprecations::inspect_type(
#pyo3_path::impl_::extract_argument::extract_argument(_value.into(), &mut #holder, #name)?,
&#gil_refs_checker
);
}
}
});
};

let mut cfg_attrs = TokenStream::new();
if let PropertyType::Descriptor { field, .. } = &property_type {
Expand Down
4 changes: 3 additions & 1 deletion src/impl_/extract_argument.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,9 @@ pub fn argument_extraction_error(py: Python<'_>, arg_name: &str, error: PyErr) -
/// `argument` must not be `None`
#[doc(hidden)]
#[inline]
pub unsafe fn unwrap_required_argument(argument: Option<PyArg<'_>>) -> PyArg<'_> {
pub unsafe fn unwrap_required_argument<'a, 'py>(
argument: Option<&'a Bound<'py, PyAny>>,
) -> &'a Bound<'py, PyAny> {
match argument {
Some(value) => value,
#[cfg(debug_assertions)]
Expand Down
23 changes: 12 additions & 11 deletions tests/ui/static_ref.stderr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,18 @@ error: lifetime may not live long enough
|
= note: this error originates in the attribute macro `pyfunction` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0597]: `output[_]` does not live long enough
--> tests/ui/static_ref.rs:4:1
|
4 | #[pyfunction]
| ^^^^^^^^^^^^-
| | |
| | `output[_]` dropped here while still borrowed
| borrowed value does not live long enough
| argument requires that `output[_]` is borrowed for `'static`
|
= note: this error originates in the attribute macro `pyfunction` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0597]: `holder_0` does not live long enough
--> tests/ui/static_ref.rs:5:15
|
Expand All @@ -21,17 +33,6 @@ error[E0597]: `holder_0` does not live long enough
5 | fn static_ref(list: &'static Bound<'_, PyList>) -> usize {
| ^^^^^^^ borrowed value does not live long enough

error[E0716]: temporary value dropped while borrowed
--> tests/ui/static_ref.rs:5:21
|
4 | #[pyfunction]
| -------------
| | |
| | temporary value is freed at the end of this statement
| argument requires that borrow lasts for `'static`
5 | fn static_ref(list: &'static Bound<'_, PyList>) -> usize {
| ^ creates a temporary value which is freed while still in use

error: lifetime may not live long enough
--> tests/ui/static_ref.rs:9:1
|
Expand Down

0 comments on commit 8f87b86

Please sign in to comment.