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.

[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]

@woile

What about hint? Which is a verb, and similar to python type hints, you get a suggestion, and it's usually opt in to adopt.

In this case the IDE would give you hints.

cargo LEVEL=hint

[epage]

As mentioned, this has precedence in LSP.

My personal concern with it is that it can come across as too prescriptive /
passive-aggressive. Maybe there is a better way of expressing it, but I also find the sentence form of it awkward.

[lolbinarycat]

I worry it would be confusable with diagnostic hints.

[epage]

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

[epage]

@obi1kenobi

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

[epage]

@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]

This would likely be the case for any standard communication word (note, notice, inform).

In part, it depends on how you frame it as rustc does communucate these by default and there are recommended workflows where these do get communicated.

consider seems immune but it has a problem with the assumed short-flag is already in use.

hint depends on how you word it, see https://github.com/rust-lang/rfcs/pull/3730/files#r1848937892 for more.

[epage]

Looking at synonyms of consider

• A non-serious option is contemplate :)
• scrutinize might work (what would be the noun?)
• inspect might work (what would be the noun?)

[estebank]

I'm +1 on info (although I'm partial to note as it already has established meaning in the output we have today, but that might be a double edge sword if some notes can be controlled by the user and others can't, but we already have that for lints in general with warning and error...)

[zachs18]

info would also be parallel to Rust's logging ecosystem, which generally uses it for the log level below warn. Log levels aren't a perfect analogue of lint levels, but this parallelism could still be good.

I'd also be in favor of note.

[Lokathor]

existing lints/errors have help and note text. we can at least pick a new name.

[epage]

My concern with info is that it is a noun and runs into problems with the level/lint naming rules at https://rustc-dev-guide.rust-lang.org/diagnostics.html#lint-naming

The basic rule is: the lint name should make sense when read as "allow lint-name" or "allow lint-name items". For example, "allow deprecated items" and "allow dead_code" makes sense, while "allow unsafe_block" is ungrammatical (should be plural).

This is why in the RFC body I switched the level name to inform.

[tmccombs]

I'll point out that nitpick is also a verb:

find or point out minor faults in a fussy or pedantic way.

Pedantic would also seem appropriate, but that is an adjective, not a verb, and the verb form "pedantize" is unusual, and doesn't quite fit.

[woile]

last option I can think of: cue, which for me, it's quite an empty word, not used much, and it's not that far from hint/suggest.

cue on the let-and-return in this code

[epage]

@Lokathor from #3730 (comment)

doesn't that just give nit fatigue?

Like fundamentally if there's a lot of code you're in charge of, and it's doing something we don't detect, then we detect it, you'll have a lot to fix. and it doesn't matter if we call that new check a nit or a warning. if it suddenly says there's 4000 "whatever" in your project, you'll sigh deeply.

[epage]

epage from #3730 (comment)

The intention of this is that maintainers don't feel the need to know how many "whatever"s they have in a project. Ideally, both the code author (though LSP) and the code reviewer (through code review integration) see the feedback and decide whether it should be acted on. Or put another way, these are only of interest for new code and can be ignored otherwise.

The one exception is if you are using this to adopt a new lint gradually. In that case you can either fix every report for a lint of interest in which case you only care about the numbers for that specific lint going down or you only worry about new code, assuming the existing code is battle tested enough and through evolution will eventually be rewritten in which case you still don't care about the metric.

[epage]

@Lokathor from #3730 (comment)

I understand your intent, but absolutely nothing in this RFC actually does that. Nowhere does this RFC actually explain how "new code" is automatically detected, and nits only run against that. Are you assuming that it only scans for nits in the current git diff, or something like that? Such a system needs to be explained in the RFC if you're hanging the design on it.

* What about people not using git?

* what about people not using any revision control at all? does this never show up in the rust playground since that doesn't use version control?

[epage]

In the Guide section, the lints are shown to that user in two primary scenarios

• Rust-analyzer. I'm not sure what is capable here and so I framed that unknown as a drawback. It might not be on "only new code". Hopefully it can be done in a way that is not too noisy for users.
• A CI integration posting to the code review. I left this vague because not everyone has the same stack and technologies come and go. I did mention clippy-sarif which is what I am using in my own projects. Github tracks these across runs and I was given the impression that it will only report for new alerts but not finding it in the docs atm

[Lokathor]

Yeah, I think the easy part of this RFC is saying "show me diagnostics on new code only". I think the hard part is to define a system for what's "new code". If you can't actually say how cargo or rustc or rust-analyzer is picking out new code and only showing diagnostics for that, then the RFC is only half done.

At the same time, if there's a system to run a lint against "new code only", as a lesser level than "warn", then any time rustc adds a new lint people can just set the lint to "new_only" if they feel it's too much of a change to their codebase all at once. And all existing lints that got allowed through can be bumped to new and then bumped to warn eventually.

So I'd suggest focusing on what "new code" means, and trying to define that more specifically.

[epage]

I've summarized the Github side of my comment in 9e3c40a and reached out to r-a people in #3730 (comment)

[epage]

@Veykril any insight on how LSP clients use Information or Hint levels or what kind of UX can be made around ignorable lints without overwhelming the user with inlays or squigglies?

Looking to better talk about how this lint level will work in practice, see also #3730 (comment)

[Lokathor]

If a lint is limited to new code, then almost by definition it won't show too many of that lint at once. I think normal "information" style display isn't likely to overload the user.

[Veykril]

There is LSP support for an INFORMATION level that we currently don't emit, and then there is HINT which we do for some suggestions (very rarely). It depends on the client how they are shown, VSCode shows them like warnings/errors but different color scheme while filtering them out from the problems view. I would assume we'd not want to bunch this with other cargo diagnostics when running cargo check within rust-analyzer, but that depends on how this feature is used I guess? It could always be made an opt-in setting in rust-analyzer

[epage]

In 24c9506, I've marked r-a as an Unresolved Question but noted VSCode's behavior

[epage]

In 24c9506, I've marked r-a as an Unresolved Question but noted VSCode's behavior

[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!

[BenjaminBrienen]

Copyediting

[BenjaminBrienen]

Suggested change

	It doesn't block for local development but CI blocks it from being merged.
	It doesn't block for local development, but CI blocks it from being merged.

[BenjaminBrienen]

Suggested change

	Requiring "warnings clean" code also avoids warnings fatigue where warnings
	Requiring "warnings-clean" code also avoids warnings fatigue where warnings

[BenjaminBrienen]

Suggested change

	this approach before Rust came to be with "warnings clean" being a goal for
	this approach before Rust came to be with "warnings-clean" being a goal for

[kornelski]

I'm surprised that opt-in to show the lints is so verbose. For users who don't use an IDE, this may be tiresome to type. The verbosity of the env var could make users set it always in their default env (bashrc, etc.), and that would defeat the point of them being hidden by default.

I'd like something like cargo clippy --nits

Even when a CI is configured to display the nits, I still want to run them locally so that I can open the affected files easily (I can click file paths in my terminal), and can check whether I've fixed all instances without having to push code and wait for the CI.

[epage]

I'd like something like cargo clippy --nits

fdd4021 made note of why CLI control is deferred out of this RFC.

Even when a CI is configured to display the nits, I still want to run them locally so that I can open the affected files easily (I can click file paths in my terminal), and can check whether I've fixed all instances without having to push code and wait for the CI.

I feel like that will also run into the following drawback:

Users running CARGO_BUILD_NITS=nit cargo clippy will have a hard time finding what lints are relevant for their change. If we had a way to filter lints by part of the API or by "whats new since run X", then that could be resolved.

[kornelski]

CARGO_BUILD_NITS sounds like an env var for cargo build specifically.

Is cargo build supposed to react to this? That would be weird, because cargo clippy is already the tool for nitpicking, so there would be two separate clippys in Cargo with different UI and different nitpicks.

[epage]

This is an open question on CARGO_BUILD_WARNINGS, see rust-lang/cargo#14802 which this builds on top of. I made that explicit in 852f061

[kornelski]

I don't like nits being left to IDEs.

Rust-Analyzer/LSP are very biased towards MS Visual Studio Code. Where LSP doesn't support something, Rust Analyzer uses VSCode-specific workarounds, but other editors are left out. Other editors usually have sub-par experience. Sublime Text for example loses a lot of fidelity when displaying Rust's errors (in most cases it can't show a connection between a span and a note).

The problem is that Rust Analyzer uses VS Code as the reference implementation of LSP, and doesn't want to test and work around issues in other editors. Developers of the other IDEs/editors also just want to support LSP, and don't want to add features/workarounds for Rust Analyzer specifically. This means that any issues with how Rust errors are displayed in an LSP IDE end up being nobody's responsibility: RA tells to file bugs with your editor, and the editor says to file bugs with RA.

[Manishearth]

Yeah, I don't use Rust Analyzer, due to a bunch of different reasons: I've historically always worked on larger codebases, often with custom build systems, and IDEs have never been great at those. I also use Sublime Text and Rust IDE support has always lagged there. Both these problems are not as big a deal as they used to be now, but at this point ten years of writing Rust have made my existing workflows comparably efficient to using an IDE and I don't wish to put effort into switching now.

[kornelski]

Another option that would be interesting to me is to display nits only if there are no other warnings to display.

Unnecessary nits/lints are annoying when they reduce signal-to-noise ratio of compiler's output (e.g. displays 100 lints and 1 error, and I have to scroll through all the lints to find the error).

But if there's nothing else to report, then nits aren't getting in the way of anything.

[epage]

I've added this to the alternatives in 1d7463f.

I think showing these is problematic even when there are no warnings or errors. I suspect people will be annoyed by how "chatty" cargo would be (I know I'm annoyed with how chatty it is without warnings and I've heard from others who are more extreme and want no output on success) and that this will also nerd snipe people into addressing these.

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

[ijackson]

I find this definition of the levels is quite vague. Also, as @Manishearth notes, clippy and rusttc have very different interpretations of lint levels already.

I find the example of clippy::needless_borrow illuminating. In projects where I'm in charge of the lint policy, I always allow that lint. The needless borrow is sometimes helpful at the call site to signal to the reader that a borrow is occurring. Also, during development and refactoring, the types change back and forth, and the & becomes alternately necessary, and redundant. IME this lint leads to needless textual churn.

Similar arguments could be made about many (many most) lints in clippy's complexity category. That lint category likes to normalise the code as much as possible, subject to preserving the meaning of the current code to the compiler. But that doesn't necessarily preserve meaning of this code fragment in a slightly different context (ie, after future changes), nor the meanings implied to human readers.

Are we proposing to downgrade clippy::complexity to nit, then? If not, why not?

[epage]

Are we proposing to downgrade clippy::complexity to nit, then? If not, why not?

The RFC states

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.

Trying to cover every lint in this RFC and get buy-in from each of the teams is a recipe for endless bike shedding. It seems much better to let the individual teams decide this according to their practice and give them room to experiment and iterate.

[Manishearth]

@ijackson does my proposed alternate design with lint profiles support your use cases? #3730 (comment)

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

[Manishearth]

I submitted my design as an alternate RFC: #3926