neovim icon indicating copy to clipboard operation
neovim copied to clipboard

sync version of vim.lsp.buf.rename

Open dodomorandi opened this issue 2 years ago • 7 comments

Problem

I needed to perform an LSP replace using the same pattern on different files on a project. Easy enough: live_grep with Telescope, send to quickfix, cdo with some positioning and then running vim.lsp.buf.rename("newName"), right? Unfortunately it does not work, it only changes the last file in a broken way.

It took me a bit to realize that vim.lsp.buf.rename works asynchronously. I don't exactly understand why the problem manifests itself in this particular way, but anyway using a sync approach for the operation is probably the right thing to when performing this sort of automation.

Expected behavior

The documentation should suggest to avoid using rename for scripting purposes, and it should suggest its sync alternative (which obviously should exist). I am not totally sure if the behavior should depend on a parameter (similarly to vim.lsp.buf.format, but using async = true by default instead) or if it should be a separated function (i.e. vim.lsp.buf.rename_sync). In theory, it could be also helpful to have a callback parameter to the current rename function to be called after the operation ends, however this is not so useful when using cdo and similar functions, therefore out of scope for this issue.

dodomorandi avatar Mar 23 '24 22:03 dodomorandi

Can you use vim.wait()? https://neovim.io/doc/user/lua.html#vim.wait()

If not, this is blocked until we have a Task/Promise abstraction. https://github.com/neovim/neovim/issues/19624 We are not going to add _sync or _async variants of everything.

justinmk avatar Mar 27 '24 15:03 justinmk

Thank you for your reply @justinmk. Unfortunately I don't think that vim.wait would help (at least without long timeouts), because the issue is that if more than one rename operation overlaps, things start to go crazy.

You are totally right that a Task/Promise abstraction is exactly what would be ideal in this case, but I also think that it is not necessary to add async variants for everything. I mean, I don't think there are many others (if any) lsp.buf that explode so badly if run multiple times in a short time, and we could also use an async option instead of having a function variant for this specific case (which have been already done for vim.lsp.buf.format).

Still, I could understand if you would rather to wait for a better Task/Promise abstraction and to avoid a temporary sol-hack-ution. I already made my hackish auxiliary sync rename function that call private methods in order to solve my specific problem... but it is not a good idea to publish it as a plugin for obvious reasons :sweat_smile:. Probably, it is difficult to estimate if any other person encountered the same issue and just opted to "whatever, let's do it by hand", therefore I do not know if introducing this change could be actually useful to other people.

Let me know if you would like me to send a PR to introduce an async option (I think we are already excluding the _sync function variant) or if you would rather to close the issue. In any case no worries :relaxed:

dodomorandi avatar Mar 27 '24 17:03 dodomorandi

I understand that sending async rename requests to a language server could be problematic. However, in this specific case, I don't see why it would be necessary.

Rename works across files in the workspace, so there should be no need to find all occurrences manually and so on. That's the whole point.

So given your files are in the same workspace, doing a single rename is sufficient as long as its the same token in the workspace. If they are different tokens, then it would cause other types of problems, like the server refusing to rename be because the token is already taken.

bluebrown avatar Apr 29 '24 15:04 bluebrown

@bluebrown unfortunately that is not true, otherwise I would not have encountered the issue in the first place. You are saying that it is not possible to be in my situation so... ok I think? As I said, I created myself a sync version of the feature, and I used it to solve my problem. I opened this issue because I think that other people could be limited in the same way I have been, if the issue does not exist feel free to close it :man_shrugging:

dodomorandi avatar Apr 29 '24 16:04 dodomorandi

@bluebrown unfortunately that is not true, otherwise I would not have encountered the issue in the first place. You are saying that it is not possible to be in my situation so... ok I think? As I said, I created myself a sync version of the feature, and I used it to solve my problem. I opened this issue because I think that other people could be limited in the same way I have been, if the issue does not exist feel free to close it 🤷‍♂️

What do you mean by that's not true?

Edit: I don't doubt it is possible to run into this, but I wonder how exactly. Due to the reasons mentioned, I don't think most users would ever run into this, since normally the rename working across the entire workspace is sufficient. Just find 1 occurrence and perform the rename.

bluebrown avatar Apr 29 '24 21:04 bluebrown

What do you mean by that's not true?

I mean that the fact that

Rename works across files in the workspace, so there should be no need to find all occurrences manually and so on. That's the whole point.

So given your files are in the same workspace, doing a single rename is sufficient as long as its the same token in the workspace. If they are different tokens, then it would cause other types of problems, like the server refusing to rename be because the token is already taken.

is not true in absolute terms. Generally using LSP rename is sufficient, but in some specific cases it is not enough. Take the following line:

import myA from '../A';

And imagine you have this all around your code, and you need a refactor because previously you were directly using myA, but now you need to wrap it inside some kind of helper and you would like to keep using the wrapped version as myA. What you probably want to do is create a wrapped instance of myA and then rename the original import to myAOriginal (or something similar). But this is an arbitrary name that exists only inside this specific module, not on other ones. Therefore my initial point: you need to find the occurrences of this pattern and then perform automatic refactor including rename, and you need the sync version otherwise things start to break.

dodomorandi avatar Apr 30 '24 07:04 dodomorandi

@dodomorandi , thanks for explaining. There are langauges that have file local or private tokens, so you are right.

bluebrown avatar May 03 '24 19:05 bluebrown