diff --git a/Cargo.lock b/Cargo.lock index 0f6dcd098..e926f3dd2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -283,6 +283,7 @@ dependencies = [ "actix-web", "amalthea", "anyhow", + "assert_matches", "async-trait", "base64 0.21.0", "bus", diff --git a/crates/ark/Cargo.toml b/crates/ark/Cargo.toml index 2a8b1b6e3..333a20e39 100644 --- a/crates/ark/Cargo.toml +++ b/crates/ark/Cargo.toml @@ -66,6 +66,7 @@ tracing-error = "0.2.0" insta = { version = "1.39.0" } stdext = { path = "../stdext", features = ["testing"] } tempfile = "3.13.0" +assert_matches = "1.5.0" [build-dependencies] cc = "1.1.22" diff --git a/crates/ark/src/lsp/config.rs b/crates/ark/src/lsp/config.rs index 0628ec8c2..f4da4ff00 100644 --- a/crates/ark/src/lsp/config.rs +++ b/crates/ark/src/lsp/config.rs @@ -33,6 +33,14 @@ pub static GLOBAL_SETTINGS: &[Setting] = &[ .unwrap_or_else(|| SymbolsConfig::default().include_assignments_in_blocks) }, }, + Setting { + key: "positron.r.workspaceSymbols.includeCommentSections", + set: |cfg, v| { + cfg.workspace_symbols.include_comment_sections = v + .as_bool() + .unwrap_or_else(|| WorkspaceSymbolsConfig::default().include_comment_sections) + }, + }, ]; /// These document settings are updated on a URI basis. Each document has its @@ -77,6 +85,7 @@ pub static DOCUMENT_SETTINGS: &[Setting] = &[ pub(crate) struct LspConfig { pub(crate) diagnostics: DiagnosticsConfig, pub(crate) symbols: SymbolsConfig, + pub(crate) workspace_symbols: WorkspaceSymbolsConfig, } #[derive(Serialize, Deserialize, Clone, Debug)] @@ -85,6 +94,12 @@ pub struct SymbolsConfig { pub include_assignments_in_blocks: bool, } +#[derive(Serialize, Deserialize, Clone, Debug)] +pub struct WorkspaceSymbolsConfig { + /// Whether to include sections like `# My section ---` in workspace symbols. + pub include_comment_sections: bool, +} + /// Configuration of a document. /// /// The naming follows where possible. @@ -120,6 +135,14 @@ impl Default for SymbolsConfig { } } +impl Default for WorkspaceSymbolsConfig { + fn default() -> Self { + Self { + include_comment_sections: false, + } + } +} + impl Default for IndentationConfig { fn default() -> Self { Self { diff --git a/crates/ark/src/lsp/definitions.rs b/crates/ark/src/lsp/definitions.rs index eb26e3dc7..479676c57 100644 --- a/crates/ark/src/lsp/definitions.rs +++ b/crates/ark/src/lsp/definitions.rs @@ -20,7 +20,7 @@ use crate::lsp::traits::node::NodeExt; use crate::lsp::traits::rope::RopeExt; use crate::treesitter::NodeTypeExt; -pub unsafe fn goto_definition<'a>( +pub fn goto_definition<'a>( document: &'a Document, params: GotoDefinitionParams, ) -> Result> { @@ -75,3 +75,88 @@ pub unsafe fn goto_definition<'a>( let response = GotoDefinitionResponse::Link(vec![link]); Ok(Some(response)) } + +#[cfg(test)] +mod tests { + use assert_matches::assert_matches; + use tower_lsp::lsp_types; + + use super::*; + use crate::lsp::documents::Document; + use crate::lsp::indexer; + use crate::lsp::util::test_path; + + #[test] + fn test_goto_definition() { + let _guard = indexer::ResetIndexerGuard; + + let code = r#" +foo <- 42 +print(foo) +"#; + let doc = Document::new(code, None); + let (path, uri) = test_path(); + + indexer::update(&doc, &path).unwrap(); + + let params = GotoDefinitionParams { + text_document_position_params: lsp_types::TextDocumentPositionParams { + text_document: lsp_types::TextDocumentIdentifier { uri }, + position: lsp_types::Position::new(2, 7), + }, + work_done_progress_params: Default::default(), + partial_result_params: Default::default(), + }; + + assert_matches!( + goto_definition(&doc, params).unwrap(), + Some(GotoDefinitionResponse::Link(ref links)) => { + assert_eq!( + links[0].target_range, + lsp_types::Range { + start: lsp_types::Position::new(1, 0), + end: lsp_types::Position::new(1, 3), + } + ); + } + ); + } + + #[test] + fn test_goto_definition_comment_section() { + let _guard = indexer::ResetIndexerGuard; + + let code = r#" +# foo ---- +foo <- 1 +print(foo) +"#; + let doc = Document::new(code, None); + let (path, uri) = test_path(); + + indexer::update(&doc, &path).unwrap(); + + let params = lsp_types::GotoDefinitionParams { + text_document_position_params: lsp_types::TextDocumentPositionParams { + text_document: lsp_types::TextDocumentIdentifier { uri }, + position: lsp_types::Position::new(3, 7), + }, + work_done_progress_params: Default::default(), + partial_result_params: Default::default(), + }; + + assert_matches!( + goto_definition(&doc, params).unwrap(), + Some(lsp_types::GotoDefinitionResponse::Link(ref links)) => { + // The section should is not the target, the variable has priority + assert_eq!( + links[0].target_range, + lsp_types::Range { + start: lsp_types::Position::new(2, 0), + end: lsp_types::Position::new(2, 3), + } + ); + } + ); + } +} diff --git a/crates/ark/src/lsp/handlers.rs b/crates/ark/src/lsp/handlers.rs index 64bca86f3..9cc2f90e2 100644 --- a/crates/ark/src/lsp/handlers.rs +++ b/crates/ark/src/lsp/handlers.rs @@ -129,8 +129,9 @@ pub(crate) async fn handle_initialized( #[tracing::instrument(level = "info", skip_all)] pub(crate) fn handle_symbol( params: WorkspaceSymbolParams, + state: &WorldState, ) -> anyhow::Result>> { - symbols::symbols(¶ms) + symbols::symbols(¶ms, state) .map(|res| Some(res)) .or_else(|err| { // Missing doc: Why are we not propagating errors to the frontend? @@ -288,7 +289,7 @@ pub(crate) fn handle_goto_definition( let document = state.get_document(uri)?; // build goto definition context - let result = unwrap!(unsafe { goto_definition(&document, params) }, Err(err) => { + let result = unwrap!(goto_definition(&document, params), Err(err) => { lsp::log_error!("{err:?}"); return Ok(None); }); diff --git a/crates/ark/src/lsp/indexer.rs b/crates/ark/src/lsp/indexer.rs index 70176ca7c..5954c5b23 100644 --- a/crates/ark/src/lsp/indexer.rs +++ b/crates/ark/src/lsp/indexer.rs @@ -125,15 +125,25 @@ fn insert(path: &Path, entry: IndexEntry) -> anyhow::Result<()> { let path = str_from_path(path)?; let index = index.entry(path.to_string()).or_default(); + index_insert(index, entry); - // Retain the first occurrence in the index. In the future we'll track every occurrences and - // their scopes but for now we only track the first definition of an object (in a way, its + Ok(()) +} + +fn index_insert(index: &mut HashMap, entry: IndexEntry) { + // We generally retain only the first occurrence in the index. In the + // future we'll track every occurrences and their scopes but for now we + // only track the first definition of an object (in a way, its // declaration). - if !index.contains_key(&entry.key) { + if let Some(existing_entry) = index.get(&entry.key) { + // Give priority to non-section entries. + if matches!(existing_entry.data, IndexEntryData::Section { .. }) { + index.insert(entry.key.clone(), entry); + } + // Else, ignore. + } else { index.insert(entry.key.clone(), entry); } - - Ok(()) } fn clear(path: &Path) -> anyhow::Result<()> { @@ -148,6 +158,24 @@ fn clear(path: &Path) -> anyhow::Result<()> { Ok(()) } +#[cfg(test)] +pub(crate) fn indexer_clear() { + let mut index = WORKSPACE_INDEX.lock().unwrap(); + index.clear(); +} + +/// RAII guard that clears `WORKSPACE_INDEX` when dropped. +/// Useful for ensuring a clean index state in tests. +#[cfg(test)] +pub(crate) struct ResetIndexerGuard; + +#[cfg(test)] +impl Drop for ResetIndexerGuard { + fn drop(&mut self) { + indexer_clear(); + } +} + fn str_from_path(path: &Path) -> anyhow::Result<&str> { path.to_str().ok_or(anyhow!( "Couldn't convert path {} to string", @@ -401,7 +429,9 @@ fn index_comment( mod tests { use std::path::PathBuf; + use assert_matches::assert_matches; use insta::assert_debug_snapshot; + use tower_lsp::lsp_types; use super::*; use crate::lsp::documents::Document; @@ -532,4 +562,67 @@ class <- R6::R6Class( "# ); } + + #[test] + fn test_index_insert_priority() { + let mut index = HashMap::new(); + + let section_entry = IndexEntry { + key: "foo".to_string(), + range: Range::new( + lsp_types::Position::new(0, 0), + lsp_types::Position::new(0, 3), + ), + data: IndexEntryData::Section { + level: 1, + title: "foo".to_string(), + }, + }; + + let variable_entry = IndexEntry { + key: "foo".to_string(), + range: Range::new( + lsp_types::Position::new(1, 0), + lsp_types::Position::new(1, 3), + ), + data: IndexEntryData::Variable { + name: "foo".to_string(), + }, + }; + + // The Variable has priority and should replace the Section + index_insert(&mut index, section_entry.clone()); + index_insert(&mut index, variable_entry.clone()); + assert_matches!( + &index.get("foo").unwrap().data, + IndexEntryData::Variable { name } => assert_eq!(name, "foo") + ); + + // Inserting a Section again with the same key does not override the Variable + index_insert(&mut index, section_entry.clone()); + assert_matches!( + &index.get("foo").unwrap().data, + IndexEntryData::Variable { name } => assert_eq!(name, "foo") + ); + + let function_entry = IndexEntry { + key: "foo".to_string(), + range: Range::new( + lsp_types::Position::new(2, 0), + lsp_types::Position::new(2, 3), + ), + data: IndexEntryData::Function { + name: "foo".to_string(), + arguments: vec!["a".to_string()], + }, + }; + + // Inserting another kind of variable (e.g., Function) with the same key + // does not override it either. The first occurrence is generally retained. + index_insert(&mut index, function_entry.clone()); + assert_matches!( + &index.get("foo").unwrap().data, + IndexEntryData::Variable { name } => assert_eq!(name, "foo") + ); + } } diff --git a/crates/ark/src/lsp/main_loop.rs b/crates/ark/src/lsp/main_loop.rs index ac5588cea..0ee4be1a3 100644 --- a/crates/ark/src/lsp/main_loop.rs +++ b/crates/ark/src/lsp/main_loop.rs @@ -281,7 +281,7 @@ impl GlobalState { respond(tx, || state_handlers::initialize(params, &mut self.lsp_state, &mut self.world), LspResponse::Initialize)?; }, LspRequest::WorkspaceSymbol(params) => { - respond(tx, || handlers::handle_symbol(params), LspResponse::WorkspaceSymbol)?; + respond(tx, || handlers::handle_symbol(params, &self.world), LspResponse::WorkspaceSymbol)?; }, LspRequest::DocumentSymbol(params) => { respond(tx, || handlers::handle_document_symbol(params, &self.world), LspResponse::DocumentSymbol)?; diff --git a/crates/ark/src/lsp/symbols.rs b/crates/ark/src/lsp/symbols.rs index df6d8b84d..894f990fd 100644 --- a/crates/ark/src/lsp/symbols.rs +++ b/crates/ark/src/lsp/symbols.rs @@ -56,7 +56,10 @@ fn new_symbol_node( symbol } -pub fn symbols(params: &WorkspaceSymbolParams) -> anyhow::Result> { +pub(crate) fn symbols( + params: &WorkspaceSymbolParams, + state: &WorldState, +) -> anyhow::Result> { let query = ¶ms.query; let mut info: Vec = Vec::new(); @@ -81,17 +84,19 @@ pub fn symbols(params: &WorkspaceSymbolParams) -> anyhow::Result { - info.push(SymbolInformation { - name: title.to_string(), - kind: SymbolKind::STRING, - location: Location { - uri: Url::from_file_path(path).unwrap(), - range: entry.range, - }, - tags: None, - deprecated: None, - container_name: None, - }); + if state.config.workspace_symbols.include_comment_sections { + info.push(SymbolInformation { + name: title.to_string(), + kind: SymbolKind::STRING, + location: Location { + uri: Url::from_file_path(path).unwrap(), + range: entry.range, + }, + tags: None, + deprecated: None, + container_name: None, + }); + } }, IndexEntryData::Variable { name } => { @@ -578,7 +583,11 @@ mod tests { use tower_lsp::lsp_types::Position; use super::*; + use crate::lsp::config::LspConfig; + use crate::lsp::config::WorkspaceSymbolsConfig; use crate::lsp::documents::Document; + use crate::lsp::indexer::ResetIndexerGuard; + use crate::lsp::util::test_path; fn test_symbol(code: &str) -> Vec { let doc = Document::new(code, None); @@ -889,4 +898,43 @@ a <- function() { insta::assert_debug_snapshot!(symbols); } + + #[test] + fn test_workspace_symbols_include_comment_sections() { + fn run(include_comment_sections: bool) -> Vec { + let _guard = ResetIndexerGuard; + + let code = "# Section ----\nfoo <- 1"; + + let mut config = LspConfig::default(); + config.workspace_symbols = WorkspaceSymbolsConfig { + include_comment_sections, + }; + let mut state = WorldState::default(); + state.config = config; + + // Index the document + let doc = Document::new(code, None); + let (path, _) = test_path(); + indexer::update(&doc, &path).unwrap(); + + // Query for all symbols + let params = WorkspaceSymbolParams { + query: "Section".to_string(), + ..Default::default() + }; + let result = super::symbols(¶ms, &state).unwrap(); + let out = result.into_iter().map(|s| s.name).collect(); + + out + } + + // Should include section when true + let with_sections = run(true); + assert!(with_sections.contains(&"Section".to_string())); + + // Should not include section when false + let without_sections = run(false); + assert!(!without_sections.contains(&"Section".to_string())); + } } diff --git a/crates/ark/src/lsp/util.rs b/crates/ark/src/lsp/util.rs index 5140e4bd1..2fa50896d 100644 --- a/crates/ark/src/lsp/util.rs +++ b/crates/ark/src/lsp/util.rs @@ -28,3 +28,17 @@ pub unsafe extern "C-unwind" fn ps_object_id(object: SEXP) -> anyhow::Result (std::path::PathBuf, url::Url) { + use std::path::PathBuf; + + let path = if cfg!(windows) { + PathBuf::from(r"C:\test.R") + } else { + PathBuf::from("/test.R") + }; + let uri = url::Url::from_file_path(&path).unwrap(); + + (path, uri) +}