From 8ff8eaa49699c36a38ca67ebd34e5e273feb83c5 Mon Sep 17 00:00:00 2001 From: Scott Schafer Date: Thu, 27 Oct 2022 19:07:59 -0500 Subject: [PATCH] add a note that some warnings can be auto-fixed --- src/cargo/core/compiler/job_queue.rs | 141 ++++++++++++-- src/cargo/core/compiler/mod.rs | 17 +- tests/testsuite/check.rs | 273 ++++++++++++++++++++++++++- tests/testsuite/install.rs | 38 ++++ 4 files changed, 449 insertions(+), 20 deletions(-) diff --git a/src/cargo/core/compiler/job_queue.rs b/src/cargo/core/compiler/job_queue.rs index b568a5ee2fc..bd3a859f04d 100644 --- a/src/cargo/core/compiler/job_queue.rs +++ b/src/cargo/core/compiler/job_queue.rs @@ -129,12 +129,8 @@ struct DrainState<'cfg> { messages: Arc>, /// Diagnostic deduplication support. diag_dedupe: DiagDedupe<'cfg>, - /// Count of warnings, used to print a summary after the job succeeds. - /// - /// First value is the total number of warnings, and the second value is - /// the number that were suppressed because they were duplicates of a - /// previous warning. - warning_count: HashMap, + /// Count of warnings, used to print a summary after the job succeeds + warning_count: HashMap, active: HashMap, compiled: HashSet, documented: HashSet, @@ -170,6 +166,50 @@ struct DrainState<'cfg> { per_package_future_incompat_reports: Vec, } +/// Count of warnings, used to print a summary after the job succeeds +#[derive(Default)] +pub struct WarningCount { + /// total number of warnings + pub total: usize, + /// number of warnings that were suppressed because they + /// were duplicates of a previous warning + pub duplicates: usize, + /// number of fixable warnings set to `NotAllowed` + /// if any errors have been seen ofr the current + /// target + pub fixable: FixableWarnings, +} + +impl WarningCount { + /// If an error is seen this should be called + /// to set `fixable` to `NotAllowed` + fn disallow_fixable(&mut self) { + self.fixable = FixableWarnings::NotAllowed; + } + + /// Checks fixable if warnings are allowed + /// fixable warnings are allowed if no + /// errors have been seen for the current + /// target. If an error was seen `fixable` + /// will be `NotAllowed`. + fn fixable_allowed(&self) -> bool { + match &self.fixable { + FixableWarnings::NotAllowed => false, + _ => true, + } + } +} + +/// Used to keep track of how many fixable warnings there are +/// and if fixable warnings are allowed +#[derive(Default)] +pub enum FixableWarnings { + NotAllowed, + #[default] + Zero, + Positive(usize), +} + pub struct ErrorsDuringDrain { pub count: usize, } @@ -311,10 +351,12 @@ enum Message { id: JobId, level: String, diag: String, + fixable: bool, }, WarningCount { id: JobId, emitted: bool, + fixable: bool, }, FixDiagnostic(diagnostic_server::Message), Token(io::Result), @@ -363,13 +405,14 @@ impl<'a, 'cfg> JobState<'a, 'cfg> { Ok(()) } - pub fn emit_diag(&self, level: String, diag: String) -> CargoResult<()> { + pub fn emit_diag(&self, level: String, diag: String, fixable: bool) -> CargoResult<()> { if let Some(dedupe) = self.output { let emitted = dedupe.emit_diag(&diag)?; if level == "warning" { self.messages.push(Message::WarningCount { id: self.id, emitted, + fixable, }); } } else { @@ -377,6 +420,7 @@ impl<'a, 'cfg> JobState<'a, 'cfg> { id: self.id, level, diag, + fixable, }); } Ok(()) @@ -679,14 +723,28 @@ impl<'cfg> DrainState<'cfg> { shell.print_ansi_stderr(err.as_bytes())?; shell.err().write_all(b"\n")?; } - Message::Diagnostic { id, level, diag } => { + Message::Diagnostic { + id, + level, + diag, + fixable, + } => { let emitted = self.diag_dedupe.emit_diag(&diag)?; if level == "warning" { - self.bump_warning_count(id, emitted); + self.bump_warning_count(id, emitted, fixable); + } + if level == "error" { + let cnts = self.warning_count.entry(id).or_default(); + // If there is an error, the `cargo fix` message should not show + cnts.disallow_fixable(); } } - Message::WarningCount { id, emitted } => { - self.bump_warning_count(id, emitted); + Message::WarningCount { + id, + emitted, + fixable, + } => { + self.bump_warning_count(id, emitted, fixable); } Message::FixDiagnostic(msg) => { self.print.print(&msg)?; @@ -1127,19 +1185,34 @@ impl<'cfg> DrainState<'cfg> { Ok(()) } - fn bump_warning_count(&mut self, id: JobId, emitted: bool) { + fn bump_warning_count(&mut self, id: JobId, emitted: bool, fixable: bool) { let cnts = self.warning_count.entry(id).or_default(); - cnts.0 += 1; + cnts.total += 1; if !emitted { - cnts.1 += 1; + cnts.duplicates += 1; + // Don't add to fixable if it's already been emitted + } else if fixable { + // Do not add anything to the fixable warning count if + // is `NotAllowed` since that indicates there was an + // error while building this `Unit` + if cnts.fixable_allowed() { + cnts.fixable = match cnts.fixable { + FixableWarnings::NotAllowed => FixableWarnings::NotAllowed, + FixableWarnings::Zero => FixableWarnings::Positive(1), + FixableWarnings::Positive(fixable) => FixableWarnings::Positive(fixable + 1), + }; + } } } /// Displays a final report of the warnings emitted by a particular job. fn report_warning_count(&mut self, config: &Config, id: JobId) { let count = match self.warning_count.remove(&id) { - Some(count) => count, - None => return, + // An error could add an entry for a `Unit` + // with 0 warnings but having fixable + // warnings be disallowed + Some(count) if count.total > 0 => count, + None | Some(_) => return, }; let unit = &self.active[&id]; let mut message = format!("`{}` ({}", unit.pkg.name(), unit.target.description_named()); @@ -1151,15 +1224,47 @@ impl<'cfg> DrainState<'cfg> { message.push_str(" doc"); } message.push_str(") generated "); - match count.0 { + match count.total { 1 => message.push_str("1 warning"), n => drop(write!(message, "{} warnings", n)), }; - match count.1 { + match count.duplicates { 0 => {} 1 => message.push_str(" (1 duplicate)"), n => drop(write!(message, " ({} duplicates)", n)), } + // Only show the `cargo fix` message if its a local `Unit` + if unit.is_local() && config.nightly_features_allowed { + // Do not show this if there are any errors or no fixable warnings + if let FixableWarnings::Positive(fixable) = count.fixable { + // `cargo fix` doesnt have an option for custom builds + if !unit.target.is_custom_build() { + let mut command = { + let named = unit.target.description_named(); + // if its a lib we need to add the package to fix + if unit.target.is_lib() { + format!("{} -p {}", named, unit.pkg.name()) + } else { + named + } + }; + if unit.mode.is_rustc_test() + && !(unit.target.is_test() || unit.target.is_bench()) + { + command.push_str(" --tests"); + } + let mut suggestions = format!("{} suggestion", fixable); + if fixable > 1 { + suggestions.push_str("s") + } + drop(write!( + message, + " (run `cargo fix --{}` to apply {})", + command, suggestions + )) + } + } + } // Errors are ignored here because it is tricky to handle them // correctly, and they aren't important. drop(config.shell().warn(message)); diff --git a/src/cargo/core/compiler/mod.rs b/src/cargo/core/compiler/mod.rs index 6fac1d8014b..9fe859feee6 100644 --- a/src/cargo/core/compiler/mod.rs +++ b/src/cargo/core/compiler/mod.rs @@ -61,6 +61,7 @@ use crate::util::interning::InternedString; use crate::util::machine_message::{self, Message}; use crate::util::{add_path_args, internal, iter_join_onto, profile}; use cargo_util::{paths, ProcessBuilder, ProcessError}; +use rustfix::diagnostics::{Applicability, Diagnostic}; const RUSTDOC_CRATE_VERSION_FLAG: &str = "--crate-version"; @@ -1420,7 +1421,10 @@ fn on_stderr_line_inner( rendered: String, message: String, level: String, + // `children: Vec` if we need to check them recursively + children: Vec, } + if let Ok(mut msg) = serde_json::from_str::(compiler_message.get()) { if msg.message.starts_with("aborting due to") || msg.message.ends_with("warning emitted") @@ -1443,8 +1447,19 @@ fn on_stderr_line_inner( .expect("strip should never fail") }; if options.show_diagnostics { + let machine_applicable: bool = msg + .children + .iter() + .map(|child| { + child + .spans + .iter() + .filter_map(|span| span.suggestion_applicability) + .any(|app| app == Applicability::MachineApplicable) + }) + .any(|b| b); count_diagnostic(&msg.level, options); - state.emit_diag(msg.level, rendered)?; + state.emit_diag(msg.level, rendered, machine_applicable)?; } return Ok(true); } diff --git a/tests/testsuite/check.rs b/tests/testsuite/check.rs index cea327150d0..2cfb797cd4e 100644 --- a/tests/testsuite/check.rs +++ b/tests/testsuite/check.rs @@ -2,11 +2,12 @@ use std::fmt::{self, Write}; +use crate::messages::raw_rustc_output; use cargo_test_support::install::exe; use cargo_test_support::paths::CargoPathExt; use cargo_test_support::registry::Package; use cargo_test_support::tools; -use cargo_test_support::{basic_manifest, git, project}; +use cargo_test_support::{basic_bin_manifest, basic_manifest, git, project}; #[cargo_test] fn check_success() { @@ -1176,3 +1177,273 @@ fn git_manifest_with_project() { ) .run(); } + +#[cargo_test] +fn check_fixable_warning() { + let foo = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.0.1" + "#, + ) + .file("src/lib.rs", "use std::io;") + .build(); + + foo.cargo("check") + .masquerade_as_nightly_cargo(&["auto-fix note"]) + .with_stderr_contains("[..] (run `cargo fix --lib -p foo` to apply 1 suggestion)") + .run(); +} + +#[cargo_test] +fn check_fixable_not_nightly() { + let foo = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.0.1" + "#, + ) + .file("src/lib.rs", "use std::io;") + .build(); + + let rustc_message = raw_rustc_output(&foo, "src/lib.rs", &[]); + let expected_output = format!( + "\ +[CHECKING] foo v0.0.1 ([..]) +{}\ +[WARNING] `foo` (lib) generated 1 warning +[FINISHED] dev [unoptimized + debuginfo] target(s) in [..] +", + rustc_message + ); + foo.cargo("check").with_stderr(expected_output).run(); +} + +#[cargo_test] +fn check_fixable_test_warning() { + let foo = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.0.1" + "#, + ) + .file( + "src/lib.rs", + "\ +mod tests { + #[test] + fn t1() { + use std::io; + } +} + ", + ) + .build(); + + foo.cargo("check --all-targets") + .masquerade_as_nightly_cargo(&["auto-fix note"]) + .with_stderr_contains("[..] (run `cargo fix --lib -p foo --tests` to apply 1 suggestion)") + .run(); + foo.cargo("fix --lib -p foo --tests --allow-no-vcs").run(); + assert!(!foo.read_file("src/lib.rs").contains("use std::io;")); +} + +#[cargo_test] +fn check_fixable_error_no_fix() { + let foo = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.0.1" + "#, + ) + .file( + "src/lib.rs", + "use std::io;\n#[derive(Debug(x))]\nstruct Foo;", + ) + .build(); + + let rustc_message = raw_rustc_output(&foo, "src/lib.rs", &[]); + let expected_output = format!( + "\ +[CHECKING] foo v0.0.1 ([..]) +{}\ +[WARNING] `foo` (lib) generated 1 warning +[ERROR] could not compile `foo` due to previous error; 1 warning emitted +", + rustc_message + ); + foo.cargo("check") + .masquerade_as_nightly_cargo(&["auto-fix note"]) + .with_status(101) + .with_stderr(expected_output) + .run(); +} + +#[cargo_test] +fn check_fixable_warning_workspace() { + let p = project() + .file( + "Cargo.toml", + r#" + [workspace] + members = ["foo", "bar"] + "#, + ) + .file( + "foo/Cargo.toml", + r#" + [package] + name = "foo" + version = "0.0.1" + "#, + ) + .file("foo/src/lib.rs", "use std::io;") + .file( + "bar/Cargo.toml", + r#" + [package] + name = "bar" + version = "0.0.1" + + [dependencies] + foo = { path = "../foo" } + "#, + ) + .file("bar/src/lib.rs", "use std::io;") + .build(); + + p.cargo("check") + .masquerade_as_nightly_cargo(&["auto-fix note"]) + .with_stderr_contains("[..] (run `cargo fix --lib -p foo` to apply 1 suggestion)") + .with_stderr_contains("[..] (run `cargo fix --lib -p bar` to apply 1 suggestion)") + .run(); +} + +#[cargo_test] +fn check_fixable_example() { + let p = project() + .file("Cargo.toml", &basic_bin_manifest("foo")) + .file( + "src/main.rs", + r#" + fn hello() -> &'static str { + "hello" + } + + pub fn main() { + println!("{}", hello()) + } + "#, + ) + .file("examples/ex1.rs", "use std::fmt; fn main() {}") + .build(); + p.cargo("check --all-targets") + .masquerade_as_nightly_cargo(&["auto-fix note"]) + .with_stderr_contains("[..] (run `cargo fix --example \"ex1\"` to apply 1 suggestion)") + .run(); +} + +#[cargo_test(nightly, reason = "bench")] +fn check_fixable_bench() { + let p = project() + .file("Cargo.toml", &basic_bin_manifest("foo")) + .file( + "src/main.rs", + r#" + #![feature(test)] + #[cfg(test)] + extern crate test; + + fn hello() -> &'static str { + "hello" + } + + pub fn main() { + println!("{}", hello()) + } + + #[bench] + fn bench_hello(_b: &mut test::Bencher) { + use std::io; + assert_eq!(hello(), "hello") + } + "#, + ) + .file( + "benches/bench.rs", + " + #![feature(test)] + extern crate test; + + #[bench] + fn bench(_b: &mut test::Bencher) { use std::fmt; } + ", + ) + .build(); + p.cargo("check --all-targets") + .masquerade_as_nightly_cargo(&["auto-fix note"]) + .with_stderr_contains("[..] (run `cargo fix --bench \"bench\"` to apply 1 suggestion)") + .run(); +} + +#[cargo_test(nightly, reason = "bench")] +fn check_fixable_mixed() { + let p = project() + .file("Cargo.toml", &basic_bin_manifest("foo")) + .file( + "src/main.rs", + r#" + #![feature(test)] + #[cfg(test)] + extern crate test; + + fn hello() -> &'static str { + "hello" + } + + pub fn main() { + println!("{}", hello()) + } + + #[bench] + fn bench_hello(_b: &mut test::Bencher) { + use std::io; + assert_eq!(hello(), "hello") + } + #[test] + fn t1() { + use std::fmt; + } + "#, + ) + .file("examples/ex1.rs", "use std::fmt; fn main() {}") + .file( + "benches/bench.rs", + " + #![feature(test)] + extern crate test; + + #[bench] + fn bench(_b: &mut test::Bencher) { use std::fmt; } + ", + ) + .build(); + p.cargo("check --all-targets") + .masquerade_as_nightly_cargo(&["auto-fix note"]) + .with_stderr_contains("[..] (run `cargo fix --bin \"foo\" --tests` to apply 2 suggestions)") + .with_stderr_contains("[..] (run `cargo fix --example \"ex1\"` to apply 1 suggestion)") + .with_stderr_contains("[..] (run `cargo fix --bench \"bench\"` to apply 1 suggestion)") + .run(); +} diff --git a/tests/testsuite/install.rs b/tests/testsuite/install.rs index 70fff6aaaa7..0538a872f48 100644 --- a/tests/testsuite/install.rs +++ b/tests/testsuite/install.rs @@ -2031,3 +2031,41 @@ fn install_semver_metadata() { ) .run(); } + +#[cargo_test] +fn no_auto_fix_note() { + Package::new("auto_fix", "0.0.1") + .file("src/lib.rs", "use std::io;") + .file( + "src/main.rs", + &format!("extern crate {}; use std::io; fn main() {{}}", "auto_fix"), + ) + .publish(); + + // This should not contain a suggestion to run `cargo fix` + // + // This is checked by matching the full output as `with_stderr_does_not_contain` + // can be brittle + cargo_process("install auto_fix") + .masquerade_as_nightly_cargo(&["auto-fix note"]) + .with_stderr( + "\ +[UPDATING] `[..]` index +[DOWNLOADING] crates ... +[DOWNLOADED] auto_fix v0.0.1 (registry [..]) +[INSTALLING] auto_fix v0.0.1 +[COMPILING] auto_fix v0.0.1 +[FINISHED] release [optimized] target(s) in [..] +[INSTALLING] [CWD]/home/.cargo/bin/auto_fix[EXE] +[INSTALLED] package `auto_fix v0.0.1` (executable `auto_fix[EXE]`) +[WARNING] be sure to add `[..]` to your PATH to be able to run the installed binaries +", + ) + .run(); + assert_has_installed_exe(cargo_home(), "auto_fix"); + + cargo_process("uninstall auto_fix") + .with_stderr("[REMOVING] [CWD]/home/.cargo/bin/auto_fix[EXE]") + .run(); + assert_has_not_installed_exe(cargo_home(), "auto_fix"); +}