Send console scopes from ReadConsole
#359
Conversation
| // Get the set of installed packages | ||
| let installed_packages: Vec<String> = RFunction::new("base", ".packages") | ||
| .param("all.available", true) | ||
| .call()? | ||
| .try_into()?; |
There was a problem hiding this comment.
I am a bit worried that calling .packages() on each read_console() iteration is a little too expensive (particularly with things like network drives on Windows)
With the diagnostics IIUC it was only calling .packages() once diagnostics were actually being fired, which was on that 1 second cancellable delay once the user stopped doing anything.
There was a problem hiding this comment.
If we aren't already doing caching in a downstream PR, I think we should before merging
There was a problem hiding this comment.
Ideally the LSP would scan the active libpaths to determine this and get update events.
I was thinking one query per top-level-command is alright until then. Are you worried about the case of someone doing Cmd+Enter very quickly? Or about someone running a notebook from top to bottom? In many cases the computation of the chunk will dominate .packages(), e.g. large data manipulations, large plots, large models computation. So I'm not sure we need to address this urgently.
There was a problem hiding this comment.
The thing is that I'm not sure how we could cache this without watching the folders to trigger invalidation? Can you think of another way? I'd prefer not get into fs watching right now.
There was a problem hiding this comment.
I don't see an easy alternative right now either, we can keep it as is for now
| _ => {}, | ||
| }); | ||
|
|
||
| r_task(|| unsafe { |
There was a problem hiding this comment.
Super nice to be able to remove this. I've been thinking about how to do this for awhile now!
| } | ||
|
|
||
| fn generate_diagnostics(doc: &Document) -> Vec<Diagnostic> { | ||
| fn generate_diagnostics(doc: &Document, state: &WorldState) -> Vec<Diagnostic> { |
There was a problem hiding this comment.
It is neat to think about the fact that this could all eventually be running as a "long running task" and get its own free floating copy of WorldState without having to worry about interior mutability. That sounds quite nice.
There was a problem hiding this comment.
stay tuned for the next PRs! 😀
lionel-
force-pushed
the
feature/lsp-state
branch
from
6e31619 to
ed7d32f
Compare
lionel-
force-pushed
the
feature/read-console-scopes
branch
2 times, most recently
from
d2185a6 to
0853dea
Compare
Co-authored-by: Davis Vaughan <[email protected]>
These functions don't call R
lionel-
force-pushed
the
feature/read-console-scopes
branch
from
2592ffd to
0bf4766
Compare
Branched from #358.
This PR is a preparation for the next one where I remove
diagnostics_idfromDocument. This value is used in a debouncing mechanism for diagnostics which relies on mutex synchronisation of documents. The mutex needs to go away to make theWorldStatea pure, non-mutable value.I saw in a comment that the debouncer is motivated by the fact that it queries the current state of the search path via an
r_task()which we don't want to do too aggressively. So this PR removes this concurrentr_task(), this way we won't need the debouncer and the mutable documents. In the new approach we send the console scopes from ReadConsole, after each top-level evaluation. This makes the design cleaner and we don't need to query R as often.Progress towards #691 (Reduce concurrent R evaluations to a minimum): Querying the search path concurrently with R was unsafe I think, we might be in the middle of an
attach()or similar. We don't want to know the state of the search path in the middle of a computation, we want to know its state between top-level evaluations.Progress towards posit-dev/positron#3180 (Decouple LSP from the kernel): Sending the console scopes from read-console also brings us closer to decoupling the LSP from the kernel. We send the scopes via a Rust channel but it might just as well be a Jupyter comm.