Merge pull request #866 from posit-dev/feature/optional-section-workspace-symbols

Exclude comment sections from workspace symbols by default

Author: lionel-

Cargo.lock

@@ -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"

crates/ark/Cargo.toml

@@ -33,6 +33,14 @@ pub static GLOBAL_SETTINGS: &[Setting<LspConfig>] = &[
                 .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<DocumentConfig>] = &[
 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 <https://editorconfig.org/> 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 {

crates/ark/src/lsp/config.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<Option<GotoDefinitionResponse>> {
@@ -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),
+                    }
+                );
+            }
+        );
+    }
+}

crates/ark/src/lsp/definitions.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<Option<Vec<SymbolInformation>>> {
-    symbols::symbols(&params)
+    symbols::symbols(&params, 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);
     });

crates/ark/src/lsp/handlers.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<String, IndexEntry>, 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")
+        );
+    }
 }

crates/ark/src/lsp/indexer.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)?;