Forbid, deny, warn, allow, and ... notice?

compiler
1

A common pattern today in Rust is to have two types of lint controls

  • Project lints: defines the expected quality bar of the project
    • Generally focused on warn to not get in the way of local development
    • Previously written via attributes or .cargo/config.toml but now supported in cargo via [lints]
  • Lint enforcement: prevent project lints from being merged
    • Currently handled via RUSTFLAGS=-Dwarnings though #8424 proposes cargo check --warnings=deny

A gap that exists in this is lints you want reported to the user but you don't want to deny. You can handle this in CI by denying all warnings but allowing the individual lints (e.g. RUSTFLAGS=-D warnings --allow deprecated)

Use cases

  • Avoid deprecations from breaking CI. Having Cargo.lock checked in has helped with this but it can still be nice to decouple an upgrade from migrating APIs. I've gotten enough feedback on this in clap that I've put deprecations behind a feature flag.
  • Code-smell lints that you don't want enforced but are still good to know about

Problems

Additional context: this doesn't just apply to rustc but clippy and rustdoc. In the future, this could also apply to cargo itself, cargo-semver-checks, cargo-audit, and whichever user-defined linting tool that makes it over the line. For example, cargo audits "unmaintained" warning falls into this category for me.

What if we added a new lint level notice and had a dynamic lint group notices (feel free to bikeshed names, remark, consideration). The difference from warn/warnings is

  • Different, friendlier prefix/color
  • Separate dynamic group so we can turn one to errors without affecting the other

I've not really seen this concept before in any ecosystem so I feel like there is a reason I'm missing for why not to do this. At least a quick search of internals only popped up Lint group for lints slightly below the warn-by-default bar - #2 by pitaj which is more about a static pedantic group which has some superficial similarities but in the end is different.

5 Likes
2

When configuring what IntelliJ IDEA calls "inspections", one can specify their severity. They list all possible severities in their documentation; besides the usual, an inspection may be categorized as a "Weak Warning" or for "Consideration".

4 Likes
3

Bikeshedding the name: LLVM uses “remarks” for top-level diagnostics that are not warnings or errors.

1 Like
4

I'm not convinced by this. Presumably these would be for cases where some of them might just be false positives? And thus a reasonable response might be for the developer to ignore a particular instance?

The problem with that is that on large code bases you soon get swamped by such warnings/notices/etc. And the interesting ones gets lost in the noise. For lints to be practical for daily usage they have to be reliable and relevant enough that you can enable them as warnings with a 0-warnings policy enforced by CI.

Sure, there can be situational or opinionated lints that you can opt in to. Clippy already has that in the pedantic category. But I don't see a use case for yet another level.

Let me address the two specific use cases in the proposal:

  • Deprecations is perhaps one of only two legit use case for a "let this through for a limited time" (the other being updating to a new rust version and suddenly having hundreds of previously undiagnosed issues). I don't really see what this proposal adds over status quo there though (deny warnings, switch back to warn on deprecations/that newly diagnosed thing).

  • For code smell it just isn't practical for any large scale project to not stay on top of the warnings all the time. As soon as you let that slip you will just have hundreds or thousands of lints and no time to fix it.

So I feel this at least needs:

  • Stronger use cases.
  • A motivation how this will work for large code bases (think Linux kernel scale, but close source where you can't just work on what you personally want to fix most of the time). How to prevent the useful stuff getting lost in the noise especially.
2 Likes
5

I can understand the challenge of being inundanted with warnings for large projects.

Some ideas

  • For IDE integration, be like IntelliJ and don't show squiggles by default, see the earlier link
  • For CI, you can use SARIF reporting which helps in only reporting new lints
  • (very hand-wavy) For local development, we can explore stuff like Github's SARIF database so we only report lints from changes in your code
  • For local development, with a config setting they can be changed to allow so you can opt-in for when you want to see ideas for improving the code
6

Let me re-phrase one of the motivations: let's pretend that in 3 years a cargo user should never know RUSTFLAGS exits. How do we do that?

This is "pretend" because I don't want to force the migration of something away from RUSTFLAGS until we have a decent understanding of the problem domain / use cases and a good abstraction for it.

7

There's one more downside of -Dwarnings that I thought this was about initially, but after more consideration realised it's not:

  • Updates to lints breaking CI when using a rolling toolchain

That feels like it fits the first problem (Users can't tell what lints they are responsible for and which they aren't) more than the listed downsides, but it doesn't fit with the usecases. That is more likely solved by what I presume SARIF reporting is capable of doing (run a daily CI job that uploads the baseline warnings from the primary branch, and creates an issue for the maintainers to fix if there are any, and ignore those warnings on PR jobs).

For the bikeshed, by my understanding this is about marking lints as "todo in the not too distant future", they're not "warnings to be fixed before merge" and they're not "allowed to exist indefinitely".

8

Though, differential warnings could also help with the first usecase. You would just need to flag the upgrade PR as "allow merge with warnings", that would then add those deprecation warnings to the baseline and not block other PRs on fixing them.

9

In my thinking, there is no timeline for resolving these. Some may only be suggestions that only the developer can determine whether they are relevant or not but you don't want to liter your code with a lot of allow statements.

10

Hmmm, that sounds more like something that you wouldn't want littering the console during a normal check (and isn't how I would normally think of deprecation notices).

Another idea, if we're getting a way to set lint levels through the CLI (which we actually already have with cargo clippy -- -Dwarnings, avoiding a lot of the downsides of RUSTFLAGS, but I presume there's still even better integration that can happen if it's cargo parsing the flags), then instead of having something that rustc understands it could be entirely handled by cargo:

# allow defining custom lint groups, idk what the best syntax is
[lint-group.notices]
rust = ["deprecated"]
clippy = ["as_conversions"]

> # run a check but with a custom lint group activated
> cargo check -Wnotices
1 Like
11

When would rustc ever put a lint in the notice level instead of allow/warn/deny?

I think it's very important that the default answer for lints is that if they're worth having in rustc, they should be warn-by-default. I really don't want to go down a road of "lints have to start at notice for a while", or similar, since I'm very happy that rustc as so far avoided needing -Wall and -Wextra, or similar.

I more and more think that a bunch of things we do as "lints" are the kind of thing I never actually want to hard-deny or to see on the command line ever.

I would love to have many of these things in rust-analyzer instead, where a subtle ellipsis under a syntax item (Resharper in C# does a great job of this, for example) can indicate that there's something you might be interested in here, but that it's not urgent enough that we need to interrupt you, and that you don't need to do either.

For example, having a R-A convenience hit to fix https://rust-lang.github.io/rust-clippy/master/#/collapsible_else_if is really really useful. But I don't want clippy warning me about it most of the time, because I tend to have written it like that for a reason, particularly if the if in the body of the else mirrors the one in the body of the if part.

Resharper in C#, for example, has a nice "replace this loop with iterators" thing it can do. Sometimes that's really handly, but often I'll click it to see what happens and decide that no, I like it better as it was. (For example, if Rust code needs Iterator::scan it's sometimes clearer to just have a for loop, especially for a complex body.) And those are the kinds of things that can apply in so many places that I probably never want a command-line report of everything

So the more these could be things that show up subtly in my IDE to clean up as I'm writing or refactoring, but not in CI, the better, I think.

5 Likes
12

I can see rustc taking that approach but there is also clippy and soon cargo and maybe even cargo semverchecks one day.

13

I believe cargo clippy -- -Dwarnings is a RUSTFLAG, just by another means.

My proposal for warnings control was --warnings=<allow|deny> would not change any RUSTFLAGS passed to the compiler but cargo would capture the output of the warnings and decide whether to render them or not. This means cargo can show/hide the warnings without a recompilation.

For allowing custom groupings like this to fit in that scheme, I worry about calling them groups and trying to give them full group-like semantics (or to give the compiler's groups the semantics we are trying to give warnings). Maybe this is just a rename from "lint groups" to something else. Maybe its a warning-category or something and the compiler just sees that these are warnings but cargo uses this knowledge (and cli flags / config) to decide whether to hide them or not.

One thing I liked about --warnings=deny is that by swapping the group and the level, it hinted to the user that this isn't the same thing as the compiler's flag.

14

Well cargo clippy -- -Dwarnings already exists? You could also have something like that in Cargo.toml, perhaps also for rustc lints (not just clippy). Then there are the attributes you can put in the code. I'm not sure when you need the RUSTFLAGS approach really.

15

Sort of, it acts much closer to a WORKSPACE_RUSTFLAGS, if that existed, so it doesn't have the downside of recompiling all dependencies like setting RUSTFLAGS does, and it's additive to other ways of setting RUSTFLAGS. (It's sort of like if cargo rustc supported building multiple packages and targets with the same set of flags).

16

Now that does make sense (moving the setting to cargo so it can be smarter). That part to me isn't controversial. The issue here specifically is the notice category and having things in it by default. It just doesn't scale to large projects, especially those not using github (as far as I know gitlab or azure devops doesn't support SARIF?).

And more importantly, rust tooling should be usable as is (with good defaults for most people), without having to bolt on third party solutions to make it usable.

(Note, that is not the same as saying the rust standard library should be larger: here I'm talking about tooling and having good defaults, of course you should be able to add on things on top, and those things should ideally compose well.)

I think the Rust-analyzer idea by @Nemo157 makes a lot more sense for these types of issues.

17

Hm, isn't this really two separate Per-pre-RFCs? One about the notice group, and one about making cargo more aware of the lint control flags? It probably makes sense (in order to not just stall the whole process due to differing opinions) to separate them out.

18

Hm, isn't this really two separate Per-pre-RFCs? One about the notice group, and one about making cargo more aware of the lint control flags

If by "making cargo more away of the lint control flags" you mean --warning=deny, then already done. This proposal was never about that but I brought it up for added flavor and then again for how it interacts with a proposal someone made. I'm unsure if --warnigns=deny is up to the level of an RFC and was considering handling it as a MCP.

1 Like
19

For reference, LSP defines four diagnostic severity levels: Error, Warning, Information, Hint, in that order. It also defines a separate axis for tagging diagnostics labelling Unnecessary (unused) or Deprecated (obsolete) code. What it doesn't do is provide a suggested interpretation of the severity levels.

I think the potentially useful levels for compiler diagnostics are, in decreasing severity order:

  • Syntax error – the compiler cannot interpret what your code is saying at all, preventing any further analysis.
  • Language error – the syntax of the code is meaningful, but names/types/etc fail to resolve consistently, preventing that code from being evaluated.
  • Error lint – Code is structurally correct and evaluable, but contains some pattern which should prevent running of the code even for development purposes. Display: “red squiggly” and in interactive compilation summary.
  • Warn lint – Code is reasonable for transient development state, but contains some pattern which should be fixed before merging to main. Display: “yellow squiggly” and in interactive compilation summary.
  • Info lint – Code is reasonable, but could potentially be improved or indicate that the developer may have overlooked something. Display: “grey squiggly” and excluded from interactive compilation summary.

Forbid lints are a special case of error lints, where trying to locally relax the level is also a lint error. Allow lints are where the compiler is capable of linting but has been instructed not to.

Or pulled back to just the essentials:

  • Error: block test
  • Warn: block merge
  • Info: nonblocking

It's imho critical that the nonblocking class of lints are a "when asked only" display, as space/attention in the interactive compilation results is at a premium to avoid warning fatigue.

I honestly don't know what the expected presentation difference between LSP's Info and Hint severities are. And while this so my interpretation of useful levels, other projects might have different conceptualizations, and domain knowledge might enable a more granular breakdown.

For the specific case of deprecation, it'd be nice to be able to say to allow specific deprecated functionality within a scope, so e.g. I could say to allow (or weaken to remark, etc) all deprecated items in ::clap::* within my crate, but still report use of other deprecated items.

Or maybe even more usefully than making it that wide to apply on a namespace basis, use the since property of deprecation and only warn for things deprecated up to a specific target version of a crate.

Deprecation is a very interesting case for a lint because it's basically halfway to a user defined lint, and that means you end up with the complexity falling from different packages using deprecation in slightly different ways and that want to be treated slightly differently in the linting framework. (E.g. “has irreconcilable issues, use the new spelling that fixes them” should be a warning, whereas “removed in the next major version, migrate to this other related subsystem instead” is much less important.)

6 Likes
20

Telling rustc about the minimum version match for version requirements so it can tailor deprecated lints is one of my "API evolution" improvement ideas, going along with a stable #[stable] attribute so you can be told if you are using parts of the API that are too new.