-
Notifications
You must be signed in to change notification settings - Fork 13.5k
Port the proc macro attributes to the new attribute parsing infrastructure #143607
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?
Port the proc macro attributes to the new attribute parsing infrastructure #143607
Conversation
|
||
// Not a built-in macro | ||
None => (None, helper_attrs), | ||
}; |
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.
The logic here should be equivalent to the logic above, other than that this is a bit more readable and that we use the already parsed attribute rather than parsing it from scratch
@@ -57,7 +57,7 @@ | |||
// see gated-link-args.rs | |||
// see issue-43106-gating-of-macro_escape.rs for crate-level; but non crate-level is below at "2700" | |||
// (cannot easily test gating of crate-level #[no_std]; but non crate-level is below at "2600") | |||
#![proc_macro_derive()] //~ WARN `#[proc_macro_derive]` only has an effect | |||
#![proc_macro_derive(Test)] //~ WARN `#[proc_macro_derive]` only has an effect |
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.
Note that a breaking change happened here! The code before this change would now produce an error.
Previously a proc_macro_derive
applied to the crate could have any arguments it wants, it was not checked. This PR fixes this bug, and this now errors.
I discussed this privately with @jdonszelmann, and she said this is fine to change, though we can consider doing a crater run for this just to make sure if you wish.
Some changes occurred in compiler/rustc_attr_parsing Some changes occurred in compiler/rustc_passes/src/check_attr.rs Some changes occurred in compiler/rustc_attr_data_structures |
This comment has been minimized.
This comment has been minimized.
Some changes occurred in src/tools/clippy cc @rust-lang/clippy |
This could use a little squash I think |
Not everything, but some commits are a bit small |
a6b7f55
to
432cde9
Compare
@jdonszelmann I've squashed things down a bit. I'm experimenting a bit with what commit size is best for reviewability, this was clearly too much of a good thing :P |
432cde9
to
7a60c84
Compare
r? @jdonszelmann |
|
☔ The latest upstream changes (presumably #143645) made this pull request unmergeable. Please resolve the merge conflicts. |
2b42dae
to
f8c757f
Compare
^ Rebased |
Port the proc macro attributes to the new attribute parsing infrastructure Ports `#[proc_macro]`, `#[proc_macro_attribute]`, `#[proc_macro_derive]` and `#[rustc_builtin_macro]` to the new attribute parsing infrastructure for #131229 (comment) I've split this PR into commits for reviewability, and left some comments to clarify things I did 4 related attributes in one PR because they share a lot of their code and logic, and doing them separately is kind of annoying as I need to leave both the old and new parsing in place then. r? `@oli-obk` cc `@jdonszelmann`
@traviscross what's the proper way to notify T-lang on this? What label, and could you add that? |
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
🎉 Experiment
|
Crater results, 3 reported regressions:
So no crates are affected by this change |
f8c757f
to
3b773a0
Compare
Rebased |
3b773a0
to
e0a495b
Compare
☔ The latest upstream changes (presumably #143958) made this pull request unmergeable. Please resolve the merge conflicts. |
Perfect, @rust-lang/lang crater seems clear, r=me with your approval |
Signed-off-by: Jonathan Brouwer <[email protected]>
Signed-off-by: Jonathan Brouwer <[email protected]>
Signed-off-by: Jonathan Brouwer <[email protected]>
Signed-off-by: Jonathan Brouwer <[email protected]>
Signed-off-by: Jonathan Brouwer <[email protected]>
^ Rebased |
e0a495b
to
8166ced
Compare
It looks like the breaking change here is that when applied in the wrong place, this was previously a warning and bailed out before it also checked the arguments of the attribute, and now it'll also produce an error if the attribute arguments aren't valid. @rfcbot merge |
Team member @joshtriplett 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 |
1 similar comment
@rfcbot reviewed |
🔔 This is now entering its final comment period, as per the review above. 🔔 |
Ports
#[proc_macro]
,#[proc_macro_attribute]
,#[proc_macro_derive]
and#[rustc_builtin_macro]
to the new attribute parsing infrastructure for #131229 (comment)I've split this PR into commits for reviewability, and left some comments to clarify things
I did 4 related attributes in one PR because they share a lot of their code and logic, and doing them separately is kind of annoying as I need to leave both the old and new parsing in place then.
r? @oli-obk
cc @jdonszelmann