Renaming lifetimes doesn't work (protocol error: InvalidParams: Invalid name pallete_view: not a lifetime identifier)

[dogunbound]

Summary

Make a struct with lifetime requirements. Try renaming them with <space>-r. You should get this error: protocol error: InvalidParams: Invalid name pallete_view: not a lifetime identifier

2023-02-19.16-40-14.mp4

Reproduction Steps

Open file with helix

make a struct with lifetimes, or use this one:

struct MyStruct<'lifetime_1> {
  m: &'lifetime_1 i32
}

Try renaming 'lifetime_1 to 'a with <space>-r

Helix log

helix.log

Platform

Linux

Terminal Emulator

Konsole

Helix Version

22.12

[poliorcetics]

What’s your Rust-analyzer version ?

[dogunbound]

What’s your Rust-analyzer version ?

[dogunbound@archlinux ~]$ rust-analyzer --version
rust-analyzer 1.66.1 (90743e7 2023-01-10)

[poliorcetics]

Could you try it with the most recent one (either building it from source or using the latest release), to see if you reproduce ?

[dogunbound]

Could you try it with the most recent one (either building it from source or using the latest release), to see if you reproduce ?

[dogunbound@archlinux ~]$ rust-analyzer --version
rust-analyzer 1.67.1 (d5a82bb 2023-02-07)

Still getting: protocol error: InvalidParams: Invalid name a: not a lifetime identifier

[the-mikedavis]

It looks like rust-analyzer needs the ' in the lifetime replacement's name: 'a works fine for me but a alone gives this error. This behavior is in rust-analyzer though rather than Helix

[dogunbound]

It looks like rust-analyzer needs the ' in the lifetime replacement's name: 'a works fine for me but a alone gives this error. This behavior is in rust-analyzer though rather than Helix

As true as this is, helix isn't telling my I need the ' character, nor does it automatically put a ' character at the front of the replace. This is a helix bug imo

[the-mikedavis]

You would expect to see 'lifetime_1 in the prompt then, right?

This was discussed a bit in #2459: we use the current word as an approximation of the symbol. Adding support for LSP Prepare Rename (https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_prepareRename) would most likely give us the range of 'lifetime_1 for this request and then we could use that range to fill in the renaming prompt.

[dogunbound]

You would expect to see 'lifetime_1 in the prompt then, right?

This was discussed a bit in #2459: we use the current word as an approximation of the symbol. Adding support for LSP Prepare Rename (https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_prepareRename) would most likely give us the range of 'lifetime_1 for this request and then we could use that range to fill in the renaming prompt.

It's a helix issue to not auto-fill the rename with '. When you do <space>-r, it will fill it with lifetime_name, but it should fill it with 'lifetime_name. Atleast hint that it needs the' character in the front of it.

[dogunbound]

2023-02-20.17-41-37.mp4

[pascalkuthe]

Helix has no concept of a lifetime or anything rust/language specific. As @the-mikedavis said the way to implement this is to add support for https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_prepareRename to helix.

I don't think word would work as ' is not part of word only WORD (which we don't want to use here I think)

[askreet]

I started trying to implement PrepareRename for pre-filling the prompt but got stuck on the way callbacks work within this code.

My changes are here.

The issue I'm having is I need to use a callback to wait for the response from prepare_rename before starting a rename prompt, but I can't move Context between threads, which all the prompt compositor code requires.

@the-mikedavis @pascalkuthe do you have any insights here? Thanks!

[askreet]

I started down another path (here, very "crashy") where I block for the response to PrepareRename in the main thread. It's a bit laggy if you do this while the LSP is initializing, however it does work.

What's interesting is the range provided by the LSP does not include the ' character. After some experimentation I cannot reproduce this issue with:

helix 22.12 (96ff64a8)
rust-analyzer 0.3.1394-standalone (0b32b65ca 2023-02-05)

(I have no idea how rust-analyzer versions work.)

I still think my WIP branch is worth pursuing since PrepareRename is more likely to be correct than just expanding a word boundary, at least for some languages.

[the-mikedavis]

You will want to use cx.callback to handle the future from Client::prepare_rename asynchronously, create the Prompt from within there and push it to the compositor with compositor.push rather than ui::prompt_with_input. I pushed a branch here: https://github.com/helix-editor/helix/compare/lsp-prepare-rename. With that change, I see rust-analyzer sending the range for 'lifetime_1. Feel free to PR against that branch or finish implementing based on that and PR against master. There's an unimplemented!s, an expect() and the TODOs to clean up before that would be in a good place to merge.

[askreet]

Thanks @the-mikedavis! I appreciate the help, look forward to seeing what I had wrong with the borrowck and range selection rules.

Edit: This must be unique to my version of rust-analyzer or something. With your branch as well, I am asked to rename e.g. 'a use prefill a regardless of whether the cursor is on the ' or the a character. In either rate it works so I'll pursue polishing it up and submitting it.