Skip to content

Commit

Permalink
add a note that some warnings can be auto-fixed
Browse files Browse the repository at this point in the history
  • Loading branch information
Muscraft committed Oct 28, 2022
1 parent 1c1e9a6 commit 8ff8eaa
Show file tree
Hide file tree
Showing 4 changed files with 449 additions and 20 deletions.
141 changes: 123 additions & 18 deletions src/cargo/core/compiler/job_queue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -129,12 +129,8 @@ struct DrainState<'cfg> {
messages: Arc<Queue<Message>>,
/// Diagnostic deduplication support.
diag_dedupe: DiagDedupe<'cfg>,
/// Count of warnings, used to print a summary after the job succeeds.
///
/// First value is the total number of warnings, and the second value is
/// the number that were suppressed because they were duplicates of a
/// previous warning.
warning_count: HashMap<JobId, (usize, usize)>,
/// Count of warnings, used to print a summary after the job succeeds
warning_count: HashMap<JobId, WarningCount>,
active: HashMap<JobId, Unit>,
compiled: HashSet<PackageId>,
documented: HashSet<PackageId>,
Expand Down Expand Up @@ -170,6 +166,50 @@ struct DrainState<'cfg> {
per_package_future_incompat_reports: Vec<FutureIncompatReportPackage>,
}

/// Count of warnings, used to print a summary after the job succeeds
#[derive(Default)]
pub struct WarningCount {
/// total number of warnings
pub total: usize,
/// number of warnings that were suppressed because they
/// were duplicates of a previous warning
pub duplicates: usize,
/// number of fixable warnings set to `NotAllowed`
/// if any errors have been seen ofr the current
/// target
pub fixable: FixableWarnings,
}

impl WarningCount {
/// If an error is seen this should be called
/// to set `fixable` to `NotAllowed`
fn disallow_fixable(&mut self) {
self.fixable = FixableWarnings::NotAllowed;
}

/// Checks fixable if warnings are allowed
/// fixable warnings are allowed if no
/// errors have been seen for the current
/// target. If an error was seen `fixable`
/// will be `NotAllowed`.
fn fixable_allowed(&self) -> bool {
match &self.fixable {
FixableWarnings::NotAllowed => false,
_ => true,
}
}
}

/// Used to keep track of how many fixable warnings there are
/// and if fixable warnings are allowed
#[derive(Default)]
pub enum FixableWarnings {
NotAllowed,
#[default]
Zero,
Positive(usize),
}

pub struct ErrorsDuringDrain {
pub count: usize,
}
Expand Down Expand Up @@ -311,10 +351,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 +405,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 @@ -679,14 +723,28 @@ 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" {
let cnts = self.warning_count.entry(id).or_default();
// If there is an error, the `cargo fix` message should not show
cnts.disallow_fixable();
}
}
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 @@ -1127,19 +1185,34 @@ 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;
cnts.total += 1;
if !emitted {
cnts.1 += 1;
cnts.duplicates += 1;
// Don't add to fixable if it's already been emitted
} else if fixable {
// Do not add anything to the fixable warning count if
// is `NotAllowed` since that indicates there was an
// error while building this `Unit`
if cnts.fixable_allowed() {
cnts.fixable = match cnts.fixable {
FixableWarnings::NotAllowed => FixableWarnings::NotAllowed,
FixableWarnings::Zero => FixableWarnings::Positive(1),
FixableWarnings::Positive(fixable) => FixableWarnings::Positive(fixable + 1),
};
}
}
}

/// Displays a final report of the warnings emitted by a particular job.
fn report_warning_count(&mut self, config: &Config, id: JobId) {
let count = match self.warning_count.remove(&id) {
Some(count) => count,
None => return,
// An error could add an entry for a `Unit`
// with 0 warnings but having fixable
// warnings be disallowed
Some(count) if count.total > 0 => count,
None | Some(_) => return,
};
let unit = &self.active[&id];
let mut message = format!("`{}` ({}", unit.pkg.name(), unit.target.description_named());
Expand All @@ -1151,15 +1224,47 @@ impl<'cfg> DrainState<'cfg> {
message.push_str(" doc");
}
message.push_str(") generated ");
match count.0 {
match count.total {
1 => message.push_str("1 warning"),
n => drop(write!(message, "{} warnings", n)),
};
match count.1 {
match count.duplicates {
0 => {}
1 => message.push_str(" (1 duplicate)"),
n => drop(write!(message, " ({} duplicates)", n)),
}
// Only show the `cargo fix` message if its a local `Unit`
if unit.is_local() && config.nightly_features_allowed {
// Do not show this if there are any errors or no fixable warnings
if let FixableWarnings::Positive(fixable) = count.fixable {
// `cargo fix` doesnt have an option for custom builds
if !unit.target.is_custom_build() {
let mut command = {
let named = unit.target.description_named();
// if its a lib we need to add the package to fix
if unit.target.is_lib() {
format!("{} -p {}", named, unit.pkg.name())
} else {
named
}
};
if unit.mode.is_rustc_test()
&& !(unit.target.is_test() || unit.target.is_bench())
{
command.push_str(" --tests");
}
let mut suggestions = format!("{} suggestion", fixable);
if fixable > 1 {
suggestions.push_str("s")
}
drop(write!(
message,
" (run `cargo fix --{}` to apply {})",
command, suggestions
))
}
}
}
// Errors are ignored here because it is tricky to handle them
// correctly, and they aren't important.
drop(config.shell().warn(message));
Expand Down
17 changes: 16 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 @@ -1420,7 +1421,10 @@ fn on_stderr_line_inner(
rendered: String,
message: String,
level: String,
// `children: Vec<Diagnostic>` if we need to check them recursively
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 @@ -1443,8 +1447,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
Loading

0 comments on commit 8ff8eaa

Please sign in to comment.