fix: Iteratively apply suggestions from the compiler

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