Skip to content

Commit

Permalink
Display errors when cargo fix fails.
Browse files Browse the repository at this point in the history
It can be difficult to figure out what's wrong when a user reports that
`cargo fix` fails. There's often a large list of warnings, and it can
be hard to figure out which one caused a compile error.
  • Loading branch information
ehuss committed Dec 13, 2018
1 parent 79f962f commit fffb05d
Show file tree
Hide file tree
Showing 4 changed files with 63 additions and 13 deletions.
5 changes: 4 additions & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ log = "0.4.6"
libgit2-sys = "0.7.9"
num_cpus = "1.0"
opener = "0.3.0"
rustfix = "0.4.2"
rustfix = "0.4.3"
same-file = "1"
semver = { version = "0.9.0", features = ["serde"] }
serde = { version = "1.0.82", features = ['derive'] }
Expand Down Expand Up @@ -108,3 +108,6 @@ doc = false
deny-warnings = []
vendored-openssl = ['openssl/vendored']
pretty-env-logger = ['pretty_env_logger']

[patch.crates-io]
rustfix = {git="https://github.com/ehuss/rustfix", branch="pub-rendered"}
9 changes: 8 additions & 1 deletion src/cargo/ops/fix.rs
Original file line number Diff line number Diff line change
Expand Up @@ -496,7 +496,9 @@ fn log_failed_fix(stderr: &[u8]) -> Result<(), Error> {
.filter(|x| !x.is_empty())
.filter_map(|line| serde_json::from_str::<Diagnostic>(line).ok());
let mut files = BTreeSet::new();
let mut errors = Vec::new();
for diagnostic in diagnostics {
errors.push(diagnostic.rendered.unwrap_or(diagnostic.message));
for span in diagnostic.spans.into_iter() {
files.insert(span.file_name);
}
Expand All @@ -516,7 +518,12 @@ fn log_failed_fix(stderr: &[u8]) -> Result<(), Error> {
}

let files = files.into_iter().collect();
Message::FixFailed { files, krate }.post()?;
Message::FixFailed {
files,
krate,
errors,
}
.post()?;

Ok(())
}
Expand Down
23 changes: 22 additions & 1 deletion src/cargo/util/diagnostic_server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ pub enum Message {
FixFailed {
files: Vec<String>,
krate: Option<String>,
errors: Vec<String>,
},
ReplaceFailed {
file: String,
Expand Down Expand Up @@ -109,7 +110,11 @@ impl<'a> DiagnosticPrinter<'a> {
write!(self.config.shell().err(), "{}", PLEASE_REPORT_THIS_BUG)?;
Ok(())
}
Message::FixFailed { files, krate } => {
Message::FixFailed {
files,
krate,
errors,
} => {
if let Some(ref krate) = *krate {
self.config.shell().warn(&format!(
"failed to automatically apply fixes suggested by rustc \
Expand All @@ -133,6 +138,22 @@ impl<'a> DiagnosticPrinter<'a> {
writeln!(self.config.shell().err())?;
}
write!(self.config.shell().err(), "{}", PLEASE_REPORT_THIS_BUG)?;
if !errors.is_empty() {
writeln!(
self.config.shell().err(),
"The following errors were reported:"
)?;
for error in errors {
write!(self.config.shell().err(), "{}", error)?;
if !error.ends_with('\n') {
writeln!(self.config.shell().err())?;
}
}
}
writeln!(
self.config.shell().err(),
"Original diagnostics will follow.\n"
)?;
Ok(())
}
Message::EditionAlreadyEnabled { file, edition } => {
Expand Down
39 changes: 29 additions & 10 deletions tests/testsuite/fix.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,19 @@ fn fix_broken_if_requested() {

#[test]
fn broken_fixes_backed_out() {
// This works as follows:
// - Create a `rustc` shim (the "foo" project) which will pretend that the
// verification step fails.
// - There is an empty build script so `foo` has `OUT_DIR` to track the steps.
// - The first "check", `foo` creates a file in OUT_DIR, and it completes
// successfully with a warning diagnostic to remove unused `mut`.
// - rustfix removes the `mut`.
// - The second "check" to verify the changes, `foo` swaps out the content
// with something that fails to compile. It creates a second file so it
// won't do anything in the third check.
// - cargo fix discovers that the fix failed, and it backs out the changes.
// - The third "check" is done to display the original diagnostics of the
// original code.
let p = project()
.file(
"foo/Cargo.toml",
Expand All @@ -74,19 +87,19 @@ fn broken_fixes_backed_out() {
use std::process::{self, Command};
fn main() {
// Ignore calls to things like --print=file-names and compiling build.rs.
let is_lib_rs = env::args_os()
.map(PathBuf::from)
.any(|l| l == Path::new("src/lib.rs"));
if is_lib_rs {
let path = PathBuf::from(env::var_os("OUT_DIR").unwrap());
let path = path.join("foo");
if path.exists() {
fs::File::create("src/lib.rs")
.unwrap()
.write_all(b"not rust code")
.unwrap();
let first = path.join("first");
let second = path.join("second");
if first.exists() && !second.exists() {
fs::write("src/lib.rs", b"not rust code").unwrap();
fs::File::create(&second).unwrap();
} else {
fs::File::create(&path).unwrap();
fs::File::create(&first).unwrap();
}
}
Expand Down Expand Up @@ -127,8 +140,6 @@ fn broken_fixes_backed_out() {
.cwd(p.root().join("bar"))
.env("__CARGO_FIX_YOLO", "1")
.env("RUSTC", p.root().join("foo/target/debug/foo"))
.with_status(101)
.with_stderr_contains("[..]not rust code[..]")
.with_stderr_contains(
"\
warning: failed to automatically apply fixes suggested by rustc \
Expand All @@ -144,11 +155,19 @@ fn broken_fixes_backed_out() {
a number of compiler warnings after this message which cargo\n\
attempted to fix but failed. If you could open an issue at\n\
https://github.com/rust-lang/cargo/issues\n\
quoting the full output of this command we'd be very appreciative!\
quoting the full output of this command we'd be very appreciative!\n\
\n\
The following errors were reported:\n\
error: expected one of `!` or `::`, found `rust`\n\
",
)
.with_stderr_contains("Original diagnostics will follow.")
.with_stderr_contains("[WARNING] variable does not need to be mutable")
.with_stderr_does_not_contain("[..][FIXING][..]")
.run();

// Make sure the fix which should have been applied was backed out
assert!(p.read_file("bar/src/lib.rs").contains("let mut x = 3;"));
}

#[test]
Expand Down

0 comments on commit fffb05d

Please sign in to comment.