From 59b3617c0f06609930cca3a1e7f645f0ded71bbe Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Sat, 5 May 2018 21:37:04 -0700 Subject: [PATCH 1/2] Warn users when fixes break code The rustfix tool detects it, so let's inform the user what just happened! Closes #88 --- cargo-fix/src/cli.rs | 78 ++++++++++++++++++++--------- cargo-fix/src/diagnostics.rs | 22 +++++--- cargo-fix/src/main.rs | 40 ++++++++++++++- cargo-fix/tests/all/broken_build.rs | 37 ++++++++++---- 4 files changed, 137 insertions(+), 40 deletions(-) diff --git a/cargo-fix/src/cli.rs b/cargo-fix/src/cli.rs index d76e10c..41f9f19 100644 --- a/cargo-fix/src/cli.rs +++ b/cargo-fix/src/cli.rs @@ -1,12 +1,13 @@ 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; pub fn run() -> Result<(), Error> { @@ -41,8 +42,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 +77,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 +90,60 @@ 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")?; + + } + write!( + stream, + "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 gist the full output\n\ + of this command to https://github.com/rust-lang-nursery/rustfix/issues\n\ + we'd be very appreciative!\n\n\ + " )?; } } + 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. - 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(()) } diff --git a/cargo-fix/src/diagnostics.rs b/cargo-fix/src/diagnostics.rs index 39b3217..3a95049 100644 --- a/cargo-fix/src/diagnostics.rs +++ b/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, krate: Option }, } impl Message { @@ -62,10 +65,9 @@ impl Server { Ok(Server { listener }) } - pub fn start( - self, - on_message: F, - ) -> Result { + pub fn start(self, on_message: F) -> Result + 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(self, on_message: F) { + fn run(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); } diff --git a/cargo-fix/src/main.rs b/cargo-fix/src/main.rs index a7acf43..0a649c4 100644 --- a/cargo-fix/src/main.rs +++ b/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::(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(()) +} diff --git a/cargo-fix/tests/all/broken_build.rs b/cargo-fix/tests/all/broken_build.rs index d8376de..4583fc2 100644 --- a/cargo-fix/tests/all/broken_build.rs +++ b/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 gist the full output\n\ + of this command to https://github.com/rust-lang-nursery/rustfix/issues\n\ + we'd be very appreciative!\ + " + ) .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;")); } From 8f4465899bed3bc8811d0017955bd19a0800d822 Mon Sep 17 00:00:00 2001 From: Pascal Hertleif Date: Sun, 6 May 2018 18:49:38 +0200 Subject: [PATCH 2/2] Adjust wording when asking for bug report This omits the reference to gist (which may confuse people not familiar with gist.github.com), and moves the link to our issue tracker to its own line, which makes copying easier (for those whose terminals don't have a "ctrl+click to open link" feature). --- cargo-fix/src/cli.rs | 20 ++++++++++---------- cargo-fix/tests/all/broken_build.rs | 6 +++--- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/cargo-fix/src/cli.rs b/cargo-fix/src/cli.rs index 41f9f19..2254279 100644 --- a/cargo-fix/src/cli.rs +++ b/cargo-fix/src/cli.rs @@ -10,6 +10,15 @@ use lock; 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") @@ -121,16 +130,7 @@ fn log_message(msg: &Message, stream: &mut StandardStream) -> Result<(), Error> write!(stream, "\n")?; } - write!( - stream, - "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 gist the full output\n\ - of this command to https://github.com/rust-lang-nursery/rustfix/issues\n\ - we'd be very appreciative!\n\n\ - " - )?; + stream.write(PLEASE_REPORT_THIS_BUG.as_bytes())?; } } diff --git a/cargo-fix/tests/all/broken_build.rs b/cargo-fix/tests/all/broken_build.rs index 4583fc2..ca8f09f 100644 --- a/cargo-fix/tests/all/broken_build.rs +++ b/cargo-fix/tests/all/broken_build.rs @@ -127,9 +127,9 @@ fn broken_fixes_backed_out() { 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 gist the full output\n\ - of this command to https://github.com/rust-lang-nursery/rustfix/issues\n\ - we'd be very appreciative!\ + 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]")