improve diagnostics for empty attributes by jdonszelmann · Pull Request #146653 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation17 Commits1 Checks10 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Adds a note about them not having any effect. This was previously done for feature attributes but no other attributes. In converting the feature parser I removed that note. This PR adds it back in and makes it so all attributes benefit from it.
Not blocked on #146652, either can merge first
Area: Attributes (`#[…]`, `#![…]`)
Status: Awaiting review from the assignee but also interested parties.
Relevant to the compiler team, which will review and decide on the PR/issue.
labels
r? @SparrowLii
rustbot has assigned @SparrowLii.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.
Use r? to explicitly pick a reviewer
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest, I don't think this is an improvement. As explained below, in every case the note feels not quite right. The motivation was to restore the old behaviour for feature but I don't think it is necessary.
View changes since this review
| LL | #[macro_use()] |
|---|
| | ^^ help: remove this attribute |
| | |
| = note: attribute `macro_use` with an empty list has no effect |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit surprised this is only a warning. But also, the note feels misleading: I assume macro_use() with an empty list does have an effect, i.e. the same effect as just macro_use.
A better improvement for this error message would be to say "remove these parentheses" instead of "remove this attribute".
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed a fix that makes this error better specifically when the no-argument version is available and so valid
This PR was rebased onto a different master 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📌 Commit b3631e1 has been approved by nnethercote
It is now in the queue for this repository.
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
and removed S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
labels
bors added a commit that referenced this pull request
Rollup of 5 pull requests
Successful merges:
- #146653 (improve diagnostics for empty attributes)
- #146987 (impl Ord for params and use unstable sort)
- #147101 (Use
Iterator::eqand (dogfood)eq_byin compiler and library ) - #147123 (Fix removed version numbers of
doc_auto_cfganddoc_cfg_hide) - #147149 (add joboet to library review rotation)
r? @ghost
@rustbot modify labels: rollup
rust-timer added a commit that referenced this pull request
Rollup merge of #146653 - jdonszelmann:empty-attr-diags, r=nnethercote
improve diagnostics for empty attributes
Adds a note about them not having any effect. This was previously done for feature attributes but no other attributes. In converting the feature parser I removed that note. This PR adds it back in and makes it so all attributes benefit from it.
Not blocked on #146652, either can merge first
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request
github-actions bot pushed a commit to rust-lang/rust-analyzer that referenced this pull request
flip1995 pushed a commit to flip1995/rust-clippy that referenced this pull request
Labels
Area: Attributes (`#[…]`, `#![…]`)
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Relevant to the compiler team, which will review and decide on the PR/issue.