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

Refresh diagnostics on open, on close, on change, and after R execution #224

Merged
merged 5 commits into from
Feb 13, 2024

Conversation

DavisVaughan
Copy link
Contributor

@DavisVaughan DavisVaughan commented Jan 30, 2024

Addresses posit-dev/positron#2155
Part of posit-dev/positron#1005

Okay, after much fiddling, I've come up with the following Alpha level scheme for our diagnostics:

  • did_open(): Runs diagnostics for that file
  • did_change(): Runs diagnostics for that file if we are up to date on changes
  • did_close(): Clears diagnostics for that file
  • did R console execution: Runs diagnostics for all open files

There is much more we can eventually do, like running diagnostics for the whole workspace, maintaining diagnostics on close (and updating them through a "related documents" feature), etc, but this should be a pretty nice improvement.

I'd also like to make a follow up PR or two that trims down our usage of R while diagnostics are being run. We currently look up the set of installed packages and environment symbols at each diagnostic call, but really I think we can cache these and only update them when we do a diagnostic refresh after R console execution (and on startup).

A few of the high level changes:

  • Replaced lsp_client with the overarching lsp_backend in RMain, because the backend holds the list of the currently open documents, and we need that to refresh them after an R console execution
  • You'll see an IndexerStateManager, this is solely used to ensure that we don't run any diagnostics until the indexer has had a chance to run (30 second timeout). The indexer is a very important part of our diagnostics, and we can return some pretty bad ones if we try to run diagnostics before the indexer has finished. Since the indexer runs in a tokio thread, this is totally possible and does happen for me in dplyr where it takes around 3 second for the indexer to fully run. I think I've managed to make the initialization check pretty cheap.
  • After handle_execute_request() (R execution), we refresh diagnostics in all open documents by requesting an r_async_task() that loops over the open documents and spawns a task on the LSP tokio runtime for each of them, requesting a diagnostic refresh.

Note: We are still undecided on posit-dev/positron#1325, which is about what we should do when you are working on an R package and haven't run load_all() yet, or if you have functions in a script with a library(dplyr) call at the top but you haven't run it yet. But the big benefit of this PR is that when you do run them, the diagnostics automatically update.

Untitled.mov

Copy link
Contributor

@kevinushey kevinushey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@DavisVaughan DavisVaughan requested a review from lionel- January 31, 2024 17:30
@lionel- lionel- force-pushed the main branch 4 times, most recently from 8e0d874 to 0ed5d7d Compare February 1, 2024 11:41
@DavisVaughan DavisVaughan force-pushed the feature/refresh-diagnostics branch from 149ef60 to 9bd141e Compare February 1, 2024 16:11
`version` is only bumped on document change, but we can request new diagnostics even if the document stays the same (i.e. after Console execution). Previously, mashing `CMD + Enter` multiple times through a large ish R document would slow the statement range provider down because the `version` wasn't changing, meaning none of the diagnostic requests were being debounced.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants