Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 23 May 2023 17:12:51 +0100
From:   Gary Guo <gary@garyguo.net>
To:     Benno Lossin <benno.lossin@proton.me>
Cc:     Miguel Ojeda <ojeda@kernel.org>,
        Alex Gaynor <alex.gaynor@gmail.com>,
        Wedson Almeida Filho <wedsonaf@gmail.com>,
        Boqun Feng <boqun.feng@gmail.com>,
        =?UTF-8?B?QmrDtnJu?= Roy Baron <bjorn3_gh@protonmail.com>,
        rust-for-linux@vger.kernel.org, linux-kernel@vger.kernel.org,
        patches@lists.linux.dev, Alice Ryhl <aliceryhl@google.com>
Subject: Re: [PATCH 3/4] rust: macros: replace Self with the concrete type
 in #[pin_data]
Message-ID: <20230523171251.786e55f5.gary@garyguo.net>
In-Reply-To: <20230424081112.99890-3-benno.lossin@proton.me>
References: <20230424081112.99890-1-benno.lossin@proton.me>
        <20230424081112.99890-3-benno.lossin@proton.me>
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?dXiCIMA2FBgaRpmFPGD2aXhfjlBCMkFnkuOOTFoQ3LyMw5MtGar1kZBJSdgw?=
 =?us-ascii?Q?Lddcv9TWwZlSN82y7wYEVikHuoxtPpkLnjpChwARp32sNn5H4XcGaj6uh2hY?=
 =?us-ascii?Q?ba/EKt9ms5mHag/tM/64xEv9B4N5403XhiV07X6Lwa0fvMl/LyOihS6++oe/?=
 =?us-ascii?Q?em8O12EDey39e/woeZss42gZmC6LYBCnK/3sh0x4VMZHPZvri0/07ypkKm9e?=
 =?us-ascii?Q?Aki2iWkjKi03w2DYJL2H/if+4VdnPKigz4wJDRIn+VAnFMCSV+PVvoqeCqDC?=
 =?us-ascii?Q?uj9zHTT4s2p81J55gVGWlhiVB0Ev9i8uAHWLta9san4RJM3Eplk8D8Lp9emX?=
 =?us-ascii?Q?sjXQNLvFcx8OG5LzvDgwWvE4D/etS5c0HCXyUpAStT+VjpGzhz/9i0PrXSh8?=
 =?us-ascii?Q?ShRLr/HGr2M4pjXhIna7tGe3a38F+mZmZ+h+tXRmKh6sQSR6MP8ABhiR78R3?=
 =?us-ascii?Q?FX6bDDvixHo5rJVWzJK/MaF5qBj3vLqaTs33MHl0CU5iBxGYf+qoqJJT3bJE?=
 =?us-ascii?Q?xYNr+tLsi269xXewMSB4FdEMhGQT37NGPgYJ3H4UNF5PiPfib53Q1WNSwhaT?=
 =?us-ascii?Q?7qj2kXq4w958smZfG4Pv0dWfOfovpF7+fGvTT8x7bqM9mcvCTe8/S7l4CKdi?=
 =?us-ascii?Q?Id+tB34ZMg62WT5oQghoHCKNbDnf38HWZ86XVlvVq1zqeHKltel1/ZHbjqAN?=
 =?us-ascii?Q?P3+5/mGmbFZ3fXAZoVqjJbbvT/lSwg38fR58OatZKTZU2PlHPo9l8Wdk4bDk?=
 =?us-ascii?Q?1VLQ7hZQ35KvvwFfiUMoACWPdbUpq/Rt6wwFrNWn3OkSWGwPqrAlt7NQaz0P?=
 =?us-ascii?Q?TEfT0cwuEPWCza6a3ve/OSi29W8fCeIXlVj0bFnrRTMaobpSF6sPJw/cCCNR?=
 =?us-ascii?Q?pRxcFbsqb8nNogTjyEjneyr5ma8ehJhhRawjELsQ34hBB5SVA2OKP+SFhUgt?=
 =?us-ascii?Q?9LztdLlORnNoM7t2yqppwSBNZkCMnmTzbdmyHuyvnYYVE+KyeL0EgTmZ4pQh?=
 =?us-ascii?Q?mUuO97pv6Z7mnxUrX2zq93DcnzYQjcu1H8X5IvdVoh1js6PmCuee8mUgMY9n?=
 =?us-ascii?Q?+8FhBkB/fIp9G/ntfFS0OnBy/wIFizjYwHFKKK+MxEtzddmGFhc4rqzqtZKA?=
 =?us-ascii?Q?Ba5ablz5D08tJntCbrKoDT8aFWAy7LY0KnUjJg5g07+mMqNVLPi2+YktLukm?=
 =?us-ascii?Q?58RaFimdW5L0II91cxP35O1GEYetZeRU/IlvKX23GrJMHCi1Mipn/IRnSaoM?=
 =?us-ascii?Q?/PnH16BGUM9sQcWIU6/JNPR6sdhvPHLdVc0gis5A08+LVop+dcvSWAkdRRqr?=
 =?us-ascii?Q?U0OiLnlUEyuHjI08xn2VNOH0Ay1Wcxe823zRhcpK3kT34upROAVnwGJ6Dtda?=
 =?us-ascii?Q?fFK97SiXWngIudQxlxmDAq/10o5dpexVI1f3Gqz9kcGlFOdOz8vORQfxHXcG?=
 =?us-ascii?Q?JruLvJZXXa6oCuYdvKWtUNAqLNbQKC4XjIBVUi49ons9wBHG5T0eo/26PEpA?=
 =?us-ascii?Q?EtDfLQsehpQi1GzgRZfsWT4G4fKFvMKfZ7/wkoNBCQohMqgzfnN9Cs/SMYuH?=
 =?us-ascii?Q?x37mduCcyB52I6VRGblODa7UaqePQRZGSfaKp/Nk?=
X-OriginatorOrg: garyguo.net
X-MS-Exchange-CrossTenant-Network-Message-Id: c828f21e-2c8b-4ed1-c81c-08db5ba8906f
X-MS-Exchange-CrossTenant-AuthSource: LO2P265MB5183.GBRP265.PROD.OUTLOOK.COM
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 May 2023 16:12:54.0377
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: bbc898ad-b10f-4e10-8552-d9377b823d45
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: URzY0l+v8/qIGEh8NUDHgG7tgozgjsNu0NyxhcSQ8gaPjCjX6WScMpwWlmu8d24E1OuyWfpoHcUh17SPyRdJ0Q==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: LO3P265MB2331
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS,T_SCC_BODY_TEXT_LINE,
        URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Mon, 24 Apr 2023 08:11:43 +0000
Benno Lossin <benno.lossin@proton.me> wrote:

> When using `#[pin_data]` on a struct that used `Self` in the field
> types, a type error would be emitted when trying to use `pin_init!`.
> Since an internal type would be referenced by `Self` instead of the
> defined struct.
> This patch fixes this issue by replacing all occurrences of `Self` in
> the `#[pin_data]` macro with the concrete type circumventing the issue.
> Since rust allows type definitions inside of blocks, which are
> expressions, the macro also checks for these and emits a compile error
> when it finds `trait`, `enum`, `union`, `struct` or `impl`. These
> keywords allow creating new `Self` contexts, which conflicts with the
> current implementation of replacing every `Self` ident. If these were
> allowed, some `Self` idents would be replaced incorrectly.
> 
> Signed-off-by: Benno Lossin <benno.lossin@proton.me>
> Reported-by: Alice Ryhl <aliceryhl@google.com>

Reviewed-by: Gary Guo <gary@garyguo.net>

> ---
>  rust/macros/pin_data.rs | 108 ++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 104 insertions(+), 4 deletions(-)
> 
> diff --git a/rust/macros/pin_data.rs b/rust/macros/pin_data.rs
> index c593b05d9e8c..6d58cfda9872 100644
> --- a/rust/macros/pin_data.rs
> +++ b/rust/macros/pin_data.rs
> @@ -1,7 +1,7 @@
>  // SPDX-License-Identifier: Apache-2.0 OR MIT
> 
>  use crate::helpers::{parse_generics, Generics};
> -use proc_macro::TokenStream;
> +use proc_macro::{Group, Punct, Spacing, TokenStream, TokenTree};
> 
>  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
> @@ -12,16 +12,116 @@ pub(crate) fn pin_data(args: TokenStream, input: TokenStream) -> TokenStream {
>              impl_generics,
>              ty_generics,
>          },
> -        mut rest,
> +        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.to_string() == "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)
> +            }
> +            _ => None,
> +        })
> +        .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(),
> +            );
> +            "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.to_string() == "struct") {
> +                vec![tt]
> +            } else {
> +                replace_self_and_deny_type_defs(&struct_name, tt, &mut errs)
> +            }
> +        })
> +        .collect::<Vec<_>>();
>      // This should be the body of the struct `{...}`.
>      let last = rest.pop();
> -    quote!(::kernel::__pin_data! {
> +    let mut quoted = quote!(::kernel::__pin_data! {
>          parse_input:
>          @args(#args),
>          @sig(#(#rest)*),
>          @impl_generics(#(#impl_generics)*),
>          @ty_generics(#(#ty_generics)*),
>          @body(#last),
> -    })
> +    });
> +    quoted.extend(errs);
> +    quoted
> +}
> +
> +/// 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.to_string() == "enum"
> +                || i.to_string() == "trait"
> +                || i.to_string() == "struct"
> +                || i.to_string() == "union"
> +                || i.to_string() == "impl" =>
> +        {
> +            errs.extend(
> +                format!(
> +                    "::core::compile_error!(\"Cannot use `{i}` inside of struct definition with \
> +                        `#[pin_data]`.\");"
> +                )
> +                .parse::<TokenStream>()
> +                .unwrap()
> +                .into_iter()
> +                .map(|mut tok| {
> +                    tok.set_span(tt.span());
> +                    tok
> +                }),
> +            );
> +            vec![tt]
> +        }
> +        TokenTree::Ident(i) if i.to_string() == "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(),
> +        ))],
> +    }
>  }
> --
> 2.40.0
> 
>