-
Notifications
You must be signed in to change notification settings - Fork 13.7k
mention lint group in default level lint note #140794
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
Conversation
r? @davidtwco rustbot has assigned @davidtwco. Use |
Some changes occurred in src/tools/clippy cc @rust-lang/clippy |
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 |
I gathered my thoughts about possible approaches Possible ApproachesOption 1: Status Quo (No Change)Current behavior:
Pros:
Cons:
Option 2: Group Annotation (Current PR)Example:
Alternatives:
Pros:
Cons:
Option 3: Group Override LintBehavior: #[allow(unused_variables)]
#[allow(unused_imports)] Could trigger a suggestion like: #[allow(unused)] Pros:
Cons:
Option 4: Link to DocumentationExample:
Pros:
Cons:
Questions for Discussion
|
cc @hkBst |
We already have a few tests that are full workspaces/crates (thus have access to A better option, if possible, is to have |
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 I was going to suggest the alternative of only doing this when the user specifies All that said, clippy adding links to documentation everywhere is quite a bit of noise too.
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
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. |
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 |
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:
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):
|
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. |
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:
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment was marked as resolved.
This comment was marked as resolved.
bd18376
to
71d486a
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Apologies for my delay in getting back to this, it's looking pretty good, could you rebase and I'll do one last pass? |
71d486a
to
5cce81b
Compare
@fmease Squashed into 2 commits. You're right about the PR description, will update it shortly. |
☔ The latest upstream changes (presumably #145599) made this pull request unmergeable. Please resolve the merge conflicts. |
c5ff4bf
to
d14b83e
Compare
I rebased, and re-blessed tests, and also used the opportunity to revise and unify the commit message, PR title and test filename. |
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. |
Thanks again! @bors r=davidtwco |
…, 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 ```rust 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.: * Attribute on the lint `#[warn(unused_variables)]`: ``` note: the lint level is defined here LL | #[warn(unused_variables)] | ^^^^^^^^^^^^^^^^ ``` * Attribute on the group `#[warn(unused)]:`: ``` = note: `#[warn(unused_variables)]` implied by `#[warn(unused)]` ``` * CLI option `-W unused`: ``` = note: `-W unused-variables` implied by `-W unused` = help: to override `-W unused` add `#[allow(unused_variables)]` ``` * CLI option `-W unused-variables`: ``` = note: requested on the command line with `-W unused-variables` ```
…, 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 ```rust 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.: * Attribute on the lint `#[warn(unused_variables)]`: ``` note: the lint level is defined here LL | #[warn(unused_variables)] | ^^^^^^^^^^^^^^^^ ``` * Attribute on the group `#[warn(unused)]:`: ``` = note: `#[warn(unused_variables)]` implied by `#[warn(unused)]` ``` * CLI option `-W unused`: ``` = note: `-W unused-variables` implied by `-W unused` = help: to override `-W unused` add `#[allow(unused_variables)]` ``` * CLI option `-W unused-variables`: ``` = note: requested on the command line with `-W unused-variables` ```
Update cargo 28 commits in 840b83a10fb0e039a83f4d70ad032892c287570a..71eb84f21aef43c07580c6aed6f806a6299f5042 2025-07-30 13:59:19 +0000 to 2025-08-17 17:18:56 +0000 - update tests to match lint message changes from rust-lang/rust#140794 (rust-lang/cargo#15849) - chore: downgrade to [email protected] (rust-lang/cargo#15851) - Reorder `lto` options in profiles.md (rust-lang/cargo#15841) - feat(unstable): add -Zbuild-analysis unstable feature (rust-lang/cargo#15845) - refactor(unstable): group stabilized features (rust-lang/cargo#15846) - Fixes error while running the cargo clippy --all-targets -- -D warning (rust-lang/cargo#15843) - Clarify that `cargo doc --no-deps` is cumulative and won’t delete prev (rust-lang/cargo#15800) - docs: Formatting and cross-linking to build-dir/target-dir docs (rust-lang/cargo#15840) - Stabilize `build.build-dir` (rust-lang/cargo#15833) - make resolve features public for cargo-as-a-library (rust-lang/cargo#15835) - chore(deps): bump slab from 0.4.10 to 0.4.11 (rust-lang/cargo#15832) - chore: remove x86_64-apple-darwin from CI and tests (rust-lang/cargo#15831) - chore(deps): update msrv (3 versions) to v1.87 (rust-lang/cargo#15819) - perf(package): Always reuse the workspace's target-dir (rust-lang/cargo#15783) - More helpful error for invalid cargo-features = [] (rust-lang/cargo#15781) - Add initial integration for `--json=timings` behing `-Zsection-timings` (rust-lang/cargo#15780) - add is_inherited methods to InheritableDependency and InheritableField (rust-lang/cargo#15828) - chore(deps): update compatible (rust-lang/cargo#15804) - docs(unstable): Link out to the Plumbing commands effort (rust-lang/cargo#15821) - chore(deps): update cargo-semver-checks to v0.43.0 (rust-lang/cargo#15825) - test(build-std): relax the thread name assertion (rust-lang/cargo#15822) - chore(deps): update msrv (1 version) to v1.89 (rust-lang/cargo#15815) - Update semver tests for 1.89 (rust-lang/cargo#15816) - Accessing each build script's `OUT_DIR` and in the correct order (rust-lang/cargo#15776) - chore: bump to 0.92.0; update changelog (rust-lang/cargo#15807) - docs: `-Zpackage-workspace` has been stabilized (rust-lang/cargo#15808) - chore(deps): update rust crate cargo_metadata to 0.21.0 (rust-lang/cargo#15795) - docs(build-rs): Fix broken intra-doc links (rust-lang/cargo#15810)
Rollup of 13 pull requests Successful merges: - #139357 (Fix parameter order for `_by()` variants of `min` / `max`/ `minmax` in `std::cmp`) - #140314 (Rustdoc: typecheck scrape-examples.js) - #140794 (mention lint group in default level lint note) - #145006 (Clarify EOF handling for `BufRead::skip_until`) - #145252 (Demote x86_64-apple-darwin to Tier 2 with host tools) - #145359 (Fix bug where `rustdoc-js` tester would not pick the right `search.js` file if there is more than one) - #145381 (Implement feature `int_lowest_highest_one` for integer and NonZero types) - #145417 (std_detect: RISC-V platform guide documentation) - #145531 (Add runtime detection for APX-F and AVX10) - #145619 (`std_detect`: Use `rustc-std-workspace-*` to pull in `compiler-builtins`) - #145622 (Remove the std workspace patch for `compiler-builtins`) - #145623 (Pretty print the name of an future from calling async closure) - #145626 (add a fallback implementation for the `prefetch_*` intrinsics ) r? `@ghost` `@rustbot` modify labels: rollup
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 ```rust 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.: * Attribute on the lint `#[warn(unused_variables)]`: ``` note: the lint level is defined here LL | #[warn(unused_variables)] | ^^^^^^^^^^^^^^^^ ``` * Attribute on the group `#[warn(unused)]:`: ``` = note: `#[warn(unused_variables)]` implied by `#[warn(unused)]` ``` * CLI option `-W unused`: ``` = note: `-W unused-variables` implied by `-W unused` = help: to override `-W unused` add `#[allow(unused_variables)]` ``` * CLI option `-W unused-variables`: ``` = note: requested on the command line with `-W unused-variables` ```
Rollup of 13 pull requests Successful merges: - rust-lang/rust#139357 (Fix parameter order for `_by()` variants of `min` / `max`/ `minmax` in `std::cmp`) - rust-lang/rust#140314 (Rustdoc: typecheck scrape-examples.js) - rust-lang/rust#140794 (mention lint group in default level lint note) - rust-lang/rust#145006 (Clarify EOF handling for `BufRead::skip_until`) - rust-lang/rust#145252 (Demote x86_64-apple-darwin to Tier 2 with host tools) - rust-lang/rust#145359 (Fix bug where `rustdoc-js` tester would not pick the right `search.js` file if there is more than one) - rust-lang/rust#145381 (Implement feature `int_lowest_highest_one` for integer and NonZero types) - rust-lang/rust#145417 (std_detect: RISC-V platform guide documentation) - rust-lang/rust#145531 (Add runtime detection for APX-F and AVX10) - rust-lang/rust#145619 (`std_detect`: Use `rustc-std-workspace-*` to pull in `compiler-builtins`) - rust-lang/rust#145622 (Remove the std workspace patch for `compiler-builtins`) - rust-lang/rust#145623 (Pretty print the name of an future from calling async closure) - rust-lang/rust#145626 (add a fallback implementation for the `prefetch_*` intrinsics ) r? `@ghost` `@rustbot` modify labels: rollup
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:
After:
Unchanged Cases
Messages remain the same when the lint level is explicitly set, e.g.:
Attribute on the lint
#[warn(unused_variables)]
:Attribute on the group
#[warn(unused)]:
:CLI option
-W unused
:CLI option
-W unused-variables
: