Refactoring of `FnArg` (#4033)

* refactor `FnArg`

* add UI tests

* use enum variant types

* add comment

* remove dead code

* remove last FIXME

* review feedback davidhewitt

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,19 +17,109 @@ use crate::{
 };
 
 #[derive(Clone, Debug)]
-pub struct FnArg<'a> {
+pub struct RegularArg<'a> {
     pub name: &'a syn::Ident,
     pub ty: &'a syn::Type,
-    pub optional: Option<&'a syn::Type>,
+    pub from_py_with: Option<FromPyWithAttribute>,
-    pub default: Option<syn::Expr>,
+    pub default_value: Option<syn::Expr>,
-    pub py: bool,
+    pub option_wrapped_type: Option<&'a syn::Type>,
-    pub attrs: PyFunctionArgPyO3Attributes,
+}
-    pub is_varargs: bool,
+
-    pub is_kwargs: bool,
+/// Pythons *args argument
-    pub is_cancel_handle: bool,
+#[derive(Clone, Debug)]
+pub struct VarargsArg<'a> {
+    pub name: &'a syn::Ident,
+    pub ty: &'a syn::Type,
+}
+
+/// Pythons **kwarg argument
+#[derive(Clone, Debug)]
+pub struct KwargsArg<'a> {
+    pub name: &'a syn::Ident,
+    pub ty: &'a syn::Type,
+}
+
+#[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(VarargsArg<'a>),
+    KwArgs(KwargsArg<'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(VarargsArg { name, .. }) => name,
+            FnArg::KwArgs(KwargsArg { 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(VarargsArg { ty, .. }) => ty,
+            FnArg::KwArgs(KwargsArg { 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 { from_py_with, .. }) = self {
+            from_py_with.as_ref()
+        } else {
+            None
+        }
+    }
+
+    pub fn to_varargs_mut(&mut self) -> Result<&mut Self> {
+        if let Self::Regular(RegularArg {
+            name,
+            ty,
+            option_wrapped_type: None,
+            ..
+        }) = self
+        {
+            *self = Self::VarArgs(VarargsArg { name, ty });
+            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,
+            option_wrapped_type: Some(..),
+            ..
+        }) = self
+        {
+            *self = Self::KwArgs(KwargsArg { name, ty });
+            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<Self> {
         match arg {
@@ -41,32 +131,43 @@ 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)),
                 };
 
-                let is_cancel_handle = arg_attrs.cancel_handle.is_some();
+                if utils::is_python(&cap.ty) {
+                    return Ok(Self::Py(PyArg {
+                        name: ident,
+                        ty: &cap.ty,
+                    }));
+                }
 
-                Ok(FnArg {
+                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,
+                    }));
+                }
+
+                Ok(Self::Regular(RegularArg {
                     name: ident,
                     ty: &cap.ty,
-                    optional: utils::option_type_argument(&cap.ty),
+                    from_py_with,
-                    default: None,
+                    default_value: None,
-                    py: utils::is_python(&cap.ty),
+                    option_wrapped_type: utils::option_type_argument(&cap.ty),
-                    attrs: arg_attrs,
+                }))
-                    is_varargs: false,
-                    is_kwargs: false,
-                    is_cancel_handle,
-                })
             }
         }
     }
-
-    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 +593,14 @@ impl<'a> FnSpec<'a> {
             .signature
             .arguments
             .iter()
-            .filter(|arg| arg.is_cancel_handle);
+            .filter(|arg| matches!(arg, FnArg::CancelHandle(..)));
         let cancel_handle = cancel_handle_iter.next();
-        if let Some(arg) = cancel_handle {
+        if let Some(FnArg::CancelHandle(CancelHandleArg { name, .. })) = cancel_handle {
-            ensure_spanned!(self.asyncness.is_some(), arg.name.span() => "`cancel_handle` attribute can only be used with `async fn`");
+            ensure_spanned!(self.asyncness.is_some(), name.span() => "`cancel_handle` attribute can only be used with `async fn`");
-            if let Some(arg2) = cancel_handle_iter.next() {
+            if let Some(FnArg::CancelHandle(CancelHandleArg { name, .. })) =
-                bail_spanned!(arg2.name.span() => "`cancel_handle` may only be specified once");
+                cancel_handle_iter.next()
+            {
+                bail_spanned!(name.span() => "`cancel_handle` may only be specified once");
             }
         }
 
@@ -605,14 +708,10 @@ impl<'a> FnSpec<'a> {
                     .signature
                     .arguments
                     .iter()
-                    .map(|arg| {
+                    .map(|arg| match arg {
-                        if arg.py {
+                        FnArg::Py(..) => quote!(py),
-                            quote!(py)
+                        FnArg::CancelHandle(..) => quote!(__cancel_handle),
-                        } else if arg.is_cancel_handle {
+                        _ => unreachable!("`CallingConvention::Noargs` should not contain any arguments (reaching Python) except for `self`, which is handled below."),
-                            quote!(__cancel_handle)
-                        } else {
-                            unreachable!()
-                        }
                     })
                     .collect();
                 let call = rust_call(args, &mut holders);
@@ -635,7 +734,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();
@@ -660,7 +759,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();
@@ -684,7 +783,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);

pyo3-macros-backend/src/params.rs

@@ -1,13 +1,12 @@
 use crate::utils::Ctx;
 use crate::{
-    method::{FnArg, 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;
-use syn::Result;
 
 pub struct Holders {
     holders: Vec<syn::Ident>,
@@ -60,16 +59,7 @@ impl Holders {
 pub fn is_forwarded_args(signature: &FunctionSignature<'_>) -> bool {
     matches!(
         signature.arguments.as_slice(),
-        [
+        [FnArg::VarArgs(..), FnArg::KwArgs(..),]
-            FnArg {
-                is_varargs: true,
-                ..
-            },
-            FnArg {
-                is_kwargs: true,
-                ..
-            },
-        ]
     )
 }
 
@@ -90,7 +80,7 @@ pub fn impl_arg_params(
     fastcall: bool,
     holders: &mut Holders,
     ctx: &Ctx,
-) -> Result<(TokenStream, Vec<TokenStream>)> {
+) -> (TokenStream, Vec<TokenStream>) {
     let args_array = syn::Ident::new("output", Span::call_site());
     let Ctx { pyo3_path } = ctx;
 
@@ -100,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 = &arg.from_py_with()?.value;
-            let from_py_with_holder =
+            let from_py_with_holder = format_ident!("from_py_with_{}", i);
-                syn::Ident::new(&format!("from_py_with_{}", i), Span::call_site());
             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);
@@ -119,28 +108,16 @@ pub fn impl_arg_params(
             .arguments
             .iter()
             .enumerate()
-            .map(|(i, arg)| {
+            .map(|(i, arg)| impl_arg_param(arg, i, &mut 0, holders, ctx))
-                let from_py_with =
+            .collect();
-                    syn::Ident::new(&format!("from_py_with_{}", i), Span::call_site());
+        return (
-                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::<Result<_>>()?;
-        return Ok((
             quote! {
                 let _args = #pyo3_path::impl_::pymethods::BoundRef::ref_from_ptr(py, &_args);
                 let _kwargs = #pyo3_path::impl_::pymethods::BoundRef::ref_from_ptr_or_opt(py, &_kwargs);
                 #from_py_with
             },
             arg_convert,
-        ));
+        );
     };
 
     let positional_parameter_names = &spec.signature.python_signature.positional_parameters;
@@ -171,18 +148,8 @@ pub fn impl_arg_params(
         .arguments
         .iter()
         .enumerate()
-        .map(|(i, arg)| {
+        .map(|(i, arg)| impl_arg_param(arg, i, &mut option_pos, holders, ctx))
-            let from_py_with = syn::Ident::new(&format!("from_py_with_{}", i), Span::call_site());
+        .collect();
-            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::<Result<_>>()?;
 
     let args_handler = if spec.signature.python_signature.varargs.is_some() {
         quote! { #pyo3_path::impl_::extract_argument::TupleVarargs }
@@ -224,7 +191,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,18 +206,64 @@ 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 {
+        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)
+        }
+        FnArg::VarArgs(arg) => {
+            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
+                )?
+            }
+        }
+        FnArg::KwArgs(arg) => {
+            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
+                )?
+            }
+        }
+        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_arg_param(
+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,
-) -> Result<TokenStream> {
+) -> TokenStream {
     let Ctx { pyo3_path } = ctx;
     let pyo3_path = pyo3_path.to_tokens_spanned(arg.ty.span());
 
@@ -260,64 +273,19 @@ pub(crate) fn impl_arg_param(
         ($($tokens:tt)*) => { quote_spanned!(arg.ty.span() => $($tokens)*) }
     }
 
-    if arg.py {
+    let name_str = arg.name.to_string();
-        return Ok(quote! { py });
+    let mut default = arg.default_value.as_ref().map(|expr| quote!(#expr));
-    }
-
-    if arg.is_cancel_handle {
-        return Ok(quote! { __cancel_handle });
-    }
-
-    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));
 
     // Option<T> 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 arg.option_wrapped_type.is_some() {
         default = Some(default.map_or_else(
             || quote!(::std::option::Option::None),
             |tokens| some_wrap(tokens, ctx),
         ));
     }
 
-    let tokens = if arg
+    if arg.from_py_with.is_some() {
-        .attrs
-        .from_py_with
-        .as_ref()
-        .map(|attr| &attr.value)
-        .is_some()
-    {
         if let Some(default) = default {
             quote_arg_span! {
                 #pyo3_path::impl_::extract_argument::from_py_with_with_default(
@@ -339,7 +307,7 @@ pub(crate) fn impl_arg_param(
                 )?
             }
         }
-    } else if arg.optional.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(
@@ -374,7 +342,5 @@ pub(crate) fn impl_arg_param(
                 #name_str
             )?
         }
-    };
+    }
-
-    Ok(tokens)
 }

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, FnSpec, PyArg, RegularArg};
 use crate::pyimpl::{gen_py_const, PyClassMethodsType};
 use crate::pymethod::{
     impl_py_getter_def, impl_py_setter_def, MethodAndMethodDef, MethodAndSlotDef, PropertyType,
@@ -1143,36 +1143,22 @@ 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 {
+            FnArg::Py(PyArg {
                 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,
-            },
         ];
 
         for field in &variant.fields {
-            args.push(FnArg {
+            args.push(FnArg::Regular(RegularArg {
                 name: field.ident,
                 ty: field.ty,
-                optional: None,
+                from_py_with: None,
-                default: None,
+                default_value: None,
-                py: false,
+                option_wrapped_type: None,
-                attrs: attrs.clone(),
+            }));
-                is_varargs: false,
-                is_kwargs: false,
-                is_cancel_handle: false,
-            });
         }
         args
     };

pyo3-macros-backend/src/pyfunction/signature.rs

@@ -10,7 +10,7 @@ use syn::{
 
 use crate::{
     attributes::{kw, KeywordAttribute},
-    method::FnArg,
+    method::{FnArg, RegularArg},
 };
 
 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 {
+                match fn_arg {
-                    // If the user incorrectly tried to include py: Python in the
+                    crate::method::FnArg::Py(..) => {
-                    // signature, give a useful error as a hint.
+                        // If the user incorrectly tried to include py: Python in the
-                    ensure_spanned!(
+                        // signature, give a useful error as a hint.
-                        name != fn_arg.name,
+                        ensure_spanned!(
-                        name.span() => "arguments of type `Python` must not be part of the signature"
+                            name != fn_arg.name(),
-                    );
+                            name.span() => "arguments of type `Python` must not be part of the signature"
-                    // Otherwise try next argument.
+                        );
-                    continue;
+                        // Otherwise try next argument.
+                        continue;
+                    }
+                    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.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);
+                    }
                 }
-                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;
-                }
-
-                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"
@@ -398,7 +401,15 @@ impl<'a> FunctionSignature<'a> {
                         arg.span(),
                     )?;
                     if let Some((_, default)) = &arg.eq_and_default {
-                        fn_arg.default = Some(default.clone());
+                        if let FnArg::Regular(arg) = fn_arg {
+                            arg.default_value = Some(default.clone());
+                        } else {
+                            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."
+                            );
+                        }
                     }
                 }
                 SignatureItem::VarargsSep(sep) => {
@@ -406,12 +417,12 @@ impl<'a> FunctionSignature<'a> {
                 }
                 SignatureItem::Varargs(varargs) => {
                     let fn_arg = next_non_py_argument_checked(&varargs.ident)?;
-                    fn_arg.is_varargs = true;
+                    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)?;
-                    fn_arg.is_kwargs = true;
+                    fn_arg.to_kwargs_mut()?;
                     parse_state.add_kwargs(&mut python_signature, kwargs)?;
                 }
                 SignatureItem::PosargsSep(sep) => {
@@ -421,9 +432,11 @@ 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, 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())
             );
         }
 
@@ -439,15 +452,20 @@ 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, FnArg::Py(..) | FnArg::CancelHandle(..)) {
                 continue;
             }
 
-            if arg.optional.is_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"
                 );
 
@@ -457,7 +475,7 @@ impl<'a> FunctionSignature<'a> {
 
             python_signature
                 .positional_parameters
-                .push(arg.name.unraw().to_string());
+                .push(arg.name().unraw().to_string());
         }
 
         Ok(Self {
@@ -469,8 +487,12 @@ 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(fn_arg) = self.arguments.iter().find(|arg| arg.name() == parameter) {
-            if let Some(arg_default) = fn_arg.default.as_ref() {
+            if let FnArg::Regular(RegularArg {
+                default_value: Some(arg_default),
+                ..
+            }) = fn_arg
+            {
                 match arg_default {
                     // literal values
                     syn::Expr::Lit(syn::ExprLit { lit, .. }) => match lit {
@@ -496,7 +518,11 @@ impl<'a> FunctionSignature<'a> {
                     // others, unsupported yet so defaults to `...`
                     _ => {}
                 }
-            } else if fn_arg.optional.is_some() {
+            } else if let FnArg::Regular(RegularArg {
+                option_wrapped_type: Some(..),
+                ..
+            }) = fn_arg
+            {
                 // functions without a `#[pyo3(signature = (...))]` option
                 // will treat trailing `Option<T>` arguments as having a default of `None`
                 default = "None".to_string();

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::method::{CallingConvention, ExtractErrorMode, PyArg};
-use crate::params::{check_arg_for_gil_refs, impl_arg_param, Holders};
+use crate::params::{check_arg_for_gil_refs, impl_regular_arg_param, Holders};
 use crate::utils::Ctx;
 use crate::utils::PythonDoc;
 use crate::{
@@ -487,7 +487,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;
@@ -537,7 +537,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)"
         );
     }
@@ -607,7 +607,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());
                 (
@@ -622,20 +622,21 @@ pub fn impl_py_setter_def(
                 (quote!(), syn::Ident::new("dummy", Span::call_site()))
             };
 
-            let extract = impl_arg_param(
+            let arg = if let FnArg::Regular(arg) = &value_arg {
-                &args[0],
+                arg
+            } else {
+                bail_spanned!(value_arg.name().span() => "The #[setter] value argument can't be *args, **kwargs or `cancel_handle`.");
+            };
+
+            let tokens = impl_regular_arg_param(
+                arg,
                 ident,
                 quote!(::std::option::Option::Some(_value.into())),
                 &mut holders,
                 ctx,
-            )
+            );
-            .map(|tokens| {
+            let extract =
-                check_arg_for_gil_refs(
+                check_arg_for_gil_refs(tokens, holders.push_gil_refs_checker(arg.ty.span()), ctx);
-                    tokens,
-                    holders.push_gil_refs_checker(value_arg.ty.span()),
-                    ctx,
-                )
-            })?;
             quote! {
                 #from_py_with
                 let _val = #extract;
@@ -721,7 +722,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;
@@ -843,9 +844,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),
     }
 }
@@ -1052,14 +1053,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(
@@ -1073,7 +1074,7 @@ impl Ty {
                         #ident
                     }
                 },
-                arg.ty.span(),
+                arg.ty().span(),
                 ctx
             ),
             Ty::NonNullObject => extract_object(
@@ -1081,7 +1082,7 @@ impl Ty {
                 holders,
                 &name_str,
                 quote! { #ident.as_ptr() },
-                arg.ty.span(),
+                arg.ty().span(),
                 ctx
             ),
             Ty::IPowModulo => extract_object(
@@ -1089,7 +1090,7 @@ impl Ty {
                 holders,
                 &name_str,
                 quote! { #ident.as_ptr() },
-                arg.ty.span(),
+                arg.ty().span(),
                 ctx
             ),
             Ty::CompareOp => extract_error_mode.handle_error(
@@ -1100,7 +1101,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()))
@@ -1523,12 +1524,12 @@ fn extract_proto_arguments(
     let mut non_python_args = 0;
 
     for arg in &spec.signature.arguments {
-        if arg.py {
+        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);

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() {}

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
    |

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<T>(value: T) {}
@@ -16,6 +16,14 @@ fn destructured_argument((a, b): (i32, i32)) {}
 #[pyfunction]
 fn function_with_required_after_option(_opt: Option<i32>, _x: i32) {}
 
+#[pyfunction]
+#[pyo3(signature=(*args))]
+fn function_with_optional_args(args: Option<Bound<'_, PyTuple>>) {}
+
+#[pyfunction]
+#[pyo3(signature=(**kwargs))]
+fn function_with_required_kwargs(kwargs: Bound<'_, PyDict>) {}
+
 #[pyfunction(pass_module)]
 fn pass_module_but_no_arguments<'py>() {}
 

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<i32>, _x: i32) {}
    |                                                               ^^^
 
-error: expected `&PyModule` or `Py<PyModule>` as first argument with `pass_module`
+error: args cannot be optional
-  --> tests/ui/invalid_pyfunctions.rs:20:37
+  --> tests/ui/invalid_pyfunctions.rs:21:32
    |
-20 | fn pass_module_but_no_arguments<'py>() {}
+21 | fn function_with_optional_args(args: Option<Bound<'_, PyTuple>>) {}
+   |                                ^^^^
+
+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<PyModule>` as first argument with `pass_module`
+  --> tests/ui/invalid_pyfunctions.rs:28:37
+   |
+28 | fn pass_module_but_no_arguments<'py>() {}
    |                                     ^^
 
 error[E0277]: the trait bound `&str: From<BoundRef<'_, '_, pyo3::prelude::PyModule>>` 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<BoundRef<'_, '_, pyo3::prelude::PyModule>>` is not implemented for `&str`, which is required by `BoundRef<'_, '_, pyo3::prelude::PyModule>: Into<_>`
    |
    = help: the following other types implement trait `From<T>`: