Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Implement renaming for local variables #4185

Merged
merged 15 commits into from
Jan 20, 2025
Merged

Implement renaming for local variables #4185

merged 15 commits into from
Jan 20, 2025
+1,212 −71

Conversation

GearsDatapacks
Copy link
Member

@GearsDatapacks GearsDatapacks commented Jan 18, 2025 •
edited
Loading

Closes #2956
This PR implements renaming of local variables in the Language Server including:

  • Setting up the new route and server capability
  • Setting up a test environment for testing renaming
  • Tree walking to find references to the variable
  • Rejecting renames if an invalid name is provided (e.g. renaming a variable to SomeVar)

A few things I did while implementing this:

  • I used definition location to decide whether two variables were the same. That's mainly because since variable shadowing exists in Gleam, two variables with the same name don't necessarily refer to the same variable.
  • To allow renaming to also rename references to variables in clause guards, I added a definition_location field to the ClauseGuard::Var variant. Since ClauseGuard is just one type for both typed and untyped versions, this field has to be present in the untyped AST, where it has no meaning (we don't know where the variable is defined yet). That was the most straightforward solution I could find for the time being.
  • Similarly, I added clause guards to the AST visitor. I only really implemented the bare-bones for what this PR needs; it seemed a bit wasteful to implement it all. I'll probably open a separate PR to implement the full visitor for it

@GearsDatapacks GearsDatapacks marked this pull request as ready for review January 18, 2025 14:21
@GearsDatapacks GearsDatapacks mentioned this pull request Jan 18, 2025
Merged
@GearsDatapacks
Copy link
Member Author

Not sure why checks are failing... I haven't changed anything to do with deps or cargo deny

lpil
lpil reviewed Jan 19, 2025
View reviewed changes
Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahhh this is so exciting!! Amazing! I've left one question inline

compiler-core/src/language_server/tests/rename.rs Show resolved Hide resolved
@GearsDatapacks
Copy link
Member Author

I have now made it work properly with:

  • Assignment patterns (e.g. 1 as one)
  • Label shorthand
  • Arguments

One thing I wasn't totally sure about is the following:

let Wibble(wibble:) = todo
//         ^ If you put you cursor here and trigger a rename

What should happen? Should it treat is as renaming the local variable: wibble: wobble, or should it (once we have this feature implemented) rename the label itself? The latter is probably something we want eventually, however currently this PR implements the former. Let me know what you think

@GearsDatapacks
Copy link
Member Author

Just realised that the prepare_rename function wasn't up-to-date with the regular rename function. I've now fixed that and checked prepare_rename in the tests, so that shouldn't happen in the future. However, it is quite a lot code duplication. All the pattern matching has to be performed in two different places. I wonder if there's a way around this

lpil
lpil approved these changes Jan 20, 2025
View reviewed changes
Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fantastic!!! I love this!

I found a tiny bug, it's not renaming variables used in bit array literals

pub fn starts_with(bits: BitArray, prefix: BitArray) -> Bool {
  let prefix_size = bit_size(prefix)

  case bits {
    <<pref:bits-size(prefix_size), _:bits>> if pref == prefix -> True
    _ -> False
  }
}

Try renaming prefix_size here

@GearsDatapacks
Copy link
Member Author

Yay, another edge-case. I'll look into it

@GearsDatapacks GearsDatapacks mentioned this pull request Jan 20, 2025
Closed
lpil
lpil approved these changes Jan 20, 2025
View reviewed changes
Copy link
Member

@lpil lpil left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exciting!!!! You're a star!!

@lpil lpil merged commit 33ef3d7 into gleam-lang:main Jan 20, 2025
12 checks passed
@GearsDatapacks GearsDatapacks deleted the rename-variable branch January 20, 2025 19:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lpil lpil lpil approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

LSP: rename local variable
2 participants
@GearsDatapacks @lpil