Merge pull request #94 from killercup/warn-when-rustfix-cant-replace-things

Custom warning when rustfix replacing fails

Author: alexcrichton

Cargo.toml

@@ -21,6 +21,7 @@ serde = "1.0"
 serde_json = "1.0"
 serde_derive = "1.0"
 failure = "0.1.1"
+log = "0.4.1"
 
 [dev-dependencies]
 duct = "0.8.2"

cargo-fix/src/cli.rs

@@ -102,6 +102,16 @@ fn log_message(msg: &Message, stream: &mut StandardStream) -> Result<(), Error>
                 stream,
             )?;
         }
+        ReplaceFailed { ref file, ref message } => {
+            stream.set_color(ColorSpec::new().set_bold(true).set_fg(Some(Color::Yellow)))?;
+            write!(stream, "warning")?;
+            stream.reset()?;
+            stream.set_color(ColorSpec::new().set_bold(true))?;
+            write!(stream, ": error applying suggestions to `{}`\n", file)?;
+            stream.reset()?;
+            write!(stream, "The full error message was:\n\n> {}\n\n", message)?;
+            stream.write(PLEASE_REPORT_THIS_BUG.as_bytes())?;
+        }
         FixFailed { ref files, ref krate } => {
             stream.set_color(ColorSpec::new().set_bold(true).set_fg(Some(Color::Yellow)))?;
             write!(stream, "warning")?;

cargo-fix/src/diagnostics.rs

@@ -17,6 +17,7 @@ static DIAGNOSICS_SERVER_VAR: &str = "__CARGO_FIX_DIAGNOSTICS_SERVER";
 pub enum Message {
     Fixing { file: String, fixes: usize },
     FixFailed { files: Vec<String>, krate: Option<String> },
+    ReplaceFailed { file: String, message: String },
 }
 
 impl Message {

cargo-fix/src/main.rs

@@ -229,11 +229,19 @@ fn rustfix_crate(rustc: &Path, filename: &str) -> Result<FixedCrate, Error> {
 
         messages.push(Message::fixing(&file, num_suggestions));
 
-        let new_code = rustfix::apply_suggestions(&code, &suggestions)?;
-        File::create(&file)
-            .and_then(|mut f| f.write_all(new_code.as_bytes()))
-            .with_context(|_| format!("failed to write file `{}`", file))?;
-        original_files.insert(file, code);
+        match rustfix::apply_suggestions(&code, &suggestions) {
+            Err(e) => {
+                diagnostics::Message::ReplaceFailed { file: file, message: e.to_string() }.post()?;
+                // TODO: Add flag to decide if we want to continue or bail out
+                continue;
+            }
+            Ok(new_code) => {
+                File::create(&file)
+                    .and_then(|mut f| f.write_all(new_code.as_bytes()))
+                    .with_context(|_| format!("failed to write file `{}`", file))?;
+                original_files.insert(file, code);
+            }
+        }
     }
 
     Ok(FixedCrate {

cargo-fix/tests/all/broken_lints.rs

@@ -0,0 +1,35 @@
+//! Ensure we give good error message when rustfix failes to apply changes
+//!
+//! TODO: Add rustc shim that outputs wrong suggestions instead of depending on
+//! actual rustc bugs!
+
+use super::project;
+
+#[test]
+fn tell_user_about_broken_lints() {
+    let p = project()
+        .file(
+            "src/lib.rs",
+            r#"
+                pub fn foo() {
+                    let mut i = 42;
+                }
+            "#,
+        )
+        .build();
+
+    p.expect_cmd("cargo-fix fix")
+        .stderr_contains(r"warning: error applying suggestions to `src/lib.rs`")
+        .stderr_contains("The full error message was:")
+        .stderr_contains("> Could not replace range 56...60 in file -- maybe parts of it were already replaced?")
+        .stderr_contains("\
+            This likely indicates a bug in either rustc or rustfix itself,\n\
+            and we would appreciate a bug report! You're likely to see \n\
+            a number of compiler warnings after this message which rustfix\n\
+            attempted to fix but failed. If you could open an issue at\n\
+            https://github.com/rust-lang-nursery/rustfix/issues\n\
+            quoting the full output of this command we'd be very appreciative!\n\n\
+        ")
+        .status(0)
+        .run();
+}

cargo-fix/tests/all/main.rs

@@ -316,6 +316,7 @@ fn diff(expected: &str, actual: &str) {
 }
 
 mod broken_build;
+mod broken_lints;
 mod dependencies;
 mod smoke;
 mod subtargets;

src/lib.rs

@@ -1,4 +1,6 @@
 #[macro_use]
+extern crate log;
+#[macro_use]
 extern crate failure;
 #[cfg(test)]
 #[macro_use]

src/replace.rs

@@ -89,8 +89,23 @@ impl Data {
                 .iter()
                 .position(|p| p.start <= from && p.end >= up_to_and_including)
                 .ok_or_else(|| {
+                    use log::Level::Debug;
+                    if log_enabled!(Debug) {
+                        let slices = self.parts
+                            .iter()
+                            .map(|p| (p.start, p.end, match p.data {
+                                State::Initial => "initial",
+                                State::Replaced(..) => "replaced",
+                            }))
+                            .collect::<Vec<_>>();
+                        debug!("no single slice covering {}...{}, current slices: {:?}",
+                            from, up_to_and_including, slices,
+                        );
+                    }
+
                     format_err!(
-                        "Could not find data slice that covers range {}..{}",
+                        "Could not replace range {}...{} in file \
+                        -- maybe parts of it were already replaced?",
                         from,
                         up_to_and_including
                     )