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.

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

POC for "rename" functionality #676

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Tuxified
Copy link

@Tuxified Tuxified commented Feb 11, 2022

LSPs can offer a rename capability to rename a "symbol" across a workspace.
We're not there yet, but this is a starting point. In VS Code it can be used by
right clicking a symbol (currently variable or call to local function) and
selecting "Rename symbol". A dialog should pop up asking for a new name.

Official spec:
https://microsoft.github.io/language-server-protocol/specifications/specification-current/#textDocument_rename

Please consider this as a starting point for a discussion on how to work towards (fully) implementing this functionality (this is my first PR for this project, so I'm not sure how you usually do this ;) )

This implementation is based on:

  • Elixir 1.13s Code.Fragment.surround_context/2 (to understand what kind of symbol/token we're dealing with)
  • ElixirSense.definition & ElixirSense.references (to figure out at which locations we need changes)

I'm unfamiliar with ElixirSense & ElixirLS, so some things are clumsy and are up for discussion:

  • ElixirSense.definition returns the first definition (so a function with multiple heads don't get fully renamed)
  • ElixirSense.definition & references return %Location{} without file (so POC only works for rename across current file)
  • LSP communicates with 0-based locations (line & column) but ElixirSense, Code.Fragment etc use 1-based locations so the translation look clumsy ( +1 & -1 on wrong places?)

A small preview:
rename_symbol_v0 1

Looking forward to your thoughts and I wish you a nice weekend in advance. Thanks for all your hard work, it was really helpful to be able to rely on ElixirSense for most of this :D 🤗

[edit]

This PR addresses the rename part as requested in #436

LSPs can offer a rename capability to rename a "symbol" across a workspace.
We're not there yet, but this is a starting point. In VS Code it can be used by
right clicking a symbol (currently variable or call to local function) and
selecting "Rename symbol". A dialog should pop up asking for a new name.

Official spec:
https://microsoft.github.io/language-server-protocol/specifications/specification-current/#textDocument_rename
@lukaszsamson
Copy link
Collaborator

Looks promising @Tuxified. I've a few small comments and I hope they'll help you with making it complete.

Elixir 1.13s Code.Fragment.surround_context/2 (to understand what kind of symbol/token we're dealing with)

I tried porting elixir 1.13 goodness from Code.Fragment to earlier elixir versions (I did that for 1.12 stuff) but without 1.13 tokenizer those features cannot work. We need to either do some kind of a backport (see e.g. ElixirSense.Source.subject) or decide we only support this on 1.13+

ElixirSense.definition & references return %Location{} without file (so POC only works for rename across current file)

nil file means the current text buffer. When it returns a binary, it's a path.

ElixirSense.definition returns the first definition (so a function with multiple heads don't get fully renamed)

It's stems from a limitation of EIP48 document format - it only keeps the first position. Parse the file with elixir sense. Metadata will store all found defs

LSP communicates with 0-based locations (line & column) but ElixirSense, Code.Fragment etc use 1-based locations so the translation look clumsy ( +1 & -1 on wrong places?)

Not only that. Positions in elixir and elixir_sense are in UTF8 code points, LSP needs UTF16

- Make rename_example a `.ex` file so we can later test multi-file
  refactors
- Simplify rename_example to make it easier to follow
- Extract some common patterns out in the tests
@timgent
Copy link

timgent commented Oct 21, 2022

@lukaszsamson @Tuxified I've started a branch off of this PR to resolve those outstanding issues. Rename functionality has been updated to rename all function headers and across multiple files. I still need to fix the prepare functionality across files though. That'll probably not be for another couple of weeks when I get time.

In the meantime I ran into some trouble when trying out the updated language server with vim.

[coc.nvim] Error on applyEdits: Error: file:///.... changed before apply edit

The rename does work with other language servers so I don't think it's an issue with CoC. Just wanted to check if you had any pointers. I'll work on debugging that anyway.

In terms of version support, supporting only on 1.13+ seems like a reasonable first step --> better on the 2 latest versions than not at all? WDYT?

Not only that. Positions in elixir and elixir_sense are in UTF8 code points, LSP needs UTF16

Are there implications for this PR in terms of the different encoding of code points?

Fix renaming functions with multiple headers and across multiple files
@lukaszsamson lukaszsamson mentioned this pull request Nov 8, 2022
12 tasks
@timgent
Copy link

timgent commented Nov 11, 2022

Cross-posting here - some more fixes for this. I've PR'd to the fork to add improvements to this branch.

Tuxified#2

@rupurt
Copy link
Contributor

rupurt commented Mar 16, 2023

How can I help get this over the line? 😄

@timgent
Copy link

timgent commented Mar 16, 2023

@rupurt checkout the latest comments on this version of the PR - #791 - which is slightly further along.

I've recently had a kid so not going to have time to work on it in the foreseeable

@rupurt
Copy link
Contributor

rupurt commented Mar 16, 2023

Right on, thanks @timgent. Congrats on the new addition!! 👶 🎂

You've gone beast mode here already so more than happy to chip in.

@rupurt
Copy link
Contributor

rupurt commented Apr 7, 2023

Rebased with master and started trying to comprehend the work you've done and how it needs to be updated to handle the new app structure along with ElixirSense v2. Thank you for creating the examples and failing tests @timgent. Gives me a great starting point.

https://github.com/elixir-lsp/elixir-ls/compare/master...rupurt:elixir-ls:rupurt/tuxified/poc_rename_symbol?expand=1

@timgent
Copy link

timgent commented Apr 7, 2023

Nice one @rupurt , looking forward to seeing how you get on. Let me know if you need a hand understanding any of the stuff that is there so far

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants