Warn when #[macro_export] is applied on decl macros #114413
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @cjgillot (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
S-waiting-on-review
CohenArthur
commented
Aug 3, 2023
Comment on lines
2108
to
2122
| // special case when `#[macro_export]` is applied to a macro 2.0 | ||
| if target == Target::MacroDef { | ||
| let (macro_definition, _) = | ||
| self.tcx.hir().find(hir_id).unwrap().expect_item().expect_macro(); | ||
| let is_macro_2_0 = !macro_definition.macro_rules; | ||
|
|
||
| if is_macro_2_0 { | ||
| self.tcx.emit_spanned_lint( | ||
| UNUSED_ATTRIBUTES, | ||
| hir_id, | ||
| attr.span, | ||
| errors::MacroExport::OnDeclMacro, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Not sure if this should say macro 2.0 or rather decl macro - I'm not against changing it
Suggested change
| // special case when `#[macro_export]` is applied to a macro 2.0 | |
| if target == Target::MacroDef { | |
| let (macro_definition, _) = | |
| self.tcx.hir().find(hir_id).unwrap().expect_item().expect_macro(); | |
| let is_macro_2_0 = !macro_definition.macro_rules; | |
| if is_macro_2_0 { | |
| self.tcx.emit_spanned_lint( | |
| UNUSED_ATTRIBUTES, | |
| hir_id, | |
| attr.span, | |
| errors::MacroExport::OnDeclMacro, | |
| ); | |
| } | |
| } | |
| // special case when `#[macro_export]` is applied to a decl macro | |
| if target == Target::MacroDef { | |
| let (macro_definition, _) = | |
| self.tcx.hir().find(hir_id).unwrap().expect_item().expect_macro(); | |
| let is_decl_macro = !macro_definition.macro_rules; | |
| if is_decl_macro { | |
| self.tcx.emit_spanned_lint( | |
| UNUSED_ATTRIBUTES, | |
| hir_id, | |
| attr.span, | |
| errors::MacroExport::OnDeclMacro, | |
| ); | |
| } | |
| } |
There was a problem hiding this comment.
From a quick seach over the code, it seems to me that the term "macro 2.0" appears more often, but in identifiers, decl_macro appears more often than macro_2_0. So ideally it would be changed in that way.
There was a problem hiding this comment.
Addressed in 327361e187b
cjgillot
reviewed
Aug 6, 2023
| @@ -2105,7 +2105,23 @@ impl CheckAttrVisitor<'_> { | |||
| } | |||
|
|
|||
| fn check_macro_export(&self, hir_id: HirId, attr: &Attribute, target: Target) { | |||
| if target != Target::MacroDef { | |||
| // special case when `#[macro_export]` is applied to a macro 2.0 | |||
| if target == Target::MacroDef { | |||
There was a problem hiding this comment.
Should this be put as an extra else branch below?
There was a problem hiding this comment.
sure, that's fine with me - I wanted to keep the "MacroDef checks" grouped together but having it the else is also fine. I'll change it
There was a problem hiding this comment.
Addressed in 327361e
cjgillot
added
S-waiting-on-author
CohenArthur
force-pushed
the
warn-macro-export-decl-macros
branch
from
21e769d
to
327361e
Compare
est31
reviewed
Aug 7, 2023
| @@ -2105,7 +2105,7 @@ impl CheckAttrVisitor<'_> { | |||
| } | |||
|
|
|||
| fn check_macro_export(&self, hir_id: HirId, attr: &Attribute, target: Target) { | |||
| if target != Target::MacroDef { | |||
| if target != Target::MacroDef { | |||
There was a problem hiding this comment.
rustfmt doesn't support if let chains so it doesn't format this, but this extra space should be removed.
There was a problem hiding this comment.
woops, that managed to sneak past me. Thanks for catching it :) fixed in bdf4e3d and rebased the branch
The `debug_assert_matches` macro was marked with the `#[macro_export]` attribute, despite being a declarative macro/macro 2.0, for which the exporting rules are similar to items. In fact, `#[macro_export]` on a decl macro has no effect on its visibility.
The compiler should emit a more specific error when the `#[macro_export]` attribute is present on a decl macro, instead of silently ignoring it. This commit adds the required error message in rustc_passes/messages.ftl, as well as a note. A new variant is added to the `errors::MacroExport` enum, specifically for the case where the attribute is added to a macro 2.0.
CohenArthur
force-pushed
the
warn-macro-export-decl-macros
branch
from
327361e
to
bdf4e3d
Compare
|
Thanks! |
bors
added
S-waiting-on-bors
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Aug 8, 2023
…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#114376 (Avoid exporting __rust_alloc_error_handler_should_panic more than once.) - rust-lang#114413 (Warn when #[macro_export] is applied on decl macros) - rust-lang#114497 (Revert rust-lang#98333 "Re-enable atomic loads and stores for all RISC-V targets") - rust-lang#114500 (Remove arm crypto target feature) - rust-lang#114566 (Store the laziness of type aliases in their `DefKind`) - rust-lang#114594 (Structurally normalize weak and inherent in new solver) - rust-lang#114596 (Rename method in `opt-dist`) r? `@ghost` `@rustbot` modify labels: rollup
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The existing code checks if
#[macro_export]is being applied to an item other than a macro, and warns in that case, but fails to take into account macros 2.0/decl macros, despite the attribute having no effect on these macros.This PR adds a special case for decl macros with the aforementioned attribute, so that the warning is a bit more precise. Instead of just saying "this attribute has no effect", hint towards the fact that decl macros get exported and resolved like regular items.
It also removes a
#[macro_export]attribute which was applied on one ofcore's decl macros.debug_assert_matches