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

Warn when #[macro_export] is applied on decl macros #114413

Merged
merged 2 commits into from
Aug 8, 2023

Conversation

CohenArthur
Copy link
Contributor

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 of core's decl macros.

  • core: Remove #[macro_export] from debug_assert_matches
  • check_attrs: Warn when #[macro_export] is used on macros 2.0

@rustbot
Copy link
Collaborator

rustbot commented Aug 3, 2023

r? @cjgillot

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels 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,
);
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,
);
}
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 327361e187b

Copy link
Contributor

@cjgillot cjgillot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me with one nit

@@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be put as an extra else branch below?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@CohenArthur CohenArthur Aug 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 327361e

@cjgillot cjgillot 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 Aug 6, 2023
@CohenArthur CohenArthur force-pushed the warn-macro-export-decl-macros branch from 21e769d to 327361e Compare August 7, 2023 13:02
@@ -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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rustfmt doesn't support if let chains so it doesn't format this, but this extra space should be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 CohenArthur force-pushed the warn-macro-export-decl-macros branch from 327361e to bdf4e3d Compare August 7, 2023 19:15
@cjgillot
Copy link
Contributor

cjgillot commented Aug 7, 2023

Thanks!
@bors r+ rollup

@bors
Copy link
Contributor

bors commented Aug 7, 2023

📌 Commit bdf4e3d has been approved by cjgillot

It is now in the queue for this repository.

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 7, 2023
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
@bors bors merged commit 42cdc7d into rust-lang:master Aug 8, 2023
11 checks passed
@rustbot rustbot added this to the 1.73.0 milestone Aug 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants