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.

[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

[Manishearth]

Hm, I think I got a handle on one of my underlying concerns with this RFC: the problem is one faced by a lot of different people regardless of the tooling they use, but the solution proposed is squarely in the realm of third-party IDE/CI support. Which isn't necessarily a non-starter, but I do think this problem can be solved "in universe" in a way that benefits everyone.

IDEs are an optional thing one may use when working with Rust. So is cargo, admittedly, but cargo is much "less" optional and typically people replace cargo with an equivalent tool, unlike IDEs, which can be replaced with plain editors.

CI is also optional but since by and large the primary problem being solved here involves "what shows up locally vs what shows up in CI¹" it's not really a problem in a non-CI world.

However, choice of CI systems is optional. Not every CI system will end up with this type of integrated tool, and the ones who do may not have one flexible enough to support folks' needs. People do a lot of fancy stuff in their Rust CI and I wouldn't expect a "Run lints but only show nits on the CI" to tool to actually be usable by most users initially. As a datapoint, ICU4X has had the "show lints on PRs" CI task enabled and disabled a bunch of times for various reasons.

Footnotes

1. n.b: "shows up" here is about what the user actually notices: I'd consider warn lints as "showing up" locally, but not "showing up" in CI, since people seldom look at the logs of successful CI runs. ↩

[epage]

This RFC gives people the building blocks to make that policy. If they don't implement it, they still have the ability to show these lints on the command-line. Granted, that would be a firehose as pointed out in the RFC. The community can explore and iterate on tools to help with this.

In the end, if the user does nothing extra, nit will be like allow and those users will have lost nothing from this.

[Manishearth]

Okay, but I don't think "don't make the experience worse for other people" ought to be the bar for an RFC. This RFC lists problems that I have, that many people have, a its motivation. If the RFC does not solve those problems for us, I think that that's definitely an issue! It means the RFC is not fully solving the problems it talks about; and I don't think that's good for an RFC.

"Build your own CI tooling around this" is an acceptable conclusion for these folks. But "build your own CI tooling around this" was already a solution in the current world; this is just making that a little easier.

[epage]

Okay, but I don't think "don't make the experience worse for other people" ought to be the bar for an RFC.

If this isn't solving a problem for some users, how is it making the problem worse for them? The only thing I can think of is that linter teams decide to demote warnings into nits and they no longer show up. Except there is already a common expectation of needing to do a lot of customization, so is that much of a change?

If the RFC does not solve those problems for us, I think that that's definitely an issue! It means the RFC is not fully solving the problems it talks about; and I don't think that's good for an RFC.

Not every solution can solve a problem for every user. Sometimes we intentionally don't try to solve problems for every user.

This is somewhat talked about in the other thread, like with #3730 (comment)

But "build your own CI tooling around this" was already a solution in the current world; this is just making that a little easier.

This is at least partially answered in the RFC. To do this today, you need to use RUSTFLAGS. If you just set the RUSTFLAGS in CI, you then can't reproduce it locally.

What you'd likely need to do is

• Maintain a config.toml, separate from your [lints], for this
• Point rust-analyzer to this file through --config
  • Set up a separate CARGO_TARGET_DIR to avoid cache thrashing until fix(fingerprint): Don't throwaway the cache on RUSTFLAGS changes  cargo#14830 (if this third attempt even pans out)
• Point CI at this file through --config
• Document for people to know about this file and how to use it with --config

Even then to view the lints today you will get cache thrashing. If rust-lang/cargo#14830 ends up working, you won't get cache thrashing but you won't get any cache reuse between these nit runs and regular runs.

CI will either need a separate run without the nits config.toml to run with -Dwarnings (requiring a full rebuild) or it will have to be the first thing your config does before showing turning all of the other allows into warnings. With the latter, the user will then have r-a turning all standard warnings into errors which at minimum will block for anything built after. if you then use --keep-going, it will only block dependents. Maybe instead you have two separate config.toml files, one that has -Dwarnings and one that doesn't, requiring you to track two the nits in two places. The r-a user then has the problem that there is no way to distinguish nits from regular warnings.

That is a complicated path requiring knowledge of various advanced features and likely not discovering some of the problems i called out until they are hit. The burden for adopting this and maintaining it severely limits the likelihood someone would bother to adopt it.

So no, this is not about making it a little easier. This is about smoothing the path so people are even willing to do it in the first place.

[Lokathor]

Solving a problem for some but not all users often uses a lot of time and energy, which are usually both stretched quite thin within the Rust project.

A solution that suits some people can make a solution for the others become farther off because people don't always have the energy to do a thing, 'finally get it done', and then turn around and tackle almost the same problem all over again.

[Manishearth]

If this isn't solving a problem for some users, how is it making the problem worse for them? T

You misunderstand, I'm not saying it makes the problem worse for them, I just think that "doesn't make it worse for people" isn't the right bar for an RFC that's trying to solve a problem like this. You're free to disagree with that.

There is one way in which I'm worried this RFC makes things worse: people have wanted a nit-style "less than warn" level for different reasons¹ and I'm somewhat worried that having nit be used for this purpose would hinder the design of such a thing in the future. A design that explicitly declares that CARGO_BUILD_NITS=deny is never going to be a thing would definitely be a problem for a feature that lets you tweak lint UX for some lints but not others.

Not every solution can solve a problem for every user. Sometimes we intentionally don't try to solve problems for every user.

This is somewhat talked about in the other thread, like with #3730 (comment)

Yes, but I'm not convinced this solves the problem for enough of the people who have it. As mentioned before there's a huge diversity of attitudes towards linting and I'm skeptical this is sufficient for them.

And to be clear, I'm not talking about other lint modality problems like "deny only when releasing" or "deny only when testing" as I was elsewhere, I'm talking about the problems listed in the motivation here.

Furthermore as @Lokathor mentioned, partial solutions do often legitimately hinder full solutions in the future.

To do this today, you need to use RUSTFLAGS.

No, that's not what I'm talking about here. This is in the context of PR-integrated CI systems (like "show errors on PRs", e.g. clippy-action)

This RFC proposes a feature that is useful if IDEs pick it up (and then, that's only useful to people using IDEs!), or if PR-integrated CI tooling systems pick it up. The CI thing isn't simply a use of RUSTFLAGS because for this to be surfaced to the user (people don't read non-failing CI logs) it needs to do the thing where the warnings are slurped up and tagged on the PR diff using the GitHub API (or whatever).

My current experience with that type of PR-integrated CI tooling is that it tends to work for simple things and often ends up needing much more configurability than is available when a codebase gets more complex. In which case you're kind of required to roll your own. Overall I don't see people doing that since it's a lot of work, instead when this happens folks just don't use the PR-integrated CI tooling, after all it's mostly just a QoL improvement: you can always figure out your lints from failing CI or a local run.

Thing is, if you're rolling your own anyway, then you can also fix the problems motivating this RFC by making the PR-integrated CI tool have a config for lints like this. That's relatively minor in the scheme of things when it comes to the amount of work involved in building one of these.

That's what I'm talking about when I say "it makes it a little easier". This RFC is predicated on PR-integrated CI tooling being built that consumes this, but you can do this today in clippy-action using its flag settings. This RFC doesn't really enable anything new for these systems, just a better convention that nicely integrates with [lints]. Which is valuable! It's far better for this to work with [lints] than some clippy-action.toml or clippy_flags: -Dfoo -Abar or whatever since it's great for [lints] to be a one-stop shop, and I really don't want to undersell the benefit of that!

But! If a PR-integrated CI system doesn't work for me, I have to roll my own anyway, at which point this better convention doesn't buy me much in the scheme of things. That's what i mean by "it makes it a little easier", I'm talking about people for whom the existing PR-integrated CI systems don't work well. I don't think that's a small set of people at all, I've seen relatively low uptake of these systems, partially because of permissions issues, partly because of more bespoke build setups, etc.

It makes it a lot easier for people for whom this style of integrated CI tooling works well out of the box. I'm not convinced that's a lot of people.

Footnotes

1. Specifically, having a level that indicates "different, less noisy UX", for example collapsing all lints down into a small report, or perhaps making them oneliners. Basically changing how they get reported. Which is kind of what's going on with the r-a and CI proposals here, but there are a lot more different ways the UX can be tweaked around. ↩

[Manishearth]

Sometimes we intentionally don't try to solve problems for every user.

Specifically about this: my point here is to highlight that users who care about replicable CI, users who don't use IDEs, and users for whom PR-integrated CI doesn't work are not covered by this RFC, and if we are being intentionally excluded from the solutions here, that is fine, but that intention needs to be there, and when the lang team makes a call on this they should be aware, and attempt to understand what percentage of the target audience of this RFC's motivation is intentionally excluded from the solution.

I don't want it to fly under the radar that these groups are not going to benefit from this RFC. You're absolutely right that we sometimes intentionally don't solve problems for every user. However, we actually do have to be intentional about it.

[Manishearth]

Posted in a different thread on this RFC (please continue discussion on it there), but I sketched up a design for a solution that extends to this https://hackmd.io/@Manishearth/BJi8K8AGJg

I do still want a nit level but with the goal of nit being a different lint UX, not for solving the blocking/non-blocking problem. My design does not particularly preclude a nit level in the future.

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