Skip to content

Commit

Permalink
add a note that some warnings (and/or errors) can be auto-fixed
Browse files Browse the repository at this point in the history
  • Loading branch information
Muscraft committed Aug 15, 2022
1 parent 8494149 commit 283c3f4
Show file tree
Hide file tree
Showing 5 changed files with 191 additions and 7 deletions.
68 changes: 62 additions & 6 deletions src/cargo/core/compiler/job_queue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,14 @@ struct DrainState<'cfg> {
/// the number that were suppressed because they were duplicates of a
/// previous warning.
warning_count: HashMap<JobId, (usize, usize)>,
/// This keeps track of the total number of fixable errors and warnings.
///i.e. [Applicability::MachineApplicable].
///
/// The first value is the number of fixable errors, the second value is
/// the number of fixable warnings.
///
/// [Applicability::MachineApplicable]: rustfix::diagnostics::Applicability::MachineApplicable
fixable_count: (usize, usize),
active: HashMap<JobId, Unit>,
compiled: HashSet<PackageId>,
documented: HashSet<PackageId>,
Expand Down Expand Up @@ -311,10 +319,12 @@ enum Message {
id: JobId,
level: String,
diag: String,
fixable: bool,
},
WarningCount {
id: JobId,
emitted: bool,
fixable: bool,
},
FixDiagnostic(diagnostic_server::Message),
Token(io::Result<Acquired>),
Expand Down Expand Up @@ -363,20 +373,22 @@ impl<'a, 'cfg> JobState<'a, 'cfg> {
Ok(())
}

pub fn emit_diag(&self, level: String, diag: String) -> CargoResult<()> {
pub fn emit_diag(&self, level: String, diag: String, fixable: bool) -> CargoResult<()> {
if let Some(dedupe) = self.output {
let emitted = dedupe.emit_diag(&diag)?;
if level == "warning" {
self.messages.push(Message::WarningCount {
id: self.id,
emitted,
fixable,
});
}
} else {
self.messages.push_bounded(Message::Diagnostic {
id: self.id,
level,
diag,
fixable,
});
}
Ok(())
Expand Down Expand Up @@ -516,6 +528,7 @@ impl<'cfg> JobQueue<'cfg> {
messages: Arc::new(Queue::new(100)),
diag_dedupe: DiagDedupe::new(cx.bcx.config),
warning_count: HashMap::new(),
fixable_count: (0, 0),
active: HashMap::new(),
compiled: HashSet::new(),
documented: HashSet::new(),
Expand Down Expand Up @@ -668,14 +681,26 @@ impl<'cfg> DrainState<'cfg> {
shell.print_ansi_stderr(err.as_bytes())?;
shell.err().write_all(b"\n")?;
}
Message::Diagnostic { id, level, diag } => {
Message::Diagnostic {
id,
level,
diag,
fixable,
} => {
let emitted = self.diag_dedupe.emit_diag(&diag)?;
if level == "warning" {
self.bump_warning_count(id, emitted);
self.bump_warning_count(id, emitted, fixable);
}
if level == "error" && fixable {
self.fixable_count.0 += 1;
}
}
Message::WarningCount { id, emitted } => {
self.bump_warning_count(id, emitted);
Message::WarningCount {
id,
emitted,
fixable,
} => {
self.bump_warning_count(id, emitted, fixable);
}
Message::FixDiagnostic(msg) => {
self.print.print(&msg)?;
Expand Down Expand Up @@ -860,6 +885,34 @@ impl<'cfg> DrainState<'cfg> {
}
self.progress.clear();

let fixable_errors = match self.fixable_count.0 {
0 => String::new(),
1 => "1 error".to_string(),
n => format!("{n} errors"),
};

let fixable_warnings = match self.fixable_count.1 {
0 => String::new(),
1 => "1 warning".to_string(),
n => format!("{n} warnings"),
};

let fixable = match (fixable_errors.is_empty(), fixable_warnings.is_empty()) {
(true, true) => String::new(),
(true, false) => fixable_warnings,
(false, true) => fixable_errors,
(false, false) => format!("{fixable_errors} and {fixable_warnings}"),
};

if !fixable.is_empty() {
drop(
cx.bcx
.config
.shell()
.note(format!("to fix {fixable} automatically, run `cargo fix`")),
);
}

let profile_name = cx.bcx.build_config.requested_profile;
// NOTE: this may be a bit inaccurate, since this may not display the
// profile for what was actually built. Profile overrides can change
Expand Down Expand Up @@ -1116,12 +1169,15 @@ impl<'cfg> DrainState<'cfg> {
Ok(())
}

fn bump_warning_count(&mut self, id: JobId, emitted: bool) {
fn bump_warning_count(&mut self, id: JobId, emitted: bool, fixable: bool) {
let cnts = self.warning_count.entry(id).or_default();
cnts.0 += 1;
if !emitted {
cnts.1 += 1;
}
if fixable {
self.fixable_count.1 += 1;
}
}

/// Displays a final report of the warnings emitted by a particular job.
Expand Down
16 changes: 15 additions & 1 deletion src/cargo/core/compiler/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ use crate::util::interning::InternedString;
use crate::util::machine_message::{self, Message};
use crate::util::{add_path_args, internal, iter_join_onto, profile};
use cargo_util::{paths, ProcessBuilder, ProcessError};
use rustfix::diagnostics::{Applicability, Diagnostic};

const RUSTDOC_CRATE_VERSION_FLAG: &str = "--crate-version";

Expand Down Expand Up @@ -1407,7 +1408,9 @@ fn on_stderr_line_inner(
rendered: String,
message: String,
level: String,
children: Vec<Diagnostic>,
}

if let Ok(mut msg) = serde_json::from_str::<CompilerMessage>(compiler_message.get()) {
if msg.message.starts_with("aborting due to")
|| msg.message.ends_with("warning emitted")
Expand All @@ -1430,8 +1433,19 @@ fn on_stderr_line_inner(
.expect("strip should never fail")
};
if options.show_diagnostics {
let machine_applicable: bool = msg
.children
.iter()
.map(|child| {
child
.spans
.iter()
.filter_map(|span| span.suggestion_applicability)
.any(|app| app == Applicability::MachineApplicable)
})
.any(|b| b);
count_diagnostic(&msg.level, options);
state.emit_diag(msg.level, rendered)?;
state.emit_diag(msg.level, rendered, machine_applicable)?;
}
return Ok(true);
}
Expand Down
74 changes: 74 additions & 0 deletions tests/testsuite/check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,80 @@ fn check_fail() {
.run();
}

#[cargo_test]
fn check_fixable_warning() {
let foo = project()
.file(
"Cargo.toml",
r#"
[package]
name = "foo"
version = "0.0.1"
authors = []
"#,
)
.file("src/lib.rs", "use std::io;")
.build();

foo.cargo("check")
.with_status(0)
.with_stderr_contains("[..]note: to fix 1 warning automatically, run `cargo fix`[..]")
.run();
}

#[cargo_test]
fn check_fixable_error() {
let foo = project()
.file(
"Cargo.toml",
r#"
[package]
name = "foo"
version = "0.0.1"
authors = []
"#,
)
.file("src/lib.rs", "#[derive(Debug(x))]\nstruct Foo;")
.build();

foo.cargo("check")
.with_status(101)
.with_stderr_contains("[..]note: to fix 1 error automatically, run `cargo fix`[..]")
.with_stderr_does_not_contain(
"[..]note: to fix 1 warning automatically, run `cargo fix`[..]",
)
.run();
}

#[cargo_test]
fn check_fixable_both() {
let foo = project()
.file(
"Cargo.toml",
r#"
[package]
name = "foo"
version = "0.0.1"
authors = []
"#,
)
.file(
"src/lib.rs",
"use std::io;\n#[derive(Debug(x))]\nstruct Foo;",
)
.build();

foo.cargo("check")
.with_status(101)
.with_stderr_contains(
"[..]note: to fix 1 error and 1 warning automatically, run `cargo fix`[..]",
)
.with_stderr_does_not_contain(
"[..]note: to fix 1 warning automatically, run `cargo fix`[..]",
)
.run();
}

#[cargo_test]
fn custom_derive() {
let foo = project()
Expand Down
38 changes: 38 additions & 0 deletions tests/testsuite/install.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2032,3 +2032,41 @@ fn install_semver_metadata() {
)
.run();
}

#[cargo_test]
fn no_auto_fix_note() {
Package::new("auto_fix", "0.0.1")
.file("src/lib.rs", "use std::io;")
.file(
"src/main.rs",
&format!("extern crate {}; use std::io; fn main() {{}}", "auto_fix"),
)
.publish();

// This should not contain something along the lines of:
// "note: to fix 1 warning automatically, run `cargo fix"
//
// This is checked by matching the full output as `with_stderr_does_not_contain`
// can be brittle
cargo_process("install auto_fix")
.with_stderr(
"\
[UPDATING] `[..]` index
[DOWNLOADING] crates ...
[DOWNLOADED] auto_fix v0.0.1 (registry [..])
[INSTALLING] auto_fix v0.0.1
[COMPILING] auto_fix v0.0.1
[FINISHED] release [optimized] target(s) in [..]
[INSTALLING] [CWD]/home/.cargo/bin/auto_fix[EXE]
[INSTALLED] package `auto_fix v0.0.1` (executable `auto_fix[EXE]`)
[WARNING] be sure to add `[..]` to your PATH to be able to run the installed binaries
",
)
.run();
assert_has_installed_exe(cargo_home(), "auto_fix");

cargo_process("uninstall auto_fix")
.with_stderr("[REMOVING] [CWD]/home/.cargo/bin/auto_fix[EXE]")
.run();
assert_has_not_installed_exe(cargo_home(), "auto_fix");
}
2 changes: 2 additions & 0 deletions tests/testsuite/messages.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ fn deduplicate_messages_basic() {
"{}\
warning: `foo` (lib) generated 1 warning
warning: `foo` (lib test) generated 1 warning (1 duplicate)
note: to fix 2 warnings automatically, run `cargo fix`
[FINISHED] [..]
[EXECUTABLE] unittests src/lib.rs (target/debug/deps/foo-[..][EXE])
",
Expand Down Expand Up @@ -106,6 +107,7 @@ fn deduplicate_messages_mismatched_warnings() {
warning: `foo` (lib) generated 1 warning
{}\
warning: `foo` (lib test) generated 2 warnings (1 duplicate)
note: to fix 2 warnings automatically, run `cargo fix`
[FINISHED] [..]
[EXECUTABLE] unittests src/lib.rs (target/debug/deps/foo-[..][EXE])
",
Expand Down

0 comments on commit 283c3f4

Please sign in to comment.