search

flycheck / flycheck-rust

Better Rust/Cargo support for Flycheck
GNU General Public License v3.0
121 stars 19 forks source link

Refactor flycheck-rust using cargo #43

Closed fmdkdd closed 7 years ago

fmdkdd commented 8 years ago

This PR is there to start a discussion on the basis of a working implementation, but is not meant to merge as-is until the discussion is over.

The gist of it is that I'm proposing to:

  1. Use cargo (and only cargo) to find the build target for the current file, and set the variables used by rust-cargo accordingly.
  2. Leave the rust checker for single rust files, which means not setting up any variables for it in flycheck-rust.

And as I'm unsure whether this will break anyone's setup, I'm asking for feedback.

cc @flycheck/rust

Read on for the details.

To fix #38, I figured I might as well clean up the logic a bit in flycheck-rust, which was getting a bit redundant. Originally, it seems we used various methods to get the information to pass to the rustc checker in order to be able to check single rust files. Then we started to use cargo directly to get this information, and now we have two ways of doing things. I think that #8 intended for cargo to be the main way to extract the information needed to run the rust checkers, and this PR aims to do that.

As I understand it, the role of flycheck-rust is to set the variables used by the rust checkers in flycheck, so the checkers can produce meaningful errors for the current file. The rust checkers (especially rust-cargo) won't actually do anything without the proper variables set first, especially flycheck-rust-crate-type and flycheck-rust-binary-name. These variables could also be set directly by the user, by file-local variables or .dir-locals.el or any other means. Flycheck-rust will just try to set them when their value can be found automatically.

Also, I understand there's a slight mismatch between flycheck checkers that are meant to run on single files, but cargo only builds targets, not individual files. We cannot point cargo to the file X and say "give me the compilation errors for X". We have to say "give me compilation errors for the binary crate Y", where crate Y contains and makes use of the file X, and we will get errors for X (among all the other files in Y). The problem with that is that we have no way to know for sure that file X is part of crate Y. cargo won't tell us, and we would need to resolve the imports and build a dependency graph to know the answer; that's certainly out of scope. The only cases where we can be certain is when cargo read-manifest gives us the exact filenames for targets. Other than that, we can make educated guesses based on the conventional cargo layout (e.g., any file under src/ other than src/main.rs contribute to the library target). These guesses may fail if the project actually does not follow the convention, but then the user can always override these guesses with local variables.

I set up a test crate, added all the relevant test cases from the conventional cargo layout I could find, and just used cargo to find the correct target to build based on the buffer filename. Even when deleting most of the code, it just works and sets up the flycheck-rust-crate-type and flycheck-rust-binary-name correctly for all the test cases.

This fixes #38, increases our coverage, and gets rid of most of the code. However, I don't know if I've broken anything else, especially for the plain rust checker.

I left out setting these variables:

As I gathered from #8, the use case for the bare rust checker is to check single files that are not part of a cargo project (like StackOverflow answers, or minimal working examples), leaving the rust-cargo checker for pretty much anything else. Though if that is the case, I don't understand why we we pass the variable flycheck-rust-crate-type to rustc in the rust checker, or flycheck-rust-library-path, especially as we have flycheck-rust-args to already pass arbitrary arguments to rustc. That may be a separate PR to flycheck if it turns out these variables are indeed superfluous.

Note: an alternative would be to add the ability to cargo to give us a list of build targets a file is associated with. That way, we could just get rid of flycheck-rust altogether and just invoke that new command in rust-cargo directly. I don't know if the cargo maintainers would be open to it, or how complex it would be to implement (I'm guessing not much, since the dependency information should already be available, but I haven't checked).

swsnr commented 8 years ago

Wow massive, thanks a lot! I'll read through your post tomorrow!

Incidentally https://internals.rust-lang.org/t/introducing-rust-language-server-source-release/4209 just hit my twitter stream 😊 Looks as if checking a file is already implemented; means that we could provide an experimental checker in this project as well.

swsnr commented 8 years ago

@fmdkdd Thanks again for this massive effort; thank you so much for your work on Rust support.

I'm afraid that I don't known enough about Rust to make any informed decision about Rust support anymore, so I'll leave the detailed feedback to others.

What I can say is that it's not unlikely that some options for Rust support in Flycheck are redundant. Rust support came very early—I think somewhere around 0.7—and things have considerable changed since then. There's probably quite a bit technical debt in our current Rust support, both in this package and in Flycheck itself. So if you believe that some options are superfluous, you're quite likely correct 😎 So go ahead, and remove them from Flycheck proper. If none objects it's unlikely that the options were important anyway.

Likewise don't be afraid to break anyone's setup! I consider this package unstable—which is why there's no tag for MELPA Stable yet—and in Flycheck we explicitly make no promises about backwards compatibility to maintain the freedom to change for the better if possible.

So as far as I'm concerned any ever so small improvement to Rust support is absolutely worth even massive breaking changes.

I think that we won't settle on any stable Rust support until the language server is ready for production and provides a stable interface to check a single file within a Rust project. By that day we'll likely remove Rust support from Flycheck upstream entirely in favour of a dedicated language server checker in this package (similar to flycheck-ocaml), and maybe even remove this package in favour of a catch-all rust-ide package with Flycheck support maintained by the Rust community rather than the Flycheck community.

TL;DR: Feel free to go one with whatever changes to Rust support you consider necessary, and don't be afraid of breaking things :+1:

fmdkdd commented 7 years ago

@lunaryorn Thanks for the answers. I also saw the alpha release of the RLS at about the same time, and experimented a bit with it. Once it stabilizes, it should solve most of the problems with Rust support on our end, so I'm following its development with great interest.

This PR can help in the meantime. Good to know I can go ahead and break things :) I'll try not to too though, and just enhance the support as I can until we shift to the RLS.

sickHamm commented 7 years ago

"cargo check" has been merged into Cargo. https://github.com/rust-lang/cargo/pull/3296#event-893283611

fmdkdd commented 7 years ago

@sickHamm Thanks for the heads up. I'll try that command in nightly and see how it goes.

blaenk commented 7 years ago

Figured I'd try this out after I ran into #38 but I think I'm missing something. I have a test at tests/buffer.rs and flycheck-compile is outputting an error:

-*- mode: compilation; default-directory: "~/code/rust/hoedown/tests/" -*-
Compilation started at Wed Dec 28 15:30:28

cargo rustc --bin buffer -- -Z no-trans -Z unstable-options --error-format\=json --test
error: no bin target named `buffer`

Compilation exited abnormally with code 101 at Wed Dec 28 15:30:29

So it seems like I'd need to add buffer as a binary target to my Cargo.toml? You think there might be a way around that?

rustc 1.14.0 (e8a012324 2016-12-16) cargo 0.15.0-nightly (298a012 2016-12-20) GNU Emacs 25.1.1

algesten commented 7 years ago

about "a stable interface to check a single file". I'm new to rust, but when I split my project up into multiple files, the compilation order is still dictated by main.rs or lib.rs. if one file rely on say a struct defined in another file, that file must be declared to compile before the current one.

what I'm wondering is what we mean that we want to check one single file? do we only want errors output for one single file although there may be other errors in other files?

consider macros. they are so flexible you can pretty much define your own language. they can be defined in another file than the one you are working in.

isn't it rather the case that we must run the full cargo compilation and pick out the output just for the file that is in context in emacs? or is that what we mean we "check a single file"? - that we get the output for one file despite compiling the entire project?

fmdkdd commented 7 years ago

@blaenk This PR relies on a matching refactoring to the rust checker in flycheck.el as well. I've pushed these changes on the branch wip-refactor-rust-cargo if you want to test them. You can leave feedback here, it would be much appreciated 👍

@algesten

that we get the output for one file despite compiling the entire project?

This. The tricky part is determining which compilation target to build when you are looking at a single file at a time in Emacs. Compare that to the workflow of IDEs like Eclipse or IntelliJ where the unit you are editing is the project rather than the file. There, you can "Run target" and get compilation errors. Flycheck checkers are associated to one buffer rather than one project, so we must determine which target to build automatically, and that is the sole purpose of the code in flycheck-rust.

blaenk commented 7 years ago

Thanks I can confirm that works. I'll let you know how it goes but so far so good!

blaenk commented 7 years ago

Been working perfectly fine so far.

blaenk commented 7 years ago

Just wanted to chime in and say that it's been working well. Any news on the merging of these changes?

mkpankov commented 7 years ago

@fmdkdd unless you're still working on some improvements, I think we should merge the PR

mkpankov commented 7 years ago

@fmdkdd btw, I believe we need to open a PR in flycheck itself with aforementioned branch: https://github.com/flycheck/flycheck/compare/wip-refactor-rust-cargo?expand=1

fmdkdd commented 7 years ago

Thanks for the enthusiasm for these changes. There was actually an issue in the Flycheck branch regarding dependencies and the way we pass --error-format=json to rustc via cargo. The fix relied on using --message-format instead, but this was not compatible with -Zno-trans until 1.15.

Now that 1.15 is released, I've pushed flycheck/flycheck#1201 as a pre-requisite to handling multiple targets. Once that PR is merged, I will submit a proper PR for the wip-refactor-rust-cargo branch, and finally take care of the current PR.

fmdkdd commented 7 years ago

I've updated the PR to use cargo metadata (#46) while I was at it. Seems to work on my end. It should be good to merge when flycheck/flycheck#1206 lands.

The main gotcha of this PR is that we do not set the variables that are specific to the plain rust checker: flycheck-rust-library-path and flycheck-rust-crate-root. We could, but the code is simpler without it. If you find it convenient to have them set automatically, now is the time to speak (but later is fine too).