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: Check quote! macro is always used with curyl braces #7278

Closed
DevinR528 opened this issue May 26, 2021 · 4 comments · Fixed by #7299
Closed

New lint: Check quote! macro is always used with curyl braces #7278

DevinR528 opened this issue May 26, 2021 · 4 comments · Fixed by #7299
Labels
A-lint Area: New lints

Comments

@DevinR528
Copy link
Contributor

What it does

What does this lint do?

What it does

Check quote! is always used with curly braces, this could maybe apply to other macros or other bracing combos?

Categories

  • Nursery

This is mostly a consistency lint although using () or [] doesn't give you a semicolon in item position, which can be unexpected.

Drawbacks

None 🤷

Example

quote! ( some tokens )

Could be written as:

quote! { some tokens }

If everyone is ok with this in the nursery I can open a PR.

@jplatte
Copy link
Contributor

jplatte commented May 26, 2021

This extends to other macros as well, e.g. vec! is almost exclusively used with square brackets and it would be nice if that could be enforced.

@xFrednet
Copy link
Member

This will most likely be a bit difficult to implement as Clippy only uses lint passes after macros like quote! and vec! have been expanded.

One possible solution would be:

  1. Check if an expression is the result of the expansion of a specific macro
  2. Use clippy_utils::source::snippet_opt() to get the source code as a string
  3. Check if it has the prefix quote!{ and the suffix }

However, this is a hack. It might be worth to wait until we have a better solution for the pre macro expansion checks.

@DevinR528
Copy link
Contributor Author

Do you know why adding pre_expansion lints is bad / forbidden?

pub fn register_pre_expansion_lints(store: &mut rustc_lint::LintStore) {
    // NOTE: Do not add any more pre-expansion passes. These should be removed eventually.
    ...

Does it not catch all macros for some reason, I messed with it for a bit before giving up and implementing it in the hacky way (or to follow soon) but it seemed to miss vec! and println. I could have missed something obvious and since I'm doing it the other way its not that important.

@giraffate giraffate added the A-lint Area: New lints label May 31, 2021
@xFrednet
Copy link
Member

I only have a rough idea based on a discussion here

Unfortunately, since #5518 we're actively avoiding adding new pre-expansion passes for the reasons stated there. TL;DR it's not clear they will be supported in the future in rustc. Pre-expansion passes are also quite problematic since you miss a lot of code. In this particular case, we're explicitly avoiding macros, so it would work, but otherwise this same construct would not be detected if it was generated during expansion.

There is currently nobody focusing on this topic to my knowledge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants