Merge pull request #93 from alexcrichton/warn-when-break

Warn users when fixes break code

Author: killercup

cargo-fix/src/cli.rs

@@ -1,14 +1,24 @@
 use std::env;
 use std::process::Command;
+use std::io::Write;
 
-use failure::{Error, ResultExt};
 use clap::{App, AppSettings, Arg, SubCommand};
-use atty;
+use failure::{Error, ResultExt};
+use termcolor::{Color, ColorSpec, StandardStream, WriteColor};
 
 use lock;
-use diagnostics;
+use diagnostics::{Message, Server};
 use super::exit_with;
 
+static PLEASE_REPORT_THIS_BUG: &str = "\
+    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\
+";
+
 pub fn run() -> Result<(), Error> {
     let matches = App::new("Cargo Fix")
         .bin_name("cargo")
@@ -41,8 +51,10 @@ pub fn run() -> Result<(), Error> {
 
     // Spin up our diagnostics server which our subprocesses will use to send
     // use their dignostics messages in an ordered way.
-    let _lockserver = diagnostics::Server::new()?.start(|m| {
-        let _ = log_message(&m);
+    let _lockserver = Server::new()?.start(|m, stream| {
+        if let Err(e) = log_message(&m, stream) {
+            warn!("failed to log message: {}", e);
+        }
     })?;
 
     let cargo = env::var_os("CARGO").unwrap_or("cargo".into());
@@ -74,7 +86,7 @@ pub fn run() -> Result<(), Error> {
     exit_with(status);
 }
 
-fn log_message(msg: &diagnostics::Message) -> Result<(), Error> {
+fn log_message(msg: &Message, stream: &mut StandardStream) -> Result<(), Error> {
     use diagnostics::Message::*;
 
     match *msg {
@@ -87,33 +99,51 @@ fn log_message(msg: &diagnostics::Message) -> Result<(), Error> {
                     n = fixes,
                     fixes = if *fixes > 1 { "fixes" } else { "fix" },
                 ),
+                stream,
             )?;
         }
+        FixFailed { ref files, ref krate } => {
+            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, ": ")?;
+            if let Some(ref krate) = *krate {
+                write!(
+                    stream,
+                    "failed to automatically apply fixes suggested by rustc \
+                     to crate `{}`\n",
+                    krate,
+                )?;
+            } else {
+                write!(stream, "failed to automatically apply fixes suggested by rustc\n")?;
+            }
+            if files.len() > 0 {
+                write!(
+                    stream,
+                    "\nafter fixes were automatically applied the compiler \
+                     reported errors within these files:\n\n"
+                )?;
+                for file in files {
+                    write!(stream, "  * {}\n", file)?;
+                }
+                write!(stream, "\n")?;
+
+            }
+            stream.write(PLEASE_REPORT_THIS_BUG.as_bytes())?;
+        }
     }
 
+    stream.reset()?;
+    stream.flush()?;
     Ok(())
 }
 
-fn log_for_human(kind: &str, msg: &str) -> Result<(), Error> {
-    use std::io::Write;
-    use termcolor::{Color, ColorChoice, ColorSpec, StandardStream, WriteColor};
-
-    // Adapted from cargo, cf. <https://github.com/rust-lang/cargo/blob/5986492773e6de61cced57f457da3700607f4a3a/src/cargo/core/shell.rs#L291>
-    let color_choice = if atty::is(atty::Stream::Stderr) {
-        ColorChoice::Auto
-    } else {
-        ColorChoice::Never
-    };
-    let mut stream = StandardStream::stderr(color_choice);
-    stream.reset()?;
-
+fn log_for_human(kind: &str, msg: &str, stream: &mut StandardStream) -> Result<(), Error> {
     stream.set_color(ColorSpec::new().set_bold(true).set_fg(Some(Color::Cyan)))?;
     // Justify to 12 chars just like cargo
-    write!(&mut stream, "{:>12}", kind)?;
+    write!(stream, "{:>12}", kind)?;
     stream.reset()?;
-
-    write!(&mut stream, " {}\n", msg)?;
-    stream.flush()?;
-
+    write!(stream, " {}\n", msg)?;
     Ok(())
 }

cargo-fix/src/diagnostics.rs

@@ -6,14 +6,17 @@ use std::io::{BufReader, Write, Read};
 use std::net::{Shutdown, SocketAddr, TcpListener, TcpStream};
 use std::thread::{self, JoinHandle};
 
+use atty;
 use failure::{Error, ResultExt};
 use serde_json;
+use termcolor::{ColorChoice, StandardStream};
 
 static DIAGNOSICS_SERVER_VAR: &str = "__CARGO_FIX_DIAGNOSTICS_SERVER";
 
 #[derive(Deserialize, Serialize)]
 pub enum Message {
     Fixing { file: String, fixes: usize },
+    FixFailed { files: Vec<String>, krate: Option<String> },
 }
 
 impl Message {
@@ -62,10 +65,9 @@ impl Server {
         Ok(Server { listener })
     }
 
-    pub fn start<F: Fn(Message) + Send + 'static>(
-        self,
-        on_message: F,
-    ) -> Result<StartedServer, Error> {
+    pub fn start<F>(self, on_message: F) -> Result<StartedServer, Error>
+        where F: Fn(Message, &mut StandardStream) + Send + 'static,
+    {
         let _addr = self.listener.local_addr()?;
         let thread = thread::spawn(move || {
             self.run(on_message);
@@ -77,11 +79,19 @@ impl Server {
         })
     }
 
-    fn run<F: Fn(Message)>(self, on_message: F) {
+    fn run<F>(self, on_message: F)
+        where F: Fn(Message, &mut StandardStream)
+    {
+        let color_choice = if atty::is(atty::Stream::Stderr) {
+            ColorChoice::Auto
+        } else {
+            ColorChoice::Never
+        };
+        let mut stream = StandardStream::stderr(color_choice);
         while let Ok((client, _)) = self.listener.accept() {
             let mut client = BufReader::new(client);
             match serde_json::from_reader(client) {
-                Ok(message) => on_message(message),
+                Ok(message) => on_message(message, &mut stream),
                 Err(e) => {
                     warn!("invalid diagnostics message: {}", e);
                 }

cargo-fix/src/main.rs

@@ -11,7 +11,7 @@ extern crate serde_derive;
 extern crate serde_json;
 extern crate termcolor;
 
-use std::collections::{HashMap, HashSet};
+use std::collections::{HashMap, HashSet, BTreeSet};
 use std::env;
 use std::fs::File;
 use std::io::{Read, Write};
@@ -86,6 +86,7 @@ fn cargo_fix_rustc() -> Result<(), Error> {
     let mut cmd = Command::new(&rustc);
     cmd.args(env::args().skip(1));
     cmd.arg("--cap-lints=warn");
+    cmd.arg("--error-format=json");
     if fixes.original_files.len() > 0 {
         let output = cmd.output().context("failed to spawn rustc")?;
 
@@ -111,9 +112,13 @@ fn cargo_fix_rustc() -> Result<(), Error> {
                     .and_then(|mut f| f.write_all(v.as_bytes()))
                     .with_context(|_| format!("failed to write file `{}`", k))?;
             }
+            log_failed_fix(&output.stderr)?;
         }
     }
 
+    let mut cmd = Command::new(&rustc);
+    cmd.args(env::args().skip(1));
+    cmd.arg("--cap-lints=warn");
     exit_with(cmd.status().context("failed to spawn rustc")?);
 }
 
@@ -248,3 +253,36 @@ fn exit_with(status: ExitStatus) -> ! {
     }
     process::exit(status.code().unwrap_or(3));
 }
+
+fn log_failed_fix(stderr: &[u8]) -> Result<(), Error> {
+    let stderr = str::from_utf8(stderr)
+        .context("failed to parse rustc stderr as utf-8")?;
+
+    let diagnostics = stderr.lines()
+        .filter(|x| !x.is_empty())
+        .filter_map(|line| serde_json::from_str::<Diagnostic>(line).ok());
+    let mut files = BTreeSet::new();
+    for diagnostic in diagnostics {
+        for span in diagnostic.spans.into_iter() {
+            files.insert(span.file_name);
+        }
+    }
+    let mut krate = None;
+    let mut prev_dash_dash_krate_name = false;
+    for arg in env::args() {
+        if prev_dash_dash_krate_name {
+            krate = Some(arg.clone());
+        }
+
+        if arg == "--crate-name" {
+            prev_dash_dash_krate_name = true;
+        } else {
+            prev_dash_dash_krate_name = false;
+        }
+    }
+
+    let files = files.into_iter().collect();
+    Message::FixFailed { files, krate }.post()?;
+
+    Ok(())
+}

cargo-fix/tests/all/broken_build.rs

@@ -53,11 +53,12 @@ fn broken_fixes_backed_out() {
         )
         .file(
             "foo/src/main.rs",
-            r#"
+            r##"
                 use std::env;
-                use std::process::{self, Command};
-                use std::path::{Path, PathBuf};
                 use std::fs;
+                use std::io::Write;
+                use std::path::{Path, PathBuf};
+                use std::process::{self, Command};
 
                 fn main() {
                     let is_lib_rs = env::args_os()
@@ -67,7 +68,10 @@ fn broken_fixes_backed_out() {
                         let path = PathBuf::from(env::var_os("OUT_DIR").unwrap());
                         let path = path.join("foo");
                         if path.exists() {
-                            panic!("failing the second compilation, oh no!");
+                            fs::File::create("src/lib.rs")
+                                .unwrap()
+                                .write_all(b"not rust code")
+                                .unwrap();
                         } else {
                             fs::File::create(&path).unwrap();
                         }
@@ -79,7 +83,7 @@ fn broken_fixes_backed_out() {
                         .expect("failed to run rustc");
                     process::exit(status.code().unwrap_or(2));
                 }
-            "#,
+            "##,
         )
         .file(
             "bar/Cargo.toml",
@@ -109,11 +113,26 @@ fn broken_fixes_backed_out() {
     p.expect_cmd("cargo-fix fix")
         .cwd("bar")
         .env("RUSTC", p.root.join("foo/target/debug/foo"))
-        .stderr_contains("oh no")
+        .stderr_contains("not rust code")
+        .stderr_contains(
+            "\
+                warning: failed to automatically apply fixes suggested by rustc \
+                to crate `bar`\n\
+                \n\
+                after fixes were automatically applied the compiler reported \
+                errors within these files:\n\
+                \n  \
+                * src/lib.rs\n\
+                \n\
+                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\
+            "
+        )
         .stderr_not_contains("[FIXING]")
         .status(101)
         .run();
-
-    // Make sure the fix which should have been applied was backed out
-    assert!(p.read("bar/src/lib.rs").contains("let mut x = 3;"));
 }