-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Add information about group a lint belongs to #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
base: master
Are you sure you want to change the base?
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 |
// | ||
// Ideally, we'd like to use lints that are part of `unused` group as shown in the issue. | ||
// This is not possible in an ui test, because `unused` lints are enabled with `-A unused` | ||
// in such tests, and the we're testing a scenario with no modification to the default settings. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// in such tests, and the we're testing a scenario with no modification to the default settings. | |
// in such tests, and we want to test a scenario with no modification of the default settings. |
Do we want that, or are the default settings the only option here? To be honest, I wonder if this test is necessary at all, given the coverage provided by all the existing tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm you may be right, the test in current state doesn't actually test anything better than rest of the tests that capture lint output.
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):
|
Co-authored-by: Marijn Schouten <[email protected]>
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:
|
Description
Fixes: #65464
Changes Made
`#[warn(unused_variables)]` (part of `#[warn(unused)]`) on by default
`#[warn(unused_variables)]` (implied by `#[warn(unused)]`) on by default
Rationale
Implementation Notes
Examples
Case 1: Unchanged behavior when lint level is overridden
Result:
Case 2: Changed behavior for default lint levels
New output:
Previous output:
Discussion Points