From 8ffd9cd86f8e901cbefcdff7cc262095f2f95759 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Tue, 2 Jan 2024 14:29:12 -0800 Subject: [PATCH 1/2] Add tests validating the sequence for `cargo fix`. This adds a set of tests which validates various edge cases around how `cargo fix` works in terms of calling `rustc` multiple times. This uses a replacement of `rustc` so it doesn't depend on the behavior of rustc itself which is not always stable. --- src/cargo/util/diagnostic_server.rs | 2 +- tests/testsuite/fix.rs | 175 +--------- tests/testsuite/fix_n_times.rs | 513 ++++++++++++++++++++++++++++ tests/testsuite/main.rs | 1 + 4 files changed, 516 insertions(+), 175 deletions(-) create mode 100644 tests/testsuite/fix_n_times.rs diff --git a/src/cargo/util/diagnostic_server.rs b/src/cargo/util/diagnostic_server.rs index f8eeabfc29d..2c2d65cde8f 100644 --- a/src/cargo/util/diagnostic_server.rs +++ b/src/cargo/util/diagnostic_server.rs @@ -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\ diff --git a/tests/testsuite/fix.rs b/tests/testsuite/fix.rs index 7cb5bd65e9d..2ab1e5a15dc 100644 --- a/tests/testsuite/fix.rs +++ b/tests/testsuite/fix.rs @@ -6,7 +6,7 @@ use cargo_test_support::git::{self, init}; use cargo_test_support::paths::{self, CargoPathExt}; use cargo_test_support::registry::{Dependency, Package}; use cargo_test_support::tools; -use cargo_test_support::{basic_manifest, is_nightly, project, Project}; +use cargo_test_support::{basic_manifest, is_nightly, project}; #[cargo_test] fn do_not_fix_broken_builds() { @@ -53,179 +53,6 @@ fn fix_broken_if_requested() { .run(); } -fn rustc_shim_for_cargo_fix() -> Project { - // This works as follows: - // - Create a `rustc` shim (the "foo" project) which will pretend that the - // verification step fails. - // - There is an empty build script so `foo` has `OUT_DIR` to track the steps. - // - The first "check", `foo` creates a file in OUT_DIR, and it completes - // successfully with a warning diagnostic to remove unused `mut`. - // - rustfix removes the `mut`. - // - The second "check" to verify the changes, `foo` swaps out the content - // with something that fails to compile. It creates a second file so it - // won't do anything in the third check. - // - cargo fix discovers that the fix failed, and it backs out the changes. - // - The third "check" is done to display the original diagnostics of the - // original code. - let p = project() - .file( - "foo/Cargo.toml", - r#" - [package] - name = 'foo' - version = '0.1.0' - [workspace] - "#, - ) - .file( - "foo/src/main.rs", - r#" - use std::env; - use std::fs; - use std::io::Write; - use std::path::{Path, PathBuf}; - use std::process::{self, Command}; - - fn main() { - // Ignore calls to things like --print=file-names and compiling build.rs. - // Also compatible for rustc invocations with `@path` argfile. - let is_lib_rs = env::args_os() - .map(PathBuf::from) - .flat_map(|p| if let Some(p) = p.to_str().unwrap_or_default().strip_prefix("@") { - fs::read_to_string(p).unwrap().lines().map(PathBuf::from).collect() - } else { - vec![p] - }) - .any(|l| l == Path::new("src/lib.rs")); - if is_lib_rs { - let path = PathBuf::from(env::var_os("OUT_DIR").unwrap()); - let first = path.join("first"); - let second = path.join("second"); - if first.exists() && !second.exists() { - fs::write("src/lib.rs", b"not rust code").unwrap(); - fs::File::create(&second).unwrap(); - } else { - fs::File::create(&first).unwrap(); - } - } - let status = Command::new("rustc") - .args(env::args().skip(1)) - .env_remove("CARGO_MAKEFLAGS") - .status() - .expect("failed to run rustc"); - process::exit(status.code().unwrap_or(2)); - } - "#, - ) - .file( - "bar/Cargo.toml", - r#" - [package] - name = 'bar' - version = '0.1.0' - [workspace] - "#, - ) - .file("bar/build.rs", "fn main() {}") - .file( - "bar/src/lib.rs", - r#" - pub fn foo() { - let mut x = 3; - drop(x); - } - "#, - ) - .build(); - - // Build our rustc shim - p.cargo("build").cwd("foo").run(); - - p -} - -#[cargo_test] -fn broken_fixes_backed_out() { - let p = rustc_shim_for_cargo_fix(); - // Attempt to fix code, but our shim will always fail the second compile. - p.cargo("fix --allow-no-vcs --lib") - .cwd("bar") - .env("__CARGO_FIX_YOLO", "1") - .env("RUSTC", p.root().join("foo/target/debug/foo")) - .with_stderr_contains( - "warning: failed to automatically apply fixes suggested by rustc \ - to crate `bar`\n\ - \n\ - after fixes were automatically applied the compiler reported \ - errors within these files:\n\ - \n \ - * src/lib.rs\n\ - \n\ - 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\ - a number of compiler warnings after this message which cargo\n\ - attempted to fix but failed. If you could open an issue at\n\ - https://github.com/rust-lang/rust/issues\n\ - quoting the full output of this command we'd be very appreciative!\n\ - Note that you may be able to make some more progress in the near-term\n\ - fixing code with the `--broken-code` flag\n\ - \n\ - The following errors were reported:\n\ - error: expected one of `!` or `::`, found `rust`\n\ - ", - ) - .with_stderr_contains("Original diagnostics will follow.") - .with_stderr_contains("[WARNING] variable does not need to be mutable") - .with_stderr_does_not_contain("[..][FIXED][..]") - .run(); - - // Make sure the fix which should have been applied was backed out - assert!(p.read_file("bar/src/lib.rs").contains("let mut x = 3;")); -} - -#[cargo_test] -fn broken_clippy_fixes_backed_out() { - let p = rustc_shim_for_cargo_fix(); - // Attempt to fix code, but our shim will always fail the second compile. - // Also, we use `clippy` as a workspace wrapper to make sure that we properly - // generate the report bug text. - p.cargo("fix --allow-no-vcs --lib") - .cwd("bar") - .env("__CARGO_FIX_YOLO", "1") - .env("RUSTC", p.root().join("foo/target/debug/foo")) - // We can't use `clippy` so we use a `rustc` workspace wrapper instead - .env("RUSTC_WORKSPACE_WRAPPER", tools::wrapped_clippy_driver()) - .with_stderr_contains( - "warning: failed to automatically apply fixes suggested by rustc \ - to crate `bar`\n\ - \n\ - after fixes were automatically applied the compiler reported \ - errors within these files:\n\ - \n \ - * src/lib.rs\n\ - \n\ - 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\ - a number of compiler warnings after this message which cargo\n\ - attempted to fix but failed. If you could open an issue at\n\ - https://github.com/rust-lang/rust-clippy/issues\n\ - quoting the full output of this command we'd be very appreciative!\n\ - Note that you may be able to make some more progress in the near-term\n\ - fixing code with the `--broken-code` flag\n\ - \n\ - The following errors were reported:\n\ - error: expected one of `!` or `::`, found `rust`\n\ - ", - ) - .with_stderr_contains("Original diagnostics will follow.") - .with_stderr_contains("[WARNING] variable does not need to be mutable") - .with_stderr_does_not_contain("[..][FIXED][..]") - .run(); - - // Make sure the fix which should have been applied was backed out - assert!(p.read_file("bar/src/lib.rs").contains("let mut x = 3;")); -} - #[cargo_test] fn fix_path_deps() { let p = project() diff --git a/tests/testsuite/fix_n_times.rs b/tests/testsuite/fix_n_times.rs new file mode 100644 index 00000000000..b669924d858 --- /dev/null +++ b/tests/testsuite/fix_n_times.rs @@ -0,0 +1,513 @@ +//! Tests for the `cargo fix` command, specifically targeting the logic around +//! running rustc multiple times to apply and verify fixes. +//! +//! These tests use a replacement of rustc ("rustc-fix-shim") which emits JSON +//! messages based on what the test is exercising. It uses an environment +//! variable RUSTC_FIX_SHIM_SEQUENCE which determines how it should behave +//! based on how many times `rustc` has run. It keeps track of how many times +//! rustc has run in a local file. +//! +//! For example, a sequence of `[Step::OneFix, Step::Error]` will emit one +//! suggested fix the first time `rustc` is run, and then the next time it is +//! run it will generate an error. +//! +//! The [`expect_fix_runs_rustc_n_times`] function handles setting everything +//! up, and verifying the results. + +use cargo_test_support::{basic_manifest, paths, project, tools, Execs}; +use std::path::PathBuf; +use std::sync::{Mutex, OnceLock}; + +/// The action that the `rustc` shim should take in the current sequence of +/// events. +#[derive(Clone, Copy, PartialEq)] +#[repr(u8)] +enum Step { + /// Exits with success with no messages. + SuccessNoOutput = b'0', + /// Emits one suggested fix. + /// + /// The suggested fix involves updating the number of the first line + /// comment which starts as `// fix-count 0`. + OneFix = b'1', + /// Emits two suggested fixes which overlap, which rustfix can only apply + /// one of them, and fails for the other. + /// + /// The suggested fix is the same as `Step::OneFix`, it just shows up + /// twice. + TwoFixOverlapping = b'2', + /// Generates a warning without a suggestion. + Warning = b'w', + /// Generates an error message with no suggestion. + Error = b'e', + /// Emits one suggested fix and an error. + OneFixError = b'f', +} + +/// Verifies `cargo fix` behavior based on the given sequence of behaviors for +/// `rustc`. +/// +/// - `sequence` is the sequence of behaviors for each call to `rustc`. +/// If rustc is called more often than the number of steps, then it will panic. +/// - `extra_execs` a callback that allows extra customization of the [`Execs`]. +/// - `expected_stderr` is the expected output from cargo. +/// - `expected_lib_rs` is the expected contents of `src/lib.rs` after the +/// fixes have been applied. The file starts out with the content `// +/// fix-count 0`, and the number increases based on which suggestions are +/// applied. +fn expect_fix_runs_rustc_n_times( + sequence: &[Step], + extra_execs: impl FnOnce(&mut Execs), + expected_stderr: &str, + expected_lib_rs: &str, +) { + let rustc = rustc_for_cargo_fix(); + let p = project().file("src/lib.rs", "// fix-count 0").build(); + + let sequence_vec: Vec<_> = sequence.iter().map(|x| *x as u8).collect(); + let sequence_str = std::str::from_utf8(&sequence_vec).unwrap(); + + let mut execs = p.cargo("fix --allow-no-vcs --lib"); + execs + .env("RUSTC", &rustc) + .env("RUSTC_FIX_SHIM_SEQUENCE", sequence_str) + .with_stderr(expected_stderr); + extra_execs(&mut execs); + execs.run(); + let lib_rs = p.read_file("src/lib.rs"); + assert_eq!(expected_lib_rs, lib_rs); + let count: usize = p.read_file("rustc-fix-shim-count").parse().unwrap(); + assert_eq!(sequence.len(), count); +} + +/// Returns the path to the rustc replacement executable. +fn rustc_for_cargo_fix() -> PathBuf { + static FIX_SHIM: OnceLock>> = OnceLock::new(); + + let mut lock = FIX_SHIM.get_or_init(|| Default::default()).lock().unwrap(); + if let Some(path) = &*lock { + return path.clone(); + } + + let p = project() + .at(paths::global_root().join("rustc-fix-shim")) + .file("Cargo.toml", &basic_manifest("rustc-fix-shim", "1.0.0")) + .file( + "src/main.rs", + r##" +fn main() { + if std::env::var_os("CARGO_PKG_NAME").is_none() { + // Handle things like rustc -Vv + let r = std::process::Command::new("rustc") + .args(std::env::args_os().skip(1)) + .status(); + std::process::exit(r.unwrap().code().unwrap_or(2)); + } + + // Keep track of which step in the sequence that needs to run. + let successful_count = std::fs::read_to_string("rustc-fix-shim-count") + .map(|c| c.parse().unwrap()) + .unwrap_or(0); + std::fs::write("rustc-fix-shim-count", format!("{}", successful_count + 1)).unwrap(); + // The sequence tells us which behavior we should have. + let seq = std::env::var("RUSTC_FIX_SHIM_SEQUENCE").unwrap(); + if successful_count >= seq.len() { + panic!("rustc called too many times count={}, \ + make sure to update the Step sequence", successful_count); + } + match seq.as_bytes()[successful_count] { + b'0' => return, + b'1' => { + output_suggestion(successful_count + 1); + } + b'2' => { + output_suggestion(successful_count + 1); + output_suggestion(successful_count + 2); + } + b'w' => { + output_message("warning", successful_count + 1); + } + b'e' => { + output_message("error", successful_count + 1); + std::process::exit(1); + } + b'f' => { + output_suggestion(successful_count + 1); + output_message("error", successful_count + 2); + std::process::exit(1); + } + _ => panic!("unexpected sequence"), + } +} + +fn output_suggestion(count: usize) { + let json = format!( + r#"{{ + "$message_type": "diagnostic", + "message": "rustc fix shim comment {count}", + "code": null, + "level": "warning", + "spans": + [ + {{ + "file_name": "src/lib.rs", + "byte_start": 13, + "byte_end": 14, + "line_start": 1, + "line_end": 1, + "column_start": 14, + "column_end": 15, + "is_primary": true, + "text": + [ + {{ + "text": "// fix-count 0", + "highlight_start": 14, + "highlight_end": 15 + }} + ], + "label": "increase this number", + "suggested_replacement": null, + "suggestion_applicability": null, + "expansion": null + }} + ], + "children": + [ + {{ + "message": "update the number here", + "code": null, + "level": "help", + "spans": + [ + {{ + "file_name": "src/lib.rs", + "byte_start": 13, + "byte_end": 14, + "line_start": 1, + "line_end": 1, + "column_start": 14, + "column_end": 15, + "is_primary": true, + "text": + [ + {{ + "text": "// fix-count 0", + "highlight_start": 14, + "highlight_end": 15 + }} + ], + "label": null, + "suggested_replacement": "{count}", + "suggestion_applicability": "MachineApplicable", + "expansion": null + }} + ], + "children": [], + "rendered": null + }} + ], + "rendered": "rustc fix shim comment {count}" + }}"#, + ) + .replace("\n", ""); + eprintln!("{json}"); +} + +fn output_message(level: &str, count: usize) { + let json = format!( + r#"{{ + "$message_type": "diagnostic", + "message": "rustc fix shim {level} count={count}", + "code": null, + "level": "{level}", + "spans": + [ + {{ + "file_name": "src/lib.rs", + "byte_start": 0, + "byte_end": 0, + "line_start": 1, + "line_end": 1, + "column_start": 1, + "column_end": 1, + "is_primary": true, + "text": + [ + {{ + "text": "// fix-count 0", + "highlight_start": 1, + "highlight_end": 4 + }} + ], + "label": "forced error", + "suggested_replacement": null, + "suggestion_applicability": null, + "expansion": null + }} + ], + "children": [], + "rendered": "rustc fix shim {level} count={count}" +}}"#, + ) + .replace("\n", ""); + eprintln!("{json}"); +} + "##, + ) + .build(); + p.cargo("build").run(); + let path = p.bin("rustc-fix-shim"); + *lock = Some(path.clone()); + path +} + +#[cargo_test] +fn fix_no_suggestions() { + // No suggested fixes. + expect_fix_runs_rustc_n_times( + &[Step::SuccessNoOutput, Step::SuccessNoOutput], + |_execs| {}, + "\ +[CHECKING] foo [..] +[FINISHED] [..] +", + "// fix-count 0", + ); +} + +#[cargo_test] +fn fix_one_suggestion() { + // One suggested fix, with a successful verification, no output. + expect_fix_runs_rustc_n_times( + &[Step::OneFix, Step::SuccessNoOutput], + |_execs| {}, + "\ +[CHECKING] foo [..] +[FIXED] src/lib.rs (1 fix) +[FINISHED] [..] +", + "// fix-count 1", + ); +} + +#[cargo_test] +fn fix_one_overlapping() { + // Two suggested fixes, where one fails, then the next step returns no suggestions. + expect_fix_runs_rustc_n_times( + &[ + Step::TwoFixOverlapping, + Step::SuccessNoOutput, + Step::SuccessNoOutput, + ], + |_execs| {}, + "\ +[CHECKING] foo [..] +[FIXED] src/lib.rs (1 fix) +[FINISHED] [..] +", + "// fix-count 2", + ); +} + +#[cargo_test] +fn fix_overlapping_max() { + // Rustc repeatedly spits out suggestions that overlap, which should hit + // the limit of 4 attempts. It should show the output from the 5th attempt. + expect_fix_runs_rustc_n_times( + &[ + Step::TwoFixOverlapping, + Step::TwoFixOverlapping, + Step::TwoFixOverlapping, + Step::TwoFixOverlapping, + Step::TwoFixOverlapping, + Step::TwoFixOverlapping, + ], + |_execs| {}, + "\ +[CHECKING] foo [..] +warning: error applying suggestions to `src/lib.rs` + +The full error message was: + +> cannot replace slice of data that was already replaced + +This likely indicates a bug in either rustc or cargo itself, +and we would appreciate a bug report! You're likely to see +a number of compiler warnings after this message which cargo +attempted to fix but failed. If you could open an issue at +https://github.com/rust-lang/rust/issues +quoting the full output of this command we'd be very appreciative! +Note that you may be able to make some more progress in the near-term +fixing code with the `--broken-code` flag + +[FIXED] src/lib.rs (4 fixes) +rustc fix shim comment 6 +rustc fix shim comment 7 +warning: `foo` (lib) generated 2 warnings (run `cargo fix --lib -p foo` to apply 2 suggestions) +[FINISHED] [..] +", + "// fix-count 5", + ); +} + +#[cargo_test] +fn fix_verification_failed() { + // One suggested fix, with an error in the verification step. + // This should cause `cargo fix` to back out the changes. + expect_fix_runs_rustc_n_times( + &[Step::OneFix, Step::Error, Step::OneFix], + |_execs| {}, + "\ +[CHECKING] foo [..] +warning: failed to automatically apply fixes suggested by rustc to crate `foo` + +after fixes were automatically applied the compiler reported errors within these files: + + * src/lib.rs + +This likely indicates a bug in either rustc or cargo itself, +and we would appreciate a bug report! You're likely to see +a number of compiler warnings after this message which cargo +attempted to fix but failed. If you could open an issue at +https://github.com/rust-lang/rust/issues +quoting the full output of this command we'd be very appreciative! +Note that you may be able to make some more progress in the near-term +fixing code with the `--broken-code` flag + +The following errors were reported: +rustc fix shim error count=2 +Original diagnostics will follow. + +rustc fix shim comment 3 +warning: `foo` (lib) generated 1 warning (run `cargo fix --lib -p foo` to apply 1 suggestion) +[FINISHED] [..] +", + "// fix-count 0", + ); +} + +#[cargo_test] +fn fix_verification_failed_clippy() { + // This is the same as `fix_verification_failed_clippy`, except it checks + // the error message has the customization for the clippy URL and + // subcommand. + expect_fix_runs_rustc_n_times( + &[Step::OneFix, Step::Error, Step::OneFix], + |execs| { + execs.env("RUSTC_WORKSPACE_WRAPPER", tools::wrapped_clippy_driver()); + }, + "\ +[CHECKING] foo [..] +warning: failed to automatically apply fixes suggested by rustc to crate `foo` + +after fixes were automatically applied the compiler reported errors within these files: + + * src/lib.rs + +This likely indicates a bug in either rustc or cargo itself, +and we would appreciate a bug report! You're likely to see +a number of compiler warnings after this message which cargo +attempted to fix but failed. If you could open an issue at +https://github.com/rust-lang/rust-clippy/issues +quoting the full output of this command we'd be very appreciative! +Note that you may be able to make some more progress in the near-term +fixing code with the `--broken-code` flag + +The following errors were reported: +rustc fix shim error count=2 +Original diagnostics will follow. + +rustc fix shim comment 3 +warning: `foo` (lib) generated 1 warning (run `cargo clippy --fix --lib -p foo` to apply 1 suggestion) +[FINISHED] [..] +", + "// fix-count 0", + ); +} + +#[cargo_test] +fn warnings() { + // Only emits warnings. + expect_fix_runs_rustc_n_times( + &[Step::Warning, Step::Warning], + |_execs| {}, + "\ +[CHECKING] foo [..] +rustc fix shim warning count=2 +warning: `foo` (lib) generated 1 warning +[FINISHED] [..] +", + "// fix-count 0", + ); +} + +#[cargo_test] +fn starts_with_error() { + // The source code doesn't compile to start with. + expect_fix_runs_rustc_n_times( + &[Step::Error, Step::Error], + |execs| { + execs.with_status(101); + }, + "\ +[CHECKING] foo [..] +rustc fix shim error count=2 +error: could not compile `foo` (lib) due to 1 previous error +", + "// fix-count 0", + ); +} + +#[cargo_test] +fn broken_code_no_suggestions() { + // --broken-code with no suggestions + expect_fix_runs_rustc_n_times( + &[Step::Error, Step::Error], + |execs| { + execs.arg("--broken-code").with_status(101); + }, + "\ +[CHECKING] foo [..] +rustc fix shim error count=2 +error: could not compile `foo` (lib) due to 1 previous error +", + "// fix-count 0", + ); +} + +#[cargo_test] +fn broken_code_one_suggestion() { + // --broken-code where there is an error and a suggestion. + expect_fix_runs_rustc_n_times( + &[Step::OneFixError, Step::Error, Step::Error], + |execs| { + execs.arg("--broken-code").with_status(101); + }, + "\ +[CHECKING] foo [..] +warning: failed to automatically apply fixes suggested by rustc to crate `foo` + +after fixes were automatically applied the compiler reported errors within these files: + + * src/lib.rs + +This likely indicates a bug in either rustc or cargo itself, +and we would appreciate a bug report! You're likely to see +a number of compiler warnings after this message which cargo +attempted to fix but failed. If you could open an issue at +https://github.com/rust-lang/rust/issues +quoting the full output of this command we'd be very appreciative! +Note that you may be able to make some more progress in the near-term +fixing code with the `--broken-code` flag + +The following errors were reported: +rustc fix shim error count=2 +Original diagnostics will follow. + +rustc fix shim error count=3 +error: could not compile `foo` (lib) due to 1 previous error +", + "// fix-count 1", + ); +} diff --git a/tests/testsuite/main.rs b/tests/testsuite/main.rs index 8e2d6dedf76..1cb2d77cf1b 100644 --- a/tests/testsuite/main.rs +++ b/tests/testsuite/main.rs @@ -90,6 +90,7 @@ mod features2; mod features_namespaced; mod fetch; mod fix; +mod fix_n_times; mod freshness; mod future_incompat_report; mod generate_lockfile; From e7eaa5190968e8b1d6d0898ca70913b5684832ca Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Tue, 2 Jan 2024 14:42:21 -0800 Subject: [PATCH 2/2] `cargo fix`: Call rustc fewer times. This changes `cargo fix` so that it keeps track of the output so that it doesn't need to run the final "show the output" step. --- crates/rustfix/src/lib.rs | 9 ++ src/cargo/ops/fix.rs | 222 ++++++++++++++++++++------------- tests/testsuite/fix.rs | 3 +- tests/testsuite/fix_n_times.rs | 39 +++--- 4 files changed, 163 insertions(+), 110 deletions(-) diff --git a/crates/rustfix/src/lib.rs b/crates/rustfix/src/lib.rs index dd3ebbd63e9..8af88cd50d0 100644 --- a/crates/rustfix/src/lib.rs +++ b/crates/rustfix/src/lib.rs @@ -215,6 +215,8 @@ pub fn collect_suggestions( /// 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 { @@ -222,6 +224,7 @@ impl CodeFix { pub fn new(s: &str) -> CodeFix { CodeFix { data: replace::Data::new(s.as_bytes()), + modified: false, } } @@ -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(()) @@ -240,6 +244,11 @@ impl CodeFix { pub fn finish(&self) -> Result { 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`. diff --git a/src/cargo/ops/fix.rs b/src/cargo/ops/fix.rs index 87d2e47e428..d57b94e0184 100644 --- a/src/cargo/ops/fix.rs +++ b/src/cargo/ops/fix.rs @@ -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 _}; @@ -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, + /// 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, fixes_applied: u32, @@ -472,11 +492,6 @@ fn rustfix_crate( args: &FixArgs, config: &Config, ) -> CargoResult { - 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. @@ -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 @@ -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(), @@ -565,7 +609,11 @@ 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. @@ -573,11 +621,11 @@ fn rustfix_crate( /// 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, 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(); @@ -596,7 +644,7 @@ fn rustfix_and_fix( filename, output.status.code() ); - return Ok(()); + return Ok((output, false)); } let fix_mode = config @@ -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 @@ -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 @@ -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!( diff --git a/tests/testsuite/fix.rs b/tests/testsuite/fix.rs index 2ab1e5a15dc..87b23b89063 100644 --- a/tests/testsuite/fix.rs +++ b/tests/testsuite/fix.rs @@ -1522,11 +1522,10 @@ fn fix_shared_cross_workspace() { fn abnormal_exit() { // rustc fails unexpectedly after applying fixes, should show some error information. // - // This works with a proc-macro that runs three times: + // This works with a proc-macro that runs twice: // - First run (collect diagnostics pass): writes a file, exits normally. // - Second run (verify diagnostics work): it detects the presence of the // file, removes the file, and aborts the process. - // - Third run (collecting messages to display): file not found, exits normally. let p = project() .file( "Cargo.toml", diff --git a/tests/testsuite/fix_n_times.rs b/tests/testsuite/fix_n_times.rs index b669924d858..e74fd595dc4 100644 --- a/tests/testsuite/fix_n_times.rs +++ b/tests/testsuite/fix_n_times.rs @@ -266,7 +266,7 @@ fn output_message(level: &str, count: usize) { fn fix_no_suggestions() { // No suggested fixes. expect_fix_runs_rustc_n_times( - &[Step::SuccessNoOutput, Step::SuccessNoOutput], + &[Step::SuccessNoOutput], |_execs| {}, "\ [CHECKING] foo [..] @@ -295,11 +295,7 @@ fn fix_one_suggestion() { fn fix_one_overlapping() { // Two suggested fixes, where one fails, then the next step returns no suggestions. expect_fix_runs_rustc_n_times( - &[ - Step::TwoFixOverlapping, - Step::SuccessNoOutput, - Step::SuccessNoOutput, - ], + &[Step::TwoFixOverlapping, Step::SuccessNoOutput], |_execs| {}, "\ [CHECKING] foo [..] @@ -321,7 +317,6 @@ fn fix_overlapping_max() { Step::TwoFixOverlapping, Step::TwoFixOverlapping, Step::TwoFixOverlapping, - Step::TwoFixOverlapping, ], |_execs| {}, "\ @@ -342,8 +337,8 @@ Note that you may be able to make some more progress in the near-term fixing code with the `--broken-code` flag [FIXED] src/lib.rs (4 fixes) +rustc fix shim comment 5 rustc fix shim comment 6 -rustc fix shim comment 7 warning: `foo` (lib) generated 2 warnings (run `cargo fix --lib -p foo` to apply 2 suggestions) [FINISHED] [..] ", @@ -356,7 +351,7 @@ fn fix_verification_failed() { // One suggested fix, with an error in the verification step. // This should cause `cargo fix` to back out the changes. expect_fix_runs_rustc_n_times( - &[Step::OneFix, Step::Error, Step::OneFix], + &[Step::OneFix, Step::Error], |_execs| {}, "\ [CHECKING] foo [..] @@ -379,7 +374,7 @@ The following errors were reported: rustc fix shim error count=2 Original diagnostics will follow. -rustc fix shim comment 3 +rustc fix shim comment 1 warning: `foo` (lib) generated 1 warning (run `cargo fix --lib -p foo` to apply 1 suggestion) [FINISHED] [..] ", @@ -393,7 +388,7 @@ fn fix_verification_failed_clippy() { // the error message has the customization for the clippy URL and // subcommand. expect_fix_runs_rustc_n_times( - &[Step::OneFix, Step::Error, Step::OneFix], + &[Step::OneFix, Step::Error], |execs| { execs.env("RUSTC_WORKSPACE_WRAPPER", tools::wrapped_clippy_driver()); }, @@ -418,7 +413,7 @@ The following errors were reported: rustc fix shim error count=2 Original diagnostics will follow. -rustc fix shim comment 3 +rustc fix shim comment 1 warning: `foo` (lib) generated 1 warning (run `cargo clippy --fix --lib -p foo` to apply 1 suggestion) [FINISHED] [..] ", @@ -430,11 +425,11 @@ warning: `foo` (lib) generated 1 warning (run `cargo clippy --fix --lib -p foo` fn warnings() { // Only emits warnings. expect_fix_runs_rustc_n_times( - &[Step::Warning, Step::Warning], + &[Step::Warning], |_execs| {}, "\ [CHECKING] foo [..] -rustc fix shim warning count=2 +rustc fix shim warning count=1 warning: `foo` (lib) generated 1 warning [FINISHED] [..] ", @@ -446,13 +441,13 @@ warning: `foo` (lib) generated 1 warning fn starts_with_error() { // The source code doesn't compile to start with. expect_fix_runs_rustc_n_times( - &[Step::Error, Step::Error], + &[Step::Error], |execs| { execs.with_status(101); }, "\ [CHECKING] foo [..] -rustc fix shim error count=2 +rustc fix shim error count=1 error: could not compile `foo` (lib) due to 1 previous error ", "// fix-count 0", @@ -463,13 +458,13 @@ error: could not compile `foo` (lib) due to 1 previous error fn broken_code_no_suggestions() { // --broken-code with no suggestions expect_fix_runs_rustc_n_times( - &[Step::Error, Step::Error], + &[Step::Error], |execs| { execs.arg("--broken-code").with_status(101); }, "\ [CHECKING] foo [..] -rustc fix shim error count=2 +rustc fix shim error count=1 error: could not compile `foo` (lib) due to 1 previous error ", "// fix-count 0", @@ -480,7 +475,7 @@ error: could not compile `foo` (lib) due to 1 previous error fn broken_code_one_suggestion() { // --broken-code where there is an error and a suggestion. expect_fix_runs_rustc_n_times( - &[Step::OneFixError, Step::Error, Step::Error], + &[Step::OneFixError, Step::Error], |execs| { execs.arg("--broken-code").with_status(101); }, @@ -505,8 +500,10 @@ The following errors were reported: rustc fix shim error count=2 Original diagnostics will follow. -rustc fix shim error count=3 -error: could not compile `foo` (lib) due to 1 previous error +rustc fix shim comment 1 +rustc fix shim error count=2 +warning: `foo` (lib) generated 1 warning +error: could not compile `foo` (lib) due to 1 previous error; 1 warning emitted ", "// fix-count 1", );