"Alexandre Courbot" <[email protected]> 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 <[email protected]> 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/
