diff --git a/Cargo.toml b/Cargo.toml index 8f6bc43b401..6a775e86261 100644 --- a/Cargo.toml +++ b/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" diff --git a/src/cargo/ops/fix.rs b/src/cargo/ops/fix.rs index 5d7ddb80345..52ca0780d78 100644 --- a/src/cargo/ops/fix.rs +++ b/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 { // 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 { // 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 { #[derive(Default)] struct FixedCrate { - messages: Vec, - original_files: HashMap, + files: HashMap, +} + +struct FixedFile { + errors_applying_fixes: Vec, + 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::()`. + // 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) -> ! { diff --git a/src/cargo/util/diagnostic_server.rs b/src/cargo/util/diagnostic_server.rs index 624762b1cc8..f5018753756 100644 --- a/src/cargo/util/diagnostic_server.rs +++ b/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, @@ -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")?; diff --git a/tests/testsuite/fix.rs b/tests/testsuite/fix.rs index 2dcad7934b8..af25d037bdf 100644 --- a/tests/testsuite/fix.rs +++ b/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() {} + 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::()")); +}