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

Remove ARK_VDOCS by sending vdocs over a Kernel -> LSP event #666

Merged
merged 2 commits into from
Jan 23, 2025

Conversation

DavisVaughan
Copy link
Contributor

@DavisVaughan DavisVaughan commented Jan 10, 2025

Part of #661

There was a TODO to do this anyways, and it seemed like a pretty reasonable alternative to the global.

Getting this right is a little tricky. It's common for the base R packages (and anything loaded in your .Rprofile) to have their namespace vdocs be generated very quickly after startup before the LSP has even started up yet. This means we wouldn't have access to those namespace vdoc files, which means you would not be able to debug those packages.

We could try and fix the timing so that we don't generate any vdocs until after the LSP starts up, but that is pretty brittle.

The alternative here is to maintain our own copy of the virtual documents on the kernel side. As soon as the LSP connects and we get lsp_events_tx, we send over any known virtual documents (anything we attempted to send over before this connection would have been dropped). This also has the benefit of being able to refresh the LSP if it restarts out from under us.


Side note: In the future I think it might be better if the kernel itself was the one registered as the TextDocumentContentProvider for these virtual documents. It's unlikely that the debugger will ever move out of ark, but the LSP probably will. It feels quite weird for Positron to be asking the LSP for the contents of one of these documents. I wonder if instead our VirtualDocumentProvider on the positron-r side can perform a Jupyter request to ark for the document contents?

@DavisVaughan DavisVaughan changed the title Remove ARK_VDOCS by sending vdocs over an LSP event Remove ARK_VDOCS by sending vdocs over a Kernel -> LSP event Jan 10, 2025
@DavisVaughan DavisVaughan requested a review from lionel- January 10, 2025 20:50
Copy link
Contributor

@lionel- lionel- left a comment

Choose a reason for hiding this comment

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

The alternative here is to maintain our own copy of the virtual documents on the kernel side. As soon as the LSP connects and we get lsp_events_tx, we send over any known virtual documents (anything we attempted to send over before this connection would have been dropped). This also has the benefit of being able to refresh the LSP if it restarts out from under us.

Nice that's how I envisioned this to work.

Side note: In the future I think it might be better if the kernel itself was the one registered as the TextDocumentContentProvider for these virtual documents. It's unlikely that the debugger will ever move out of ark, but the LSP probably will. It feels quite weird for Positron to be asking the LSP for the contents of one of these documents. I wonder if instead our VirtualDocumentProvider on the positron-r side can perform a Jupyter request to ark for the document contents?

I actually think the vdocs should be provided by the LSP. Ideally it would be based on downloaded sources. This will be useful for things like code navigation.

I'm not sure how this will interact with the srcref feature though. If we can't make it work, maybe we'll need two kinds of vdocs, one for analysis/navigation and one for debugging, but I'm hoping it will not come to that. I imagine that we can deparse the function and carefully match the text (taking into account formatting differences) to a package source and use the latter if there is only one occurrence. Such a procedure can't work reliably 100% of the time though, so we might end up needing two sorts of vdocs or rely on the srcref fallback.

crates/ark/src/lsp/main_loop.rs Outdated Show resolved Hide resolved
@DavisVaughan DavisVaughan merged commit 8a957a4 into main Jan 23, 2025
5 of 6 checks passed
@DavisVaughan DavisVaughan deleted the feature/vdoc-channel branch January 23, 2025 20:28
@github-actions github-actions bot locked and limited conversation to collaborators Jan 23, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants