In-server cargo check watching #2668

kiljacken · 2019-12-25T11:26:21Z

Opening a draft now so people can follow the progress, and comment if they spot something stupid.

Things that need doing:

Running cargo check on save
Pipe through configuration options from client
Tests for parsing behavior
Remove existing cargo watch support from VSCode extension
Progress notification in VSCode extension using LSP 3.15 $/progress notification
~~Rework ra-ide diagnostics to support secondary messages~~
~~Make cargo-check watcher use ra-ide diagnostics~~

I'd love some input on whether to try to keep the status bar progress thingy for VSCode? It will require some plumbing, and maintaining yet another rust-analyzer specific LSP notification, which I'm not sure we want to.

Fixes #1894

lnicola · 2019-12-25T11:29:47Z

I'd love some input on whether to try to keep the status bar progress thingy for VSCode? It will require some plumbing, and maintaining yet another rust-analyzer specific LSP notification, which I'm not sure we want to.

Does https://microsoft.github.io/language-server-protocol/specifications/specification-3-15/#progress help here?

kiljacken · 2019-12-25T11:31:17Z

Does https://microsoft.github.io/language-server-protocol/specifications/specification-3-15/#progress help here?

Amazing, that seems to be just the kind of notification we'd need! They must be reading our collective minds.

Still keeps tests around for reference when porting them to rust

kiljacken · 2019-12-25T19:02:47Z

I decided against the changing anything with regard to ra-ide's diagnostic system for now, to keep the pull request at a manageable scope. If the CI passes, this should be ready for review.

kiljacken · 2019-12-25T19:36:18Z

Okay, looks good to me, but this is large enough to need a couple eyes.

Key points:

cargo check (or another compatible output command) gets run on file save, as check needs up to date source on disk to do it's magic. This also means that no complex de-bouncing logic is needed currently.
Most of the diagnostic parsing code is ported directly from the Typescript implementation, so current behavior is maintained
It's currently enabled by default. I'm open to changing this, but until we have a broader set of diagnostics implemented by the server this is provides a better experience.
VSCode configuration remains mostly backwards compatible, with exception of arguments as we'd have to do full shell argument parsing to keep that compatible.
VSCode retains it's status display by using the new LSP 3.15 progress command, and adding a listener to the extension.

crates/ra_lsp_server/src/lib.rs

crates/ra_lsp_server/src/cargo_check.rs

matklad · 2019-12-25T20:18:44Z

crates/ra_lsp_server/src/cargo_check.rs

@@ -0,0 +1,1316 @@
+//! cargo_check provides the functionality needed to run `cargo check` or
+//! another compatible command (f.x. clippy) in a background thread and provide
+//! LSP diagnostics based on the output of the command.


This module/crate would benefit from discussing our threading/scheduling/cancellation model. Like, how many threads do we use them, when we fire them up, how we make sure they are cleanly joined.

matklad · 2019-12-25T20:23:06Z

I've looked through this, and in general I like it! This is already big, so I am biased towards merging this sooner, rather than later, and fixing the fallout, rather than trying to do the perfect thing from the first try.

To do this, I think the main pre-requsite would be:

split into a separate crate
separate boring verbose code for parsing from complicated code that manages threads, processes and events (just move them to different modules)
move verbose tests to a separate file

It would also be nice, but not strictly necessary, to figure out the cancellation story from the start (as it might affect the overall design quite a bit).

…eck subprocess

kiljacken · 2019-12-27T10:46:24Z

@matklad I've pushed some commits extracting the code into a ra_cargo_watch crate, splitting it into a few modules, and setting up a proper shutdown/cancellation strategy.

For shutdown or cancellation, dropping the corresponding struct always does "the right thing", e.g shutting down the thread and waiting for it to end. Hope that makes sense!

Even though this didn't error, it became clear to me that it was closing the wrong channel, resulting in the child thread never finishing.

flodiebold · 2019-12-27T16:16:00Z

I tried this out, and while it works great, I'm seeing some constant CPU usage from crossbeam select inside CheckWatcherState::run even while the server should be completely idle 😕

kiljacken · 2019-12-27T16:21:19Z

@flodiebold I might know how to fix that. It's probably because the sub-process wrapper thread is ended, so one of the channels being selected from is always "ready", in that it always has a Err result ready. Shouldn't be hard to fix, give me a few minutes :)

kiljacken · 2019-12-27T16:29:36Z

There we go, that solves the issue on my end.

@flodiebold Thanks for spotting it!

flodiebold · 2019-12-27T16:39:12Z

That indeed fixes it! 👍

matklad · 2019-12-29T12:57:09Z

LGTM!

bors r+

2668: In-server cargo check watching r=matklad a=kiljacken Opening a draft now so people can follow the progress, and comment if they spot something stupid. Things that need doing: - [x] Running cargo check on save - [x] Pipe through configuration options from client - [x] Tests for parsing behavior - [x] Remove existing cargo watch support from VSCode extension - [x] Progress notification in VSCode extension using LSP 3.15 `$/progress` notification - [ ] ~~Rework ra-ide diagnostics to support secondary messages~~ - [ ] ~~Make cargo-check watcher use ra-ide diagnostics~~ ~~I'd love some input on whether to try to keep the status bar progress thingy for VSCode? It will require some plumbing, and maintaining yet another rust-analyzer specific LSP notification, which I'm not sure we want to.~~ Fixes #1894 Co-authored-by: Emil Lauridsen <mine809@gmail.com>

bors · 2019-12-29T13:08:02Z

Build succeeded

Rust
TypeScript

more info: #93 rust-lang/rust-analyzer#2668

kiljacken changed the title ~~Initial implementation of cargo check watching~~ In-server cargo check wathcing Dec 25, 2019

kiljacken changed the title ~~In-server cargo check wathcing~~ In-server cargo check watching Dec 25, 2019

kiljacken added 11 commits December 25, 2019 17:37

Initial implementation of cargo check watching

66e8ef5

Remove cargo-watch from vscode extension.

41a1ec7

Still keeps tests around for reference when porting them to rust

Configuration plumbing for cargo watcher

6af4bf7

Fix use of wrong message in diagnostic

17360b5

Migrate tests from extension to rust

c21fbc3

Remove cargo watch supporting code and tests from vscode extension

500fe46

Re-implement status display using LSP 3.15 progress event

178c23f

Re-format VSCode extension changes

b9c10ed

Re-format ra_lsp_server changes

f7d04d0

Actually respect disabling cargo check functionality

069c165

Some documentatioN

71d2d81

kiljacken marked this pull request as ready for review December 25, 2019 19:02

Keep VSCode config mostly backwards compatible

0cdbd08

matklad reviewed Dec 25, 2019

View reviewed changes

crates/ra_lsp_server/src/lib.rs Outdated Show resolved Hide resolved

matklad reviewed Dec 25, 2019

View reviewed changes

crates/ra_lsp_server/src/cargo_check.rs Outdated Show resolved Hide resolved

matklad reviewed Dec 25, 2019

View reviewed changes

crates/ra_lsp_server/src/cargo_check.rs Outdated Show resolved Hide resolved

matklad reviewed Dec 25, 2019

View reviewed changes

kiljacken added 3 commits December 27, 2019 11:10

Move cargo watch functionality to separate crate

428a6ff

Consistent, hopefully robust, shutdown/cancelation story for cargo ch…

a2d1069

…eck subprocess

Shutdown/cancelation story for main cargo watch thread

02ce5bb

kiljacken added 2 commits December 27, 2019 11:47

Cargo fmt run

4d33835

Add doc comment to module

59837c7

kiljacken added 2 commits December 27, 2019 12:42

Fix shutdown behavoir of main cargo-watch thread.

ed84c85

Even though this didn't error, it became clear to me that it was closing the wrong channel, resulting in the child thread never finishing.

Don't finish main cargo watch thread when subprocess finishes.

c732f21

Fix busy-waiting issue in main cargo watch thread

899dbeb

bors bot merged commit 899dbeb into rust-lang:master Dec 29, 2019

kiljacken deleted the cargo-watch branch December 29, 2019 13:09

fannheyward mentioned this pull request Dec 30, 2019

Client-side cargo watch support has been removed fannheyward/coc-rust-analyzer#93

Closed

fannheyward added a commit to fannheyward/coc-rust-analyzer that referenced this pull request Dec 30, 2019

feat(cargo_watch): remove cargo watch

c6ddffe

more info: #93 rust-lang/rust-analyzer#2668

This was referenced Jan 9, 2020

support rust-analyzer.enableCargoWatchOnStartup for emacs #1869

Closed

vscode freezes when trying to apply clippy suggestion #1713

Closed

keeneyetact added a commit to keeneyetact/rust-see that referenced this pull request May 9, 2023

feat(cargo_watch): remove cargo watch

73c0296

more info: #93 rust-lang/rust-analyzer#2668

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

In-server cargo check watching #2668

In-server cargo check watching #2668

kiljacken commented Dec 25, 2019 •

edited

Loading

lnicola commented Dec 25, 2019

kiljacken commented Dec 25, 2019

kiljacken commented Dec 25, 2019

kiljacken commented Dec 25, 2019

matklad Dec 25, 2019

matklad commented Dec 25, 2019

kiljacken commented Dec 27, 2019

flodiebold commented Dec 27, 2019

kiljacken commented Dec 27, 2019

kiljacken commented Dec 27, 2019

flodiebold commented Dec 27, 2019

matklad commented Dec 29, 2019

bors bot commented Dec 29, 2019

In-server cargo check watching #2668

In-server cargo check watching #2668

Conversation

kiljacken commented Dec 25, 2019 • edited Loading

lnicola commented Dec 25, 2019

kiljacken commented Dec 25, 2019

kiljacken commented Dec 25, 2019

kiljacken commented Dec 25, 2019

matklad Dec 25, 2019

Choose a reason for hiding this comment

matklad commented Dec 25, 2019

kiljacken commented Dec 27, 2019

flodiebold commented Dec 27, 2019

kiljacken commented Dec 27, 2019

kiljacken commented Dec 27, 2019

flodiebold commented Dec 27, 2019

matklad commented Dec 29, 2019

bors bot commented Dec 29, 2019

Build succeeded

kiljacken commented Dec 25, 2019 •

edited

Loading