rust-lang / rust-clippy

A bunch of lints to catch common mistakes and improve your Rust code. Book: https://doc.rust-lang.org/clippy/
https://rust-lang.github.io/rust-clippy/
Other
11.28k stars 1.52k forks source link

Ignoring lints added after a given Rust version #11227

Open ojeda opened 1 year ago

ojeda commented 1 year ago

It would be nice to have a way to make Clippy ignore all lints that were added starting with a certain Rust version.

This would be useful to minimize the chance of breaking the build in projects that use -D for particular lint groups (or -W with -Dwarnings): once a new Rust version is released, and somebody builds the project, new lints may get added to the group, which would make the build fail. It can also be used to keep builds warning-free for everybody until the new lints are cleaned up.

A workaround for those projects is listing every single lint to enable by hand. As an example, in the Linux kernel, when we unpinned the compiler version and started supporting a minimum Rust version, we needed to move all our Clippy lint groups to -W, including -Wclippy::all, to avoid unexpected warnings and thus breaking Clippy-enabled builds with upcoming Rust versions.

The already-existing versioning information ("Added in: 1.x.y" rendered in the docs) could be used to drive this.

(Also, since it got mentioned at some point, Clippy's msrv does not currently solve this: that one is meant to disable lints pertaining to language/library/... features introduced in newer versions (e.g. a lint that suggests using let else that wouldn't be able to be used in an old version), rather than disabling lints introduced in newer versions.)

From: https://github.com/rust-lang/rust-clippy/issues/11162#issuecomment-1637073962. Cc @tgross35 @flip1995 (and @jonhoo since Philipp said Jon mentioned it too).

tgross35 commented 1 year ago

I wonder if it would make sense to pin the default configuration options to a version in case those change. It probably wouldn't be a bad idea, but also not at all required for a MVP.

jonhoo commented 1 year ago

I think this would be fantastic! I also want it in Rust proper. I forget whether there's already an open issue for it in rust-lang/rust, but @pnkfelix might know as I've spoken to him about this in the past.

Jarcho commented 1 year ago

Not breaking the build on new releases isn't really possible if you're denying warnings. Even with restricting the available lints to those available in a specific version, changes to those lints can cause new warnings to appear.

If you need to allow a lint which doesn't exist in a later version while still supporting older versions allow(unknown_lints, clippy::some_lint) will silence the lint and the unknown lint warning at the same time.

jonhoo commented 1 year ago

It's true that changes to existing lints can cause new warnings to appear, but by and large that's way less common than a new lint starting to trigger for your code. So I think #[deny(warnings = "1.69.0")] would still be a significant improvement compared to the status quo.

ojeda commented 1 year ago

Not breaking the build on new releases isn't really possible if you're denying warnings. Even with restricting the available lints to those available in a specific version, changes to those lints can cause new warnings to appear.

Yes, of course, but like Jon says, it still helps. For instance, in the kernel, the policy is to keep builds warning-free as much as possible and it is compile-tested with warnings as errors. Warnings will still happen in some cases (one cannot compile-test all possible combinations), but that is fine, they can get cleaned up if someone finds one.

For Clippy diagnostics, for us it may be potentially doable to deny lint groups if this feature is around, since Clippy is not meant to be used in production. But even if we decided to go with -W for them (like for rustc ones, because those are enabled all the time and there it is indeed more important to avoid breaking the build for "normal users" that do not use -Dwarnings), this feature likely still makes lint groups more pleasant to use, because it minimizes the cleanups needed to keep things warning-free for everybody, until the newly added warnings are cleaned up.

And if the feature is around, then I guess Clippy could be a bit more aggressive adding lints to groups since users can opt-out.

tgross35 commented 1 year ago

Cc @epage, I know you have thought a lot about lint config recently with the manifest-lint RFC - any thoughts on this? Not directly related to Cargo manifests but I feel like it may have come up in that discussion.

epage commented 1 year ago

If what we want is lint stability, this isn't just about new lints being added but

I brought up wanting something similar in the past but when the above was explained to me, I can understand the challenges with lint stability. What I've adopted in all of my packages (context: I have over 100, I focus on scalable maintainership), is to only deny in a CI job that uses a pinned rust version and to explicitly allow deprecations in that job. I have a hard time seeing a good solution otherwise that doesn't tie our hands too much.

jonhoo commented 1 year ago

I think this is a "perfect being the enemy of good" kind of situation. True, we may see some false positives where some code that previously was error free suddenly gets an error due to lint semantic change. But that's already the behavior today! And in return the number of failed CI runs is likely going to be significantly reduced because new Rust versions won't breaks as much for people who want to deny warnings. Not to mention we could conceivably extend this to also deal with semantics down the line — that can be a future feature without changing the interface.

ojeda commented 1 year ago

Agreed, perfect stability is not required to make this useful. Getting rid of the majority of the cases already helps a lot on keeping a build warning-free as much as possible. A warning here or there due to changes in semantics is not the end of the world, at least for use cases like the kernel's.

flip1995 commented 1 year ago

false positives where some code that previously was error free suddenly gets an error due to lint semantic change...

I would even say, not suppressing those is a good thing: Assuming those do not come from a new bug in the lint, new warnings from an already existing lint, is most likely an enhancement of the lint, so that it can catch more cases. So in a perfect world you get an improved version of a lint that the crate is already using, which you probably want to continue to adhere to anyway.

Centri3 commented 1 year ago

The main issue with that is that it would break the build temporarily if warnings are denied, though after it's fixed it would definitely be a positive change