fix: default to all targets when using --edition and --edition-idioms in cargo fix

[Rustin170506]

What does this PR try to resolve?

Close #13527

As we discussed, cargo fix should use the default target selection with cargo check.

In this PR, I modified cargo fix to no longer use all targets by default. For cargo fix --edition and cargo fix --edition-idioms, it will retain the old behavior and select all targets.

How should we test and review this PR?

Unit tests

Additional information

[rustbot]

r? @epage

rustbot has assigned @epage.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[epage]

As you said, this is suspicious and we should understand why in case there is a bad interaction happening

[Rustin170506]

Yes, I’m still looking into it. It seems that if we don’t select all targets, the foo rustc is called only twice. However, if we select all targets, it will be called three times, I believe.

[Rustin170506]

With all targets: p.cargo("fix --all-targets --allow-no-vcs --broken-code")

[COMPILING] bar v0.1.0 ([ROOT]/foo/bar)
[WARNING] failed to automatically apply fixes suggested by rustc to crate `bar`
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:

thread 'main' panicked at src/main.rs:23:29:
explicit panic
[NOTE] run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Original diagnostics will follow.

[WARNING] variable does not need to be mutable
 --> src/lib.rs:1:20
  |
1 | pub fn foo() { let mut x = 3; let _ = x; }
  |                    ----^
  |                    |
  |                    [HELP] remove this `mut`
  |
  = [NOTE] `#[warn(unused_mut)]` on by default

[WARNING] `bar` (lib) generated 1 warning (run `cargo fix --lib -p bar` to apply 1 suggestion)

thread 'main' panicked at src/main.rs:23:29:
explicit panic
[NOTE] run with `RUST_BACKTRACE=1` environment variable to display a backtrace
[ERROR] could not compile `bar` (lib test)

Caused by:
  process didn't exit successfully: `/Volumes/t7/code/cargo/target/debug/cargo [ROOT]/foo/foo/target/debug/foo --crate-name bar --edition=2015 src/lib.rs --error-format=json --json=diagnostic-rendered-ansi,artifacts,future-incompat --emit=dep-info,metadata -C embed-bitcode=no -C debuginfo=2 -C split-debuginfo=unpacked --test --check-cfg 'cfg(docsrs,test)' --check-cfg 'cfg(feature, values())' -C metadata=022767cbb3148d36 -C extra-filename=-99a4e0453221de9d --out-dir [ROOT]/foo/bar/target/debug/deps -L dependency=[ROOT]/foo/bar/target/debug/deps` ([EXIT_STATUS]: 101)

Without all targets: p.cargo("fix --allow-no-vcs --broken-code")

[COMPILING] bar v0.1.0 ([ROOT]/foo/bar)
[WARNING] failed to automatically apply fixes suggested by rustc to crate `bar`
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:

thread 'main' panicked at src/main.rs:23:29:
explicit panic
[NOTE] run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Original diagnostics will follow.

[WARNING] variable does not need to be mutable
 --> src/lib.rs:1:20
  |
1 | pub fn foo() { let mut x = 3; let _ = x; }
  |                    ----^
  |                    |
  |                    [HELP] remove this `mut`
  |
  = [NOTE] `#[warn(unused_mut)]` on by default

[WARNING] `bar` (lib) generated 1 warning (run `cargo fix --lib -p bar` to apply 1 suggestion)
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s

[Rustin170506]

And if you have two targets, it also works: p.cargo("fix --lib --tests --allow-no-vcs --broken-code")

[COMPILING] bar v0.1.0 ([ROOT]/foo/bar)
[WARNING] failed to automatically apply fixes suggested by rustc to crate `bar`
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:

thread 'main' panicked at src/main.rs:23:29:
explicit panic
[NOTE] run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Original diagnostics will follow.

[WARNING] variable does not need to be mutable
 --> src/lib.rs:1:20
  |
1 | pub fn foo() { let mut x = 3; let _ = x; }
  |                    ----^
  |                    |
  |                    [HELP] remove this `mut`
  |
  = [NOTE] `#[warn(unused_mut)]` on by default

[WARNING] `bar` (lib test) generated 1 warning (run `cargo fix --lib -p bar --tests` to apply 1 suggestion)

thread 'main' panicked at src/main.rs:23:29:
explicit panic
[NOTE] run with `RUST_BACKTRACE=1` environment variable to display a backtrace
[ERROR] could not compile `bar` (lib)

Caused by:
  process didn't exit successfully: `/Volumes/t7/code/cargo/target/debug/cargo [ROOT]/foo/foo/target/debug/foo --crate-name bar --edition=2015 src/lib.rs --error-format=json --json=diagnostic-rendered-ansi,artifacts,future-incompat --crate-type lib --emit=dep-info,metadata -C embed-bitcode=no -C debuginfo=2 -C split-debuginfo=unpacked --check-cfg 'cfg(docsrs,test)' --check-cfg 'cfg(feature, values())' -C metadata=4b0938be085d245b -C extra-filename=-d245623a02104f1e --out-dir [ROOT]/foo/bar/target/debug/deps -L dependency=[ROOT]/foo/bar/target/debug/deps` ([EXIT_STATUS]: 101)

[Rustin170506]

I think it’s related to how many times foo rustc is called in this command. If it’s called only twice for one target, the panic error is caught as a warning. However, if it’s called again, it turns into a compile error and causes a bailout. Then the status becomes 101.
I’ll dig into it further. I’m not sure how we handle the compile error here, but I’ll take a look tomorrow.

[Rustin170506]

How about we keep with_status? I think it’s still useful to verify whether we can catch the error and use the correct status code here when --broken-code is specified.

[ehuss]

I don't really have much of an opinion either way. I could see that the test is primarily concerned about leaving the fixed result in place when the verification fails, and the 101 exit code is just a side-effect of doing an additional (possibly unexpected) call to rustc. But it also seems fine to verify that it keeps the broken output in that case, too.

If you leave --all-targets, I would recommend leaving a comment as to why.

[Rustin170506]

Oh, I was suggesting that we keep .with_status(0) and remove --all-targets. This means we are still verifying that Cargo sets the correct status code and respects the --broken-code flag, treating it as a warning rather than an error.

Sorry for the confusion here. I thought I was using the .with_status(0) version, but I forgot I had reverted that change.

[Rustin170506]

This is what I am suggesting: 44a91af (#15192)

[ehuss]

👍

[epage]

@rcbot fcp merge

We previous discussed this as a team (#13527 (comment)) and agreed that changing the non---edition behavior seemed like the right call. This FCP is to official record that as we are making a behavior change.

Background:

cargo fix was developed with a focus on Editions which including running --all-targets by default so that you migrated your whole project. With RFC #3772 deprecating per-build-target editions, this makes even more sense.

However, it has grown to be a general way of applying fixes suggested by the compiler. In this situation, having cargo check and cargo fix or cargo clippy and cargo clippy --fix diverge in behavior does not make sense.

In considering this, we felt that the non-edition use of cargo fix / cargo --fix is unlikely to be used programmatically in a way negatively impacted by this change, considering it can't fully resolve all lints, it can only work in some situations (due to dirty vcs), and that it can generate broken code.

[Rustin170506]

@rcbot fcp merge

@rfcbot fcp merge

See #15192 (comment)

[rfcbot]

Team member @Rustin170506 has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Eh2406
• @Muscraft
• @Rustin170506
• @arlosi
• @ehuss
• @epage
• @joshtriplett
• @weihanglo

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.