Cargo caches failed cargo clippy -- --bad-arg

[Alexendoo]

Problem

From rust-lang/rust-clippy#12623 (comment):

cargo new --lib demo && cd demo

cargo check
cargo clippy -- -x
cargo clippy

• The first clippy/check creates the target dir including target/.rustc_info.json, it seems failures aren't recorded in .rustc_info.json if it doesn't exist yet
• In the second command cargo records the result of the target info acquisition under a different hash
• The third command uses the same hash to retrieve the previously failed result

The issue being CLIPPY_ARGS is not taken into account when creating the hash

Output:

    Creating library `demo` package
note: see more `Cargo.toml` keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
    Checking demo v0.1.0 (/.../demo)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.14s
error: process didn't exit successfully: `/.../rustc -vV` (exit status: 1)
--- stderr
error: Unrecognized option: 'x'


error: process didn't exit successfully: `/.../clippy-driver /.../rustc -vV` (exit status: 1)
--- stderr
error: Unrecognized option: 'x'

cargo clippy -- -x is expected to fail but the following cargo clippy is not

In normal operation clippy-driver gets far enough along to register CLIPPY_ARGS into the env depinfo but that doesn't happen here

Steps

No response

Possible Solution(s)

CLIPPY_ARGS could be added to the fingerprint as a special case, but the issue could also exist for other wrappers

Notes

No response

Version

No response

[epage]

Note that cargo has no knowledge of CLIPPY_ARGS, one way or the other, to be able to fingerprint it.

[Alexendoo]

Would not caching failed invocations in .rustc_info.json be an option?

[epage]

The command being invoked is

$ /home/epage/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/bin/clippy-driver /home/epage/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/bin/rustc -vV

So CLIPPY_ARGS is being passed through to this which is causing it to fail.

Trying 1.75, this wasn't a problem. Looks like this might have been broken by #13659

CC @RalfJung

[epage]

@RalfJung , I'm not seeing a problem statement in #10885. Any further details on that?

[epage]

Would not caching failed invocations in .rustc_info.json be an option?

Caching of failures was intentionally added in #9112 (cc5e9df)

This commit updates the rustc info cache to cache failures to execute
rustc as well as successes. This fixes a weird issue where if you're
probing for flags the rustc_info_cache test fails on channels which
don't have the flag since previously a failure to execute rustc resulted
in never caching the result.

The problem in that case isn't too clear to me and would need further investigation to go down that route.

[RalfJung]

@RalfJung , I'm not seeing a problem statement in #10885. Any further details on that?

That issue explains the motivation, not sure what to add. Also, what good is a "wrapper" that doesn't actually get invoked for all rustc invocation? If the goal of the wrapper is to e.g. swap out which rustc is being called, it is crucial that all invocations are made through the wrapper.

Seems like the clippy wrapper is just blindly adding extra arguments to all invocations. Note that rustc -vV is not the only such "query invocation" cargo does, it also does a rustc invocation to print all available targets, and that has always gone through the wrapper. So something must be treating that query different -- either that doesn't get cached for some reason (seems unlikely) or clippy is already aware of it and avoids adding potentially faulty arguments, in which case that logic needs to be extended to recognize other "queries" as well.

[epage]

That issue explains the motivation, not sure what to add.

From my looking it came down to

This is causing us a bunch of pain right now since we need to figure out how to best intercept this -vV call.

without an explanation of why that call needs to be intercepted

Seems like the clippy wrapper is just blindly adding extra arguments to all invocations. Note that rustc -vV is not the only such "query invocation" cargo does, it also does a rustc invocation to print all available targets, and that has always gone through the wrapper. So something must be treating that query different -- either that doesn't get cached for some reason (seems unlikely) or clippy is already aware of it and avoids adding potentially faulty arguments, in which case that logic needs to be extended to recognize other "queries" as well.

This would be a good next step to investigate when considering all possible directions for resolving this.

[RalfJung]

Please also read the rest of the issue. :)

The context is that Miri and bootstrap use RUSTC_WRAPPER to completely replace the rustc being invoked; there is no a priori relationship between $RUSTC_WRAPPER rustc and the rustc in the path. Why they need to do that is not super relevant, it's exactly the kind of usecase that RUSTC_WRAPPER is designed for and they have done this for many years. Because $RUSTC_WRAPPER rustc invokes a completely different binary, the result cargo gets by invoking rustc -vV without the wrapper is obviously bogus since it is talking to the wrong compiler.

[Alexendoo]

It was an issue in 1.75 also

export RUSTUP_TOOLCHAIN=1.75
cargo new --lib demo && cd demo

cargo check
cargo clippy -- -x
cargo clippy

error: failed to run `rustc` to learn about target-specific information

Caused by:
  process didn't exit successfully: `...\clippy-driver.exe '...\rustc.exe' 
- --crate-name ___ --print=file-names --crate-type bin --crate-type rlib --crate-type dylib --crate-type cdylib --crate-type staticlib --crate-type proc-macro --print=sysroot --print=split-debuginfo --print=crate-name --print=cfg` (exit code: 1)
  --- stderr
  error: Unrecognized option: 'x'

error: failed to run `rustc` to learn about target-specific information

Caused by:
  process didn't exit successfully: `...\clippy-driver.exe '...\rustc.exe' 
- --crate-name ___ --print=file-names --crate-type bin --crate-type rlib --crate-type dylib --crate-type cdylib --crate-type staticlib --crate-type proc-macro --print=sysroot --print=split-debuginfo --print=crate-name --print=cfg` (exit code: 1)
  --- stderr
  error: Unrecognized option: 'x'

Skipping some steps if we encounter --print/etc is something I'll try on the clippy driver side

[RalfJung]

Yeah that is exactly the other "query invocation" I mentioned above. #13659 just made it so that both query invocations are fed through the wrapper consistently, but clippy's wrapper behavior in combination with caching was a problem even when just one of the queries is fed through the wrapper.

[RalfJung]

Skipping some steps if we encounter --print/etc is something I'll try on the clippy driver side

FWIW, this is Miri's logic for determining whether the invocation is an "info query" and hence needs to be treated differently:

https://github.com/rust-lang/miri/blob/471dcec041122b1bba8ab4fd8085f88e3bd275ff/cargo-miri/src/phases.rs#L298-L302

[epage]

@RalfJung please assume people have made a good faith effort. In this case, I did look over the issue and another one it links to, multiple times in case I missed something.

Whats still not clear is the end-user side effect of what you are wanting, the end-user either being the RUSTC_WRAPPER author or their end user. So far this is coming across as a purity argument ("its not that tool so its not that version").

Looking over the code, it looks like we primarily use it for MSRV checks and fingerprinting of rustc. The first requires the result to be representative of the compiler / stdlib version being performed. Unsure if there are other factors helping with fingerprinting or if that is ultimately what you are looking for here.

[RalfJung]

@epage sorry for that. I was slightly frustrated since your questions sounded somewhat dismissive. I think there are nicer ways to ask "would you mind explaining which RUSTC_WRAPPER usecase is affected by this"; the undertone of your question that I received was "this looks useless, what are you doing". My reply was still unnecessary.

As described here (but admittedly without going into much detail), the end users I am concerned about are rustc bootstrap and cargo-miri. They both set RUSTC_WRAPPER. rustc bootstrap uses the wrapper to decide which stage's compiler to invoke. Things would obviously go quite wrong if cargo got the wrong rustc version here. To achieve that, bootstrap currently sets both the RUSTC and RUSTC_WRAPPER env vars, but that then requires awkward hacks in the rustc binary that it uses for both of those env vars because the binary has to auto-detect whether it got invoked as $RUSTC or as $RUSTC_WRAPPER $RUSTC. That hack also got linked in the issue I referenced above. cargo-miri has similar hacks. (Sadly Miri will likely have to keep these hacks forever as there are too many build scripts out there that invoke rustc without accounting for the wrapper. But that is no excuse for cargo itself to do this incorrectly, and rustc bootstrap may get away with dropping the hack since it more tightly controls which crates it is run on.)

Anyway I think the discussion is rather moot now since the Clippy issue predates my PR.

[epage]

Yes, with the verification that that change didn't break rustc, it is no longer a blocker for this issue. I'll copy the relevant notes to that issue.

[epage]

This commit updates the rustc info cache to cache failures to execute
rustc as well as successes. This fixes a weird issue where if you're
probing for flags the rustc_info_cache test fails on channels which
don't have the flag since previously a failure to execute rustc resulted
in never caching the result.

Alex was referring to calls like

        let supports_split_debuginfo = rustc
            .cached_output(process.clone().arg("-Csplit-debuginfo=packed"))
            .is_ok();

which we do when we don't wait two releases before calling something.

If we reverted cc5e9df, then the next time we add a probe, it will break the tests for our caching and the mixture of messages from the failure (not cached) and success (cached) cases would reduce the tests insight into what is working. This test change would then need to be reverted if/when we remove the probe.

This wasn't called out but cc5e9df should have also reduced the overhead from the flag probing.

While miri and clippy can workaround this, it is a foot gun for anyone writing a RUSTC_WRAPPER and I'd prefer that we not have tests be negatively impacting the user experience like that. I'm also not sure what we should change the tests to. The most important aspect is that we don't have behavior regressions. Personally, I tend to put a lower priority on testing "is an implicit cache giving us the right performance" because I don't think its usually worth the invasive changes to verify.

[RalfJung]

FWIW I don't think Miri is concerned with working around this. It anyway has to detect these "info queries" as they otherwise throw off our heuristics for whether the current rustc invocation is a "host" or a "target" build (which matters a lot for Miri because all "target" builds are just interpreted, not compiled to machine code).