Auto merge of #9799 - ehuss:fix-abnormal-error, r=alexcrichton

Show information about abnormal `fix` errors.

During a recent crater run, we ran into a few circumstances where `cargo fix` failed unexpectedly, and we can't reproduce the errors locally.  The sequence was:

1. Cargo ran `rustc` and collected the diagnostics to apply, and modified the files.
2. Cargo ran `rustc` again to verify the fixes. This step failed, but only emitted warnings.
3. Cargo ran `rustc` again to show the original diagnostics, and this exited normally with warnings.

We don't know why the second step failed.  This change makes it so that cargo will collect any non-diagnostic messages (like ICEs), and will also display the exit code if it is abnormal.

Author: bors

src/cargo/ops/fix.rs

@@ -46,7 +46,7 @@ use std::process::{self, Command, ExitStatus};
 use std::str;
 
 use anyhow::{bail, Context, Error};
-use cargo_util::{paths, ProcessBuilder};
+use cargo_util::{exit_status_to_string, is_simple_exit_code, paths, ProcessBuilder};
 use log::{debug, trace, warn};
 use rustfix::diagnostics::Diagnostic;
 use rustfix::{self, CodeFix};
@@ -374,7 +374,7 @@ pub fn fix_maybe_exec_rustc(config: &Config) -> CargoResult<bool> {
                     paths::write(path, &file.original_code)?;
                 }
             }
-            log_failed_fix(&output.stderr)?;
+            log_failed_fix(&output.stderr, output.status)?;
         }
     }
 
@@ -662,7 +662,7 @@ fn exit_with(status: ExitStatus) -> ! {
     process::exit(status.code().unwrap_or(3));
 }
 
-fn log_failed_fix(stderr: &[u8]) -> Result<(), Error> {
+fn log_failed_fix(stderr: &[u8], status: ExitStatus) -> Result<(), Error> {
     let stderr = str::from_utf8(stderr).context("failed to parse rustc stderr as utf-8")?;
 
     let diagnostics = stderr
@@ -677,6 +677,13 @@ fn log_failed_fix(stderr: &[u8]) -> Result<(), Error> {
             files.insert(span.file_name);
         }
     }
+    // Include any abnormal messages (like an ICE or whatever).
+    errors.extend(
+        stderr
+            .lines()
+            .filter(|x| !x.starts_with('{'))
+            .map(|x| x.to_string()),
+    );
     let mut krate = None;
     let mut prev_dash_dash_krate_name = false;
     for arg in env::args() {
@@ -692,10 +699,16 @@ fn log_failed_fix(stderr: &[u8]) -> Result<(), Error> {
     }
 
     let files = files.into_iter().collect();
+    let abnormal_exit = if status.code().map_or(false, is_simple_exit_code) {
+        None
+    } else {
+        Some(exit_status_to_string(status))
+    };
     Message::FixFailed {
         files,
         krate,
         errors,
+        abnormal_exit,
     }
     .post()?;
 

src/cargo/util/diagnostic_server.rs

@@ -48,6 +48,7 @@ pub enum Message {
         files: Vec<String>,
         krate: Option<String>,
         errors: Vec<String>,
+        abnormal_exit: Option<String>,
     },
     ReplaceFailed {
         file: String,
@@ -135,6 +136,7 @@ impl<'a> DiagnosticPrinter<'a> {
                 files,
                 krate,
                 errors,
+                abnormal_exit,
             } => {
                 if let Some(ref krate) = *krate {
                     self.config.shell().warn(&format!(
@@ -171,6 +173,13 @@ impl<'a> DiagnosticPrinter<'a> {
                         }
                     }
                 }
+                if let Some(exit) = abnormal_exit {
+                    writeln!(
+                        self.config.shell().err(),
+                        "rustc exited abnormally: {}",
+                        exit
+                    )?;
+                }
                 writeln!(
                     self.config.shell().err(),
                     "Original diagnostics will follow.\n"

tests/testsuite/fix.rs

@@ -3,7 +3,7 @@
 use cargo::core::Edition;
 use cargo_test_support::compare::assert_match_exact;
 use cargo_test_support::git;
-use cargo_test_support::paths::CargoPathExt;
+use cargo_test_support::paths::{self, CargoPathExt};
 use cargo_test_support::registry::{Dependency, Package};
 use cargo_test_support::tools;
 use cargo_test_support::{basic_manifest, is_nightly, project};
@@ -1597,3 +1597,82 @@ fn fix_shared_cross_workspace() {
         &p.read_file("foo/src/shared.rs"),
     );
 }
+
+#[cargo_test]
+fn abnormal_exit() {
+    // rustc fails unexpectedly after applying fixes, should show some error information.
+    //
+    // This works with a proc-macro that runs three times:
+    // - First run (collect diagnostics pass): writes a file, exits normally.
+    // - Second run (verify diagnostics work): it detects the presence of the
+    //   file, removes the file, and aborts the process.
+    // - Third run (collecting messages to display): file not found, exits normally.
+    let p = project()
+        .file(
+            "Cargo.toml",
+            r#"
+                [package]
+                name = "foo"
+                version = "0.1.0"
+
+                [dependencies]
+                pm = {path="pm"}
+            "#,
+        )
+        .file(
+            "src/lib.rs",
+            r#"
+                pub fn f() {
+                    let mut x = 1;
+                    pm::crashme!();
+                }
+            "#,
+        )
+        .file(
+            "pm/Cargo.toml",
+            r#"
+                [package]
+                name = "pm"
+                version = "0.1.0"
+                edition = "2018"
+
+                [lib]
+                proc-macro = true
+            "#,
+        )
+        .file(
+            "pm/src/lib.rs",
+            r#"
+                use proc_macro::TokenStream;
+                #[proc_macro]
+                pub fn crashme(_input: TokenStream) -> TokenStream {
+                    // Use a file to succeed on the first pass, and fail on the second.
+                    let p = std::env::var_os("ONCE_PATH").unwrap();
+                    let check_path = std::path::Path::new(&p);
+                    if check_path.exists() {
+                        eprintln!("I'm not a diagnostic.");
+                        std::fs::remove_file(check_path).unwrap();
+                        std::process::abort();
+                    } else {
+                        std::fs::write(check_path, "").unwrap();
+                        "".parse().unwrap()
+                    }
+                }
+            "#,
+        )
+        .build();
+
+    p.cargo("fix --lib --allow-no-vcs")
+        .env(
+            "ONCE_PATH",
+            paths::root().join("proc-macro-run-once").to_str().unwrap(),
+        )
+        .with_stderr_contains(
+            "[WARNING] failed to automatically apply fixes suggested by rustc to crate `foo`",
+        )
+        .with_stderr_contains("I'm not a diagnostic.")
+        // "signal: 6, SIGABRT: process abort signal" on some platforms
+        .with_stderr_contains("rustc exited abnormally: [..]")
+        .with_stderr_contains("Original diagnostics will follow.")
+        .run();
+}