Skip to content

improve diagnostics for empty attributes #146653

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 29, 2025
Merged

improve diagnostics for empty attributes #146653

merged 1 commit into from
Sep 29, 2025
+46 −16

Conversation

jdonszelmann
Copy link
Contributor

@jdonszelmann jdonszelmann commented Sep 16, 2025 •
edited by rustbot
Loading

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

@rustbot rustbot added A-attributes Area: Attributes (`#[…]`, `#![…]`) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 16, 2025
@rustbot
Copy link
Collaborator

rustbot commented Sep 16, 2025

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

@jdonszelmann jdonszelmann mentioned this pull request Sep 16, 2025
Open
@jdonszelmann
Copy link
Contributor Author

r? compiler

@rustbot rustbot assigned BoxyUwU and unassigned SparrowLii Sep 18, 2025
@bors
Copy link
Collaborator

bors commented Sep 22, 2025

☔ The latest upstream changes (presumably #146862) made this pull request unmergeable. Please resolve the merge conflicts.

@jdonszelmann
Copy link
Contributor Author

r? nnethercote

@rustbot rustbot assigned nnethercote and unassigned BoxyUwU Sep 22, 2025
nnethercote
nnethercote reviewed Sep 22, 2025
View reviewed changes
Copy link
Contributor

@nnethercote nnethercote left a comment •
edited
Loading

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

tests/ui/macros/macro-use-all-and-none.stderr Outdated
LL | #[macro_use()]
| ^^ help: remove this attribute
|
= note: attribute `macro_use` with an empty list has no effect
Copy link
Contributor

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".

Copy link
Contributor Author

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

@rustbot
Copy link
Collaborator

rustbot commented Sep 27, 2025

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.

jdonszelmann
jdonszelmann commented Sep 27, 2025
View reviewed changes
Copy link
Contributor Author

@jdonszelmann jdonszelmann left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

github is borken (this comment was made as a dummy to get around a UI bug)

View changes since this review

@jdonszelmann
Copy link
Contributor Author

@rustbot review

@jdonszelmann
Copy link
Contributor Author

(@nnethercote)

@nnethercote
Copy link
Contributor

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Sep 29, 2025

📌 Commit b3631e1 has been approved by nnethercote

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors 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 Sep 29, 2025
@Zalathar Zalathar mentioned this pull request Sep 29, 2025
Merged
bors added a commit that referenced this pull request Sep 29, 2025
@bors
Auto merge of #147151 - Zalathar:rollup-w81rn0j, r=Zalathar
21a13b8 
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::eq` and (dogfood) `eq_by` in compiler and library )
 - #147123 (Fix removed version numbers of `doc_auto_cfg` and `doc_cfg_hide`)
 - #147149 (add joboet to library review rotation)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit cf07cce into rust-lang:master Sep 29, 2025
10 checks passed
@rustbot rustbot added this to the 1.92.0 milestone Sep 29, 2025
rust-timer added a commit that referenced this pull request Sep 29, 2025
@rust-timer
Unrolled build for #146653
1d14c61 
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](#146652) 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 Sep 30, 2025
@bors
Auto merge of #147151 - Zalathar:rollup-w81rn0j, r=Zalathar
8422b2c 
Rollup of 5 pull requests

Successful merges:

 - rust-lang/rust#146653 (improve diagnostics for empty attributes)
 - rust-lang/rust#146987 (impl Ord for params and use unstable sort)
 - rust-lang/rust#147101 (Use `Iterator::eq` and (dogfood) `eq_by` in compiler and library )
 - rust-lang/rust#147123 (Fix removed version numbers of `doc_auto_cfg` and `doc_cfg_hide`)
 - rust-lang/rust#147149 (add joboet to library review rotation)

r? `@ghost`
`@rustbot` modify labels: rollup
github-actions bot pushed a commit to rust-lang/rust-analyzer that referenced this pull request Oct 2, 2025
@bors
Auto merge of #147151 - Zalathar:rollup-w81rn0j, r=Zalathar
84e54dd 
Rollup of 5 pull requests

Successful merges:

 - rust-lang/rust#146653 (improve diagnostics for empty attributes)
 - rust-lang/rust#146987 (impl Ord for params and use unstable sort)
 - rust-lang/rust#147101 (Use `Iterator::eq` and (dogfood) `eq_by` in compiler and library )
 - rust-lang/rust#147123 (Fix removed version numbers of `doc_auto_cfg` and `doc_cfg_hide`)
 - rust-lang/rust#147149 (add joboet to library review rotation)

r? `@ghost`
`@rustbot` modify labels: rollup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nnethercote nnethercote nnethercote approved these changes

Assignees

@nnethercote nnethercote

Labels
A-attributes Area: Attributes (`#[…]`, `#![…]`) S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.92.0
Development

Successfully merging this pull request may close these issues.
6 participants
@jdonszelmann @rustbot @bors @nnethercote @BoxyUwU @SparrowLii