Hi folks. I went through the last 2 years of rustc upgrades in a project I maintain and found quite a bit of breakage due to clippy lints with high blast radius or false positives.
I add #![warn(clippy:all, clippy::pedantic)] to all of my crates. These are some of the not fun bugs I've encountered:
I've suggested in a few of these issues that new clippy lints be tested with crater, but haven't gotten much engagement. I'd like new lints to go through crater runs to determine:
What blast radius they have.
How much churn they suggest.
Whether there are false positives.
Whether the suggestions compile.
I think a theme of these linked tickets is that there tend to not be tests for code that should not be triggered by a new lint. In a world where the project has the infrastructure to test compiler changes on the entire ecosystem, I'd hope there would be fewer false positives in clippy that make it to stable.
I understand where you're coming from, false positives and lints that would cause API breaks are reasons why adoption in large scale projects is hard. (Especially if those had been started before clippy in its current state). That being said, I don't think we get enough actionable feedback from those crater runs to warrant executing one for each and every lint—they are somewhat costly and would they need to be redone for any adjustment made to the lint?
To be honest, my understanding of pedantic is an explicit opt-in to some sense of 'false positives'. Let me explain. It's role is much rather to answer 'Can this code be confusing to any reasonable Rust programmer?' than it is to answer the question 'Does this code contain problems'. The former question is probably useful as you develop code but much less useful for maintaining an existing code base. Consider that as Rust evolves and we get new insight in the language and, for example, better convenience constructs in the language we could make an argument that its answer must change over time, conflicting with not introducing churn. If you had written this pattern a while ago:
Some(1) | Some(2) | Some(3)
It will now warn you that you should instead use:
Some(1 | 2 | 3)
This is (rather) new! Specifically it is only stable since Rust 1.53. and it has been introduced specifically to make such patterns tersers and more readable. If the code has initially been written before that point there was not other option, the only scenario is that now you must change it. I think it isn't possible to fulfill both the goal of using the best language and library tools for readability and having no churn.
All that being said, I think there's an argument for having some designated subset of clippy lints that is specifically chosen for having very few or no false positives, and only geared towards incorrect and inefficient code instead of personal 'taste'. Maybe clippy::incorrect.
The sub patterns lint is great and I gladly made the changes that it suggested.
I also gladly added must_use annotations everywhere in my code base clippy suggested. I found that useful for preventing bugs.
What has not been fun is redoing the must_use annotations in the subsequent Rust release because the must_use lint was significantly reworked because it was so spammy.
Not accounting for debug_assert in the missing_panics_doc is not so much a false positive as an outright bug which I believe would have been caught if the lint had been run over in-the-wild Rust code.
Unlike a linter like RuboCop, the Rust toolchain does not give the option to skip a clippy upgrade if there are bugs in the new lints that impact my code. My only option is to avoid updating the compiler until the bugs are fixed.
When I mention churn, I don't mean the new lints clippy adds that modernize my code for the latest stable idioms. I love these lints and I enable them because I intend to track the latest stable compiler.
Instead, I mean something like this sequence of PRs which works around a bug in the linter:
One trick for linters specifically is to run them not on arbitrary code (where there will be loads of false errors, but even more true errors), but on known good code. For high quality crates, new lints firing have a much bigger probability to be a false warning.
Indeed, I was recently bitten by a couple clippy lints when I moved one of my libraries to MSRV 1.38, so it could be directly built with mrustc (matches!() which was stablized in 1..45 IIRC, and non_exhaustive which was 1.40). I can imagine crates like that (especially ones that have deny(warnings) on) would get pinged with some frequency by clippy crater runs.
Hi, I believe we are technically aware of this problem:D
A while ago I made a small tool that runs clippy on a small set of crates on a daily basis and saves the logs (clippy lint results) into a repo so you can easily git diff yesterdays lints against todays lints.
Unfortunately since I started my dayjob my open source involvement has been decimated, I don't really have the time to review the findings anymore.
If someone wants to volunteer and check if a lint causes tons of warnings that look suspicious, please go ahead (you might wanna drop a note in the clippy zulip channel to make the team aware ).
Check the repos git logs if there is something that looks suspicious, download the source, run clippy on it and inspect the code, and make a ticket with a small reproducer.
I guess doing this like once a week before might be sufficient, we can then decide if the lint can be easily fixed or if the lint should be downgraded to nursery or allow-by-default until it has matured and false positives have been fixed.
For clippy::pedantic, this level of rigor is almost certainly too much. The pedantic lint group is specifically for lints which are more widely applicable than restriction lints, but still have a notable bias that can lead to suggesting negative changes, if not outright false positives.
If you're specifically opting in to lints which aren't on by default, you're knowingly acknowledging that the lints are less than universally applicable, and that you might want to #[allow] them in some places.
Specifically only for lints which are added to on by default, though, it might be worth a partial crater run (over, say, the top 1000 or so crates by downloads) to get an idea of their impact on "in the wild" code. If my memory isn't deceiving me, there was a clippy lint that was on (warn) by default that hit stable somewhat recentish but had also been moved to the nursery lint group after beta was cut due to false positives.
I don't know the best way of handling it, but (assuming my memory isn't lying to me) it is kind of silly to end up with a nursery lint on by default in stable.
That's the clippy::correctness lint group, and perhaps clippy::suspicious.
It would be awesome if clippy lints that hit stable as part of the default-warn (or deny) set had fewer false positives, and I do think there's a lot to win there. Unfortunately from my interactions with the team (as a maintainer with a bunch of projects that run clippy in CI) seem to indicate that driving false positives (for new lints) down does not seem like a very high priority.
I'd find this easier to do if we had the #[expect] pragma so I could document which lints I've allowed because of false positives. This would also allow me to be automatically notified when those false positives were fixed or the lints moved to the nursery, instead of having to patrol the clippy issue tracker.
It's true that we sadly have a high FP rate for some new lints. Developing a system to avoid this is part of the Clippy Roadmap 2021. There has been little progress so far, but it's on our to-do list.
Stabilizing lint reasons and the expect attribute would definitely be a win for Clippy. I've got a mostly working implementation of the expect attribute in rust#87835. The final review and approval is sadly blocked on some incremental compilation restrictions described here. I've seen some other PRs which could potentially resolve these problems.
).