Simplify refresh and publication of diagnostics #360
Conversation
There was a problem hiding this comment.
A few questions on this one, but in general I am really enjoying how this is coming together! I like our event loop a lot so far.
I like the snappier diagnostics but I would bet that there are going to be people out there who enjoy the "delayed" diagnostics from RStudio as well (we can deal with that later). We are probably going to have to iterate on the ERROR node diagnostics too to make sure that we dont cause squiggles in the whole file as often, because now they will almost immediately show up if you slow down during your typing at all.
|
|
||
| // Refresh LSP state now since we probably have missed some updates | ||
| // while the channel was offline. This is currently not an ideal timing | ||
| // as the channel is set up from a preemptive `r_task()` after the LSP |
There was a problem hiding this comment.
i.e. that r_task() could be happening at interrupt time, which is possibly bad with respect to looking at the envs on the search path in console_inputs() (among other things)?
I would also be in favor of making it an idle task to run once any of the user's startup code has stopped running and we get back into read_console()
crates/ark/src/lsp/backend.rs
Outdated
| events_tx.send(LspEvent::PublishDiagnostics( | ||
| url, |
There was a problem hiding this comment.
Oh pushing the publish through our event loop is neat
|
|
||
| // Check for changes to the working directory |
There was a problem hiding this comment.
It seems like we refresh at each read_console() iteration now rather than refreshing after an execute-request.
That seems reasonable to me.
Do you think the other handlers here related to graphics / working directory should also be moved eventually? It would be nice (theoretically) if this handle_execute_request() handler truly only dealt with execute-request specific things.
There was a problem hiding this comment.
yep that sounds right.
Related: posit-dev/positron#2060 (comment)
| } | ||
|
|
||
| fn generate_diagnostics(doc: &Document, state: &WorldState) -> Vec<Diagnostic> { | ||
| pub fn generate_diagnostics(doc: Document, state: WorldState) -> Vec<Diagnostic> { |
There was a problem hiding this comment.
Crazy how much this simplifies when you don't need debounce infrastructure
| // Start indexing | ||
| tokio::task::spawn_blocking(|| { | ||
| indexer::start(folders); | ||
| }); |
There was a problem hiding this comment.
I am still a little unsure of how you can guarantee that the initial round of indexing has finished before generating the first set of diagnostics anymore
There was a problem hiding this comment.
Yea I have convinced myself that we still need something here. Maybe when the indexer finishes it should kick off an "update all diagnostics" event?
I opened dplyr to bind-rows.R and immediately saw
But note that dataframe_ish is a function that is defined in that file, it is at the bottom. Previously the diagnostics would wait until the indexer finished the initial round before generating diagnostics.
I ran 1 in the Console to retrigger diagnostics and then poof the squiggle under dataframe_ish() disappears because by that time the indexer was finished.
There was a problem hiding this comment.
you're right!
On a related note, it seems the did_open() handler should trigger an index update? It doesn't on main either, but I'm pretty sure it should? We're indexing files in the workspace already but the newly opened file might be outside the workspace.
And did_close() should clear the index for that file? But only if not in the workspace?
Also deleting a file from the FS doesn't remove it from our index. And updating it from outside positron (e.g. by changing branch) does not update it either (we don't receive did_change notifications from unmanaged files in the workspace - i.e. we only receive these notifs for files for which we got a did_open).
And of course there's the issue that diagnostics of a file are influenced by completely unrelated files. And in an inconsistent way as only defined functions are in scope, not other symbols.
ok it's a giant mess, but we don't need to solve all that right now. For now how about firing diagnostics for a file after it has finished indexing? This won't take into account functions in other files, but we don't handle that particularly well anyway.
There was a problem hiding this comment.
For now how about firing diagnostics for a file after it has finished indexing
Seems good to me
There was a problem hiding this comment.
I'll fix that in the next PR to avoid merge conflicts
lionel-
force-pushed
the
feature/read-console-scopes
branch
from
d89a8bc
to
8ce6a47
Compare
lionel-
force-pushed
the
feature/diagnostics-event-loop
branch
2 times, most recently
from
096e23e
to
fba54fa
Compare
lionel-
force-pushed
the
feature/read-console-scopes
branch
from
2592ffd
to
0bf4766
Compare
lionel-
force-pushed
the
feature/diagnostics-event-loop
branch
from
fba54fa
to
f01c670
Compare
Branched from #359.
Addresses posit-dev/positron#2630.
Follow-up to #224 (Refresh diagnostics on open, on close, on change, and after R execution).
This PR focuses on diagnostics to tighten the control flow and concurrency between the different parts. Note that this will be improved further in the next PR so I wouldn't focus too much on the names of these events and how we send them. But I think it's still helpful to review this step separately.
Introduce blocking tasks. We now run diagnostics in blocking tasks instead of async ones to avoid clogging up the async pool thread. The initial round of indexing is now also spawned in a blocking task instead of being run synchronously. The next PR will add more infrastructure around launching such tasks.
New task events managed by our LSP loop introduced in Send console scopes from
ReadConsole#359 to refresh and publish diagnostics. This part is largely refactored in the next PR, but the main ideas remain.Remove the initialisation manager for the indexer. We now launch initial diagnostics from the indexer instead of in a timer.
Remove the debouncer to make worldstate a pure value (the mutable diagnostic ID is removed from document). We could reintroduce a debouncer later on but I think that should be managed externally with a dedicated task managing the debouncing for each doc. That said I like the snappy diagnostics and immediate feedback.
Thanks to these changes, diagnostics functions no longer take a clone of the backend. I've also reorganised things a bit so that there is a better file separation between diagnostics code and LSP handling code.
I've commented out the "package not installed diagnostic" code for these reasons:
This lint steers users towards an action they don't necessarily want to take (installing a package), and is rather distracting in that case: Ark: Diagnostics: Change "package not installed" from diagnostic to assist? positron#2672
We'd like to avoid running R code during diagnostics: Ark: Infrastructure: Reduce concurrent R evaluations to a minimum positron#2322
The diagnostic meshes R and tree-sitter objects in a way that's not perfectly safe and we have a known crash logged: R: Diagnostics crash related to passing tree sitter nodes back and forth with R positron#2630. This diagnostic uses R for argument matching but since we prefer to avoid running
r_task()in LSP code we should just reimplement argument matching on the Rust side.Given all these considerations I think it's best to just remove this feature for now.