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.

Sign up for GitHub

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

Jump to bottom

cargo fix: Call rustc fewer times. #13243

Merged
merged 2 commits into from
Jan 3, 2024
Merged

cargo fix: Call rustc fewer times. #13243

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8ffd9cd
Add tests validating the sequence for `cargo fix`.
ehuss Jan 2, 2024
e7eaa51
`cargo fix`: Call rustc fewer times.
ehuss Jan 2, 2024
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
9 changes: 9 additions & 0 deletions crates/rustfix/src/lib.rs
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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