Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add option to error when warnings are emitted, or ignore warnings #12875

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion src/bin/cargo/cli.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use anyhow::{anyhow, Context as _};
use cargo::core::shell::Shell;
use cargo::core::{features, CliUnstable};
use cargo::util::config::WarningHandling;
use cargo::{self, drop_print, drop_println, CargoResult, CliResult, Config};
use clap::{builder::UnknownArgumentValueParser, Arg, ArgMatches};
use itertools::Itertools;
Expand Down Expand Up @@ -181,7 +182,9 @@ Run with 'cargo -Z [FLAG] [COMMAND]'",
config_configure(config, &expanded_args, subcommand_args, global_args, &exec)?;
super::init_git(config);

exec.exec(config, subcommand_args)
let result = exec.exec(config, subcommand_args);
config.shell().error_for_warnings()?;
result
Comment on lines +185 to +187
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not every command comes back to this, like cargo run does an exec_replace

}

pub fn get_version_string(is_verbose: bool) -> String {
Expand Down Expand Up @@ -394,6 +397,13 @@ fn config_configure(
let frozen = args.flag("frozen") || global_args.frozen;
let locked = args.flag("locked") || global_args.locked;
let offline = args.flag("offline") || global_args.offline;
let warnings = match args.get_one::<String>("warnings").map(String::as_str) {
Some("ignore") => Some(WarningHandling::Ignore),
Some("warn") => Some(WarningHandling::Warn),
Some("error") => Some(WarningHandling::Error),
None => None,
_ => unreachable!(),
};
let mut unstable_flags = global_args.unstable_flags;
if let Some(values) = args.get_many::<String>("unstable-features") {
unstable_flags.extend(values.cloned());
Expand All @@ -405,6 +415,7 @@ fn config_configure(
config.configure(
verbose,
quiet,
warnings,
color,
frozen,
locked,
Expand Down Expand Up @@ -595,6 +606,11 @@ See '<cyan,bold>cargo help</> <cyan><<command>></>' for more information on a sp
.value_name("WHEN")
.global(true),
)
.arg(
opt("warnings", "Warning behavior")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be unstable initially via -Zunstable-options?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We likely should if we move forward with this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we have --config "term.warnings='error'" do we need the --warnings CLI option?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I lean towards

  • Stabilize the config and add the flag later (take the incremental approach).
    • That also means that we'll mostly have early adopters at first which will reduce the amount annoyance if we say skip cargo warnings and add them later
    • This will help us vet how much the flag is needed compared to the config
  • When we add the flag, maybe only put it on warning-heavy commands (cargo check, cargo build) to help people discover it without flooding every command with it (cargo version --warnings error?)

.value_parser(["error", "warn", "ignore"])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use PossibleValuesParser with TypedValueParser::map to make this work with the native type, rather than returning a string, centralizing the processing

.global(true),
)
.arg(
Arg::new("directory")
.help("Change to DIRECTORY before doing anything (nightly-only)")
Expand Down
7 changes: 5 additions & 2 deletions src/cargo/core/compiler/job_queue/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,7 @@ use crate::core::compiler::future_incompat::{
};
use crate::core::resolver::ResolveBehavior;
use crate::core::{PackageId, Shell, TargetKind};
use crate::util::config::WarningHandling;
use crate::util::diagnostic_server::{self, DiagnosticPrinter};
use crate::util::errors::AlreadyPrintedError;
use crate::util::machine_message::{self, Message as _};
Expand Down Expand Up @@ -314,8 +315,10 @@ impl<'cfg> DiagDedupe<'cfg> {
return Ok(false);
}
let mut shell = self.config.shell();
shell.print_ansi_stderr(diag.as_bytes())?;
shell.err().write_all(b"\n")?;
if shell.warnings() != WarningHandling::Ignore {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels weird that shell knows of this, seems more like a config thing

shell.print_ansi_stderr(diag.as_bytes())?;
shell.err().write_all(b"\n")?;
Comment on lines +318 to +320
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I missed it but how are we turning compiler warnings into errors?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're not turning the warnings into errors. They're still printed as warnings.

After all the warnings are emitted, a single error is emitted if any warnings were present.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put another way, how are we tracking compiler warnings to fail the process.

}
Ok(true)
}
}
Expand Down
43 changes: 40 additions & 3 deletions src/cargo/core/shell.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use std::io::IsTerminal;
use anstream::AutoStream;
use anstyle::Style;

use crate::util::config::WarningHandling;
use crate::util::errors::CargoResult;
use crate::util::style::*;

Expand Down Expand Up @@ -57,6 +58,10 @@ pub struct Shell {
/// Flag that indicates the current line needs to be cleared before
/// printing. Used when a progress bar is currently displayed.
needs_clear: bool,
/// How warning should be handled.
warnings: WarningHandling,
/// The number of warnings so far.
warning_count: usize,
}

impl fmt::Debug for Shell {
Expand Down Expand Up @@ -115,6 +120,8 @@ impl Shell {
},
verbosity: Verbosity::Verbose,
needs_clear: false,
warnings: WarningHandling::Warn,
warning_count: 0,
}
}

Expand All @@ -124,6 +131,8 @@ impl Shell {
output: ShellOut::Write(AutoStream::never(out)), // strip all formatting on write
verbosity: Verbosity::Verbose,
needs_clear: false,
warnings: WarningHandling::Warn,
warning_count: 0,
}
}

Expand Down Expand Up @@ -263,10 +272,13 @@ impl Shell {

/// Prints an amber 'warning' message.
pub fn warn<T: fmt::Display>(&mut self, message: T) -> CargoResult<()> {
match self.verbosity {
Verbosity::Quiet => Ok(()),
_ => self.print(&"warning", Some(&message), &WARN, false),
self.warning_count += 1;
if matches!(self.verbosity, Verbosity::Quiet)
|| matches!(self.warnings, WarningHandling::Ignore)
{
return Ok(());
}
self.print(&"warning", Some(&message), &WARN, false)
Comment on lines 274 to +281
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it only apply to rustc warnings (not Cargo warnings) until cargo has better control over diagnostics system (#12235)?

}

/// Prints a cyan 'note' message.
Expand All @@ -284,6 +296,31 @@ impl Shell {
self.verbosity
}

/// Updates how warnings are handled.
pub fn set_warnings(&mut self, warnings: WarningHandling) {
self.warnings = warnings;
}

/// Gets the warning handling behavior.
pub fn warnings(&self) -> WarningHandling {
self.warnings
}

/// Emit an error if any warnings have been seen.
pub fn error_for_warnings(&self) -> CargoResult<()> {
if self.warning_count > 0 && self.warnings == WarningHandling::Error {
let quiet_note = if self.verbosity == Verbosity::Quiet {
" (note that verbosity is set to quiet, which may hide warnings)"
} else {
""
};
anyhow::bail!(
"warnings detected and warnings are disallowed by configuration{quiet_note}"
);
}
Ok(())
}

/// Updates the color choice (always, never, or auto) from a string..
pub fn set_color_choice(&mut self, color: Option<&str>) -> CargoResult<()> {
if let ShellOut::Stream {
Expand Down
14 changes: 14 additions & 0 deletions src/cargo/util/config/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,16 @@ pub struct CredentialCacheValue {
pub operation_independent: bool,
}

/// Whether warnings should warn, be ignored, or cause an error.
#[derive(Debug, Copy, Clone, PartialEq, Eq, Deserialize, Default)]
#[serde(rename_all = "kebab-case")]
pub enum WarningHandling {
#[default]
Warn,
Ignore,
Error,
}

/// Configuration information for cargo. This is not specific to a build, it is information
/// relating to cargo itself.
#[derive(Debug)]
Expand Down Expand Up @@ -971,6 +981,7 @@ impl Config {
&mut self,
verbose: u32,
quiet: bool,
warnings: Option<WarningHandling>,
color: Option<&str>,
frozen: bool,
locked: bool,
Expand Down Expand Up @@ -1032,6 +1043,8 @@ impl Config {

self.shell().set_verbosity(verbosity);
self.shell().set_color_choice(color)?;
self.shell()
.set_warnings(warnings.or_else(|| term.warnings).unwrap_or_default());
self.progress_config = term.progress.unwrap_or_default();
self.extra_verbose = extra_verbose;
self.frozen = frozen;
Expand Down Expand Up @@ -2563,6 +2576,7 @@ struct TermConfig {
#[serde(default)]
#[serde(deserialize_with = "progress_or_string")]
progress: Option<ProgressConfig>,
warnings: Option<WarningHandling>,
}

#[derive(Debug, Default, Deserialize)]
Expand Down
11 changes: 11 additions & 0 deletions src/doc/man/includes/options-display.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,3 +22,14 @@ Control when colored output is used. Valid values:
May also be specified with the `term.color`
[config value](../reference/config.html).
{{/option}}

{{#option "`--warnings`"}}
Overrides how warnings are handled. Valid values:

* `warn` (default): warnings are displayed and do not fail the operation.
* `error`: if any warnings are encountered an error will be emitted at the of the operation.
* `ignore`: warnings will be silently ignored.

May also be specified with the `term.warnings`
[config value](../reference/config.html).
{{/option}}
13 changes: 13 additions & 0 deletions src/doc/src/reference/config.md
Original file line number Diff line number Diff line change
Expand Up @@ -1257,6 +1257,19 @@ Controls whether or not extra detailed messages are displayed by Cargo.
Specifying the `--quiet` flag will override and disable verbose output.
Specifying the `--verbose` flag will override and force verbose output.

#### `term.warnings`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where should the config option go?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

term feels wrong.

We have build but this will control other circumstances (maybe even cargo metadata!). However, build might be the first place people look.

Maybe we should have some kind of general category?

* Type: string
* Default: "warn"
* Environment: `CARGO_TERM_WARNINGS`

Overrides how warnings are handled, including warnings that come from `rustc`.

The `--warnings=` CLI option overrides this configuration value.

* `warn` (default): warnings are displayed and do not fail the operation.
* `error`: if any warnings are encountered an error will be emitted at the of the operation.
* `ignore`: warnings will be silently ignored.
Comment on lines +1269 to +1271
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How come you did error and ignore rather than deny/forbid and allow?

On one hand, the semantics are slightly different than the levels but on the other hand, they are familiar, predictable terms

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seemed clearer what the behavior would be. If we do deny for "emit an error" and allow for "ignore warnings" what do we call the default "warn" option?

Maybe deny/warn/allow ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

forbid might better fit our semantics, but either way that was what I had originally assumed.


#### `term.color`
* Type: string
* Default: "auto"
Expand Down
1 change: 1 addition & 0 deletions tests/testsuite/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ impl ConfigBuilder {
0,
false,
None,
None,
false,
false,
false,
Expand Down
1 change: 1 addition & 0 deletions tests/testsuite/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,7 @@ mod vendor;
mod verify_project;
mod version;
mod warn_on_failure;
mod warning_override;
mod weak_dep_features;
mod workspaces;
mod yank;
Expand Down
85 changes: 85 additions & 0 deletions tests/testsuite/warning_override.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
//! Tests for overriding warning behavior using `--warnings=` on the CLI or `term.warnings` config option.

use cargo_test_support::{project, Project};

const WARNING1: &'static str = "[WARNING] unused variable: `x`";
const WARNING2: &'static str = "[WARNING] unused config key `build.xyz` in `[..]`";

fn make_project(main_src: &str) -> Project {
project()
.file(
"Cargo.toml",
r#"
[package]
name = "foo"
version = "0.0.1"
authors = []
"#,
)
.file("src/main.rs", &format!("fn main() {{ {} }}", main_src))
.build()
}

#[cargo_test]
fn rustc_warnings() {
let p = make_project("let x = 3;");
p.cargo("check --warnings=warn")
.with_stderr_contains(WARNING1)
.run();

p.cargo("check --warnings=error")
.with_stderr_contains(WARNING1)
.with_stderr_contains(
"[ERROR] warnings detected and warnings are disallowed by configuration",
)
.with_status(101)
.run();

p.cargo("check --warnings=ignore")
.with_stderr_does_not_contain(WARNING1)
.run();
}

#[cargo_test]
fn config() {
let p = make_project("let x = 3;");
p.cargo("check")
.env("CARGO_TERM_WARNINGS", "error")
.with_stderr_contains(WARNING1)
.with_stderr_contains(
"[ERROR] warnings detected and warnings are disallowed by configuration",
)
.with_status(101)
.run();

// CLI has precedence over config
p.cargo("check --warnings=ignore")
.env("CARGO_TERM_WARNINGS", "error")
.with_stderr_does_not_contain(WARNING1)
.run();

p.cargo("check")
.env("CARGO_TERM_WARNINGS", "ignore")
.with_stderr_does_not_contain(WARNING1)
.run();
}

#[cargo_test]
/// Warnings that come from cargo rather than rustc
fn cargo_warnings() {
let p = make_project("");
p.change_file(".cargo/config.toml", "[build]\nxyz = false");
p.cargo("check").with_stderr_contains(WARNING2).run();

p.cargo("check --warnings=error")
.with_stderr_contains(WARNING2)
.with_stderr_contains(
"[ERROR] warnings detected and warnings are disallowed by configuration",
)
.with_status(101)
.run();

p.cargo("check --warnings=ignore")
.with_stderr_does_not_contain(WARNING2)
.run();
}