Prefer current file in jump to definition (#886)

Author: lionel-

crates/ark/src/lsp/definitions.rs

@@ -42,10 +42,20 @@ pub fn goto_definition<'a>(
     let end = convert_point_to_position(contents, node.end_position());
     let range = Range { start, end };
 
-    // search for a reference in the document index
+    // Search for a reference in the document index
     if node.is_identifier() {
         let symbol = document.contents.node_slice(&node)?.to_string();
-        if let Some((path, entry)) = indexer::find(symbol.as_str()) {
+
+        let uri = &params.text_document_position_params.text_document.uri;
+        let info = if let Ok(preferred_path) = uri.to_file_path() {
+            // First search in current file, then in all files
+            indexer::find_in_file(symbol.as_str(), &preferred_path)
+                .or_else(|| indexer::find(symbol.as_str()))
+        } else {
+            indexer::find(symbol.as_str())
+        };
+
+        if let Some((path, entry)) = info {
             let link = LocationLink {
                 origin_selection_range: None,
                 target_uri: Url::from_file_path(path).unwrap(),
@@ -95,7 +105,7 @@ foo <- 42
 print(foo)
 "#;
         let doc = Document::new(code, None);
-        let (path, uri) = test_path();
+        let (path, uri) = test_path("test.R");
 
         indexer::update(&doc, &path).unwrap();
 
@@ -132,7 +142,7 @@ foo <- 1
 print(foo)
 "#;
         let doc = Document::new(code, None);
-        let (path, uri) = test_path();
+        let (path, uri) = test_path("test.R");
 
         indexer::update(&doc, &path).unwrap();
 
@@ -159,4 +169,110 @@ print(foo)
             }
         );
     }
+
+    #[test]
+    fn test_goto_definition_prefers_local_symbol() {
+        let _guard = indexer::ResetIndexerGuard;
+
+        // Both files define the same symbol
+        let code1 = r#"
+foo <- 1
+foo
+"#;
+        let code2 = r#"
+foo <- 2
+foo
+"#;
+
+        let doc1 = Document::new(code1, None);
+        let doc2 = Document::new(code2, None);
+
+        let (path1, uri1) = test_path("file1.R");
+        let (path2, uri2) = test_path("file2.R");
+
+        indexer::update(&doc1, &path1).unwrap();
+        indexer::update(&doc2, &path2).unwrap();
+
+        // Go to definition for foo in file1
+        let params1 = GotoDefinitionParams {
+            text_document_position_params: lsp_types::TextDocumentPositionParams {
+                text_document: lsp_types::TextDocumentIdentifier { uri: uri1.clone() },
+                position: lsp_types::Position::new(2, 0),
+            },
+            work_done_progress_params: Default::default(),
+            partial_result_params: Default::default(),
+        };
+        assert_matches!(
+            goto_definition(&doc1, params1).unwrap(),
+            Some(GotoDefinitionResponse::Link(ref links)) => {
+                // Should jump to foo in file1
+                assert_eq!(links[0].target_uri, uri1);
+            }
+        );
+
+        // Go to definition for foo in file2
+        let params2 = GotoDefinitionParams {
+            text_document_position_params: lsp_types::TextDocumentPositionParams {
+                text_document: lsp_types::TextDocumentIdentifier { uri: uri2.clone() },
+                position: lsp_types::Position::new(2, 0),
+            },
+            work_done_progress_params: Default::default(),
+            partial_result_params: Default::default(),
+        };
+        assert_matches!(
+            goto_definition(&doc2, params2).unwrap(),
+            Some(GotoDefinitionResponse::Link(ref links)) => {
+                // Should jump to foo in file2
+                assert_eq!(links[0].target_uri, uri2);
+            }
+        );
+    }
+
+    #[test]
+    fn test_goto_definition_falls_back_to_other_file() {
+        let _guard = indexer::ResetIndexerGuard;
+
+        // file1 defines foo, file2 does not
+        let code1 = r#"
+foo <- 1
+"#;
+        let code2 = r#"
+foo
+"#;
+
+        let doc1 = Document::new(code1, None);
+        let doc2 = Document::new(code2, None);
+
+        // Use test_path for cross-platform compatibility
+        let (path1, uri1) = crate::lsp::util::test_path("file1.R");
+        let (path2, uri2) = crate::lsp::util::test_path("file2.R");
+
+        indexer::update(&doc1, &path1).unwrap();
+        indexer::update(&doc2, &path2).unwrap();
+
+        // Go to definition for foo in file2 (should jump to file1)
+        let params2 = GotoDefinitionParams {
+            text_document_position_params: lsp_types::TextDocumentPositionParams {
+                text_document: lsp_types::TextDocumentIdentifier { uri: uri2.clone() },
+                position: lsp_types::Position::new(1, 0),
+            },
+            work_done_progress_params: Default::default(),
+            partial_result_params: Default::default(),
+        };
+        let result2 = goto_definition(&doc2, params2).unwrap();
+        assert_matches!(
+            result2,
+            Some(GotoDefinitionResponse::Link(ref links)) => {
+                // Should jump to foo in file1
+                assert_eq!(links[0].target_uri, uri1);
+                assert_eq!(
+                    links[0].target_range,
+                    lsp_types::Range {
+                        start: lsp_types::Position::new(1, 0),
+                        end: lsp_types::Position::new(1, 3),
+                    }
+                );
+            }
+        );
+    }
 }

crates/ark/src/lsp/indexer.rs

@@ -91,6 +91,7 @@ pub fn start(folders: Vec<String>) {
     );
 }
 
+/// Search the workspace files and return the first symbol match
 pub fn find(symbol: &str) -> Option<(String, IndexEntry)> {
     let index = WORKSPACE_INDEX.lock().unwrap();
 
@@ -103,6 +104,21 @@ pub fn find(symbol: &str) -> Option<(String, IndexEntry)> {
     None
 }
 
+/// Search a specific workspace file for a symbol
+pub fn find_in_file(symbol: &str, path: &std::path::Path) -> Option<(String, IndexEntry)> {
+    let index = WORKSPACE_INDEX.lock().unwrap();
+
+    if let Ok(path_str) = str_from_path(path) {
+        if let Some(index) = index.get(path_str) {
+            if let Some(entry) = index.get(symbol) {
+                return Some((path_str.to_string(), entry.clone()));
+            }
+        }
+    }
+
+    None
+}
+
 pub fn map(mut callback: impl FnMut(&Path, &String, &IndexEntry)) {
     let index = WORKSPACE_INDEX.lock().unwrap();
 
@@ -194,7 +210,7 @@ impl Drop for ResetIndexerGuard {
 fn str_from_path(path: &Path) -> anyhow::Result<&str> {
     path.to_str().ok_or(anyhow!(
         "Couldn't convert path {} to string",
-        path.display()
+        path.to_string_lossy()
     ))
 }
 

crates/ark/src/lsp/symbols.rs

@@ -990,7 +990,7 @@ outer <- 4
 
             // Index the document
             let doc = Document::new(code, None);
-            let (path, _) = test_path();
+            let (path, _) = test_path("test.R");
             indexer::update(&doc, &path).unwrap();
 
             // Query for all symbols

crates/ark/src/lsp/util.rs

@@ -29,16 +29,12 @@ pub unsafe extern "C-unwind" fn ps_object_id(object: SEXP) -> anyhow::Result<SEX
     return Ok(Rf_mkString(value.as_ptr() as *const c_char));
 }
 
+/// Create an absolute path from a file name.
+/// File name is joined to the temp directory in a cross-platform way.
+/// Returns both a `PathBuf` and a `Url`. Does not create the file.
 #[cfg(test)]
-pub(crate) fn test_path() -> (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")
-    };
+pub(crate) fn test_path(file_name: &str) -> (std::path::PathBuf, url::Url) {
+    let path = std::env::temp_dir().join(file_name);
     let uri = url::Url::from_file_path(&path).unwrap();
-
     (path, uri)
 }