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.

[Nadrieril]

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.

It might be possible to have a clippy-specific "nightly" process: completely independently of rustc nightlies, advanced users could enable "unstable = true` in some clippy.toml and that would enable beleeding-edge lints. That way even people who use rustc stable can provide feedback on new lints, with a longer testing period if we want.

[matthiaskrgr]

This sounds a bit like the current nursery lint group,

[flip1995]

Yes, that would be a nightly group that could be enabled.

What I want to achieve here would be a system where you don't have to change any configuration between stable and nightly Clippy to test nightly Clippy features. So your expectation should be that when you run cargo clippy -- -Dclippy::all -Dclippy::some_additional_lint on stable and nightly it automatically does all the new feature testing on nightly, the Clippy project cares about.

This might be harder to implement in Clippy, but makes it easier to run nightly Clippy in CIs along with stable Clippy. And it should be as easy as possible for users of Clippy to help us test Clippy. Otherwise we won't get much feedback.

[phaylon]

I'm not sure if this has come up before, but would it be possible to have lint warnings collected but not shown except as summary? Instead of new lints generating noise, there could be a phase where you run clippy and get a line like Found {} warnings from upcoming lints. Run cargo clippy --upcoming to see them. Maybe with a small bit of extra text explaining that this would be a great time to review and give feedback on those. Something that isn't in the way but also won't be easily missed. Also, --upcoming could add feedback links for those lints.

I'm just wondering if something like this could work as a middle ground for the wider community that is both transparent and non-disruptive.

[kpreid]

• 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.

An idea: what if lints that are less than one stable release old had a specific note that said “lintnamehere is a new Clippy lint and may have bugs; if this warning seems dubious, please let us know: ...”?

Of course, not very many people will read all the details. But some will, and that might be enough to get feedback from more nightly users.

[xFrednet]

This suggestion sounds in line with our idea to have lints be only on nightly for one or two releases #6623

[kpreid]

A small clarification: I don't mean just testing longer in nightly, but also that lints that are newly in stable should also have this note (for one stable release), so that the audience of only-stable users gets the information too.

[xFrednet]

Ahh okay, I'm not sure if that wouldn't create too much noise in the lint output. It could be something, which is only printed on the first lint trigger or something similar to reduce the text per lint emission.