| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CA+tqQ4JPMg7CGq7YiN2EwzzQBC2grRE5OFgRQTws+xh8UbzqEw@mail.gmail.com>
Date: Fri, 26 Dec 2025 18:36:36 +0900
From: Jesung Yang <y.j3ms.n@...il.com>
To: Benno Lossin <lossin@...nel.org>
Cc: 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>, 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 Fri, Dec 26, 2025 at 2:40 AM Benno Lossin <lossin@...nel.org> wrote:
>
> 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).
You're right, I missed that.
> > + }
> > + }
> > +
> > + 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
I think it's worth noting. I'll add a small comment for this.
> > + // 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?
> > + 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.
> > +
> > + 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.
> > + 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`.
> > + 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.
Thanks a lot for the time and effort you put into reviewing it!
Best regards,
Jesung
Powered by blists - more mailing lists