Merge pull request #91 from alexcrichton/do-not-lie-if-not-fixing

Don't print "Fixing" if fixes are reverted

Author: killercup

cargo-fix/src/diagnostics.rs

@@ -2,8 +2,8 @@
 //! cross-platform way.
 
 use std::env;
-use std::io::BufReader;
-use std::net::{SocketAddr, TcpListener, TcpStream};
+use std::io::{BufReader, Write, Read};
+use std::net::{Shutdown, SocketAddr, TcpListener, TcpStream};
 use std::thread::{self, JoinHandle};
 
 use failure::{Error, ResultExt};
@@ -29,8 +29,16 @@ impl Message {
         let mut client =
             TcpStream::connect(&addr).context("failed to connect to parent diagnostics target")?;
 
-        serde_json::to_writer(&mut client, &self)
+        let s = serde_json::to_string(self)
+            .context("failed to serialize message")?;
+        client.write_all(s.as_bytes())
             .context("failed to write message to diagnostics target")?;
+        client.shutdown(Shutdown::Write)
+            .context("failed to shutdown")?;
+
+        let mut tmp = Vec::new();
+        client.read_to_end(&mut tmp)
+            .context("failed to receive a disconnect")?;
 
         Ok(())
     }

cargo-fix/src/main.rs

@@ -22,6 +22,8 @@ use std::path::Path;
 use rustfix::diagnostics::Diagnostic;
 use failure::{Error, ResultExt};
 
+use diagnostics::Message;
+
 mod cli;
 mod lock;
 mod diagnostics;
@@ -65,11 +67,11 @@ fn cargo_fix_rustc() -> Result<(), Error> {
     // To that end we only actually try to fix things if it looks like we're
     // compiling a Rust file and it *doesn't* have an absolute filename. That's
     // not the best heuristic but matches what Cargo does today at least.
-    let mut files_to_restore = HashMap::new();
+    let mut fixes = FixedCrate::default();
     if let Some(path) = filename {
         if !Path::new(&path).is_absolute() {
             trace!("start rustfixing {:?}", path);
-            files_to_restore = rustfix_crate(rustc.as_ref(), &path)?;
+            fixes = rustfix_crate(rustc.as_ref(), &path)?;
         }
     }
 
@@ -84,9 +86,15 @@ fn cargo_fix_rustc() -> Result<(), Error> {
     let mut cmd = Command::new(&rustc);
     cmd.args(env::args().skip(1));
     cmd.arg("--cap-lints=warn");
-    if files_to_restore.len() > 0 {
+    if fixes.original_files.len() > 0 {
         let output = cmd.output().context("failed to spawn rustc")?;
 
+        if output.status.success() {
+            for message in fixes.messages.drain(..) {
+                message.post()?;
+            }
+        }
+
         // If we succeeded then we'll want to commit to the changes we made, if
         // any. If stderr is empty then there's no need for the final exec at
         // the end, we just bail out here.
@@ -98,7 +106,7 @@ fn cargo_fix_rustc() -> Result<(), Error> {
         // user's code with our changes. Back out everything and fall through
         // below to recompile again.
         if !output.status.success() {
-            for (k, v) in files_to_restore {
+            for (k, v) in fixes.original_files {
                 File::create(&k)
                     .and_then(|mut f| f.write_all(v.as_bytes()))
                     .with_context(|_| format!("failed to write file `{}`", k))?;
@@ -109,7 +117,13 @@ fn cargo_fix_rustc() -> Result<(), Error> {
     exit_with(cmd.status().context("failed to spawn rustc")?);
 }
 
-fn rustfix_crate(rustc: &Path, filename: &str) -> Result<HashMap<String, String>, Error> {
+#[derive(Default)]
+struct FixedCrate {
+    messages: Vec<Message>,
+    original_files: HashMap<String, String>,
+}
+
+fn rustfix_crate(rustc: &Path, filename: &str) -> Result<FixedCrate, Error> {
     // If not empty, filter by these lints
     //
     // TODO: Implement a way to specify this
@@ -143,7 +157,7 @@ fn rustfix_crate(rustc: &Path, filename: &str) -> Result<HashMap<String, String>
             filename,
             output.status.code()
         );
-        return Ok(HashMap::new());
+        return Ok(Default::default())
     }
 
     // Sift through the output of the compiler to look for JSON messages
@@ -194,7 +208,8 @@ fn rustfix_crate(rustc: &Path, filename: &str) -> Result<HashMap<String, String>
         num_suggestion, filename
     );
 
-    let mut old_files = HashMap::with_capacity(file_map.len());
+    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
@@ -207,16 +222,19 @@ fn rustfix_crate(rustc: &Path, filename: &str) -> Result<HashMap<String, String>
         let num_suggestions = suggestions.len();
         debug!("applying {} fixes to {}", num_suggestions, file);
 
-        diagnostics::Message::fixing(&file, num_suggestion).post()?;
+        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))?;
-        old_files.insert(file, code);
+        original_files.insert(file, code);
     }
 
-    Ok(old_files)
+    Ok(FixedCrate {
+        messages,
+        original_files,
+    })
 }
 
 fn exit_with(status: ExitStatus) -> ! {

cargo-fix/tests/all/broken_build.rs

@@ -110,6 +110,7 @@ fn broken_fixes_backed_out() {
         .cwd("bar")
         .env("RUSTC", p.root.join("foo/target/debug/foo"))
         .stderr_contains("oh no")
+        .stderr_not_contains("[FIXING]")
         .status(101)
         .run();
 

cargo-fix/tests/all/main.rs

@@ -83,6 +83,7 @@ impl Project {
             stdout_contains: Vec::new(),
             stderr: None,
             stderr_contains: Vec::new(),
+            stderr_not_contains: Vec::new(),
             status: 0,
             ran: false,
             cwd: None,
@@ -108,6 +109,7 @@ struct ExpectCmd<'a> {
     stdout_contains: Vec<String>,
     stderr: Option<String>,
     stderr_contains: Vec<String>,
+    stderr_not_contains: Vec<String>,
     status: i32,
     cwd: Option<PathBuf>,
 }
@@ -144,6 +146,11 @@ impl<'a> ExpectCmd<'a> {
         self
     }
 
+    fn stderr_not_contains(&mut self, s: &str) -> &mut Self {
+        self.stderr_not_contains.push(s.to_string());
+        self
+    }
+
     fn run(&mut self) {
         self.ran = true;
         let mut parts = self.cmd.split_whitespace();
@@ -199,11 +206,22 @@ impl<'a> ExpectCmd<'a> {
         if code != self.status {
             panic!("expected exit code `{}` got `{}`", self.status, code);
         }
-        self.match_std(&output.stdout, &self.stdout, &self.stdout_contains);
-        self.match_std(&output.stderr, &self.stderr, &self.stderr_contains);
+        self.match_std(&output.stdout, &self.stdout, &self.stdout_contains, &[]);
+        self.match_std(
+            &output.stderr,
+            &self.stderr,
+            &self.stderr_contains,
+            &self.stderr_not_contains,
+        );
     }
 
-    fn match_std(&self, actual: &[u8], expected: &Option<String>, contains: &[String]) {
+    fn match_std(
+        &self,
+        actual: &[u8],
+        expected: &Option<String>,
+        contains: &[String],
+        not_contains: &[String],
+    ) {
         let actual = match str::from_utf8(actual) {
             Ok(s) => s,
             Err(_) => panic!("std wasn't utf8"),
@@ -224,6 +242,14 @@ impl<'a> ExpectCmd<'a> {
             );
             panic!("test failed");
         }
+        for s in not_contains {
+            let s = self.clean(s);
+            if !actual.contains(&s) {
+                continue;
+            }
+            println!("expected to not find `{}`", s);
+            panic!("test failed");
+        }
     }
 
     fn clean(&self, s: &str) -> String {