Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Display errors when cargo fix fails. #6419

Merged
merged 2 commits into from
Dec 14, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 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.4"
same-file = "1"
semver = { version = "0.9.0", features = ["serde"] }
serde = { version = "1.0.82", features = ['derive'] }
Expand Down
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