cargo-check reissues warnings between builds for `bin` crates, but not for `lib`

[crumblingstatue]

$ cargo new --bin mybin && cd mybin && sed -i '1ifn foo(){}' src/main.rs && cargo check && cargo check:

    Created binary (application) `mybin` project
   Compiling mybin v0.1.0 (file:///tmp/mybin)
warning: function is never used: `foo`, #[warn(dead_code)] on by default
 --> src/main.rs:1:1
  |
1 | fn foo(){}
  | ^^^^^^^^^^

    Finished dev [unoptimized + debuginfo] target(s) in 0.47 secs
   Compiling mybin v0.1.0 (file:///tmp/mybin)
warning: function is never used: `foo`, #[warn(dead_code)] on by default
 --> src/main.rs:1:1
  |
1 | fn foo(){}
  | ^^^^^^^^^^

    Finished dev [unoptimized + debuginfo] target(s) in 0.50 secs

$ cargo new --lib mylib && cd mylib && sed -i '1ifn foo(){}' src/lib.rs && cargo check && cargo check:

     Created library `mylib` project
   Compiling mylib v0.1.0 (file:///tmp/mylib)
warning: function is never used: `foo`, #[warn(dead_code)] on by default
 --> src/lib.rs:1:1
  |
1 | fn foo(){}
  | ^^^^^^^^^^

    Finished dev [unoptimized + debuginfo] target(s) in 0.20 secs
    Finished dev [unoptimized + debuginfo] target(s) in 0.0 secs

Which one is the intended behavior for cargo-check? I personally believe it is useful to reissue warnings. The user might accidentally close or clear the terminal, and might want to get the warnings back without modifying the code.

[alexcrichton]

This former behavior is what I'd expect because the latter shows that the output is cached, which is definitely isn't!

cc @nrc, I think we may be caching something and not rerunning perhaps?

[JelteF]

Any updates on this? I was hoping that the just released to stable cargo check would fix the issue I was having.

[fmdkdd]

cargo check still exhibits this issue with 1.19. In combination with -Zno-trans being a hard error, that means editor tools that rely on calling cargo check for displaying errors cannot reliably get warnings on subsequent runs unless they cache the diagnostics themselves.

I can try to make a PR, but any pointers as to where to look would be appreciated.

[alexcrichton]

@fmdkdd I'm not precisely sure where this would be located but it's pretty likely to be somewhere around src/cargo/ops/cargo_rustc/*.rs most likely

[ehuss]

I've taken a quick look at this. I don't know much about Cargo internals, but here's what I've found so far. cargo_rustc::compile thinks the bin target is dirty because the target filename is missing. There is special case code in Context::target_filenames when doing cargo check that sets the filename to lib{}.rmeta, but this snippet of code does not make much sense to me. @fmdkdd do you want to work together on this?

[nrc]

There is special case code in Context::target_filenames when doing cargo check that sets the filename to lib{}.rmeta, but this snippet of code does not make much sense to me

This is because when running cargo check we are emitting metadata files rather than object files and so we use the rmeta extension.

[ehuss]

The bin target does not seem to emit an rmeta file. The reason I was confused is because that code seems to assume we are creating an rmeta file for a library regardless what the target is.

Regardless, a valid fingerprint is generated for lib targets which prevents it from being re-checked. Perhaps the freshness should be forced to dirty if profile.check is true?

[fmdkdd]

@fmdkdd do you want to work together on this?

Sure, any help is appreciated, as I'm not familiar with cargo internals either.

Regardless, a valid fingerprint is generated for lib targets which prevents it from being re-checked. Perhaps the freshness should be forced to dirty if profile.check is true?

The freshness of the lib target? Seems like a decent approach. I'll try that.

[ehuss]

The freshness of the lib target? Seems like a decent approach. I'll try that.

I assume when a user runs cargo check it should not re-check dependencies assuming the rmeta file is up-to-date for the dependency. I'm thinking adding something like the following to cargo_rustc::compile:

if unit.profile.check && unit.pkg.package_id() == cx.ws.current_opt().unwrap().package_id() {
     freshness = Freshness::Dirty;
}

I haven't thought through what the implications are inside a workspace with multiple packages, and obviously the unwrap should probably be expect.

[fmdkdd]

Great! I was about to post that just testing this:

@@ -250,6 +251,10 @@ fn compile<'a, 'cfg: 'a>(cx: &mut Context<'a, 'cfg>,
             freshness = Freshness::Dirty;
         }

+        if unit.profile.check && unit.target.is_lib() {
+            freshness = Freshness::Dirty;
+        }
+
         (dirty, fresh, freshness)
     };
     jobs.enqueue(cx, unit, Job::new(dirty, fresh), freshness)?;

was probably not enough, as it recompiled all libraries, not just the crate one.

With your suggestion, the warnings are available on subsequent runs.

The downside is performance: testing with cargo itself, it takes around 6sec to cargo check --lib on an unmodified project, whereas it took 0sec previously. I guess that cannot be helped, since we need to do more work to produce the warnings.

I haven't thought through what the implications are inside a workspace with multiple packages

Me neither.

and obviously the unwrap should probably be expect

I have no idea what a virtual manifest means in this context, but if it has no real library associated, we probably don't need to override the freshness if current_opt is None?

[ehuss]

The downside is performance

Yea. Someone else opened #2494 as a feature request to cache the messages, but I think that would be a significant project for a limited use case.

I have no idea what a virtual manifest means in this context, but if it has no real library associated, we probably don't need to override the freshness if current_opt is None?

I believe in this situation, the top-level manifest in a workspace (using the [workspace] feature in Cargo.toml) is called a virtual manifest. cargo check forbids running in the top-level of a workspace, so I do not think it is possible for current_opt to return None.

[fmdkdd]

I do not think it is possible for current_opt to return None.

I confirm that cargo check won't take it. Can't we use current()? then instead of current_opt().expect(msg)?

[ehuss]

I confirm that cargo check won't take it. Can't we use current()? then instead of current_opt().expect(msg)?

I think that should be fine.

[apiraino]

My IDE autoruns cargo check on file save, so I don't see warnings if I immediately run it from the shell (to expand on errors decription).

I need to alias cargo check and trick its metadata:
alias cargo_check='touch -cm src/main.rs src/lib.rs && cargo check'

[bossmc]

Since the output of cargo-check is a .rmeta file, could that be used to store any warnings from the build (or even a "there were warnings" flag)? That way check (and clippy), which are run for their "side-effects" rather than for their outputs, can efficiently be re-run without losing their purpose?

[bossmc]

I'm not sure what the format of an .rmeta file is (according to file /path/to/file.rmeta they are either empty or "data") or if they're used elsewhere (I think they might be included in .rlib archives?).

[scottjmaddox]

Are there any plans to fix this? It's still pretty annoying.

[Rua]

I get the same behaviour for both cases: the warning is issued the first time, but not repeated the second time. It would be really useful if the warnings were repeated.

[apiraino]

@ehuss is this issue also related/dupe of https://github.com/rust-lang/cargo/issues/6986?

[ehuss]

Yep, closing in favor of the tracking issue.