diff --git a/CHANGELOG.md b/CHANGELOG.md index ba768cb0..060b5b9c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -72,6 +72,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Fix incorrect enum names being returned by `repr` for enums renamed by `#[pyclass(name)]` [#2457](https://github.com/PyO3/pyo3/pull/2457) - Fix incorrect Python version cfg definition on `PyObject_CallNoArgs`[#2476](https://github.com/PyO3/pyo3/pull/2476) - Fix use-after-free in `PyCapsule` type. [#2481](https://github.com/PyO3/pyo3/pull/2481) +- Fix several clippy warnings generated by `#[pyfunction]` arguments. [#2503](https://github.com/PyO3/pyo3/pull/2503) ## [0.16.5] - 2022-05-15 diff --git a/guide/src/class.md b/guide/src/class.md index f614690e..efca7642 100644 --- a/guide/src/class.md +++ b/guide/src/class.md @@ -967,12 +967,24 @@ impl ::pyo3::PyClass for MyClass { type Frozen = pyo3::pyclass::boolean_struct::False; } -impl<'a> ::pyo3::derive_utils::ExtractExt<'a> for &'a mut MyClass { - type Target = ::pyo3::PyRefMut<'a, MyClass>; +impl<'a, 'py> ::pyo3::impl_::extract_argument::PyFunctionArgument<'a, 'py> for &'a MyClass +{ + type Holder = ::std::option::Option<::pyo3::PyRef<'py, MyClass>>; + + #[inline] + fn extract(obj: &'py ::pyo3::PyAny, holder: &'a mut Self::Holder) -> ::pyo3::PyResult { + ::pyo3::impl_::extract_argument::extract_pyclass_ref(obj, holder) + } } -impl<'a> ::pyo3::derive_utils::ExtractExt<'a> for &'a MyClass { - type Target = ::pyo3::PyRef<'a, MyClass>; +impl<'a, 'py> ::pyo3::impl_::extract_argument::PyFunctionArgument<'a, 'py> for &'a mut MyClass +{ + type Holder = ::std::option::Option<::pyo3::PyRefMut<'py, MyClass>>; + + #[inline] + fn extract(obj: &'py ::pyo3::PyAny, holder: &'a mut Self::Holder) -> ::pyo3::PyResult { + ::pyo3::impl_::extract_argument::extract_pyclass_ref_mut(obj, holder) + } } impl pyo3::IntoPy for MyClass { diff --git a/pyo3-build-config/src/lib.rs b/pyo3-build-config/src/lib.rs index 260ef0da..9795ac94 100644 --- a/pyo3-build-config/src/lib.rs +++ b/pyo3-build-config/src/lib.rs @@ -148,6 +148,11 @@ pub fn print_feature_cfgs() { if rustc_minor_version >= 51 { println!("cargo:rustc-cfg=addr_of"); } + + // Enable use of Option::insert on Rust 1.53 and greater + if rustc_minor_version >= 53 { + println!("cargo:rustc-cfg=option_insert"); + } } /// Private exports used in PyO3's build.rs diff --git a/pyo3-macros-backend/src/method.rs b/pyo3-macros-backend/src/method.rs index ee8e193b..eaf9a9dd 100644 --- a/pyo3-macros-backend/src/method.rs +++ b/pyo3-macros-backend/src/method.rs @@ -462,9 +462,6 @@ impl<'a> FnSpec<'a> { let deprecations = &self.deprecations; let self_conversion = self.tp.self_conversion(cls, ExtractErrorMode::Raise); let self_arg = self.tp.self_arg(); - let arg_names = (0..self.args.len()) - .map(|pos| syn::Ident::new(&format!("arg{}", pos), Span::call_site())) - .collect::>(); let py = syn::Ident::new("_py", Span::call_site()); let func_name = &self.name; let rust_name = if let Some(cls) = cls { @@ -474,19 +471,22 @@ impl<'a> FnSpec<'a> { }; // The method call is necessary to generate a decent error message. - let rust_call = quote! { - let mut ret = #rust_name(#self_arg #(#arg_names),*); + let rust_call = |args: Vec| { + quote! { + let mut ret = #rust_name(#self_arg #(#args),*); - if false { - use _pyo3::impl_::ghost::IntoPyResult; - ret.assert_into_py_result(); + if false { + use _pyo3::impl_::ghost::IntoPyResult; + ret.assert_into_py_result(); + } + + _pyo3::callback::convert(#py, ret) } - - _pyo3::callback::convert(#py, ret) }; Ok(match self.convention { CallingConvention::Noargs => { + let call = rust_call(vec![]); quote! { unsafe extern "C" fn #ident ( _slf: *mut _pyo3::ffi::PyObject, @@ -498,13 +498,14 @@ impl<'a> FnSpec<'a> { let #py = gil.python(); _pyo3::callback::panic_result_into_callback_output(#py, ::std::panic::catch_unwind(move || -> _pyo3::PyResult<_> { #self_conversion - #rust_call + #call })) } } } CallingConvention::Fastcall => { - let arg_convert = impl_arg_params(self, cls, &py, true)?; + let (arg_convert, args) = impl_arg_params(self, cls, &py, true)?; + let call = rust_call(args); quote! { unsafe extern "C" fn #ident ( _slf: *mut _pyo3::ffi::PyObject, @@ -518,13 +519,14 @@ impl<'a> FnSpec<'a> { _pyo3::callback::panic_result_into_callback_output(#py, ::std::panic::catch_unwind(move || -> _pyo3::PyResult<_> { #self_conversion #arg_convert - #rust_call + #call })) } } } CallingConvention::Varargs => { - let arg_convert = impl_arg_params(self, cls, &py, false)?; + let (arg_convert, args) = impl_arg_params(self, cls, &py, false)?; + let call = rust_call(args); quote! { unsafe extern "C" fn #ident ( _slf: *mut _pyo3::ffi::PyObject, @@ -537,14 +539,14 @@ impl<'a> FnSpec<'a> { _pyo3::callback::panic_result_into_callback_output(#py, ::std::panic::catch_unwind(move || -> _pyo3::PyResult<_> { #self_conversion #arg_convert - #rust_call + #call })) } } } CallingConvention::TpNew => { - let rust_call = quote! { #rust_name(#(#arg_names),*) }; - let arg_convert = impl_arg_params(self, cls, &py, false)?; + let (arg_convert, args) = impl_arg_params(self, cls, &py, false)?; + let call = quote! { #rust_name(#(#args),*) }; quote! { unsafe extern "C" fn #ident ( subtype: *mut _pyo3::ffi::PyTypeObject, @@ -557,7 +559,7 @@ impl<'a> FnSpec<'a> { let #py = gil.python(); _pyo3::callback::panic_result_into_callback_output(#py, ::std::panic::catch_unwind(move || -> _pyo3::PyResult<_> { #arg_convert - let result = #rust_call; + let result = #call; let initializer: _pyo3::PyClassInitializer::<#cls> = result.convert(#py)?; initializer.into_new_object(#py, subtype) })) diff --git a/pyo3-macros-backend/src/params.rs b/pyo3-macros-backend/src/params.rs index 04a3981b..c6d7c0a8 100644 --- a/pyo3-macros-backend/src/params.rs +++ b/pyo3-macros-backend/src/params.rs @@ -4,7 +4,6 @@ use crate::{ attributes::FromPyWithAttribute, method::{FnArg, FnSpec}, pyfunction::Argument, - utils::{remove_lifetime, unwrap_ty_group}, }; use proc_macro2::{Span, TokenStream}; use quote::{quote, quote_spanned}; @@ -55,9 +54,9 @@ pub fn impl_arg_params( self_: Option<&syn::Type>, py: &syn::Ident, fastcall: bool, -) -> Result { +) -> Result<(TokenStream, Vec)> { if spec.args.is_empty() { - return Ok(TokenStream::new()); + return Ok((TokenStream::new(), vec![])); } let args_array = syn::Ident::new("output", Span::call_site()); @@ -65,15 +64,18 @@ pub fn impl_arg_params( if !fastcall && is_forwarded_args(&spec.args, &spec.attrs) { // In the varargs convention, we can just pass though if the signature // is (*args, **kwds). - let mut arg_convert = vec![]; - for (i, arg) in spec.args.iter().enumerate() { - arg_convert.push(impl_arg_param(arg, spec, i, &mut 0, py, &args_array)?); - } - return Ok(quote! { - let _args = #py.from_borrowed_ptr::<_pyo3::types::PyTuple>(_args); - let _kwargs: ::std::option::Option<&_pyo3::types::PyDict> = #py.from_borrowed_ptr_or_opt(_kwargs); - #(#arg_convert)* - }); + let arg_convert = spec + .args + .iter() + .map(|arg| impl_arg_param(arg, spec, &mut 0, py, &args_array)) + .collect::>()?; + return Ok(( + quote! { + let _args = #py.from_borrowed_ptr::<_pyo3::types::PyTuple>(_args); + let _kwargs: ::std::option::Option<&_pyo3::types::PyDict> = #py.from_borrowed_ptr_or_opt(_kwargs); + }, + arg_convert, + )); }; let mut positional_parameter_names = Vec::new(); @@ -111,18 +113,12 @@ pub fn impl_arg_params( let num_params = positional_parameter_names.len() + keyword_only_parameters.len(); - let mut param_conversion = Vec::new(); let mut option_pos = 0; - for (idx, arg) in spec.args.iter().enumerate() { - param_conversion.push(impl_arg_param( - arg, - spec, - idx, - &mut option_pos, - py, - &args_array, - )?); - } + let param_conversion = spec + .args + .iter() + .map(|arg| impl_arg_param(arg, spec, &mut option_pos, py, &args_array)) + .collect::>()?; let (accept_args, accept_kwargs) = accept_args_kwargs(&spec.attrs); let args_handler = if accept_args { @@ -165,21 +161,22 @@ pub fn impl_arg_params( }; // create array of arguments, and then parse - Ok(quote! { - const DESCRIPTION: _pyo3::impl_::extract_argument::FunctionDescription = _pyo3::impl_::extract_argument::FunctionDescription { - cls_name: #cls_name, - func_name: stringify!(#python_name), - positional_parameter_names: &[#(#positional_parameter_names),*], - positional_only_parameters: #positional_only_parameters, - required_positional_parameters: #required_positional_parameters, - keyword_only_parameters: &[#(#keyword_only_parameters),*], - }; + Ok(( + quote! { + const DESCRIPTION: _pyo3::impl_::extract_argument::FunctionDescription = _pyo3::impl_::extract_argument::FunctionDescription { + cls_name: #cls_name, + func_name: stringify!(#python_name), + positional_parameter_names: &[#(#positional_parameter_names),*], + positional_only_parameters: #positional_only_parameters, + required_positional_parameters: #required_positional_parameters, + keyword_only_parameters: &[#(#keyword_only_parameters),*], + }; - let mut #args_array = [::std::option::Option::None; #num_params]; - let (_args, _kwargs) = #extract_expression; - - #(#param_conversion)* - }) + let mut #args_array = [::std::option::Option::None; #num_params]; + let (_args, _kwargs) = #extract_expression; + }, + param_conversion, + )) } /// Re option_pos: The option slice doesn't contain the py: Python argument, so the argument @@ -187,7 +184,6 @@ pub fn impl_arg_params( fn impl_arg_param( arg: &FnArg<'_>, spec: &FnSpec<'_>, - idx: usize, option_pos: &mut usize, py: &syn::Ident, args_array: &syn::Ident, @@ -198,13 +194,10 @@ fn impl_arg_param( ($($tokens:tt)*) => { quote_spanned!(arg.ty.span() => $($tokens)*) } } - let arg_name = syn::Ident::new(&format!("arg{}", idx), Span::call_site()); - if arg.py { - return Ok(quote_arg_span! { let #arg_name = #py; }); + return Ok(quote_arg_span! { #py }); } - let ty = arg.ty; let name = arg.name; let name_str = name.to_string(); @@ -214,7 +207,11 @@ fn impl_arg_param( arg.name.span() => "args cannot be optional" ); return Ok(quote_arg_span! { - let #arg_name = _pyo3::impl_::extract_argument::extract_argument(_args, #name_str)?; + _pyo3::impl_::extract_argument::extract_argument( + _args, + &mut { _pyo3::impl_::extract_argument::FunctionArgumentHolder::INIT }, + #name_str + )? }); } else if is_kwargs(&spec.attrs, name) { ensure_spanned!( @@ -222,31 +219,35 @@ fn impl_arg_param( arg.name.span() => "kwargs must be Option<_>" ); return Ok(quote_arg_span! { - let #arg_name = _pyo3::impl_::extract_argument::extract_optional_argument(_kwargs.map(|kwargs| kwargs.as_ref()), #name_str)?; + _pyo3::impl_::extract_argument::extract_optional_argument( + _kwargs.map(|kwargs| kwargs.as_ref()), + &mut { _pyo3::impl_::extract_argument::FunctionArgumentHolder::INIT }, + #name_str + )? }); } let arg_value = quote_arg_span!(#args_array[#option_pos]); *option_pos += 1; - let arg_value_or_default = if let Some(FromPyWithAttribute { + let tokens = if let Some(FromPyWithAttribute { value: expr_path, .. }) = &arg.attrs.from_py_with { match (spec.default_value(name), arg.optional.is_some()) { (Some(default), true) if default.to_string() != "None" => { quote_arg_span! { - _pyo3::impl_::extract_argument::from_py_with_with_default(#arg_value, #name_str, #expr_path, || Some(#default)) + _pyo3::impl_::extract_argument::from_py_with_with_default(#arg_value, #name_str, #expr_path, || Some(#default))? } } (Some(default), _) => { quote_arg_span! { - _pyo3::impl_::extract_argument::from_py_with_with_default(#arg_value, #name_str, #expr_path, || #default) + _pyo3::impl_::extract_argument::from_py_with_with_default(#arg_value, #name_str, #expr_path, || #default)? } } (None, true) => { quote_arg_span! { - _pyo3::impl_::extract_argument::from_py_with_with_default(#arg_value, #name_str, #expr_path, || Some(None)) + _pyo3::impl_::extract_argument::from_py_with_with_default(#arg_value, #name_str, #expr_path, || Some(None))? } } (None, false) => { @@ -255,7 +256,7 @@ fn impl_arg_param( _pyo3::impl_::extract_argument::unwrap_required_argument(#arg_value), #name_str, #expr_path, - ) + )? } } } @@ -263,59 +264,43 @@ fn impl_arg_param( match (spec.default_value(name), arg.optional.is_some()) { (Some(default), true) if default.to_string() != "None" => { quote_arg_span! { - _pyo3::impl_::extract_argument::extract_argument_with_default(#arg_value, #name_str, || Some(#default)) + _pyo3::impl_::extract_argument::extract_argument_with_default( + #arg_value, + &mut { _pyo3::impl_::extract_argument::FunctionArgumentHolder::INIT }, + #name_str, + || Some(#default) + )? } } (Some(default), _) => { quote_arg_span! { - _pyo3::impl_::extract_argument::extract_argument_with_default(#arg_value, #name_str, || #default) + _pyo3::impl_::extract_argument::extract_argument_with_default( + #arg_value, + &mut { _pyo3::impl_::extract_argument::FunctionArgumentHolder::INIT }, + #name_str, + || #default + )? } } (None, true) => { quote_arg_span! { - _pyo3::impl_::extract_argument::extract_optional_argument(#arg_value, #name_str) + _pyo3::impl_::extract_argument::extract_optional_argument( + #arg_value, + &mut { _pyo3::impl_::extract_argument::FunctionArgumentHolder::INIT }, + #name_str + )? } } (None, false) => { quote_arg_span! { _pyo3::impl_::extract_argument::extract_argument( _pyo3::impl_::extract_argument::unwrap_required_argument(#arg_value), + &mut { _pyo3::impl_::extract_argument::FunctionArgumentHolder::INIT }, #name_str - ) + )? } } } }; - - return if let syn::Type::Reference(tref) = unwrap_ty_group(arg.optional.unwrap_or(ty)) { - let tref = remove_lifetime(tref); - let mut_ = tref.mutability; - let (target_ty, borrow_tmp) = if arg.optional.is_some() { - // Get Option<&T> from Option> - ( - quote_arg_span! { ::std::option::Option<<#tref as _pyo3::derive_utils::ExtractExt<'_>>::Target> }, - if mut_.is_some() { - quote_arg_span! { _tmp.as_deref_mut() } - } else { - quote_arg_span! { _tmp.as_deref() } - }, - ) - } else { - // Get &T from PyRef - ( - quote_arg_span! { <#tref as _pyo3::derive_utils::ExtractExt<'_>>::Target }, - quote_arg_span! { &#mut_ *_tmp }, - ) - }; - - Ok(quote_arg_span! { - let #mut_ _tmp: #target_ty = #arg_value_or_default?; - #[allow(clippy::needless_option_as_deref)] - let #arg_name = #borrow_tmp; - }) - } else { - Ok(quote_arg_span! { - let #arg_name = #arg_value_or_default?; - }) - }; + Ok(tokens) } diff --git a/pyo3-macros-backend/src/pyclass.rs b/pyo3-macros-backend/src/pyclass.rs index 8041e469..816d329c 100644 --- a/pyo3-macros-backend/src/pyclass.rs +++ b/pyo3-macros-backend/src/pyclass.rs @@ -777,21 +777,36 @@ impl<'a> PyClassImplsBuilder<'a> { let cls = self.cls; if self.attr.options.frozen.is_some() { quote! { - impl<'a> _pyo3::derive_utils::ExtractExt<'a> for &'a #cls + impl<'a, 'py> _pyo3::impl_::extract_argument::PyFunctionArgument<'a, 'py> for &'a #cls { - type Target = _pyo3::PyRef<'a, #cls>; + type Holder = ::std::option::Option<_pyo3::PyRef<'py, #cls>>; + + #[inline] + fn extract(obj: &'py _pyo3::PyAny, holder: &'a mut Self::Holder) -> _pyo3::PyResult { + _pyo3::impl_::extract_argument::extract_pyclass_ref(obj, holder) + } } } } else { quote! { - impl<'a> _pyo3::derive_utils::ExtractExt<'a> for &'a #cls + impl<'a, 'py> _pyo3::impl_::extract_argument::PyFunctionArgument<'a, 'py> for &'a #cls { - type Target = _pyo3::PyRef<'a, #cls>; + type Holder = ::std::option::Option<_pyo3::PyRef<'py, #cls>>; + + #[inline] + fn extract(obj: &'py _pyo3::PyAny, holder: &'a mut Self::Holder) -> _pyo3::PyResult { + _pyo3::impl_::extract_argument::extract_pyclass_ref(obj, holder) + } } - impl<'a> _pyo3::derive_utils::ExtractExt<'a> for &'a mut #cls + impl<'a, 'py> _pyo3::impl_::extract_argument::PyFunctionArgument<'a, 'py> for &'a mut #cls { - type Target = _pyo3::PyRefMut<'a, #cls>; + type Holder = ::std::option::Option<_pyo3::PyRefMut<'py, #cls>>; + + #[inline] + fn extract(obj: &'py _pyo3::PyAny, holder: &'a mut Self::Holder) -> _pyo3::PyResult { + _pyo3::impl_::extract_argument::extract_pyclass_ref_mut(obj, holder) + } } } } diff --git a/pyo3-macros-backend/src/pymethod.rs b/pyo3-macros-backend/src/pymethod.rs index dc4af8e0..bccac283 100644 --- a/pyo3-macros-backend/src/pymethod.rs +++ b/pyo3-macros-backend/src/pymethod.rs @@ -4,7 +4,7 @@ use std::borrow::Cow; use crate::attributes::NameAttribute; use crate::method::{CallingConvention, ExtractErrorMode}; -use crate::utils::{ensure_not_async_fn, remove_lifetime, unwrap_ty_group, PythonDoc}; +use crate::utils::{ensure_not_async_fn, PythonDoc}; use crate::{deprecations::Deprecations, utils}; use crate::{ method::{FnArg, FnSpec, FnType, SelfType}, @@ -12,7 +12,6 @@ use crate::{ }; use proc_macro2::{Span, TokenStream}; use quote::{format_ident, quote, ToTokens}; -use syn::Ident; use syn::{ext::IdentExt, spanned::Spanned, Result}; /// Generated code for a single pymethod item. @@ -839,81 +838,66 @@ impl Ty { arg: &FnArg<'_>, extract_error_mode: ExtractErrorMode, ) -> TokenStream { + let name_str = arg.name.unraw().to_string(); match self { - Ty::Object => { - let extract = handle_error( - extract_error_mode, - py, - quote! { - #py.from_borrowed_ptr::<_pyo3::PyAny>(#ident).extract() - }, - ); - extract_object(arg.ty, ident, extract) - } - Ty::MaybeNullObject => { - let extract = handle_error( - extract_error_mode, - py, - quote! { - #py.from_borrowed_ptr::<_pyo3::PyAny>( - if #ident.is_null() { - _pyo3::ffi::Py_None() - } else { - #ident - } - ).extract() - }, - ); - extract_object(arg.ty, ident, extract) - } - Ty::NonNullObject => { - let extract = handle_error( - extract_error_mode, - py, - quote! { - #py.from_borrowed_ptr::<_pyo3::PyAny>(#ident.as_ptr()).extract() - }, - ); - extract_object(arg.ty, ident, extract) - } - Ty::IPowModulo => { - let extract = handle_error( - extract_error_mode, - py, - quote! { - #ident.extract(#py) - }, - ); - extract_object(arg.ty, ident, extract) - } - Ty::CompareOp => { - let extract = handle_error( - extract_error_mode, - py, - quote! { - _pyo3::class::basic::CompareOp::from_raw(#ident) - .ok_or_else(|| _pyo3::exceptions::PyValueError::new_err("invalid comparison operator")) - }, - ); + Ty::Object => extract_object( + extract_error_mode, + py, + &name_str, quote! { - let #ident = #extract; - } - } + #py.from_borrowed_ptr::<_pyo3::PyAny>(#ident) + }, + ), + Ty::MaybeNullObject => extract_object( + extract_error_mode, + py, + &name_str, + quote! { + #py.from_borrowed_ptr::<_pyo3::PyAny>( + if #ident.is_null() { + _pyo3::ffi::Py_None() + } else { + #ident + } + ) + }, + ), + Ty::NonNullObject => extract_object( + extract_error_mode, + py, + &name_str, + quote! { + #py.from_borrowed_ptr::<_pyo3::PyAny>(#ident.as_ptr()) + }, + ), + Ty::IPowModulo => extract_object( + extract_error_mode, + py, + &name_str, + quote! { + #ident.to_borrowed_any(#py) + }, + ), + Ty::CompareOp => handle_error( + extract_error_mode, + py, + quote! { + _pyo3::class::basic::CompareOp::from_raw(#ident) + .ok_or_else(|| _pyo3::exceptions::PyValueError::new_err("invalid comparison operator")) + }, + ), Ty::PySsizeT => { let ty = arg.ty; - let extract = handle_error( + handle_error( extract_error_mode, py, quote! { ::std::convert::TryInto::<#ty>::try_into(#ident).map_err(|e| _pyo3::exceptions::PyValueError::new_err(e.to_string())) }, - ); - quote! { - let #ident = #extract; - } + ) } // Just pass other types through unmodified - Ty::PyBuffer | Ty::Int | Ty::PyHashT | Ty::Void => quote! {}, + Ty::PyBuffer | Ty::Int | Ty::PyHashT | Ty::Void => quote! { #ident }, } } } @@ -934,19 +918,23 @@ fn handle_error( } } -fn extract_object(target: &syn::Type, ident: &syn::Ident, extract: TokenStream) -> TokenStream { - if let syn::Type::Reference(tref) = unwrap_ty_group(target) { - let tref = remove_lifetime(tref); - let mut_ = tref.mutability; +fn extract_object( + extract_error_mode: ExtractErrorMode, + py: &syn::Ident, + name: &str, + source: TokenStream, +) -> TokenStream { + handle_error( + extract_error_mode, + py, quote! { - let #mut_ #ident: <#tref as _pyo3::derive_utils::ExtractExt<'_>>::Target = #extract; - let #ident = &#mut_ *#ident; - } - } else { - quote! { - let #ident = #extract; - } - } + _pyo3::impl_::extract_argument::extract_argument( + #source, + &mut { _pyo3::impl_::extract_argument::FunctionArgumentHolder::INIT }, + #name + ) + }, + ) } enum ReturnMode { @@ -1096,12 +1084,8 @@ fn generate_method_body( ) -> Result { let self_conversion = spec.tp.self_conversion(Some(cls), extract_error_mode); let rust_name = spec.name; - let (arg_idents, arg_count, conversions) = - extract_proto_arguments(py, &spec.args, arguments, extract_error_mode)?; - if arg_count != arguments.len() { - bail_spanned!(spec.name.span() => format!("Expected {} arguments, got {}", arguments.len(), arg_count)); - } - let call = quote! { _pyo3::callback::convert(#py, #cls::#rust_name(_slf, #(#arg_idents),*)) }; + let args = extract_proto_arguments(py, spec, arguments, extract_error_mode)?; + let call = quote! { _pyo3::callback::convert(#py, #cls::#rust_name(_slf, #(#args),*)) }; let body = if let Some(return_mode) = return_mode { return_mode.return_call_output(py, call) } else { @@ -1109,7 +1093,6 @@ fn generate_method_body( }; Ok(quote! { #self_conversion - #conversions #body }) } @@ -1243,30 +1226,30 @@ const __RPOW__: SlotFragmentDef = SlotFragmentDef::new("__rpow__", &[Ty::Object, fn extract_proto_arguments( py: &syn::Ident, - method_args: &[FnArg<'_>], + spec: &FnSpec<'_>, proto_args: &[Ty], extract_error_mode: ExtractErrorMode, -) -> Result<(Vec, usize, TokenStream)> { - let mut arg_idents = Vec::with_capacity(method_args.len()); +) -> Result> { + let mut args = Vec::with_capacity(spec.args.len()); let mut non_python_args = 0; - let mut args_conversions = Vec::with_capacity(proto_args.len()); - - for arg in method_args { + for arg in &spec.args { if arg.py { - arg_idents.push(py.clone()); + args.push(quote! { #py }); } else { let ident = syn::Ident::new(&format!("arg{}", non_python_args), Span::call_site()); let conversions = proto_args.get(non_python_args) .ok_or_else(|| err_spanned!(arg.ty.span() => format!("Expected at most {} non-python arguments", proto_args.len())))? .extract(py, &ident, arg, extract_error_mode); non_python_args += 1; - args_conversions.push(conversions); - arg_idents.push(ident); + args.push(conversions); } } - let conversions = quote!(#(#args_conversions)*); - Ok((arg_idents, non_python_args, conversions)) + + if non_python_args != proto_args.len() { + bail_spanned!(spec.name.span() => format!("Expected {} arguments, got {}", proto_args.len(), non_python_args)); + } + Ok(args) } struct StaticIdent(&'static str); diff --git a/pyo3-macros-backend/src/utils.rs b/pyo3-macros-backend/src/utils.rs index e50a9d4d..1542e0b9 100644 --- a/pyo3-macros-backend/src/utils.rs +++ b/pyo3-macros-backend/src/utils.rs @@ -164,13 +164,6 @@ pub fn unwrap_ty_group(mut ty: &syn::Type) -> &syn::Type { ty } -/// Remove lifetime from reference -pub(crate) fn remove_lifetime(tref: &syn::TypeReference) -> syn::TypeReference { - let mut tref = tref.to_owned(); - tref.lifetime = None; - tref -} - /// Extract the path to the pyo3 crate, or use the default (`::pyo3`). pub(crate) fn get_pyo3_crate(attr: &Option) -> syn::Path { attr.as_ref() diff --git a/src/class/number.rs b/src/class/number.rs index 63451b62..a9db5f82 100644 --- a/src/class/number.rs +++ b/src/class/number.rs @@ -733,7 +733,7 @@ where .try_borrow_mut()? .__ipow__( extract_or_return_not_implemented!(other), - match modulo.extract(py) { + match modulo.to_borrowed_any(py).extract() { Ok(value) => value, Err(_) => { let res = crate::ffi::Py_NotImplemented(); diff --git a/src/derive_utils.rs b/src/derive_utils.rs index 2b2666bb..242cdf68 100644 --- a/src/derive_utils.rs +++ b/src/derive_utils.rs @@ -6,19 +6,6 @@ use crate::{types::PyModule, PyCell, PyClass, PyErr, Python}; -/// Utility trait to enable &PyClass as a pymethod/function argument -#[doc(hidden)] -pub trait ExtractExt<'a> { - type Target: crate::FromPyObject<'a>; -} - -impl<'a, T> ExtractExt<'a> for T -where - T: crate::FromPyObject<'a>, -{ - type Target = T; -} - /// A trait for types that can be borrowed from a cell. /// /// This serves to unify the use of `PyRef` and `PyRefMut` in automatically diff --git a/src/impl_/extract_argument.rs b/src/impl_/extract_argument.rs index fc2c26ee..f74aa280 100644 --- a/src/impl_/extract_argument.rs +++ b/src/impl_/extract_argument.rs @@ -1,17 +1,91 @@ use crate::{ exceptions::PyTypeError, ffi, + pyclass::boolean_struct::False, types::{PyDict, PyString, PyTuple}, - FromPyObject, PyAny, PyErr, PyResult, PyTypeInfo, Python, + FromPyObject, PyAny, PyClass, PyErr, PyRef, PyRefMut, PyResult, PyTypeInfo, Python, }; +/// A trait which is used to help PyO3 macros extract function arguments. +/// +/// `#[pyclass]` structs need to extract as `PyRef` and `PyRefMut` +/// wrappers rather than extracting `&T` and `&mut T` directly. The `Holder` type is used +/// to hold these temporary wrappers - the way the macro is constructed, these wrappers +/// will be dropped as soon as the pyfunction call ends. +/// +/// There exists a trivial blanket implementation for `T: FromPyObject` with `Holder = ()`. +pub trait PyFunctionArgument<'a, 'py>: Sized + 'a { + type Holder: FunctionArgumentHolder; + fn extract(obj: &'py PyAny, holder: &'a mut Self::Holder) -> PyResult; +} + +impl<'a, 'py, T> PyFunctionArgument<'a, 'py> for T +where + T: FromPyObject<'py> + 'a, +{ + type Holder = (); + + #[inline] + fn extract(obj: &'py PyAny, _: &'a mut ()) -> PyResult { + obj.extract() + } +} + +/// Trait for types which can be a function argument holder - they should +/// to be able to const-initialize to an empty value. +pub trait FunctionArgumentHolder: Sized { + const INIT: Self; +} + +impl FunctionArgumentHolder for () { + const INIT: Self = (); +} + +impl FunctionArgumentHolder for Option { + const INIT: Self = None; +} + +#[inline] +pub fn extract_pyclass_ref<'a, 'py: 'a, T: PyClass>( + obj: &'py PyAny, + holder: &'a mut Option>, +) -> PyResult<&'a T> { + #[cfg(not(option_insert))] + { + *holder = Some(obj.extract()?); + return Ok(holder.as_deref().unwrap()); + } + + #[cfg(option_insert)] + Ok(&*holder.insert(obj.extract()?)) +} + +#[inline] +pub fn extract_pyclass_ref_mut<'a, 'py: 'a, T: PyClass>( + obj: &'py PyAny, + holder: &'a mut Option>, +) -> PyResult<&'a mut T> { + #[cfg(not(option_insert))] + { + *holder = Some(obj.extract()?); + return Ok(holder.as_deref_mut().unwrap()); + } + + #[cfg(option_insert)] + Ok(&mut *holder.insert(obj.extract()?)) +} + /// The standard implementation of how PyO3 extracts a `#[pyfunction]` or `#[pymethod]` function argument. #[doc(hidden)] -pub fn extract_argument<'py, T>(obj: &'py PyAny, arg_name: &str) -> PyResult +pub fn extract_argument<'a, 'py, T>( + obj: &'py PyAny, + holder: &'a mut T::Holder, + arg_name: &str, +) -> PyResult where - T: FromPyObject<'py>, + T: PyFunctionArgument<'a, 'py>, { - match obj.extract() { + match PyFunctionArgument::extract(obj, holder) { Ok(value) => Ok(value), Err(e) => Err(argument_extraction_error(obj.py(), arg_name, e)), } @@ -20,31 +94,39 @@ where /// Alternative to [`extract_argument`] used for `Option` arguments (because they are implicitly treated /// as optional if at the end of the positional parameters). #[doc(hidden)] -pub fn extract_optional_argument<'py, T>( +pub fn extract_optional_argument<'a, 'py, T>( obj: Option<&'py PyAny>, + holder: &'a mut T::Holder, arg_name: &str, ) -> PyResult> where - T: FromPyObject<'py>, + T: PyFunctionArgument<'a, 'py>, { match obj { - Some(obj) => extract_argument(obj, arg_name), + Some(obj) => { + if obj.is_none() { + Ok(None) + } else { + extract_argument(obj, holder, arg_name).map(Some) + } + } None => Ok(None), } } /// Alternative to [`extract_argument`] used when the argument has a default value provided by an annotation. #[doc(hidden)] -pub fn extract_argument_with_default<'py, T>( +pub fn extract_argument_with_default<'a, 'py, T>( obj: Option<&'py PyAny>, + holder: &'a mut T::Holder, arg_name: &str, default: fn() -> T, ) -> PyResult where - T: FromPyObject<'py>, + T: PyFunctionArgument<'a, 'py>, { match obj { - Some(obj) => extract_argument(obj, arg_name), + Some(obj) => extract_argument(obj, holder, arg_name), None => Ok(default()), } } diff --git a/src/impl_/pymethods.rs b/src/impl_/pymethods.rs index 12fbac75..8b058c65 100644 --- a/src/impl_/pymethods.rs +++ b/src/impl_/pymethods.rs @@ -1,5 +1,5 @@ use crate::internal_tricks::{extract_cstr_or_leak_cstring, NulByteInString}; -use crate::{ffi, AsPyPointer, FromPyObject, PyAny, PyObject, PyResult, Python}; +use crate::{ffi, AsPyPointer, PyAny, PyObject, PyResult, Python}; use std::ffi::CStr; use std::fmt; use std::os::raw::{c_int, c_void}; @@ -25,14 +25,14 @@ pub type ipowfunc = unsafe extern "C" fn( impl IPowModulo { #[cfg(Py_3_8)] #[inline] - pub fn extract<'a, T: FromPyObject<'a>>(self, py: Python<'a>) -> PyResult { - unsafe { py.from_borrowed_ptr::(self.0) }.extract() + pub fn to_borrowed_any(self, py: Python<'_>) -> &PyAny { + unsafe { py.from_borrowed_ptr::(self.0) } } #[cfg(not(Py_3_8))] #[inline] - pub fn extract<'a, T: FromPyObject<'a>>(self, py: Python<'a>) -> PyResult { - unsafe { py.from_borrowed_ptr::(ffi::Py_None()) }.extract() + pub fn to_borrowed_any(self, py: Python<'_>) -> &PyAny { + unsafe { py.from_borrowed_ptr::(ffi::Py_None()) } } }