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

fix: willRename source path #248

Merged
merged 3 commits into from
Dec 10, 2023
Merged

Conversation

luckasRanarison
Copy link
Contributor

@luckasRanarison luckasRanarison commented Dec 6, 2023

Closes #247.
I think getting the relative path is unnecessary because the glob pattern will always match the full path, and it can mess up with some server filters, for example: tsserver uses **/*.js but the relative path omit the parent dir.

Copy link
Owner

@stevearc stevearc left a comment

Choose a reason for hiding this comment

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

This may fix the immediate problem, but it will introduce more issues. Any pattern that doesn't start with ** will fail to match because we're passing in the absolute path to the file. The root issue here is vim's glob2regpat does not handle ** the way we expect. See:
:echo glob2regpat('*.js') -> \.js$
:echo glob2regpat('**/*.js') -> /.*\.js$
:echo glob2regpat('**/**/*.js') -> /.*/.*\.js$

This is technically correct by some definitions, but does not behave the way LSP expects.

I think the best solution we have available right now is to use the private method vim.lsp._watchfiles._match when it's available, and fall back to glob2regpat when it's not.

https://github.com/neovim/neovim/blob/ca7f8786a0eb578895400e23cd21e25cc0f91800/runtime/lua/vim/lsp/_watchfiles.lua#L83-L89

@luckasRanarison
Copy link
Contributor Author

luckasRanarison commented Dec 6, 2023

Any pattern that doesn't start with ** will fail to match because we're passing in the absolute path to the file.

This is actually not true, try the following:

local pat = vim.fn.glob2regpat("*.js")
local match = vim.fn.match("/dir/file.js", pat)
print(match) -- 9

@stevearc
Copy link
Owner

stevearc commented Dec 6, 2023

Ah, I was overly broad. Yes it's true that a pattern of *.js also works, but if the pattern doesn't start with a * or **, then it will fail. Example:

local pat = vim.fn.glob2regpat("src/*.js")
local match = vim.fn.match("/home/user/project/src/file.js", pat)
print(match) -- -1

@luckasRanarison
Copy link
Contributor Author

luckasRanarison commented Dec 7, 2023

Oh, I see. But I think it's unlikely going to happen, I think FileOperartionPattern is meant to be matched against the absolute path. tsserver, rust_analyzer and php actor which support willRenameFiles all use **/*.filetype.

I'll update this later and use vim.lsp._watchfiles and fallback to both absolute path and relative path as a workaround if you're concerned about it.

@stevearc
Copy link
Owner

Thanks for the PR!

@stevearc stevearc merged commit 24027ed into stevearc:master Dec 10, 2023
7 checks passed
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.

bug: LSP willRenameFiles not work
2 participants