Skip to content

Commit

Permalink
Auto merge of #13243 - ehuss:fix-fewer-rustc, r=epage
Browse files Browse the repository at this point in the history 
`cargo fix`: Call rustc fewer times.

This is an improvement of `cargo fix` so that it calls `rustc` one fewer times per target. The original code an extra step of calling `rustc` to display final diagnostics to the user. Part of the reason is that in the past, cargo did not always use JSON, and so `cargo fix` was forced to call `rustc` with and without JSON. Now that cargo uses JSON all the time, that is not necessary. This avoids the final call to `rustc` by remembering the original output from `rustc`.

This needs to keep track of both the first and last output from `rustc`. This is to handle the situation where `cargo fix` fails to apply some suggestion (either because the verification fails, or `rustfix` fails). The `cargo fix` output includes both the error, and the original diagnostics before the error.

The first commit is a little test framework to exercise the various edge cases around how fix works. The comments should explain how it works, but it essentially has a `rustc` replacement that emits various different diagnostics and counts how often it is called.

The subsequent commit includes the change to keep track of the output from `rustc` and to avoid the final call.

Fixes #13215
  • Loading branch information
@bors
bors committed Jan 3, 2024
2 parents b41f084 + e7eaa51 commit dfdd5ff
Show file tree
Hide file tree
Showing 6 changed files with 658 additions and 264 deletions.
9 changes: 9 additions & 0 deletions crates/rustfix/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -215,13 +215,16 @@ pub fn collect_suggestions<S: ::std::hash::BuildHasher>(
/// 3. Calls [`CodeFix::finish`] to get the "fixed" code.
pub struct CodeFix {
data: replace::Data,
/// Whether or not the data has been modified.
modified: bool,
}

impl CodeFix {
/// Creates a `CodeFix` with the source of a file to modify.
pub fn new(s: &str) -> CodeFix {
CodeFix {
data: replace::Data::new(s.as_bytes()),
modified: false,
}
}

Expand All @@ -231,6 +234,7 @@ impl CodeFix {
for r in &sol.replacements {
self.data
.replace_range(r.snippet.range.clone(), r.replacement.as_bytes())?;
self.modified = true;
}
}
Ok(())
Expand All @@ -240,6 +244,11 @@ impl CodeFix {
pub fn finish(&self) -> Result<String, Error> {
Ok(String::from_utf8(self.data.to_vec())?)
}

/// Returns whether or not the data has been modified.
pub fn modified(&self) -> bool {
self.modified
}
}

/// Applies multiple `suggestions` to the given `code`.
Expand Down
222 changes: 135 additions & 87 deletions src/cargo/ops/fix.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,13 +35,12 @@
//! applied cleanly, rustc is run again to verify the suggestions didn't
//! break anything. The change will be backed out if it fails (unless
//! `--broken-code` is used).
//! - If there are any warnings or errors, rustc will be run one last time to
//! show them to the user.

use std::collections::{BTreeSet, HashMap, HashSet};
use std::ffi::OsString;
use std::io::Write;
use std::path::{Path, PathBuf};
use std::process::{self, ExitStatus};
use std::process::{self, ExitStatus, Output};
use std::{env, fs, str};

use anyhow::{bail, Context as _};
Expand Down Expand Up @@ -388,73 +387,94 @@ pub fn fix_exec_rustc(config: &Config, lock_addr: &str) -> CargoResult<()> {
trace!("start rustfixing {:?}", args.file);
let fixes = rustfix_crate(&lock_addr, &rustc, &args.file, &args, config)?;

// Ok now we have our final goal of testing out the changes that we applied.
// If these changes went awry and actually started to cause the crate to
// *stop* compiling then we want to back them out and continue to print
// warnings to the user.
//
// If we didn't actually make any changes then we can immediately execute the
// new rustc, and otherwise we capture the output to hide it in the scenario
// that we have to back it all out.
if !fixes.files.is_empty() {
debug!("calling rustc for final verification: {rustc}");
let output = rustc.output()?;

if output.status.success() {
for (path, file) in fixes.files.iter() {
Message::Fixed {
file: path.clone(),
fixes: file.fixes_applied,
}
.post(config)?;
if fixes.last_output.status.success() {
for (path, file) in fixes.files.iter() {
Message::Fixed {
file: path.clone(),
fixes: file.fixes_applied,
}
.post(config)?;
}
// Display any remaining diagnostics.
emit_output(&fixes.last_output)?;
return Ok(());
}

// If we succeeded then we'll want to commit to the changes we made, if
// any. If stderr is empty then there's no need for the final exec at
// the end, we just bail out here.
if output.status.success() && output.stderr.is_empty() {
return Ok(());
let allow_broken_code = config.get_env_os(BROKEN_CODE_ENV_INTERNAL).is_some();

// There was an error running rustc during the last run.
//
// Back out all of the changes unless --broken-code was used.
if !allow_broken_code {
for (path, file) in fixes.files.iter() {
debug!("reverting {:?} due to errors", path);
paths::write(path, &file.original_code)?;
}
}

// Otherwise, if our rustc just failed, then that means that we broke the
// user's code with our changes. Back out everything and fall through
// below to recompile again.
if !output.status.success() {
if config.get_env_os(BROKEN_CODE_ENV_INTERNAL).is_none() {
for (path, file) in fixes.files.iter() {
debug!("reverting {:?} due to errors", path);
paths::write(path, &file.original_code)?;
// If there were any fixes, let the user know that there was a failure
// attempting to apply them, and to ask for a bug report.
//
// FIXME: The error message here is not correct with --broken-code.
// https://github.com/rust-lang/cargo/issues/10955
if fixes.files.is_empty() {
// No fixes were available. Display whatever errors happened.
emit_output(&fixes.last_output)?;
exit_with(fixes.last_output.status);
} else {
let krate = {
let mut iter = rustc.get_args();
let mut krate = None;
while let Some(arg) = iter.next() {
if arg == "--crate-name" {
krate = iter.next().and_then(|s| s.to_owned().into_string().ok());
}
}

let krate = {
let mut iter = rustc.get_args();
let mut krate = None;
while let Some(arg) = iter.next() {
if arg == "--crate-name" {
krate = iter.next().and_then(|s| s.to_owned().into_string().ok());
}
}
krate
};
log_failed_fix(config, krate, &output.stderr, output.status)?;
}
krate
};
log_failed_fix(
config,
krate,
&fixes.last_output.stderr,
fixes.last_output.status,
)?;
// Display the diagnostics that appeared at the start, before the
// fixes failed. This can help with diagnosing which suggestions
// caused the failure.
emit_output(&fixes.first_output)?;
// Exit with whatever exit code we initially started with. `cargo fix`
// treats this as a warning, and shouldn't return a failure code
// unless the code didn't compile in the first place.
exit_with(fixes.first_output.status);
}
}

// This final fall-through handles multiple cases;
// - If the fix failed, show the original warnings and suggestions.
// - If `--broken-code`, show the error messages.
// - If the fix succeeded, show any remaining warnings.
debug!("calling rustc to display remaining diagnostics: {rustc}");
exit_with(rustc.status()?);
fn emit_output(output: &Output) -> CargoResult<()> {
// Unfortunately if there is output on stdout, this does not preserve the
// order of output relative to stderr. In practice, rustc should never
// print to stdout unless some proc-macro does it.
std::io::stderr().write_all(&output.stderr)?;
std::io::stdout().write_all(&output.stdout)?;
Ok(())
}

#[derive(Default)]
struct FixedCrate {
/// Map of file path to some information about modifications made to that file.
files: HashMap<String, FixedFile>,
/// The output from rustc from the first time it was called.
///
/// This is needed when fixes fail to apply, so that it can display the
/// original diagnostics to the user which can help with diagnosing which
/// suggestions caused the failure.
first_output: Output,
/// The output from rustc from the last time it was called.
///
/// This will be displayed to the user to show any remaining diagnostics
/// or errors.
last_output: Output,
}

#[derive(Debug)]
struct FixedFile {
errors_applying_fixes: Vec<String>,
fixes_applied: u32,
Expand All @@ -472,11 +492,6 @@ fn rustfix_crate(
args: &FixArgs,
config: &Config,
) -> CargoResult<FixedCrate> {
if !args.can_run_rustfix(config)? {
// This fix should not be run. Skipping...
return Ok(FixedCrate::default());
}

// First up, we want to make sure that each crate is only checked by one
// process at a time. If two invocations concurrently check a crate then
// it's likely to corrupt it.
Expand All @@ -488,6 +503,23 @@ fn rustfix_crate(
// modification.
let _lock = LockServerClient::lock(&lock_addr.parse()?, "global")?;

// Map of files that have been modified.
let mut files = HashMap::new();

if !args.can_run_rustfix(config)? {
// This fix should not be run. Skipping...
// We still need to run rustc at least once to make sure any potential
// rmeta gets generated, and diagnostics get displayed.
debug!("can't fix {filename:?}, running rustc: {rustc}");
let last_output = rustc.output()?;
let fixes = FixedCrate {
files,
first_output: last_output.clone(),
last_output,
};
return Ok(fixes);
}

// Next up, this is a bit suspicious, but we *iteratively* execute rustc and
// collect suggestions to feed to rustfix. Once we hit our limit of times to
// execute rustc or we appear to be reaching a fixed point we stop running
Expand Down Expand Up @@ -521,41 +553,53 @@ fn rustfix_crate(
// Detect this when a fix fails to get applied *and* no suggestions
// successfully applied to the same file. In that case looks like we
// definitely can't make progress, so bail out.
let mut fixes = FixedCrate::default();
let mut last_fix_counts = HashMap::new();
let iterations = config
let max_iterations = config
.get_env("CARGO_FIX_MAX_RETRIES")
.ok()
.and_then(|n| n.parse().ok())
.unwrap_or(4);
for _ in 0..iterations {
last_fix_counts.clear();
for (path, file) in fixes.files.iter_mut() {
last_fix_counts.insert(path.clone(), file.fixes_applied);
let mut last_output;
let mut last_made_changes;
let mut first_output = None;
let mut current_iteration = 0;
loop {
for file in files.values_mut() {
// We'll generate new errors below.
file.errors_applying_fixes.clear();
}
rustfix_and_fix(&mut fixes, rustc, filename, config)?;
(last_output, last_made_changes) = rustfix_and_fix(&mut files, rustc, filename, config)?;
if current_iteration == 0 {
first_output = Some(last_output.clone());
}
let mut progress_yet_to_be_made = false;
for (path, file) in fixes.files.iter_mut() {
for (path, file) in files.iter_mut() {
if file.errors_applying_fixes.is_empty() {
continue;
}
debug!("had rustfix apply errors in {path:?} {file:?}");
// If anything was successfully fixed *and* there's at least one
// error, then assume the error was spurious and we'll try again on
// the next iteration.
if file.fixes_applied != *last_fix_counts.get(path).unwrap_or(&0) {
if last_made_changes {
progress_yet_to_be_made = true;
}
}
if !progress_yet_to_be_made {
break;
}
current_iteration += 1;
if current_iteration >= max_iterations {
break;
}
}
if last_made_changes {
debug!("calling rustc one last time for final results: {rustc}");
last_output = rustc.output()?;
}

// Any errors still remaining at this point need to be reported as probably
// bugs in Cargo and/or rustfix.
for (path, file) in fixes.files.iter_mut() {
for (path, file) in files.iter_mut() {
for error in file.errors_applying_fixes.drain(..) {
Message::ReplaceFailed {
file: path.clone(),
Expand All @@ -565,19 +609,23 @@ fn rustfix_crate(
}
}

Ok(fixes)
Ok(FixedCrate {
files,
first_output: first_output.expect("at least one iteration"),
last_output,
})
}

/// Executes `rustc` to apply one round of suggestions to the crate in question.
///
/// This will fill in the `fixes` map with original code, suggestions applied,
/// and any errors encountered while fixing files.
fn rustfix_and_fix(
fixes: &mut FixedCrate,
files: &mut HashMap<String, FixedFile>,
rustc: &ProcessBuilder,
filename: &Path,
config: &Config,
) -> CargoResult<()> {
) -> CargoResult<(Output, bool)> {
// If not empty, filter by these lints.
// TODO: implement a way to specify this.
let only = HashSet::new();
Expand All @@ -596,7 +644,7 @@ fn rustfix_and_fix(
filename,
output.status.code()
);
return Ok(());
return Ok((output, false));
}

let fix_mode = config
Expand Down Expand Up @@ -664,6 +712,7 @@ fn rustfix_and_fix(
filename.display(),
);

let mut made_changes = false;
for (file, suggestions) in file_map {
// Attempt to read the source code for this file. If this fails then
// that'd be pretty surprising, so log a message and otherwise keep
Expand All @@ -682,14 +731,11 @@ fn rustfix_and_fix(
// code, so save it. If the file already exists then the original code
// doesn't need to be updated as we've just read an interim state with
// some fixes but perhaps not all.
let fixed_file = fixes
.files
.entry(file.clone())
.or_insert_with(|| FixedFile {
errors_applying_fixes: Vec::new(),
fixes_applied: 0,
original_code: code.clone(),
});
let fixed_file = files.entry(file.clone()).or_insert_with(|| FixedFile {
errors_applying_fixes: Vec::new(),
fixes_applied: 0,
original_code: code.clone(),
});
let mut fixed = CodeFix::new(&code);

// As mentioned above in `rustfix_crate`, we don't immediately warn
Expand All @@ -701,17 +747,19 @@ fn rustfix_and_fix(
Err(e) => fixed_file.errors_applying_fixes.push(e.to_string()),
}
}
let new_code = fixed.finish()?;
paths::write(&file, new_code)?;
if fixed.modified() {
made_changes = true;
let new_code = fixed.finish()?;
paths::write(&file, new_code)?;
}
}

Ok(())
Ok((output, made_changes))
}

fn exit_with(status: ExitStatus) -> ! {
#[cfg(unix)]
{
use std::io::Write;
use std::os::unix::prelude::*;
if let Some(signal) = status.signal() {
drop(writeln!(
Expand Down
2 changes: 1 addition & 1 deletion src/cargo/util/diagnostic_server.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,7 @@ https://doc.rust-lang.org/edition-guide/editions/transitioning-an-existing-proje
fn gen_please_report_this_bug_text(url: &str) -> String {
format!(
"This likely indicates a bug in either rustc or cargo itself,\n\
and we would appreciate a bug report! You're likely to see \n\
and we would appreciate a bug report! You're likely to see\n\
a number of compiler warnings after this message which cargo\n\
attempted to fix but failed. If you could open an issue at\n\
{}\n\
Expand Down
Loading

0 comments on commit dfdd5ff

Please sign in to comment.