feat: Allow goto definition for strings

Author: felix-andreas

crates/roughly/src/cli.rs

@@ -473,7 +473,7 @@ pub fn ast(path: &Path) -> Result<(), DebugError> {
     };
 
     let tree = tree::parse(&mut tree::new_parser(), &text, None);
-    eprintln!("{}", tree::format(tree.root_node()));
+    eprintln!("{}", tree::display_ast(tree.root_node()));
     Ok(())
 }
 

crates/roughly/src/definition.rs

@@ -17,8 +17,11 @@ pub fn goto(
     tree: &Tree,
     symbols_map: &impl SymbolsMap,
 ) -> Option<GotoDefinitionResponse> {
-    let start = tree::try_get_identifier(tree, line, col)?;
-    let name = rope.byte_slice(start.byte_range()).to_string();
+    // note: R allows to call strings like "function_name"().
+    // also useful for S4:  e.g. to goto "Person" definition in
+    // setMethod("age", "Person", ...) or setClass("Employee", contains = "Person")
+    let (start, byte_range) = tree::try_get_identifier_or_string(tree, line, col)?;
+    let name = rope.byte_slice(byte_range).to_string();
 
     tracing::debug!(?name, start = start.kind(), "goto definition");
 
@@ -35,23 +38,26 @@ pub fn goto(
         )));
     }
 
-    let is_local_scope = tree::find_containing_function(start).is_some();
+    let is_global_scope = tree::find_containing_function(start).is_none();
 
     let globals = symbols_map.filter_map(
         |path, symbols| {
             let other_uri = Uri::from_file_path(path).unwrap();
-            // Only include locations from other files
-            (if is_local_scope || &other_uri != uri {
-                symbols
-            } else {
-                &[]
-            })
-            .iter()
-            .filter(|symbol| symbol.name == name)
-            .map(move |symbol| Location {
-                uri: other_uri.clone(),
-                range: symbol.range,
-            })
+            // note that s4 and r6 are not found by find_previous_definition.
+            // therefore if is same file and global scope we must ignore later defintions
+            let ignore_later = is_global_scope && *uri == other_uri;
+            let name = &name;
+            symbols
+                .iter()
+                .filter(move |symbol| {
+                    symbol.name == *name
+                        && (!ignore_later
+                            || symbol.range.end < utils::point_to_position(start.start_position()))
+                })
+                .map(move |symbol| Location {
+                    uri: other_uri.clone(),
+                    range: symbol.range,
+                })
         },
         128,
     );
@@ -115,10 +121,9 @@ mod tests {
 
     fn setup_a(src: &str, line: usize, col: usize) -> Option<(usize, usize)> {
         let rope = Rope::from_str(src);
-        let mut parser = tree::new_parser();
-        let tree = tree::parse(&mut parser, src, None);
-        let start = tree::try_get_identifier(&tree, line, col).unwrap();
-        let name = rope.byte_slice(start.byte_range()).to_string();
+        let tree = tree::parse(&mut tree::new_parser(), src, None);
+        let (start, byte_range) = tree::try_get_identifier_or_string(&tree, line, col)?;
+        let name = rope.byte_slice(byte_range).to_string();
         find_previous_definition(start, &rope, &name).map(|node| {
             let point = node.start_position();
             (point.row, point.column)
@@ -127,24 +132,25 @@ mod tests {
 
     fn setup_b(src: &str, line: usize, col: usize) -> Option<String> {
         let rope = Rope::from_str(src);
-        let mut parser = tree::new_parser();
-        let tree = tree::parse(&mut parser, src, None);
-        tree::try_get_identifier(&tree, line, col)
-            .map(|node| rope.byte_slice(node.byte_range()).to_string())
+        let tree = tree::parse(&mut tree::new_parser(), src, None);
+        tree::try_get_identifier_or_string(&tree, line, col)
+            .map(|(_, byte_range)| rope.byte_slice(byte_range).to_string())
     }
 
     #[test]
-    fn try_get_identifier_returns_none_for_non_identifier() {
+    fn try_get_identifier_base() {
         let src = indoc! {r#"
             123
-            "string"
             x <- 1
-             x
+                x
+            setMethod("age", "Person", \(x) x@age)
         "#};
         assert_eq!(setup_b(src, 0, 0), None);
-        assert_eq!(setup_b(src, 1, 0), None);
-        assert_eq!(setup_b(src, 2, 0), Some("x".to_string()));
-        assert_eq!(setup_b(src, 3, 0), None);
+        assert_eq!(setup_b(src, 1, 0), Some("x".into()));
+        assert_eq!(setup_b(src, 2, 0), None);
+        assert_eq!(setup_b(src, 3, 17), Some("Person".into()));
+        // assert_eq!(setup_b(src, 3, 18), Some("Person".into()));
+        assert_eq!(setup_b(src, 3, 24), Some("Person".into()));
     }
 
     #[test]

crates/roughly/src/index.rs

@@ -69,7 +69,7 @@ impl Item {
 pub trait SymbolsMap {
     fn filter_map<'a, T, I>(
         &'a self,
-        key: impl Fn(&'a PathBuf, &'a [Item]) -> I,
+        key: impl Fn(&'a Path, &'a [Item]) -> I,
         limit: usize,
     ) -> Vec<T>
     where
@@ -79,7 +79,7 @@ pub trait SymbolsMap {
 impl SymbolsMap for HashMap<PathBuf, Vec<Item>> {
     fn filter_map<'a, T, I>(
         &'a self,
-        key: impl Fn(&'a PathBuf, &'a [Item]) -> I,
+        key: impl Fn(&'a Path, &'a [Item]) -> I,
         limit: usize,
     ) -> Vec<T>
     where
@@ -394,7 +394,7 @@ fn index_call(call: Node, rope: &Rope, nested: bool) -> Option<Item> {
                 })
                 .unwrap_or_else(|| "Unknown".to_string());
 
-            Some(Item::new(
+            (!method_name.is_empty()).then_some(Item::new(
                 method_name,
                 None,
                 range,

crates/roughly/src/symbols.rs

@@ -5,7 +5,11 @@ use crate::{
 };
 
 pub fn document(items: &[Item]) -> Vec<DocumentSymbol> {
-    items.iter().map(to_document_symbol).collect()
+    items
+        .iter()
+        .filter(|item| !item.name.is_empty()) // lsp: doens't allow empty names
+        .map(to_document_symbol)
+        .collect()
 }
 
 pub fn workspace(query: &str, workspace_symbols: &impl SymbolsMap) -> Vec<WorkspaceSymbol> {

crates/roughly/src/tree.rs

@@ -131,24 +131,43 @@ pub fn try_get_identifier<'tree>(
     line: usize,
     col: usize,
 ) -> Option<Node<'tree>> {
-    let start = {
-        let point = Point::new(line, col);
-        let node = tree.root_node().descendant_for_point_range(point, point)?;
-        // If the cursor is at the very start of the program, `descendant_for_point_range` may
-        // return the program node itself. Since only the program node can start at the same
-        // position as an identifier, it's safe to check the first child node in this case
-        match node.kind_id() {
-            kind::PROGRAM => match node.child(0) {
-                Some(child) if point_in_range(point, child.range()) => {
-                    child.descendant_for_point_range(point, point)?
-                }
-                _ => return None,
-            },
-            _ => node,
+    let start = node_at_position(tree, Point::new(line, col))?;
+    (start.kind_id() == kind::IDENTIFIER).then_some(start)
+}
+
+pub fn try_get_identifier_or_string<'tree>(
+    tree: &'tree Tree,
+    line: usize,
+    col: usize,
+) -> Option<(Node<'tree>, std::ops::Range<usize>)> {
+    let start = node_at_position(tree, Point::new(line, col))?;
+    Some(match start.kind_id() {
+        kind::IDENTIFIER => (start, start.byte_range()),
+        kind::STRING_CONTENT | kind::SINGLE_QUOTE | kind::DOUBLE_QUOTE => {
+            let parent = start.parent()?;
+            (
+                parent,
+                parent.child_by_field_id(field::CONTENT)?.byte_range(),
+            )
         }
-    };
+        _ => return None,
+    })
+}
 
-    (start.kind_id() == kind::IDENTIFIER).then_some(start)
+pub fn node_at_position<'tree>(tree: &'tree Tree, point: Point) -> Option<Node<'tree>> {
+    let node = tree.root_node().descendant_for_point_range(point, point)?;
+    // If the cursor is at the very start of the program, `descendant_for_point_range` may
+    // return the program node itself. Since only the program node can start at the same
+    // position as e.g. an identifier, it's safe to check the first child node in this case
+    match node.kind_id() {
+        kind::PROGRAM => match node.child(0) {
+            Some(child) if point_in_range(point, child.range()) => {
+                child.descendant_for_point_range(point, point)
+            }
+            _ => None,
+        },
+        _ => Some(node),
+    }
 }
 
 pub fn is_rhs_of_extract_or_namespace(node: Node) -> bool {
@@ -161,10 +180,10 @@ pub fn is_rhs_of_extract_or_namespace(node: Node) -> bool {
 }
 
 //
-// FORMAT AST
+// DISPLAY AST
 //
 
-pub fn format(node: Node) -> String {
+pub fn display_ast(node: Node) -> String {
     fn traverse(cursor: &mut TreeCursor, output: &mut String) {
         let indent = "    ".repeat(cursor.depth() as usize);
         let node = cursor.node();

crates/roughly/src/utils.rs

@@ -1,8 +1,8 @@
 use {
-    crate::lsp_types::{Position, Range as LspRange},
+    crate::lsp_types::{Position, Range},
     ropey::Rope,
     std::{fs::File, io::BufReader, path::Path},
-    tree_sitter::Node,
+    tree_sitter::{Node, Point},
 };
 
 pub fn starts_with_lowercase(name: &str, query: &str) -> bool {
@@ -13,15 +13,18 @@ pub fn read_to_rope(path: impl AsRef<Path>) -> std::io::Result<Rope> {
     Rope::from_reader(BufReader::new(File::open(path)?))
 }
 
-pub fn node_range(node: Node) -> LspRange {
-    let start = node.start_position();
-    let end = node.end_position();
-    LspRange::new(
-        Position::new(start.row as u32, start.column as u32),
-        Position::new(end.row as u32, end.column as u32),
+pub fn node_range(node: Node) -> Range {
+    Range::new(
+        point_to_position(node.start_position()),
+        point_to_position(node.end_position()),
     )
 }
 
+#[inline(always)]
+pub fn point_to_position(point: Point) -> Position {
+    Position::new(point.row as u32, point.column as u32)
+}
+
 // adapted from https://doc.rust-lang.org/stable/nightly-rustc/src/clippy_utils/str_utils.rs.html
 
 /// ```

justfile

@@ -38,6 +38,9 @@ install-extension:
 # BUILD
 #
 
+build:
+	cargo build
+
 build-linux:
 	cargo build --release --target x86_64-unknown-linux-gnu
 
@@ -71,7 +74,7 @@ publish $version $kind:
 	set -euo pipefail
 
 	just bump-version $version
-	just build $version $kind
+	just release $version $kind
 	just publish-github $version
 	just publish-marketplace $version $kind
 
@@ -83,10 +86,10 @@ publish-commit $version="":
 		version=$(git rev-parse --short=6 HEAD)
 		echo "info: using git revision $version as version"
 	fi
-	just build $version pre-release
+	just release $version pre-release
 	just publish-github $version
 
-build $version $kind:
+release $version $kind:
 	#!/usr/bin/env bash
 	set -euo pipefail