From dd69ce1ab8d0863c8b97a6fa543eda332ec1e736 Mon Sep 17 00:00:00 2001 From: Scott Schafer Date: Tue, 18 Oct 2022 13:48:48 -0500 Subject: [PATCH] add a note that some warnings can be auto-fixed --- src/cargo/core/compiler/job_queue.rs | 93 ++++++++- src/cargo/core/compiler/mod.rs | 16 +- tests/testsuite/check.rs | 269 ++++++++++++++++++++++++++- tests/testsuite/install.rs | 38 ++++ tests/testsuite/messages.rs | 2 +- 5 files changed, 405 insertions(+), 13 deletions(-) diff --git a/src/cargo/core/compiler/job_queue.rs b/src/cargo/core/compiler/job_queue.rs index b568a5ee2fca..61b118501267 100644 --- a/src/cargo/core/compiler/job_queue.rs +++ b/src/cargo/core/compiler/job_queue.rs @@ -131,10 +131,11 @@ struct DrainState<'cfg> { 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 + /// First value is the total number of warnings, the second value is /// the number that were suppressed because they were duplicates of a - /// previous warning. - warning_count: HashMap, + /// previous warning, and the third value is for the number of fixable + /// warnings (set to -1 if there are any errors). + warning_count: HashMap, active: HashMap, compiled: HashSet, documented: HashSet, @@ -311,10 +312,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 +366,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 +381,7 @@ impl<'a, 'cfg> JobState<'a, 'cfg> { id: self.id, level, diag, + fixable, }); } Ok(()) @@ -679,14 +684,31 @@ 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" { + // If there is an error set fixable count to -1 so the `cargo fix` + // message does not show + let cnts = self.warning_count.entry(id).or_default(); + if fixable { + cnts.2 = -1; + } } } - 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,18 +1149,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; if !emitted { cnts.1 += 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 `-1` since that indicates there was an error while + // building this `Unit` + if cnts.2 >= 0 { + cnts.2 += 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, + Some(count) => { + // An error could add an entry for a `Unit` + // with 0 warnings but with count.2 = -1 + if count.0 > 0 { + count + } else { + return; + } + } None => return, }; let unit = &self.active[&id]; @@ -1160,6 +1198,41 @@ impl<'cfg> DrainState<'cfg> { 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 { + match count.2 { + // Do not show this if there are any errors (-1) or no fixable warnings + -1 | 0 => {} + n => { + // `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 named == "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", n); + if n > 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 affd415e3b42..f21963859237 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,9 @@ fn on_stderr_line_inner( rendered: String, message: String, level: String, + 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 +1446,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 cea327150d09..187e67887cae 100644 --- a/tests/testsuite/check.rs +++ b/tests/testsuite/check.rs @@ -6,7 +6,7 @@ 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 +1176,270 @@ 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" + authors = [] + "#, + ) + .file("src/lib.rs", "use std::io;") + .build(); + + foo.cargo("check") + .masquerade_as_nightly_cargo(&["auto-fix note"]) + .with_status(0) + .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" + authors = [] + "#, + ) + .file("src/lib.rs", "use std::io;") + .build(); + + foo.cargo("check") + .with_status(0) + .with_stderr_does_not_contain("[..] (run `cargo fix --lib -p foo` to apply 1 suggestion)") + .run(); +} + +#[cargo_test] +fn check_fixable_test_warning() { + let foo = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.0.1" + authors = [] + "#, + ) + .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_status(0) + .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") + .with_status(0) + .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" + authors = [] + "#, + ) + .file( + "src/lib.rs", + "use std::io;\n#[derive(Debug(x))]\nstruct Foo;", + ) + .build(); + + foo.cargo("check") + .masquerade_as_nightly_cargo(&["auto-fix note"]) + .with_status(101) + .with_stderr_does_not_contain("(run `cargo fix --lib -p foo` to apply 1 suggestion)[..]") + .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" + authors = [] + "#, + ) + .file("foo/src/lib.rs", "use std::io;") + .file( + "bar/Cargo.toml", + r#" + [package] + name = "bar" + version = "0.0.1" + authors = [] + + [dependencies] + foo = { path = "../foo" } + "#, + ) + .file("bar/src/lib.rs", "use std::io;") + .build(); + + p.cargo("check") + .masquerade_as_nightly_cargo(&["auto-fix note"]) + .with_status(0) + .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_status(0) + .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_status(0) + .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_status(0) + .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 70fff6aaaa78..0538a872f48f 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"); +} diff --git a/tests/testsuite/messages.rs b/tests/testsuite/messages.rs index 00d6b0df4b0a..c462273a63bc 100644 --- a/tests/testsuite/messages.rs +++ b/tests/testsuite/messages.rs @@ -103,7 +103,7 @@ fn deduplicate_messages_mismatched_warnings() { let expected_output = format!( "\ {}\ -warning: `foo` (lib) generated 1 warning +warning: `foo` (lib) generated 1 warning (run `cargo fix --lib -p foo` to apply 1 suggestion) {}\ warning: `foo` (lib test) generated 2 warnings (1 duplicate) [FINISHED] [..]