Conversation
|
Some changes occurred in compiler/rustc_attr_parsing |
| this: used, | ||
| other: unused, | ||
| this: unused, | ||
| other: used, |
There was a problem hiding this comment.
This order was inconsistent with the order in Warn and WarnButFutureError
|
Aren't these technically breaking changes? |
|
Why is this a breaking change? I think erroring/warning on a different attribute should be fine right? |
|
Well in the cases where it warns, it compiles. Let's say there are two inline attributes, one says always the other never. It will warn about the innermost, apply the outermost, and be inlined. If we make the attribute keep the innermost, it warns about the outermost and not inline |
|
Ooh right, yeah you're right, good catch. |
This comment has been minimized.
This comment has been minimized.
|
@bors try |
This comment has been minimized.
This comment has been minimized.
Remove `ATTRIBUTE_ORDER`
6bafe7f to
698abd4
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@craterbot check |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
This comment has been minimized.
This comment has been minimized.
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
698abd4 to
976c53f
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
🎉 Experiment
Footnotes
|
|
All 4 regressed and 3 fixed are spurious. @rust-lang/lang Previously when multiple of the same attribute were present, we would FCW and then sometimes pick the first (outermost) and sometimes pick the second (innermost) attribute to keep, depending on the exact attribute. Most attributes already picked the first (outermost) attribute to keep, a minority picked the second (innermost). This PR makes this consistent, always keeping the first (outermost) and warning on the second (innermost) attribute. For example, this PR changes the meaning of this program, previously we would pick #[export_name = "exported_symbol_name"]
#[export_name = "exported_symbol_name2"]
fn ...This affects the following non-rustc attributes:
|
|
I would also be inclined to bump the lint to deny-by-default as part of it, since if there's code outside crater that does have this, I think it'd probably be better to force them to look at it and |
|
Sounds right. Thanks @JonathanBrouwer. @rfcbot fcp merge lang @JonathanBrouwer: What's the story on the accompanying linting? We were talking about this in the meeting. What's the most we could do to flag this to people while not breaking any legitimate use cases, e.g., macros? |
|
👍 for trying to catch obvious cases with a lint, but there may be legitimate instances of intentionally wanting to override a macro-specified attribute with a different value. |
|
@rfcbot reviewed |
|
Team member @traviscross 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. |
|
🔔 This is now entering its final comment period, as per the review above. 🔔 |
View all comments
r? @jdonszelmann
There's only a few attributes that use
KeepInnermostand I think I like the consistency of just making them allKeepOutermost