mention lint group in default level lint note by karolzwolak · Pull Request #140794 · 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

Conversation73 Commits2 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 }})

@karolzwolak

Summary

This PR updates lint diagnostics so that default-level notes now mention the lint group they belong to, if any.
Fixes: #65464.

Example

Before:

= note: `#[warn(unused_variables)]` on by default

After:

= note: `#[warn(unused_variables)]` (part of `#[warn(unused)]`) on by default

Unchanged Cases

Messages remain the same when the lint level is explicitly set, e.g.:

note: the lint level is defined here  
LL | #[warn(unused_variables)]  
   |        ^^^^^^^^^^^^^^^^  
= note: `#[warn(unused_variables)]` implied by `#[warn(unused)]`  
= note: `-W unused-variables` implied by `-W unused`  
= help: to override `-W unused` add `#[allow(unused_variables)]`  
= note: requested on the command line with `-W unused-variables`

@rustbot

r? @davidtwco

rustbot has assigned @davidtwco.
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

@rustbot rustbot added 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

May 8, 2025

@rustbot

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@karolzwolak

I'm not satisfied with tests for this change, I tried to add clippy ui test, but I wasn't able to do so, because they use -D warnings option, which triggers different messages. I'd grateful for ideas how to get around this. Similarly, unused group cannot be tested, because of -A unused option is used in ui tests.

@karolzwolak

I gathered my thoughts about possible approaches

Possible Approaches

Option 1: Status Quo (No Change)

Current behavior:

= note: `#[warn(unused_variables)]` on by default

Pros:

Cons:

Option 2: Group Annotation (Current PR)

Example:

= note: `#[warn(unused_variables)]` (part of `#[warn(unused)]`) on by default

Alternatives:

Pros:

Cons:

Option 3: Group Override Lint

Behavior:
Suggests a group-level override when multiple members of the same group are explicitly overridden:

#[allow(unused_variables)] #[allow(unused_imports)]

Could trigger a suggestion like:

Pros:

Cons:

Example:

= note: `#[warn(unused_variables)]` on by default
         see https://doc.rust-lang.org/rustc/lints/... for details

Pros:

Cons:

Questions for Discussion

  1. Terminology:
    • Which phrasing feels clearest:
      “implied by”, “from group”, “part of”, “member of”?
  2. Configuration Defaults:
    • For the group annotation message, should it trigger for external lints?
    • For the override lint, what should be the default behavior?
      * Allow-by-default (opt-in)
      * Warn-by-default (opt-out)

@karolzwolak

cc @hkBst
I'd like to hear your thoughts.

@Centri3

I'm not satisfied with tests for this change, I tried to add clippy ui test, but I wasn't able to do so, because they use -D warnings option, which triggers different messages. I'd grateful for ideas how to get around this. Similarly, unused group cannot be tested, because of -A unused option is used in ui tests.

We already have a few tests that are full workspaces/crates (thus have access to RUSTFLAGS). I see no reason against that unless compiletest overwrites the lint levels we set there.

A better option, if possible, is to have RUSTFLAGS passed by compiletest. There is //@compile-flags which might work, I'm not sure.

@Centri3

Option 2: Group Annotation (Current PR)

So, about the topic at hand. This current PR feels like it adds a ton of noise for not much benefit tbh, raising awareness for lint groups is -fine- but they aren't really all that commonly needed. Extra noise and confusion just so the user knows about unused is a bit overkill imo.

I was going to suggest the alternative of only doing this when the user specifies deny(unused) or something but it turns out rustc already does this (I knew I recognized this from somewhere). Really, that's the only place I could see there having been potential confusion.

All that said, clippy adding links to documentation everywhere is quite a bit of noise too.

Option 3: Group Override Lint

This will have huge issues in Clippy, whose categories are just for lint levels and aren't really related lints at all. So we do definitely need that opt-out behavior.

This should probably be special cased to unused only—that said, future-incompatible and nonstandard-style seem reasonable enough too

Option 4: Link to Documentation

This is already done for clippy lints and is pretty standard. These are auto-generated and the same can probably be done in rustc as well. Seems reasonable to me considering prior art. It would be nice if there was something like rustc's error codes index to make this a bit more concise, that is perhaps something to look into if you go down this route.

@Centri3

Option 3: Group Override Lint

sup dawg, I heard you like your linters, so I added a linter for your linter so you can lint your lints while you lint

hkBst

hkBst

@hkBst

Hi @karolzwolak,

I really like your work, and the current PR seems like the most natural option of the proposed alternatives. I think this is a very good way of teaching users about the existence of various lint groups and adds a lot of value to the compiler messages.

Let's consider the following program, which contains all three case of individual lint specified, lint group specified, default lint:

pub fn unused_var() { #[warn(unused_variables)] let x = 5; }

pub fn unused() { #[warn(unused)] let x = 5; }

pub fn default_() { let x = 5; }

My proposed change from current (stable) output would be:

warning: unused variable: `x`
 --> src/lib.rs:3:9
  |
3 |     let x = 5;
  |         ^ help: if this is intentional, prefix it with an underscore: `_x`
  |
note: the lint level is defined here
 --> src/lib.rs:2:12
  |
2 |     #[warn(unused_variables)]
  |            ^^^^^^^^^^^^^^^^
+  = note: lint `unused_variables` is part of lint group `unused`

warning: unused variable: `x`
 --> src/lib.rs:8:9
  |
8 |     let x = 5;
  |         ^ help: if this is intentional, prefix it with an underscore: `_x`
  |
note: the lint level is defined here
 --> src/lib.rs:7:12
  |
7 |     #[warn(unused)]
  |            ^^^^^^
-  = note: `#[warn(unused_variables)]` implied by `#[warn(unused)]`
+  = note: `#[warn(unused)]` implies `#[warn(unused_variables)]`

warning: unused variable: `x`
  --> src/lib.rs:12:9
   |
12 |     let x = 5;
   |         ^ help: if this is intentional, prefix it with an underscore: `_x`
   |
-   = note: `#[warn(unused_variables)]` on by default
+   = note: default `#[warn(unused)]` implies `#[warn(unused_variables)]`

This last case with "default", assumes that defaults are lint groups instead of individual lints. I'm not sure that is (always) the case. If this is wrong, then perhaps instead just add this line (like for the first function):

+  = note: lint `unused_variables` is part of lint group `unused`

@karolzwolak

This last case with "default", assumes that defaults are lint groups instead of individual lints. I'm not sure that is (always) the case. If this is wrong, then perhaps instead just add this line (like for the first function):

+  = note: lint `unused_variables` is part of lint group `unused`

In my understanding, individual lints are set on default, and the groups are a way to refer to bunch of lints at the same time, but I could be wrong. However I really like the your suggestions for the messages, and perhaps this slight inaccuracy (if I'm right) is okay here, because it makes the messages consistent.

@karolzwolak

My proposed change from current (stable) output would be:

warning: unused variable: `x`
--> src/lib.rs:3:9
 |
3 |     let x = 5;
 |         ^ help: if this is intentional, prefix it with an underscore: `_x`
 |
note: the lint level is defined here
--> src/lib.rs:2:12
 |
2 |     #[warn(unused_variables)]
 |            ^^^^^^^^^^^^^^^^
+  = note: lint `unused_variables` is part of lint group `unused`

warning: unused variable: `x`
--> src/lib.rs:8:9
 |
8 |     let x = 5;
 |         ^ help: if this is intentional, prefix it with an underscore: `_x`
 |
note: the lint level is defined here
--> src/lib.rs:7:12
 |
7 |     #[warn(unused)]
 |            ^^^^^^
-  = note: `#[warn(unused_variables)]` implied by `#[warn(unused)]`
+  = note: `#[warn(unused)]` implies `#[warn(unused_variables)]`

warning: unused variable: `x`
 --> src/lib.rs:12:9
  |
12 |     let x = 5;
  |         ^ help: if this is intentional, prefix it with an underscore: `_x`
  |
-   = note: `#[warn(unused_variables)]` on by default
+   = note: default `#[warn(unused)]` implies `#[warn(unused_variables)]`

I think these messages look great, and might be the way to go, since as @Centri3 pointed out, lint wouldn't work too well in this example. @Centri3 thanks for providing feedback, what do you thing about these messages?

EDIT:
What about external lints?
This change would make messages for rustc lints consistent, but that would make them inconsistent with external lints like clippy, unless we'd also change the messages for them. It think I'd be great, but

This will have huge issues in Clippy, whose categories are just for lint levels and aren't really related lints at all.

davidtwco

@rustbot rustbot added S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.

and removed S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

labels

May 27, 2025

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@bors

This comment was marked as resolved.

@bors

This comment was marked as resolved.

@davidtwco

Apologies for my delay in getting back to this, it's looking pretty good, could you rebase and I'll do one last pass?

@fmease

Thanks!

@bors r=davidtwco

Alleviate pressure on queue
@bors rollup-

@bors

📌 Commit c5ff4bf has been approved by davidtwco

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-author

Status: This is awaiting some action (such as code changes or more information) from the author.

labels

Aug 19, 2025

@karolzwolak karolzwolak changed the titleAdd information about group a lint belongs to include group info in default level lint note messages

Aug 19, 2025

@bors

@bors bors added S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.

and removed S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

labels

Aug 19, 2025

@karolzwolak

@karolzwolak

@karolzwolak

I rebased, and re-blessed tests, and also used the opportunity to revise and unify the commit message, PR title and test filename.

@rustbot

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.

@karolzwolak karolzwolak changed the titleinclude group info in default level lint note messages mention lint group in default level lint note

Aug 19, 2025

@fmease

Thanks again! @bors r=davidtwco

@bors

📌 Commit d14b83e has been approved by davidtwco

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-author

Status: This is awaiting some action (such as code changes or more information) from the author.

labels

Aug 19, 2025

jhpratt added a commit to jhpratt/rust that referenced this pull request

Aug 19, 2025

@jhpratt

…, r=davidtwco

mention lint group in default level lint note

Summary

This PR updates lint diagnostics so that default-level notes now mention the lint group they belong to, if any. Fixes: rust-lang#65464.

Example

fn main() {
    let x = 5;
}

Before:

= note: `#[warn(unused_variables)]` on by default

After:

= note: `#[warn(unused_variables)]` (part of `#[warn(unused)]`) on by default

Unchanged Cases

Messages remain the same when the lint level is explicitly set, e.g.:

jhpratt added a commit to jhpratt/rust that referenced this pull request

Aug 19, 2025

@jhpratt

…, r=davidtwco

mention lint group in default level lint note

Summary

This PR updates lint diagnostics so that default-level notes now mention the lint group they belong to, if any. Fixes: rust-lang#65464.

Example

fn main() {
    let x = 5;
}

Before:

= note: `#[warn(unused_variables)]` on by default

After:

= note: `#[warn(unused_variables)]` (part of `#[warn(unused)]`) on by default

Unchanged Cases

Messages remain the same when the lint level is explicitly set, e.g.:

This was referenced

Aug 19, 2025

github-actions bot pushed a commit to rust-lang/miri that referenced this pull request

Aug 20, 2025

@bors

bors added a commit that referenced this pull request

Aug 20, 2025

@bors

Rollup of 13 pull requests

Successful merges:

r? @ghost @rustbot modify labels: rollup

rust-timer added a commit that referenced this pull request

Aug 20, 2025

@rust-timer

Rollup merge of #140794 - karolzwolak:allow-unused-doc-65464, r=davidtwco

mention lint group in default level lint note

Summary

This PR updates lint diagnostics so that default-level notes now mention the lint group they belong to, if any. Fixes: #65464.

Example

fn main() {
    let x = 5;
}

Before:

= note: `#[warn(unused_variables)]` on by default

After:

= note: `#[warn(unused_variables)]` (part of `#[warn(unused)]`) on by default

Unchanged Cases

Messages remain the same when the lint level is explicitly set, e.g.:

github-actions bot pushed a commit to rust-lang/miri that referenced this pull request

Aug 21, 2025

@bors

Labels

S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

T-clippy

Relevant to the Clippy team.

T-compiler

Relevant to the compiler team, which will review and decide on the PR/issue.