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.

[epage]

@joshtriplett

I think nit is a fine name.

Of course someone was ok with my bike-shed avoidance name :). I guess I should have been more out there.

When I first switched to a company that used nit in code review, it felt odd and the intent wasn't to clear. Was this a nitpick? If they recognize it as such, why is it posted?

Also, not quite sure how to put to translate #[nit(let_and_return)] into a sentence in the same way as the other levels.

[joshtriplett]

That's fair: unlike error, warn, and allow, it isn't a verb.

[joshtriplett]

Feel free to resolve this thread in favor of the more general bikeshed. The point of this comment was just "don't rule out nit as a possible name", rather than advocating nit as the best possible name.

[tmccombs]

nitpick is a verb.

Nitpick might be better, since reader may not be familiar with the use of "nit" as a shorthand for nitpick

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

@llogiq the idea of gradually promoting lints through this level is being discussed at #3730 (comment)

clippy groups are being discussed as part of #3730 (comment)

[epage]

That statement is made within the context of the start of the motivation section:

By its name and literal usage, the warn level is non-blocking.
However, most projects treat warn as a soft-error.
It doesn't block for local development but CI blocks it from being merged.
This is an attempt to balance final correctness with rapid prototyping.
Requiring "warnings clean" code also avoids warnings fatigue where warnings
make it hard to see "relevant" compiler output, and make a codebase feel
lower-quality in a way that does not inspire people or invite people to
help solve the problem.
This convention is not new with the Rust community; many C++ projects have take
this approach before Rust came to be with "warnings clean" being a goal for
correctness.
Cargo is looking to further cement this meaning by adding
CARGO_BUILD_WARNINGS=deny
for CIs to set rather than RUSTFLAGS=-Dwarnings.

[Manishearth]

Why do people -Dwarnings?

The RFC has an interesting tidbit that inadvertantly reveals one of the subtler things underlying this discussion

If more clippy lints were non-blocking, maybe the Cargo team would not have set all to allow and been able to catch this more easily.

The interesting thing is ... the lints in question are, technically speaking, non-blocking! They were warn! And the choice the cargo team made with their codebase here was ultimately to block on the Clippy correctness lint group ... which is deny by default! They sort of arrived at the choices made by clippy available by default, the long way around.

...except that's not quite the case in a -Dwarnings world, right? In such a world, "warn" counts as blocking. And if you're used to a -Dwarnings world, it is a totally consistent thing to claim all of Clippy's warn lints as being "blocking", even though that's not a default.

Which makes me step back a bit and think a bit more about why people use -Dwarnings with clippy. Because, yes, that is an absolutely supported and valid workflow, and even somewhat encouraged, but it's not the default. If we wanted it to be the default we'd just mark more of our lints as deny! But a lot of people CI with -Dwarnings, including most codebases I work on.

And it comes back to some principles about warning fatigue. A linter is only useful if it's not noisy. One of the ways a linter can be noisy is a workflow where people do not necessarily run the linter on every commit, so there's plenty of code in the codebase that throws lints, and a contributor adding code will find lints about their changes buried amongst hundreds of lints from the rest of the codebase¹.

So most people CI with -Dwarnings to ensure that the codebase is linter-clean, so that any new warnings are by necessity warnings in new code, and thus relevant.

Which means that, of course, the -Dwarnings world is "normal" for most of us, at least in CI.

Given all this, though, do we think that nits will be treated differently?

Stepping through the steps again:

1. For nits to be useful, your codebase should not be emitting tons of them, and only showing you relevant ones
2. The easiest way to ensure this is to ensure that the main branch is nit-free
3. People are going to want to build with -Dnits
4. We have the same issues as people who build with -Dwarnings are having.

... which makes me feel like this is introducing a distinction without a difference, that won't actually solve any of these problems in practice, unless paired with other improvements elsewhere.

I think the underlying problem here is (1): lints are not useful if a codebase is full of them. That could potentially be improved upon for nits, somehow.

For example, we could potentially make it the norm to rarely, if ever use -Dnits (or even make it unavailable, though I don't like restricting functionality). Instead, this feature is design in concert with IDEs to provide the right UX such that nits are still useful even if a codebase is not nit-free from the get go. The RFC has a good start of it mentioning the default as being hidden except in IDEs (and that CI systems could filter), but I think more holistic UX work could be done there. Either only showing nits on code that has been modified by the current user, or just expecting they be shown with much less weight so that seeing a file full of nits is overall not a big idea.

I also wonder if, as mentioned elsewhere, supporting lint profiles would be a good solution to many of the issues here. Being able to say "by default build with these lint settings, but when building with --profile morelints or --lints morelints build with these settings instead". Perhaps IDEs are set up to request slightly noisier lints.

As it stands I'm not convinced that nit as a feature will in and of itself fix the problems talked about in the motivation here. I think it is a component of many solutions, but not a solution in and of itself.

Footnotes

1. Tooling can help here; there are code review designs where the only lints shown are ones relevant to the actual code being edited. But most people don't use things like that. ↩

[Manishearth]

I also wonder if, as mentioned elsewhere, supporting lint profiles would be a good solution to many of the issues here. Being able to say "by default build with these lint settings, but when building with --profile morelints or --lints morelints build with these settings instead". Perhaps IDEs are set up to request slightly noisier lints.

thought: this proposal is basically giving the ability to do more fine grained versions of -Dwarnings. Because it's true that a codebase wants to run different "sets" of lints at different times: -Dwarnings during development is terrible, but somewhat normal during dev, and having finer grained control would be amazing.

[alice-i-cecile]

Yes, this would meet our needs, both for the custom linter and for the project as a whole. It would be sweet to be able to share and export them for users via crates.io somehow, but that's easy to do in follow-up.

[tmccombs]

One thing I think that could help with this would be having better tooling for tracking lints that have been seen, and determined not to be useful. This could take the form of interactive feedback for lints that allows you to automatically add #allow annotations, or maybe having a separate file that contains a list of allowed exceptions for lints, so that you don't get spammed with a bunch of lints for existing code, that can be populated either interactively, or by dumping all current non-fatal warnings.

Another possible solution could be to have a way to run lints against a diff of the code. Although, I'm not sure if it would be possible to avoid both false positives (lints not actually introduced by the changes) and false negatives (new lints hits introduced by an interaction with code that didn't change).

[ijackson]

Thanks @Manishearth for your analysis, which seems sound, and your design, which seems like it would meet (all or almost all of) the needs I and my colleagues have.

Reading Test Modalities, I think we do need something like Option 2. Specifically,

lint profiles need to keep a list of their lint sets for test and non-test contexts

I think ^ this property is necessary complexity to be able to categorise lints. Separability of the classification of a lint when found in test code, vs when found in production code, is key. (Also, there should be a way to declare some apparently-"production" code to be treated as a test, and vice versa.)

[Manishearth]

@ijackson thank you, that is useful feedback! From my own use cases Option 2 isn't necessary (I think?) but it's weird to design two axes of a feature (being able to have multiple switchable profiles, and being able to control test-only stuff) and not have them be mixable in a useful way. So it's good to know if people do care about the mixing of the two.

[Manishearth]

Also, there should be a way to declare some apparently-"production" code to be treated as a test, and vice versa

I suspect the way to get such behavior would be to be able to define custom lint groups that can be toggled in code. That could be built on top of profiles but needs a bunch of rustc side work. I'll list it in a future work section.

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

[Manishearth]

I actually think this would be an interesting avenue to explore: being able to do custom lint groupings and profiles.

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

Suggested change

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

Nit: missing period.

[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

	default to the respective teams. Any lints referenced in this document are
	default to the respective teams. Any lints referenced in this document are

[BenjaminBrienen]

Suggested change

	*Note:* This RFC leaves the determination of what lints will be `nit` by
	*Note:* This RFC leaves the determination of which lints will be `nit` by

[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

[BenjaminBrienen]

Suggested change

	- 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
	- CIs could opt-in and open issues in the code review to show nits introduced in the current PR without blocking merging

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

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

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

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

[Manishearth]

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