Skip to content

Commit

Permalink
fix(cargo-fix): dont fix into standard library
Browse files Browse the repository at this point in the history
This takes a more aggressive approach by stop any write into sysroot.
  • Loading branch information
weihanglo committed Apr 23, 2024
1 parent 8410c1e commit 0810627
Show file tree
Hide file tree
Showing 2 changed files with 43 additions and 55 deletions.
51 changes: 43 additions & 8 deletions src/cargo/ops/fix.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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
///
/// * <https://github.com/rust-lang/cargo/issues/9857>
/// * <https://github.com/rust-lang/rust/issues/88514#issuecomment-2043469384>
const SYSROOT_INTERNAL: &str = "__CARGO_FIX_RUST_SRC";

pub struct FixOptions {
pub edition: bool,
Expand All @@ -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
Expand All @@ -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());
Expand All @@ -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
Expand Down Expand Up @@ -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) => {
Expand All @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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());
}
Expand Down Expand Up @@ -734,6 +753,7 @@ fn rustfix_and_fix(
files: &mut HashMap<String, FixedFile>,
rustc: &ProcessBuilder,
filename: &Path,
args: &FixArgs,
gctx: &GlobalContext,
) -> CargoResult<(Output, bool)> {
// If not empty, filter by these lints.
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -958,6 +985,8 @@ struct FixArgs {
other: Vec<OsString>,
/// Path to the `rustc` executable.
rustc: PathBuf,
/// Path to host sysroot.
sysroot: Option<PathBuf>,
}

impl FixArgs {
Expand Down Expand Up @@ -1029,13 +1058,19 @@ 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,
idioms,
enabled_edition,
other,
rustc,
sysroot,
})
}

Expand Down
47 changes: 0 additions & 47 deletions tests/testsuite/fix.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
|
Expand Down

0 comments on commit 0810627

Please sign in to comment.