Message-Id: <DFOJAWAGBPBD.1SKKFVK435H9O@garyguo.net>
Date: Wed, 14 Jan 2026 18:47:00 +0000
From: "Gary Guo" <gary@...yguo.net>
To: "Benno Lossin" <lossin@...nel.org>, "Gary Guo" <gary@...yguo.net>,
 "Miguel Ojeda" <ojeda@...nel.org>, "Boqun Feng" <boqun.feng@...il.com>,
 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>,
 "Fiona Behrens" <me@...enk.dev>, "Tamir Duberstein" <tamird@...il.com>,
 "Alban Kurti" <kurti@...icto.ai>
Cc: <linux-kernel@...r.kernel.org>, <rust-for-linux@...r.kernel.org>
Subject: Re: [PATCH v3 07/15] rust: pin-init: rewrite `#[pin_data]` using
 `syn`

On Wed Jan 14, 2026 at 6:18 PM GMT, Benno Lossin wrote:
> Rewrite the attribute macro `#[pin_data]` using `syn`. No functional
> changes intended aside from improved error messages on syntactic and
> semantical errors. For example if one forgets a comma at the end of a
> field:
>
>     #[pin_data]
>     struct Foo {
>         a: Box<Foo>
>         b: Box<Foo>
>     }
>
> The declarative macro reports the following errors:
>
>     error: expected `,`, or `}`, found `b`
>      --> tests/ui/compile-fail/pin_data/missing_comma.rs:5:16
>       |
>     5 |     a: Box<Foo>
>       |                ^ help: try adding a comma: `,`
>
>     error: recursion limit reached while expanding `$crate::__pin_data!`
>      --> tests/ui/compile-fail/pin_data/missing_comma.rs:3:1
>       |
>     3 | #[pin_data]
>       | ^^^^^^^^^^^
>       |
>       = help: consider increasing the recursion limit by adding a `#![recursion_limit = "256"]` attribute to your crate (`$CRATE`)
>       = note: this error originates in the macro `$crate::__pin_data` which comes from the expansion of the attribute macro `pin_data` (in Nightly builds, run with -Z macro-backtrace for more info)
>
> The new `syn` version reports:
>
>     error: expected `,`, or `}`, found `b`
>      --> tests/ui/compile-fail/pin_data/missing_comma.rs:5:16
>       |
>     5 |     a: Box<Foo>
>       |                ^ help: try adding a comma: `,`
>
>     error: expected `,`
>      --> tests/ui/compile-fail/pin_data/missing_comma.rs:6:5
>       |
>     6 |     b: Box<Foo>
>       |     ^
>
> Tested-by: Andreas Hindborg <a.hindborg@...nel.org>
> Signed-off-by: Benno Lossin <lossin@...nel.org>
> ---
> Changes in v3:
> * use DiagCtxt error handling
> Changes in v2:
> * improved error handling
> * fix clippy warnings
> * fix typos and variable names
> * collect the information about the pinned/not pinned fields only once
>   at the beginning
> ---
>  rust/pin-init/internal/src/helpers.rs  | 149 ------
>  rust/pin-init/internal/src/lib.rs      |   7 +-
>  rust/pin-init/internal/src/pin_data.rs | 633 ++++++++++++++++++++-----
>  rust/pin-init/src/macros.rs            | 574 ----------------------
>  4 files changed, 525 insertions(+), 838 deletions(-)
>  delete mode 100644 rust/pin-init/internal/src/helpers.rs
>
> diff --git a/rust/pin-init/internal/src/pin_data.rs b/rust/pin-init/internal/src/pin_data.rs
> index 86a53b37cc66..fb4b8507c01d 100644
> --- a/rust/pin-init/internal/src/pin_data.rs
> +++ b/rust/pin-init/internal/src/pin_data.rs
> @@ -1,126 +1,535 @@
>  // SPDX-License-Identifier: Apache-2.0 OR MIT
>  
> -use crate::helpers::{parse_generics, Generics};
> -use proc_macro2::{Group, Punct, Spacing, TokenStream, TokenTree};
> -use quote::quote;
> +use proc_macro2::TokenStream;
> +use quote::{format_ident, quote};
> +use syn::{
> +    parse::{End, Nothing, Parse},
> +    parse_quote, parse_quote_spanned,
> +    spanned::Spanned,
> +    visit_mut::VisitMut,
> +    Field, Generics, Ident, Item, PathSegment, Type, TypePath, Visibility, WhereClause,
> +};
>  
> -pub(crate) fn pin_data(args: TokenStream, input: TokenStream) -> TokenStream {
> -    // This proc-macro only does some pre-parsing and then delegates the actual parsing to
> -    // `pin_init::__pin_data!`.
> +use crate::diagnostics::{DiagCtxt, ErrorGuaranteed};
>  
> -    let (
> -        Generics {
> -            impl_generics,
> -            decl_generics,
> -            ty_generics,
> -        },
> -        rest,
> -    ) = parse_generics(input);
> -    // The struct definition might contain the `Self` type. Since `__pin_data!` will define a new
> -    // type with the same generics and bounds, this poses a problem, since `Self` will refer to the
> -    // new type as opposed to this struct definition. Therefore we have to replace `Self` with the
> -    // concrete name.
> -
> -    // Errors that occur when replacing `Self` with `struct_name`.
> -    let mut errs = TokenStream::new();
> -    // The name of the struct with ty_generics.
> -    let struct_name = rest
> -        .iter()
> -        .skip_while(|tt| !matches!(tt, TokenTree::Ident(i) if i == "struct"))
> -        .nth(1)
> -        .and_then(|tt| match tt {
> -            TokenTree::Ident(_) => {
> -                let tt = tt.clone();
> -                let mut res = vec![tt];
> -                if !ty_generics.is_empty() {
> -                    // We add this, so it is maximally compatible with e.g. `Self::CONST` which
> -                    // will be replaced by `StructName::<$generics>::CONST`.
> -                    res.push(TokenTree::Punct(Punct::new(':', Spacing::Joint)));
> -                    res.push(TokenTree::Punct(Punct::new(':', Spacing::Alone)));
> -                    res.push(TokenTree::Punct(Punct::new('<', Spacing::Alone)));
> -                    res.extend(ty_generics.iter().cloned());
> -                    res.push(TokenTree::Punct(Punct::new('>', Spacing::Alone)));
> -                }
> -                Some(res)
> +pub(crate) mod kw {
> +    syn::custom_keyword!(PinnedDrop);
> +}
> +
> +pub(crate) enum Args {
> +    Nothing(Nothing),
> +    #[allow(dead_code)]
> +    PinnedDrop(kw::PinnedDrop),
> +}
> +
> +impl Parse for Args {
> +    fn parse(input: syn::parse::ParseStream<'_>) -> syn::Result<Self> {
> +        let lh = input.lookahead1();
> +        if lh.peek(End) {
> +            input.parse().map(Self::Nothing)
> +        } else if lh.peek(kw::PinnedDrop) {
> +            let res = input.parse().map(Self::PinnedDrop)?;
> +            let lh = input.lookahead1();
> +            if lh.peek(End) {
> +                Ok(res)
> +            } else {
> +                Err(lh.error())
>              }

I believe that this `End` peek is unnecessary?

> -            _ => None,
> +        } else {
> +            Err(lh.error())
> +        }
> +    }
> +}
> +
> +pub(crate) fn pin_data(
> +    args: Args,
> +    input: Item,
> +    dcx: &mut DiagCtxt,
> +) -> Result<TokenStream, ErrorGuaranteed> {
> +    let mut struct_ = match input {
> +        Item::Struct(struct_) => struct_,
> +        Item::Enum(enum_) => {
> +            return Err(dcx.error(
> +                enum_.enum_token,
> +                "`#[pin_data]` only supports structs for now",
> +            ));
> +        }
> +        Item::Union(union) => {
> +            return Err(dcx.error(
> +                union.union_token,
> +                "`#[pin_data]` only supports structs for now",
> +            ));
> +        }
> +        rest => {
> +            return Err(dcx.error(
> +                rest,
> +                "`#[pin_data]` can only be applied to struct, enum and union definitions",
> +            ));
> +        }
> +    };
> +
> +    // The generics might contain the `Self` type. Since this macro will define a new type with the
> +    // same generics and bounds, this poses a problem: `Self` will refer to the new type as opposed
> +    // to this struct definition. Therefore we have to replace `Self` with the concrete name.
> +    let mut replacer = {
> +        let name = &struct_.ident;
> +        let (_, ty_generics, _) = struct_.generics.split_for_impl();
> +        SelfReplacer(parse_quote!(#name #ty_generics))
> +    };
> +    replacer.visit_generics_mut(&mut struct_.generics);
> +    replacer.visit_fields_mut(&mut struct_.fields);
> +
> +    let fields: Vec<(bool, Field)> = struct_
> +        .fields
> +        .iter()
> +        .cloned()
> +        .map(|mut field| {
> +            let pinned = is_field_structurally_pinned(&field);

There is only one occurance now so you can just check if a field is pinned by
looking at if `field.attrs.len()` is changed the line below (see my syn series
where Tamir and I had some discussion about something similar).

> +            field.attrs.retain(|a| !a.path().is_ident("pin"));
> +            (pinned, field)
>          })
> -        .unwrap_or_else(|| {
> -            // If we did not find the name of the struct then we will use `Self` as the replacement
> -            // and add a compile error to ensure it does not compile.
> -            errs.extend(
> -                "::core::compile_error!(\"Could not locate type name.\");"
> -                    .parse::<TokenStream>()
> -                    .unwrap(),
> +        .collect();
> +
> +    for (pinned, field) in &fields {
> +        if !pinned && is_phantom_pinned(&field.ty) {
> +            dcx.error(
> +                field,
> +                format!(
> +                    "The field `{}` of type `PhantomPinned` only has an effect \
> +                    if it has the `#[pin]` attribute",
> +                    field.ident.as_ref().unwrap(),
> +                ),
>              );
> -            "Self".parse::<TokenStream>().unwrap().into_iter().collect()
> -        });
> -    let impl_generics = impl_generics
> -        .into_iter()
> -        .flat_map(|tt| replace_self_and_deny_type_defs(&struct_name, tt, &mut errs))
> -        .collect::<Vec<_>>();
> -    let mut rest = rest
> -        .into_iter()
> -        .flat_map(|tt| {
> -            // We ignore top level `struct` tokens, since they would emit a compile error.
> -            if matches!(&tt, TokenTree::Ident(i) if i == "struct") {
> -                vec![tt]
> -            } else {
> -                replace_self_and_deny_type_defs(&struct_name, tt, &mut errs)
> +        }
> +    }
> +
> +    let unpin_impl = generate_unpin_impl(&struct_.ident, &struct_.generics, &fields);
> +    let drop_impl = generate_drop_impl(&struct_.ident, &struct_.generics, args);
> +    let projections =
> +        generate_projections(&struct_.vis, &struct_.ident, &struct_.generics, &fields);
> +    let the_pin_data =
> +        generate_the_pin_data(&struct_.vis, &struct_.ident, &struct_.generics, &fields);
> +
> +    strip_pin_annotations(&mut struct_);

I think this can be removed if you clone the fields after the `retain` -- or
even better, just avoid the clone of fields.

> +
> +    Ok(quote! {
> +        #struct_
> +        #projections
> +        // We put the rest into this const item, because it then will not be accessible to anything
> +        // outside.
> +        const _: () = {
> +            #the_pin_data
> +            #unpin_impl
> +            #drop_impl
> +        };
> +    })
> +}
> +
> +fn is_phantom_pinned(ty: &Type) -> bool {
> +    match ty {
> +        Type::Path(TypePath { qself: None, path }) => {
> +            // Cannot possibly refer to `PhantomPinned` (except alias, but that's on the user).
> +            if path.segments.len() > 3 {
> +                return false;
>              }
> -        })
> -        .collect::<Vec<_>>();
> -    // This should be the body of the struct `{...}`.
> -    let last = rest.pop();
> -    let mut quoted = quote!(::pin_init::__pin_data! {
> -        parse_input:
> -        @args(#args),
> -        @sig(#(#rest)*),
> -        @impl_generics(#(#impl_generics)*),
> -        @ty_generics(#(#ty_generics)*),
> -        @decl_generics(#(#decl_generics)*),
> -        @body(#last),
> -    });
> -    quoted.extend(errs);
> -    quoted
> +            // If there is a `::`, then the path needs to be `::core::marker::PhantomPinned` or
> +            // `::std::marker::PhantomPinned`.
> +            if path.leading_colon.is_some() && path.segments.len() != 3 {
> +                return false;
> +            }
> +            let expected: Vec<&[&str]> = vec![&["PhantomPinned"], &["marker"], &["core", "std"]];
> +            for (actual, expected) in path.segments.iter().rev().zip(expected) {
> +                if !actual.arguments.is_empty() || expected.iter().all(|e| actual.ident != e) {
> +                    return false;
> +                }
> +            }
> +            true
> +        }
> +        _ => false,
> +    }
>  }
>  
> -/// Replaces `Self` with `struct_name` and errors on `enum`, `trait`, `struct` `union` and `impl`
> -/// keywords.
> -///
> -/// The error is appended to `errs` to allow normal parsing to continue.
> -fn replace_self_and_deny_type_defs(
> -    struct_name: &Vec<TokenTree>,
> -    tt: TokenTree,
> -    errs: &mut TokenStream,
> -) -> Vec<TokenTree> {
> -    match tt {
> -        TokenTree::Ident(ref i)
> -            if i == "enum" || i == "trait" || i == "struct" || i == "union" || i == "impl" =>
> +fn is_field_structurally_pinned(field: &Field) -> bool {
> +    field.attrs.iter().any(|a| a.path().is_ident("pin"))
> +}
> +
> +fn generate_unpin_impl(
> +    ident: &Ident,
> +    generics: &Generics,
> +    fields: &[(bool, Field)],
> +) -> TokenStream {
> +    let generics_with_pin_lt = {
> +        let mut g = generics.clone();
> +        g.params.insert(0, parse_quote!('__pin));
> +        let _ = g.make_where_clause();
> +        g
> +    };
> +    let (
> +        impl_generics_with_pin_lt,
> +        ty_generics_with_pin_lt,
> +        Some(WhereClause {
> +            where_token,
> +            predicates,
> +        }),
> +    ) = generics_with_pin_lt.split_for_impl()
> +    else {
> +        unreachable!()
> +    };
> +    let (_, ty_generics, _) = generics.split_for_impl();
> +    let pinned_fields = fields.iter().filter_map(|(b, f)| b.then_some(f));
> +    quote! {
> +        // This struct will be used for the unpin analysis. It is needed, because only structurally
> +        // pinned fields are relevant whether the struct should implement `Unpin`.
> +        #[allow(dead_code)] // The fields below are never used.
> +        struct __Unpin #generics_with_pin_lt
> +        #where_token
> +            #predicates
>          {
> -            errs.extend(
> -                format!(
> -                    "::core::compile_error!(\"Cannot use `{i}` inside of struct definition with \
> -                        `#[pin_data]`.\");"
> +            __phantom_pin: ::core::marker::PhantomData<fn(&'__pin ()) -> &'__pin ()>,
> +            __phantom: ::core::marker::PhantomData<
> +                fn(#ident #ty_generics) -> #ident #ty_generics
> +            >,
> +            #(#pinned_fields),*
> +        }
> +
> +        #[doc(hidden)]
> +        impl #impl_generics_with_pin_lt ::core::marker::Unpin for #ident #ty_generics
> +        #where_token
> +            __Unpin #ty_generics_with_pin_lt: ::core::marker::Unpin,
> +            #predicates
> +        {}
> +    }
> +}
> +
> +fn generate_drop_impl(ident: &Ident, generics: &Generics, args: Args) -> TokenStream {
> +    let (impl_generics, ty_generics, whr) = generics.split_for_impl();
> +    let has_pinned_drop = matches!(args, Args::PinnedDrop(_));
> +    // We need to disallow normal `Drop` implementation, the exact behavior depends on whether
> +    // `PinnedDrop` was specified in `args`.
> +    if has_pinned_drop {
> +        // When `PinnedDrop` was specified we just implement `Drop` and delegate.
> +        quote! {
> +            impl #impl_generics ::core::ops::Drop for #ident #ty_generics
> +                #whr
> +            {
> +                fn drop(&mut self) {
> +                    // SAFETY: Since this is a destructor, `self` will not move after this function
> +                    // terminates, since it is inaccessible.
> +                    let pinned = unsafe { ::core::pin::Pin::new_unchecked(self) };
> +                    // SAFETY: Since this is a drop function, we can create this token to call the
> +                    // pinned destructor of this type.
> +                    let token = unsafe { ::pin_init::__internal::OnlyCallFromDrop::new() };
> +                    ::pin_init::PinnedDrop::drop(pinned, token);
> +                }
> +            }
> +        }
> +    } else {
> +        // When no `PinnedDrop` was specified, then we have to prevent implementing drop.
> +        quote! {
> +            // We prevent this by creating a trait that will be implemented for all types implementing
> +            // `Drop`. Additionally we will implement this trait for the struct leading to a conflict,
> +            // if it also implements `Drop`
> +            trait MustNotImplDrop {}
> +            #[expect(drop_bounds)]
> +            impl<T: ::core::ops::Drop> MustNotImplDrop for T {}
> +            impl #impl_generics MustNotImplDrop for #ident #ty_generics
> +                #whr
> +            {}
> +            // We also take care to prevent users from writing a useless `PinnedDrop` implementation.
> +            // They might implement `PinnedDrop` correctly for the struct, but forget to give
> +            // `PinnedDrop` as the parameter to `#[pin_data]`.
> +            #[expect(non_camel_case_types)]
> +            trait UselessPinnedDropImpl_you_need_to_specify_PinnedDrop {}
> +            impl<T: ::pin_init::PinnedDrop>
> +                UselessPinnedDropImpl_you_need_to_specify_PinnedDrop for T {}
> +            impl #impl_generics
> +                UselessPinnedDropImpl_you_need_to_specify_PinnedDrop for #ident #ty_generics
> +                #whr
> +            {}
> +        }
> +    }
> +}
> +
> +fn generate_projections(
> +    vis: &Visibility,
> +    ident: &Ident,
> +    generics: &Generics,
> +    fields: &[(bool, Field)],
> +) -> TokenStream {
> +    let (impl_generics, ty_generics, _) = generics.split_for_impl();
> +    let mut generics = generics.clone();
> +    generics.params.insert(0, parse_quote!('__pin));
> +    let (_, ty_generics_with_pin_lt, whr) = generics.split_for_impl();

This part uses split-modify-split while `generate_unpin_impl` uses
clone-then-split. I am fine with either but please make them consistent.

Best,
Gary

> +    let projection = format_ident!("{ident}Projection");
> +    let this = format_ident!("this");
> +
> +    let (fields_decl, fields_proj) = collect_tuple(fields.iter().map(
> +        |(
> +            pinned,
> +            Field {
> +                vis,
> +                ident,
> +                ty,
> +                attrs,
> +                ..
> +            },
> +        )| {
> +            let mut attrs = attrs.clone();
> +            attrs.retain(|a| !a.path().is_ident("pin"));
> +            let mut no_doc_attrs = attrs.clone();
> +            no_doc_attrs.retain(|a| !a.path().is_ident("doc"));
> +            let ident = ident
> +                .as_ref()
> +                .expect("only structs with named fields are supported");
> +            if *pinned {
> +                (
> +                    quote!(
> +                        #(#attrs)*
> +                        #vis #ident: ::core::pin::Pin<&'__pin mut #ty>,
> +                    ),
> +                    quote!(
> +                        #(#no_doc_attrs)*
> +                        // SAFETY: this field is structurally pinned.
> +                        #ident: unsafe { ::core::pin::Pin::new_unchecked(&mut #this.#ident) },
> +                    ),
>                  )
> -                .parse::<TokenStream>()
> -                .unwrap()
> -                .into_iter()
> -                .map(|mut tok| {
> -                    tok.set_span(tt.span());
> -                    tok
> -                }),
> -            );
> -            vec![tt]
> -        }
> -        TokenTree::Ident(i) if i == "Self" => struct_name.clone(),
> -        TokenTree::Literal(_) | TokenTree::Punct(_) | TokenTree::Ident(_) => vec![tt],
> -        TokenTree::Group(g) => vec![TokenTree::Group(Group::new(
> -            g.delimiter(),
> -            g.stream()
> -                .into_iter()
> -                .flat_map(|tt| replace_self_and_deny_type_defs(struct_name, tt, errs))
> -                .collect(),
> -        ))],
> +            } else {
> +                (
> +                    quote!(
> +                        #(#attrs)*
> +                        #vis #ident: &'__pin mut #ty,
> +                    ),
> +                    quote!(
> +                        #(#no_doc_attrs)*
> +                        #ident: &mut #this.#ident,
> +                    ),
> +                )
> +            }
> +        },
> +    ));
> +    let structurally_pinned_fields_docs = fields
> +        .iter()
> +        .filter_map(|(pinned, field)| pinned.then_some(field))
> +        .map(|Field { ident, .. }| format!(" - `{}`", ident.as_ref().unwrap()));
> +    let not_structurally_pinned_fields_docs = fields
> +        .iter()
> +        .filter_map(|(pinned, field)| (!pinned).then_some(field))
> +        .map(|Field { ident, .. }| format!(" - `{}`", ident.as_ref().unwrap()));
> +    let docs = format!(" Pin-projections of [`{ident}`]");
> +    quote! {
> +        #[doc = #docs]
> +        #[allow(dead_code)]
> +        #[doc(hidden)]
> +        #vis struct #projection #generics {
> +            #(#fields_decl)*
> +            ___pin_phantom_data: ::core::marker::PhantomData<&'__pin mut ()>,
> +        }
> +
> +        impl #impl_generics #ident #ty_generics
> +            #whr
> +        {
> +            /// Pin-projects all fields of `Self`.
> +            ///
> +            /// These fields are structurally pinned:
> +            #(#[doc = #structurally_pinned_fields_docs])*
> +            ///
> +            /// These fields are **not** structurally pinned:
> +            #(#[doc = #not_structurally_pinned_fields_docs])*
> +            #[inline]
> +            #vis fn project<'__pin>(
> +                self: ::core::pin::Pin<&'__pin mut Self>,
> +            ) -> #projection #ty_generics_with_pin_lt {
> +                // SAFETY: we only give access to `&mut` for fields not structurally pinned.
> +                let #this = unsafe { ::core::pin::Pin::get_unchecked_mut(self) };
> +                #projection {
> +                    #(#fields_proj)*
> +                    ___pin_phantom_data: ::core::marker::PhantomData,
> +                }
> +            }
> +        }
> +    }
> +}

Powered by blists - more mailing lists