Refactoring of FnArg

[Icxolu]

Following the discussion from #4002 (comment)

This reworks the FnArg type in our macro codegen to contain a FnArgKind enum instead of a bunch of booleans. This is done to make much clearer which branches there are, depending on the argument kind. It also makes weird combinations (like optional py/varargs, required kwargs, ...) unrepresentable. As a consequence the error checking for those cases are now at construction/parsing side instead of usage side, which also feels more appropriate.

I left a few FIXMEs, so I leave this as a draft for now, but feedback is welcome 😄

[mejrs]

Thanks, this is great. I'm not very familiar with this part of the code base but I have some thoughts:

[mejrs]

You could write

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,
    default: Whatever,
    ty_opt: Whatever,
    ctx: &Ctx,
) -> TokenStream {
    let FnArg {
        name,
        ty,
        attrs,
        ..
    } = arg;

or you could make enum variant types:

pub struct Regular<'a> {
    pub name: &'a syn::Ident,
    pub ty: &'a syn::Type,
    pub attrs: PyFunctionArgPyO3Attributes,
    pub kind: FnArgKind<'a>,
    default: Option<syn::Expr>,
    ty_opt: Option<&'a syn::Type>,
}

pub struct VarArgs<'a> {
    pub name: &'a syn::Ident,
    pub ty: &'a syn::Type,
    pub attrs: PyFunctionArgPyO3Attributes,
    pub kind: FnArgKind<'a>,
}

pub enum FnArg<'a> {
    Regular(Regular<'a>)
    VarArgs(VarArgs<'a>),
    ...
}

[Icxolu]

Thanks for the feedback 👍, I though about using enum variant types, but initially decided against them because it might get very verbose. But maybe it is beneficial to have a separate type for each kind of argument. I think I'll give it a try and see how it turns out.

[mejrs]

maybe some kind of

enum PyOrCancelHandle{
    Py,
    CancelHandle,
}

could get rid of this unreachable 🤔 Just some food for thoughts

[Icxolu]

Hmm, since this from the general parsed types, I don't think we can do a lot to avoid it. But since this is under CallingConvention::Noargs there really shouldn't be any "real" argument's (reaching Python), except for self which is handled separately. So I think unreachable!() is appropriate here, but an explanation comment might be good, I'll add that.

[Icxolu]

I actually like a lot how the enum variant types turned out. It's a bit more boilerplate for type declaration, but it allows to actually see and specify the type system which argument types its operates on or returns.

[wyfo]

Why did you choose VarArgs(VarArg<'a>), over

    VarArgs {
        name: &'a syn::Ident,
        ty: &'a syn::Type,
    },

? I haven't seen VarArg type and the others being used in the code, except for constructor ofc, so a tuple variant could be enough, couldn't?

Also, I've compared both versions, and I prefer FnArgKind one, because I find it more natural with less boilerplate (but I'm not a reviewer, so my opinion doesn't count a lot).

[Icxolu]

Valid question 😄, opinions are always welcome

I did settle on this one, because it allows to specific argument types be handles in separate. It's true that no all of them are used currently this way, but for example impl_regular_arg_param only takes &RegularArg<'_> now. That one was also the origin of this whole refactoring process, so that the implementation could be reused for the #[setter] argument (see #3998 and #4002). That one felt a bit clunky with the FnArgKind approach, because I could not make sure it gets the correct kind at compile time. Another one is split_off_python_arg which now guarantees statically that the split off argument is indeed a PyArg (if any).

For the other variants I did it part of consistency and part to make future refactorings around argument kinds easier.

Personally I also find the current state looks nicer over the FnArgKind approach, but that's subjective I guess 😁

[davidhewitt]

Looks great to me, thanks! A couple of name suggestions and tiny edge cases, otherwise please feel free to merge 👍