-
Notifications
You must be signed in to change notification settings - Fork 13.6k
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.
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
base: master
Are you sure you want to change the base?
Conversation
Some changes occurred in compiler/rustc_attr_parsing Some changes occurred in compiler/rustc_attr_data_structures Some changes occurred in src/tools/clippy cc @rust-lang/clippy Some changes occurred in compiler/rustc_passes/src/check_attr.rs |
@@ -26,12 +22,15 @@ macro_rules! d { | |||
} | |||
|
|||
#[macro_export()] | |||
//~^ ERROR malformed `macro_export` attribute input |
There was a problem hiding this comment.
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.
This comment has been minimized.
This comment has been minimized.
12474a6
to
bb6c5dd
Compare
This comment has been minimized.
This comment has been minimized.
☔ The latest upstream changes (presumably #140717) made this pull request unmergeable. Please resolve the merge conflicts. |
@Periodic1911 if you rebase I'll run a crater |
@rust-lang/lang this makes a long-standing warning an error with a crater run. Just wanted to notify you |
bb6c5dd
to
92071cc
Compare
92071cc
to
ac1f122
Compare
@bors try |
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`
☀️ Try build successful - checks-actions |
@craterbot check |
We discussed this in the lang team triage meeting. We agreed on a plan to upgrade If and when we upgrade this lint to a hard error, we can add it to the removed_lints list and that should handle it correctly. FCP to ratify this plan: @rfcbot fcp merge |
Team member @tmandry has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns. |
@rfcbot reviewed |
🔔 This is now entering its final comment period, as per the review above. 🔔 |
Oh thanks scottmcm, I didn't know the dependencies tab showed results that aren't in the root results tab. Sorry, I'm new to this! 🙇♀️ And thank you for checking my work |
The final comment period, with a disposition to merge, as per the review above, is now complete. As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed. This will be merged soon. |
@Periodic1911 FCP finished. if you rebase we can go for merge! :) |
ac1f122
to
f9b9221
Compare
@@ -4187,8 +4187,13 @@ declare_lint! { | |||
/// You can't have multiple arguments in a `#[macro_export(..)]`, or mention arguments other than `local_inner_macros`. | |||
/// | |||
pub INVALID_MACRO_EXPORT_ARGUMENTS, | |||
Warn, | |||
Deny, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This lint is now deny by default and FCW
@jdonszelmann rebased! And I made some changes to the parser error handling in accordance with the lang team's decision. Did I comply exactly with what they had in mind? |
@@ -0,0 +1,40 @@ | |||
Future incompatibility report: Future breakage diagnostic: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although the invalid_macro_export_argument.rs
test passes (check-pass
) with #![allow(invalid_macro_export_arguments)]
, it also prints FCWs to stderr
The job Click to see the possible cause of the failure (guessed by this bot)
|
Ports macro_export to the new attribute parsing infrastructure for #131229 (comment)
r? @oli-obk
cc @JonathanBrouwer @jdonszelmann