Detect Assign effects

crates/ark/src/lsp/rename.rs

@@ -3,7 +3,6 @@ use std::collections::HashMap;
 use aether_lsp_utils::proto::from_proto;
 use aether_lsp_utils::proto::to_proto;
 use anyhow::Context;
-use oak_core::identifier::to_identifier_text;
 use oak_db::Db;
 use tower_lsp_server::ls_types as lsp_types;
 use tower_lsp_server::ls_types::PrepareRenameResponse;
@@ -63,28 +62,27 @@ pub(crate) fn rename(
 
     let offset = from_proto::offset_from_position(position, file.line_index(db), encoding)?;
 
-    // Normalize the new name to its canonical R syntax (backtick-wrapped if
-    // needed) before searching, so an invalid name fails fast.
-    let new_text = to_identifier_text(&new_name)?;
-    let ranges = oak_ide::rename(db, file, offset)?;
+    // oak_ide resolves the sites and renders each edit in its own spelling
+    // (bare identifier, or a quoted string for a string-form binding).
+    let edits = oak_ide::rename(db, file, offset, &new_name)?;
 
     let mut changes: HashMap<lsp_types::Uri, Vec<TextEdit>> = HashMap::new();
-    for file_range in ranges {
-        let line_index = file_range.file.line_index(db);
-        let path = file_range.file.path(db);
+    for edit in edits {
+        let line_index = edit.file.line_index(db);
+        let path = edit.file.path(db);
 
         // A rename is all or nothing. Skipping a file here would rename some
         // uses of the symbol and silently leave the rest behind, so we bail
         // instead.
         let target_uri = state
-            .wire_uri(file_range.file)
+            .wire_uri(edit.file)
             .with_context(|| format!("Can't rename: no valid URI for `{path}`."))?;
-        let range = to_proto::range(file_range.range, line_index, encoding)
+        let range = to_proto::range(edit.range, line_index, encoding)
             .with_context(|| format!("Can't rename: no valid text range in `{path}`."))?;
 
         changes.entry(target_uri).or_default().push(TextEdit {
             range,
-            new_text: new_text.clone(),
+            new_text: edit.new_text,
         });
     }
 

crates/ark/src/lsp/tests/rename.rs

@@ -199,6 +199,141 @@ fn test_rename_cross_file_via_source() {
     }]);
 }
 
+#[test]
+fn test_rename_string_assignment_keeps_quotes() {
+    // `"foo" <- 1` binds `foo` via a string. Renaming keeps it a string rather
+    // than unquoting to `bar <- 1`, so the binding form is preserved.
+    let code = "\"foo\" <- 1\nfoo\n";
+    let (state, uri) = make_state(test_path("test.R").as_str(), code);
+
+    let params = make_rename_params(&uri, 1, 0, "bar");
+    let edit = rename(params, &state).unwrap().unwrap();
+
+    let mut edits = edit.changes.unwrap().remove(&uri).unwrap();
+    edits.sort_by_key(|e| e.range.start);
+    let expected: Vec<TextEdit> = vec![
+        TextEdit {
+            range: range((0, 0), (0, 5)),
+            new_text: "\"bar\"".to_string(),
+        },
+        TextEdit {
+            range: range((1, 0), (1, 3)),
+            new_text: "bar".to_string(),
+        },
+    ];
+    assert_eq!(edits, expected);
+}
+
+#[test]
+fn test_rename_assign_call_keeps_quotes() {
+    // `assign("foo", 1)` binds `foo` via a string argument. Unquoting it to
+    // `assign(bar, 1)` would change the program, so the rename stays quoted.
+    let code = "assign(\"foo\", 1)\nfoo\n";
+    let (state, uri) = make_state(test_path("test.R").as_str(), code);
+
+    let params = make_rename_params(&uri, 1, 0, "bar");
+    let edit = rename(params, &state).unwrap().unwrap();
+
+    let mut edits = edit.changes.unwrap().remove(&uri).unwrap();
+    edits.sort_by_key(|e| e.range.start);
+    let expected: Vec<TextEdit> = vec![
+        TextEdit {
+            range: range((0, 7), (0, 12)),
+            new_text: "\"bar\"".to_string(),
+        },
+        TextEdit {
+            range: range((1, 0), (1, 3)),
+            new_text: "bar".to_string(),
+        },
+    ];
+    assert_eq!(edits, expected);
+}
+
+#[test]
+fn test_rename_assign_call_from_definition_site() {
+    // Rename invoked with the cursor ON the `assign()` name literal (not a later
+    // use) works too. This is what the def's own range enables: `definition_at`
+    // hit-tests the name token at the definition site.
+    let code = "assign(\"foo\", 1)\nfoo\n";
+    let (state, uri) = make_state(test_path("test.R").as_str(), code);
+
+    // Cursor inside the `"foo"` literal.
+    let params = make_rename_params(&uri, 0, 8, "bar");
+    let edit = rename(params, &state).unwrap().unwrap();
+
+    let mut edits = edit.changes.unwrap().remove(&uri).unwrap();
+    edits.sort_by_key(|e| e.range.start);
+    let expected: Vec<TextEdit> = vec![
+        TextEdit {
+            range: range((0, 7), (0, 12)),
+            new_text: "\"bar\"".to_string(),
+        },
+        TextEdit {
+            range: range((1, 0), (1, 3)),
+            new_text: "bar".to_string(),
+        },
+    ];
+    assert_eq!(edits, expected);
+
+    // TODO!(nse-resolver): rename of a `%<>%`/`%<~%` binding can't be tested here
+    // yet, from a use or from its definition site. `SalsaImportsResolver` only
+    // resolves `base`, so the operators aren't recognized as assign effects at
+    // this layer until the resolver walks the search path. Operator recognition
+    // and its name range are covered in `oak_semantic`'s builder tests meanwhile.
+}
+
+#[test]
+fn test_rename_assign_call_preserves_single_quote_delimiter() {
+    let code = "assign('foo', 1)\nfoo\n";
+    let (state, uri) = make_state(test_path("test.R").as_str(), code);
+
+    let params = make_rename_params(&uri, 1, 0, "bar");
+    let edit = rename(params, &state).unwrap().unwrap();
+
+    let def_edit = edit
+        .changes
+        .unwrap()
+        .remove(&uri)
+        .unwrap()
+        .into_iter()
+        .find(|e| e.range.start.line == 0)
+        .unwrap();
+    assert_eq!(def_edit.new_text, "'bar'");
+}
+
+#[test]
+fn test_rename_string_bound_symbol_to_spaced_name_stays_a_string() {
+    // A name needing backticks as an identifier is a plain string in the
+    // string-form site: `"foo" <- 1` -> `"new name" <- 1`, use -> `` `new name` ``.
+    let code = "\"foo\" <- 1\nfoo\n";
+    let (state, uri) = make_state(test_path("test.R").as_str(), code);
+
+    let params = make_rename_params(&uri, 1, 0, "new name");
+    let edit = rename(params, &state).unwrap().unwrap();
+
+    let mut edits = edit.changes.unwrap().remove(&uri).unwrap();
+    edits.sort_by_key(|e| e.range.start);
+    assert_eq!(edits[0].new_text, "\"new name\"");
+    assert_eq!(edits[1].new_text, "`new name`");
+}
+
+#[test]
+fn test_rename_assign_call_to_non_syntactic_name_stays_a_string() {
+    // A non-syntactic target lands verbatim inside the quotes,
+    // `assign("non-syntactic", 1)`, with no backticks (a string holds any name).
+    // The bare-identifier use of the same symbol still gets backticks.
+    let code = "assign(\"foo\", 1)\nfoo\n";
+    let (state, uri) = make_state(test_path("test.R").as_str(), code);
+
+    let params = make_rename_params(&uri, 1, 0, "non-syntactic");
+    let edit = rename(params, &state).unwrap().unwrap();
+
+    let mut edits = edit.changes.unwrap().remove(&uri).unwrap();
+    edits.sort_by_key(|e| e.range.start);
+    assert_eq!(edits[0].new_text, "\"non-syntactic\"");
+    assert_eq!(edits[1].new_text, "`non-syntactic`");
+}
+
 #[test]
 fn test_rename_to_reserved_word_errors() {
     let code = "foo <- 1\n";