Auto merge of #5842 - alexcrichton:fix-lots, r=ehuss

fix: Iteratively apply suggestions from the compiler

This commit updates the `cargo fix` implementation to iteratively apply fixes
from the compiler instead of only once. Currently the compiler can sometimes
emit overlapping suggestions, such as in the case of transitioning

    ::foo::<::Bar>();

to ...

    crate::foo::<crate::Bar>();

and `rustfix` rightfully can't handle overlapping suggestions as there's no
clear way of how to disambiguate the fixes. To fix this problem Cargo will now
run `rustc` and `rustfix` multiple times, attempting to reach a steady state
where no fixes failed to apply.

Naturally this is a pretty tricky thing to do and we want to be sure that Cargo
doesn't loop forever, for example. A number of safeguards are in place to
prevent Cargo from going off into the weeds when fixing files, notably avoiding
to reattempt fixes if no successful fixes ended up being applied.

Closes #5813
Closes rust-lang/rust#52754

Author: bors

Cargo.toml

@@ -40,7 +40,7 @@ libc = "0.2"
 log = "0.4"
 libgit2-sys = "0.7.5"
 num_cpus = "1.0"
-rustfix = "0.4"
+rustfix = "0.4.2"
 same-file = "1"
 semver = { version = "0.9.0", features = ["serde"] }
 serde = "1.0"

src/cargo/ops/fix.rs

@@ -9,7 +9,7 @@ use std::str;
 use failure::{Error, ResultExt};
 use git2;
 use rustfix::diagnostics::Diagnostic;
-use rustfix;
+use rustfix::{self, CodeFix};
 use serde_json;
 
 use core::Workspace;
@@ -144,15 +144,18 @@ pub fn fix_maybe_exec_rustc() -> CargoResult<bool> {
     // If we didn't actually make any changes then we can immediately exec 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.original_files.is_empty() {
+    if !fixes.files.is_empty() {
         let mut cmd = Command::new(&rustc);
         args.apply(&mut cmd);
         cmd.arg("--error-format=json");
         let output = cmd.output().context("failed to spawn rustc")?;
 
         if output.status.success() {
-            for message in fixes.messages.drain(..) {
-                message.post()?;
+            for (path, file) in fixes.files.iter() {
+                Message::Fixing {
+                    file: path.clone(),
+                    fixes: file.fixes_applied,
+                }.post()?;
             }
         }
 
@@ -167,9 +170,9 @@ pub fn fix_maybe_exec_rustc() -> CargoResult<bool> {
         // user's code with our changes. Back out everything and fall through
         // below to recompile again.
         if !output.status.success() {
-            for (k, v) in fixes.original_files {
-                fs::write(&k, v)
-                    .with_context(|_| format!("failed to write file `{}`", k))?;
+            for (path, file) in fixes.files.iter() {
+                fs::write(path, &file.original_code)
+                    .with_context(|_| format!("failed to write file `{}`", path))?;
             }
             log_failed_fix(&output.stderr)?;
         }
@@ -182,8 +185,13 @@ pub fn fix_maybe_exec_rustc() -> CargoResult<bool> {
 
 #[derive(Default)]
 struct FixedCrate {
-    messages: Vec<Message>,
-    original_files: HashMap<String, String>,
+    files: HashMap<String, FixedFile>,
+}
+
+struct FixedFile {
+    errors_applying_fixes: Vec<String>,
+    fixes_applied: u32,
+    original_code: String,
 }
 
 fn rustfix_crate(lock_addr: &str, rustc: &Path, filename: &Path, args: &FixArgs)
@@ -192,11 +200,6 @@ fn rustfix_crate(lock_addr: &str, rustc: &Path, filename: &Path, args: &FixArgs)
     args.verify_not_preparing_for_enabled_edition()?;
     args.warn_if_preparing_probably_inert()?;
 
-    // If not empty, filter by these lints
-    //
-    // TODO: Implement a way to specify this
-    let only = HashSet::new();
-
     // 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.
@@ -205,7 +208,96 @@ fn rustfix_crate(lock_addr: &str, rustc: &Path, filename: &Path, args: &FixArgs)
     // argument that looks like a Rust file.
     let _lock = LockServerClient::lock(&lock_addr.parse()?, filename)?;
 
-    let mut cmd = Command::new(&rustc);
+    // 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
+    // rustc.
+    //
+    // This is currently done to handle code like:
+    //
+    //      ::foo::<::Bar>();
+    //
+    // where there are two fixes to happen here: `crate::foo::<crate::Bar>()`.
+    // The spans for these two suggestions are overlapping and its difficult in
+    // the compiler to *not* have overlapping spans here. As a result, a naive
+    // implementation would feed the two compiler suggestions for the above fix
+    // into `rustfix`, but one would be rejected because it overlaps with the
+    // other.
+    //
+    // In this case though, both suggestions are valid and can be automatically
+    // applied! To handle this case we execute rustc multiple times, collecting
+    // fixes each time we do so. Along the way we discard any suggestions that
+    // failed to apply, assuming that they can be fixed the next time we run
+    // rustc.
+    //
+    // Naturally we want a few protections in place here though to avoid looping
+    // forever or otherwise losing data. To that end we have a few termination
+    // conditions:
+    //
+    // * Do this whole process a fixed number of times. In theory we probably
+    //   need an infinite number of times to apply fixes, but we're not gonna
+    //   sit around waiting for that.
+    // * If it looks like a fix genuinely can't be applied we need to bail out.
+    //   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 = env::var("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);
+            file.errors_applying_fixes.clear(); // we'll generate new errors below
+        }
+        rustfix_and_fix(&mut fixes, rustc, filename, args)?;
+        let mut progress_yet_to_be_made = false;
+        for (path, file) in fixes.files.iter_mut() {
+            if file.errors_applying_fixes.len() == 0 {
+                continue
+            }
+            // 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) {
+                progress_yet_to_be_made = true;
+            }
+        }
+        if !progress_yet_to_be_made {
+            break
+        }
+    }
+
+    // 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 error in file.errors_applying_fixes.drain(..) {
+            Message::ReplaceFailed {
+                file: path.clone(),
+                message: error,
+            }.post()?;
+        }
+    }
+
+    Ok(fixes)
+}
+
+/// Execute `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, rustc: &Path, filename: &Path, args: &FixArgs)
+    -> Result<(), Error>
+{
+    // If not empty, filter by these lints
+    //
+    // TODO: Implement a way to specify this
+    let only = HashSet::new();
+
+    let mut cmd = Command::new(rustc);
     cmd.arg("--error-format=json");
     args.apply(&mut cmd);
     let output = cmd.output()
@@ -280,8 +372,6 @@ fn rustfix_crate(lock_addr: &str, rustc: &Path, filename: &Path, args: &FixArgs)
         filename.display(),
     );
 
-    let mut original_files = HashMap::with_capacity(file_map.len());
-    let mut messages = Vec::new();
     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
@@ -296,29 +386,35 @@ fn rustfix_crate(lock_addr: &str, rustc: &Path, filename: &Path, args: &FixArgs)
         let num_suggestions = suggestions.len();
         debug!("applying {} fixes to {}", num_suggestions, file);
 
-        messages.push(Message::fixing(&file, num_suggestions));
-
-        match rustfix::apply_suggestions(&code, &suggestions) {
-            Err(e) => {
-                Message::ReplaceFailed {
-                    file,
-                    message: e.to_string(),
-                }.post()?;
-                // TODO: Add flag to decide if we want to continue or bail out
-                continue;
-            }
-            Ok(new_code) => {
-                fs::write(&file, new_code)
-                    .with_context(|_| format!("failed to write file `{}`", file))?;
-                original_files.insert(file, code);
+        // If this file doesn't already exist then we just read the original
+        // 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 mut fixed = CodeFix::new(&code);
+
+        // As mentioned above in `rustfix_crate`, we don't immediately warn
+        // about suggestions that fail to apply here, and instead we save them
+        // off for later processing.
+        for suggestion in suggestions.iter().rev() {
+            match fixed.apply(suggestion) {
+                Ok(()) => fixed_file.fixes_applied += 1,
+                Err(e) => fixed_file.errors_applying_fixes.push(e.to_string()),
             }
         }
+        let new_code = fixed.finish()?;
+        fs::write(&file, new_code)
+            .with_context(|_| format!("failed to write file `{}`", file))?;
     }
 
-    Ok(FixedCrate {
-        messages,
-        original_files,
-    })
+    Ok(())
 }
 
 fn exit_with(status: ExitStatus) -> ! {

src/cargo/util/diagnostic_server.rs

@@ -30,7 +30,7 @@ const PLEASE_REPORT_THIS_BUG: &str =
 pub enum Message {
     Fixing {
         file: String,
-        fixes: usize,
+        fixes: u32,
     },
     FixFailed {
         files: Vec<String>,
@@ -51,13 +51,6 @@ pub enum Message {
 }
 
 impl Message {
-    pub fn fixing(file: &str, num: usize) -> Message {
-        Message::Fixing {
-            file: file.into(),
-            fixes: num,
-        }
-    }
-
     pub fn post(&self) -> Result<(), Error> {
         let addr = env::var(DIAGNOSICS_SERVER_VAR)
             .context("diagnostics collector misconfigured")?;

tests/testsuite/fix.rs

@@ -927,3 +927,42 @@ issues in preparation for the 2018 edition
             .with_status(0),
     );
 }
+
+#[test]
+fn fix_overlapping() {
+    if !is_nightly() {
+        return
+    }
+    let p = project()
+        .file(
+            "src/lib.rs",
+            r#"
+                #![feature(rust_2018_preview)]
+
+                pub fn foo<T>() {}
+                pub struct A;
+
+                pub mod bar {
+                    pub fn baz() {
+                        ::foo::<::A>();
+                    }
+                }
+            "#
+        )
+        .build();
+
+    let stderr = "\
+[CHECKING] foo [..]
+[FIXING] src[/]lib.rs (2 fixes)
+[FINISHED] dev [..]
+";
+
+    assert_that(
+        p.cargo("fix --allow-no-vcs --prepare-for 2018 --lib"),
+        execs().with_status(0).with_stderr(stderr),
+    );
+
+    let contents = p.read_file("src/lib.rs");
+    println!("{}", contents);
+    assert!(contents.contains("crate::foo::<crate::A>()"));
+}