From 08106278911fb7450933f9e52da37a31ab2c5303 Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Tue, 23 Apr 2024 10:17:37 -0400 Subject: [PATCH] fix(cargo-fix): dont fix into standard library This takes a more aggressive approach by stop any write into sysroot. --- src/cargo/ops/fix.rs | 51 +++++++++++++++++++++++++++++++++++------- tests/testsuite/fix.rs | 47 -------------------------------------- 2 files changed, 43 insertions(+), 55 deletions(-) diff --git a/src/cargo/ops/fix.rs b/src/cargo/ops/fix.rs index 88c90e31af4..c952d153ed2 100644 --- a/src/cargo/ops/fix.rs +++ b/src/cargo/ops/fix.rs @@ -50,6 +50,7 @@ use rustfix::CodeFix; use semver::Version; use tracing::{debug, trace, warn}; +use crate::core::compiler::CompileKind; use crate::core::compiler::RustcTargetData; use crate::core::resolver::features::{DiffMap, FeatureOpts, FeatureResolver, FeaturesFor}; use crate::core::resolver::{HasDevUnits, Resolve, ResolveBehavior}; @@ -78,6 +79,14 @@ const EDITION_ENV_INTERNAL: &str = "__CARGO_FIX_EDITION"; /// **Internal only.** /// For passing [`FixOptions::idioms`] through to cargo running in proxy mode. const IDIOMS_ENV_INTERNAL: &str = "__CARGO_FIX_IDIOMS"; +/// **Internal only.** +/// The sysroot path. +/// +/// This is for preventing `cargo fix` from fixing rust std/core libs. See +/// +/// * +/// * +const SYSROOT_INTERNAL: &str = "__CARGO_FIX_RUST_SRC"; pub struct FixOptions { pub edition: bool, @@ -97,6 +106,8 @@ pub fn fix( ) -> CargoResult<()> { check_version_control(gctx, opts)?; + let mut target_data = + RustcTargetData::new(original_ws, &opts.compile_opts.build_config.requested_kinds)?; if opts.edition { let specs = opts.compile_opts.spec.to_package_id_specs(&original_ws)?; let members: Vec<&Package> = original_ws @@ -105,7 +116,7 @@ pub fn fix( .collect(); migrate_manifests(original_ws, &members)?; - check_resolver_change(&original_ws, opts)?; + check_resolver_change(&original_ws, &mut target_data, opts)?; } let mut ws = Workspace::new(&root_manifest, gctx)?; ws.set_honor_rust_version(original_ws.honor_rust_version()); @@ -129,6 +140,11 @@ pub fn fix( wrapper.env(IDIOMS_ENV_INTERNAL, "1"); } + let sysroot = &target_data.info(CompileKind::Host).sysroot; + if sysroot.is_dir() { + wrapper.env(SYSROOT_INTERNAL, sysroot); + } + *opts .compile_opts .build_config @@ -328,7 +344,11 @@ fn add_feature_for_unused_deps(pkg: &Package, parent: &mut dyn toml_edit::TableL fixes } -fn check_resolver_change(ws: &Workspace<'_>, opts: &FixOptions) -> CargoResult<()> { +fn check_resolver_change<'gctx>( + ws: &Workspace<'gctx>, + target_data: &mut RustcTargetData<'gctx>, + opts: &FixOptions, +) -> CargoResult<()> { let root = ws.root_maybe(); match root { MaybePackage::Package(root_pkg) => { @@ -355,12 +375,10 @@ fn check_resolver_change(ws: &Workspace<'_>, opts: &FixOptions) -> CargoResult<( // 2018 without `resolver` set must be V1 assert_eq!(ws.resolve_behavior(), ResolveBehavior::V1); let specs = opts.compile_opts.spec.to_package_id_specs(ws)?; - let mut target_data = - RustcTargetData::new(ws, &opts.compile_opts.build_config.requested_kinds)?; let mut resolve_differences = |has_dev_units| -> CargoResult<(WorkspaceResolve<'_>, DiffMap)> { let ws_resolve = ops::resolve_ws_with_opts( ws, - &mut target_data, + target_data, &opts.compile_opts.build_config.requested_kinds, &opts.compile_opts.cli_features, &specs, @@ -371,7 +389,7 @@ fn check_resolver_change(ws: &Workspace<'_>, opts: &FixOptions) -> CargoResult<( let feature_opts = FeatureOpts::new_behavior(ResolveBehavior::V2, has_dev_units); let v2_features = FeatureResolver::resolve( ws, - &mut target_data, + target_data, &ws_resolve.targeted_resolve, &ws_resolve.pkg_set, &opts.compile_opts.cli_features, @@ -677,7 +695,8 @@ fn rustfix_crate( // We'll generate new errors below. file.errors_applying_fixes.clear(); } - (last_output, last_made_changes) = rustfix_and_fix(&mut files, rustc, filename, gctx)?; + (last_output, last_made_changes) = + rustfix_and_fix(&mut files, rustc, filename, args, gctx)?; if current_iteration == 0 { first_output = Some(last_output.clone()); } @@ -734,6 +753,7 @@ fn rustfix_and_fix( files: &mut HashMap, rustc: &ProcessBuilder, filename: &Path, + args: &FixArgs, gctx: &GlobalContext, ) -> CargoResult<(Output, bool)> { // If not empty, filter by these lints. @@ -798,10 +818,17 @@ fn rustfix_and_fix( continue; }; + let file_path = Path::new(&file_name); // Do not write into registry cache. See rust-lang/cargo#9857. - if Path::new(&file_name).starts_with(home_path) { + if file_path.starts_with(home_path) { continue; } + // Do not write into standard library source. See rust-lang/cargo#9857. + if let Some(sysroot) = args.sysroot.as_deref() { + if file_path.starts_with(sysroot) { + continue; + } + } if !file_names.clone().all(|f| f == &file_name) { trace!("rejecting as it changes multiple files: {:?}", suggestion); @@ -958,6 +985,8 @@ struct FixArgs { other: Vec, /// Path to the `rustc` executable. rustc: PathBuf, + /// Path to host sysroot. + sysroot: Option, } impl FixArgs { @@ -1029,6 +1058,11 @@ impl FixArgs { .saturating_next() }); + // ALLOWED: For the internal mechanism of `cargo fix` only. + // Shouldn't be set directly by anyone. + #[allow(clippy::disallowed_methods)] + let sysroot = env::var_os(SYSROOT_INTERNAL).map(PathBuf::from); + Ok(FixArgs { file, prepare_for_edition, @@ -1036,6 +1070,7 @@ impl FixArgs { enabled_edition, other, rustc, + sysroot, }) } diff --git a/tests/testsuite/fix.rs b/tests/testsuite/fix.rs index 4875b0bafb2..4175a35b2d7 100644 --- a/tests/testsuite/fix.rs +++ b/tests/testsuite/fix.rs @@ -2200,53 +2200,6 @@ fn fix_in_rust_src() { .env("RUSTC", &rustc_bin) .with_status(101) .with_stderr(r#"[CHECKING] foo v0.0.0 ([..]) -[WARNING] failed to automatically apply fixes suggested by rustc to crate `foo` - -after fixes were automatically applied the compiler reported errors within these files: - - * [..]/lib/rustlib/src/rust/library/core/src/macros/mod.rs - * 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: -error[E0308]: mismatched types - --> lib.rs:5:9 - | -2 | / if true { -3 | | writeln!(w, "`;?` here ->")?; -4 | | } else { -5 | | writeln!(w, "but not here") - | | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `()`, found `Result<(), Error>` -6 | | } - | |_____- expected this to be `()` - | - = note: expected unit type `()` - found enum `Result<(), std::fmt::Error>` - = note: this error originates in the macro `writeln` (in Nightly builds, run with -Z macro-backtrace for more info) -help: consider using a semicolon here - | -6 | }; - | + -help: you might have meant to return this value - | -5 | return writeln!(w, "but not here"); - | ++++++ + -help: use the `?` operator to extract the `Result<(), std::fmt::Error>` value, propagating a `Result::Err` value to the caller - --> [..]/lib/rustlib/src/rust/library/core/src/macros/mod.rs:670:58 - | -67| $dst.write_fmt($crate::format_args_nl!($($arg)*))? - | + - -Original diagnostics will follow. - error[E0308]: mismatched types --> lib.rs:5:9 |