Message-Id: <DF7HDE1T2BOS.33WUHP49WWO1M@kernel.org>
Date: Thu, 25 Dec 2025 18:40:54 +0100
From: "Benno Lossin" <lossin@...nel.org>
To: <y.j3ms.n@...il.com>, "Miguel Ojeda" <ojeda@...nel.org>, "Boqun Feng"
 <boqun.feng@...il.com>, "Gary Guo" <gary@...yguo.net>,
 Björn Roy Baron <bjorn3_gh@...tonmail.com>, "Andreas
 Hindborg" <a.hindborg@...nel.org>, "Alice Ryhl" <aliceryhl@...gle.com>,
 "Trevor Gross" <tmgross@...ch.edu>, "Danilo Krummrich" <dakr@...nel.org>,
 "Alexandre Courbot" <acourbot@...dia.com>
Cc: <linux-kernel@...r.kernel.org>, <rust-for-linux@...r.kernel.org>,
 <nouveau@...ts.freedesktop.org>
Subject: Re: [PATCH v4 1/4] rust: macros: add derive macro for `Into`

On Thu Dec 25, 2025 at 9:37 AM CET, Jesung Yang via B4 Relay wrote:
> +fn derive(target: DeriveTarget, input: DeriveInput) -> syn::Result<TokenStream> {
> +    let mut errors: Option<syn::Error> = None;
> +    let mut combine_error = |err| match errors.as_mut() {
> +        Some(errors) => errors.combine(err),
> +        None => errors = Some(err),
> +    };
> +
> +    let (helper_tys, repr_ty) = parse_attrs(target, &input.attrs)?;
> +    for ty in &helper_tys {
> +        if let Err(err) = ty.validate() {
> +            combine_error(err);
> +        }
> +    }
> +
> +    let data_enum = match input.data {
> +        Data::Enum(data) => data,
> +        Data::Struct(data) => {
> +            let msg = format!(
> +                "expected `enum`, found `struct`; \
> +                 `#[derive({})]` can only be applied to a unit-only enum",
> +                target.get_trait_name()
> +            );
> +            return Err(syn::Error::new(data.struct_token.span(), msg));
> +        }
> +        Data::Union(data) => {
> +            let msg = format!(
> +                "expected `enum`, found `union`; \
> +                 `#[derive({})]` can only be applied to a unit-only enum",
> +                target.get_trait_name()
> +            );
> +            return Err(syn::Error::new(data.union_token.span(), msg));
> +        }
> +    };
> +
> +    for variant in &data_enum.variants {
> +        match &variant.fields {
> +            Fields::Named(fields) => {
> +                let msg = format!(
> +                    "expected unit-like variant, found struct-like variant; \
> +                    `#[derive({})]` can only be applied to a unit-only enum",
> +                    target.get_trait_name()
> +                );
> +                combine_error(syn::Error::new_spanned(fields, msg));
> +            }
> +            Fields::Unnamed(fields) => {
> +                let msg = format!(
> +                    "expected unit-like variant, found tuple-like variant; \
> +                    `#[derive({})]` can only be applied to a unit-only enum",
> +                    target.get_trait_name()
> +                );
> +                combine_error(syn::Error::new_spanned(fields, msg));
> +            }
> +            _ => (),

We should be exhaustive here to exclude any future additions (ie break
the build if a new `Fields::...` variant is added).

> +        }
> +    }
> +
> +    if let Some(errors) = errors {
> +        return Err(errors);
> +    }
> +
> +    let variants: Vec<_> = data_enum
> +        .variants
> +        .into_iter()
> +        .map(|variant| variant.ident)
> +        .collect();
> +
> +    Ok(derive_for_enum(
> +        target,
> +        &input.ident,
> +        &variants,
> +        &helper_tys,
> +        &repr_ty,
> +    ))
> +}
> +
> +#[derive(Clone, Copy, Debug)]
> +enum DeriveTarget {
> +    Into,
> +}
> +
> +impl DeriveTarget {
> +    fn get_trait_name(&self) -> &'static str {
> +        match self {
> +            Self::Into => "Into",
> +        }
> +    }
> +
> +    fn get_helper_name(&self) -> &'static str {
> +        match self {
> +            Self::Into => "into",
> +        }
> +    }
> +}
> +
> +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

> +                // 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.

> +    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));

> +
> +    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.

> +        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.

> +    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?

Cheers,
Benno

> +///
> +/// - The macro generates a compile-time assertion for every variant to ensure its
> +///   discriminant value fits within the type being converted into.
> +///
> +/// # Supported types in `#[into(...)]`
> +///
> +/// - [`bool`]
> +/// - Primitive integer types (e.g., [`i8`], [`u8`])
> +/// - [`Bounded`]
> +///
> +/// [`Bounded`]: ../kernel/num/bounded/struct.Bounded.html

Powered by blists - more mailing lists