rust: macros: update 'paste!' macro to accept string literals

Message ID 20231008094816.320424-1-tmgross@umich.edu
State New
Headers
Series rust: macros: update 'paste!' macro to accept string literals |

Commit Message

Trevor Gross Oct. 8, 2023, 9:48 a.m. UTC
  Enable combining identifiers with string literals in the 'paste!' macro.
This allows combining user-specified strings with affixes to create
namespaced identifiers.

This sample code:

    macro_rules! m {
        ($name:lit) => {
            paste!(struct [<_some_ $name _struct_>];)
        }
    }

    m!("foo_bar");

Would previously cause a compilation error. It will now generate:

    struct _some_foo_bar_struct_;

Reported-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
Signed-off-by: Trevor Gross <tmgross@umich.edu>
---

Original mention of this problem in [1]

[1]: https://lore.kernel.org/rust-for-linux/20231008.164906.1151622782836568538.fujita.tomonori@gmail.com/

 rust/macros/paste.rs | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)
  

Comments

Martin Rodriguez Reboredo Oct. 8, 2023, 12:27 p.m. UTC | #1
On 10/8/23 06:48, Trevor Gross wrote:
> Enable combining identifiers with string literals in the 'paste!' macro.
> This allows combining user-specified strings with affixes to create
> namespaced identifiers.
> 
> This sample code:
> 
>      macro_rules! m {
>          ($name:lit) => {
>              paste!(struct [<_some_ $name _struct_>];)
>          }
>      }
> 
>      m!("foo_bar");
> 
> Would previously cause a compilation error. It will now generate:
> 
>      struct _some_foo_bar_struct_;
> 
> Reported-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> Signed-off-by: Trevor Gross <tmgross@umich.edu>
> ---
> 
> Original mention of this problem in [1]
> 
> [1]: https://lore.kernel.org/rust-for-linux/20231008.164906.1151622782836568538.fujita.tomonori@gmail.com/

Next time I think you should put this in `Fixes:`.

> [...]
Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
  
Trevor Gross Oct. 9, 2023, 3:03 a.m. UTC | #2
On Sun, Oct 8, 2023 at 8:27 AM Martin Rodriguez Reboredo
<yakoyoku@gmail.com> wrote:
>
> Next time I think you should put this in `Fixes:`.
>
> > [...]
> Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
>

Good point, thanks! I'll add that if there is a v2 (or Miguel can
probably add it if not)
  
Vincenzo Palazzo Oct. 9, 2023, 8:44 a.m. UTC | #3
> This allows combining user-specified strings with affixes to create
> namespaced identifiers.
>
> This sample code:
>
>     macro_rules! m {
>         ($name:lit) => {
>             paste!(struct [<_some_ $name _struct_>];)
>         }
>     }
>
>     m!("foo_bar");
>
> Would previously cause a compilation error. It will now generate:
>
>     struct _some_foo_bar_struct_;
>
> Reported-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> Signed-off-by: Trevor Gross <tmgross@umich.edu>
> ---

Reviewed-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
  
Alice Ryhl Oct. 9, 2023, 10:02 a.m. UTC | #4
On Sun, Oct 8, 2023 at 11:51 AM Trevor Gross <tmgross@umich.edu> wrote:
> Enable combining identifiers with string literals in the 'paste!' macro.
> This allows combining user-specified strings with affixes to create
> namespaced identifiers.
>
> This sample code:
>
>     macro_rules! m {
>         ($name:lit) => {
>             paste!(struct [<_some_ $name _struct_>];)
>         }
>     }
>
>     m!("foo_bar");
>
> Would previously cause a compilation error. It will now generate:
>
>     struct _some_foo_bar_struct_;
>
> Reported-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> Signed-off-by: Trevor Gross <tmgross@umich.edu>

Reviewed-by: Alice Ryhl <aliceryhl@google.com>
  
Miguel Ojeda Oct. 9, 2023, 10:49 a.m. UTC | #5
On Mon, Oct 9, 2023 at 5:04 AM Trevor Gross <tmgross@umich.edu> wrote:
>
> Good point, thanks! I'll add that if there is a v2 (or Miguel can
> probably add it if not)

Yes, I add them myself when I notice they are missing (e.g. most
recently 2 of the ones in `rust-fixes`), but patches should definitely
come with the `Fixes: ` tag themselves, i.e. it should be the
exceptional case.

However, is this actually a fix? The title and commit message make it
sound like it is a new feature rather than a fix. And the docs of the
macro says literals are not supported, right?

So this probably needs to update those docs too (and ideally add an
example with the newly supported construct too). Or am I
misunderstanding?

Cheers,
Miguel
  
Benno Lossin Oct. 9, 2023, 2:51 p.m. UTC | #6
On 08.10.23 11:48, Trevor Gross wrote:
> Enable combining identifiers with string literals in the 'paste!' macro.
> This allows combining user-specified strings with affixes to create
> namespaced identifiers.
> 
> This sample code:
> 
>      macro_rules! m {
>          ($name:lit) => {
>              paste!(struct [<_some_ $name _struct_>];)
>          }
>      }
> 
>      m!("foo_bar");
> 
> Would previously cause a compilation error. It will now generate:
> 
>      struct _some_foo_bar_struct_;
> 
> Reported-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> Signed-off-by: Trevor Gross <tmgross@umich.edu>

Reviewed-by: Benno Lossin <benno.lossin@proton.me>
  
Trevor Gross Oct. 9, 2023, 7:14 p.m. UTC | #7
On Mon, Oct 9, 2023 at 6:49 AM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Mon, Oct 9, 2023 at 5:04 AM Trevor Gross <tmgross@umich.edu> wrote:
> >
> > Good point, thanks! I'll add that if there is a v2 (or Miguel can
> > probably add it if not)
>
> Yes, I add them myself when I notice they are missing (e.g. most
> recently 2 of the ones in `rust-fixes`), but patches should definitely
> come with the `Fixes: ` tag themselves, i.e. it should be the
> exceptional case.
>
> However, is this actually a fix? The title and commit message make it
> sound like it is a new feature rather than a fix. And the docs of the
> macro says literals are not supported, right?

I suppose it is something that augments current behavior and "fixes"
the linked use case by making it possible. I am not sure what
qualifies as a fix.

> So this probably needs to update those docs too (and ideally add an
> example with the newly supported construct too). Or am I
> misunderstanding?
>
> Cheers,
> Miguel

I will update the documentation, thanks for the catch.
  
Miguel Ojeda Oct. 12, 2023, 8:45 p.m. UTC | #8
On Mon, Oct 9, 2023 at 9:14 PM Trevor Gross <tmgross@umich.edu> wrote:
>
> I suppose it is something that augments current behavior and "fixes"
> the linked use case by making it possible. I am not sure what
> qualifies as a fix.

`Fixes` is meant for issues/bugs. So if the macro was broken, i.e. it
does not do what it says it would, it would be a fix.

But if I understand correctly, the docs say this was not supported, so
it is not a fix, it is just expanding the feature set.

Similarly, `Reported-by` is not meant for feature requests.

> I will update the documentation, thanks for the catch.

Thanks!

Cheers,
Miguel
  
Gary Guo Oct. 24, 2023, 12:54 p.m. UTC | #9
On Sun,  8 Oct 2023 05:48:18 -0400
Trevor Gross <tmgross@umich.edu> wrote:

> Enable combining identifiers with string literals in the 'paste!' macro.
> This allows combining user-specified strings with affixes to create
> namespaced identifiers.
> 
> This sample code:
> 
>     macro_rules! m {
>         ($name:lit) => {
>             paste!(struct [<_some_ $name _struct_>];)
>         }
>     }
> 
>     m!("foo_bar");
> 
> Would previously cause a compilation error. It will now generate:
> 
>     struct _some_foo_bar_struct_;
> 
> Reported-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> Signed-off-by: Trevor Gross <tmgross@umich.edu>

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

> ---
> 
> Original mention of this problem in [1]
> 
> [1]: https://lore.kernel.org/rust-for-linux/20231008.164906.1151622782836568538.fujita.tomonori@gmail.com/
> 
>  rust/macros/paste.rs | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/rust/macros/paste.rs b/rust/macros/paste.rs
> index 385a78434224..f40d42b35b58 100644
> --- a/rust/macros/paste.rs
> +++ b/rust/macros/paste.rs
> @@ -9,7 +9,15 @@ fn concat(tokens: &[TokenTree], group_span: Span) -> TokenTree {
>      loop {
>          match tokens.next() {
>              None => break,
> -            Some(TokenTree::Literal(lit)) => segments.push((lit.to_string(), lit.span())),
> +            Some(TokenTree::Literal(lit)) => {
> +                // Allow us to concat string literals by stripping quotes
> +                let mut value = lit.to_string();
> +                if value.starts_with('"') && value.ends_with('"') {
> +                    value.remove(0);
> +                    value.pop();
> +                }
> +                segments.push((value, lit.span()));
> +            }
>              Some(TokenTree::Ident(ident)) => {
>                  let mut value = ident.to_string();
>                  if value.starts_with("r#") {
  
Boqun Feng Oct. 24, 2023, 4:51 p.m. UTC | #10
On Sun, Oct 08, 2023 at 05:48:18AM -0400, Trevor Gross wrote:
> Enable combining identifiers with string literals in the 'paste!' macro.
> This allows combining user-specified strings with affixes to create
> namespaced identifiers.
> 
> This sample code:
> 
>     macro_rules! m {
>         ($name:lit) => {
>             paste!(struct [<_some_ $name _struct_>];)
>         }
>     }
> 
>     m!("foo_bar");
> 
> Would previously cause a compilation error. It will now generate:
> 
>     struct _some_foo_bar_struct_;
> 
> Reported-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> Signed-off-by: Trevor Gross <tmgross@umich.edu>

This looks good to me, but could you (in a follow-up patch mabye) add an
example demonstrating the usage, which could also serve as a test if we
can run doctest for macro doc. Thanks!

Regards,
Boqun

> ---
> 
> Original mention of this problem in [1]
> 
> [1]: https://lore.kernel.org/rust-for-linux/20231008.164906.1151622782836568538.fujita.tomonori@gmail.com/
> 
>  rust/macros/paste.rs | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/rust/macros/paste.rs b/rust/macros/paste.rs
> index 385a78434224..f40d42b35b58 100644
> --- a/rust/macros/paste.rs
> +++ b/rust/macros/paste.rs
> @@ -9,7 +9,15 @@ fn concat(tokens: &[TokenTree], group_span: Span) -> TokenTree {
>      loop {
>          match tokens.next() {
>              None => break,
> -            Some(TokenTree::Literal(lit)) => segments.push((lit.to_string(), lit.span())),
> +            Some(TokenTree::Literal(lit)) => {
> +                // Allow us to concat string literals by stripping quotes
> +                let mut value = lit.to_string();
> +                if value.starts_with('"') && value.ends_with('"') {
> +                    value.remove(0);
> +                    value.pop();
> +                }
> +                segments.push((value, lit.span()));
> +            }
>              Some(TokenTree::Ident(ident)) => {
>                  let mut value = ident.to_string();
>                  if value.starts_with("r#") {
> -- 
> 2.34.1
> 
>
  

Patch

diff --git a/rust/macros/paste.rs b/rust/macros/paste.rs
index 385a78434224..f40d42b35b58 100644
--- a/rust/macros/paste.rs
+++ b/rust/macros/paste.rs
@@ -9,7 +9,15 @@  fn concat(tokens: &[TokenTree], group_span: Span) -> TokenTree {
     loop {
         match tokens.next() {
             None => break,
-            Some(TokenTree::Literal(lit)) => segments.push((lit.to_string(), lit.span())),
+            Some(TokenTree::Literal(lit)) => {
+                // Allow us to concat string literals by stripping quotes
+                let mut value = lit.to_string();
+                if value.starts_with('"') && value.ends_with('"') {
+                    value.remove(0);
+                    value.pop();
+                }
+                segments.push((value, lit.span()));
+            }
             Some(TokenTree::Ident(ident)) => {
                 let mut value = ident.to_string();
                 if value.starts_with("r#") {