[PATCH 0/1] Fix redefine const_name in `vtable` macro

[PATCH 0/1] Fix redefine const_name in `vtable` macro

[Qingsong Chen]

Hi!

When using the `vtable` macro, I may find a problem.
If we define same function name in a trait (using `#[cfg]`),
the `vtable` macro will redefine `gen_const_name` for it, for
example:
```rust
    #[vtable]
    pub trait Foo {
        #[cfg(CONFIG_X)]
        fn bar();

        #[cfg(not(CONFIG_X))]
        fn bar(x: usize);
    }
```
This will define `HAS_BAR` twice. So I try to fix this by
using `HashSet`.

Qingsong Chen (1):
  rust: macros: fix redefine const_name in `vtable`

 rust/macros/vtable.rs | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

-- 
2.40.1

[PATCH 1/1] rust: macros: fix redefine const_name in `vtable`

[Qingsong Chen]

If the trait has same function name, the `vtable` macro
will redefine its `gen_const_name`, e.g.:
```rust
    #[vtable]
    pub trait Foo {
        #[cfg(CONFIG_X)]
        fn bar();

        #[cfg(not(CONFIG_X))]
        fn bar(x: usize);
    }
```
Use `HashSet` to avoid this.

Signed-off-by: Qingsong Chen <changxian.cqs@antgroup.com>
---
 rust/macros/vtable.rs | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/rust/macros/vtable.rs b/rust/macros/vtable.rs
index 34d5e7fb5768..08eb0355f99b 100644
--- a/rust/macros/vtable.rs
+++ b/rust/macros/vtable.rs
@@ -27,7 +27,7 @@ pub(crate) fn vtable(_attr: TokenStream, ts: TokenStream) -> TokenStream {
     };
 
     let mut body_it = body.stream().into_iter();
-    let mut functions = Vec::new();
+    let mut functions = HashSet::new();
     let mut consts = HashSet::new();
     while let Some(token) = body_it.next() {
         match token {
@@ -37,7 +37,7 @@ pub(crate) fn vtable(_attr: TokenStream, ts: TokenStream) -> TokenStream {
                     // Possibly we've encountered a fn pointer type instead.
                     _ => continue,
                 };
-                functions.push(fn_name);
+                functions.insert(fn_name);
             }
             TokenTree::Ident(ident) if ident.to_string() == "const" => {
                 let const_name = match body_it.next() {
-- 
2.40.1

Re: [PATCH 1/1] rust: macros: fix redefine const_name in `vtable`

[Martin Rodriguez Reboredo]

On 6/26/23 04:42, Qingsong Chen wrote:
> If the trait has same function name, the `vtable` macro
> will redefine its `gen_const_name`, e.g.:
> ```rust
>      #[vtable]
>      pub trait Foo {
>          #[cfg(CONFIG_X)]
>          fn bar();
> 
>          #[cfg(not(CONFIG_X))]
>          fn bar(x: usize);
>      }
> ```
> Use `HashSet` to avoid this.
> 
> Signed-off-by: Qingsong Chen <changxian.cqs@antgroup.com>
> ---
> [...]
Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>

Re: [PATCH 1/1] rust: macros: fix redefine const_name in `vtable`

[Benno Lossin]

On 26.06.23 09:42, Qingsong Chen wrote:
> If the trait has same function name, the `vtable` macro
> will redefine its `gen_const_name`, e.g.:
> ```rust
>      #[vtable]
>      pub trait Foo {
>          #[cfg(CONFIG_X)]
>          fn bar();
> 
>          #[cfg(not(CONFIG_X))]
>          fn bar(x: usize);
>      }
> ```
> Use `HashSet` to avoid this.
> 
> Signed-off-by: Qingsong Chen <changxian.cqs@antgroup.com>

Reviewed-by: Benno Lossin <benno.lossin@proton.me>

> ---
>   rust/macros/vtable.rs | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/rust/macros/vtable.rs b/rust/macros/vtable.rs
> index 34d5e7fb5768..08eb0355f99b 100644
> --- a/rust/macros/vtable.rs
> +++ b/rust/macros/vtable.rs
> @@ -27,7 +27,7 @@ pub(crate) fn vtable(_attr: TokenStream, ts: TokenStream) -> TokenStream {
>       };
> 
>       let mut body_it = body.stream().into_iter();
> -    let mut functions = Vec::new();
> +    let mut functions = HashSet::new();
>       let mut consts = HashSet::new();
>       while let Some(token) = body_it.next() {
>           match token {
> @@ -37,7 +37,7 @@ pub(crate) fn vtable(_attr: TokenStream, ts: TokenStream) -> TokenStream {
>                       // Possibly we've encountered a fn pointer type instead.
>                       _ => continue,
>                   };
> -                functions.push(fn_name);
> +                functions.insert(fn_name);
>               }
>               TokenTree::Ident(ident) if ident.to_string() == "const" => {
>                   let const_name = match body_it.next() {
> --
> 2.40.1
>

Re: [PATCH 1/1] rust: macros: fix redefine const_name in `vtable`

[Gary Guo]

On Mon, 26 Jun 2023 15:42:42 +0800
"Qingsong Chen" <changxian.cqs@antgroup.com> wrote:

> If the trait has same function name, the `vtable` macro
> will redefine its `gen_const_name`, e.g.:
> ```rust
>     #[vtable]
>     pub trait Foo {
>         #[cfg(CONFIG_X)]
>         fn bar();
> 
>         #[cfg(not(CONFIG_X))]
>         fn bar(x: usize);
>     }
> ```
> Use `HashSet` to avoid this.
> 
> Signed-off-by: Qingsong Chen <changxian.cqs@antgroup.com>

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

> ---
>  rust/macros/vtable.rs | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/rust/macros/vtable.rs b/rust/macros/vtable.rs
> index 34d5e7fb5768..08eb0355f99b 100644
> --- a/rust/macros/vtable.rs
> +++ b/rust/macros/vtable.rs
> @@ -27,7 +27,7 @@ pub(crate) fn vtable(_attr: TokenStream, ts: TokenStream) -> TokenStream {
>      };
>  
>      let mut body_it = body.stream().into_iter();
> -    let mut functions = Vec::new();
> +    let mut functions = HashSet::new();
>      let mut consts = HashSet::new();
>      while let Some(token) = body_it.next() {
>          match token {
> @@ -37,7 +37,7 @@ pub(crate) fn vtable(_attr: TokenStream, ts: TokenStream) -> TokenStream {
>                      // Possibly we've encountered a fn pointer type instead.
>                      _ => continue,
>                  };
> -                functions.push(fn_name);
> +                functions.insert(fn_name);
>              }
>              TokenTree::Ident(ident) if ident.to_string() == "const" => {
>                  let const_name = match body_it.next() {

Re: [PATCH 1/1] rust: macros: fix redefine const_name in `vtable`

[Miguel Ojeda]

On Mon, Jun 26, 2023 at 9:48 AM Qingsong Chen
<changxian.cqs@antgroup.com> wrote:
>
> If the trait has same function name, the `vtable` macro
> will redefine its `gen_const_name`, e.g.:
> ```rust
>     #[vtable]
>     pub trait Foo {
>         #[cfg(CONFIG_X)]
>         fn bar();
>
>         #[cfg(not(CONFIG_X))]
>         fn bar(x: usize);
>     }
> ```
> Use `HashSet` to avoid this.
>
> Signed-off-by: Qingsong Chen <changxian.cqs@antgroup.com>

Applied to `rust-fixes`, thanks! I have reworded it with content from
the cover letter to make it more clear. Please double-check if it is
OK.

By the way, for single-patches, you don't need the cover letter,
especially if both have the same information like here (in fact, in
this case the cover letter was more clear, so it should have been part
of the commit message ideally).

Cheers,
Miguel

Re: [PATCH 1/1] rust: macros: fix redefine const_name in `vtable`

[Miguel Ojeda]

On Wed, Aug 2, 2023 at 7:39 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> Applied to `rust-fixes`, thanks! I have reworded it with content from
> the cover letter to make it more clear. Please double-check if it is
> OK.

I also added a `Fixes` tag, by the way.

Cheers,
Miguel

Re: [PATCH 1/1] rust: macros: fix redefine const_name in `vtable`

[Alice Ryhl]

Qingsong Chen <changxian.cqs@antgroup.com> writes:
>      let mut body_it = body.stream().into_iter();
> -    let mut functions = Vec::new();
> +    let mut functions = HashSet::new();

Please use a `BTreeSet` instead of a `HashSet`. A `HashSet` has
non-deterministic iteration order, so this will make the macro output
different things each time you run it.

Alice

Re: [PATCH 1/1] rust: macros: fix redefine const_name in `vtable`

[Gary Guo]

On Wed,  2 Aug 2023 19:50:20 +0000
Alice Ryhl <aliceryhl@google.com> wrote:

> Qingsong Chen <changxian.cqs@antgroup.com> writes:
> >      let mut body_it = body.stream().into_iter();
> > -    let mut functions = Vec::new();
> > +    let mut functions = HashSet::new();
> 
> Please use a `BTreeSet` instead of a `HashSet`. A `HashSet` has
> non-deterministic iteration order, so this will make the macro output
> different things each time you run it.
> 
> Alice
> 

Good point.

Another way is to just use the existing `consts` HashSet to avoid
duplication. Something like

	consts.insert(gen_const_name);

after emitting the const item would probably be sufficient.

Best,
Gary

Re: [PATCH 1/1] rust: macros: fix redefine const_name in `vtable`

[Qingsong Chen]

On 8/3/23 4:02 AM, Gary Guo wrote:
> On Wed,  2 Aug 2023 19:50:20 +0000
> Alice Ryhl <aliceryhl@google.com> wrote:
> 
>> Qingsong Chen <changxian.cqs@antgroup.com> writes:
>>>      let mut body_it = body.stream().into_iter();
>>> -    let mut functions = Vec::new();
>>> +    let mut functions = HashSet::new();
>>
>> Please use a `BTreeSet` instead of a `HashSet`. A `HashSet` has
>> non-deterministic iteration order, so this will make the macro output
>> different things each time you run it.
>>
>> Alice
>>
> 
> Good point.
> 
> Another way is to just use the existing `consts` HashSet to avoid
> duplication. Something like
> 
> 	consts.insert(gen_const_name);
> 
> after emitting the const item would probably be sufficient.

According to the suggestions of Alice and Gary, we could do the fix
like this:

--- a/rust/macros/vtable.rs
+++ b/rust/macros/vtable.rs
@@ -1,7 +1,7 @@
  // SPDX-License-Identifier: GPL-2.0

  use proc_macro::{Delimiter, Group, TokenStream, TokenTree};
-use std::collections::HashSet;
+use std::collections::BTreeSet;
  use std::fmt::Write;

  pub(crate) fn vtable(_attr: TokenStream, ts: TokenStream) -> TokenStream {
@@ -28,7 +28,7 @@ pub(crate) fn vtable(_attr: TokenStream, ts: 
TokenStream) -> TokenStream {

      let mut body_it = body.stream().into_iter();
      let mut functions = Vec::new();
-    let mut consts = HashSet::new();
+    let mut consts = BTreeSet::new();
      while let Some(token) = body_it.next() {
          match token {
              TokenTree::Ident(ident) if ident.to_string() == "fn" => {
@@ -74,6 +74,7 @@ pub(crate) fn vtable(_attr: TokenStream, ts: 
TokenStream) -> TokenStream {
                  const {gen_const_name}: bool = false;",
              )
              .unwrap();
+            consts.insert(gen_const_name);
          }
      } else {
          const_items = "const USE_VTABLE_ATTR: () = ();".to_owned();

Re: [PATCH 1/1] rust: macros: fix redefine const_name in `vtable`

[Miguel Ojeda]

On Thu, Aug 3, 2023 at 11:20 AM Qingsong Chen
<changxian.cqs@antgroup.com> wrote:
>
> According to the suggestions of Alice and Gary, we could do the fix
> like this:

Could you please send a v2?

I removed the patch from `rust-fixes`, this is the reworded commit
message I used, in case you want to take a look / reuse it (of course,
updating it for v2 as needed):

    rust: macros: vtable: fix `HAS_*` redefinition (`gen_const_name`)

    If we define the same function name twice in a trait (using `#[cfg]`),
    the `vtable` macro will redefine its `gen_const_name`, e.g. this will
    define `HAS_BAR` twice:

    ```rust
        #[vtable]
        pub trait Foo {
            #[cfg(CONFIG_X)]
            fn bar();

            #[cfg(not(CONFIG_X))]
            fn bar(x: usize);
        }
    ```

    Use `HashSet` to avoid this.

Thanks!

Cheers,
Miguel