Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Rust] Macro improperly marked as invalid #3904

Open
Rapptz opened this issue Jan 14, 2024 · 3 comments · May be fixed by #3912
Open

[Rust] Macro improperly marked as invalid #3904

Rapptz opened this issue Jan 14, 2024 · 3 comments · May be fixed by #3912
Labels
C: Syntax T: bug A bug in an existing language feature

Comments

@Rapptz
Copy link

Rapptz commented Jan 14, 2024

What happened?

The following macro gives an erroneous source.rust meta.macro.rust invalid.illegal.rust scope:

macro_rules! test {
    ($($name:ident),+$(,)?) => {
        pub struct Foo {
            $(
                pub $name : Option<String>
            ),+
        }

        pub async fn thing() -> () {}
    }
}

image

From a cursory view in the repository and from testing it seems this is applied because of this rule

macro-semi-sep:
- include: comments
- match: ';'
scope: punctuation.terminator.rust
pop: true
- match: '(?=[})\]])'
pop: true
- match: '\S'
# This is intended to help make it evident when you forget a semicolon.
scope: invalid.illegal.rust

Adding the ; after the struct definition does make the scope go away but ironically it leads to invalid code since you can't have ; after a struct definition:

error: expected item, found `;`
  --> src/main.rs:15:2
   |
15 | };
   |  ^ help: remove this semicolon
   |
   = help: braced struct declarations are not followed by a semicolon
@deathaxe deathaxe added T: bug A bug in an existing language feature C: Syntax labels Jan 14, 2024
@FichteFoll FichteFoll linked a pull request Jan 28, 2024 that will close this issue
@FichteFoll
Copy link
Collaborator

FichteFoll commented Jan 28, 2024

The issue was a bit more involved than you initially suspected because the problem was that macro repetitions inside transcribers where basically unsupported. The linked PR #3912 fixes this particular issue for structs, but due to a lack of real world examples for other block statements I didn't check if they exhibit the same or similar issues.

If you have some more complex examples for macros, that would be much appreciated.

Edit: Well, didn't take long for me to find a problem with that implementation. No promises when I'll find time to work on this again, considering that I also don't have an idea how to resolve it currently

@Rapptz
Copy link
Author

Rapptz commented Jan 30, 2024

At first glance, I couldn't find any other macros that error out (that use repetitions) outside of this (partially censored) one:

#[macro_export]
macro_rules! metadata {
    (
        namespace $ns:literal;
        $(
            $name:ident: $ity:ty
        ),+$(,)?
    ) => {
        #[derive(Debug, Clone, Default, Eq, PartialEq)]
        pub struct Metadata {
            $(
                $name: Option<$ity>
            ),+
        }

        impl $crate::database::Metadata for Metadata {
            const NAMESPACE: &'static str = $ns;
        }
    }
}

The others that I use that have repetitions currently do not error, e.g.

#[macro_export]
macro_rules! header_map {
    ($($name:literal => $value:expr),* $(,)*) => {{
        let mut headers = reqwest::header::HeaderMap::with_capacity($crate::__count!($($name)*));
        $(
            headers.insert(
                reqwest::header::HeaderName::from_static($name),
                reqwest::header::HeaderValue::from_static($value),
            );
        )*

        headers
    }};
}

@FichteFoll
Copy link
Collaborator

Yeah, it's (multiple) macro transcribers inside other blocks such as a struct definition that are a problem here (or similarly for enums). Since they can appear just about everywhere, I'd prefer a generic solution on the syntax definition side over a specific one, but it may just not be that easy to realize because of state tracking … Will need to ponder about it a bit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: Syntax T: bug A bug in an existing language feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants