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

New lint: checking for typos in macro_rules #12614

Closed
sosthene-nitrokey opened this issue Apr 2, 2024 · 4 comments
Closed

New lint: checking for typos in macro_rules #12614

sosthene-nitrokey opened this issue Apr 2, 2024 · 4 comments
Labels
A-lint Area: New lints T-macros Type: Issues with macros and macro expansion

Comments

@sosthene-nitrokey
Copy link

What it does

This would lint against use of $smth in the expansion part of a macro_rules! if smth is not a capture. While such a pattern could be used in a recursive macro, it is being phased out, so in practice it is likely an typo, especially if the name of smth is close to an actual capture.

See dzamlo/rust-bitfield#40 for the case that prompted this lint suggestion.

Advantage

  • Detect potential bugs in macros.
    Macros are hard to debug and have very little compile-time validity checks. Complex recursive macros are very complex, and can have many rules. Any tool to improve that is good.

Drawbacks

There is a possibility of false positives

Example

macro_rules! testing {
    ($testing:ty)  => {$testin}
}

Will never be usable because testin is not a capture (notice the missing g) is not a capture.

it should instead be written as:

macro_rules! testing {
    ($testing:ty)  => {$testing}
}
@sosthene-nitrokey sosthene-nitrokey added the A-lint Area: New lints label Apr 2, 2024
@y21
Copy link
Member

y21 commented Apr 2, 2024

Implementing such a lint is probably tricky because the AST for macro definitions only has the raw TokenStream available, making it difficult to look for anything (even just finding macro matchers). So this sounds like it'd require some manual parsing.

Other similar lints have done this by looking for expansions, but that doesn't seem viable here since at that point you would get a proper compile error instead

@y21 y21 added the T-macros Type: Issues with macros and macro expansion label Apr 2, 2024
@PatchMixolydic
Copy link
Contributor

PatchMixolydic commented Apr 2, 2024

This is already caught by the allow-by-default meta_variable_misuse lint in rustc (playground):

warning: unknown macro variable `testin`
 --> src/lib.rs:4:24
  |
4 |     ($testing:ty)  => {$testin}
  |                        ^^^^^^^
  |
note: the lint level is defined here
 --> src/lib.rs:1:9
  |
1 | #![warn(meta_variable_misuse)]
  |         ^^^^^^^^^^^^^^^^^^^^

@sosthene-nitrokey
Copy link
Author

Oh thank you! Though it's sad to see it being allow by default, but it seems like a hard problem to solve.

Maybe this issue should be closed?

@flip1995
Copy link
Member

flip1995 commented Apr 3, 2024

Closing, as already covered by rustc.

@flip1995 flip1995 closed this as not planned Won't fix, can't repro, duplicate, stale Apr 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints T-macros Type: Issues with macros and macro expansion
Projects
None yet
Development

No branches or pull requests

4 participants