Auto merge of #10989 - Muscraft:add-auto-fix-note, r=weihanglo

add a note that some warnings (and/or errors) can be auto-fixed

This adds a note that some warnings (and/or errors) can be auto-fixed by running `cargo --fix`. This only supports `cargo check` as we can't show the same note for `cargo clippy` since it is an external subcommand.

The message for the note was taken from [#10976 (comment)](https://github.com/rust-lang/cargo/issues/10976#issuecomment-1212517765).

src/cargo/core/compiler/job_queue.rs

@@ -129,12 +129,8 @@ struct DrainState<'cfg> {
     messages: Arc<Queue<Message>>,
     /// 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<JobId, (usize, usize)>,
+    /// Count of warnings, used to print a summary after the job succeeds
+    warning_count: HashMap<JobId, WarningCount>,
     active: HashMap<JobId, Unit>,
     compiled: HashSet<PackageId>,
     documented: HashSet<PackageId>,
@@ -170,6 +166,50 @@ struct DrainState<'cfg> {
     per_package_future_incompat_reports: Vec<FutureIncompatReportPackage>,
 }
 
+/// 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<Acquired>),
@@ -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));

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<Diagnostic>` if we need to check them recursively
+                children: Vec<Diagnostic>,
             }
+
             if let Ok(mut msg) = serde_json::from_str::<CompilerMessage>(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);
             }

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();
+}

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");
+}