rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
98.34k stars 12.72k forks source link

empty expansion of a macro pattern does not cause the comma-separated repetition expander to omit the comma #96787

Open daym opened 2 years ago

daym commented 2 years ago

I tried this code:

macro_rules! make_accessors {
(
        struct $struct_name:ident {
                $(
                        $(#[$field_meta:meta])*
                        $field_vis:vis $field_name:ident : $field_ty:ty
                        $(: get $getter_ty:ty)?,
                )* $(,)?
        }
) => {
        struct $struct_name {
                $(
                        $(#[$field_meta])*
                        $(
                                $field_name : $getter_ty
                        )?
                ),*
        }
}}

make_accessors! {
        struct A {
                a: u32 : get u32,
                b: u32,
                c: u32 : get u32,
        }
}

fn main() {
}

I expected to see this happen:

Since the expansion of the $(...)? and $field_meta is the empty expansion, I would have expected the $(...),* not to add a comma.

Instead, this happened:

It added a comma after the empty expansion.

After that, the Rust compiler failed with the following error message:

error: expected identifier, found `,`
  --> src/main.rs:18:4
   |
18 |         ),*
   |          ^
   |          |
   |          expected identifier
   |          help: remove this comma

Even cargo expand failed:

cargo expand
    Checking zzmac1 v0.1.0 (/home/dannym/backup/zzmac1)
error: expected identifier, found `,`
  --> src/main.rs:18:4
   |
18 |         ),*
   |          ^
   |          |
   |          expected identifier
   |          help: remove this comma

#![feature(prelude_import)]
#[prelude_import]
use std::prelude::rust_2021::*;
#[macro_use]
extern crate std;
struct A {
    a: u32,
}
fn main() {
}

Tested in rustc 1.62.0-nightly (055bf4ccd 2022-04-25) and in rustc 1.57.0.

Wardenfar commented 2 years ago

Your macro declaration is incorrect. In the pattern part of the macro, $field_name is at the 2nd level of "$ scopes", but in the replacement part of the macro :  $field_name is at the 3rd level of "$ scopes".

$($field_name : $getter_ty)?
This part mix 2/3 level, and that's why you are maybe confused.

When the compiler sees the field b without a : get <type> part.

$field_name has a value but not $getter_ty, so it gets replaced with nothing. At the end, there is a blank after b : and rustc expect an identifier for the field type.

daym commented 2 years ago

This part mix 2/3 level.

This is on purpose. I want it to mix 2/3 level.

In fact, what I'd really like is to to put the following into the replacement part of the macro definition:

        struct $struct_name {
                $(
                        $(
                                $(#[$field_meta])* // yes, from the parent level
                                $field_name : $getter_ty
                        )?
                ),*
        }

but I guess that's not relevant for this bug report (which is a minimal reproducer of the problem that occurs when I do the workaround at the top instead of the previous block which is what I really would want).

$field_name has a value but not $getter_ty, so it gets replaced with nothing.

Yes, I want the entire text b: u32, to be replaced by nothing in the case of a missing getter.

I would expect that if the $(...)? is not present, neither the $field_name nor the : nor the $getter_ty gets output. But you say that in that case b: still gets output in the case of $()? having 0 matches. Are you sure that's the case?

The goal is this:

I want Rust to create a struct with only the fields that have a : get, with the type for the respective field being that type that is referred to after the : set.

For another example,

make_accessors! {
        struct A {
                a: u32 : get Q,
                b: u32,
                c: u32 : get R,
        }
}

I'd want this output:

        struct A {
                a: Q,
                c: R,
        }

But instead, I think it does this (I can't tell for sure since cargo expand does not work):

        struct A {
                a: Q,
                ,
                c: R,
        }
Wardenfar commented 2 years ago

I made it work by moving the comma in the $ scope. You were right.

macro_rules! make_accessors {
    (
            struct $struct_name:ident {
                    $(
                            $(#[$field_meta:meta])*
                            $field_vis:vis $field_name:ident : $field_ty:ty
                            $(: get $getter_ty:ty)?,
                    )* $(,)?
            }
    ) => {
            struct $struct_name {
                    $(
                            $(#[$field_meta])*
                            $(
                                    $field_name : $getter_ty, // put it there
                            )?
                    )* // remove the comma here
            }
    }}
    make_accessors! {
            struct A {
                    a: u32 : get u32,
                    b: u32,
                    c: u32 : get u32,
            }
    }

    fn main() {
    }
daym commented 2 years ago

With your solution, if there's a field with a meta and without a get, then it will still output the meta and then that will apply to the next field.

For example

    make_accessors! {
            struct A {
                    #[quux]
                    a: u32 : get Q,
                    #[foo]
                    b: u32,
                    #[bar]
                    c: u32 : get R,
            }
    }

With your variant, the following gets output:

            struct A {
                    #[quux]
                    a: Q,
                    #[foo] // uh oh
                    #[bar]
                    c: R,
            }

This is very very bad for my purposes.

Wardenfar commented 2 years ago

Ok, I don't know how to solve this problem with declarative macros. You should consider using proc-macros. #[derive(****)] So this is not a bug. Maybe close this issue ?

daym commented 2 years ago

Yeah, maybe a derive macro is the right solution here.

But I think macro_rules outputting an extra comma between nothings is a bug.

Doing so doesn't seem like something $(...),* should be doing. After all, that's probably why there is a special comma form of $(...)* in the first place: To not emit extra commas where they shouldn't be commas.

If I wanted to emit a comma unconditionally, $(...),* is unnecessary and wouldn't exist (one could just do $(...,)* then).

If this is the wrong forum, fair enough.

I have to say macros are kinda underdocumented. I'm not sure what the actual intended semantics of $(...),* in the replacement area are--I can only do educated guesses.

Also, cargo expand erroring out and just skipping the entire thing is a bug. I mean what are you using cargo expand for if not to debug broken macro replacements? And in such a case it just skips the output entirely?

Wardenfar commented 2 years ago

Also, cargo expand erroring out and just skipping the entire thing is a bug. I mean what are you using cargo expand for if not to debug broken macro replacements? And in such a case it just skips the output entirely?

cargo expand show the result after expansion, but in this case the expansion failed, cargo expand cannot print anything.

daym commented 2 years ago

cargo expand show the result after expansion, but in this case the expansion failed, cargo expand cannot print anything.

What do you use in such a case to find what it did?