Auto merge of #13243 - ehuss:fix-fewer-rustc, r=epage

`cargo fix`: Call rustc fewer times.

This is an improvement of `cargo fix` so that it calls `rustc` one fewer times per target. The original code an extra step of calling `rustc` to display final diagnostics to the user. Part of the reason is that in the past, cargo did not always use JSON, and so `cargo fix` was forced to call `rustc` with and without JSON. Now that cargo uses JSON all the time, that is not necessary. This avoids the final call to `rustc` by remembering the original output from `rustc`.

This needs to keep track of both the first and last output from `rustc`. This is to handle the situation where `cargo fix` fails to apply some suggestion (either because the verification fails, or `rustfix` fails). The `cargo fix` output includes both the error, and the original diagnostics before the error.

The first commit is a little test framework to exercise the various edge cases around how fix works. The comments should explain how it works, but it essentially has a `rustc` replacement that emits various different diagnostics and counts how often it is called.

The subsequent commit includes the change to keep track of the output from `rustc` and to avoid the final call.

Fixes #13215

Author: bors

crates/rustfix/src/lib.rs

@@ -215,13 +215,16 @@ pub fn collect_suggestions<S: ::std::hash::BuildHasher>(
 /// 3. Calls [`CodeFix::finish`] to get the "fixed" code.
 pub struct CodeFix {
     data: replace::Data,
+    /// Whether or not the data has been modified.
+    modified: bool,
 }
 
 impl CodeFix {
     /// Creates a `CodeFix` with the source of a file to modify.
     pub fn new(s: &str) -> CodeFix {
         CodeFix {
             data: replace::Data::new(s.as_bytes()),
+            modified: false,
         }
     }
 
@@ -231,6 +234,7 @@ impl CodeFix {
             for r in &sol.replacements {
                 self.data
                     .replace_range(r.snippet.range.clone(), r.replacement.as_bytes())?;
+                self.modified = true;
             }
         }
         Ok(())
@@ -240,6 +244,11 @@ impl CodeFix {
     pub fn finish(&self) -> Result<String, Error> {
         Ok(String::from_utf8(self.data.to_vec())?)
     }
+
+    /// Returns whether or not the data has been modified.
+    pub fn modified(&self) -> bool {
+        self.modified
+    }
 }
 
 /// Applies multiple `suggestions` to the given `code`.

src/cargo/ops/fix.rs

@@ -35,13 +35,12 @@
 //!   applied cleanly, rustc is run again to verify the suggestions didn't
 //!   break anything. The change will be backed out if it fails (unless
 //!   `--broken-code` is used).
-//! - If there are any warnings or errors, rustc will be run one last time to
-//!   show them to the user.
 
 use std::collections::{BTreeSet, HashMap, HashSet};
 use std::ffi::OsString;
+use std::io::Write;
 use std::path::{Path, PathBuf};
-use std::process::{self, ExitStatus};
+use std::process::{self, ExitStatus, Output};
 use std::{env, fs, str};
 
 use anyhow::{bail, Context as _};
@@ -388,73 +387,94 @@ pub fn fix_exec_rustc(config: &Config, lock_addr: &str) -> CargoResult<()> {
     trace!("start rustfixing {:?}", args.file);
     let fixes = rustfix_crate(&lock_addr, &rustc, &args.file, &args, config)?;
 
-    // Ok now we have our final goal of testing out the changes that we applied.
-    // If these changes went awry and actually started to cause the crate to
-    // *stop* compiling then we want to back them out and continue to print
-    // warnings to the user.
-    //
-    // If we didn't actually make any changes then we can immediately execute the
-    // new rustc, and otherwise we capture the output to hide it in the scenario
-    // that we have to back it all out.
-    if !fixes.files.is_empty() {
-        debug!("calling rustc for final verification: {rustc}");
-        let output = rustc.output()?;
-
-        if output.status.success() {
-            for (path, file) in fixes.files.iter() {
-                Message::Fixed {
-                    file: path.clone(),
-                    fixes: file.fixes_applied,
-                }
-                .post(config)?;
+    if fixes.last_output.status.success() {
+        for (path, file) in fixes.files.iter() {
+            Message::Fixed {
+                file: path.clone(),
+                fixes: file.fixes_applied,
             }
+            .post(config)?;
         }
+        // Display any remaining diagnostics.
+        emit_output(&fixes.last_output)?;
+        return Ok(());
+    }
 
-        // If we succeeded then we'll want to commit to the changes we made, if
-        // any. If stderr is empty then there's no need for the final exec at
-        // the end, we just bail out here.
-        if output.status.success() && output.stderr.is_empty() {
-            return Ok(());
+    let allow_broken_code = config.get_env_os(BROKEN_CODE_ENV_INTERNAL).is_some();
+
+    // There was an error running rustc during the last run.
+    //
+    // Back out all of the changes unless --broken-code was used.
+    if !allow_broken_code {
+        for (path, file) in fixes.files.iter() {
+            debug!("reverting {:?} due to errors", path);
+            paths::write(path, &file.original_code)?;
         }
+    }
 
-        // Otherwise, if our rustc just failed, then that means that we broke the
-        // user's code with our changes. Back out everything and fall through
-        // below to recompile again.
-        if !output.status.success() {
-            if config.get_env_os(BROKEN_CODE_ENV_INTERNAL).is_none() {
-                for (path, file) in fixes.files.iter() {
-                    debug!("reverting {:?} due to errors", path);
-                    paths::write(path, &file.original_code)?;
+    // If there were any fixes, let the user know that there was a failure
+    // attempting to apply them, and to ask for a bug report.
+    //
+    // FIXME: The error message here is not correct with --broken-code.
+    //        https://github.com/rust-lang/cargo/issues/10955
+    if fixes.files.is_empty() {
+        // No fixes were available. Display whatever errors happened.
+        emit_output(&fixes.last_output)?;
+        exit_with(fixes.last_output.status);
+    } else {
+        let krate = {
+            let mut iter = rustc.get_args();
+            let mut krate = None;
+            while let Some(arg) = iter.next() {
+                if arg == "--crate-name" {
+                    krate = iter.next().and_then(|s| s.to_owned().into_string().ok());
                 }
             }
-
-            let krate = {
-                let mut iter = rustc.get_args();
-                let mut krate = None;
-                while let Some(arg) = iter.next() {
-                    if arg == "--crate-name" {
-                        krate = iter.next().and_then(|s| s.to_owned().into_string().ok());
-                    }
-                }
-                krate
-            };
-            log_failed_fix(config, krate, &output.stderr, output.status)?;
-        }
+            krate
+        };
+        log_failed_fix(
+            config,
+            krate,
+            &fixes.last_output.stderr,
+            fixes.last_output.status,
+        )?;
+        // Display the diagnostics that appeared at the start, before the
+        // fixes failed. This can help with diagnosing which suggestions
+        // caused the failure.
+        emit_output(&fixes.first_output)?;
+        // Exit with whatever exit code we initially started with. `cargo fix`
+        // treats this as a warning, and shouldn't return a failure code
+        // unless the code didn't compile in the first place.
+        exit_with(fixes.first_output.status);
     }
+}
 
-    // This final fall-through handles multiple cases;
-    // - If the fix failed, show the original warnings and suggestions.
-    // - If `--broken-code`, show the error messages.
-    // - If the fix succeeded, show any remaining warnings.
-    debug!("calling rustc to display remaining diagnostics: {rustc}");
-    exit_with(rustc.status()?);
+fn emit_output(output: &Output) -> CargoResult<()> {
+    // Unfortunately if there is output on stdout, this does not preserve the
+    // order of output relative to stderr. In practice, rustc should never
+    // print to stdout unless some proc-macro does it.
+    std::io::stderr().write_all(&output.stderr)?;
+    std::io::stdout().write_all(&output.stdout)?;
+    Ok(())
 }
 
-#[derive(Default)]
 struct FixedCrate {
+    /// Map of file path to some information about modifications made to that file.
     files: HashMap<String, FixedFile>,
+    /// The output from rustc from the first time it was called.
+    ///
+    /// This is needed when fixes fail to apply, so that it can display the
+    /// original diagnostics to the user which can help with diagnosing which
+    /// suggestions caused the failure.
+    first_output: Output,
+    /// The output from rustc from the last time it was called.
+    ///
+    /// This will be displayed to the user to show any remaining diagnostics
+    /// or errors.
+    last_output: Output,
 }
 
+#[derive(Debug)]
 struct FixedFile {
     errors_applying_fixes: Vec<String>,
     fixes_applied: u32,
@@ -472,11 +492,6 @@ fn rustfix_crate(
     args: &FixArgs,
     config: &Config,
 ) -> CargoResult<FixedCrate> {
-    if !args.can_run_rustfix(config)? {
-        // This fix should not be run. Skipping...
-        return Ok(FixedCrate::default());
-    }
-
     // First up, we want to make sure that each crate is only checked by one
     // process at a time. If two invocations concurrently check a crate then
     // it's likely to corrupt it.
@@ -488,6 +503,23 @@ fn rustfix_crate(
     // modification.
     let _lock = LockServerClient::lock(&lock_addr.parse()?, "global")?;
 
+    // Map of files that have been modified.
+    let mut files = HashMap::new();
+
+    if !args.can_run_rustfix(config)? {
+        // This fix should not be run. Skipping...
+        // We still need to run rustc at least once to make sure any potential
+        // rmeta gets generated, and diagnostics get displayed.
+        debug!("can't fix {filename:?}, running rustc: {rustc}");
+        let last_output = rustc.output()?;
+        let fixes = FixedCrate {
+            files,
+            first_output: last_output.clone(),
+            last_output,
+        };
+        return Ok(fixes);
+    }
+
     // Next up, this is a bit suspicious, but we *iteratively* execute rustc and
     // collect suggestions to feed to rustfix. Once we hit our limit of times to
     // execute rustc or we appear to be reaching a fixed point we stop running
@@ -521,41 +553,53 @@ fn rustfix_crate(
     //   Detect this when a fix fails to get applied *and* no suggestions
     //   successfully applied to the same file. In that case looks like we
     //   definitely can't make progress, so bail out.
-    let mut fixes = FixedCrate::default();
-    let mut last_fix_counts = HashMap::new();
-    let iterations = config
+    let max_iterations = config
         .get_env("CARGO_FIX_MAX_RETRIES")
         .ok()
         .and_then(|n| n.parse().ok())
         .unwrap_or(4);
-    for _ in 0..iterations {
-        last_fix_counts.clear();
-        for (path, file) in fixes.files.iter_mut() {
-            last_fix_counts.insert(path.clone(), file.fixes_applied);
+    let mut last_output;
+    let mut last_made_changes;
+    let mut first_output = None;
+    let mut current_iteration = 0;
+    loop {
+        for file in files.values_mut() {
             // We'll generate new errors below.
             file.errors_applying_fixes.clear();
         }
-        rustfix_and_fix(&mut fixes, rustc, filename, config)?;
+        (last_output, last_made_changes) = rustfix_and_fix(&mut files, rustc, filename, config)?;
+        if current_iteration == 0 {
+            first_output = Some(last_output.clone());
+        }
         let mut progress_yet_to_be_made = false;
-        for (path, file) in fixes.files.iter_mut() {
+        for (path, file) in files.iter_mut() {
             if file.errors_applying_fixes.is_empty() {
                 continue;
             }
+            debug!("had rustfix apply errors in {path:?} {file:?}");
             // If anything was successfully fixed *and* there's at least one
             // error, then assume the error was spurious and we'll try again on
             // the next iteration.
-            if file.fixes_applied != *last_fix_counts.get(path).unwrap_or(&0) {
+            if last_made_changes {
                 progress_yet_to_be_made = true;
             }
         }
         if !progress_yet_to_be_made {
             break;
         }
+        current_iteration += 1;
+        if current_iteration >= max_iterations {
+            break;
+        }
+    }
+    if last_made_changes {
+        debug!("calling rustc one last time for final results: {rustc}");
+        last_output = rustc.output()?;
     }
 
     // Any errors still remaining at this point need to be reported as probably
     // bugs in Cargo and/or rustfix.
-    for (path, file) in fixes.files.iter_mut() {
+    for (path, file) in files.iter_mut() {
         for error in file.errors_applying_fixes.drain(..) {
             Message::ReplaceFailed {
                 file: path.clone(),
@@ -565,19 +609,23 @@ fn rustfix_crate(
         }
     }
 
-    Ok(fixes)
+    Ok(FixedCrate {
+        files,
+        first_output: first_output.expect("at least one iteration"),
+        last_output,
+    })
 }
 
 /// Executes `rustc` to apply one round of suggestions to the crate in question.
 ///
 /// This will fill in the `fixes` map with original code, suggestions applied,
 /// and any errors encountered while fixing files.
 fn rustfix_and_fix(
-    fixes: &mut FixedCrate,
+    files: &mut HashMap<String, FixedFile>,
     rustc: &ProcessBuilder,
     filename: &Path,
     config: &Config,
-) -> CargoResult<()> {
+) -> CargoResult<(Output, bool)> {
     // If not empty, filter by these lints.
     // TODO: implement a way to specify this.
     let only = HashSet::new();
@@ -596,7 +644,7 @@ fn rustfix_and_fix(
             filename,
             output.status.code()
         );
-        return Ok(());
+        return Ok((output, false));
     }
 
     let fix_mode = config
@@ -664,6 +712,7 @@ fn rustfix_and_fix(
         filename.display(),
     );
 
+    let mut made_changes = false;
     for (file, suggestions) in file_map {
         // Attempt to read the source code for this file. If this fails then
         // that'd be pretty surprising, so log a message and otherwise keep
@@ -682,14 +731,11 @@ fn rustfix_and_fix(
         // code, so save it. If the file already exists then the original code
         // doesn't need to be updated as we've just read an interim state with
         // some fixes but perhaps not all.
-        let fixed_file = fixes
-            .files
-            .entry(file.clone())
-            .or_insert_with(|| FixedFile {
-                errors_applying_fixes: Vec::new(),
-                fixes_applied: 0,
-                original_code: code.clone(),
-            });
+        let fixed_file = files.entry(file.clone()).or_insert_with(|| FixedFile {
+            errors_applying_fixes: Vec::new(),
+            fixes_applied: 0,
+            original_code: code.clone(),
+        });
         let mut fixed = CodeFix::new(&code);
 
         // As mentioned above in `rustfix_crate`, we don't immediately warn
@@ -701,17 +747,19 @@ fn rustfix_and_fix(
                 Err(e) => fixed_file.errors_applying_fixes.push(e.to_string()),
             }
         }
-        let new_code = fixed.finish()?;
-        paths::write(&file, new_code)?;
+        if fixed.modified() {
+            made_changes = true;
+            let new_code = fixed.finish()?;
+            paths::write(&file, new_code)?;
+        }
     }
 
-    Ok(())
+    Ok((output, made_changes))
 }
 
 fn exit_with(status: ExitStatus) -> ! {
     #[cfg(unix)]
     {
-        use std::io::Write;
         use std::os::unix::prelude::*;
         if let Some(signal) = status.signal() {
             drop(writeln!(

src/cargo/util/diagnostic_server.rs

@@ -234,7 +234,7 @@ https://doc.rust-lang.org/edition-guide/editions/transitioning-an-existing-proje
 fn gen_please_report_this_bug_text(url: &str) -> String {
     format!(
         "This likely indicates a bug in either rustc or cargo itself,\n\
-     and we would appreciate a bug report! You're likely to see \n\
+     and we would appreciate a bug report! You're likely to see\n\
      a number of compiler warnings after this message which cargo\n\
      attempted to fix but failed. If you could open an issue at\n\
      {}\n\