Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use `cargo check` for rust-cargo checker #1289

Merged
merged 1 commit into from Feb 4, 2018

Conversation

@fmdkdd
Copy link
Member

commented Jul 21, 2017

Now that -Z no-trans is not an option anymore in Rust 1.19, I had to disable the flag.

Unfortunately, that means longer wait time before getting error feedback in Flycheck, and also some unwanted side effects of actually compiling binaries.

Cargo has provided the cargo check command for a while now, but I've held onto using it as our main solution because I kept running into issues with it (flycheck/flycheck-rust/issues/51)... but now we have little choice.

Here is a first implementation of cargo check that I made for flycheck/flycheck-rust/issues/55. I'm using a combination of cargo check and cargo test --no-run for checking test code as well.

It has the following advantages:

  • cargo check is the official way to get errors for a project, aside from the still-in-progress RLS.
  • It supports arcane project setups that -Zno-trans did not.

However, there is one major hurdle right now: cargo check and cargo test --no-run report warnings only on the first compilation. Subsequent calls do not report warnings (errors are always reported, because the compilation fails). You would think that's actually not an issue, since we only call cargo check on file save, the file is touched, so cargo check will always compile and give the warnings. But when testing, sometimes I see warnings, sometimes I don't. This is unacceptable, and never happened with -Zno-trans.

It also makes writing tests more cumbersome (I've worked around the lack of warnings by cargo cleaning the repo before each test).

Note that, without -Zno-trans, the current checkers (both rust-cargo and rust) exhibit the same behavior: warnings will only be emitted on the first successful compilation, and Flycheck will sometimes display them, but most likely will just flicker. So at least, cargo check is not worst than what we have right now.

Also, we cannot pass arguments to rustc anymore. The variable flycheck-cargo-rustc-args is of no use anymore, and flycheck-rustc-args will only be used by the plain rust checker. That might break some setups.

TL;DR: For Rust 1.19 and up cargo check is the better solution, but overall it's worst than what we had since 1.15, unless we find a solution to the disappearing warnings.

@fmdkdd

This comment has been minimized.

Copy link
Member Author

commented Jul 21, 2017

Update: the issue of disappearing warning is already reported: rust-lang/cargo/issues/3624.

@jjwest

This comment has been minimized.

Copy link

commented Oct 7, 2017

Is there any update on the current status of this PR?

@fmdkdd

This comment has been minimized.

Copy link
Member Author

commented Oct 7, 2017

@jjwest The upstream issue is not going to be resolved any time soon. I don't think we want to workaround it and cache the warnings ourselves in Flycheck. So, the situation has not evolved much, I'm afraid.

@fmdkdd

This comment has been minimized.

Copy link
Member Author

commented Jan 15, 2018

I've updated this PR. We can merge it after #1381 (as it's based on it).

cargo check still suffer from emitting warnings only on the first run, but the issue won't be resolved upstream in the foreseeable future, as there are no caching facilities in cargo proper.

However, cargo check is also quite a bit faster for most use cases. And we are just losing some warnings, not any errors. So I now believe the tradeoff is a good one to make.

The other downside is the loss of rustc args. I didn't use them before, but hopefully it won't break anyone's setup. If it does, users still have the option of defining the old rust-cargo checker in their config.

@fmdkdd fmdkdd changed the title [Do not merge] Use `cargo check` for rust-cargo checker Use `cargo check` for rust-cargo checker Jan 15, 2018

@fmdkdd fmdkdd referenced this pull request Jan 15, 2018
@fmdkdd

This comment has been minimized.

Copy link
Member Author

commented Jan 15, 2018

Oh, and this new PR is based on #1381, so better merge that before.

Use `cargo check` and `cargo test`
In concert, they cover all of our cases, and support additional setups, like
using [dev-dependencies] in Cargo.toml.  (See flycheck/flycheck-rust/issues/55)

Unfortunately, unlike ~cargo rustc~, they emit warnings only on their first
invocation.  In practice, this means warnings may not appear if the same files
are visited multiple times with Flycheck, and no changes are mode to them.

As a bonus, since we are not building anything, we can get rid of the ~notes~
test case.

@fmdkdd fmdkdd force-pushed the rust-cargo-check branch from b24118f to b8084a2 Jan 16, 2018

@fmdkdd

This comment has been minimized.

Copy link
Member Author

commented Jan 19, 2018

Rebased and ready to review.

cc @cpitclaudel

@cpitclaudel
Copy link
Member

left a comment

Thanks. Sorry for the long delay

@fmdkdd fmdkdd merged commit 3f6bfb3 into master Feb 4, 2018

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@fmdkdd fmdkdd deleted the rust-cargo-check branch Feb 4, 2018

@fmdkdd

This comment has been minimized.

Copy link
Member Author

commented Feb 4, 2018

No worries, I'd rather wait for reviews than not have reviews at all 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.