Diagnostic summary

[ericwu17]

Closes #11532

This draft PR implements the suggestion of writing a diagnostic summary when clippy emits at least 10 diagnostics. Right now, the diagnostic summary looks like this:

---snip---
warning: manual implementation of an assign operation
   --> src/gui/list.rs:102:17
    |
102 |                 y = y - self.y;
    |                 ^^^^^^^^^^^^^^ help: replace it with: `y -= self.y`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#assign_op_pattern
    = note: `#[warn(clippy::assign_op_pattern)]` on by default

warning: length comparison to zero
   --> src/gui/list.rs:131:12
    |
131 |         if level.current_progress.0.len() > 0 {
    |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: using `!is_empty` is clearer and more explicit: `!level.current_progress.0.is_empty()`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#len_zero

warning: `trainyard-rust` (bin "trainyard-rust") generated 62 warnings

Summary of diagnostics:
clippy::expect_fun_call: 12
clippy::single_char_pattern: 11
clippy::needless_return: 5
clippy::redundant_field_names: 5
clippy::approx_constant: 3
clippy::collapsible_if: 3
clippy::single_match: 3
clippy::collapsible_else_if: 2
clippy::len_zero: 2
clippy::needless_borrow: 2
clippy::new_without_default: 2
clippy::println_empty_string: 2
clippy::uninlined_format_args: 2
clippy::unnecessary_cast: 2
clippy::while_let_on_iterator: 2
clippy::assign_op_pattern: 1
clippy::manual_flatten: 1
clippy::manual_strip: 1
clippy::manual_swap: 1
clippy::needless_late_init: 1
clippy::needless_range_loop: 1
dead_code: 1

error: could not compile `trainyard-rust` due to 3 previous errors; 62 warnings emitted

And the same diagnostic summary is generated for cargo build and cargo run. Using the flag --summary will make cargo output a summary even when < 10 diagnostics are emitted, and using --no-summary will suppress the summary.

I have marked this PR as a draft to request feedback on any major problems with the code, and also about stylistic/formatting suggestions.

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) soon.

Please see the contribution instructions for more information.

[epage]

After looking at the example output, I feel like the summary is less important than the actual messages and the summary could easily push the messages off screen.

imo the only value I feel I'd get out of it is the diagnostic name as it only gets reported in the first instance which makes it hard to find to copy/paste it for suppressing it. Even then, without a clear association with a message, I'd likely be guessing which I need.

[ericwu17]

@epage Thanks for your feedback! I don't quite understand your concern about the summary pushing messages off the screen: the summary here comes from an example where clippy emitted 65 warnings and errors (full output is here) so even without the summary, the user will only see the last few messages on the screen.

The value you'd get from the summary is an aggregate picture of how many times each lint was triggered when linting the project. (the number to the right of the colon after the diagnostic name is the count of occurrences)

[weihanglo]

I agree with epage but from a slightly different angle. The summary shouldn't be generated by default for cargo build and cargo run. My points are:

• Cargo is already a bit too verbose.
• Seeing a summary of clippy error is not the purpose of those commands.
• clippy might be missing if a toolchain is installed with a minimal rustup profile

Instead, I'll suggest only cargo clippy outputs the summary, as running cargo clippy generally means a user is looking into clippy errors. And maybe putting it behind nightly is a better way to rollout .

[weihanglo]

I would suggest avoiding adding a new flag at first place. This is a one way door decision and cannot be reverted once stabilized. We could figure it out in follow-up PRs and discussions.

[epage]

@epage Thanks for your feedback! I don't quite understand your concern about the summary pushing messages off the screen: the summary here comes from an example where clippy emitted 65 warnings and errors (full output is here) so even without the summary, the user will only see the last few messages on the screen.

I tend to resolve the last warnings first, re-running after each iteration rather than scrolling to see what is still relevant.

The value you'd get from the summary is an aggregate picture of how many times each lint was triggered when linting the project. (the number to the right of the colon after the diagnostic name is the count of occurrences)

To clarify, that is the value you would be getting. I was speaking of the value for my own personal workflow which this would negatively affect.

Also, I feel like there is an unspoken value you are getting. An aggregate picture isn't a change in action or behavior, so it'd help in making the case for this to dig deeper and answer what value you get out of an aggregate picture.

[ericwu17]

I tend to resolve the last warnings first, re-running after each iteration rather than scrolling to see what is still relevant.

@epage Thanks for providing the perspective on why this change would be detrimental to your workflow. What do you think about only @weihanglo's suggestion of only showing the summary when running cargo clippy (never show the summary for commands like cargo build or cargo check), and showing the summary only when clippy emits more than a certain amount of warnings? Initially, I thought 10 would be a good threshold, but perhaps it should be higher?

Also, I feel like there is an unspoken value you are getting. An aggregate picture isn't a change in action or behavior, so it'd help in making the case for this to dig deeper and answer what value you get out of an aggregate picture.

To answer this, the value would be for fairly medium or large projects which migrate to using clippy. When running clippy for the first time, there might be an intimidatingly large amount of errors. If clippy emits some 300 errors, then walking through the output and fixing each error would take hours, not minutes. If I'm about to begin the task of fixing clippy's errors, then I'd like to know about the nature of the work I'm about to start. Since some lint types are much easier/quicker to fix than others, I hope that a summary like the one above would help with estimating the amount of work required to address all the warnings emitted by clippy.

[epage]

@epage Thanks for providing the perspective on why this change would be detrimental to your workflow. What do you think about only @weihanglo's suggestion of only showing the summary when running cargo clippy (never show the summary for commands like cargo build or cargo check), and showing the summary only when clippy emits more than a certain amount of warnings? Initially, I thought 10 would be a good threshold, but perhaps it should be higher?

I see some people talk about replacing cargo check with cargo clippy. While I've not done that yet, I have been considering it.

I'm unsure if there is a good heuristic for when to display this. It isn't unheard of in my projects to see a hundred warnings or so in a go, depending on what is happening.

To answer this, the value would be for fairly medium or large projects which migrate to using clippy. When running clippy for the first time, there might be an intimidatingly large amount of errors. If clippy emits some 300 errors, then walking through the output and fixing each error would take hours, not minutes. If I'm about to begin the task of fixing clippy's errors, then I'd like to know about the nature of the work I'm about to start. Since some lint types are much easier/quicker to fix than others, I hope that a summary like the one above would help with estimating the amount of work required to address all the warnings emitted by clippy.

I guess for me, I could only estimate the difference in lints if I had already been dealing a lot with the same lints in a short period of time. I rarely pay attention to or remember their names.

Again, what I would like do with this information is see "oh, this lint appears 200 times, maybe I should #[allow(...)] it for now to deal with only low hanging fruit".

In general. these are occasional workflows, so an "always on" feature (with or without a heuristic) feels odd to me.

Also, a lot of these lints can be fixed in an automated fashion; I feel like it'd be a higher priority to improve the --fix workflow and then see how much of a problem remains to then decide if something is needed (a flag, a heuristic, etc).

[ericwu17]

@weihanglo @epage What would you guys think about outputting this diagnostic summary only when the user passes in a flag like --summary? Or do you feel that this diagnostic summary is too fickle of a feature to justify the addition of a new flag, which is a feature that cannot be rolled back once stable?

I do believe this feature would add value to some users' workflows (some users do care about which lints are being caught and how many times). I understand that implementing the feature as an "always on" thing would be detrimental to some other users' workflows which would not find the summary valuable, but only a nuisance getting in the way of clippy's already verbose output. How should we move this PR forward from here?

[weihanglo]

I seldom use clippy and has a similar workflow as epage's. My editor displays a list of error, and then I just tackle them one by one. Not devalue the idea, but I completely have no idea how useful it is as I did less with clippy. I would suggest looking into other linter tools outside Rust community. Make a summary of what they've done. Those could be good references to Cargo.

Apart from that, a new flag usually comes out behind -Zunstable-options before the stabilization.

[bors]

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