Skip to content

Port #[macro_export] to the new attribute parsing infrastructure #143857

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Port #[macro_export] to the new attribute parsing infrastructure #143857

wants to merge 1 commit into from
+206 −158

Conversation

Periodic1911
Copy link
Contributor

Ports macro_export to the new attribute parsing infrastructure for #131229 (comment)

r? @oli-obk

cc @JonathanBrouwer @jdonszelmann

@rustbot rustbot added A-attributes Area: Attributes (`#[…]`, `#![…]`) 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-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. T-rustdoc-frontend Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output. labels Jul 12, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jul 12, 2025

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann

Some changes occurred in compiler/rustc_attr_data_structures

cc @jdonszelmann

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred in compiler/rustc_passes/src/check_attr.rs

cc @jdonszelmann

Periodic1911
Periodic1911 commented Jul 12, 2025
View reviewed changes
tests/ui/attributes/invalid_macro_export_argument.rs
@@ -26,12 +22,15 @@ macro_rules! d {
}

#[macro_export()]
//~^ ERROR malformed `macro_export` attribute input
Copy link
Contributor Author

@Periodic1911 Periodic1911 Jul 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This is a breaking change! Malformed macro_export attributes have had a lint since 2023 (deny(invalid_macro_export_arguments)). This PR makes this an error.
Furthermore, #[macro_export()] has been accepted since 2023, and this PR makes that an error too.

As discussed in #142838 (comment), we can make breaking changes as long as we do a crater run. So this PR needs a crater run.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Collaborator

bors commented Jul 13, 2025

☔ The latest upstream changes (presumably #140717) made this pull request unmergeable. Please resolve the merge conflicts.

@jdonszelmann jdonszelmann mentioned this pull request Jul 13, 2025
Open
9 tasks
@jdonszelmann
Copy link
Contributor

@Periodic1911 if you rebase I'll run a crater

@jdonszelmann jdonszelmann added T-lang Relevant to the language team I-lang-nominated Nominated for discussion during a lang team meeting. labels Jul 13, 2025
@jdonszelmann
Copy link
Contributor

@rust-lang/lang this makes a long-standing warning an error with a crater run. Just wanted to notify you

@traviscross traviscross added needs-fcp This change is insta-stable, or significant enough to need a team FCP to proceed. P-lang-drag-1 Lang team prioritization drag level 1. https://rust-lang.zulipchat.com/#narrow/channel/410516-t-lang and removed T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc-frontend Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output. labels Jul 13, 2025
@rustbot rustbot added the T-rustdoc-frontend Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output. label Jul 13, 2025
@jdonszelmann
Copy link
Contributor

@bors try

bors added a commit that referenced this pull request Jul 13, 2025
@bors
Auto merge of #143857 - Periodic1911:macro-export, r=<try>
723ab94 
Port #[macro_export] to the new attribute parsing infrastructure

Ports macro_export to the new attribute parsing infrastructure for #131229 (comment)

r? `@oli-obk`

cc `@JonathanBrouwer` `@jdonszelmann`
@bors
Copy link
Collaborator

bors commented Jul 13, 2025

⌛ Trying commit ac1f122 with merge 723ab94...

@bors
Copy link
Collaborator

bors commented Jul 13, 2025

☀️ Try build successful - checks-actions
Build commit: 723ab94 (723ab942d877517caf445dafde0416803ef56501)

@jdonszelmann
Copy link
Contributor

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-143857 created and queued.
🤖 Automatically detected try build 723ab94
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 13, 2025
@craterbot
Copy link
Collaborator

🚧 Experiment pr-143857 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🎉 Experiment pr-143857 is completed!
📊 23 regressed and 8 fixed (664032 total)
📰 Open the summary report.

⚠️ If you notice any spurious failure please add them to the denylist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Jul 16, 2025
@Periodic1911
Copy link
Contributor Author

So the crater run confirmed two breaking changes, the invalid_macro_export_arguments lint and #[macro_export()].

All invalid attribute arguments are being turned into errors so this lint which is specific to the macro_export attribute will be redundant. The crater run shows that no one is using invalid arguments to macro_export, and no one is setting the lint to allowed. The invalid_macro_export_arguments lint is used by four crates, all belonging to the same author, daiwanwei, and in all cases it is used in the Cargo.toml to deny the lint, which is effectively the same as making it an error. I propose that I will send a pr to daiwanwei's crates to not use the lint and then we can remove it from rustc. Alternatively, we can leave an empty lint behind.

#[macro_export()] with the () is accepted by stable Rust with no warning, although the correct syntax would be #[macro_export]. #[macro_export()] is used be nine crates. I propose that I will make #[macro_export()] a warning with a future error.

@rust-lang/lang what do you think about my proposal for how to handle these breaking changes?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@oli-obk oli-obk

Labels
A-attributes Area: Attributes (`#[…]`, `#![…]`) I-lang-nominated Nominated for discussion during a lang team meeting. needs-fcp This change is insta-stable, or significant enough to need a team FCP to proceed. P-lang-drag-1 Lang team prioritization drag level 1. https://rust-lang.zulipchat.com/#narrow/channel/410516-t-lang S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-lang Relevant to the language team T-rustdoc-frontend Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@Periodic1911 @rustbot @rust-log-analyzer @bors @jdonszelmann @craterbot @oli-obk @traviscross