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

LSP: Use workspace/willRenameFiles to handle rename resource operations #8942

Closed
the-mikedavis opened this issue Nov 29, 2023 · 4 comments · Fixed by #8949
Closed

LSP: Use workspace/willRenameFiles to handle rename resource operations #8942

the-mikedavis opened this issue Nov 29, 2023 · 4 comments · Fixed by #8949
Labels
A-language-server Area: Language server client C-bug Category: This is a bug

Comments

@the-mikedavis
Copy link
Member

With the addition of LSP renaming in #8584 we now declare willRename support - that we will send a workspace/willRenameFiles before renaming a file. The spec says that this should be used for workspace edits as well as user commands (emphasis mine):

The will rename files request is sent from the client to the server before files are actually renamed as long as the rename is triggered from within the client either by a user action or by applying a workspace edit.

Currently we just rename the file when receiving a workspace edit that attempts to rename a file (which was the proper behavior when we didn't declare willRename support):

ResourceOp::Rename(op) => {
let from = op.old_uri.to_file_path().unwrap();
let to = op.new_uri.to_file_path().unwrap();
let ignore_if_exists = op.options.as_ref().map_or(false, |options| {
!options.overwrite.unwrap_or(false) && options.ignore_if_exists.unwrap_or(false)
});
if ignore_if_exists && to.exists() {
Ok(())
} else {
fs::rename(from, &to)
}

But this needs to be updated to use willRenameFiles.

This causes a bug with rust analyzer for example, see #8923.

@the-mikedavis the-mikedavis added C-bug Category: This is a bug A-language-server Area: Language server client labels Nov 29, 2023
@the-mikedavis
Copy link
Member Author

@pascalkuthe I know that you have been looking at this block recently. Is this something you are interested in solving? If not we could add the E-help-wanted label

@pascalkuthe
Copy link
Member

pascalkuthe commented Nov 29, 2023

Yeah I will try to do it all in one go (otherwise the PRs would just conflict). That's actually a useful pointer, great find! I really need to find where VScode Handels all of this but I can't find the right place in the codebase.

@matoous
Copy link
Contributor

matoous commented Nov 29, 2023

@pascalkuthe https://github.com/microsoft/vscode/blob/main/src/vs/workbench/api/common/extHostFileSystemEventService.ts#L248 I think this might be the place to start. Would be nice to end up with something similar in Helix (as I was recently looking at willSave which could also benefit from a streamlined interface for document/file management).

@pascalkuthe
Copy link
Member

this feels like a job for the event system TBH but until that is merged having a workaround would be nice I guess. I think I will start out just fixing this issue (and the related PR) so we can get rid of the regression. A general system like that seems perfect for the event system once that lands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-language-server Area: Language server client C-bug Category: This is a bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants