reimplement arg passthrough for clippy-driver

[yaahc]

Prior to this change, the old cargo subcommand version of cargo clippy
had a feature to pass trailing args down to clippy-driver but when this
the subcommand was reimplemented inside of cargo this feature was left
out accidentally.

This change readds the feature to to capture all args after a trailing
-- and forward them down to clippy-driver via an env variable.

[rust-highfive]

r? @Eh2406

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

[yaahc]

One thing I want to mention is that the way that clippy passes the args down is inconsistent with how we did it for cargo fix --clippy

For cargo fix clippy we take the string associated with the --clippy arg if any is specified --clippy="--deny clippy::all" where as for cargo clippy we take anything after a trailing --, so cargo clippy -- --deny clippy::all.

Also this is implemented differently, clippy expects its args to be passed via a special env variable with a big __CLIPPY_HACKERY__ appended to the end of each arg to help with arg splitting I assume. With cargo fix --clippy we serialize the vec of split args as json and deserialize to preserve word splitting.

I'd like to see the visible portion of how the args are specified become consistent, at the very least. So at a minimum I'd like trailing -- [args]... added to fix which can be used for passthrough to clippy and possibly rustc to configure lints. I believe clippy's approach is the better one in this case.

As for the implementation details I'd love for it to be consistent but clippy doesn't currently bring in serde_json, and so I'm not sure if its worth going through the effort here to make it consistent or not. @Manishearth input is welcome.

[Manishearth]

Can we not use CLIPPY_HACKERY? Part of the goal here is that such hacks aren't needed anymore.

We can instead use another trailing -- on clippy.

Also, it's totally okay if cargo clippy-preview has a different interface than the old cargo clippy. The hope is to improve things.

[yaahc]

I'm stupid, we can definitely not use clippy_hackery, we dont even need to use it here because we already have the processbuilder, no resplitting is required.

[Eh2406]

@bors r? @ehuss

[yaahc]

Right now I've run into an issue with how we implemented primary_unit_rustc because its still actually a wrapper its unnecessarily adding rustc to the args list.

clippyargs ✗ $ jargo clippy-preview --verbose -- --deny clippy::all
    Finished dev [unoptimized + debuginfo] target(s) in 0.08s
    Checking clippyfix-testlib v0.1.0 (/home/jlusby/git/rust/clippyfix-testlib)
     Running `clippy-driver --deny 'clippy::all' rustc --edition=2018 --crate-name clippyfix_testlib src/lib.rs --color always --crate-type lib --emit=dep-info,metadata -C debuginfo=2 -C metadata=1ab0b936bf79082b -C extra-filename=-1ab0b936bf79082b --out-dir /home/jlusby/git/rust/clippyfix-testlib/target/debug/deps -C incremental=/home/jlusby/git/rust/clippyfix-testlib/target/debug/incremental -L dependency=/home/jlusby/git/rust/clippyfix-testlib/target/debug/deps`
error: multiple input filenames provided (first two filenames are `rustc` and `src/lib.rs`)

error: Could not compile `clippyfix-testlib`.

Caused by:
  process didn't exit successfully: `clippy-driver --deny 'clippy::all' rustc --edition=2018 --crate-name clippyfix_testlib src/lib.rs --color always --crate-type lib --emit=dep-info,metadata -C debuginfo=2 -C metadata=1ab0b936bf79082b -C extra-filename=-1ab0b936bf79082b --out-dir /home/jlusby/git/rust/clippyfix-testlib/target/debug/deps -C incremental=/home/jlusby/git/rust/clippyfix-testlib/target/debug/incremental -L dependency=/home/jlusby/git/rust/clippyfix-testlib/target/debug/deps` (exit code: 1)

I'd like to tweek primary_unit_rustc so its wrapped in an enum rather than an Option and the enum variants are wrapper or direct or something along those lines, so that users of the primary_unit_rustc can specify whether or not their process builder should be treated like a wrapper. Or an extra bool argument, I don't really care how its done. But I want the flexibility @ehuss

[yaahc]

Okay, I removed __CLIPPY_HACKERY__ and did the tweeks necessary to have clippy run as rustc instead of as a wrapper so it all works together nicely. I kept the option and added a Struct to contain the process builder and the is_wrapper bool. Using a custom enum seemed like the wrong solution because primary_unit_rustc is optional, so it would still need to be wrapped in an option, and so the enum would just be a proxy for a boolean that you're forced to check, which while nice doesn't seem necessary because this is only ever read from in one very simple location.

I think the stuff to fix the -- [args]... for fix should be done as an independent commit so all this needs is a test and code review + pass CI.

I kinda want to hold off on writing that test until #7157 is merged due to expected merge conflicts.

[bors]

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

[yaahc]

right on schedule bors, ty bby

[ehuss]

Thanks!

Instead of introducing a new enum, I think we can just make the rustc argument explicit for fix. That is:

• Call something like this in ops::fix:

  let rustc = opts.compile_opts.config.load_global_rustc(Some(ws))?;
  wrapper.arg(&rustc.path);

• Remove the call to map here so it no longer automatically adds the argument.

[yaahc]

Okay I think it's good, rereading the code it looks super clean, TY again @ehuss for always providing excellent guidance.

[ehuss]

Thanks!

@bors r+

[bors]

📌 Commit 42a00c1 has been approved by ehuss

[bors]

⌛ Testing commit 42a00c1 with merge f0075c5...

[bors]

☀️ Test successful - checks-azure
Approved by: ehuss
Pushing f0075c5 to master...