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

add a note that some warnings (and/or errors) can be auto-fixed #10989

Merged
merged 1 commit into from
Oct 28, 2022
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
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),
weihanglo marked this conversation as resolved.
Show resolved Hide resolved
}

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>,
}

Comment on lines +1425 to +1427
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we hand-pick field to deserialize? I see other places doing so.

Suggested change
children: Vec<Diagnostic>,
}
children: Vec<Diagnostic>,
}
#[derive(serde::Deserialize)]
struct Diagnostic {
spans: Vec<Span>,
// `children: Vec<Diagnostic>` if we need to check them recursively
}
#[derive(serde::Deserialize)]
struct Span {
suggestion_applicability: Option<Applicability>,
}

BTW, I feel like every existing ad-hoc deserialization should have a comment telling which struct it refers to.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we hand-pick field to deserialize? I see other places doing so.

I'm not sure what you mean by this

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean deserializing only those fields we need instead of the whole Diagnostic struct

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could do that but I was trying to use the available types from rustfix. I figured that since it had the ability to tell what things were it would be kept updated, instead of updating multiple places.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is an update of types in rustfix crate, either reuse or not we still need to update to some extent.

I was asking because other places deserialize fields-of-interest. Besides, there might be a performance hit if a package start emitting thousands of warnings with full struct fields, though I guess it is rare.

Anyway, I am good with reuse types from rustfix. Let's move on :)

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
weihanglo marked this conversation as resolved.
Show resolved Hide resolved
.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