-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Check for duplicate attributes. #88681
Conversation
r? @wesleywiser (rust-highfive has picked a reviewer for you, use r? to override) |
cc @Aaron1011, since you recently worked on this in #87739. I'm not sure I fully understood the cc @petrochenkov who has written a lot of attribute validation. |
5972325
to
6fd2739
Compare
This comment has been minimized.
This comment has been minimized.
6fd2739
to
e8e9f7a
Compare
This comment has been minimized.
This comment has been minimized.
fcf77a6
to
d026e19
Compare
This comment has been minimized.
This comment has been minimized.
d026e19
to
7c463d4
Compare
@wesleywiser I was wondering if you'd have a chance to take a look at this. |
This comment has been minimized.
This comment has been minimized.
7c463d4
to
204d789
Compare
This comment has been minimized.
This comment has been minimized.
204d789
to
aa6331c
Compare
This comment has been minimized.
This comment has been minimized.
aa6331c
to
658c4ab
Compare
This comment has been minimized.
This comment has been minimized.
658c4ab
to
0999e6a
Compare
Going to r? @petrochenkov here for review. But feel free to reassign. |
/// This is the same as `ErrorFollowing`, except the last attribute is the | ||
/// one that is "used". This is typically used in cases like codegen | ||
/// attributes which usually only honor the last attribute. | ||
ErrorPreceding, |
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.
We shouldn't make this distinction, IMO, and should merge ErrorFollowing
into this variant, and always use the last specified value. (And error on the previously specified values.)
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.
Yea, I was a little uneasy with having different following/preceding behaviors.
I can easily remove ErrorFollowing
, but I feel like that emphasizes the inconsistencies:
- Normally when there are duplicate definitions (like items), the compiler generates an error about the following entries. Why would attributes have the opposite behavior?
- It seems inconsistent to have
WarnFollowing
andErrorPreceding
. Why have one behavior for warning and the opposite for errors?- Comparing to
unused_variables
, if you havelet x = 1; let x = 2; ...
, it will warn about the firstx
.
- Comparing to
So, perhaps it would be better to remove ErrorPreceding
instead? At least, I feel like that would be more consistent with the rest of the language. That would also match the behavior of things like the proc-macro check which generates errors for following duplicates.
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.
I mean if
#[key = "value1"]
#[key = "value2"]
NODE
was intentionally allowed, then value2
would probably override value1
, similarly to how later let
or macro_rules
"override" (shadow) earlier let
or macro_rules
, or how later command line options override earlier command line options.
So value1
would be unused.
But this requires changing other parts of the compiler to harmonize the errors and the actual behavior after the error is reported, so it's probably not a material for this PR.
This removes the duplicate check, as this is now handled in a centralized location.
@bors r=petrochenkov |
📌 Commit 36dcd4c has been approved by |
⌛ Testing commit 36dcd4c with merge b5197d2f158d95cf05009aaa8a684814d7442878... |
💔 Test failed - checks-actions |
The job Click to see the possible cause of the failure (guessed by this bot)
|
☀️ Test successful - checks-actions |
Finished benchmarking commit (f7c4829): comparison url. Summary: This benchmark run did not return any relevant changes. If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. @rustbot label: -perf-regression |
This resolves the concern in favor of prohibiting multiple instances of the attribute. This is similar to non-helper attributes as introduced in rust-lang#88681.
…-Simulacrum Resolve concern of `derive_default_enum` This resolves the concern in favor of prohibiting multiple instances of the attribute. This is similar to non-helper attributes as introduced in rust-lang#88681. `@rustbot` label +S-waiting-on-review +T-libs-api
…-Simulacrum Resolve concern of `derive_default_enum` This resolves the concern in favor of prohibiting multiple instances of the attribute. This is similar to non-helper attributes as introduced in rust-lang#88681. ``@rustbot`` label +S-waiting-on-review +T-libs-api
…ylan-DPC Fix crate_type attribute to not warn on duplicates In rust-lang#88681 I accidentally marked the `crate_type` attribute as only allowing a single attribute. However, multiple attributes are allowed (they are joined together [here](https://github.com/rust-lang/rust/blob/027a232755fa9728e9699337267f6675dfd0a8ba/compiler/rustc_interface/src/util.rs#L530-L542)). This fixes it to not report a warning if duplicates are found. Closes rust-lang#95902
…ylan-DPC Fix crate_type attribute to not warn on duplicates In rust-lang#88681 I accidentally marked the `crate_type` attribute as only allowing a single attribute. However, multiple attributes are allowed (they are joined together [here](https://github.com/rust-lang/rust/blob/027a232755fa9728e9699337267f6675dfd0a8ba/compiler/rustc_interface/src/util.rs#L530-L542)). This fixes it to not report a warning if duplicates are found. Closes rust-lang#95902
This adds some checks for duplicate attributes. In many cases, the duplicates were being ignored without error or warning. This adds several kinds of checks (see
AttributeDuplicates
enum).The motivation here is to issue unused warnings with similar reasoning for any unused lint, and to error for cases where there are conflicts.
This also adds a check for empty attribute lists in a few attributes where this causes the attribute to be ignored.
Closes #55112.