Clippy seems a bit angry

[Nicholas-Baron]

Running a simple ./x.py clippy results in a roughly 49,000 line output.

Are we going to try to repair all ~3,500 warnings or are some of them not helpful?
There is an argument to be made that some do have performance implications (unwrap_or vs unwrap_or_else, push vs push_str).
Other are entirely cosmetic (one letter bindings, removing -> ()).

An initial subset of these warnings be addressed by either

1. Refactoring the code or
2. Disabling that clippy warning

@rustbot modify labels to +C-cleanup.

[Mark-Simulacrum]

This is intentional -- clippy lints are not generally allow'd in compiler crates that we don't intend to fix, as there's many false positives in the general case. We do not check that clippy passes in CI either. IMO, it's a losing battle to try and get everything clippy complains about patched in rustc at this time, and many of the lints I at least disagree with so I don't think we would necessarily want to patch them either. I think the right path forward here is to focus on upstreaming clippy lints into rustc proper (see #53224 (comment) for the current policy on that), and then fixing as those land.

FWIW PRs fixing clippy lints are generally accepted, or at least not rejected unilaterally.

[leonardo-m]

One solution is to activate only one clippy lint at a time (and do not activate the ones that aren't good) and submit patch for each lint separately. This should simplify the review.

[matthiaskrgr]

I've been looking at the clippy output when run on the rustc codebase here and there and I by now my number of disabled lints is at around 70 :P

[Nicholas-Baron]

I am looking to work on the unwrap_or to unwrap_or_else lint, as it has performance implications (lazily constructing the replacement value will probably be more efficient).

[matthiaskrgr]

I added ./x.py clippy --only in #78821 which lets you silence all lints besides the specified ones.
Should help a bit with the overwhelming wall of default output of ./x.py clippy.

[RalfJung]

FWIW PRs fixing clippy lints are generally accepted, or at least not rejected unilaterally.

My two cents: In the past we landed clippy-fix PRs that IMO didn't really help or even reduced readability, such as replacing format!("...") by "...".to_owned() (which removed symmetry from the code just because some branches have formatting arguments and some do not). Clippy even in its default configuration is not something that should be just mindlessly applied to a codebase (and to my knowledge, the clippy authors agree that allow(clippy::*) should be used liberally). So it might be worth to talk to some stakeholders about the concrete lints you want to tackle before attempting to fix style-only lints across the codebase.

For issues that go beyond style, such as unwrap_or_else, I agree fixing them globally makes a lot of sense.

[Nicholas-Baron]

warning: `rustc_graphviz` (lib) generated 2 warnings
warning: `cargo-credential` (lib) generated 3 warnings
warning: `remote-test-client` (bin "remote-test-client") generated 8 warnings
warning: `tier-check` (bin "tier-check") generated 1 warning
warning: `remote-test-server` (bin "remote-test-server") generated 4 warnings
warning: `rustc_lexer` (lib) generated 2 warnings
warning: `clippy_utils` (lib) generated 7 warnings
warning: `core` (lib) generated 209 warnings
warning: `core` (lib) generated 209 warnings (209 duplicates)

Results from cargo +nightly clippy > clippy.txt ; rg -N 'warning: `' clippy.txt

It looks like either

1. I severely overestimated the number of warnings clippy was giving
2. clippy is much more precise with its warnings (a year of development can do that), or
3. Contributors have been writing better code.

Most of the warnings come from core and are about panicking in constants.
The suggestion given to fix is adding #![feature(const_panic)] to library/core/src/panic.rs.

[Mark-Simulacrum]

We don't generally strive to keep the repository clippy-clean, so I wouldn't say this is a bug. Specific PRs are sometimes accepted, but generally our policy has not been to blanket accept, since some of clippy's lints have false positives.

[matthiaskrgr]

Some of the very noisy clippy lints [1] have been turned off inside the repo, so that might also be change that you see in those numbers

[1] many_single_char_names collapsible_if type_complexity missing_safety_doc too_many_arguments needless_lifetimes wrong_self_convention

[Nicholas-Baron]

@matthiaskrgr That is probably the most likely reason for this.

@Mark-Simulacrum I will be closing this issue as it seems better to focus on voluntary "cleaning as we go" instead of a broader, more systematic approach.

The warning clippy made: clippy.txt