Re: [PATCH v4 1/4] rust: macros: add derive macro for `Into`

["Benno Lossin" <lossin@kernel.org>]

On Fri Dec 26, 2025 at 10:36 AM CET, Jesung Yang wrote:
> On Fri, Dec 26, 2025 at 2:40 AM Benno Lossin <lossin@kernel.org> wrote:
>> On Thu Dec 25, 2025 at 9:37 AM CET, Jesung Yang via B4 Relay wrote:
>> > +fn parse_attrs(target: DeriveTarget, attrs: &[Attribute]) -> syn::Result<(Vec<ValidTy>, Ident)> {
>> > +    let helper = target.get_helper_name();
>> > +
>> > +    let mut repr_ty = None;
>> > +    let mut helper_tys = Vec::new();
>> > +    for attr in attrs {
>> > +        if attr.path().is_ident("repr") {
>> > +            attr.parse_nested_meta(|meta| {
>> > +                let ident = meta.path.get_ident();
>> > +                if ident.is_some_and(is_valid_primitive) {
>> > +                    repr_ty = ident.cloned();
>> > +                }
>>
>> While this works for now, writing `repr(C, {integer})` might become
>> meaningful in the future, see [1]. We should just accept it now as well.
>>
>> [1]: https://github.com/rust-lang/rust/issues/68585
>
> I think it's worth noting. I'll add a small comment for this.

My suggestion was to just accept `repr(C, {integer})` and make it result
in the same as `repr({integer})`.

>> > +                // Delegate `repr` attribute validation to rustc.
>> > +                Ok(())
>> > +            })?;
>> > +        } else if attr.path().is_ident(helper) && helper_tys.is_empty() {
>> > +            let args = attr.parse_args_with(Punctuated::<ValidTy, Token![,]>::parse_terminated)?;
>> > +            helper_tys.extend(args);
>> > +        }
>> > +    }
>> > +
>> > +    // Note on field-less `repr(C)` enums (quote from [1]):
>> > +    //
>> > +    //   In C, enums with discriminants that do not all fit into an `int` or all fit into an
>> > +    //   `unsigned int` are a portability hazard: such enums are only permitted since C23, and not
>> > +    //   supported e.g. by MSVC.
>> > +    //
>> > +    //   Furthermore, Rust interprets the discriminant values of `repr(C)` enums as expressions of
>> > +    //   type `isize`. This makes it impossible to implement the C23 behavior of enums where the
>> > +    //   enum discriminants have no predefined type and instead the enum uses a type large enough
>> > +    //   to hold all discriminants.
>> > +    //
>> > +    //   Therefore, `repr(C)` enums in Rust require that either all discriminants to fit into a C
>> > +    //   `int` or they all fit into an `unsigned int`.
>> > +    //
>> > +    // As such, `isize` is a reasonable representation for `repr(C)` enums, as it covers the range
>> > +    //  of both `int` and `unsigned int`.
>> > +    //
>> > +    // For more information, see:
>> > +    // - https://github.com/rust-lang/rust/issues/124403
>> > +    // - https://github.com/rust-lang/rust/pull/147017
>> > +    // - https://github.com/rust-lang/rust/blob/2ca7bcd03b87b52f7055a59b817443b0ac4a530d/compiler/rustc_lint_defs/src/builtin.rs#L5251-L5263 [1]
>> > +
>> > +    // Extract the representation passed by `#[repr(...)]` if present. If nothing is
>> > +    // specified, the default is `Rust` representation, which uses `isize` for its
>> > +    // discriminant type.
>> > +    // See: https://doc.rust-lang.org/reference/items/enumerations.html#r-items.enum.discriminant.repr-rust
>>
>> I think we should error when no `#[repr({integer})]` attribute is
>> specified.
>
> Not a blocker but just out of curiosity: are you concerned that the
> default size might change in the future, leading to silent side
> effects?

`isize` already changes size when you switch between 64 and 32 bit
architectures. I think the author of an enum they want to convert into
integers should think about which size the enum should be.

They already can choose `repr(isize)` if that is correct in that case.
As a default, I would have choosen `i32` (but that conflicts with Rust's
default, so we can't do it).

>> > +    let repr_ty = repr_ty.unwrap_or_else(|| Ident::new("isize", Span::call_site()));
>> > +    Ok((helper_tys, repr_ty))
>> > +}
>> > +
>> > +fn derive_for_enum(
>> > +    target: DeriveTarget,
>> > +    enum_ident: &Ident,
>> > +    variants: &[Ident],
>> > +    helper_tys: &[ValidTy],
>> > +    repr_ty: &Ident,
>> > +) -> TokenStream {
>> > +    let impl_fn = match target {
>> > +        DeriveTarget::Into => impl_into,
>> > +    };
>> > +
>> > +    let qualified_repr_ty: syn::Path = parse_quote! { ::core::primitive::#repr_ty };
>> > +
>> > +    return if helper_tys.is_empty() {
>> > +        let ty = ValidTy::Primitive(repr_ty.clone());
>> > +        let impls =
>> > +            std::iter::once(ty).map(|ty| impl_fn(enum_ident, variants, &qualified_repr_ty, &ty));
>> > +        ::quote::quote! { #(#impls)* }
>> > +    } else {
>> > +        let impls = helper_tys
>> > +            .iter()
>> > +            .map(|ty| impl_fn(enum_ident, variants, &qualified_repr_ty, ty));
>> > +        ::quote::quote! { #(#impls)* }
>> > +    };
>>
>> Let's just do this when we still have the `helper_tys` vector:
>>
>>     helper_tys.push(ValidTy::Primitive(repr_ty));
>
> The current implementation completely ignores what's in `#[repr(...)]`
> when the `#[into(...)]` or `#[try_from(...)]` helper attributes are
> specified. But users might expect, as you did, the macros to generate
> impls not only for types specified in the helper attributes but also
> for the one in `repr`. I think I should deduplicate `helper_tys` after
> the push, but anyway, I'll change this in v5.

Oh yeah I missed this, please do make this change :)

>> > +
>> > +    fn impl_into(
>> > +        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 cast = match input_ty {
>> > +            ValidTy::Bounded(inner) => {
>> > +                let base_ty = inner.emit_qualified_base_ty();
>> > +                let expr = parse_quote! { #param as #base_ty };
>> > +                // Since the discriminant of `#param`, an enum variant, is determined
>> > +                // at compile-time, we can rely on `Bounded::from_expr()`. It requires
>> > +                // the provided expression to be verifiable at compile-time to avoid
>> > +                // triggering a build error.
>> > +                inner.emit_from_expr(&expr)
>> > +            }
>> > +            ValidTy::Primitive(ident) if ident == "bool" => {
>> > +                ::quote::quote! { (#param as #repr_ty) == 1 }
>> > +            }
>> > +            qualified @ ValidTy::Primitive(_) => ::quote::quote! { #param as #qualified },
>> > +        };
>> > +
>> > +        ::quote::quote! {
>> > +            #[automatically_derived]
>> > +            impl ::core::convert::From<#enum_ident> for #input_ty {
>> > +                fn from(#param: #enum_ident) -> #input_ty {
>> > +                    #overflow_assertion
>> > +
>> > +                    #cast
>> > +                }
>> > +            }
>> > +        }
>> > +    }
>> > +
>> > +    fn emit_overflow_assert(
>> > +        enum_ident: &Ident,
>> > +        variants: &[Ident],
>> > +        repr_ty: &syn::Path,
>> > +        input_ty: &ValidTy,
>> > +    ) -> TokenStream {
>>
>> I feel like we should track this via traits rather than using a const
>> assert. That approach will require & generate much less code.
>
> Sorry, but could you elaborate? A small example of what you have in
> mind would help a lot.

Oh yeah sorry, I had something different in mind compared to what I'll
describe now, but it achieves the same thing without introducing new
traits:

We have two options:
1) We use `<input_ty as TryFrom<repr_ty>>::try_from` instead of writing
   the `fits` function ourself.
2) We require `input_ty: From<repr_ty>`.

The first option would still check every variant and should behave the
same as your current code.

Option 2 allows us to avoid the const altogether, but requires us to
choose the smallest integer as the representation (and if we want to be
able to use both `i8` and `u8`, we can't). I missed this before, so
using option 1 might be the only way to allow conversions of this kind.

>> > +        let qualified_i128: syn::Path = parse_quote! { ::core::primitive::i128 };
>> > +        let qualified_u128: syn::Path = parse_quote! { ::core::primitive::u128 };
>> > +
>> > +        let input_min = input_ty.emit_min();
>> > +        let input_max = input_ty.emit_max();
>> > +
>> > +        let variant_fits = variants.iter().map(|variant| {
>> > +            let msg = format!(
>> > +                "enum discriminant overflow: \
>> > +                `{enum_ident}::{variant}` does not fit in `{input_ty}`",
>> > +            );
>> > +            ::quote::quote! {
>> > +                ::core::assert!(fits(#enum_ident::#variant as #repr_ty), #msg);
>> > +            }
>> > +        });
>> > +
>> > +        ::quote::quote! {
>> > +            const _: () = {
>> > +                const fn fits(d: #repr_ty) -> ::core::primitive::bool {
>> > +                    // For every integer type, its minimum value always fits in `i128`.
>> > +                    let dst_min = #input_min;
>> > +                    // For every integer type, its maximum value always fits in `u128`.
>> > +                    let dst_max = #input_max;
>> > +
>> > +                    #[allow(unused_comparisons)]
>> > +                    let is_src_signed = #repr_ty::MIN < 0;
>> > +                    #[allow(unused_comparisons)]
>> > +                    let is_dst_signed = dst_min < 0;
>> > +
>> > +                    if is_src_signed && is_dst_signed {
>> > +                        // Casting from a signed value to `i128` does not overflow since
>> > +                        // `i128` is the largest signed primitive integer type.
>> > +                        (d as #qualified_i128) >= (dst_min as #qualified_i128)
>> > +                            && (d as #qualified_i128) <= (dst_max as #qualified_i128)
>> > +                    } else if is_src_signed && !is_dst_signed {
>> > +                        // Casting from a signed value greater than 0 to `u128` does not
>> > +                        // overflow since `u128::MAX` is greater than `i128::MAX`.
>> > +                        d >= 0 && (d as #qualified_u128) <= (dst_max as #qualified_u128)
>> > +                    } else {
>> > +                        // Casting from an unsigned value to `u128` does not overflow since
>> > +                        // `u128` is the largest unsigned primitive integer type.
>> > +                        (d as #qualified_u128) <= (dst_max as #qualified_u128)
>> > +                    }
>> > +                }
>> > +
>> > +                #(#variant_fits)*
>> > +            };
>> > +        }
>> > +    }
>> > +}
>>
>> > +
>> > +impl fmt::Display for ValidTy {
>> > +    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
>> > +        match self {
>> > +            Self::Bounded(inner) => inner.fmt(f),
>> > +            Self::Primitive(ident) => ident.fmt(f),
>> > +        }
>> > +    }
>> > +}
>> > +
>> > +struct Bounded {
>> > +    name: Ident,
>> > +    open_angle: Token![<],
>> > +    base_ty: Ident,
>>
>> We should allow a type here from syntax and then emit an error when it
>> isn't a primitive.
>
> Assuming you're talking about `syn::Type`, that seems better. I think I
> should do the same thing for `ValidTy::Primitive`.

Sounds good!

>> > +    comma: Token![,],
>> > +    bits: LitInt,
>> > +    close_angle: Token![>],
>> > +}
>>
>> > diff --git a/rust/macros/lib.rs b/rust/macros/lib.rs
>> > index b38002151871a33f6b4efea70be2deb6ddad38e2..02528d7212b75d28788f0c33479edb272fa12e27 100644
>> > --- a/rust/macros/lib.rs
>> > +++ b/rust/macros/lib.rs
>> > @@ -14,6 +14,7 @@
>> >  #[macro_use]
>> >  mod quote;
>> >  mod concat_idents;
>> > +mod convert;
>> >  mod export;
>> >  mod fmt;
>> >  mod helpers;
>> > @@ -23,6 +24,10 @@
>> >  mod vtable;
>> >
>> >  use proc_macro::TokenStream;
>> > +use syn::{
>> > +    parse_macro_input,
>> > +    DeriveInput, //
>> > +};
>> >
>> >  /// Declares a kernel module.
>> >  ///
>> > @@ -475,3 +480,155 @@ pub fn paste(input: TokenStream) -> TokenStream {
>> >  pub fn kunit_tests(attr: TokenStream, ts: TokenStream) -> TokenStream {
>> >      kunit::kunit_tests(attr, ts)
>> >  }
>> > +
>> > +/// A derive macro for providing an implementation of the [`Into`] trait.
>> > +///
>> > +/// This macro automatically derives the [`Into`] trait for a given enum by generating
>> > +/// the relevant [`From`] implementation. Currently, it only supports [unit-only enum]s.
>> > +///
>> > +/// [unit-only enum]: https://doc.rust-lang.org/reference/items/enumerations.html#r-items.enum.unit-only
>> > +///
>> > +/// # Notes
>> > +///
>> > +/// - Unlike its name suggests, the macro actually generates [`From`] implementations
>> > +///   which automatically provide corresponding [`Into`] implementations.
>> > +///
>> > +/// - The macro uses the `into` custom attribute or `repr` attribute to generate [`From`]
>> > +///   implementations. `into` always takes precedence over `repr`.
>>
>> What do you mean by "precedence" here? You always generate it for all
>> helper_tys and the repr?
>
> To quote my reply above:
>
>     The current implementation completely ignores what's in
>     `#[repr(...)]` when the `#[into(...)]` or `#[try_from(...)]` helper
>     attributes are specified.
>
> That's why I used the term "precedence." But as I said, I'll change the
> logic to generate impls for both.

Oh yeah now it makes sense :)

> Thanks a lot for the time and effort you put into reviewing it!

My pleasure :)

Cheers,
Benno