Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ignoring lints added after a given Rust version #11227

Open
ojeda opened this issue Jul 25, 2023 · 11 comments
Open

Ignoring lints added after a given Rust version #11227

ojeda opened this issue Jul 25, 2023 · 11 comments
Labels
C-an-interesting-project Category: Interesting projects, that usually are more involved design/code wise. C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages

Comments

@ojeda
Copy link
Contributor

ojeda commented Jul 25, 2023

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: #11162 (comment).
Cc @tgross35 @flip1995 (and @jonhoo since Philipp said Jon mentioned it too).

@tgross35
Copy link
Contributor

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.

@flip1995 flip1995 added C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages C-an-interesting-project Category: Interesting projects, that usually are more involved design/code wise. labels Jul 25, 2023
@jonhoo
Copy link
Contributor

jonhoo commented Jul 30, 2023

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
Copy link
Contributor

Jarcho commented Jul 30, 2023

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
Copy link
Contributor

jonhoo commented Jul 30, 2023

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
Copy link
Contributor Author

ojeda commented Jul 31, 2023

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
Copy link
Contributor

tgross35 commented Aug 4, 2023

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
Copy link

epage commented Aug 4, 2023

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

  • Lints changing groups, meaning we have track the lint group per rust version
  • The exact semantics of a lint

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
Copy link
Contributor

jonhoo commented Aug 4, 2023

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
Copy link
Contributor Author

ojeda commented Aug 5, 2023

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
Copy link
Member

flip1995 commented Aug 6, 2023

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
Copy link
Member

Centri3 commented Aug 6, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-an-interesting-project Category: Interesting projects, that usually are more involved design/code wise. C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages
Projects
None yet
Development

No branches or pull requests

7 participants