Rust-clippy: Better testing of clippy lints before enabling them by default

[dvc94ch]

needless-collect and mutable-key-type are two recent examples or buggy lints. (By buggy I mean a large number of false positives)

[matthiaskrgr]

I see it the way that there is no better way to test a lint than releasing it into the wild :)
If we are not shortly before a beta cutoff, we can always change the lint category afterwards.

[dvc94ch]

Those lints made it to stable. Don't know why they weren't caught earlier, but to me having a codebase light up with false positives is a problem. needless-collect doesn't take into account borrowing rules and mutable-key-type fires when using Bytes as a key.

[ebroto]

Like @matthiaskrgr comments, interaction from the community has worked acceptably for Clippy. Getting lints right is hard and we need people to try them and report the problems.

That being said, I think this workflow has a negative impact on Clippy.

• The majority of the people will not open an issue when they find a false positive. The number of impacted people is probably way higher.
• Some less experienced users will not think there's something wrong with the lint and will try to "correct" their code following the wrong suggestions. Some of them will not even know they can allow the lint. The experience is not great.
• Some veteran users dislike the false positive rate of Clippy and would not recommend it. This affects Clippy's credibility in general.

Ideally, if Clippy had the correct amount of people and resources, I think warn/deny-by-default lints would benefit from a stabilization process similar to the feature gates for lang/lib features.

• These lints would be enabled on nightly only, possibly behind a feature gate and/or a flag in Clippy. They would be tested by people that already accept some instability.
• Eventually after some time, someone would push for stabilization. We would get a crater run, and analyze the results. It would be possible to decide if the lint is ready. If it is, it would go through an FCP process.
• Buggy lints would remain in nightly, and could be removed without breaking stability guarantees.

I'm not sure if we want/can implement that process or something similar currently due to the aforementioned lack of resources, but I personally agree that this is something we should think about.

[dvc94ch]

It may accumulate to a lot of effort if the lint gets recursively disabled in ci's, because for example a library uses Bytes internally. And if people recursively disable it, it's likely to stay that way for a long time, so once it is ready it won't be very effective. I guess not all lints are that far reaching. I've used clippy for a while and haven't had any grievances so far, but those kind of accumulated recently which is why I opened the issue.

[flip1995]

Yeah, we definitely need to improve the process somehow. This is something I have in mind for Clippy for 2021. On the one hand usability and on the other getting more people for maintaining/triaging. A process like the one @ebroto suggested above might actually make sense.

I think one problem is, that fewer people use nightly every year. Which speaks for the Rust language and ecosystem, but makes the process of "just putting a lint out there with 2-4 weeks in nightly and hoping enough people complain" harder and harder.

Spoiler: general governance, recruiting and allocating resources across the Rust ecosystem is also on the radar of the core team and the Rust foundation. So I'm positive we'll improve the situation of Clippy during 2021. Trying to start with this process now with Covid still raging in most countries and the holidays coming won't make much sense though.

[matthiaskrgr]

At cppcheck (static analysis for c/c++) we had something called "daca" which was essentially a big loop of downloading a package source from the debian archives, running cppcheck on it and saving the results in one big file (a bit like rusts crater). I made a script that would download these cppcheck-logs so I could diff them and see if there were suddenly lint warnings appearing/disappearing.
This was running on a PI (which took around a month to iterate through all the packages).

Later Daniel wrote daca2 where anyone could launch a "checking" client which would pull cppcheck from source, build it, fetch a source package from the queue and send back the results to a central server, so multiple people could "donate" cpu power to the project
http://cppcheck1.osuosl.org:8000/
I guess this is a bit more like distributed crater but everyone could contribute.
I would love to have something like that for clippy! :)
Clippys multi-line error messages make diffing a bit difficult though.

Some time ago I spent a bit of time trying to make something like a run-clippy-on-crates-and-diff-the-stdout program but I don't remember what its state is.
https://github.com/matthiaskrgr/raca/blob/master/src/main.rs

It seems to extract one line for each clippy warning
cargo-0.35.0 clippy::module_name_repetitions src/cargo/util/toml/mod.rs:661:1->src/cargo/util/toml/mod.rs:666:1 cargo-0.35.0 clippy::pub_enum_variant_names src/cargo/util/errors.rs:166:5->src/cargo/util/errors.rs:170:5 cargo-0.35.0 clippy::redundant_static_lifetimes src/cargo/ops/cargo_package.rs:34:24 cargo-0.35.0 clippy::similar_names src/cargo/core/compiler/fingerprint.rs:340:53 cargo-0.35.0 clippy::similar_names src/cargo/core/compiler/fingerprint.rs:340:63
and print a summary at the end

````
Checking archives/cargo-0.35.0 ...

Summary: archives/cargo-0.35.0
1, E0602
1, clippy::fn_params_excessive_bools
1, clippy::pub_enum_variant_names
1, clippy::redundant_static_lifetimes
1, null
1, renamed_and_removed_lints
2, deprecated
4, clippy::unseparated_literal_suffix
9, clippy::struct_excessive_bools
16, clippy::if_not_else
16, clippy::similar_names
18, clippy::single_component_path_imports
36, clippy::items_after_statements
53, clippy::module_name_repetitions
Checking archives/crossbeam-utils-0.6.5 ...
````

If I put the logs into a git repo and run it every day (I assume this could be run on github actions) this could already provide a bit of an insight in what kind of warnings are occurring in common crates.

[matthiaskrgr]

@ebroto already suggested this here

These lints would be enabled on nightly only, possibly behind a feature gate and/or a flag in Clippy. They would be tested by people that already accept some instability.

I really like the idea of having new lints to be nightly only by default, we could then observe the amount of false positives we get and then explicitly "release" lints on beta which then ride up to stable.

To spot false positives early, we could also consider having something like our current integration tests on gha inside the our repo.
Something like a cargo dev dogfood command which downloads predefined crates and checks them with clippy and saves the logs into the clippy repo.
When a new lint is introduced, the lint author will have to audit changes in the clippy dogfood logs and improve the lint until the FPs are gone. (Ci fails if the dogfood logs don't match and changes will be easy to spot for reviewers).
Drawbacks: reviewers might have to check if changes in the lint logs are actually true positives or not.

[Diggsey]

Rust already has crater for testing compiler changes against a large body of code. You could do something similar for clippy whenever a new candidate stable release is cut.