Re: [PATCH v5 2/4] rust: macros: add derive macro for `TryFrom`

[Charalampos Mitrodimas <charmitro@posteo.net>]

"Alexandre Courbot" <acourbot@nvidia.com> writes:

> On Fri Feb 6, 2026 at 6:06 AM JST, Jesung Yang wrote:
>> On Wed Feb 4, 2026 at 10:39 AM KST, Charalampos Mitrodimas wrote:
>>> Jesung Yang via B4 Relay <devnull+y.j3ms.n.gmail.com@kernel.org> writes:
>> [...]
>>>> +    fn impl_try_from(
>>>> +        enum_ident: &Ident,
>>>> +        variants: &[Ident],
>>>> +        repr_ty: &syn::Path,
>>>> +        input_ty: &ValidTy,
>>>> +    ) -> TokenStream {
>>>> +        let param = Ident::new("value", Span::call_site());
>>>> +
>>>> +        let overflow_assertion = emit_overflow_assert(enum_ident, variants, repr_ty, input_ty);
>>>> +        let emit_cast = |variant| {
>>>> +            let variant = ::quote::quote! { #enum_ident::#variant };
>>>> +            match input_ty {
>>>> +                ValidTy::Bounded(inner) => {
>>>> +                    let base_ty = inner.emit_qualified_base_ty();
>>>> +                    let expr = parse_quote! { #variant as #base_ty };
>>>> +                    inner.emit_new(&expr)
>>>> +                }
>>>> +                ValidTy::Primitive(ident) if ident == "bool" => {
>>>> +                    ::quote::quote! { ((#variant as #repr_ty) == 1) }
>>>> +                }
>>>> +                qualified @ ValidTy::Primitive(_) => ::quote::quote! { #variant as #qualified },
>>>> +            }
>>>> +        };
>>>> +
>>>> +        let clauses = variants.iter().map(|variant| {
>>>> +            let cast = emit_cast(variant);
>>>> +            ::quote::quote! {
>>>> +                if #param == #cast {
>>>> +                    ::core::result::Result::Ok(#enum_ident::#variant)
>>>> +                } else
>>>> +            }
>>>> +        });
>>>> +
>>>> +        ::quote::quote! {
>>>> +            #[automatically_derived]
>>>> +            impl ::core::convert::TryFrom<#input_ty> for #enum_ident {
>>>> +                type Error = ::kernel::prelude::Error;
>>>> +                fn try_from(#param: #input_ty) -> Result<#enum_ident, Self::Error> {
>>>> +                    #overflow_assertion
>>>> +
>>>> +                    #(#clauses)* {
>>>> +                        ::core::result::Result::Err(::kernel::prelude::EINVAL)
>>>
>>> What happens if we need a different error type here? For example, a
>>> quick look around in nova-core's "Chipset" enum, an unrecognized chipset
>>> ID warrants ENODEV rather than EINVAL, since it's about device
>>> identification.
>>>
>>> Not sure if it fits the design, just wondering if this flexibility would
>>> be useful, but would something like an optional
>>>
>>>     error = <ERROR>
>>>
>>> in the
>>>
>>>    #[try_from(...)]
>>>
>>> attribute make sense? e.g.
>>>
>>>    #[try_from(u32, error = ENODEV)]
>>>
>>> defaulting ofcourse to EINVAL if unspecified.
>>
>> I believe this is indeed a desired change.
>>
>> Back in September, an RFC [1] using the same API (i.e., without error
>> customization) was sent; I took a quick look at the time and felt
>> everything was OK, but in hindsight, the need for this flexibility is
>> clear.
>>
>> Your proposed API looks good to me. Unless there are objections, I'll
>> move forward with this approach.
>
> One problem I can see is that ultimately the error depends on the
> context of the call, not the type itself.
>
> Nova-core's `Chipset` returning `ENODEV` is a bit too opportunistic to
> me - the only place where we are doing the conversion is within probe,
> and that's the error that probe is expected to return. But in another
> context (say, validating some user input), `EINVAL` could be the right
> error to return.
>
> There is technically only one reason for the derived `TryFrom`
> implementations to fail, and that's because the passed value doesn't
> exist in the enum. So really what we would ideally want here is a
> conversion method returning an `Option`, like enumn's `n` [1], that we
> `ok_or` into the correct error for the context.

Good one yes. This is a cleaner approach. Decoupling the error from the
type and letting the caller decode via ok_or looks better to me.

>
> But short of that, I guess we could also have a dedicated, single-value
> error type for derived `TryFrom` implementations that we `map_err`. That
> type could even have an `Into<Error>` implementation that converts it to
> `EINVAL` by default, as that's going to be the most common case.
>
> ... but if we do that, that's not very different from returning `EINVAL`
> and having callers `map_err` on that when they need it.
>
> [1] https://docs.rs/enumn/latest/enumn/