Add support for renaming local variables by lionel- · Pull Request #1232 · posit-dev/ark

lionel- · 2026-05-26T07:07:13Z

Branched from #1231

Addresses posit-dev/positron#13749
Part of #1149

Implements rename for local definitions. If a symbol is unbound locally, an error is emitted. Cross-file renaming will come along with the Salsa infrastructure.

See usage in screencast. The most surprising behaviour is with nested lazy contexts where the enclosing snapshot reaches definitions and uses below the context. That's a consequence of the over-approximation we inherit from ty. The alternative would be to perform full call analysis to determine exactly what a nested symbol can reach (intractable in general).

Screen.Recording.2026-05-26.at.08.56.49.mov

See user-facing errors in second screencast, when trying to rename something not renamable, or to an invalid identifier. There should be no backtraces in user visible popups.

Screen.Recording.2026-05-26.at.08.57.32.mov

DavisVaughan · 2026-05-27T17:24:39Z

+/// word) returns as-is. Anything else gets wrapped in backticks. Empty
+/// names, reserved words, and names containing a literal backtick return
+/// `Err` (a backtick can't appear inside a backtick-quoted identifier).
+pub fn to_identifier_text(name: &str) -> anyhow::Result<String> {


I slightly question the decision to use anyhow in our lowest level crate

Typically anyhow isn't used in a "library" style crate, but our use case is a bit blurry because our only consumers won't care much about what type of error this is.

If you care about this, I would not be against seeing a structured IdentifierError or something

ACTUALLY I now see that these errors actually make their way to the USER when they type in an invalid name!

In that case I now feel even stronger that they should be a structured error type, with a note that they display to the user as is, and maybe even some tests to validate their behavior if you dont have any yet

(GREAT user experience though!)

hmm I'm very skeptical about the benefits of proactively creating error types unless we need specific handling somewhere in the stack, and quite confident about the costs.

DavisVaughan · 2026-05-27T17:25:55Z

+/// A name that parses as a plain R identifier (and isn't a reserved
+/// word) returns as-is. Anything else gets wrapped in backticks. Empty
+/// names, reserved words, and names containing a literal backtick return
+/// `Err` (a backtick can't appear inside a backtick-quoted identifier).


Can we return a Cow<str>? I feel like 95% of the time we will have a valid identifier

(I see it won't matter much for our current usage, but it still seems useful in case we use this a lot)

Returning Cow only helps if RenameTargets::new_text is also Cow (so the caller doesn't .into_owned() and re-allocate). But that would require adding a lifetime to RenameTargets, which doesn't feel great right now.

DavisVaughan · 2026-05-27T17:27:44Z

+pub fn is_valid_identifier(name: &str) -> bool {
+    let mut chars = name.chars();
+    let Some(first) = chars.next() else {
+        return false;
+    };
+    if !(first.is_ascii_alphabetic() || first == '.') {
+        return false;
+    }
+    if first == '.' {
+        if let Some(second) = name.chars().nth(1) {
+            if second.is_ascii_digit() {
+                return false;
+            }
+        }
+    }
+    chars.all(|c| c.is_ascii_alphanumeric() || c == '.' || c == '_')
+}
+
+/// R reserved words that cannot be used as identifier names. Source:
+/// `?Reserved` in R. Note that `return` is a function, not a reserved
+/// word, so it's missing from this list (`return <- 1` is valid R).
+/// `_` became reserved in R 4.2 for use as the `|>` pipe placeholder.
+pub fn is_reserved(name: &str) -> bool {


are you sure you want these to be pub?

I'm agnostic about it since it's an internal crate.

DavisVaughan · 2026-05-27T17:31:43Z

+    let Some(first) = chars.next() else {
+        return false;
+    };
+    if !(first.is_ascii_alphabetic() || first == '.') {


i think this will fail for valid non ascii identifiers, like μ <- 1

getting it fully correct might be Extremely Hard™

oops of course

surprisingly easy!

DavisVaughan · 2026-05-27T17:33:39Z

+}
+
+/// R reserved words that cannot be used as identifier names. Source:
+/// `?Reserved` in R. Note that `return` is a function, not a reserved


funny how we both touched this this week r-lib/tree-sitter-r#197

DavisVaughan · 2026-05-27T17:46:07Z

+    match ident {
+        Identifier::Definition { def, name, .. } => Some((def.range(), name.to_string())),
+        Identifier::Use { use_site, name, .. } => Some((use_site.range(), name.to_string())),
+        Identifier::NamespaceAccess { .. } => None,


I'm not entirely sure what this should even do with NamespaceAccesses

Would only work when the package is in the workspace. Sometimes you've got to qualify your own package for reason x or y (I had to do it in rlang). Sometimes you're working on two packages at a time and one of them calls the other with qualified names.

DavisVaughan · 2026-05-27T17:54:58Z

+    params: TextDocumentPositionParams,
+    state: &WorldState,
+) -> LspResult<Option<PrepareRenameResponse>> {
+    // Don't propagate errors to the frontend


Would you mention why? Because the protocol definitely encourages it

error: code and message set in case the element can’t be renamed. Clients should show the information in their user interface.

IIUC, it is because prepare_rename() isn't set up to return actionable errors to the user right now

I copied ty's behaviour which doesn't propagate. And then I got super confused when I demo'd rename to Hadley because I was expecting an error right away but instead had to pick a name first! We now propagate errors as recommended in the spec, good catch.

DavisVaughan · 2026-05-27T18:01:46Z

+    if is_valid_identifier(name) {
+        return Ok(name.to_string());
+    }
+    if name.contains('`') {


I think you should add a special case before this of "starts with and ends with a backtick"

I tried renaming a symbol to

`i know what im doing`

and it refused to allow me to do so, but i think this should be allowed. i know i can drop the backticks here, but i feel like i shouldnt have to because im an "expert" and gave you the right thing to begin with

Makes sense!

lionel- force-pushed the oak/rename-intra-file branch from 233e283 to 0bead42 Compare May 26, 2026 07:28

lionel- requested a review from DavisVaughan May 26, 2026 15:37

DavisVaughan approved these changes May 27, 2026

View reviewed changes

lionel- force-pushed the oak/find-refs-intra-file branch from e150326 to 2bc2610 Compare May 27, 2026 20:58

lionel- force-pushed the oak/rename-intra-file branch from 0bead42 to d34bb18 Compare May 27, 2026 22:46

lionel- force-pushed the oak/find-refs-intra-file branch from 82d3822 to 014546a Compare May 27, 2026 22:50

Base automatically changed from oak/find-refs-intra-file to main May 27, 2026 22:50

lionel- added 9 commits May 28, 2026 00:50

Support within-file renames

a73dbf2

More accurate error messages

5cfc763

Don't include backtraces in user-facing RPC errors

31b5ec3

Rename FileOffset to FilePosition

30e13e2

Extract helpers in LSP test client

6458dd7

Address code review

e3f942f

Propagate errors to the frontend

39d7134

Allow user to rename to a name with backticks

968a513

Allow unicode-aware letters in identifiers

3b0ed35

lionel- force-pushed the oak/rename-intra-file branch from d34bb18 to 3b0ed35 Compare May 27, 2026 22:50

lionel- merged commit 931f76d into main May 27, 2026
16 of 17 checks passed

lionel- deleted the oak/rename-intra-file branch May 27, 2026 22:51

github-actions Bot locked and limited conversation to collaborators May 27, 2026

Conversation

lionel- commented May 26, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants