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

Fix unused attributes on macro_rules. #85312

Merged
merged 1 commit into from
May 16, 2021

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented May 15, 2021

The unused_attributes lint wasn't firing on attributes of macro_rules definitions. The consequence is that many attributes are silently ignored on macro_rules. The reason is that unused_attributes is a late-lint pass, and only has access to the HIR, which does not have macro_rules definitions.

My solution here is to change non_exported_macro_attrs to be macro_attrs (a list of all attributes used for macro_rules, instead of just those for macro_export), and then to check this list in the unused_attributes lint. There are a number of alternate approaches, but this seemed the most reliable and least invasive. I am open to completely different approaches, though.

One concern is that I don't fully understand the implications of extending non_exported_macro_attrs to include non-exported macros. That list was originally added in #62042 to handle stability attributes, so I suspect it was just an optimization since that was all that was needed. It was later extended to be included in SVH in #83901. #80641 also added a use to check for invalid attributes, which seems a little odd to me (it didn't validate non-exported macros, and seems highly specific).

Overall, there doesn't seem to be a clear story of when unused_attributes should be used versus an error like E0518. I considered alternatively using an "allow list" of built-in attributes that can be used on macro_rules (allow, warn, deny, forbid, cfg, cfg_attr, macro_export, deprecated, doc), but I feel like that could be a pain to maintain.

Some built-in attributes already present hard-errors when used with macro_rules. These are each hard-coded in various places:

  • derive
  • test and bench
  • proc_macro and proc_macro_derive
  • inline
  • global_allocator

The primary motivation is that I sometimes see people use #[macro_use] in front of macro_rules, which indicates there is some confusion out there (evident that there was even a case of it in rustc).

@rust-highfive
Copy link
Collaborator

r? @petrochenkov

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 15, 2021
@petrochenkov
Copy link
Contributor

petrochenkov commented May 15, 2021

I am open to completely different approaches, though.

The proper solution is to keep all macros in HIR like any other items (#73754 (comment)), it would help with other issues as well.
That's a significantly larger work though.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 15, 2021
@ehuss
Copy link
Contributor Author

ehuss commented May 15, 2021

The proper solution is to keep all macros in HIR

I'd be willing to try that if you want. Is that something you'd be willing to review? Does it just boil down to adding macros to hir::ItemKind and then dealing with the consequences?

@petrochenkov petrochenkov added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 16, 2021
@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented May 16, 2021

📌 Commit 5bbc240 has been approved by petrochenkov

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 16, 2021
@petrochenkov
Copy link
Contributor

Is that something you'd be willing to review?

Yes, sure.

Does it just boil down to adding macros to hir::ItemKind and then dealing with the consequences?

Yes, with "dealing with the consequences" probably being the harder part.

@bors
Copy link
Contributor

bors commented May 16, 2021

⌛ Testing commit 5bbc240 with merge fe72845...

@bors
Copy link
Contributor

bors commented May 16, 2021

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing fe72845 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 16, 2021
@bors bors merged commit fe72845 into rust-lang:master May 16, 2021
@rustbot rustbot added this to the 1.54.0 milestone May 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants