RFC: Add a semantically non-blocking lint level

[epage]

Add a new visible lint level below warn to allow linters to act like a pair-programmer / code-reviewer where feedback is evaluated on a case-by-case basis.

• cargo does not display these lints by default, requiring an opt-in
• This RFC assumes LSPs/IDEs will opt-in
• CIs could opt-in and open issues in the code review to show nits introduced in the current PR but not block merge on them

There is no expectation that crates will be nit-free.

Note: The name nit is being used for illustrative purposes and is assumed to not be the final choice

Note: This RFC leaves the determination of what lints will be nit by default to the respective teams. Any lints referenced in this document are for illustrating the intent of this feature and how teams could reason about this new lint level.

Rendered

[epage]

I marked this a T-lang as I got the impression that this is primarily their decision. This also directly touches on T-cargo and T-compiler (@estebank) but I'm hoping we can go with an informal acceptance, rather than 3-way FCP. This also has implications for T-clippy (@flip1995).

[joshtriplett]

I think nit is a fine name.

I think this makes sense: IDEs or cargo can opt into this in order to obtain warnings of potential interest without assuming the user wants to see them by default or block on them.

[clarfonthey]

Not to focus too much on the name, but mention might be another option for it. It goes well with the ordering of deny, warn, mention, allow.

[obi1kenobi]

I believe Visual Studio (at least back in the day) called this level "info", but I like "mention" and "nit" better.

cargo-semver-checks would benefit from such a level as well, since there are cases we want to highlight for PR review purposes, but only once instead of on every PR or every release. Think "this is allowed but somewhat dangerous, flagging + offering context so you can explicitly make an informed decision."

[epage]

@clarfonthey "mention" is another good candidate that balances "you should know about this" with "you are welcome to take or leave this". Added in b7093ce

[kpreid]

"mention" has the disadvantage that the intended default user-visible effect of #[mention(x)] is to not mention it when building. This is at odds with the verb interpretation of #[warn(x)] and I expect it would be frequently surprising.

[epage]

@kpreid I copied your comment over to https://github.com/rust-lang/rfcs/pull/3730/files#r1848944052 so we can more easily follow that conversation.

[llogiq]

I have one general thought to add to this, namely there are two reasons in clippy to warn-but-not-block:

1. (which if my understanding is correct this RFC tackles) lints that give suggestions, but shouldn't stop anyone from doing their job. I personally like nit well enough to just roll with it, but wouldn't be against any of the proposed alternatives either
2. lints that are newly introduced and would break CI if we were to activate them. For those, a future lint group might be a good solution. That group would also have a warn-but-never-fail-the-build lint level and thus give people some time to fix their code before a new lint becomes active in earnest

So I suggest that if we add a lint level that amounts to warn-but-succeed, we should probably add both lint groups within clippy.

[Lokathor]

I'd like to push back on (2) slightly. It has long been the policy that future versions of Rust can always introduce a new warning, and if your CI can't cope with that at all then your CI has the problem, not Rust. The "-Wall -Werror" attitude that comes from C is exactly what we don't want to let develop into the rust ecosystem long term.

[epage]

@llogiq the idea of gradually promoting lints through this level is being discussed at https://github.com/rust-lang/rfcs/pull/3730#discussion_r1849082065

clippy groups are being discussed as part of https://github.com/rust-lang/rfcs/pull/3730#discussion_r1850056640

[Manishearth]

@Lokathor Clippy has different policies from Rust on this, though, and that's fine.

If anything, Clippy is more liberal about "breaking" code on clippy updates, however we do try and manage that in ways that are smooth to the user.

[Lokathor]

I wasn't referring to or considering clippy in any way when i said the above.

[Manishearth]

@Lokathor Okay, in that case please reread @llogiq's comment that you were pushing back on, because it is explicitly about clippy's choices :smile:

[Lokathor]

.... dag you're right

[Manishearth]

I have comments along a couple axes and for cleanliness I'll post them as separate comments.

Clippy vs Rust lint level standards

So this RFC cites experiences with clippy as a motivation and the discussion here includes a bunch of talk of clippy and I think before stuff proceeds we should make it clear that Clippy and Rust do lint leveling differently, and that's deliberate. This is going to be relevant when considering motivations about struggles with clippy, or considering how clippy may choose to use such a feature.

In Rust:

• deny: This is basically always bad.
• warn: This is a strong stylistic/cleanup suggestion that the vast majority of Rust users will agree with.
• allow: (varies)

In clippy:

• deny: This is a correctness smell. It's probably broken but it might not be, either way, it should be flagged and rewritten or the lint silenced
• warn: This is often a stylistic preference. People may disagree with this and as a codebase choose to disable that, and that's okay. We have often stated "clippy is intended to be used with a liberal sprinkling of allow().
• allow: Varies. Can be for restriction lints, that are "this is NOT bad but codebases may wish to disable it for use-case specific reasons", can be for pedantic which are "this is kind of dispreferred but weakly so", can be for nursery which is "this lint is buggy".

This is a very rough sketch, but the fundamental difference is that clippy lints tend to be a bit weaker, because the expectation is that you will customize your clippy lint set to your needs. By default, clippy's deny lints are closer in severity to Rust's warn lints, than Rust's deny lints, and clippy's warn lints are closer in severity to Rust's allow lints.

In part, Clippy exists because of this: clippy is where you go for lints that can't be in rustc because people might disagree on them.

I think when discussing clippy's levels it's worth keeping this in mind.

Clippy tries to make things easier here with lint groupings so that you don't actually have to go through and individually consider 400 lints one by one, instead you can consider which groups look interesting and then enable others you find interesting or disable ones you find less useful.

[epage]

I have comments along a couple axes and for cleanliness I'll post them as separate comments.

@Manishearth if you are posting several, please post these as comments on lines or on the file, rather than using the main thread. It makes the conversations much easier to track

EDIT: context

[Manishearth]

Hmm, they're broader than any particular part they anchor on, but sure. My first comment above in particular is not trying to start a discussion, it's trying to provide context. But I'll post the others as file comments.

[alice-i-cecile]

To talk a bit about why Bevy (as a random large Rust project) uses "deny warnings" as our standard CI config, we fundamentally want to hard deny these lints from getting merged into main. However, setting the lint levels to deny/forbid locally slows down the development process too much, as devs can't iterate with half-broken code locally. Code that "barely compiles" (with todos, bad style, perf issues etc) is great for sketching out a design and .

Having a level below warning for "it's fine if we ship this but it would be good to fix eventually" would be handy, especially for phasing in new lints from nightly without blocking merges. Coupling these directly to clippy's categories would be quite frustrating for us though: we need to be able to control which lints fit into this category due to the bespoke needs of each project. We'd also like to be able to use this new level for our own Bevy-specific lints!

[ijackson]

[In CI] we fundamentally want to hard deny these lints from getting merged into main. However, setting the lint levels to deny/forbid locally slows down the development process too much, as devs can't iterate with half-broken code locally. Code that "barely compiles" (with todos, bad style, perf issues etc) is great for sketching out a design and .

This is definitely true. But I don't think the right solution is a new level for the lints.

The solution is an easier way to get these lints out of the way when developing. My personal practice often ends up being "add #![allow(unused, clippy::complexity, ...)] // XXXX remove before merge to the top of the file temporarily, with CI spotting the XXXX if I forget.

I find this is oiften better than changing the compiler options, becuase it lives with the branch and modules I'm working on, so that if I switch branches, the warning profile switches too.

[Manishearth]

@ijackson does my proposed alternate design with lint profiles support your use cases? https://github.com/rust-lang/rfcs/pull/3730#discussion_r1859460148

[epage]

Sorry for the delays. With the various holidays, I got swamped. I expect to be able to focus on this again.

I'm also hoping that taking a break will help us come to this with fresher eyes. I know I'm going to be more carefully re-reading things to evaluate the different potential design directions.