get to acceptable state

Author: felix-andreas

crates/roughly/src/definition.rs

@@ -98,7 +98,7 @@ fn try_get_identifier<'tree>(tree: &'tree Tree, line: usize, col: usize) -> Opti
     (start.kind_id() == kind::IDENTIFIER).then_some(start)
 }
 
-fn find_previous_definition<'a>(start: Node<'a>, rope: &Rope, name: &str) -> Option<Node<'a>> {
+pub fn find_previous_definition<'a>(start: Node<'a>, rope: &Rope, name: &str) -> Option<Node<'a>> {
     let mut node = start;
     loop {
         let is_descendent;

crates/roughly/src/rename.rs

@@ -1,14 +1,20 @@
 use {
     crate::{
+        definition,
         lsp_types::{TextEdit, Url as Uri, WorkspaceEdit},
         tree::{self, field, kind},
         utils,
     },
     ropey::Rope,
-    std::collections::HashMap,
+    std::collections::{HashMap, HashSet},
     tree_sitter::{Node, Point, Tree},
 };
 
+// 1. figure out if expression at position can be renamed (is identifier and not rhs of @ or $)
+// 2. find previous definition for identifier
+// 3. if identifier is global scope -> stop (for now, we would to rename in all files)
+// 4. find all references for identifier
+
 pub fn rename(
     uri: &Uri,
     line: usize,
@@ -19,60 +25,58 @@ pub fn rename(
 ) -> Option<WorkspaceEdit> {
     let start = try_get_identifier(tree, line, col)?;
     let parent = start.parent()?;
-    let current_name = rope.byte_slice(start.byte_range()).to_string();
+    let name = rope.byte_slice(start.byte_range()).to_string();
 
     tracing::debug!(
-        ?current_name,
+        ?name,
         ?new_name,
         start = start.kind(),
         parent = parent.kind(),
         "rename request"
     );
 
-    // Check if rename is allowed - don't allow renaming RHS of certain operators
     if !is_rename_allowed(start, parent) {
         tracing::debug!("Rename not allowed for this symbol");
         return None;
     }
 
-    // Find all references to this symbol in the appropriate scope
-    let references = find_all_references(start, rope, &current_name)?;
-
-    // Convert references to text edits
-    let mut edits = Vec::new();
-    for reference in references {
-        let range = utils::node_range(reference);
-        edits.push(TextEdit {
-            range,
-            new_text: new_name.to_string(),
-        });
-    }
+    let definition = definition::find_previous_definition(start, rope, &name)?;
+    let scope = find_scope_containing(definition)?; // for now we return if global scope
+
+    let mut references = Vec::new();
+    find_references_in_scope(scope, rope, &name, definition, &mut references);
+
+    let edits: Vec<_> = references
+        .into_iter()
+        .filter_map({
+            let mut seen = HashSet::new(); // make sure we don't edit same node multiple times
+            move |reference| {
+                seen.insert(reference.id()).then(|| TextEdit {
+                    range: utils::node_range(reference),
+                    new_text: new_name.to_string(),
+                })
+            }
+        })
+        .collect();
 
     if edits.is_empty() {
         return None;
     }
 
-    // Create workspace edit
-    let mut changes = HashMap::new();
-    changes.insert(uri.clone(), edits);
+    tracing::debug!(?edits);
 
     Some(WorkspaceEdit {
-        changes: Some(changes),
+        changes: Some(HashMap::from_iter([(uri.clone(), edits)])),
         ..Default::default()
     })
 }
 
 fn is_rename_allowed(symbol: Node, parent: Node) -> bool {
     // Don't allow renaming RHS of extract @, extract $, or namespace :: operators
-    if [kind::EXTRACT_OPERATOR, kind::NAMESPACE_OPERATOR].contains(&parent.kind_id())
+    !([kind::EXTRACT_OPERATOR, kind::NAMESPACE_OPERATOR].contains(&parent.kind_id())
         && parent
             .child_by_field_id(field::RHS)
-            .is_some_and(|rhs| rhs.id() == symbol.id())
-    {
-        return false;
-    }
-
-    true
+            .is_some_and(|rhs| rhs.id() == symbol.id()))
 }
 
 fn try_get_identifier<'tree>(tree: &'tree Tree, line: usize, col: usize) -> Option<Node<'tree>> {
@@ -94,82 +98,9 @@ fn try_get_identifier<'tree>(tree: &'tree Tree, line: usize, col: usize) -> Opti
     (start.kind_id() == kind::IDENTIFIER).then_some(start)
 }
 
-/// Find all references to a symbol within its scope
-fn find_all_references<'a>(start: Node<'a>, rope: &Rope, name: &str) -> Option<Vec<Node<'a>>> {
-    let mut references = Vec::new();
-
-    // Find the definition of the symbol
-    let definition = find_definition(start, rope, name)?;
-
-    // Find the scope containing the definition
-    let scope = find_scope_containing(definition)?;
-
-    // Find all references within this scope
-    find_references_in_scope(scope, rope, name, definition, &mut references);
-
-    // Deduplicate references by node ID
-    let mut unique_refs = Vec::new();
-    let mut seen_ids = std::collections::HashSet::new();
-
-    for reference in references {
-        if seen_ids.insert(reference.id()) {
-            unique_refs.push(reference);
-        }
-    }
-
-    Some(unique_refs)
-}
-
-fn find_definition<'a>(start: Node<'a>, rope: &Rope, name: &str) -> Option<Node<'a>> {
-    let mut node = start;
-    loop {
-        let is_descendent;
-        if let Some(sibling) = node.prev_sibling() {
-            node = sibling;
-            is_descendent = false;
-        } else if let Some(parent) = node.parent() {
-            node = parent;
-            is_descendent = true;
-        } else {
-            break None;
-        }
-
-        let maybe_definition = match node.kind_id() {
-            kind::PARAMETERS => node
-                .children_by_field_name("parameter", &mut node.walk())
-                .filter_map(|parameter| parameter.child_by_field_id(field::NAME))
-                .find(|child| rope.byte_slice(child.byte_range()) == name),
-            kind::BINARY_OPERATOR => {
-                let maybe_lhs = node.child_by_field_id(field::LHS);
-                let maybe_op = node.child_by_field_id(field::OPERATOR);
-                maybe_lhs.filter(|lhs| {
-                    lhs.kind_id() == kind::IDENTIFIER
-                        && maybe_op.is_some_and(|op| {
-                            [kind::EQUAL, kind::LEFT_ASSIGN].contains(&op.kind_id())
-                        })
-                        && rope.byte_slice(lhs.byte_range()) == name
-                        && (!is_descendent || lhs.id() == start.id())
-                })
-            }
-            _ => None,
-        };
-
-        if maybe_definition.is_some() {
-            return maybe_definition;
-        }
-    }
-}
-
-fn find_scope_containing<'a>(node: Node<'a>) -> Option<Node<'a>> {
-    let mut current = Some(node);
-    while let Some(node) = current {
-        match node.kind_id() {
-            kind::FUNCTION_DEFINITION => return Some(node),
-            kind::PROGRAM => return Some(node),
-            _ => current = node.parent(),
-        }
-    }
-    None
+fn find_scope_containing(node: Node) -> Option<Node> {
+    std::iter::successors(Some(node), |node| node.parent())
+        .find(|node| node.kind_id() == kind::FUNCTION_DEFINITION)
 }
 
 fn find_references_in_scope<'a>(
@@ -186,47 +117,41 @@ fn find_references_in_scope<'a>(
         // If this is an identifier with matching name, check if it refers to our definition
         if node.kind_id() == kind::IDENTIFIER
             && rope.byte_slice(node.byte_range()) == name
-            && is_reference_to_symbol(node, rope, name)
+            // todo consider moving this check up
+            && (node
+                .parent()
+                .is_none_or(|parent| is_rename_allowed(node, parent)))
             && refers_to_definition(node, definition, rope, name)
         {
             references.push(node);
         }
 
-        // Add children to queue for processing
-        let mut child_cursor = node.walk();
-        if child_cursor.goto_first_child() {
+        let mut cursor = node.walk();
+        if cursor.goto_first_child() {
             loop {
-                let child = child_cursor.node();
+                let child = cursor.node();
 
                 // We need to traverse nested functions too for free variables
                 // But we need to be careful about scope - we'll let the definition
                 // finding logic handle whether a reference is valid
                 queue.push(child);
 
-                if !child_cursor.goto_next_sibling() {
+                if !cursor.goto_next_sibling() {
                     break;
                 }
             }
         }
     }
 }
 
-fn is_reference_to_symbol(node: Node, rope: &Rope, name: &str) -> bool {
-    node.kind_id() == kind::IDENTIFIER
-        && rope.byte_slice(node.byte_range()) == name
-        && (node
-            .parent()
-            .is_none_or(|parent| is_rename_allowed(node, parent)))
-}
-
 fn refers_to_definition(reference: Node, definition: Node, rope: &Rope, name: &str) -> bool {
     // If this is the definition itself, include it
     if reference.id() == definition.id() {
         return true;
     }
 
     // Use the same logic as find_definition to see if this reference would resolve to our definition
-    let found_definition = find_definition(reference, rope, name);
+    let found_definition = definition::find_previous_definition(reference, rope, name);
     match found_definition {
         Some(found_def) => found_def.id() == definition.id(),
         None => false,
@@ -239,21 +164,41 @@ mod tests {
 
     fn setup(src: &str, line: usize, col: usize, new_name: &str) -> Option<WorkspaceEdit> {
         let rope = Rope::from_str(src);
-        let mut parser = tree::new_parser();
-        let tree = tree::parse(&mut parser, src, None);
+        let tree = tree::parse(&mut tree::new_parser(), src, None);
         let uri = Uri::parse("file:///test.R").unwrap();
         rename(&uri, line, col, new_name, &rope, &tree)
     }
 
+    // #[test]
+    // fn global_variable_rename() {
+    //     let src = indoc! {r#"
+    //         x <- 1
+    //         y <- x + 2
+    //         x
+    //     "#};
+
+    //     let result = setup(src, 0, 0, "new_var").unwrap();
+    //     let changes = result.changes.unwrap();
+    //     let uri = Uri::parse("file:///test.R").unwrap();
+    //     let edits = &changes[&uri];
+
+    //     assert_eq!(edits.len(), 3); // Should find 3 references
+    //     assert_eq!(edits[0].new_text, "new_var");
+    //     assert_eq!(edits[1].new_text, "new_var");
+    //     assert_eq!(edits[2].new_text, "new_var");
+    // }
+
     #[test]
-    fn basic_variable_rename() {
+    fn local_variable_rename() {
         let src = indoc! {r#"
-            x <- 1
-            y <- x + 2
-            x
+            function() {
+                x <- 1
+                y <- x + 2
+                x
+            }
         "#};
 
-        let result = setup(src, 0, 0, "new_var").unwrap();
+        let result = setup(src, 2, 9, "new_var").unwrap();
         let changes = result.changes.unwrap();
         let uri = Uri::parse("file:///test.R").unwrap();
         let edits = &changes[&uri];
@@ -263,7 +208,6 @@ mod tests {
         assert_eq!(edits[1].new_text, "new_var");
         assert_eq!(edits[2].new_text, "new_var");
     }
-
     #[test]
     fn parameter_rename() {
         let src = indoc! {r#"
@@ -367,17 +311,19 @@ mod tests {
     }
 
     #[test]
-    fn shadowing_variables() {
+    fn shadowing_local_variables() {
         let src = indoc! {r#"
-            x <- 1
             function() {
-                x <- 2
-                x
+                x <- 1
+                function() {
+                    x <- 2
+                    x
+                }
             }
         "#};
 
         // Rename outer x - should only affect outer scope
-        let result = setup(src, 0, 0, "outer_x").unwrap();
+        let result = setup(src, 1, 4, "outer_x").unwrap();
         let changes = result.changes.unwrap();
         let uri = Uri::parse("file:///test.R").unwrap();
         let edits = &changes[&uri];
@@ -408,7 +354,7 @@ mod tests {
     fn variable_in_control_structures() {
         let src = indoc! {r#"
             x <- 1
-            if (TRUE) {
+            if (condition) {
                 x <- 2
                 x
             }