call clippy-driver directly rather than as a rustc_wrapper

[yaahc]

As part of our ongoing effort in rust-lang/rust-clippy#3837 to move clippy up into cargo so we can integrate it with fix and maybe other features that @Manishearth can add we're thinking the next step is to tweek clippy-preview so that it calls clippy-driver directly rather than treating it like a rustc wrapper.

When we looked into making it so that clippy-driver can be called as rustc instead of as a rustc wrapper, we realized it really doesn't do anything when its being a wrapper other than remove the rustc argument from the args list. So we should already be good to go on that front.

I'd like to implement this feature myself but manish and I don't know cargo well enough to know the proper way to go about it and manish recommended I ask @alexcrichton and @killercup for ideas on how to go about doing it.

Some additional questions from manish

• can this be done easily?
• would this help fix integration? if not, what would?
• can we make this work such that build scripts still get the original RUSTC ?

rust-lang/rust-clippy#2087 is relevant to that last question

cc @ehuss

Edit: tracking the checklist in original post rather than below

• implement cargo fix --clippy over cargo clippy-preview --fix (gonna gate the --clippy arg behind the unstable flag in the same way clippy-preview is gated)
• implement wrapper for primary crate only logic unnecessary
  • support using the wrapper for all local workspace crates when using the --all flag unnecessary
• implement clippy argument passthrough for cargo fix
• have clippy override the fingerprint to force a run when using cargo fix unnecessary?
• add a test for cargo fixing a clippy lint, @Manishearth what is a good stable clippy lint to use?
• move clippy is available test from cache_message.rs to somewhere more clippy centric, along with above new test.

[Manishearth]

cc @killercup @alexcrichton

[yaahc]

Bumping this for the notification for alex

[alexcrichton]

Thanks for the ping @yaahallo! At this point I think the next steps are basically to just go ahead and implement this in Cargo under a new name, get some tests running, and go from there. (making sure it's all unstable so we can test it out before fully switching over)

Do you need some help about doing this in Cargo? If so, we can always try to help out!

[yaahc]

@alexcrichton I think I can handle getting started and then rely on code review for course correction :)

[Manishearth]

Oh, here's how we'd like it to work:

• It should still respect RUSTC_WRAPPER for the non-toplevel crate
• It should have a way to work with cargo fix (cargo clippy --fix? cargo fix --clippy?)
• It should build the current crate, but also cargo clippy --all should work like cargo fix --all

These don't have to be fixed here, but we should design it such that these can be made to work.

@alexcrichton got any tips for how best to design this? I think an additional flag in the check compile mode that gets passed around everywhere would be nice, i.e. we should not make this an internal rustc wrapper.

[yaahc]

RE: bullet point 2

is there any reason not to support both cargo clippy --fix and cargo fix --clippy?

[alexcrichton]

I don't think I have a strong enough grasp on what needs to be done here to say how to design it best, but I'm more than happy to read over PRs and offer guidance! I suspect that something cargo-fix like is appropriate here, though.

I'd probably start with cargo clippy --fix or something like that, and we can tack on more options. I wouldn't add auto-fixing in the first iteration though.

[yaahc]

rust-lang/rustfix#130 just gonna link this here since it seems related

[yaahc]

@killercup should I just dissect how all the json format stuff works and essentially bake the minimum necessary logic from https://github.com/rust-lang-nursery/rustfix/blob/2239a2386d72a6b2db6d904e0b1b5cc6e5525533/examples/fix-json.rs into cargo?

My current plan is to start with cargo clippy --fix essentially and work backwards from there to figure out what level of integration is required for clippy in cargo.

[ehuss]

I think going with cargo fix --clippy might be a better option. I think it follows better the action of "fixing", and cargo fix has a bunch of flags that are relevant (--allow-dirty, etc.) that would not be possible to use from cargo clippy.

There's also the question of the union of flags from fix and clippy. Since clippy has relatively fewer flags, I think they could be passed as arguments to --clippy. For example:

cargo fix --clippy="-W clippy::pedantic"

As for implementation, I think it should be a relatively simple thing. The cargo fix command can set some environment variable when --clippy is specified (maybe just RUSTC_WRAPPER, or something special like __CARGO_FIX_CLIPPY, the latter might be easier and safer?), and inside rustfix_and_fix it would just use that process (clippy-driver) instead of rustc. I tested this locally and it seems to work with just a few lines of code.

[Manishearth]

I would rather move away from using a wrapper since it interacts badly with other wrappers (e.g. sccache). I feel like an internal flag that makes a check compilation mode clippylike is what we'll end up needing for that, so cargo clippy is internally "clippylike cargo check", and cargo fix --clippy is "clippylike cargo fix".

To be clear, I don't think we should worry about UI just yet, the reason I suggest cargo clippy -- fix is that cargo clippy-preview is unstable right now so we can iterate on it. The main point here is to get the plumbing right, we can pick the right way to expose it later.

[ehuss]

My point is that supporting clippy in cargo fix is independent of that plumbing. cargo fix manually chooses which rustc to run. Instead of running rustc it can just choose to run clippy-driver.

What is the use case for clippy and sccache? Doesn't clippy inherently want to force a run, and sccache caching isn't desired?

Here are some things I'd like to see addressed:

• Only have clippy run for "primary" crates.
• Clippy should "force" a run, overridding the fingerprint.
• Avoid sharing the rmeta caches with cargo check (for "primary" crates only), dependencies should be shared.

The only way I can think of for now to start on this is to have BuildConfig have an additional field to indicate a "primary crate wrapper". I didn't follow #6759 super closely, but I think it originally had some kind of multiple-wrapper support, but it was dropped? I'd like to dig into that more. Are there any other options?

For forcing, I think clippy can just set BuildConfig::force_rebuild. I'd also like to look at changing force_rebuild to only run for primary crates. I don't know why it runs for all crates today. I think cargo fix is the only thing that uses it, and it just causes it to needlessly rebuild all dependencies. Can that be changed? EDIT: I was mistaken, it already only runs for primary crates.

For the third point, we'll need to add something to the fingerprint when clippy is running. I was originally considering adding rustc_wrapper to the fingerprint, but that might have some unexpected consequences. Clippy and sccache are some of the main users of RUSTC_WRAPPER that I know of. I think sccache will be ok, although it might cause recompiles if you enable/disable it, that might be a good thing? The old cargo clippy would no longer share the cache with cargo check, which is bad for dependencies, but good for the primary crates. If we put that fingerprint addition behind an unstable flag, we could maybe test it out and see what problems there are?

[Manishearth]

Instead of running rustc it can just choose to run clippy-driver.

That's the issue though! You only want to run clippy-driver on the last crate (or, in the case of --all, the local workspace crates). You want rustc elsewhere.

This is not something wrapper functionality exposes at the moment. What clippy currently does is basically guess if it needs to be run on a given crate, and shell out to rustc otherwise. This has bugs.

What is the use case for clippy and sccache? Doesn't clippy inherently want to force a run, and sccache caching isn't desired?

Again, dependencies. You want sccache for deps and clippy for the final check.

Here are some things I'd like to see addressed:

+1

The only way I can think of for now to start on this is to have BuildConfig have an additional field to indicate a "primary crate wrapper".

Honestly I think it won't be productive to generalize this to a new kind of wrapper, we should just start with a flag that controls if it's a clippy run, and if so, inserts clippy-driver calls.

But if you feel this is the better way, we can do it that way.

[ehuss]

That's the issue though! You only want to run clippy-driver on the last crate (or, in the case of --all, the local workspace crates). You want rustc elsewhere.

fix already makes this distinction. For fix, the only change is this line needs to run "clippy-driver" instead. If we add a "primary unit only wrapper", then fix could use that instead, and for the "fix clippy" case, I think it should be able to just pass clippy-driver instead of rustc as the first argument.

If you mean for the cargo clippy case, I think I outlined that above (which I think fix should use the same logic), though feel free (anyone) to ask questions if you need more clarification.

Again, dependencies. You want sccache for deps and clippy for the final check.

Ah, yea, if the "wrapper for primary only" is implemented, this should just work.

Honestly I think it won't be productive to generalize this to a new kind of wrapper, we should just start with a flag that controls if it's a clippy run, and if so, inserts clippy-driver calls.

I think it will be fine to be general. cargo fix can use the same logic, so this can be removed.

(Note: I edited my comment above, I was mistaken on how force works. It already does the right thing.)

[Manishearth]

SGTM, you know your own codebase better 😄

[yaahc]

I didn't follow #6759 super closely, but I think it originally had some kind of multiple-wrapper support, but it was dropped?

That is correct, I toyed with having clippy wrappers stored in a vec rather than just as a single item, but we didn't want to derail that PR with figuring out what that would entail so we decided to shelve the idea.

[yaahc]

Also, I'm going to outline what I plan on doing based on the conversation over the weekend, please correct anything that I get wrong.

• implement cargo fix --clippy over cargo clippy-preview --fix (gonna gate the --clippy arg behind the unstable flag in the same way clippy-preview is gated)
• implement wrapper for primary crate only logic
  • support using the wrapper for all local workspace crates when using the --all flag
• implement clippy argument passthrough for cargo fix
• have clippy override the fingerprint to force a run when using cargo fix

Avoid sharing the rmeta caches with cargo check (for "primary" crates only), dependencies should be shared.

I dont know what the rmeta cache is yet so I'll need some more info to understand this and what needs to be done here.

As a bonus I might spend some time testing the interaction between sccache and all of the above changes to make sure they get along nicely.

[ehuss]

I dont know what the rmeta cache is yet so I'll need some more info to understand this and what needs to be done here.

cargo check emits *.rmeta files as the result of compilation. This, along with the fingerprint file, indicates if something should be rebuilt. If you set the "force" flag for clippy, this won't be too important right now. However, the message caching feature (which is currently unstable) will interact poorly if you run "cargo clippy" and then run "cargo check". We'll probably want the path to the wrapper in the Metadata hash, which is the -0bbaae6bc018431f hash on the end of every file/directory so that clippy and check are kept separate. We may also want to include the timestamp of the wrapper in the fingerprint so if it is ever update cargo will rebuild.

That's a lower priority right now, just something I'd like to see done to get message caching working better. We'll also need to determine if it is correct/safe to cache clippy output (in which case we can skip the whole "force" thing once message caching is stabilized). I'm not 100% this will all work, but I think it should. This may have subtle effects with other use cases, so it will need some testing.