From 2869ca1ff2bbd9c58571ebe62c9687598a0454e3 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Sat, 11 May 2024 17:04:43 +0200 Subject: [PATCH 1/7] Make `Document` clonable --- crates/ark/src/lsp/documents.rs | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/crates/ark/src/lsp/documents.rs b/crates/ark/src/lsp/documents.rs index 54acbcf77..718eaeaf3 100644 --- a/crates/ark/src/lsp/documents.rs +++ b/crates/ark/src/lsp/documents.rs @@ -5,6 +5,9 @@ // // +use std::sync::Arc; +use std::sync::RwLock; + use anyhow::*; use ropey::Rope; use tower_lsp::lsp_types::DidChangeTextDocumentParams; @@ -34,6 +37,7 @@ fn compute_point(point: Point, text: &str) -> Point { } } +#[derive(Clone)] pub struct Document { // The document's textual contents. pub contents: Rope, @@ -51,8 +55,9 @@ pub struct Document { // hasn't changed (i.e. after console execution). pub diagnostics_id: i64, - // The parser used to generate the AST. - pub parser: Parser, + // The parser used to generate the AST. TODO: Once LSP handlers are + // properly synchronised, remove the RwLock. + pub parser: Arc>, // The document's AST. pub ast: Tree, @@ -92,7 +97,7 @@ impl Document { pending, version, diagnostics_id, - parser, + parser: Arc::new(RwLock::new(parser)), ast, } } @@ -220,7 +225,11 @@ impl Document { let contents = &self.contents; let callback = &mut |byte, point| Self::parse_callback(contents, byte, point); - let ast = self.parser.parse_with(callback, Some(&self.ast)); + let ast = self + .parser + .write() + .unwrap() + .parse_with(callback, Some(&self.ast)); self.ast = ast.unwrap(); Ok(()) From 5ba94526d9a6c2ed4a25e589d0f0b3615eb5b21d Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Sun, 12 May 2024 07:43:41 +0200 Subject: [PATCH 2/7] Disable `library()` lints for now --- crates/ark/src/lsp/diagnostics.rs | 318 ++++++++++++++---------------- 1 file changed, 144 insertions(+), 174 deletions(-) diff --git a/crates/ark/src/lsp/diagnostics.rs b/crates/ark/src/lsp/diagnostics.rs index 5df27a973..f2eff1169 100644 --- a/crates/ark/src/lsp/diagnostics.rs +++ b/crates/ark/src/lsp/diagnostics.rs @@ -7,35 +7,13 @@ use std::collections::HashMap; use std::collections::HashSet; -use std::marker::PhantomData; use std::time::Duration; use anyhow::anyhow; use anyhow::bail; use anyhow::Result; -use harp::call::r_expr_quote; -use harp::exec::RFunction; -use harp::exec::RFunctionExt; -use harp::external_ptr::ExternalPointer; -use harp::object::RObject; -use harp::r_symbol; use harp::utils::is_symbol_valid; -use harp::utils::r_is_null; use harp::utils::sym_quote_invalid; -use libr::R_NilValue; -use libr::Rf_ScalarInteger; -use libr::Rf_allocVector; -use libr::Rf_cons; -use libr::Rf_lang1; -use libr::Rf_xlength; -use libr::CDR; -use libr::RAW; -use libr::RAWSXP; -use libr::SETCDR; -use libr::SET_TAG; -use libr::SET_VECTOR_ELT; -use libr::VECSXP; -use libr::VECTOR_ELT; use ropey::Rope; use stdext::*; use tower_lsp::lsp_types::Diagnostic; @@ -51,7 +29,6 @@ use crate::lsp::encoding::convert_tree_sitter_range_to_lsp_range; use crate::lsp::indexer; use crate::lsp::state::WorldState; use crate::lsp::traits::rope::RopeExt; -use crate::r_task; use crate::r_task::r_async_task; use crate::treesitter::BinaryOperatorType; use crate::treesitter::NodeType; @@ -782,137 +759,150 @@ fn recurse_call_arguments_default( ().ok() } -struct TreeSitterCall<'a> { - // A call of the form (list(0L, ), foo = list(1L, )) - pub call: RObject, - node_phantom: PhantomData<&'a Node<'a>>, -} - -impl<'a> TreeSitterCall<'a> { - pub unsafe fn new( - node: Node<'a>, - function: &str, - context: &mut DiagnosticContext, - ) -> Result { - // start with a call to the function: () - let sym = r_symbol!(function); - let call = RObject::new(Rf_lang1(sym)); - - // then augment it with arguments - let mut tail = *call; - - if let Some(arguments) = node.child_by_field_name("arguments") { - let mut cursor = arguments.walk(); - let children = arguments.children_by_field_name("argument", &mut cursor); - let mut i = 0; - for child in children { - let arg_list = RObject::from(Rf_allocVector(VECSXP, 2)); - - // set the argument to a list<2>, with its first element: a scalar integer - // that corresponds to its O-based position. The position is used below to - // map back to the Node - SET_VECTOR_ELT(*arg_list, 0, Rf_ScalarInteger(i as i32)); - - // Set the second element of the list to an external pointer - // to the child node. - if let Some(value) = child.child_by_field_name("value") { - // TODO: Wrap this in a nice constructor - let node_size = std::mem::size_of::(); - let node_storage = Rf_allocVector(RAWSXP, node_size as isize); - SET_VECTOR_ELT(*arg_list, 1, node_storage); - - let p_node_storage: *mut Node<'a> = RAW(node_storage) as *mut Node<'a>; - std::ptr::copy_nonoverlapping(&value, p_node_storage, 1); - } - - SETCDR(tail, Rf_cons(*arg_list, R_NilValue)); - tail = CDR(tail); - - // potentially add the argument name - if let Some(name) = child.child_by_field_name("name") { - let name = context.contents.node_slice(&name)?.to_string(); - let sym_name = r_symbol!(name); - SET_TAG(tail, sym_name); - } - - i = i + 1; - } - } - - Ok(Self { - call, - node_phantom: PhantomData, - }) - } -} - -fn recurse_call_arguments_custom( - node: Node, - context: &mut DiagnosticContext, - diagnostics: &mut Vec, - function: &str, - diagnostic_function: &str, -) -> Result<()> { - r_task(|| unsafe { - // Build a call that mixes treesitter nodes (as external pointers) - // library(foo, pos = 2 + 2) - // -> - // library([0, ], pos = [1, ]) - // where: - // - node0 is an external pointer to a treesitter Node for the identifier `foo` - // - node1 is an external pointer to a treesitter Node for the call `2 + 2` - // - // The TreeSitterCall object holds on to the nodes, so that they can be - // safely passed down to the R side as external pointers - let call = TreeSitterCall::new(node, function, context)?; - - let custom_diagnostics = RFunction::from(diagnostic_function) - .add(r_expr_quote(call.call)) - .add(ExternalPointer::new(context.contents)) - .call()?; - - if !r_is_null(*custom_diagnostics) { - let n = Rf_xlength(*custom_diagnostics); - for i in 0..n { - // diag is a list with: - // - The kind of diagnostic: skip, default, simple - // - The node external pointer, i.e. the ones made in TreeSitterCall::new - // - The message, when kind is "simple" - let diag = VECTOR_ELT(*custom_diagnostics, i); - - let kind: String = RObject::view(VECTOR_ELT(diag, 0)).try_into()?; - - if kind == "skip" { - // skip the diagnostic entirely, e.g. - // library(foo) - // ^^^ - continue; - } - - let ptr = VECTOR_ELT(diag, 1); - let value: Node<'static> = *(RAW(ptr) as *mut Node<'static>); - - if kind == "default" { - // the R side gives up, so proceed as normal, e.g. - // library(foo, pos = ...) - // ^^^ - recurse(value, context, diagnostics)?; - } else if kind == "simple" { - // Simple diagnostic from R, e.g. - // library("ggplot3") - // ^^^^^^^ Package 'ggplot3' is not installed - let message: String = RObject::view(VECTOR_ELT(diag, 2)).try_into()?; - let range = value.range(); - let range = convert_tree_sitter_range_to_lsp_range(context.contents, range); - let diagnostic = Diagnostic::new_simple(range, message.into()); - diagnostics.push(diagnostic); - } - } - } - - ().ok() - }) -} +// This is commented out because: +// +// - The package not installed lint is a bit too distracting. Should it become +// an assist? +// https://github.com/posit-dev/positron/issues/2672 +// - We'd like to avoid running R code during diagnostics +// https://github.com/posit-dev/positron/issues/2321 +// - The diagnostic meshes R and tree-sitter objects in a way that's not +// perfectly safe and we have a known crash logged: +// https://github.com/posit-dev/positron/issues/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. + +// struct TreeSitterCall<'a> { +// // A call of the form (list(0L, ), foo = list(1L, )) +// pub call: RObject, +// node_phantom: PhantomData<&'a Node<'a>>, +// } +// +// impl<'a> TreeSitterCall<'a> { +// pub unsafe fn new( +// node: Node<'a>, +// function: &str, +// context: &mut DiagnosticContext, +// ) -> Result { +// // start with a call to the function: () +// let sym = r_symbol!(function); +// let call = RObject::new(Rf_lang1(sym)); +// +// // then augment it with arguments +// let mut tail = *call; +// +// if let Some(arguments) = node.child_by_field_name("arguments") { +// let mut cursor = arguments.walk(); +// let children = arguments.children_by_field_name("argument", &mut cursor); +// let mut i = 0; +// for child in children { +// let arg_list = RObject::from(Rf_allocVector(VECSXP, 2)); +// +// // set the argument to a list<2>, with its first element: a scalar integer +// // that corresponds to its O-based position. The position is used below to +// // map back to the Node +// SET_VECTOR_ELT(*arg_list, 0, Rf_ScalarInteger(i as i32)); +// +// // Set the second element of the list to an external pointer +// // to the child node. +// if let Some(value) = child.child_by_field_name("value") { +// // TODO: Wrap this in a nice constructor +// let node_size = std::mem::size_of::(); +// let node_storage = Rf_allocVector(RAWSXP, node_size as isize); +// SET_VECTOR_ELT(*arg_list, 1, node_storage); +// +// let p_node_storage: *mut Node<'a> = RAW(node_storage) as *mut Node<'a>; +// std::ptr::copy_nonoverlapping(&value, p_node_storage, 1); +// } +// +// SETCDR(tail, Rf_cons(*arg_list, R_NilValue)); +// tail = CDR(tail); +// +// // potentially add the argument name +// if let Some(name) = child.child_by_field_name("name") { +// let name = context.contents.node_slice(&name)?.to_string(); +// let sym_name = r_symbol!(name); +// SET_TAG(tail, sym_name); +// } +// +// i = i + 1; +// } +// } +// +// Ok(Self { +// call, +// node_phantom: PhantomData, +// }) +// } +// } +// +// fn recurse_call_arguments_custom( +// node: Node, +// context: &mut DiagnosticContext, +// diagnostics: &mut Vec, +// function: &str, +// diagnostic_function: &str, +// ) -> Result<()> { +// r_task(|| unsafe { +// // Build a call that mixes treesitter nodes (as external pointers) +// // library(foo, pos = 2 + 2) +// // -> +// // library([0, ], pos = [1, ]) +// // where: +// // - node0 is an external pointer to a treesitter Node for the identifier `foo` +// // - node1 is an external pointer to a treesitter Node for the call `2 + 2` +// // +// // The TreeSitterCall object holds on to the nodes, so that they can be +// // safely passed down to the R side as external pointers +// let call = TreeSitterCall::new(node, function, context)?; +// +// let custom_diagnostics = RFunction::from(diagnostic_function) +// .add(r_expr_quote(call.call)) +// .add(ExternalPointer::new(context.contents)) +// .call()?; +// +// if !r_is_null(*custom_diagnostics) { +// let n = Rf_xlength(*custom_diagnostics); +// for i in 0..n { +// // diag is a list with: +// // - The kind of diagnostic: skip, default, simple +// // - The node external pointer, i.e. the ones made in TreeSitterCall::new +// // - The message, when kind is "simple" +// let diag = VECTOR_ELT(*custom_diagnostics, i); +// +// let kind: String = RObject::view(VECTOR_ELT(diag, 0)).try_into()?; +// +// if kind == "skip" { +// // skip the diagnostic entirely, e.g. +// // library(foo) +// // ^^^ +// continue; +// } +// +// let ptr = VECTOR_ELT(diag, 1); +// let value: Node<'static> = *(RAW(ptr) as *mut Node<'static>); +// +// if kind == "default" { +// // the R side gives up, so proceed as normal, e.g. +// // library(foo, pos = ...) +// // ^^^ +// recurse(value, context, diagnostics)?; +// } else if kind == "simple" { +// // Simple diagnostic from R, e.g. +// // library("ggplot3") +// // ^^^^^^^ Package 'ggplot3' is not installed +// let message: String = RObject::view(VECTOR_ELT(diag, 2)).try_into()?; +// let range = value.range(); +// let range = convert_tree_sitter_range_to_lsp_range(context.contents, range); +// let diagnostic = Diagnostic::new_simple(range, message.into()); +// diagnostics.push(diagnostic); +// } +// } +// } +// +// ().ok() +// }) +// } fn recurse_call( node: Node, @@ -934,26 +924,6 @@ fn recurse_call( let fun = fun.as_str(); match fun { - // TODO: there should be some sort of registration mechanism so - // that functions can declare that they know how to generate - // diagnostics, i.e. ggplot2::aes() would skip diagnostics - - // special case to deal with library() and require() nse - "library" => recurse_call_arguments_custom( - node, - context, - diagnostics, - "library", - ".ps.diagnostics.custom.library", - )?, - "require" => recurse_call_arguments_custom( - node, - context, - diagnostics, - "require", - ".ps.diagnostics.custom.require", - )?, - // default case: recurse into each argument _ => recurse_call_arguments_default(node, context, diagnostics)?, }; From 126178917386fa6c246f89f83a07878f2980434a Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Sun, 12 May 2024 12:04:16 +0200 Subject: [PATCH 3/7] Integrate diagnostics in LSP event loop --- crates/ark/src/interface.rs | 43 +++----- crates/ark/src/lsp/backend.rs | 123 +++++++++++++++++---- crates/ark/src/lsp/diagnostics.rs | 174 ++---------------------------- crates/ark/src/lsp/documents.rs | 9 -- crates/ark/src/lsp/handler.rs | 4 +- crates/ark/src/lsp/indexer.rs | 84 +++------------ crates/ark/src/main.rs | 11 +- crates/ark/src/shell.rs | 3 - 8 files changed, 140 insertions(+), 311 deletions(-) diff --git a/crates/ark/src/interface.rs b/crates/ark/src/interface.rs index 2285b1af6..8fd963de5 100644 --- a/crates/ark/src/interface.rs +++ b/crates/ark/src/interface.rs @@ -91,8 +91,6 @@ use regex::Regex; use serde_json::json; use stdext::result::ResultOrLog; use stdext::*; -use tokio::runtime::Runtime; -use tokio::sync::mpsc::UnboundedSender as TokioUnboundedSender; use crate::dap::dap::DapBackendEvent; use crate::dap::dap_r_main::RMainDap; @@ -101,9 +99,9 @@ use crate::errors; use crate::help::message::HelpReply; use crate::help::message::HelpRequest; use crate::kernel::Kernel; -use crate::lsp::backend::Backend; use crate::lsp::backend::ConsoleInputs; use crate::lsp::backend::LspEvent; +use crate::lsp::backend::TokioUnboundedSender; use crate::lsp::events::EVENTS; use crate::modules; use crate::plots::graphics_device; @@ -143,7 +141,6 @@ pub fn start_r( stdin_reply_rx: Receiver>, iopub_tx: Sender, kernel_init_tx: Bus, - lsp_runtime: Arc, dap: Arc>, ) { // Initialize global state (ensure we only do this once!) @@ -164,7 +161,6 @@ pub fn start_r( stdin_reply_rx, iopub_tx, kernel_init_tx, - lsp_runtime, dap, )); }); @@ -269,13 +265,6 @@ pub struct RMain { pub help_tx: Option>, pub help_rx: Option>, - // LSP tokio runtime used to spawn LSP tasks on the executor and the - // corresponding backend used to send LSP requests to the frontend. - // The backend is initialized on LSP start up, and is refreshed after a - // frontend reconnect. - lsp_runtime: Arc, - lsp_backend: Option, - /// Event channel for notifying the LSP. In principle, could be a Jupyter comm. lsp_events_tx: Option>, @@ -360,7 +349,6 @@ impl RMain { stdin_reply_rx: Receiver>, iopub_tx: Sender, kernel_init_tx: Bus, - lsp_runtime: Arc, dap: Arc>, ) -> Self { Self { @@ -382,8 +370,6 @@ impl RMain { error_traceback: Vec::new(), help_tx: None, help_rx: None, - lsp_runtime, - lsp_backend: None, lsp_events_tx: None, dap: RMainDap::new(dap), is_busy: false, @@ -597,6 +583,7 @@ impl RMain { }, Err(err) => log::error!("Can't retrieve console inputs: {err:?}"), } + self.send_lsp(LspEvent::RefreshAllDiagnostics()); } // Signal prompt @@ -1073,27 +1060,23 @@ impl RMain { &self.kernel } - pub fn get_lsp_runtime(&self) -> &Arc { - &self.lsp_runtime - } - - pub fn get_lsp_backend(&self) -> Option<&Backend> { - self.lsp_backend.as_ref() - } - fn send_lsp(&self, event: LspEvent) { if let Some(ref tx) = self.lsp_events_tx { tx.send(event).unwrap(); } } - pub fn set_lsp_backend( - &mut self, - backend: Backend, - lsp_events_tx: TokioUnboundedSender, - ) { - self.lsp_backend = Some(backend); - self.lsp_events_tx = Some(lsp_events_tx); + pub fn set_lsp_channel(&mut self, lsp_events_tx: TokioUnboundedSender) { + self.lsp_events_tx = Some(lsp_events_tx.clone()); + + // 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 + // is set up. Might want to do this in an idle task. + if let Ok(inputs) = console_inputs() { + self.send_lsp(LspEvent::DidChangeConsoleInputs(inputs)); + } + self.send_lsp(LspEvent::RefreshAllDiagnostics()); } pub fn call_frontend_method(&self, request: UiFrontendRequest) -> anyhow::Result { diff --git a/crates/ark/src/lsp/backend.rs b/crates/ark/src/lsp/backend.rs index 616c5c812..7c9b54142 100644 --- a/crates/ark/src/lsp/backend.rs +++ b/crates/ark/src/lsp/backend.rs @@ -30,6 +30,8 @@ use tower_lsp::LspService; use tower_lsp::Server; use tree_sitter::Point; +pub(crate) type TokioUnboundedSender = tokio::sync::mpsc::UnboundedSender; + use crate::interface::RMain; use crate::lsp::completions::provide_completions; use crate::lsp::completions::resolve_completion; @@ -42,7 +44,6 @@ use crate::lsp::encoding::get_position_encoding_kind; use crate::lsp::help_topic; use crate::lsp::hover::hover; use crate::lsp::indexer; -use crate::lsp::indexer::IndexerStateManager; use crate::lsp::selection_range::convert_selection_range_from_tree_sitter_to_lsp; use crate::lsp::selection_range::selection_range; use crate::lsp::signature_help::signature_help; @@ -65,6 +66,9 @@ macro_rules! backend_trace { #[derive(Debug)] pub enum LspEvent { DidChangeConsoleInputs(ConsoleInputs), + RefreshDiagnostics(Url, Document, WorldState), + RefreshAllDiagnostics(), + PublishDiagnostics(Url, Vec, Option), } #[derive(Clone, Debug)] @@ -75,8 +79,8 @@ pub struct Backend { /// Global world state containing all inputs for LSP analysis. pub state: WorldState, - /// Synchronisation util for indexer. - pub indexer_state_manager: IndexerStateManager, + /// Channel for communication with the LSP. + events_tx: TokioUnboundedSender, } /// Information sent from the kernel to the LSP after each top-level evaluation. @@ -120,6 +124,57 @@ impl Backend { return callback(document.value()); } + + fn did_change_console_inputs(&self, inputs: ConsoleInputs) { + *self.state.console_scopes.lock() = inputs.console_scopes; + *self.state.installed_packages.lock() = inputs.installed_packages; + } + + fn refresh_diagnostics(&self, url: Url, document: Document, state: WorldState) { + tokio::task::spawn_blocking({ + let events_tx = self.events_tx.clone(); + + move || { + let diagnostics = diagnostics::generate_diagnostics(document.clone(), state); + events_tx.send(LspEvent::PublishDiagnostics( + url, + diagnostics, + document.version, + )) + } + }); + } + + fn refresh_all_diagnostics(&self) { + for doc_ref in self.state.documents.iter() { + tokio::task::spawn_blocking({ + let url = doc_ref.key().clone(); + let document = doc_ref.value().clone(); + let version = document.version.clone(); + + let state = self.state.clone(); + let events_tx = self.events_tx.clone(); + + move || { + let diagnostics = diagnostics::generate_diagnostics(document, state); + events_tx + .send(LspEvent::PublishDiagnostics(url, diagnostics, version)) + .unwrap(); + } + }); + } + } + + async fn publish_diagnostics( + &self, + uri: Url, + diagnostics: Vec, + version: Option, + ) { + self.client + .publish_diagnostics(uri, diagnostics, version) + .await + } } #[tower_lsp::async_trait] @@ -143,8 +198,10 @@ impl LanguageServer for Backend { } } - // start indexing - indexer::start(folders, self.indexer_state_manager.clone()); + // Start indexing + tokio::task::spawn_blocking(|| { + indexer::start(folders); + }); Ok(InitializeResult { server_info: Some(ServerInfo { @@ -274,11 +331,17 @@ impl LanguageServer for Backend { let uri = params.text_document.uri; let version = params.text_document.version; - self.state - .documents - .insert(uri.clone(), Document::new(contents, Some(version))); + let document = Document::new(contents, Some(version)); + + self.state.documents.insert(uri.clone(), document.clone()); - diagnostics::refresh_diagnostics(self.clone(), uri.clone(), Some(version)); + self.events_tx + .send(LspEvent::RefreshDiagnostics( + uri, + document, + self.state.clone(), + )) + .unwrap(); } async fn did_change(&self, params: DidChangeTextDocumentParams) { @@ -309,11 +372,17 @@ impl LanguageServer for Backend { } } - // publish diagnostics - but only publish them if the version of + // Publish diagnostics - but only publish them if the version of // the document now matches the version of the change after applying // it in `on_did_change()` (i.e. no changes left in the out of order queue) if params.text_document.version == version { - diagnostics::refresh_diagnostics(self.clone(), uri.clone(), Some(version)); + self.events_tx + .send(LspEvent::RefreshDiagnostics( + uri.clone(), + doc.clone(), + self.state.clone(), + )) + .unwrap(); } } @@ -326,7 +395,10 @@ impl LanguageServer for Backend { let uri = params.text_document.uri; - diagnostics::clear_diagnostics(self.clone(), uri.clone(), None); + // Publish empty set of diagnostics to clear them + self.client + .publish_diagnostics(uri.clone(), Vec::new(), None) + .await; match self.state.documents.remove(&uri) { Some(_) => { @@ -584,33 +656,40 @@ pub fn start_lsp(runtime: Arc, address: String, conn_init_tx: Sender(); + // Create backend. // Note that DashMap uses synchronization primitives internally, so we // don't guard access to the map via a mutex. let backend = Backend { client, + events_tx: events_tx.clone(), state: WorldState { documents: Arc::new(DashMap::new()), workspace: Arc::new(Mutex::new(Workspace::default())), console_scopes: Arc::new(Mutex::new(vec![])), installed_packages: Arc::new(Mutex::new(vec![])), }, - indexer_state_manager: IndexerStateManager::new(), }; - let (events_tx, mut events_rx) = tokio_unbounded_channel::(); - - // LSP event loop. To be integrated in our synchronising dispatcher - // once implemented. + // Watcher task for LSP events. To be integrated in our + // synchronising dispatcher once implemented. tokio::spawn({ let backend = backend.clone(); async move { loop { match events_rx.recv().await.unwrap() { LspEvent::DidChangeConsoleInputs(inputs) => { - *backend.state.console_scopes.lock() = inputs.console_scopes; - *backend.state.installed_packages.lock() = - inputs.installed_packages; + backend.did_change_console_inputs(inputs); + }, + LspEvent::RefreshDiagnostics(url, document, state) => { + backend.refresh_diagnostics(url, document, state); + }, + LspEvent::RefreshAllDiagnostics() => { + backend.refresh_all_diagnostics(); + }, + LspEvent::PublishDiagnostics(uri, diagnostics, version) => { + backend.publish_diagnostics(uri, diagnostics, version).await; }, } } @@ -624,10 +703,10 @@ pub fn start_lsp(runtime: Arc, address: String, conn_init_tx: Sender DiagnosticContext<'a> { } } -/// Clear the diagnostics of a single file -/// -/// Note that we don't reference the `document` in the DashMap in any way, -/// in case it has already been removed by the time the thread runs. -/// -/// Must be called from an LSP method so it runs on the LSP tokio `Runtime` -pub fn clear_diagnostics(backend: Backend, uri: Url, version: Option) { - tokio::spawn(async move { - // Empty set to clear them - let diagnostics = Vec::new(); - - backend - .client - .publish_diagnostics(uri.clone(), diagnostics, version) - .await - }); -} - -/// Refresh the diagnostics of a single file -/// -/// Must be called from an LSP method so it runs on the LSP tokio `Runtime` -pub fn refresh_diagnostics(backend: Backend, uri: Url, version: Option) { - tokio::spawn(async move { - refresh_diagnostics_impl(backend, uri, version).await; - }); -} - -/// Request a full diagnostic refresh on all open documents -/// -/// Called after each R console execution so diagnostics are dynamic to code sent to the -/// console. -/// -/// Still goes through `request_diagnostics()` with its 1 second delay before actually -/// generating diagnostics. This avoids being too aggressive with the refresh, since -/// generating diagnostics does require R. -pub fn refresh_all_open_file_diagnostics() { - r_async_task(|| { - let main = RMain::get(); - - let Some(backend) = main.get_lsp_backend() else { - log::error!("No LSP `backend` to request a diagnostic refresh with."); - return; - }; - - let runtime = main.get_lsp_runtime(); - - for document in backend.state.documents.iter() { - let backend = backend.clone(); - let uri = document.key().clone(); - let version = document.version.clone(); - - // Explicit `drop()` before we request diagnostics, which requires `get()`ting - // this document again on the thread. Likely not needed, but better to be safe. - drop(document); - - runtime.spawn(async move { - refresh_diagnostics_impl(backend, uri, version).await; - }); - } - }); -} - -async fn refresh_diagnostics_impl(backend: Backend, uri: Url, version: Option) { - let diagnostics = match request_diagnostics(&backend, &uri).await { - Ok(diagnostics) => diagnostics, - Err(err) => { - log::error!("While refreshing diagnostics for '{uri}': {err:?}"); - return; - }, - }; - - let Some(diagnostics) = diagnostics else { - // File was closed or `version` changed. Not an error, just a side effect - // of delaying diagnostics. - return; - }; - - backend - .client - .publish_diagnostics(uri.clone(), diagnostics, version) - .await -} - -async fn request_diagnostics( - backend: &Backend, - uri: &Url, -) -> anyhow::Result>> { - // SAFETY: It is absolutely imperative that the `doc` be `Drop`ped outside - // of any `await` context. That is why the extraction of `doc` is captured - // inside of `try_generate_diagnostics()` and `get_diagnostics_id()`; this ensures - // that any `doc` is `Drop`ped before the `sleep().await` call. If this doesn't - // happen, then the `await` could switch us to a different LSP task, which will also - // try and access a document, causing a deadlock since it won't be able to access a - // document until our `doc` reference is dropped, but we can't drop until we get - // control back from the `await`. - - // Get the `diagnostics_id` for this request, before sleeping - let diagnostics_id = get_diagnostics_id(backend, uri)?; - - // Wait some amount of time. Note that the `diagnostics_id` is updated on every - // diagnostic request, so if another request comes in while this task is waiting, - // we'll see that the current `diagnostics_id` is now past the id associated with this - // request and toss it away. - tokio::time::sleep(Duration::from_millis(1000)).await; - - Ok(try_generate_diagnostics(backend, uri, diagnostics_id)) -} - -fn get_diagnostics_id(backend: &Backend, uri: &Url) -> anyhow::Result { - let Some(mut document) = backend.state.documents.get_mut(uri) else { - return Err(anyhow!("Unknown document URI '{uri}'.")); - }; - - // First, bump the id to correspond to this request - document.diagnostics_id += 1; - - // Return the bumped id - Ok(document.diagnostics_id) -} - -fn try_generate_diagnostics( - backend: &Backend, - uri: &Url, - diagnostics_id: i64, -) -> Option> { - // Get reference to document. - // At this point we already know this document existed before we slept, so if it - // doesn't exist now, that is because it must have been closed, so if that occurs - // then simply return. - let Some(doc) = backend.state.documents.get(uri) else { - log::info!("Document with uri '{uri}' no longer exists after diagnostics delay. It was likely closed."); - return None; - }; - - // Check if the `diagnostics_id` has been bumped by another diagnostics request while - // we were asleep - let current_diagnostics_id = doc.diagnostics_id; - if diagnostics_id != current_diagnostics_id { - // log::info!("[diagnostics({diagnostics_id}, {uri})] Aborting diagnostics in favor of id {current_diagnostics_id}."); - return None; - } - - // If we've made it this far, we really do want diagnostics, and we want them to - // be accurate. The indexer is a very important part of our diagnostics, so we need - // it to finish an initial run before we generate any diagnostics, otherwise they - // can be pretty bad and annoying. Importantly, we place this check after the 1 sec - // timeout delay and version check to ensure that the `lock()` doesn't run needlessly. - backend.indexer_state_manager.wait_until_initialized(); - - Some(generate_diagnostics(&doc, &backend.state)) -} - -fn generate_diagnostics(doc: &Document, state: &WorldState) -> Vec { +pub fn generate_diagnostics(doc: Document, state: WorldState) -> Vec { let mut diagnostics = Vec::new(); { @@ -1334,7 +1176,7 @@ mod tests { 2 # hi there )"; let document = Document::new(text, None); - let diagnostics = generate_diagnostics(&document, &DEFAULT_STATE); + let diagnostics = generate_diagnostics(document, DEFAULT_STATE.clone()); assert!(diagnostics.is_empty()); }) } @@ -1345,7 +1187,7 @@ mod tests { let text = "match(1, 2 3)"; let document = Document::new(text, None); - let diagnostics = generate_diagnostics(&document, &DEFAULT_STATE); + let diagnostics = generate_diagnostics(document, DEFAULT_STATE.clone()); assert_eq!(diagnostics.len(), 1); let diagnostic = diagnostics.get(0).unwrap(); @@ -1372,12 +1214,12 @@ mod tests { let text = "x$foo"; let document = Document::new(text, None); - let diagnostics = generate_diagnostics(&document, &state); + let diagnostics = generate_diagnostics(document.clone(), state.clone()); assert!(diagnostics.is_empty()); let text = "x@foo"; let document = Document::new(text, None); - let diagnostics = generate_diagnostics(&document, &state); + let diagnostics = generate_diagnostics(document.clone(), state.clone()); assert!(diagnostics.is_empty()); // Clean up @@ -1395,7 +1237,7 @@ mod tests { y + x + z "; let document = Document::new(text, None); - let diagnostics = generate_diagnostics(&document, &DEFAULT_STATE); + let diagnostics = generate_diagnostics(document.clone(), DEFAULT_STATE.clone()); assert!(diagnostics.is_empty()); }) } @@ -1409,7 +1251,7 @@ mod tests { y + x "; let document = Document::new(text, None); - let diagnostics = generate_diagnostics(&document, &DEFAULT_STATE); + let diagnostics = generate_diagnostics(document.clone(), DEFAULT_STATE.clone()); assert!(diagnostics.is_empty()); }) } @@ -1424,7 +1266,7 @@ mod tests { "; let document = Document::new(text, None); - let diagnostics = generate_diagnostics(&document, &DEFAULT_STATE); + let diagnostics = generate_diagnostics(document.clone(), DEFAULT_STATE.clone()); assert_eq!(diagnostics.len(), 1); // Only marks the `x` before the `x <- 1` diff --git a/crates/ark/src/lsp/documents.rs b/crates/ark/src/lsp/documents.rs index 718eaeaf3..977c37a83 100644 --- a/crates/ark/src/lsp/documents.rs +++ b/crates/ark/src/lsp/documents.rs @@ -49,12 +49,6 @@ pub struct Document { // None if the document hasn't been synchronized yet. pub version: Option, - // An identifier tied to the current diagnostic request. Used during diagnostic - // debouncing to determine if diagnostics should still proceed after a short delay. - // Can't be `version` as diagnostics can be requested even if the document itself - // hasn't changed (i.e. after console execution). - pub diagnostics_id: i64, - // The parser used to generate the AST. TODO: Once LSP handlers are // properly synchronised, remove the RwLock. pub parser: Arc>, @@ -89,14 +83,11 @@ impl Document { let pending = Vec::new(); - let diagnostics_id = 0; - // return generated document Self { contents: document, pending, version, - diagnostics_id, parser: Arc::new(RwLock::new(parser)), ast, } diff --git a/crates/ark/src/lsp/handler.rs b/crates/ark/src/lsp/handler.rs index 9611b3ce7..a98f47af6 100644 --- a/crates/ark/src/lsp/handler.rs +++ b/crates/ark/src/lsp/handler.rs @@ -24,9 +24,9 @@ pub struct Lsp { } impl Lsp { - pub fn new(runtime: Arc, kernel_init_rx: BusReader) -> Self { + pub fn new(kernel_init_rx: BusReader) -> Self { Self { - runtime, + runtime: Arc::new(tokio::runtime::Runtime::new().unwrap()), kernel_init_rx, kernel_initialized: false, } diff --git a/crates/ark/src/lsp/indexer.rs b/crates/ark/src/lsp/indexer.rs index 4532f0b0e..d02a4414b 100644 --- a/crates/ark/src/lsp/indexer.rs +++ b/crates/ark/src/lsp/indexer.rs @@ -10,13 +10,9 @@ use std::path::Path; use std::result::Result::Ok; use std::sync::Arc; use std::sync::Mutex; -use std::sync::Once; -use std::time::Duration; use std::time::SystemTime; use anyhow::*; -use crossbeam::channel::Receiver; -use crossbeam::channel::Sender; use lazy_static::lazy_static; use log::*; use regex::Regex; @@ -35,50 +31,6 @@ use crate::treesitter::BinaryOperatorType; use crate::treesitter::NodeType; use crate::treesitter::NodeTypeExt; -#[derive(Clone, Debug)] -pub struct IndexerStateManager { - init_tx: Arc>>, - init_rx: Arc>>, -} - -impl IndexerStateManager { - pub fn new() -> Self { - let (init_tx, init_rx) = crossbeam::channel::bounded(1); - - let init_tx = Arc::new(Mutex::new(init_tx)); - let init_rx = Arc::new(Mutex::new(init_rx)); - - Self { init_tx, init_rx } - } - - pub fn initialize(&self) { - let init_tx = self.init_tx.lock().unwrap(); - init_tx.send(()).unwrap(); - } - - pub fn wait_until_initialized(&self) { - // Ensures that only 1 thread can call the initialization function. - // All other calling threads get blocked until the initializer has run. - // All subsequent calls essentially become no-ops. - static ONCE: Once = std::sync::Once::new(); - - ONCE.call_once(|| { - let init_rx = self.init_rx.lock().unwrap(); - - match init_rx.recv_timeout(Duration::from_secs(30)) { - Ok(_) => { - log::info!( - "Received signal that indexer was initialized, proceeding with diagnostics." - ) - }, - Err(err) => log::error!( - "Indexer wasn't initialized after 30 seconds, proceeding with diagnostics. {err:?}" - ), - }; - }) - } -} - #[derive(Clone, Debug)] pub enum IndexEntryData { Function { @@ -108,32 +60,26 @@ lazy_static! { static ref RE_COMMENT_SECTION: Regex = Regex::new(r"^\s*(#+)\s*(.*?)\s*[#=-]{4,}\s*$").unwrap(); } -pub fn start(folders: Vec, indexer_state_manager: IndexerStateManager) { - // create a task that indexes these folders - let _handle = tokio::spawn(async move { - let now = SystemTime::now(); - info!("Indexing started."); - - for folder in folders { - let walker = WalkDir::new(folder); - for entry in walker.into_iter().filter_entry(|e| filter_entry(e)) { - if let Ok(entry) = entry { - if entry.file_type().is_file() { - if let Err(error) = index_file(entry.path()) { - error!("{:?}", error); - } +pub fn start(folders: Vec) { + let now = SystemTime::now(); + info!("Indexing started."); + + for folder in folders { + let walker = WalkDir::new(folder); + for entry in walker.into_iter().filter_entry(|e| filter_entry(e)) { + if let Ok(entry) = entry { + if entry.file_type().is_file() { + if let Err(error) = index_file(entry.path()) { + error!("{:?}", error); } } } } + } - if let Ok(elapsed) = now.elapsed() { - info!("Indexing finished after {:?}.", elapsed); - } - - // Send notification that indexer has finished initial indexing - indexer_state_manager.initialize(); - }); + if let Ok(elapsed) = now.elapsed() { + info!("Indexing finished after {:?}.", elapsed); + } } pub fn find(symbol: &str) -> Option<(String, IndexEntry)> { diff --git a/crates/ark/src/main.rs b/crates/ark/src/main.rs index ac8db7b6e..9e509948e 100644 --- a/crates/ark/src/main.rs +++ b/crates/ark/src/main.rs @@ -74,18 +74,10 @@ fn start_kernel( let (r_request_tx, r_request_rx) = bounded::(1); let (kernel_request_tx, kernel_request_rx) = bounded::(1); - // The tokio runtime used to manage LSP task execution. - // Used in the main LSP thread and from R callbacks. - // Not wrapped in a `Mutex` since none of the methods require a mutable reference. - let lsp_runtime = Arc::new(tokio::runtime::Runtime::new().unwrap()); - // Create the LSP and DAP clients. // Not all Amalthea kernels provide these, but ark does. // They must be able to deliver messages to the shell channel directly. - let lsp = Arc::new(Mutex::new(lsp::handler::Lsp::new( - Arc::clone(&lsp_runtime), - kernel_init_tx.add_rx(), - ))); + let lsp = Arc::new(Mutex::new(lsp::handler::Lsp::new(kernel_init_tx.add_rx()))); // DAP needs the `RRequest` channel to communicate with // `read_console()` and send commands to the debug interpreter @@ -151,7 +143,6 @@ fn start_kernel( stdin_reply_rx, iopub_tx, kernel_init_tx, - lsp_runtime, dap, ) } diff --git a/crates/ark/src/shell.rs b/crates/ark/src/shell.rs index 944b8ade8..2262232b4 100644 --- a/crates/ark/src/shell.rs +++ b/crates/ark/src/shell.rs @@ -51,7 +51,6 @@ use crate::help::r_help::RHelp; use crate::interface::KernelInfo; use crate::interface::RMain; use crate::kernel::Kernel; -use crate::lsp::diagnostics; use crate::plots::graphics_device; use crate::r_task; use crate::request::KernelRequest; @@ -238,8 +237,6 @@ impl ShellHandler for Shell { ) }; - diagnostics::refresh_all_open_file_diagnostics(); - // Check for changes to the working directory if let Err(err) = kernel.poll_working_directory() { warn!("Error polling working directory: {}", err); From 6662cb2f1108934f9987c4d9167b34d7f3e21dd2 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Mon, 13 May 2024 07:44:25 +0200 Subject: [PATCH 4/7] Reveal file path on indexer error --- crates/ark/src/lsp/indexer.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/ark/src/lsp/indexer.rs b/crates/ark/src/lsp/indexer.rs index d02a4414b..8ab103c1d 100644 --- a/crates/ark/src/lsp/indexer.rs +++ b/crates/ark/src/lsp/indexer.rs @@ -69,8 +69,8 @@ pub fn start(folders: Vec) { for entry in walker.into_iter().filter_entry(|e| filter_entry(e)) { if let Ok(entry) = entry { if entry.file_type().is_file() { - if let Err(error) = index_file(entry.path()) { - error!("{:?}", error); + if let Err(err) = index_file(entry.path()) { + error!("Can't index file {:?}: {err:?}", entry.path()); } } } From df05e7578e33ff2043e7480d0c50724c4c12486c Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Wed, 29 May 2024 13:26:28 +0200 Subject: [PATCH 5/7] Extract `refresh_lsp()` --- crates/ark/src/interface.rs | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/crates/ark/src/interface.rs b/crates/ark/src/interface.rs index 8fd963de5..e8d3ae050 100644 --- a/crates/ark/src/interface.rs +++ b/crates/ark/src/interface.rs @@ -577,13 +577,7 @@ impl RMain { // here, but only containing high-level information such as `search()` // contents and `ls(rho)`. if !info.browser && !info.incomplete && !info.input_request { - match console_inputs() { - Ok(inputs) => { - self.send_lsp(LspEvent::DidChangeConsoleInputs(inputs)); - }, - Err(err) => log::error!("Can't retrieve console inputs: {err:?}"), - } - self.send_lsp(LspEvent::RefreshAllDiagnostics()); + self.refresh_lsp(); } // Signal prompt @@ -1072,9 +1066,16 @@ impl RMain { // 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 - // is set up. Might want to do this in an idle task. - if let Ok(inputs) = console_inputs() { - self.send_lsp(LspEvent::DidChangeConsoleInputs(inputs)); + // is set up. We'll want to do this in an idle task. + self.refresh_lsp(); + } + + pub fn refresh_lsp(&self) { + match console_inputs() { + Ok(inputs) => { + self.send_lsp(LspEvent::DidChangeConsoleInputs(inputs)); + }, + Err(err) => log::error!("Can't retrieve console inputs: {err:?}"), } self.send_lsp(LspEvent::RefreshAllDiagnostics()); } From f53777edbb09b9e43abf67fd621c51800f8dee3c Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Wed, 29 May 2024 13:40:58 +0200 Subject: [PATCH 6/7] Clone version instead of the whole document --- crates/ark/src/lsp/backend.rs | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/crates/ark/src/lsp/backend.rs b/crates/ark/src/lsp/backend.rs index 7c9b54142..80c6177cb 100644 --- a/crates/ark/src/lsp/backend.rs +++ b/crates/ark/src/lsp/backend.rs @@ -135,12 +135,9 @@ impl Backend { let events_tx = self.events_tx.clone(); move || { - let diagnostics = diagnostics::generate_diagnostics(document.clone(), state); - events_tx.send(LspEvent::PublishDiagnostics( - url, - diagnostics, - document.version, - )) + let version = document.version.clone(); + let diagnostics = diagnostics::generate_diagnostics(document, state); + events_tx.send(LspEvent::PublishDiagnostics(url, diagnostics, version)) } }); } From f01c6704557f5e2e57815bfc30a76fa1876931a6 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Wed, 29 May 2024 14:04:38 +0200 Subject: [PATCH 7/7] Finish removing `library()` diagnostics --- crates/ark/src/lsp/diagnostics.rs | 145 ------------------ crates/ark/src/lsp/mod.rs | 1 - crates/ark/src/lsp/treesitter.rs | 38 ----- crates/ark/src/modules/positron/diagnostics.R | 106 ------------- crates/ark/src/modules/positron/treesitter.R | 16 -- 5 files changed, 306 deletions(-) delete mode 100644 crates/ark/src/lsp/treesitter.rs delete mode 100644 crates/ark/src/modules/positron/diagnostics.R delete mode 100644 crates/ark/src/modules/positron/treesitter.R diff --git a/crates/ark/src/lsp/diagnostics.rs b/crates/ark/src/lsp/diagnostics.rs index 16719d2f5..3776490dd 100644 --- a/crates/ark/src/lsp/diagnostics.rs +++ b/crates/ark/src/lsp/diagnostics.rs @@ -601,151 +601,6 @@ fn recurse_call_arguments_default( ().ok() } -// This is commented out because: -// -// - The package not installed lint is a bit too distracting. Should it become -// an assist? -// https://github.com/posit-dev/positron/issues/2672 -// - We'd like to avoid running R code during diagnostics -// https://github.com/posit-dev/positron/issues/2321 -// - The diagnostic meshes R and tree-sitter objects in a way that's not -// perfectly safe and we have a known crash logged: -// https://github.com/posit-dev/positron/issues/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. - -// struct TreeSitterCall<'a> { -// // A call of the form (list(0L, ), foo = list(1L, )) -// pub call: RObject, -// node_phantom: PhantomData<&'a Node<'a>>, -// } -// -// impl<'a> TreeSitterCall<'a> { -// pub unsafe fn new( -// node: Node<'a>, -// function: &str, -// context: &mut DiagnosticContext, -// ) -> Result { -// // start with a call to the function: () -// let sym = r_symbol!(function); -// let call = RObject::new(Rf_lang1(sym)); -// -// // then augment it with arguments -// let mut tail = *call; -// -// if let Some(arguments) = node.child_by_field_name("arguments") { -// let mut cursor = arguments.walk(); -// let children = arguments.children_by_field_name("argument", &mut cursor); -// let mut i = 0; -// for child in children { -// let arg_list = RObject::from(Rf_allocVector(VECSXP, 2)); -// -// // set the argument to a list<2>, with its first element: a scalar integer -// // that corresponds to its O-based position. The position is used below to -// // map back to the Node -// SET_VECTOR_ELT(*arg_list, 0, Rf_ScalarInteger(i as i32)); -// -// // Set the second element of the list to an external pointer -// // to the child node. -// if let Some(value) = child.child_by_field_name("value") { -// // TODO: Wrap this in a nice constructor -// let node_size = std::mem::size_of::(); -// let node_storage = Rf_allocVector(RAWSXP, node_size as isize); -// SET_VECTOR_ELT(*arg_list, 1, node_storage); -// -// let p_node_storage: *mut Node<'a> = RAW(node_storage) as *mut Node<'a>; -// std::ptr::copy_nonoverlapping(&value, p_node_storage, 1); -// } -// -// SETCDR(tail, Rf_cons(*arg_list, R_NilValue)); -// tail = CDR(tail); -// -// // potentially add the argument name -// if let Some(name) = child.child_by_field_name("name") { -// let name = context.contents.node_slice(&name)?.to_string(); -// let sym_name = r_symbol!(name); -// SET_TAG(tail, sym_name); -// } -// -// i = i + 1; -// } -// } -// -// Ok(Self { -// call, -// node_phantom: PhantomData, -// }) -// } -// } -// -// fn recurse_call_arguments_custom( -// node: Node, -// context: &mut DiagnosticContext, -// diagnostics: &mut Vec, -// function: &str, -// diagnostic_function: &str, -// ) -> Result<()> { -// r_task(|| unsafe { -// // Build a call that mixes treesitter nodes (as external pointers) -// // library(foo, pos = 2 + 2) -// // -> -// // library([0, ], pos = [1, ]) -// // where: -// // - node0 is an external pointer to a treesitter Node for the identifier `foo` -// // - node1 is an external pointer to a treesitter Node for the call `2 + 2` -// // -// // The TreeSitterCall object holds on to the nodes, so that they can be -// // safely passed down to the R side as external pointers -// let call = TreeSitterCall::new(node, function, context)?; -// -// let custom_diagnostics = RFunction::from(diagnostic_function) -// .add(r_expr_quote(call.call)) -// .add(ExternalPointer::new(context.contents)) -// .call()?; -// -// if !r_is_null(*custom_diagnostics) { -// let n = Rf_xlength(*custom_diagnostics); -// for i in 0..n { -// // diag is a list with: -// // - The kind of diagnostic: skip, default, simple -// // - The node external pointer, i.e. the ones made in TreeSitterCall::new -// // - The message, when kind is "simple" -// let diag = VECTOR_ELT(*custom_diagnostics, i); -// -// let kind: String = RObject::view(VECTOR_ELT(diag, 0)).try_into()?; -// -// if kind == "skip" { -// // skip the diagnostic entirely, e.g. -// // library(foo) -// // ^^^ -// continue; -// } -// -// let ptr = VECTOR_ELT(diag, 1); -// let value: Node<'static> = *(RAW(ptr) as *mut Node<'static>); -// -// if kind == "default" { -// // the R side gives up, so proceed as normal, e.g. -// // library(foo, pos = ...) -// // ^^^ -// recurse(value, context, diagnostics)?; -// } else if kind == "simple" { -// // Simple diagnostic from R, e.g. -// // library("ggplot3") -// // ^^^^^^^ Package 'ggplot3' is not installed -// let message: String = RObject::view(VECTOR_ELT(diag, 2)).try_into()?; -// let range = value.range(); -// let range = convert_tree_sitter_range_to_lsp_range(context.contents, range); -// let diagnostic = Diagnostic::new_simple(range, message.into()); -// diagnostics.push(diagnostic); -// } -// } -// } -// -// ().ok() -// }) -// } - fn recurse_call( node: Node, context: &mut DiagnosticContext, diff --git a/crates/ark/src/lsp/mod.rs b/crates/ark/src/lsp/mod.rs index dcb4a0af7..1adf7bfa2 100644 --- a/crates/ark/src/lsp/mod.rs +++ b/crates/ark/src/lsp/mod.rs @@ -27,5 +27,4 @@ pub mod state; pub mod statement_range; pub mod symbols; pub mod traits; -pub mod treesitter; pub mod util; diff --git a/crates/ark/src/lsp/treesitter.rs b/crates/ark/src/lsp/treesitter.rs deleted file mode 100644 index 122e86638..000000000 --- a/crates/ark/src/lsp/treesitter.rs +++ /dev/null @@ -1,38 +0,0 @@ -// -// treesitter.rs -// -// Copyright (C) 2023 Posit Software, PBC. All rights reserved. -// -// - -use harp::external_ptr::ExternalPointer; -use harp::object::RObject; -use libr::RAW; -use libr::SEXP; -use ropey::Rope; -use tree_sitter::Node; - -use crate::lsp::traits::rope::RopeExt; - -#[harp::register] -pub unsafe extern "C" fn ps_treesitter_node_text( - ffi_node: SEXP, - ffi_contents: SEXP, -) -> anyhow::Result { - let node: Node<'static> = *(RAW(ffi_node) as *mut Node<'static>); - let contents = ExternalPointer::::reference(ffi_contents); - - let text = contents - .node_slice(&node) - .map(|slice| slice.to_string()) - .unwrap_or(String::from("")); - - Ok(*RObject::from(text)) -} - -#[harp::register] -pub unsafe extern "C" fn ps_treesitter_node_kind(ffi_node: SEXP) -> anyhow::Result { - let node: Node<'static> = *(RAW(ffi_node) as *mut Node<'static>); - - Ok(*RObject::from(node.kind())) -} diff --git a/crates/ark/src/modules/positron/diagnostics.R b/crates/ark/src/modules/positron/diagnostics.R deleted file mode 100644 index a429011e7..000000000 --- a/crates/ark/src/modules/positron/diagnostics.R +++ /dev/null @@ -1,106 +0,0 @@ -# -# completions.R -# -# Copyright (C) 2022-2024 Posit Software, PBC. All rights reserved. -# -# - -#' @export -.ps.diagnostics.diagnostic <- function(kind = "skip", node = NULL, message = NULL) { - list(kind, node, message) -} - -#' @export -.ps.diagnostics.custom.library <- function(call, contents, fun = base::library) { - # TODO: it could be interesting to have a diagnostic - # when this will fail on wrong argument, e.g. - # library(pkg = ggplot2) - # ^^^ : unused argument (pkg) - matched_call <- match.call(fun, call) - - # here we get a call where arguments are named, e.g. - # library(package = ) where is a list of 2 things: - # - a 0-based integer position for this argument. This is not - # currently used - # - an external pointer to a treesitter Node, which can be queried - # with .ps.treesitter.node.text() and .ps.treesitter.node.kind() - # - # We might simplify and only pass around the external pointer if - # we realize the position isn't useful. - - # identify if character.only was set, so that we can - # adapt the diagnostic appropriately - is_character_only <- function(matched_call, contents) { - character_only <- matched_call[["character.only"]] - - if (is.null(character_only)) { - FALSE - } else { - ptr <- character_only[[2L]] - text <- .ps.treesitter.node.text(ptr, contents) - !identical(text, "FALSE") - } - } - - # deal with arguments `package` and `help` which use - # non standard evaluation, e.g. library(ggplot2) - diagnostic_package <- function(arg, contents, character_only) { - index <- arg[[1L]] - node <- arg[[2L]] - - kind <- .ps.treesitter.node.kind(node) - - # library(foo, character.only = TRUE) - if (kind == "identifier" && character_only) { - return(.ps.diagnostics.diagnostic("default", node)) - } - - if (kind %in% c("string", "identifier")) { - # library("foo") or library(foo) - pkg <- .ps.treesitter.node.text(node, contents) - - if (kind == "string") { - pkg <- gsub("^(['\"])(.*)\\1$", "\\2", pkg) - } - - # The package is installed, just skip the diagnostic - if (pkg %in% base::.packages(all.available = TRUE)) { - return(.ps.diagnostics.diagnostic("skip")) - } - - msg <- sprintf("Package '%s' is not installed", pkg) - return(.ps.diagnostics.diagnostic("simple", node, message = msg)) - } - - .ps.diagnostics.diagnostic("default", node) - } - - # Before scanning all arguments, we need to check if - # character.only is set, so that we can adapt how the - # package and help arguments are handled - character_only <- is_character_only(matched_call, contents) - - # Scan the given arguments and make diagnostics for each - n <- length(matched_call) - out <- vector("list", n - 1L) - names <- names(matched_call) - for (i in seq_len(n - 1L)) { - arg <- matched_call[[i + 1L]] - name <- names[[i + 1L]] - - diagnostic <- if (name %in% c("package", "help")) { - diagnostic_package(arg, contents, character_only) - } else { - .ps.diagnostics.diagnostic("default", node = arg[[2L]]) - } - - out[[i]] <- diagnostic - } - - out -} - -#' @export -.ps.diagnostics.custom.require <- function(call, contents, fun = base::require) { - .ps.diagnostics.custom.library(call, contents, fun) -} diff --git a/crates/ark/src/modules/positron/treesitter.R b/crates/ark/src/modules/positron/treesitter.R deleted file mode 100644 index 0df65b88f..000000000 --- a/crates/ark/src/modules/positron/treesitter.R +++ /dev/null @@ -1,16 +0,0 @@ -# -# treesitter.R -# -# Copyright (C) 2023-2024 Posit Software, PBC. All rights reserved. -# -# - -#' @export -.ps.treesitter.node.text <- function(node, contents) { - .ps.Call("ps_treesitter_node_text", node, contents) -} - -#' @export -.ps.treesitter.node.kind <- function(node) { - .ps.Call("ps_treesitter_node_kind", node) -}