From deec73939996f95fc7be2d1f53cfa3c275969f93 Mon Sep 17 00:00:00 2001 From: Icxolu <10486322+Icxolu@users.noreply.github.com> Date: Sun, 31 Mar 2024 20:25:20 +0200 Subject: [PATCH 1/7] refactor `FnArg` --- pyo3-macros-backend/src/method.rs | 91 ++++++---- pyo3-macros-backend/src/params.rs | 167 +++++++++--------- pyo3-macros-backend/src/pyclass.rs | 19 +- .../src/pyfunction/signature.rs | 115 ++++++++---- pyo3-macros-backend/src/pymethod.rs | 22 ++- 5 files changed, 228 insertions(+), 186 deletions(-) diff --git a/pyo3-macros-backend/src/method.rs b/pyo3-macros-backend/src/method.rs index 155c554025d..bdb1d7539fe 100644 --- a/pyo3-macros-backend/src/method.rs +++ b/pyo3-macros-backend/src/method.rs @@ -20,13 +20,20 @@ use crate::{ pub struct FnArg<'a> { pub name: &'a syn::Ident, pub ty: &'a syn::Type, - pub optional: Option<&'a syn::Type>, - pub default: Option, - pub py: bool, pub attrs: PyFunctionArgPyO3Attributes, - pub is_varargs: bool, - pub is_kwargs: bool, - pub is_cancel_handle: bool, + pub kind: FnArgKind<'a>, +} + +#[derive(Clone, Debug)] +pub enum FnArgKind<'a> { + Regular { + default: Option, + ty_opt: Option<&'a syn::Type>, + }, + VarArgs, + KwArgs, + Py, + CancelHandle, } impl<'a> FnArg<'a> { @@ -47,26 +54,36 @@ impl<'a> FnArg<'a> { other => return Err(handle_argument_error(other)), }; - let is_cancel_handle = arg_attrs.cancel_handle.is_some(); + if utils::is_python(&cap.ty) { + return Ok(Self { + name: ident, + ty: &cap.ty, + attrs: arg_attrs, + kind: FnArgKind::Py, + }); + } + + if arg_attrs.cancel_handle.is_some() { + return Ok(Self { + name: ident, + ty: &cap.ty, + attrs: arg_attrs, + kind: FnArgKind::CancelHandle, + }); + } - Ok(FnArg { + Ok(Self { name: ident, ty: &cap.ty, - optional: utils::option_type_argument(&cap.ty), - default: None, - py: utils::is_python(&cap.ty), attrs: arg_attrs, - is_varargs: false, - is_kwargs: false, - is_cancel_handle, + kind: FnArgKind::Regular { + default: None, + ty_opt: utils::option_type_argument(&cap.ty), + }, }) } } } - - 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 { @@ -492,12 +509,22 @@ impl<'a> FnSpec<'a> { .signature .arguments .iter() - .filter(|arg| arg.is_cancel_handle); + .filter(|arg| matches!(arg.kind, FnArgKind::CancelHandle)); let cancel_handle = cancel_handle_iter.next(); - if let Some(arg) = cancel_handle { - ensure_spanned!(self.asyncness.is_some(), arg.name.span() => "`cancel_handle` attribute can only be used with `async fn`"); - if let Some(arg2) = cancel_handle_iter.next() { - bail_spanned!(arg2.name.span() => "`cancel_handle` may only be specified once"); + if let Some(FnArg { + name, + kind: FnArgKind::CancelHandle, + .. + }) = cancel_handle + { + ensure_spanned!(self.asyncness.is_some(), name.span() => "`cancel_handle` attribute can only be used with `async fn`"); + if let Some(FnArg { + name, + kind: FnArgKind::CancelHandle, + .. + }) = cancel_handle_iter.next() + { + bail_spanned!(name.span() => "`cancel_handle` may only be specified once"); } } @@ -616,14 +643,10 @@ impl<'a> FnSpec<'a> { .signature .arguments .iter() - .map(|arg| { - if arg.py { - quote!(py) - } else if arg.is_cancel_handle { - quote!(__cancel_handle) - } else { - unreachable!() - } + .map(|arg| match arg.kind { + FnArgKind::Py => quote!(py), + FnArgKind::CancelHandle { .. } => quote!(__cancel_handle), + _ => unreachable!(), }) .collect(); let call = rust_call(args, &mut holders); @@ -646,7 +669,7 @@ impl<'a> FnSpec<'a> { } CallingConvention::Fastcall => { let mut holders = Holders::new(); - let (arg_convert, args) = impl_arg_params(self, cls, true, &mut holders, ctx)?; + let (arg_convert, args) = impl_arg_params(self, cls, true, &mut holders, ctx); let call = rust_call(args, &mut holders); let init_holders = holders.init_holders(ctx); let check_gil_refs = holders.check_gil_refs(); @@ -671,7 +694,7 @@ impl<'a> FnSpec<'a> { } CallingConvention::Varargs => { let mut holders = Holders::new(); - let (arg_convert, args) = impl_arg_params(self, cls, false, &mut holders, ctx)?; + let (arg_convert, args) = impl_arg_params(self, cls, false, &mut holders, ctx); let call = rust_call(args, &mut holders); let init_holders = holders.init_holders(ctx); let check_gil_refs = holders.check_gil_refs(); @@ -695,7 +718,7 @@ impl<'a> FnSpec<'a> { } CallingConvention::TpNew => { let mut holders = Holders::new(); - let (arg_convert, args) = impl_arg_params(self, cls, false, &mut holders, ctx)?; + let (arg_convert, args) = impl_arg_params(self, cls, false, &mut holders, ctx); let self_arg = self .tp .self_arg(cls, ExtractErrorMode::Raise, &mut holders, ctx); diff --git a/pyo3-macros-backend/src/params.rs b/pyo3-macros-backend/src/params.rs index fa50d260986..bf071c7082b 100644 --- a/pyo3-macros-backend/src/params.rs +++ b/pyo3-macros-backend/src/params.rs @@ -1,13 +1,12 @@ use crate::utils::Ctx; use crate::{ - method::{FnArg, FnSpec}, + method::{FnArg, FnArgKind, FnSpec}, pyfunction::FunctionSignature, quotes::some_wrap, }; use proc_macro2::{Span, TokenStream}; use quote::{quote, quote_spanned}; use syn::spanned::Spanned; -use syn::Result; pub struct Holders { holders: Vec, @@ -62,11 +61,11 @@ pub fn is_forwarded_args(signature: &FunctionSignature<'_>) -> bool { signature.arguments.as_slice(), [ FnArg { - is_varargs: true, + kind: FnArgKind::VarArgs, .. }, FnArg { - is_kwargs: true, + kind: FnArgKind::KwArgs, .. }, ] @@ -90,7 +89,7 @@ pub fn impl_arg_params( fastcall: bool, holders: &mut Holders, ctx: &Ctx, -) -> Result<(TokenStream, Vec)> { +) -> (TokenStream, Vec) { let args_array = syn::Ident::new("output", Span::call_site()); let Ctx { pyo3_path } = ctx; @@ -119,28 +118,16 @@ pub fn impl_arg_params( .arguments .iter() .enumerate() - .map(|(i, arg)| { - 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()), - ctx, - ) - }) - }) - .collect::>()?; - return Ok(( + .map(|(i, arg)| impl_arg_param(arg, i, &mut 0, holders, ctx)) + .collect(); + return ( 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, - )); + ); }; let positional_parameter_names = &spec.signature.python_signature.positional_parameters; @@ -171,18 +158,8 @@ pub fn impl_arg_params( .arguments .iter() .enumerate() - .map(|(i, arg)| { - 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) - }) - }) - .collect::>()?; + .map(|(i, arg)| impl_arg_param(arg, i, &mut option_pos, holders, ctx)) + .collect(); let args_handler = if spec.signature.python_signature.varargs.is_some() { quote! { #pyo3_path::impl_::extract_argument::TupleVarargs } @@ -224,7 +201,7 @@ pub fn impl_arg_params( }; // create array of arguments, and then parse - Ok(( + ( quote! { const DESCRIPTION: #pyo3_path::impl_::extract_argument::FunctionDescription = #pyo3_path::impl_::extract_argument::FunctionDescription { cls_name: #cls_name, @@ -239,80 +216,96 @@ pub fn impl_arg_params( #from_py_with }, param_conversion, - )) + ) +} + +fn impl_arg_param( + arg: &FnArg<'_>, + pos: usize, + option_pos: &mut usize, + holders: &mut Holders, + ctx: &Ctx, +) -> TokenStream { + let Ctx { pyo3_path } = ctx; + let args_array = syn::Ident::new("output", Span::call_site()); + + match arg.kind { + FnArgKind::Regular { .. } => { + let from_py_with = syn::Ident::new(&format!("from_py_with_{}", pos), Span::call_site()); + let arg_value = quote!(#args_array[#option_pos].as_deref()); + *option_pos += 1; + let tokens = impl_regular_arg_param(arg, from_py_with, arg_value, holders, ctx); + check_arg_for_gil_refs(tokens, holders.push_gil_refs_checker(arg.ty.span()), ctx) + } + FnArgKind::VarArgs => { + let holder = holders.push_holder(arg.name.span()); + let name_str = arg.name.to_string(); + quote_spanned! { arg.name.span() => + #pyo3_path::impl_::extract_argument::extract_argument( + &_args, + &mut #holder, + #name_str + )? + } + } + FnArgKind::KwArgs => { + let holder = holders.push_holder(arg.name.span()); + let name_str = arg.name.to_string(); + quote_spanned! { arg.name.span() => + #pyo3_path::impl_::extract_argument::extract_optional_argument( + _kwargs.as_deref(), + &mut #holder, + #name_str, + || ::std::option::Option::None + )? + } + } + FnArgKind::Py => quote! { py }, + FnArgKind::CancelHandle { .. } => quote! { __cancel_handle }, + } } /// 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 -pub(crate) fn impl_arg_param( +pub(crate) fn impl_regular_arg_param( arg: &FnArg<'_>, from_py_with: syn::Ident, arg_value: TokenStream, // expected type: Option<&'a Bound<'py, PyAny>> holders: &mut Holders, ctx: &Ctx, -) -> Result { +) -> TokenStream { + // FIXME: Is there a better way to guarantee this in the type system? + let FnArg { + name, + ty, + attrs, + kind: FnArgKind::Regular { default, ty_opt }, + } = arg + else { + unreachable!() + }; let Ctx { pyo3_path } = ctx; - let pyo3_path = pyo3_path.to_tokens_spanned(arg.ty.span()); + let pyo3_path = pyo3_path.to_tokens_spanned(ty.span()); // Use this macro inside this function, to ensure that all code generated here is associated // with the function argument macro_rules! quote_arg_span { - ($($tokens:tt)*) => { quote_spanned!(arg.ty.span() => $($tokens)*) } - } - - if arg.py { - return Ok(quote! { py }); - } - - if arg.is_cancel_handle { - return Ok(quote! { __cancel_handle }); + ($($tokens:tt)*) => { quote_spanned!(ty.span() => $($tokens)*) } } - let name = arg.name; let name_str = name.to_string(); - - if arg.is_varargs { - ensure_spanned!( - arg.optional.is_none(), - arg.name.span() => "args cannot be optional" - ); - let holder = holders.push_holder(arg.ty.span()); - return Ok(quote_arg_span! { - #pyo3_path::impl_::extract_argument::extract_argument( - &_args, - &mut #holder, - #name_str - )? - }); - } else if arg.is_kwargs { - ensure_spanned!( - arg.optional.is_some(), - arg.name.span() => "kwargs must be Option<_>" - ); - let holder = holders.push_holder(arg.name.span()); - return Ok(quote_arg_span! { - #pyo3_path::impl_::extract_argument::extract_optional_argument( - _kwargs.as_deref(), - &mut #holder, - #name_str, - || ::std::option::Option::None - )? - }); - } - - let mut default = arg.default.as_ref().map(|expr| quote!(#expr)); + let mut default = default.as_ref().map(|expr| quote!(#expr)); // Option arguments have special treatment: the default should be specified _without_ the // Some() wrapper. Maybe this should be changed in future?! - if arg.optional.is_some() { + if ty_opt.is_some() { default = Some(default.map_or_else( || quote!(::std::option::Option::None), |tokens| some_wrap(tokens, ctx), )); } - let tokens = if arg - .attrs + if attrs .from_py_with .as_ref() .map(|attr| &attr.value) @@ -339,7 +332,7 @@ pub(crate) fn impl_arg_param( )? } } - } else if arg.optional.is_some() { + } else if ty_opt.is_some() { let holder = holders.push_holder(arg.name.span()); quote_arg_span! { #pyo3_path::impl_::extract_argument::extract_optional_argument( @@ -353,7 +346,7 @@ pub(crate) fn impl_arg_param( )? } } else if let Some(default) = default { - let holder = holders.push_holder(arg.name.span()); + let holder = holders.push_holder(name.span()); quote_arg_span! { #pyo3_path::impl_::extract_argument::extract_argument_with_default( #arg_value, @@ -366,7 +359,7 @@ pub(crate) fn impl_arg_param( )? } } else { - let holder = holders.push_holder(arg.name.span()); + let holder = holders.push_holder(name.span()); quote_arg_span! { #pyo3_path::impl_::extract_argument::extract_argument( #pyo3_path::impl_::extract_argument::unwrap_required_argument(#arg_value), @@ -374,7 +367,5 @@ pub(crate) fn impl_arg_param( #name_str )? } - }; - - Ok(tokens) + } } diff --git a/pyo3-macros-backend/src/pyclass.rs b/pyo3-macros-backend/src/pyclass.rs index cf8d9aae801..059fd4399e5 100644 --- a/pyo3-macros-backend/src/pyclass.rs +++ b/pyo3-macros-backend/src/pyclass.rs @@ -7,7 +7,7 @@ use crate::attributes::{ }; use crate::deprecations::Deprecations; use crate::konst::{ConstAttributes, ConstSpec}; -use crate::method::{FnArg, FnSpec}; +use crate::method::{FnArg, FnArgKind, FnSpec}; use crate::pyimpl::{gen_py_const, PyClassMethodsType}; use crate::pymethod::{ impl_py_getter_def, impl_py_setter_def, MethodAndMethodDef, MethodAndSlotDef, PropertyType, @@ -1151,13 +1151,8 @@ fn complex_enum_struct_variant_new<'a>( FnArg { name: &arg_py_ident, ty: &arg_py_type, - optional: None, - default: None, - py: true, attrs: attrs.clone(), - is_varargs: false, - is_kwargs: false, - is_cancel_handle: false, + kind: FnArgKind::Py, }, ]; @@ -1165,13 +1160,11 @@ fn complex_enum_struct_variant_new<'a>( args.push(FnArg { name: field.ident, ty: field.ty, - optional: None, - default: None, - py: false, attrs: attrs.clone(), - is_varargs: false, - is_kwargs: false, - is_cancel_handle: false, + kind: FnArgKind::Regular { + default: None, + ty_opt: None, + }, }); } args diff --git a/pyo3-macros-backend/src/pyfunction/signature.rs b/pyo3-macros-backend/src/pyfunction/signature.rs index baf01285658..0aebdc0e54d 100644 --- a/pyo3-macros-backend/src/pyfunction/signature.rs +++ b/pyo3-macros-backend/src/pyfunction/signature.rs @@ -10,7 +10,7 @@ use syn::{ use crate::{ attributes::{kw, KeywordAttribute}, - method::FnArg, + method::{FnArg, FnArgKind}, }; pub struct Signature { @@ -351,36 +351,39 @@ impl<'a> FunctionSignature<'a> { let mut next_non_py_argument_checked = |name: &syn::Ident| { for fn_arg in args_iter.by_ref() { - if fn_arg.py { - // If the user incorrectly tried to include py: Python in the - // signature, give a useful error as a hint. - ensure_spanned!( - name != fn_arg.name, - name.span() => "arguments of type `Python` must not be part of the signature" - ); - // Otherwise try next argument. - continue; - } - if fn_arg.is_cancel_handle { - // If the user incorrectly tried to include cancel: CoroutineCancel in the - // signature, give a useful error as a hint. - ensure_spanned!( - name != fn_arg.name, - name.span() => "`cancel_handle` argument must not be part of the signature" - ); - // Otherwise try next argument. - continue; + match fn_arg.kind { + crate::method::FnArgKind::Py => { + // If the user incorrectly tried to include py: Python in the + // signature, give a useful error as a hint. + ensure_spanned!( + name != fn_arg.name, + name.span() => "arguments of type `Python` must not be part of the signature" + ); + // Otherwise try next argument. + continue; + } + crate::method::FnArgKind::CancelHandle => { + // If the user incorrectly tried to include cancel: CoroutineCancel in the + // signature, give a useful error as a hint. + ensure_spanned!( + name != fn_arg.name, + name.span() => "`cancel_handle` argument must not be part of the signature" + ); + // Otherwise try next argument. + continue; + } + _ => { + ensure_spanned!( + name == fn_arg.name, + name.span() => format!( + "expected argument from function definition `{}` but got argument `{}`", + fn_arg.name.unraw(), + name.unraw(), + ) + ); + return Ok(fn_arg); + } } - - ensure_spanned!( - name == fn_arg.name, - name.span() => format!( - "expected argument from function definition `{}` but got argument `{}`", - fn_arg.name.unraw(), - name.unraw(), - ) - ); - return Ok(fn_arg); } bail_spanned!( name.span() => "signature entry does not have a corresponding function argument" @@ -397,8 +400,15 @@ impl<'a> FunctionSignature<'a> { arg.eq_and_default.is_none(), arg.span(), )?; - if let Some((_, default)) = &arg.eq_and_default { - fn_arg.default = Some(default.clone()); + if let Some((_, default_expr)) = &arg.eq_and_default { + if let FnArgKind::Regular { default, .. } = &mut fn_arg.kind { + *default = Some(default_expr.clone()); + } else { + // FIXME: In what case can this happen? What should the error message be? + bail_spanned!( + default_expr.span() => "todo" + ) + } } } SignatureItem::VarargsSep(sep) => { @@ -406,12 +416,22 @@ impl<'a> FunctionSignature<'a> { } SignatureItem::Varargs(varargs) => { let fn_arg = next_non_py_argument_checked(&varargs.ident)?; - fn_arg.is_varargs = true; + // FIXME: This needs a ui test covering it + ensure_spanned!( + matches!(fn_arg.kind, FnArgKind::Regular { ty_opt: None, .. }), + fn_arg.name.span() => "args cannot be optional" + ); + fn_arg.kind = FnArgKind::VarArgs; parse_state.add_varargs(&mut python_signature, varargs)?; } SignatureItem::Kwargs(kwargs) => { let fn_arg = next_non_py_argument_checked(&kwargs.ident)?; - fn_arg.is_kwargs = true; + // FIXME: This needs ui test covering it + ensure_spanned!( + matches!(fn_arg.kind, FnArgKind::Regular { ty_opt: Some(_), .. }), + fn_arg.name.span() => "kwargs must be Option<_>" + ); + fn_arg.kind = FnArgKind::KwArgs; parse_state.add_kwargs(&mut python_signature, kwargs)?; } SignatureItem::PosargsSep(sep) => { @@ -421,7 +441,9 @@ impl<'a> FunctionSignature<'a> { } // Ensure no non-py arguments remain - if let Some(arg) = args_iter.find(|arg| !arg.py && !arg.is_cancel_handle) { + if let Some(arg) = + args_iter.find(|arg| !matches!(arg.kind, FnArgKind::Py | FnArgKind::CancelHandle)) + { bail_spanned!( attribute.kw.span() => format!("missing signature entry for argument `{}`", arg.name) ); @@ -439,11 +461,11 @@ impl<'a> FunctionSignature<'a> { let mut python_signature = PythonSignature::default(); for arg in &arguments { // Python<'_> arguments don't show in Python signature - if arg.py || arg.is_cancel_handle { + if matches!(arg.kind, FnArgKind::Py | FnArgKind::CancelHandle) { continue; } - if arg.optional.is_none() { + if matches!(arg.kind, FnArgKind::Regular { ty_opt: None, .. }) { // This argument is required, all previous arguments must also have been required ensure_spanned!( python_signature.required_positional_parameters == python_signature.positional_parameters.len(), @@ -470,7 +492,15 @@ impl<'a> FunctionSignature<'a> { fn default_value_for_parameter(&self, parameter: &str) -> String { let mut default = "...".to_string(); if let Some(fn_arg) = self.arguments.iter().find(|arg| arg.name == parameter) { - if let Some(arg_default) = fn_arg.default.as_ref() { + if let FnArg { + kind: + FnArgKind::Regular { + default: Some(arg_default), + .. + }, + .. + } = fn_arg + { match arg_default { // literal values syn::Expr::Lit(syn::ExprLit { lit, .. }) => match lit { @@ -496,7 +526,14 @@ impl<'a> FunctionSignature<'a> { // others, unsupported yet so defaults to `...` _ => {} } - } else if fn_arg.optional.is_some() { + } else if let FnArg { + kind: + FnArgKind::Regular { + ty_opt: Some(..), .. + }, + .. + } = fn_arg + { // functions without a `#[pyo3(signature = (...))]` option // will treat trailing `Option` arguments as having a default of `None` default = "None".to_string(); diff --git a/pyo3-macros-backend/src/pymethod.rs b/pyo3-macros-backend/src/pymethod.rs index ee7d3d7aaee..ac30a9a33e6 100644 --- a/pyo3-macros-backend/src/pymethod.rs +++ b/pyo3-macros-backend/src/pymethod.rs @@ -1,8 +1,8 @@ use std::borrow::Cow; use crate::attributes::{NameAttribute, RenamingRule}; -use crate::method::{CallingConvention, ExtractErrorMode}; -use crate::params::{check_arg_for_gil_refs, impl_arg_param, Holders}; +use crate::method::{CallingConvention, ExtractErrorMode, FnArgKind}; +use crate::params::{check_arg_for_gil_refs, impl_regular_arg_param, Holders}; use crate::utils::Ctx; use crate::utils::PythonDoc; use crate::{ @@ -606,20 +606,18 @@ pub fn impl_py_setter_def( (quote!(), syn::Ident::new("dummy", Span::call_site())) }; - let extract = impl_arg_param( + let tokens = impl_regular_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, - ) - })?; + ); + let extract = check_arg_for_gil_refs( + tokens, + holders.push_gil_refs_checker(value_arg.ty.span()), + ctx, + ); quote! { #from_py_with let _val = #extract; @@ -1507,7 +1505,7 @@ fn extract_proto_arguments( let mut non_python_args = 0; for arg in &spec.signature.arguments { - if arg.py { + if let FnArgKind::Py = arg.kind { args.push(quote! { py }); } else { let ident = syn::Ident::new(&format!("arg{}", non_python_args), Span::call_site()); From 3087e6dba08a01f081c48fc410a408632e80ea2c Mon Sep 17 00:00:00 2001 From: Icxolu <10486322+Icxolu@users.noreply.github.com> Date: Mon, 1 Apr 2024 23:36:05 +0200 Subject: [PATCH 2/7] add UI tests --- .../src/pyfunction/signature.rs | 2 -- tests/ui/invalid_pyfunctions.rs | 10 +++++++++- tests/ui/invalid_pyfunctions.stderr | 20 +++++++++++++++---- 3 files changed, 25 insertions(+), 7 deletions(-) diff --git a/pyo3-macros-backend/src/pyfunction/signature.rs b/pyo3-macros-backend/src/pyfunction/signature.rs index 0aebdc0e54d..db281fba846 100644 --- a/pyo3-macros-backend/src/pyfunction/signature.rs +++ b/pyo3-macros-backend/src/pyfunction/signature.rs @@ -416,7 +416,6 @@ impl<'a> FunctionSignature<'a> { } SignatureItem::Varargs(varargs) => { let fn_arg = next_non_py_argument_checked(&varargs.ident)?; - // FIXME: This needs a ui test covering it ensure_spanned!( matches!(fn_arg.kind, FnArgKind::Regular { ty_opt: None, .. }), fn_arg.name.span() => "args cannot be optional" @@ -426,7 +425,6 @@ impl<'a> FunctionSignature<'a> { } SignatureItem::Kwargs(kwargs) => { let fn_arg = next_non_py_argument_checked(&kwargs.ident)?; - // FIXME: This needs ui test covering it ensure_spanned!( matches!(fn_arg.kind, FnArgKind::Regular { ty_opt: Some(_), .. }), fn_arg.name.span() => "kwargs must be Option<_>" diff --git a/tests/ui/invalid_pyfunctions.rs b/tests/ui/invalid_pyfunctions.rs index eaa241c074b..1a95c9e4a34 100644 --- a/tests/ui/invalid_pyfunctions.rs +++ b/tests/ui/invalid_pyfunctions.rs @@ -1,5 +1,5 @@ use pyo3::prelude::*; -use pyo3::types::PyString; +use pyo3::types::{PyDict, PyString, PyTuple}; #[pyfunction] fn generic_function(value: T) {} @@ -16,6 +16,14 @@ fn destructured_argument((a, b): (i32, i32)) {} #[pyfunction] fn function_with_required_after_option(_opt: Option, _x: i32) {} +#[pyfunction] +#[pyo3(signature=(*args))] +fn function_with_optional_args(args: Option>) {} + +#[pyfunction] +#[pyo3(signature=(**kwargs))] +fn function_with_required_kwargs(kwargs: Bound<'_, PyDict>) {} + #[pyfunction(pass_module)] fn pass_module_but_no_arguments<'py>() {} diff --git a/tests/ui/invalid_pyfunctions.stderr b/tests/ui/invalid_pyfunctions.stderr index 299b20687cb..893d7cbec2c 100644 --- a/tests/ui/invalid_pyfunctions.stderr +++ b/tests/ui/invalid_pyfunctions.stderr @@ -29,16 +29,28 @@ error: required arguments after an `Option<_>` argument are ambiguous 17 | fn function_with_required_after_option(_opt: Option, _x: i32) {} | ^^^ +error: args cannot be optional + --> tests/ui/invalid_pyfunctions.rs:21:32 + | +21 | fn function_with_optional_args(args: Option>) {} + | ^^^^ + +error: kwargs must be Option<_> + --> tests/ui/invalid_pyfunctions.rs:25:34 + | +25 | fn function_with_required_kwargs(kwargs: Bound<'_, PyDict>) {} + | ^^^^^^ + error: expected `&PyModule` or `Py` as first argument with `pass_module` - --> tests/ui/invalid_pyfunctions.rs:20:37 + --> tests/ui/invalid_pyfunctions.rs:28:37 | -20 | fn pass_module_but_no_arguments<'py>() {} +28 | fn pass_module_but_no_arguments<'py>() {} | ^^ error[E0277]: the trait bound `&str: From>` is not satisfied - --> tests/ui/invalid_pyfunctions.rs:24:13 + --> tests/ui/invalid_pyfunctions.rs:32:13 | -24 | string: &str, +32 | string: &str, | ^ the trait `From>` is not implemented for `&str`, which is required by `BoundRef<'_, '_, pyo3::prelude::PyModule>: Into<_>` | = help: the following other types implement trait `From`: From b97d2c4f625f577be162588f79c0aae70ad6c9c9 Mon Sep 17 00:00:00 2001 From: Icxolu <10486322+Icxolu@users.noreply.github.com> Date: Tue, 2 Apr 2024 20:43:38 +0200 Subject: [PATCH 3/7] use enum variant types --- pyo3-macros-backend/src/method.rs | 167 +++++++++++++----- pyo3-macros-backend/src/params.rs | 65 +++---- pyo3-macros-backend/src/pyclass.rs | 18 +- .../src/pyfunction/signature.rs | 74 ++++---- pyo3-macros-backend/src/pymethod.rs | 45 ++--- 5 files changed, 212 insertions(+), 157 deletions(-) diff --git a/pyo3-macros-backend/src/method.rs b/pyo3-macros-backend/src/method.rs index bdb1d7539fe..c55e6147f8a 100644 --- a/pyo3-macros-backend/src/method.rs +++ b/pyo3-macros-backend/src/method.rs @@ -6,7 +6,7 @@ use syn::{ext::IdentExt, spanned::Spanned, Ident, Result}; use crate::utils::Ctx; use crate::{ - attributes::{TextSignatureAttribute, TextSignatureAttributeValue}, + attributes::{FromPyWithAttribute, TextSignatureAttribute, TextSignatureAttributeValue}, deprecations::{Deprecation, Deprecations}, params::{impl_arg_params, Holders}, pyfunction::{ @@ -17,26 +17,125 @@ use crate::{ }; #[derive(Clone, Debug)] -pub struct FnArg<'a> { +pub struct RegularArg<'a> { pub name: &'a syn::Ident, pub ty: &'a syn::Type, pub attrs: PyFunctionArgPyO3Attributes, - pub kind: FnArgKind<'a>, + pub default_value: Option, + pub option_wrapped_type: Option<&'a syn::Type>, } #[derive(Clone, Debug)] -pub enum FnArgKind<'a> { - Regular { - default: Option, - ty_opt: Option<&'a syn::Type>, - }, - VarArgs, - KwArgs, - Py, - CancelHandle, +pub struct VarArg<'a> { + pub name: &'a syn::Ident, + pub ty: &'a syn::Type, + pub attrs: PyFunctionArgPyO3Attributes, +} + +#[derive(Clone, Debug)] +pub struct KwArg<'a> { + pub name: &'a syn::Ident, + pub ty: &'a syn::Type, + pub attrs: PyFunctionArgPyO3Attributes, +} + +#[derive(Clone, Debug)] +pub struct CancelHandleArg<'a> { + pub name: &'a syn::Ident, + pub ty: &'a syn::Type, +} + +#[derive(Clone, Debug)] +pub struct PyArg<'a> { + pub name: &'a syn::Ident, + pub ty: &'a syn::Type, +} + +#[derive(Clone, Debug)] +pub enum FnArg<'a> { + Regular(RegularArg<'a>), + VarArgs(VarArg<'a>), + KwArgs(KwArg<'a>), + Py(PyArg<'a>), + CancelHandle(CancelHandleArg<'a>), } impl<'a> FnArg<'a> { + pub fn name(&self) -> &'a syn::Ident { + match self { + FnArg::Regular(RegularArg { name, .. }) => name, + FnArg::VarArgs(VarArg { name, .. }) => name, + FnArg::KwArgs(KwArg { name, .. }) => name, + FnArg::Py(PyArg { name, .. }) => name, + FnArg::CancelHandle(CancelHandleArg { name, .. }) => name, + } + } + + pub fn ty(&self) -> &'a syn::Type { + match self { + FnArg::Regular(RegularArg { ty, .. }) => ty, + FnArg::VarArgs(VarArg { ty, .. }) => ty, + FnArg::KwArgs(KwArg { ty, .. }) => ty, + FnArg::Py(PyArg { ty, .. }) => ty, + FnArg::CancelHandle(CancelHandleArg { ty, .. }) => ty, + } + } + + #[allow(clippy::wrong_self_convention)] + pub fn from_py_with(&self) -> Option<&FromPyWithAttribute> { + if let FnArg::Regular(RegularArg { attrs, .. }) = self { + attrs.from_py_with.as_ref() + } else { + None + } + } + + pub fn to_varargs_mut(&mut self) -> Result<&mut Self> { + if let Self::Regular(RegularArg { + name, + ty, + attrs, + option_wrapped_type: None, + .. + }) = self + { + let attrs = std::mem::replace( + attrs, + PyFunctionArgPyO3Attributes { + from_py_with: None, + cancel_handle: None, + }, + ); + *self = Self::VarArgs(VarArg { name, ty, attrs }); + Ok(self) + } else { + bail_spanned!(self.name().span() => "args cannot be optional") + } + } + + pub fn to_kwargs_mut(&mut self) -> Result<&mut Self> { + if let Self::Regular(RegularArg { + name, + ty, + attrs, + option_wrapped_type: Some(..), + .. + }) = self + { + let attrs = std::mem::replace( + attrs, + PyFunctionArgPyO3Attributes { + from_py_with: None, + cancel_handle: None, + }, + ); + *self = Self::KwArgs(KwArg { name, ty, attrs }); + Ok(self) + } else { + bail_spanned!(self.name().span() => "kwargs must be Option<_>") + } + } + /// Transforms a rust fn arg parsed with syn into a method::FnArg pub fn parse(arg: &'a mut syn::FnArg) -> Result { match arg { @@ -55,32 +154,26 @@ impl<'a> FnArg<'a> { }; if utils::is_python(&cap.ty) { - return Ok(Self { + return Ok(Self::Py(PyArg { name: ident, ty: &cap.ty, - attrs: arg_attrs, - kind: FnArgKind::Py, - }); + })); } if arg_attrs.cancel_handle.is_some() { - return Ok(Self { + return Ok(Self::CancelHandle(CancelHandleArg { name: ident, ty: &cap.ty, - attrs: arg_attrs, - kind: FnArgKind::CancelHandle, - }); + })); } - Ok(Self { + Ok(Self::Regular(RegularArg { name: ident, ty: &cap.ty, attrs: arg_attrs, - kind: FnArgKind::Regular { - default: None, - ty_opt: utils::option_type_argument(&cap.ty), - }, - }) + default_value: None, + option_wrapped_type: utils::option_type_argument(&cap.ty), + })) } } } @@ -509,20 +602,12 @@ impl<'a> FnSpec<'a> { .signature .arguments .iter() - .filter(|arg| matches!(arg.kind, FnArgKind::CancelHandle)); + .filter(|arg| matches!(arg, FnArg::CancelHandle(..))); let cancel_handle = cancel_handle_iter.next(); - if let Some(FnArg { - name, - kind: FnArgKind::CancelHandle, - .. - }) = cancel_handle - { + if let Some(FnArg::CancelHandle(CancelHandleArg { name, .. })) = cancel_handle { ensure_spanned!(self.asyncness.is_some(), name.span() => "`cancel_handle` attribute can only be used with `async fn`"); - if let Some(FnArg { - name, - kind: FnArgKind::CancelHandle, - .. - }) = cancel_handle_iter.next() + if let Some(FnArg::CancelHandle(CancelHandleArg { name, .. })) = + cancel_handle_iter.next() { bail_spanned!(name.span() => "`cancel_handle` may only be specified once"); } @@ -643,9 +728,9 @@ impl<'a> FnSpec<'a> { .signature .arguments .iter() - .map(|arg| match arg.kind { - FnArgKind::Py => quote!(py), - FnArgKind::CancelHandle { .. } => quote!(__cancel_handle), + .map(|arg| match arg { + FnArg::Py(..) => quote!(py), + FnArg::CancelHandle(..) => quote!(__cancel_handle), _ => unreachable!(), }) .collect(); diff --git a/pyo3-macros-backend/src/params.rs b/pyo3-macros-backend/src/params.rs index bf071c7082b..f8b86339039 100644 --- a/pyo3-macros-backend/src/params.rs +++ b/pyo3-macros-backend/src/params.rs @@ -1,11 +1,11 @@ use crate::utils::Ctx; use crate::{ - method::{FnArg, FnArgKind, FnSpec}, + method::{FnArg, FnSpec, RegularArg}, pyfunction::FunctionSignature, quotes::some_wrap, }; use proc_macro2::{Span, TokenStream}; -use quote::{quote, quote_spanned}; +use quote::{format_ident, quote, quote_spanned}; use syn::spanned::Spanned; pub struct Holders { @@ -59,16 +59,7 @@ impl Holders { pub fn is_forwarded_args(signature: &FunctionSignature<'_>) -> bool { matches!( signature.arguments.as_slice(), - [ - FnArg { - kind: FnArgKind::VarArgs, - .. - }, - FnArg { - kind: FnArgKind::KwArgs, - .. - }, - ] + [FnArg::VarArgs(..), FnArg::KwArgs(..),] ) } @@ -99,9 +90,8 @@ pub fn impl_arg_params( .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()); + let from_py_with = &arg.from_py_with().as_ref()?.value; + let from_py_with_holder = format_ident!("from_py_with_{}", i); Some(quote_spanned! { from_py_with.span() => let e = #pyo3_path::impl_::deprecations::GilRefs::new(); let #from_py_with_holder = #pyo3_path::impl_::deprecations::inspect_fn(#from_py_with, &e); @@ -229,15 +219,15 @@ fn impl_arg_param( let Ctx { pyo3_path } = ctx; let args_array = syn::Ident::new("output", Span::call_site()); - match arg.kind { - FnArgKind::Regular { .. } => { - let from_py_with = syn::Ident::new(&format!("from_py_with_{}", pos), Span::call_site()); + match arg { + FnArg::Regular(arg) => { + let from_py_with = format_ident!("from_py_with_{}", pos); let arg_value = quote!(#args_array[#option_pos].as_deref()); *option_pos += 1; let tokens = impl_regular_arg_param(arg, from_py_with, arg_value, holders, ctx); check_arg_for_gil_refs(tokens, holders.push_gil_refs_checker(arg.ty.span()), ctx) } - FnArgKind::VarArgs => { + FnArg::VarArgs(arg) => { let holder = holders.push_holder(arg.name.span()); let name_str = arg.name.to_string(); quote_spanned! { arg.name.span() => @@ -248,7 +238,7 @@ fn impl_arg_param( )? } } - FnArgKind::KwArgs => { + FnArg::KwArgs(arg) => { let holder = holders.push_holder(arg.name.span()); let name_str = arg.name.to_string(); quote_spanned! { arg.name.span() => @@ -260,52 +250,43 @@ fn impl_arg_param( )? } } - FnArgKind::Py => quote! { py }, - FnArgKind::CancelHandle { .. } => quote! { __cancel_handle }, + FnArg::Py(..) => quote! { py }, + FnArg::CancelHandle(..) => quote! { __cancel_handle }, } } /// 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 pub(crate) fn impl_regular_arg_param( - arg: &FnArg<'_>, + arg: &RegularArg<'_>, from_py_with: syn::Ident, arg_value: TokenStream, // expected type: Option<&'a Bound<'py, PyAny>> holders: &mut Holders, ctx: &Ctx, ) -> TokenStream { - // FIXME: Is there a better way to guarantee this in the type system? - let FnArg { - name, - ty, - attrs, - kind: FnArgKind::Regular { default, ty_opt }, - } = arg - else { - unreachable!() - }; let Ctx { pyo3_path } = ctx; - let pyo3_path = pyo3_path.to_tokens_spanned(ty.span()); + let pyo3_path = pyo3_path.to_tokens_spanned(arg.ty.span()); // Use this macro inside this function, to ensure that all code generated here is associated // with the function argument macro_rules! quote_arg_span { - ($($tokens:tt)*) => { quote_spanned!(ty.span() => $($tokens)*) } + ($($tokens:tt)*) => { quote_spanned!(arg.ty.span() => $($tokens)*) } } - let name_str = name.to_string(); - let mut default = default.as_ref().map(|expr| quote!(#expr)); + let name_str = arg.name.to_string(); + let mut default = arg.default_value.as_ref().map(|expr| quote!(#expr)); // Option arguments have special treatment: the default should be specified _without_ the // Some() wrapper. Maybe this should be changed in future?! - if ty_opt.is_some() { + if arg.option_wrapped_type.is_some() { default = Some(default.map_or_else( || quote!(::std::option::Option::None), |tokens| some_wrap(tokens, ctx), )); } - if attrs + if arg + .attrs .from_py_with .as_ref() .map(|attr| &attr.value) @@ -332,7 +313,7 @@ pub(crate) fn impl_regular_arg_param( )? } } - } else if ty_opt.is_some() { + } else if arg.option_wrapped_type.is_some() { let holder = holders.push_holder(arg.name.span()); quote_arg_span! { #pyo3_path::impl_::extract_argument::extract_optional_argument( @@ -346,7 +327,7 @@ pub(crate) fn impl_regular_arg_param( )? } } else if let Some(default) = default { - let holder = holders.push_holder(name.span()); + let holder = holders.push_holder(arg.name.span()); quote_arg_span! { #pyo3_path::impl_::extract_argument::extract_argument_with_default( #arg_value, @@ -359,7 +340,7 @@ pub(crate) fn impl_regular_arg_param( )? } } else { - let holder = holders.push_holder(name.span()); + 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), diff --git a/pyo3-macros-backend/src/pyclass.rs b/pyo3-macros-backend/src/pyclass.rs index 059fd4399e5..081bd497024 100644 --- a/pyo3-macros-backend/src/pyclass.rs +++ b/pyo3-macros-backend/src/pyclass.rs @@ -7,7 +7,7 @@ use crate::attributes::{ }; use crate::deprecations::Deprecations; use crate::konst::{ConstAttributes, ConstSpec}; -use crate::method::{FnArg, FnArgKind, FnSpec}; +use crate::method::{FnArg, FnSpec, PyArg, RegularArg}; use crate::pyimpl::{gen_py_const, PyClassMethodsType}; use crate::pymethod::{ impl_py_getter_def, impl_py_setter_def, MethodAndMethodDef, MethodAndSlotDef, PropertyType, @@ -1148,24 +1148,20 @@ fn complex_enum_struct_variant_new<'a>( let mut args = vec![ // py: Python<'_> - FnArg { + FnArg::Py(PyArg { name: &arg_py_ident, ty: &arg_py_type, - attrs: attrs.clone(), - kind: FnArgKind::Py, - }, + }), ]; for field in &variant.fields { - args.push(FnArg { + args.push(FnArg::Regular(RegularArg { name: field.ident, ty: field.ty, attrs: attrs.clone(), - kind: FnArgKind::Regular { - default: None, - ty_opt: None, - }, - }); + default_value: None, + option_wrapped_type: None, + })); } args }; diff --git a/pyo3-macros-backend/src/pyfunction/signature.rs b/pyo3-macros-backend/src/pyfunction/signature.rs index db281fba846..ecc973f68e9 100644 --- a/pyo3-macros-backend/src/pyfunction/signature.rs +++ b/pyo3-macros-backend/src/pyfunction/signature.rs @@ -10,7 +10,7 @@ use syn::{ use crate::{ attributes::{kw, KeywordAttribute}, - method::{FnArg, FnArgKind}, + method::{FnArg, RegularArg}, }; pub struct Signature { @@ -351,22 +351,22 @@ impl<'a> FunctionSignature<'a> { let mut next_non_py_argument_checked = |name: &syn::Ident| { for fn_arg in args_iter.by_ref() { - match fn_arg.kind { - crate::method::FnArgKind::Py => { + match fn_arg { + crate::method::FnArg::Py(..) => { // If the user incorrectly tried to include py: Python in the // signature, give a useful error as a hint. ensure_spanned!( - name != fn_arg.name, + name != fn_arg.name(), name.span() => "arguments of type `Python` must not be part of the signature" ); // Otherwise try next argument. continue; } - crate::method::FnArgKind::CancelHandle => { + crate::method::FnArg::CancelHandle(..) => { // If the user incorrectly tried to include cancel: CoroutineCancel in the // signature, give a useful error as a hint. ensure_spanned!( - name != fn_arg.name, + name != fn_arg.name(), name.span() => "`cancel_handle` argument must not be part of the signature" ); // Otherwise try next argument. @@ -374,10 +374,10 @@ impl<'a> FunctionSignature<'a> { } _ => { ensure_spanned!( - name == fn_arg.name, + name == fn_arg.name(), name.span() => format!( "expected argument from function definition `{}` but got argument `{}`", - fn_arg.name.unraw(), + fn_arg.name().unraw(), name.unraw(), ) ); @@ -400,13 +400,13 @@ impl<'a> FunctionSignature<'a> { arg.eq_and_default.is_none(), arg.span(), )?; - if let Some((_, default_expr)) = &arg.eq_and_default { - if let FnArgKind::Regular { default, .. } = &mut fn_arg.kind { - *default = Some(default_expr.clone()); + if let Some((_, default)) = &arg.eq_and_default { + if let FnArg::Regular(arg) = fn_arg { + arg.default_value = Some(default.clone()); } else { // FIXME: In what case can this happen? What should the error message be? bail_spanned!( - default_expr.span() => "todo" + default.span() => "todo" ) } } @@ -416,20 +416,12 @@ impl<'a> FunctionSignature<'a> { } SignatureItem::Varargs(varargs) => { let fn_arg = next_non_py_argument_checked(&varargs.ident)?; - ensure_spanned!( - matches!(fn_arg.kind, FnArgKind::Regular { ty_opt: None, .. }), - fn_arg.name.span() => "args cannot be optional" - ); - fn_arg.kind = FnArgKind::VarArgs; + fn_arg.to_varargs_mut()?; parse_state.add_varargs(&mut python_signature, varargs)?; } SignatureItem::Kwargs(kwargs) => { let fn_arg = next_non_py_argument_checked(&kwargs.ident)?; - ensure_spanned!( - matches!(fn_arg.kind, FnArgKind::Regular { ty_opt: Some(_), .. }), - fn_arg.name.span() => "kwargs must be Option<_>" - ); - fn_arg.kind = FnArgKind::KwArgs; + fn_arg.to_kwargs_mut()?; parse_state.add_kwargs(&mut python_signature, kwargs)?; } SignatureItem::PosargsSep(sep) => { @@ -440,10 +432,10 @@ impl<'a> FunctionSignature<'a> { // Ensure no non-py arguments remain if let Some(arg) = - args_iter.find(|arg| !matches!(arg.kind, FnArgKind::Py | FnArgKind::CancelHandle)) + args_iter.find(|arg| !matches!(arg, FnArg::Py(..) | FnArg::CancelHandle(..))) { bail_spanned!( - attribute.kw.span() => format!("missing signature entry for argument `{}`", arg.name) + attribute.kw.span() => format!("missing signature entry for argument `{}`", arg.name()) ); } @@ -459,15 +451,20 @@ impl<'a> FunctionSignature<'a> { let mut python_signature = PythonSignature::default(); for arg in &arguments { // Python<'_> arguments don't show in Python signature - if matches!(arg.kind, FnArgKind::Py | FnArgKind::CancelHandle) { + if matches!(arg, FnArg::Py(..) | FnArg::CancelHandle(..)) { continue; } - if matches!(arg.kind, FnArgKind::Regular { ty_opt: None, .. }) { + if let FnArg::Regular(RegularArg { + ty, + option_wrapped_type: None, + .. + }) = arg + { // This argument is required, all previous arguments must also have been required ensure_spanned!( python_signature.required_positional_parameters == python_signature.positional_parameters.len(), - arg.ty.span() => "required arguments after an `Option<_>` argument are ambiguous\n\ + ty.span() => "required arguments after an `Option<_>` argument are ambiguous\n\ = help: add a `#[pyo3(signature)]` annotation on this function to unambiguously specify the default values for all optional parameters" ); @@ -477,7 +474,7 @@ impl<'a> FunctionSignature<'a> { python_signature .positional_parameters - .push(arg.name.unraw().to_string()); + .push(arg.name().unraw().to_string()); } Ok(Self { @@ -489,15 +486,11 @@ impl<'a> FunctionSignature<'a> { fn default_value_for_parameter(&self, parameter: &str) -> String { let mut default = "...".to_string(); - if let Some(fn_arg) = self.arguments.iter().find(|arg| arg.name == parameter) { - if let FnArg { - kind: - FnArgKind::Regular { - default: Some(arg_default), - .. - }, + if let Some(fn_arg) = self.arguments.iter().find(|arg| arg.name() == parameter) { + if let FnArg::Regular(RegularArg { + default_value: Some(arg_default), .. - } = fn_arg + }) = fn_arg { match arg_default { // literal values @@ -524,13 +517,10 @@ impl<'a> FunctionSignature<'a> { // others, unsupported yet so defaults to `...` _ => {} } - } else if let FnArg { - kind: - FnArgKind::Regular { - ty_opt: Some(..), .. - }, + } else if let FnArg::Regular(RegularArg { + option_wrapped_type: Some(..), .. - } = fn_arg + }) = fn_arg { // functions without a `#[pyo3(signature = (...))]` option // will treat trailing `Option` arguments as having a default of `None` diff --git a/pyo3-macros-backend/src/pymethod.rs b/pyo3-macros-backend/src/pymethod.rs index ac30a9a33e6..cb55401fd30 100644 --- a/pyo3-macros-backend/src/pymethod.rs +++ b/pyo3-macros-backend/src/pymethod.rs @@ -1,7 +1,7 @@ use std::borrow::Cow; use crate::attributes::{NameAttribute, RenamingRule}; -use crate::method::{CallingConvention, ExtractErrorMode, FnArgKind}; +use crate::method::{CallingConvention, ExtractErrorMode, PyArg}; use crate::params::{check_arg_for_gil_refs, impl_regular_arg_param, Holders}; use crate::utils::Ctx; use crate::utils::PythonDoc; @@ -471,7 +471,7 @@ fn impl_py_class_attribute( let (py_arg, args) = split_off_python_arg(&spec.signature.arguments); ensure_spanned!( args.is_empty(), - args[0].ty.span() => "#[classattr] can only have one argument (of type pyo3::Python)" + args[0].ty().span() => "#[classattr] can only have one argument (of type pyo3::Python)" ); let name = &spec.name; @@ -521,7 +521,7 @@ fn impl_call_setter( bail_spanned!(spec.name.span() => "setter function expected to have one argument"); } else if args.len() > 1 { bail_spanned!( - args[1].ty.span() => + args[1].ty().span() => "setter function can have at most two arguments ([pyo3::Python,] and value)" ); } @@ -591,7 +591,7 @@ pub fn impl_py_setter_def( 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) + &value_arg.from_py_with().as_ref().map(|f| &f.value) { let ident = syn::Ident::new("from_py_with", from_py_with.span()); ( @@ -606,18 +606,21 @@ pub fn impl_py_setter_def( (quote!(), syn::Ident::new("dummy", Span::call_site())) }; + let arg = if let FnArg::Regular(arg) = &value_arg { + arg + } else { + bail_spanned!(value_arg.name().span() => "The #[setter] value argument can't be *args or **kwargs"); + }; + let tokens = impl_regular_arg_param( - &args[0], + arg, ident, quote!(::std::option::Option::Some(_value.into())), &mut holders, ctx, ); - let extract = check_arg_for_gil_refs( - tokens, - holders.push_gil_refs_checker(value_arg.ty.span()), - ctx, - ); + let extract = + check_arg_for_gil_refs(tokens, holders.push_gil_refs_checker(arg.ty.span()), ctx); quote! { #from_py_with let _val = #extract; @@ -703,7 +706,7 @@ fn impl_call_getter( let slf = self_type.receiver(cls, ExtractErrorMode::Raise, holders, ctx); ensure_spanned!( args.is_empty(), - args[0].ty.span() => "getter function can only have one argument (of type pyo3::Python)" + args[0].ty().span() => "getter function can only have one argument (of type pyo3::Python)" ); let name = &spec.name; @@ -825,9 +828,9 @@ pub fn impl_py_getter_def( } /// Split an argument of pyo3::Python from the front of the arg list, if present -fn split_off_python_arg<'a>(args: &'a [FnArg<'a>]) -> (Option<&FnArg<'_>>, &[FnArg<'_>]) { +fn split_off_python_arg<'a>(args: &'a [FnArg<'a>]) -> (Option<&PyArg<'_>>, &[FnArg<'_>]) { match args { - [py, args @ ..] if utils::is_python(py.ty) => (Some(py), args), + [FnArg::Py(py), args @ ..] => (Some(py), args), args => (None, args), } } @@ -1034,14 +1037,14 @@ impl Ty { ctx: &Ctx, ) -> TokenStream { let Ctx { pyo3_path } = ctx; - let name_str = arg.name.unraw().to_string(); + let name_str = arg.name().unraw().to_string(); match self { Ty::Object => extract_object( extract_error_mode, holders, &name_str, quote! { #ident }, - arg.ty.span(), + arg.ty().span(), ctx ), Ty::MaybeNullObject => extract_object( @@ -1055,7 +1058,7 @@ impl Ty { #ident } }, - arg.ty.span(), + arg.ty().span(), ctx ), Ty::NonNullObject => extract_object( @@ -1063,7 +1066,7 @@ impl Ty { holders, &name_str, quote! { #ident.as_ptr() }, - arg.ty.span(), + arg.ty().span(), ctx ), Ty::IPowModulo => extract_object( @@ -1071,7 +1074,7 @@ impl Ty { holders, &name_str, quote! { #ident.as_ptr() }, - arg.ty.span(), + arg.ty().span(), ctx ), Ty::CompareOp => extract_error_mode.handle_error( @@ -1082,7 +1085,7 @@ impl Ty { ctx ), Ty::PySsizeT => { - let ty = arg.ty; + let ty = arg.ty(); extract_error_mode.handle_error( quote! { ::std::convert::TryInto::<#ty>::try_into(#ident).map_err(|e| #pyo3_path::exceptions::PyValueError::new_err(e.to_string())) @@ -1505,12 +1508,12 @@ fn extract_proto_arguments( let mut non_python_args = 0; for arg in &spec.signature.arguments { - if let FnArgKind::Py = arg.kind { + if let FnArg::Py(..) = arg { 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())))? + .ok_or_else(|| err_spanned!(arg.ty().span() => format!("Expected at most {} non-python arguments", proto_args.len())))? .extract(&ident, arg, extract_error_mode, holders, ctx); non_python_args += 1; args.push(conversions); From f578da1c4cb9414386a20a6971d573ad490f8b52 Mon Sep 17 00:00:00 2001 From: Icxolu <10486322+Icxolu@users.noreply.github.com> Date: Tue, 2 Apr 2024 21:11:18 +0200 Subject: [PATCH 4/7] add comment --- pyo3-macros-backend/src/method.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyo3-macros-backend/src/method.rs b/pyo3-macros-backend/src/method.rs index c55e6147f8a..e043de321c8 100644 --- a/pyo3-macros-backend/src/method.rs +++ b/pyo3-macros-backend/src/method.rs @@ -731,7 +731,7 @@ impl<'a> FnSpec<'a> { .map(|arg| match arg { FnArg::Py(..) => quote!(py), FnArg::CancelHandle(..) => quote!(__cancel_handle), - _ => unreachable!(), + _ => unreachable!("`CallingConvention::Noargs` should not contain any arguments (reaching Python) except for `self`, which is handled below."), }) .collect(); let call = rust_call(args, &mut holders); From 7c924e9b8bce5036348e73eadce0848f0499e645 Mon Sep 17 00:00:00 2001 From: Icxolu <10486322+Icxolu@users.noreply.github.com> Date: Wed, 3 Apr 2024 00:16:44 +0200 Subject: [PATCH 5/7] remove dead code --- pyo3-macros-backend/src/method.rs | 22 ++-------------------- 1 file changed, 2 insertions(+), 20 deletions(-) diff --git a/pyo3-macros-backend/src/method.rs b/pyo3-macros-backend/src/method.rs index e043de321c8..071d000ff65 100644 --- a/pyo3-macros-backend/src/method.rs +++ b/pyo3-macros-backend/src/method.rs @@ -29,14 +29,12 @@ pub struct RegularArg<'a> { pub struct VarArg<'a> { pub name: &'a syn::Ident, pub ty: &'a syn::Type, - pub attrs: PyFunctionArgPyO3Attributes, } #[derive(Clone, Debug)] pub struct KwArg<'a> { pub name: &'a syn::Ident, pub ty: &'a syn::Type, - pub attrs: PyFunctionArgPyO3Attributes, } #[derive(Clone, Debug)] @@ -94,19 +92,11 @@ impl<'a> FnArg<'a> { if let Self::Regular(RegularArg { name, ty, - attrs, option_wrapped_type: None, .. }) = self { - let attrs = std::mem::replace( - attrs, - PyFunctionArgPyO3Attributes { - from_py_with: None, - cancel_handle: None, - }, - ); - *self = Self::VarArgs(VarArg { name, ty, attrs }); + *self = Self::VarArgs(VarArg { name, ty }); Ok(self) } else { bail_spanned!(self.name().span() => "args cannot be optional") @@ -117,19 +107,11 @@ impl<'a> FnArg<'a> { if let Self::Regular(RegularArg { name, ty, - attrs, option_wrapped_type: Some(..), .. }) = self { - let attrs = std::mem::replace( - attrs, - PyFunctionArgPyO3Attributes { - from_py_with: None, - cancel_handle: None, - }, - ); - *self = Self::KwArgs(KwArg { name, ty, attrs }); + *self = Self::KwArgs(KwArg { name, ty }); Ok(self) } else { bail_spanned!(self.name().span() => "kwargs must be Option<_>") From e7640670c3467df4334b0a2cb511383545f7a345 Mon Sep 17 00:00:00 2001 From: Icxolu <10486322+Icxolu@users.noreply.github.com> Date: Wed, 3 Apr 2024 23:27:33 +0200 Subject: [PATCH 6/7] remove last FIXME --- pyo3-macros-backend/src/pyfunction/signature.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/pyo3-macros-backend/src/pyfunction/signature.rs b/pyo3-macros-backend/src/pyfunction/signature.rs index ecc973f68e9..3daa79c89f5 100644 --- a/pyo3-macros-backend/src/pyfunction/signature.rs +++ b/pyo3-macros-backend/src/pyfunction/signature.rs @@ -404,10 +404,11 @@ impl<'a> FunctionSignature<'a> { if let FnArg::Regular(arg) = fn_arg { arg.default_value = Some(default.clone()); } else { - // FIXME: In what case can this happen? What should the error message be? - bail_spanned!( - default.span() => "todo" - ) + unreachable!( + "`Python` and `CancelHandle` are already handled above and `*args`/`**kwargs` are \ + parsed and transformed below. Because the have to come last and are only allowed \ + once, this has to be a regular argument." + ); } } } From 68bff0fd398902dabaf6535381e70efa3da76beb Mon Sep 17 00:00:00 2001 From: Icxolu <10486322+Icxolu@users.noreply.github.com> Date: Sun, 14 Apr 2024 14:21:22 +0200 Subject: [PATCH 7/7] review feedback davidhewitt --- pyo3-macros-backend/src/method.rs | 41 ++++++++++++++++----------- pyo3-macros-backend/src/params.rs | 10 ++----- pyo3-macros-backend/src/pyclass.rs | 5 +--- pyo3-macros-backend/src/pymethod.rs | 2 +- tests/ui/invalid_cancel_handle.rs | 6 ++++ tests/ui/invalid_cancel_handle.stderr | 6 ++++ 6 files changed, 41 insertions(+), 29 deletions(-) diff --git a/pyo3-macros-backend/src/method.rs b/pyo3-macros-backend/src/method.rs index 071d000ff65..08ddc8b4577 100644 --- a/pyo3-macros-backend/src/method.rs +++ b/pyo3-macros-backend/src/method.rs @@ -20,19 +20,21 @@ use crate::{ pub struct RegularArg<'a> { pub name: &'a syn::Ident, pub ty: &'a syn::Type, - pub attrs: PyFunctionArgPyO3Attributes, + pub from_py_with: Option, pub default_value: Option, pub option_wrapped_type: Option<&'a syn::Type>, } +/// Pythons *args argument #[derive(Clone, Debug)] -pub struct VarArg<'a> { +pub struct VarargsArg<'a> { pub name: &'a syn::Ident, pub ty: &'a syn::Type, } +/// Pythons **kwarg argument #[derive(Clone, Debug)] -pub struct KwArg<'a> { +pub struct KwargsArg<'a> { pub name: &'a syn::Ident, pub ty: &'a syn::Type, } @@ -52,8 +54,8 @@ pub struct PyArg<'a> { #[derive(Clone, Debug)] pub enum FnArg<'a> { Regular(RegularArg<'a>), - VarArgs(VarArg<'a>), - KwArgs(KwArg<'a>), + VarArgs(VarargsArg<'a>), + KwArgs(KwargsArg<'a>), Py(PyArg<'a>), CancelHandle(CancelHandleArg<'a>), } @@ -62,8 +64,8 @@ impl<'a> FnArg<'a> { pub fn name(&self) -> &'a syn::Ident { match self { FnArg::Regular(RegularArg { name, .. }) => name, - FnArg::VarArgs(VarArg { name, .. }) => name, - FnArg::KwArgs(KwArg { name, .. }) => name, + FnArg::VarArgs(VarargsArg { name, .. }) => name, + FnArg::KwArgs(KwargsArg { name, .. }) => name, FnArg::Py(PyArg { name, .. }) => name, FnArg::CancelHandle(CancelHandleArg { name, .. }) => name, } @@ -72,8 +74,8 @@ impl<'a> FnArg<'a> { pub fn ty(&self) -> &'a syn::Type { match self { FnArg::Regular(RegularArg { ty, .. }) => ty, - FnArg::VarArgs(VarArg { ty, .. }) => ty, - FnArg::KwArgs(KwArg { ty, .. }) => ty, + FnArg::VarArgs(VarargsArg { ty, .. }) => ty, + FnArg::KwArgs(KwargsArg { ty, .. }) => ty, FnArg::Py(PyArg { ty, .. }) => ty, FnArg::CancelHandle(CancelHandleArg { ty, .. }) => ty, } @@ -81,8 +83,8 @@ impl<'a> FnArg<'a> { #[allow(clippy::wrong_self_convention)] pub fn from_py_with(&self) -> Option<&FromPyWithAttribute> { - if let FnArg::Regular(RegularArg { attrs, .. }) = self { - attrs.from_py_with.as_ref() + if let FnArg::Regular(RegularArg { from_py_with, .. }) = self { + from_py_with.as_ref() } else { None } @@ -96,7 +98,7 @@ impl<'a> FnArg<'a> { .. }) = self { - *self = Self::VarArgs(VarArg { name, ty }); + *self = Self::VarArgs(VarargsArg { name, ty }); Ok(self) } else { bail_spanned!(self.name().span() => "args cannot be optional") @@ -111,7 +113,7 @@ impl<'a> FnArg<'a> { .. }) = self { - *self = Self::KwArgs(KwArg { name, ty }); + *self = Self::KwArgs(KwargsArg { name, ty }); Ok(self) } else { bail_spanned!(self.name().span() => "kwargs must be Option<_>") @@ -129,7 +131,10 @@ impl<'a> FnArg<'a> { bail_spanned!(cap.ty.span() => IMPL_TRAIT_ERR); } - let arg_attrs = PyFunctionArgPyO3Attributes::from_attrs(&mut cap.attrs)?; + let PyFunctionArgPyO3Attributes { + from_py_with, + cancel_handle, + } = PyFunctionArgPyO3Attributes::from_attrs(&mut cap.attrs)?; let ident = match &*cap.pat { syn::Pat::Ident(syn::PatIdent { ident, .. }) => ident, other => return Err(handle_argument_error(other)), @@ -142,7 +147,11 @@ impl<'a> FnArg<'a> { })); } - if arg_attrs.cancel_handle.is_some() { + if cancel_handle.is_some() { + // `PyFunctionArgPyO3Attributes::from_attrs` validates that + // only compatible attributes are specified, either + // `cancel_handle` or `from_py_with`, dublicates and any + // combination of the two are already rejected. return Ok(Self::CancelHandle(CancelHandleArg { name: ident, ty: &cap.ty, @@ -152,7 +161,7 @@ impl<'a> FnArg<'a> { Ok(Self::Regular(RegularArg { name: ident, ty: &cap.ty, - attrs: arg_attrs, + from_py_with, default_value: None, option_wrapped_type: utils::option_type_argument(&cap.ty), })) diff --git a/pyo3-macros-backend/src/params.rs b/pyo3-macros-backend/src/params.rs index f8b86339039..d9f77fa07bc 100644 --- a/pyo3-macros-backend/src/params.rs +++ b/pyo3-macros-backend/src/params.rs @@ -90,7 +90,7 @@ pub fn impl_arg_params( .iter() .enumerate() .filter_map(|(i, arg)| { - let from_py_with = &arg.from_py_with().as_ref()?.value; + let from_py_with = &arg.from_py_with()?.value; let from_py_with_holder = format_ident!("from_py_with_{}", i); Some(quote_spanned! { from_py_with.span() => let e = #pyo3_path::impl_::deprecations::GilRefs::new(); @@ -285,13 +285,7 @@ pub(crate) fn impl_regular_arg_param( )); } - if arg - .attrs - .from_py_with - .as_ref() - .map(|attr| &attr.value) - .is_some() - { + if arg.from_py_with.is_some() { if let Some(default) = default { quote_arg_span! { #pyo3_path::impl_::extract_argument::from_py_with_with_default( diff --git a/pyo3-macros-backend/src/pyclass.rs b/pyo3-macros-backend/src/pyclass.rs index 081bd497024..4c993bfca5b 100644 --- a/pyo3-macros-backend/src/pyclass.rs +++ b/pyo3-macros-backend/src/pyclass.rs @@ -1143,9 +1143,6 @@ fn complex_enum_struct_variant_new<'a>( let arg_py_type: syn::Type = parse_quote!(#pyo3_path::Python<'_>); let args = { - let mut no_pyo3_attrs = vec![]; - let attrs = crate::pyfunction::PyFunctionArgPyO3Attributes::from_attrs(&mut no_pyo3_attrs)?; - let mut args = vec![ // py: Python<'_> FnArg::Py(PyArg { @@ -1158,7 +1155,7 @@ fn complex_enum_struct_variant_new<'a>( args.push(FnArg::Regular(RegularArg { name: field.ident, ty: field.ty, - attrs: attrs.clone(), + from_py_with: None, default_value: None, option_wrapped_type: None, })); diff --git a/pyo3-macros-backend/src/pymethod.rs b/pyo3-macros-backend/src/pymethod.rs index cb55401fd30..b8beba9b6d2 100644 --- a/pyo3-macros-backend/src/pymethod.rs +++ b/pyo3-macros-backend/src/pymethod.rs @@ -609,7 +609,7 @@ pub fn impl_py_setter_def( let arg = if let FnArg::Regular(arg) = &value_arg { arg } else { - bail_spanned!(value_arg.name().span() => "The #[setter] value argument can't be *args or **kwargs"); + bail_spanned!(value_arg.name().span() => "The #[setter] value argument can't be *args, **kwargs or `cancel_handle`."); }; let tokens = impl_regular_arg_param( diff --git a/tests/ui/invalid_cancel_handle.rs b/tests/ui/invalid_cancel_handle.rs index 59076b14418..cff6c5dcbad 100644 --- a/tests/ui/invalid_cancel_handle.rs +++ b/tests/ui/invalid_cancel_handle.rs @@ -19,4 +19,10 @@ async fn cancel_handle_wrong_type(#[pyo3(cancel_handle)] _param: String) {} #[pyfunction] async fn missing_cancel_handle_attribute(_param: pyo3::coroutine::CancelHandle) {} +#[pyfunction] +async fn cancel_handle_and_from_py_with( + #[pyo3(cancel_handle, from_py_with = "cancel_handle")] _param: pyo3::coroutine::CancelHandle, +) { +} + fn main() {} diff --git a/tests/ui/invalid_cancel_handle.stderr b/tests/ui/invalid_cancel_handle.stderr index ffd0b3fd0da..41a2c0854b7 100644 --- a/tests/ui/invalid_cancel_handle.stderr +++ b/tests/ui/invalid_cancel_handle.stderr @@ -16,6 +16,12 @@ error: `cancel_handle` attribute can only be used with `async fn` 14 | fn cancel_handle_synchronous(#[pyo3(cancel_handle)] _param: String) {} | ^^^^^^ +error: `from_py_with` and `cancel_handle` cannot be specified together + --> tests/ui/invalid_cancel_handle.rs:24:12 + | +24 | #[pyo3(cancel_handle, from_py_with = "cancel_handle")] _param: pyo3::coroutine::CancelHandle, + | ^^^^^^^^^^^^^ + error[E0308]: mismatched types --> tests/ui/invalid_cancel_handle.rs:16:1 |