Fix cargo check --lib not emitting warnings on subsequent runs

[fmdkdd]

cargo check for binaries will always emit warnings, even when the files in the
project haven't changed.

For cargo check --lib, warnings were emitted on the first run, but subsequent
runs would not trigger a recompilation of the library when no files have
changed, hence preventing warnings to appear.

This commit forces a recompilation of the library top-level target (not
library dependencies) when using the check command.

Closes GH-3624.

[rust-highfive]

r? @matklad

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

[fmdkdd]

As stated in #3624, this seems to fix the issue at the cost of a longer turn around. Running cargo check --lib on cargo itself (unmodified) now takes around 6sec on my machine, whereas it took 0sec previously (no recompilation).

It's up to you whether this tradeoff is worth it.

Also, I would very much like a double check on whether the fix as any unintended side-effects, as I am not familiar with cargo internals.

[matklad]

I certainly sympathize with the problem here, but I am afraid that the real fix unfortunately needs to be much more involved. I think the issue of missing warnings affects not only cargo check, but cargo build as well. For example, if inside my workspace, there are two crates, A and B, and B depends on A, then executing cargo build may show errors for B, but not for A, if A is fresh. This certainly has bitten me in practice, when I've committed code which I though was warnings-clean, but in fact had a ton of unfixed warnings.

So ideally we want to record rustc's warnings during dirty compilations and then replay them for fresh ones.

That said, perhaps a hack to always rebuild crates specifically for cargo check would fall under "practicality beats purity"? I am not sure though how this should work in a multi-package workspace scenario though...

[fmdkdd]

@matklad I'm all for making the correct fix, but I will need some guidance on what to target and how to tackle it.

This certainly has bitten me in practice, when I've committed code which I though was warnings-clean, but in fact had a ton of unfixed warnings.

In this situation, would a -D warnings have worked? Or would cargo have skipped the crate A entirely because it was fresh?

So ideally we want to record rustc's warnings during dirty compilations and then replay them for fresh ones.

That sounds like a definitive solution, yes. That way, we wouldn't even have to recompile fresh binary targets.

How exactly would we cache the results? Since it's between runs, we have to save them somewhere. Do we extend the existing metadata files, do we create new files?

At what granularity should we cache these warnings? For each crate (a unit here is associated to a crate, right?)? For each file?

[matklad]

Sorry for the long reply @fmdkdd !

In this situation, would a -D warnings have worked? Or would cargo have skipped the crate A entirely because it was fresh?

Denying warnings would help in a sense that code with warnings would just fail to compile. However there's a workflow (which I personally use) when you allow warnings during development, and check before commit that there are no warnings left.

How exactly would we cache the results? Since it's between runs, we have to save them somewhere. Do we extend the existing metadata files, do we create new files?

I think we can store new "compiler_output.txt" files, because the info here is not essential, and we should not fail if these files are missing. However, I don't have an idea how these files should look like, especially if we take into account colored output, which, on windows, is achieved via syscalls and not via escape codes.

At what granularity should we cache these warnings? For each crate (a unit here is associated to a create, right?)? For each file?

unit (crate) looks like a right granularity here. Per file storage won't work really well because compiler itself works on the crate level granularity.

@alexcrichton do you have any suggestions here? I would love to have this feature, because I want to know that my code is warning free without resorting to #[deny(warnings)]. At the same time, the implementation of this feature likely needs to be quite involved...

[alexcrichton]

Ah yeah I think I do agree with your thoughts @matklad in that the "what to fix here" is a bit deeper, especially in the multiple crate workspace scenario.

It does seem like the best option here is for Cargo to record the compiler's output and replay it back, but this definitely isn't easily done today. The theory is that with JSON messages Cargo should be able to recreate rustc's output but that's unfortunately not currently the case as it would have to reimplement a significant amount of logic.

In the meantime though I'd personally prefer to stick to what future Cargo will continue to do, which is not executing rustc at all on "fresh" crates and simply moving to the next crate. Perhaps though we can push more on the upstream design for json messages to get this implemented?

[matklad]

Perhaps though we can push more on the upstream design for json messages to get this implemented?

Yeah, perhaps we can extract a rustc-diagnostics crate, used by both rustc and cargo, with something like this:

#[Serialize, Deserialize]
struct Diagnostic { ... }

impl Diagnostic {
    fn print<W: Writer>(&self, color: ColorConfig)
}

[matklad]

The theory is that with JSON messages Cargo should be able to recreate rustc's output but that's unfortunately not currently the case as it would have to reimplement a significant amount of logic.

Can't we just intercept rustc output and replay it byte by byte? Implementation wise, this seems like a middle ground in that we need to implement saving and restoring of the output, but we can punt on "produce human output from JSON" thing. This won't work on windows b/c colors there use syscalls, but IIRC the recent versions of windows are happy with ANSI escapes, so perhaps we could make something which "almost work" now, and seamlessly improve this later with JSON thing?

[alexcrichton]

Yeah, perhaps we can extract a rustc-diagnostics crate, used by both rustc and cargo

Sounds great!

Can't we just intercept rustc output and replay it byte by byte? Implementation wise

This is plausible, yes, but doesn't work on Windows yeah due to how colors work today. That's the only bug that needs to get fixed to make it work though! (AFAIK)

[ehuss]

Can't we just intercept rustc output and replay it byte by byte?

I would prefer it to not do that. It needs to honor the --message-format flag (my tooling needs JSON, and would be very surprised if it spit out text if the user ran cargo check manually). There are also some flags like --color that affect output that might be confusing if they stopped working as expected because the output is cached.

[alexcrichton]

@ehuss my thinking is that Cargo understands the workflow at a high level, so it'd be able to react and "do the right thing" in that sort of situation. In that if you run cargo check manually you won't get JSON output and Cargo would also respect --color as well as auto-detection of what color mode is in use.

[ehuss]

it'd be able to react and "do the right thing"

How would that work? Since those flags cause rustc to be called with different flags, would the cached output need to keep track of which flags were used and re-run rustc if they are different? Or use the rustc-diagnostics crate idea? Or something else?

[alexcrichton]

Heh well we haven't implemented it so I'm not sure! Regardless sounds like a bug to avoid.

[fmdkdd]

So, the current plan would be for cargo to record rustc output in a machine-friendly format, then replay it in case the crates are fresh. The output can be serialized by cargo in whatever format we see fit. We might not be able to use the JSON format rustc already emits directly, as we may need to save extra information (e.g., colored output, or other flags used during the cargo check call to ensure we give the right answer).

Since cargo would then have to be able to output the diagnostics in the human-readable or JSON format (or whatever comes in the future), the actual formatting of diagnostics would then need to be moved to a separate rustc-diagnostic crate, used by both cargo and rustc.

Is this functionality something we want to see implemented soon? (As in, should I start working on a proof of concept?) Or are there any other tasks that must be tackled before fixing this becomes a reality? Maybe extracting printing of diagnostics to a rustc-diagnostic crate first?

[alexcrichton]

@fmdkdd that's the basic thinking I believe so, yeah. Unfortunately much of this doesn't exist (e.g. rustc won't tell cargo about colors in "machine readable" mode) so the timeline is probably not in the very near future. Moving to extract a crate though would be a great start!

[alexcrichton]

I'm going to close this for now due to inactivity, but we can of course reconsider with diagnostics captured and such!