Fail early when entity at cursor is not renamable

Author: lionel-

crates/ark/src/lsp/rename.rs

@@ -29,7 +29,7 @@ pub(crate) fn prepare_rename(
 
     let offset = from_proto::offset_from_position(position, file.line_index(db), encoding)?;
 
-    let Some((range, placeholder)) = oak_ide::prepare_rename(db, file, offset) else {
+    let Some((range, placeholder)) = oak_ide::prepare_rename(db, file, offset)? else {
         return Ok(None);
     };
 

crates/oak_ide/src/rename.rs

@@ -6,6 +6,7 @@ use oak_db::Db;
 use oak_db::Definition;
 use oak_db::File;
 use oak_db::Identifier;
+use oak_db::Name;
 use oak_db::RootKind;
 
 use crate::find_references;
@@ -22,22 +23,28 @@ pub struct RenameTargets {
 }
 
 /// Identify the renamable identifier at `offset`, returning its range and
-/// current (unquoted) name. Returns `None` when the cursor is on a
-/// non-identifier, a `pkg::sym` namespace access, or a `$`/`@` member name.
-pub fn prepare_rename(db: &dyn Db, file: File, offset: TextSize) -> Option<(TextRange, String)> {
-    match Identifier::classify(db, file, offset)? {
-        Identifier::Variable { name, range } => Some((range, name.text(db).to_string())),
-        Identifier::Member { .. } => None,
-    }
+/// current (unquoted) name.
+///
+/// Returns `Ok(None)` when the cursor isn't on something we can rename (a
+/// non-identifier, a `pkg::sym` namespace access, or a `$`/`@` member name),
+/// so the client simply offers no rename. Returns `Err` when the cursor is on
+/// a renamable identifier that we still refuse, today only a symbol defined in
+/// an installed package, so the client can surface why at prepare time.
+pub fn prepare_rename(
+    db: &dyn Db,
+    file: File,
+    offset: TextSize,
+) -> anyhow::Result<Option<(TextRange, String)>> {
+    Ok(renamable_at(db, file, offset)?.map(|(range, name)| (range, name.text(db).to_string())))
 }
 
 /// Compute all rename edits across the database.
 ///
 /// Returns `Err` when:
 /// - `new_name` is empty, is an R reserved word, or contains a literal
 ///   backtick (which can't appear in a backtick-quoted identifier).
-/// - The cursor isn't on a renamable identifier (no `prepare_rename()` target).
-/// - Any target definition lives in an installed package.
+/// - The cursor isn't on a renamable identifier, or it resolves to an
+///   installed package (both via `renamable_at`).
 /// - Nothing in the database binds the cursor's symbol. Rename would
 ///   produce no edits, so we refuse rather than silently succeed.
 pub fn rename(
@@ -48,19 +55,10 @@ pub fn rename(
 ) -> anyhow::Result<RenameTargets> {
     let new_text = to_identifier_text(new_name)?;
 
-    let ident = Identifier::classify(db, file, offset);
-    let Some(Identifier::Variable { range, .. }) = &ident else {
+    let Some(_) = renamable_at(db, file, offset)? else {
         return Err(anyhow!("Can't rename identifier at cursor."));
     };
 
-    for def in file.resolve_at(db, range.start()) {
-        if is_library_def(db, def) {
-            return Err(anyhow!(
-                "Can't rename: symbol is defined in an installed package."
-            ));
-        }
-    }
-
     let ranges = find_references(db, file, offset, true);
     if ranges.is_empty() {
         return Err(anyhow!(
@@ -71,6 +69,26 @@ pub fn rename(
     Ok(RenameTargets { ranges, new_text })
 }
 
+fn renamable_at<'db>(
+    db: &'db dyn Db,
+    file: File,
+    offset: TextSize,
+) -> anyhow::Result<Option<(TextRange, Name<'db>)>> {
+    let Some(Identifier::Variable { name, range }) = Identifier::classify(db, file, offset) else {
+        return Ok(None);
+    };
+
+    for def in file.resolve_at(db, range.start()) {
+        if is_library_def(db, def) {
+            return Err(anyhow!(
+                "Can't rename: symbol is defined in an installed package."
+            ));
+        }
+    }
+
+    Ok(Some((range, name)))
+}
+
 fn is_library_def(db: &dyn Db, def: Definition) -> bool {
     def.file(db)
         .root(db)

crates/oak_ide/tests/integration/rename.rs

@@ -48,7 +48,7 @@ fn test_prepare_rename_on_def() {
     let mut db = OakDatabase::new();
     let file = upsert(&mut db, "test.R", "foo <- 1\nfoo\n");
 
-    let result = prepare_rename(&db, file, offset(0)).unwrap();
+    let result = prepare_rename(&db, file, offset(0)).unwrap().unwrap();
     assert_eq!(result, (range(0, 3), "foo".to_string()));
 }
 
@@ -57,7 +57,7 @@ fn test_prepare_rename_on_use() {
     let mut db = OakDatabase::new();
     let file = upsert(&mut db, "test.R", "foo <- 1\nfoo\n");
 
-    let result = prepare_rename(&db, file, offset(9)).unwrap();
+    let result = prepare_rename(&db, file, offset(9)).unwrap().unwrap();
     assert_eq!(result, (range(9, 12), "foo".to_string()));
 }
 
@@ -66,15 +66,24 @@ fn test_prepare_rename_namespace_access_returns_none() {
     let mut db = OakDatabase::new();
     let file = upsert(&mut db, "test.R", "dplyr::mutate\n");
 
-    assert!(prepare_rename(&db, file, offset(7)).is_none());
+    assert!(prepare_rename(&db, file, offset(7)).unwrap().is_none());
 }
 
 #[test]
 fn test_prepare_rename_non_identifier_returns_none() {
     let mut db = OakDatabase::new();
     let file = upsert(&mut db, "test.R", "x <- 1\n");
 
-    assert!(prepare_rename(&db, file, offset(3)).is_none());
+    assert!(prepare_rename(&db, file, offset(3)).unwrap().is_none());
+}
+
+#[test]
+fn test_prepare_rename_library_package_symbol_errors() {
+    let mut db = OakDatabase::new();
+    let lib_file = build_library_package_file(&mut db, "foo <- function() {}\n");
+
+    let err = prepare_rename(&db, lib_file, offset(0)).unwrap_err();
+    assert!(err.to_string().contains("installed package"));
 }
 
 // --- rename: basic ---