Skip to content

Commit

Permalink
Remove ARK_VDOCS by sending vdocs over a Kernel -> LSP event (#666)
Browse files Browse the repository at this point in the history
* Remove `ARK_VDOCS` by sending vdocs over an LSP event

* Move `impl`s to bottom
  • Loading branch information
DavisVaughan authored Jan 23, 2025
1 parent 794cb85 commit 8a957a4
Show file tree
Hide file tree
Showing 6 changed files with 120 additions and 24 deletions.
56 changes: 50 additions & 6 deletions crates/ark/src/interface.rs
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ use crate::errors;
use crate::help::message::HelpEvent;
use crate::help::r_help::RHelp;
use crate::lsp::events::EVENTS;
use crate::lsp::main_loop::DidOpenVirtualDocumentParams;
use crate::lsp::main_loop::Event;
use crate::lsp::main_loop::KernelNotification;
use crate::lsp::main_loop::TokioUnboundedSender;
Expand Down Expand Up @@ -221,6 +222,10 @@ pub struct RMain {
/// Event channel for notifying the LSP. In principle, could be a Jupyter comm.
lsp_events_tx: Option<TokioUnboundedSender<Event>>,

/// The kernel's copy of virtual documents to notify the LSP about when the LSP
/// initially connects and after an LSP restart.
lsp_virtual_documents: HashMap<String, String>,

dap: RMainDap,

pub positron_ns: Option<RObject>,
Expand Down Expand Up @@ -560,6 +565,7 @@ impl RMain {
help_event_tx: None,
help_port: None,
lsp_events_tx: None,
lsp_virtual_documents: HashMap::new(),
dap: RMainDap::new(dap),
tasks_interrupt_rx,
tasks_idle_rx,
Expand Down Expand Up @@ -1781,11 +1787,21 @@ impl RMain {
}

fn send_lsp_notification(&mut self, event: KernelNotification) {
if let Some(ref tx) = self.lsp_events_tx {
if let Err(err) = tx.send(Event::Kernel(event)) {
log::error!("Failed to send LSP notification: {err:?}");
self.lsp_events_tx = None;
}
log::trace!(
"Sending LSP notification: {event:#?}",
event = event.trace()
);

let Some(ref tx) = self.lsp_events_tx else {
log::trace!("Failed to send LSP notification. LSP events channel isn't open yet, or has been closed. Event: {event:?}", event = event.trace());
return;
};

if let Err(err) = tx.send(Event::Kernel(event)) {
log::error!(
"Failed to send LSP notification. Removing LSP events channel. Error: {err:?}"
);
self.lsp_events_tx = None;
}
}

Expand All @@ -1796,10 +1812,12 @@ impl RMain {
// 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
// is set up. We'll want to do this in an idle task.
log::trace!("LSP channel opened. Refreshing state.");
self.refresh_lsp();
self.notify_lsp_of_known_virtual_documents();
}

pub fn refresh_lsp(&mut self) {
fn refresh_lsp(&mut self) {
match console_inputs() {
Ok(inputs) => {
self.send_lsp_notification(KernelNotification::DidChangeConsoleInputs(inputs));
Expand All @@ -1808,6 +1826,32 @@ impl RMain {
}
}

fn notify_lsp_of_known_virtual_documents(&mut self) {
// Clone the whole HashMap since we need to own the uri/contents to send them
// over anyways. We don't want to clear the map in case the LSP restarts later on
// and we need to send them over again.
let virtual_documents = self.lsp_virtual_documents.clone();

for (uri, contents) in virtual_documents {
self.send_lsp_notification(KernelNotification::DidOpenVirtualDocument(
DidOpenVirtualDocumentParams { uri, contents },
))
}
}

pub fn did_open_virtual_document(&mut self, uri: String, contents: String) {
// Save our own copy of the virtual document. If the LSP is currently closed
// or restarts, we can notify it of all virtual documents it should know about
// in the LSP channel setup step. It is common for the kernel to create the
// virtual documents for base R packages before the LSP has started up.
self.lsp_virtual_documents
.insert(uri.clone(), contents.clone());

self.send_lsp_notification(KernelNotification::DidOpenVirtualDocument(
DidOpenVirtualDocumentParams { uri, contents },
))
}

pub fn call_frontend_method(&self, request: UiFrontendRequest) -> anyhow::Result<RObject> {
log::trace!("Calling frontend method {request:?}");

Expand Down
10 changes: 3 additions & 7 deletions crates/ark/src/lsp/handlers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@
//

use anyhow::anyhow;
use dashmap::DashMap;
use once_cell::sync::Lazy;
use serde_json::Value;
use stdext::unwrap;
use struct_field_names_as_array::FieldNamesAsArray;
Expand Down Expand Up @@ -384,14 +382,12 @@ pub(crate) fn handle_indent(
})
}

// TODO: Should be in WorldState and updated via message passing
pub static mut ARK_VDOCS: Lazy<DashMap<String, String>> = Lazy::new(|| DashMap::new());

pub(crate) fn handle_virtual_document(
params: VirtualDocumentParams,
state: &WorldState,
) -> anyhow::Result<VirtualDocumentResponse> {
if let Some(doc) = unsafe { ARK_VDOCS.get(&params.path) } {
Ok(doc.clone())
if let Some(contents) = state.virtual_documents.get(&params.path) {
Ok(contents.clone())
} else {
Err(anyhow!("Can't find virtual document {}", params.path))
}
Expand Down
50 changes: 45 additions & 5 deletions crates/ark/src/lsp/main_loop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,18 @@ pub(crate) enum Event {
#[derive(Debug)]
pub(crate) enum KernelNotification {
DidChangeConsoleInputs(ConsoleInputs),
DidOpenVirtualDocument(DidOpenVirtualDocumentParams),
}

/// A thin wrapper struct with a custom `Debug` method more appropriate for trace logs
pub(crate) struct TraceKernelNotification<'a> {
inner: &'a KernelNotification,
}

#[derive(Debug)]
pub(crate) struct DidOpenVirtualDocumentParams {
pub(crate) uri: String,
pub(crate) contents: String,
}

#[derive(Debug)]
Expand Down Expand Up @@ -311,7 +323,7 @@ impl GlobalState {
respond(tx, handlers::handle_indent(params, &self.world), LspResponse::OnTypeFormatting)?;
},
LspRequest::VirtualDocument(params) => {
respond(tx, handlers::handle_virtual_document(params), LspResponse::VirtualDocument)?;
respond(tx, handlers::handle_virtual_document(params, &self.world), LspResponse::VirtualDocument)?;
},
LspRequest::InputBoundaries(params) => {
respond(tx, handlers::handle_input_boundaries(params), LspResponse::InputBoundaries)?;
Expand All @@ -320,10 +332,17 @@ impl GlobalState {
},
},

Event::Kernel(notif) => match notif {
KernelNotification::DidChangeConsoleInputs(inputs) => {
state_handlers::did_change_console_inputs(inputs, &mut self.world)?;
},
Event::Kernel(notif) => {
lsp::log_info!("{notif:#?}", notif = notif.trace());

match notif {
KernelNotification::DidChangeConsoleInputs(inputs) => {
state_handlers::did_change_console_inputs(inputs, &mut self.world)?;
},
KernelNotification::DidOpenVirtualDocument(params) => {
state_handlers::did_open_virtual_document(params, &mut self.world)?;
}
}
},
}

Expand Down Expand Up @@ -589,3 +608,24 @@ pub(crate) fn publish_diagnostics(uri: Url, diagnostics: Vec<Diagnostic>, versio
version,
));
}

impl KernelNotification {
pub(crate) fn trace(&self) -> TraceKernelNotification {
TraceKernelNotification { inner: self }
}
}

impl std::fmt::Debug for TraceKernelNotification<'_> {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
match self.inner {
KernelNotification::DidChangeConsoleInputs(_) => f.write_str("DidChangeConsoleInputs"),
KernelNotification::DidOpenVirtualDocument(params) => f
.debug_struct("DidOpenVirtualDocument")
.field("uri", &params.uri)
.field("contents", &"<snip>")
.finish(),
// NOTE: Uncomment if we have notifications we don't care to specially handle
//notification => std::fmt::Debug::fmt(notification, f),
}
}
}
4 changes: 4 additions & 0 deletions crates/ark/src/lsp/state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@ pub(crate) struct WorldState {
/// Watched folders
pub(crate) workspace: Workspace,

/// Virtual documents that the LSP serves as a text document content provider for
/// Maps a `String` uri to the contents of the document
pub(crate) virtual_documents: HashMap<String, String>,

/// The scopes for the console. This currently contains a list (outer `Vec`)
/// of names (inner `Vec`) within the environments on the search path, starting
/// from the global environment and ending with the base package. Eventually
Expand Down
11 changes: 11 additions & 0 deletions crates/ark/src/lsp/state_handlers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ use crate::lsp::diagnostics::DiagnosticsConfig;
use crate::lsp::documents::Document;
use crate::lsp::encoding::get_position_encoding_kind;
use crate::lsp::indexer;
use crate::lsp::main_loop::DidOpenVirtualDocumentParams;
use crate::lsp::main_loop::LspState;
use crate::lsp::state::workspace_uris;
use crate::lsp::state::WorldState;
Expand Down Expand Up @@ -393,6 +394,16 @@ pub(crate) fn did_change_console_inputs(
Ok(())
}

#[tracing::instrument(level = "info", skip_all)]
pub(crate) fn did_open_virtual_document(
params: DidOpenVirtualDocumentParams,
state: &mut WorldState,
) -> anyhow::Result<()> {
// Insert new document, replacing any old one
state.virtual_documents.insert(params.uri, params.contents);
Ok(())
}

// FIXME: The initial indexer is currently racing against our state notification
// handlers. The indexer is synchronised through a mutex but we might end up in
// a weird state. Eventually the index should be moved to WorldState and created
Expand Down
13 changes: 7 additions & 6 deletions crates/ark/src/srcref.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use harp::r_symbol;
use harp::utils::r_typeof;
use libr::*;

use crate::lsp::handlers::ARK_VDOCS;
use crate::interface::RMain;
use crate::modules::ARK_ENVS;
use crate::r_task;
use crate::variables::variable::is_binding_fancy;
Expand Down Expand Up @@ -96,11 +96,12 @@ pub(crate) async fn ns_populate_srcref(ns_name: String) -> anyhow::Result<()> {
vdoc.len()
);

// SAFETY: That's a DashMap so should be safe across threads
unsafe {
// Save virtual document containing the namespace source
ARK_VDOCS.insert(uri_path, vdoc.join("\n"));
}
let contents = vdoc.join("\n");

// Notify LSP of the opened virtual document so the LSP can function as a
// text document content provider of the virtual document contents, which is
// used by the debugger.
RMain::with_mut(|main| main.did_open_virtual_document(uri_path, contents));

Ok(())
}
Expand Down

0 comments on commit 8a957a4

Please sign in to comment.