Add --deny-warnings functionality for all commands.

[nathan-at-least]

Describe the problem you are trying to solve

I often automate cargo and I typically want any automation to halt when cargo emits warnings. AFAICT the only way to do this is to try to parse the output for the way warnings are typically produced.

(Related issue: there are two categories of warnings which is confusing and needs clarification, which I filed as #8423 .)

Describe the solution you'd like

Add a commandline flag called --deny-warnings and if set, if a warning is encountered, this ensures the process will exit with a non-zero status and that no "non-dry-run" operations proceed. For example, with cargo publish --deny-warnings any of the early local packaging-style lint warnings will cause cargo publish to avoid making any requests to
crates.io.

Notes

I'm not sure about the option name --deny-warnings which I took from the #![deny(warnings)] rust compilation attribute, but I can't think of anything clearer and concise. --treat-warnings-like-errors, --exit-with-error-when-encountering-warnings, --don't-just-warn-me-stop-me!, and --please-save-me-from-myself all are too cumbersome.

Related Tickets

This is related to #3591 because in some way these are opposites (disabling warnings versus escalating them to errors). There might be some common CLI magic that addresses both issues and is clear enough to users.

Even if they use separate cli and/or config specification, if both implemented, they might lead to nonsensical / confusing combinations which a good UX would complain-and-exit about, for example: cargo do-stuff --hide-warnings --deny-warnings or something similar should probably say "I can't tell what you meant to do."

[jrvanwhy]

Commenting to support this feature request.

The project I work on has -D warnings in its .cargo/config:

[target.'cfg(all())']
rustflags = ["-D", "warnings"]

This works great for CI, where it prevents us from merging code that has warnings. However, it is painful for local development. I frequently add a few lines, run cargo check, and get errors related to unused variables. I prepend _ to variable names and remove it extremely frequently, which is a waste of time.

Adding a --deny-warnings flag to cargo would allow us to use --deny-warnings in our CI infrastructure to block on warnings but still allow iterative code development to proceed without the unnecessary toil caused by -D warnings.

[CPerezz]

You can already do this in your CI. It's just about setting your ENV variable RUSTFLAGS= -D warnings and run cargo.

Cargo will fetch the ENV variables and pass them to rustc so that the warnings get denied without needing to add any extra stuff.

This would be an example using actions-rs

on: [push]

name: CI

jobs:
  build_and_test:
    name: Rust project
    runs-on: ubuntu-latest
    env:
      RUSTFLAGS: -D warnings
    steps:
      - uses: actions/checkout@v2
      - uses: actions-rs/toolchain@v1
        with:
          toolchain: stable
      - uses: actions-rs/cargo@v1
        with:
          command: build
          args: --release --all-features

If you want to treat warnings as errors, or do any fancy stuff, check the docs about lint levels for rustc: https://doc.rust-lang.org/rustc/lints/levels.html

I think this issue can be closed considering that --deny-warnings is already something that can be achieved setting up rustflags.

[jrvanwhy]

You can already do this in your CI. It's just about setting your ENV variable RUSTFLAGS= -D warnings and run cargo.

That does not work for projects that need cargo to pass rust flags configured in .cargo/config. The cargo config documentation says the following about rust flag selection:

There are three mutually exclusive sources of extra flags. They are checked in order, with the first one being used:

1. RUSTFLAGS environment variable.
2. All matching target.<triple>.rustflags and target.<cfg>.rustflags config entries joined together.
3. build.rustflags config value.

As an example, libtock-rs uses .cargo/config to configure relocation. Setting RUSTFLAGS while invoking cargo causes cargo to ignore this config and create broken binaries.

It is possible deny warnings only in CI by including a ci feature to all our crates and adding #![cfg_attr(feature = "ci", deny(warnings)] to the top of all our crates, but that adds maintenance proportional to the number of crates in the project. Another way to do this is to write code to edit .cargo/config during CI (we are doing this in libtock-rs#271), but that is hacky and feels fragile.

[CPerezz]

I understand your point. With multiple crates things get out of control for maintenance.

We're working on an RFC that might make this easier to manage. See: rust-lang/rust-clippy#6625

Although the --deny-warnings would require a bit of design since it can have some edge cases IMO.

[wraithan]

Another reason why RUSTFLAGS="-D warnings" is insufficient as a workaround, even for non-workspace, is that it doesn't account for commands like cargo doc which is how I stumbled upon this feature request, trying to get warnings when building docs in CI to be bubbled up as failures. I can add grep or rg or w/e to process the output and look for ^warning: this way bad links in docs get caught before merged (for example).

[stephenmartindale]

And yet another reason why RUSTFLAGS is a terrible, terrible way to control compiler warnings from rustc is rust-analyzer. If one's build script hacks about with RUSTFLAGS, rust-analyzer has no way of knowing and, because a full recompile gets triggered every single time RUSTFLAGS changes (even for seemingly cosmetic things like warning promotion & suppression), the result can be a complete nightmare as rust-analyzer and the build scripts fight over target.

Hmm. Now that I write that, I realise that promoting warnings to build-failing errors actually is not a cosmetic thing and, perhaps, it actually should force a full recompile, following the same arguments behind cargo package's full recompile, because warnings-as-errors is typically something that gets employed in CI and for actual release builds.

[Anders429]

For the cargo doc use case specifically, it seems that using RUSTDOCFLAGS instead of RUSTFLAGS gets the job done. For example, running RUSTDOCFLAGS='-D warnings' cargo doc promotes all warnings to failures.

[veber-alex]

Is there anything new regarding this issue?

I want to run cargo check with deny warnings as a precommit hook but the only way is to set RUSTFLAGS which causes a recompile of the crate and all the dependencies.

[epage]

Is there anything new regarding this issue?

If its not in this issue, likely not. The closest related work is #12115 which removes some reliance on RUSTFLAGS for dealing with warnings but not in this case.

I want to run cargo check with deny warnings as a precommit hook but the only way is to set RUSTFLAGS which causes a recompile of the crate and all the dependencies.

The hard thing here is that technically your dependencies are reporting warnings and deny-warnings could affect them, so we'd need to recompile to verify that. However, with caplints, most warnings from dependencies are dropped and with our caching of warnings between runs, rebuilds aren't needed.

The challenge with RUSTFLAGS is that its opaque to us (and should remain so) and that it applies to all packages when you only care about workspace members. #8716 at least partially improves the RUSTFLAGS situation because we would generate separate files for each RUSTFLAGS, rather than overwriting, so a full rebuild would happen once and then we'd rebuild on top of that.

[veber-alex]

I don't think using RUSTFLAGS here is the correct solution, clippy has the option to control the lints directly using cargo clippy -- -Dwarnings.
You can also do this with cargo rustc --lib -- -Dwarnings but that runs only for one target.
I think cargo check and build need something like this.

[epage]

Been thinking about this more in the context of "how do we rely on RUSTFLAGS less?"

In a case like this, a cargo check -- -Dwarnings can cause things to rebuild because you are passing in a RUSTFLAGS and cargo has no idea what it will do.

What if we added a flag --warnings <deny|allow> that was completely processed within cargo, relying on the cached output from rustc that cargo replays when a crate doesn't need rebuilding (#6933)

[noraa-july-stoke]

What if we added a flag --warnings <deny|allow> that was completely processed within cargo, relying on the cached output from rustc that cargo replays when a crate doesn't need rebuilding (#6933)

That would be really useful for some of my use cases so I can just focus on looking at critical errors when I need to.

[djc]

Would be great to have an interface like clippy's for cargo check. My use case is that usually during development I want to allow warnings (for things like unused imports which I may want to keep in place while doing exploratory work) but I definitely want to deny them from being merged into master, so doing this in CI is valuable.

[epage]

@djc could you clarify what you mean

For cargo check being like clippy, I assume you are referring to supporting -- -A warnings?

Also it sounds like you are wanting the denying of warnings to be the default for your project and users have to allow them. Is that correct? Is there a reason you want that rather than warn by default and have CI turn those warnings into errors?

[epage]

I talked briefly with @ehuss about this to get some more input on the idea. This is a summary of that specific conversation and doesn't represent the team as a whole.

The main points of interest

• Cautious about insta-stabilizing
• Concerned with the user experience if we decide to add -D -A -F -W flags as --warn vs --warnings would be confusing.

The first is just a process question. For the second, my hope is that we can generally move to lints being configured on a project basis. We have [lints] already. I see this flag being a step towards that.

I do think there is still a hole though for "warnings I want to know about but not block CI". An example is if you don't want to block on deprecations when upgrading. I've started https://internals.rust-lang.org/t/forbid-deny-warn-allow-and-notice/19986/20 to discuss these kinds of cases.

There is also experimentation but people can use RUSTFLAGS for that.

We'd like want input from clippy and others to see if there are cases we are missing.

With that said, one way to delay having to make a decision on any of this is by only starting this as a config setting build.warnings / CARGO_BUILD_WARNINGS.

[epage]

Based on the PR, a question would be what non-rustc warnings should be in-scope

• all cargo warnings (Add option to error when warnings are emitted, or ignore warnings #12875)
• project warnings (won't be meaningful until we have User control over cargo warnings #12235)
• project warnings with cargo check / cargo build (more fuzzy)

[BartMassey]

It would be great if cargo check -- -D warnings and cargo build -- -D warnings could be used like cargo rustc -- -D warnings. I want to check my students' code for warnings: without actually compiling it (slower); without messing with RUSTFLAGS (error-prone and sometimes awkward); without editing Cargo.toml (want to leave student code intact).

[BartMassey]

To add to the above, turns out cargo rustc -- -D warnings won't work on crates with both a binary and a library. So there's that.

[NickeZ]

I have a build script that prints "cargo::warning=Missing ENV variable that is important for release". This i fine during development but I want it to be a compile error in CI. Is that possible? Does Cargo have a flag to stop on warnings?

[ia0]

I have a build script that prints "cargo::warning=Missing ENV variable that is important for release". This i fine during development but I want it to be a compile error in CI. Is that possible? Does Cargo have a flag to stop on warnings?

That's a bit hacky, but you could probably check the environment for a CI variable and panic instead of printing cargo::warning=.

[NickeZ]

That's a bit hacky, but you could probably check the environment for a CI variable and panic instead of printing cargo::warning=.

Nice workaround!

[epage]

Tracking issue: #14802