initial working version of cargo fix --clippy

[yaahc]

closes #7006

[rust-highfive]

r? @alexcrichton

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

[yaahc]

The argument pass-through logic doesn't currently work and I dont know why. When I make it output the command that it is running before shelling out to clippy-driver it has the arguments that I added. And if I copy paste that command and run it manually the arg works and it suppresses the warning I was trying to allow. But when it runs via cargo fix it still outputs the warning.

I initially assumed it was something clippy was doing to force allowed lints to output, but trying to pass --help through to clippy-driver likewise fails to do anything.

[yaahc]

Actually I think it is doing stuff, just very silently. When I put poorly formatted arguments into the passthrough list it silently fails and doesnt apply any changes, and instead warns on everything. My guess is that there is a final step after applying all fixes where it doesn't capture json output and instead just lets it output the remaining warnings to the user, and that the args im passing through aren't being applied to that step.

[yaahc]

It seems to be outputting leftover lints at least twice, possibly equal to the number of times it is invoked.

[yaahc]

@ehuss RE: Metadata fingerprint concerns

The metadata already includes a hash of Rustc::verbose_version which resolves to something like

master ✗ $ clippy-driver -vV
rustc 1.35.0 (3c235d560 2019-05-20)
binary: rustc
commit-hash: 3c235d5600393dfe6c36eeed34042efad8d4f26e
commit-date: 2019-05-20
host: x86_64-unknown-linux-gnu
release: 1.35.0
LLVM version: 8.0

master ✗ $ rustc -vV
rustc 1.35.0 (3c235d560 2019-05-20)
binary: rustc
commit-hash: 3c235d5600393dfe6c36eeed34042efad8d4f26e
commit-date: 2019-05-20
host: x86_64-unknown-linux-gnu
release: 1.35.0
LLVM version: 8.0

Is it fair to say this should be sufficient for our needs and that I don't need to add the path and timestamp of the rustc binary to the metadata hash?

If this is sufficient then I think the only thing I need to do is write tests and make sure it passes CI and then get it through review. Woooo!

[yaahc]

Seems like all that's left is resolution on the fingerprint requirement and the clippy-driver error handling requirement and if any changes are needed along with final code review and approval.

Lets hope the current version passes CI :)

[ehuss]

Sorry for the delay in response. I've been a bit busy lately, and I'll be gone most of this week. I'll try to check in later, just beware if it takes me a while to respond.

[yaahc]

rustc_wrapper and primary_unit_rustc are doing different things though. In the context of a Rustc object rustc_wrapper is an override for its wrapper member and primary_unit_rustc is an override for its path member.

Clippy could use primary_unit_rustc instead of wrapper because it just discards the first arg if its rustc but rustfix relies on rustc_wrapper to invoke the cargo subprocess. All the work I did was to get the invocation of the subprocess to change from cargo rustc ... for primary crates to cargo clippy-driver ....

[ehuss]

rustc_wrapper and primary_unit_rustc are doing different things though.

Yea, but I don't think it is necessary.

rustfix relies on rustc_wrapper to invoke the cargo subprocess

If primary_unit_rustc is changed to a ProcessBuilder, then fix should be fine. That is, fix would have:

opts.compile_opts.build_config.primary_unit_rustc = Some(wrapper);

This means the dependencies will be built like in a normal build. That's all that is necessary for fix. The "cargo as a wrapper" only needs to happen for primary units.

Making fix work with clippy then becomes a relatively simple change of something like:

    let rustc = if env::var(CLIPPY_FIX_ARGS).is_ok() {
        util::config::clippy_driver()
    } else {
        args.rustc.as_ref().expect("fix wrapper rustc was not set").clone()
    };

instead of encoding clippy-driver into the wrapper.

[yaahc]

So for non primary crates we never go into the subprocess cargo and rely on the fall through path? Didn't know that was allowed and in that context everything you've been saying makes way more sense.

[ehuss]

So for non primary crates we never go into the subprocess cargo and rely on the fall through path?

Yes. I believe the current design is just an artifact of how it evolved (using the existing wrapper machinery).

[yaahc]

Ok I think I did everything correctly but now its failing, my quick checking didnt show anything obviously wrong.

    fix::do_not_fix_non_relevant_deps
    fix::only_warn_for_relevant_crates

I'll try digging into the behavior and tracing where everything is going tonight.

[ehuss]

Thanks!!

@bors r+

[bors]

📌 Commit 22b430d has been approved by ehuss

[bors]

⌛ Testing commit 22b430d with merge 9f7bd62...

[bors]

☀️ Test successful - checks-travis, status-appveyor
Approved by: ehuss
Pushing 9f7bd62 to master...

[nhynes]

Would it be possible to add back CARGO_PRIMARY_PACKAGE in some form so that non-clippy rustc wrappers can get this useful information? I love the tooling, but imho, the rust-lang[-nursery] ones occupy a far too blessed position in the ecosystem.

[Manishearth]

I think a blessed position is good, these are official tools and totally could be just maintained as a single unit if we wanted to. Creating good APIs for this requires a lot of consensus building, for an area that doesn't have many users. Y'all are free to RFC for more wrapper functionality, but here we're blocked on getting important cargo fix/cargo clippy functionality out

[nhynes]

I think a blessed position is good

Sorry if my initial frustration leaked out; I really do appreciate the few people who are making everything good. I guess I'm just being idealistic when hoping for the apis to be general and extensible :)

I'll make a PR real quick. Thanks!