Stabilize fix --clippy

[yaahc]

follows #7382

[rust-highfive]

r? @nrc

(rust_highfive has picked a reviewer for you, use r? to override)

[yaahc]

Possibly blocking on rust-lang/rust-clippy#3630

[Manishearth]

cc @oli-obk @phansch @flip1995 What do y'all think we should do to get to a stage where we can stabilize this? An easy answer is to remove MachineApplicable from most lints in rust-lang/rust-clippy#3630 , which I don't like but I don't hate it either.

I'm going to slowly work my way through the list.

[oli-obk]

CI is only failing due to an unused import in tests/testsuite/fix.rs

What do y'all think we should do to get to a stage where we can stabilize this?

Yea, having MachineApplicable only on lints we know to be good seems like the right choice.

[phansch]

What do y'all think we should do to get to a stage where we can stabilize this?

Yeah, I also think that would be the best solution in the short term. I'd very much like to see cargo fix --clippy being a pleasant experience from the start. I think even if we only start with a handful of selected lints as MachineApplicable that would be much better than incorrect suggestions.

[Manishearth]

Okay, how about we punt on this until after the next rust release (sept 26)? That way this doesn't make it into beta in a broken state.

By then, hopefully I'll have that list burned down quite a bit, and we can mark any stragglers as inapplicable.

[Manishearth]

(Beta is cut early so we may be able to merge before sept 26)

[Manishearth]

rust-lang/rust-clippy#3630 is now closed, so this is unblocked from the clippy side.

r? @ehuss

[alexcrichton]

Ah if this is ready to be stabilized, perhaps it's best included in #7382?

[Manishearth]

I'm okay with either, both of these changes are theoretically independent release-wise, they just share a ton of code (and releasing one will require small tweaks to the PR for the other). It might be worth releasing this separately.

[bors]

☔ The latest upstream changes (presumably #7450) made this pull request unmergeable. Please resolve the merge conflicts.

[Manishearth]

@ehuss should we try and get this stabilized first? There may be fewer UX requirements and these actually seem pretty independent.

[ehuss]

The Cargo team discussed this, and we'd like to propose the following alternative, considering some of the concerns in #7382.

A change would be made to Cargo to allow cargo fix to use an alternate rustc. I'm not sure, but I believe the fix would be small (updating this line to call something like get_tool("rustc") instead). I haven't deeply considered the implications, but seems plausible.

The cargo-clippy plugin could then add a --fix flag. In this mode, cargo-clippy would set the RUSTC environment variable to itself and call cargo fix instead of check.

EDIT: This should probably be RUSTC_WRAPPER, and the code will need to properly handle that.

Due to the recent fixes @yaahc implemented, I think this should work, and clippy will only run for the "primary" crates. In my quick tests, it seems to work.

How does this plan sound to everyone?

[Manishearth]

This plan sounds fine provided:

• it indeed only runs for the primary crates
• it doesn't clobber metadata

[Manishearth]

presumably by RUSTC you meant PRIMARY_RUSTC. that works.

[ehuss]

presumably by RUSTC you meant PRIMARY_RUSTC. that works.

Nah, it would just be RUSTC. The fix code already is restricted to the primary units here.

[Manishearth]

@ehuss won't that affect the normal metadata builds of previous crates, though? I guess I should poke at the impl, if you say it won't break things that's fine by me 😄

[ehuss]

In @yaahc's work, it was changed so that cargo fix does a normal "check" for all dependencies, and then only engages the "cargo as rustc wrapper" for the primary crates. That's what the linked line does, and I think it should be ok.

[jyn514]

The cargo-clippy plugin could then add a --fix flag. In this mode, cargo-clippy would set the RUSTC environment variable to itself and call cargo fix instead of check.

This seems rather unfortunate - it means the interface for fix is different for clippy than for everything else. Can you expand a little more on why the cargo team chose to have clippy implement it that way?

[ehuss]

Sorry for the late response on the reasoning: Cargo doesn't know that clippy exists, or how to run it. We have somewhat intentionally kept these separate since it is a separate project.

[jyn514]

What makes clippy different from rustfmt here?

[ehuss]

I'm not sure I understand the question. cargo fmt is also a separate tool, independent of cargo.

[jyn514]

Ok then, what makes clippy different than rustdoc, which has its own builtin cargo doc subcommand?

[ehuss]

Because for rustdoc to work, cargo needs intimate knowledge of how to integrate with it (there are multiple dependency edges within a package, different fingerprinting requirements, special rules specified in Cargo.toml, etc.). Clippy does not, since its interface is identical to rustc, and does everything rustc does.