add --fix support to cargo-clippy

[yaahc]

Prior to this we had started work on integrating clippy as a subcommand directly into cargo in the form of cargo clippy-preview and cargo fix --clippy. In the course of that work it was decided that the best approach would be to strictly add the features clippy needed to cargo in order to insert clippy-driver only for workspace crates. This was accomplished by adding a RUSTC_WORKSPACE_WRAPPER env variable to cargo that will override the normal RUSTC_WRAPPER when both are present and the current crate is a workspace crate.

This change adds support to clippy to use this by setting the RUSTC_WORKSPACE_WRAPPER env variable instead RUSTC_WRAPPER and by detecting --fix as an arg and swapping out the check cargo command for fix when it is present.

WIP, here are the current issues that I still need to resolve

• Detect if we're running on nightly rust
  • Set RUSTC_WORKSPACE_WRAPPER on nightly, and RUSTC_WRAPPER on stable
  • Error out on stable when --fix is specified, because stable currently hasn't landed the PR for RUSTC_WORKSPACE_WRAPPER so if we set this it just runs check and silently fails
• Update the help text
  • The current plan is to shell out to cargo check --help and then postprocess the output to mention clippy instead of check where appropriate and to add the extra info about --fix and the -- -A lint options.
• tests?

changelog: add --fix arg to cargo-clippy

[yaahc]

I'm worried about the nightly vs stable checking, and how to make sure that maintains correctness as cargo-clippy lands on the next beta/stable versions, once the current version of cargo is stable we might be able to remove the special case error message for --fix and just let cargo print the error message for us when we set the env variable on stable / without -Zunstable-options

Also I don't think we ever need to check for -Zunstable-options because cargo will handle printing that diagnostic nicely for us when its missing / needed.

[yaahc]

Assuming that we will only ever invoke the nightly version of cargo-clippy when on nightly then this is probably all moot and we can just ignore the version checking except for the need to stick to RUSTC_WRAPPER until cargo stabilizes RUSTC_WORKSPACE_WRAPPER

[flip1995]

About the nightly check: We could ship it without this check, since it will land on stable together with the current cargo. That means, that rustup users shouldn't trip over this change. If Clippy was compiled and installed from source, this might be a problem depending on the cargo version, that is used.

[yaahc]

I think if we merged it without the nightly check it would break clippy on stable if cargo doesn't stabilize RUSTC_WORKSPACE_WRAPPER before the next stable cutoff because we would try to do RUSTC_WORKSPACE_WRAPPER=clippy-driver cargo check ... which would still be invalid on anything other than nightly + unstable-options

[flip1995]

Ah RUSTC_WORKSPACE_WRAPPER isn't stabilized in cargo. Sorry I missed that. Yeah in that case, we'll need that check.

[flip1995]

I don't know, if or how we can check if nightly cargo is used, so I guess we'll have to wait, until RUSTC_WORKSPACE_WRAPPER gets stabilized (?).

[yaahc]

I'll ping @ehuss to see if he has any ideas.

[matthiaskrgr]

I don't know, if or how we can check if nightly cargo is used

Perhaps the rustc_version crate could be helpful here?

[ehuss]

The intent was to have this as an unstable feature in clippy for a short while, and then stabilize it on both sides at some point. So you would specify cargo +nightly clippy -Zunstable-options --fix.

I was initially thinking there might be some way for cargo-clippy to detect if it is on nightly, but thinking about it more, it may not be necessary? Perhaps it could work by just checking if -Zunstable-options is in args, and if it is, enable the use of RUSTC_WORKSPACE_WRAPPER. It would pass -Zunstable-options to cargo which would handle checking if it is actually on nightly.

How does that sound?

[yaahc]

Sounds like a perfect solution

[yaahc]

Okay so this works now, wondering how much we care about getting the tests / help text change finished before merging this, given that its behind unstable options I'm thinking I should try to add one test to ensure that when we enable unstable options we call cargo with RUSTC_WORKSPACE_WRAPPER and then leave the testing fix integration and updating the help text to a follow up PR.

[flip1995]

I agree. A test for this would be good. The help can be adapted later.

[shepmaster]

I merged this branch with master and figured out how to compile my own Clippy, all because this type of thing is really valuable for revitalizing old projects (some of mine were started before Rust 1.0!). Looking forward to this being generally available.

[flip1995]

We could merge this and revisit testing this later. I'll take a final look at this later and then add it to the S-waiting-on-rustup list.

[yaahc]

Also I know that the formatting is wrong but nightly rustfmt isnt available on the current nightly which is the only one that builds clippy soooooooooooooo /shrug

[flip1995]

@phansch can you re-review this please, I still have 5 PRs in my review queue.

[phansch]

Minus the formatting I just have a small nit. Apart from that everything looks good to me =)

[flip1995]

Dogfood:

[flip1995]

@bors r=phansch,flip1995

Thanks!

[bors]

📌 Commit 5cfb9ec has been approved by phansch,flip1995

[bors]

⌛ Testing commit 5cfb9ec with merge 1765c5d...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: phansch,flip1995
Pushing 1765c5d to master...