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

["Alexandre Courbot" <acourbot@nvidia.com>]

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.

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/