-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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: using changedtick as the document version breaks workspace/executeCommand usage #29163
Comments
Repro:init.lua vim.api.nvim_create_autocmd("FileType", {
pattern = "go",
callback = function()
vim.lsp.start({ cmd = { "gopls" } })
end
}) main.go // delete me
package main
func main() {
fmt.Println()
} Steps:
|
Thanks for the repro. |
The change seems reasonable to me (I tested locally and can't break it). But I suspect we're going to have similar issues with the semantic tokens & inlay hints. If that does end up being the case and more edge-case handling is required, I think it might make sense to revert #28943 |
I was wondering about that too, but can't think of a scenario where it could be a problem. If the buffer is unchanged (modified=false), there shouldn't be any updates for semantic tokens, inlay hints, code lens and so on anyway. |
I'm having a difficult time figuring out how a plugin should call The original (current) implementation has the problem where the version is being rejected on the server side because it's too new. local bufnr = util.validate_bufnr(opts.bufnr or 0)
request(0, 'workspace/executeCommand', {
command = 'eslint.applyAllFixes',
arguments = {
{
uri = vim.uri_from_bufnr(bufnr),
version = lsp.util.buf_versions[bufnr],
},
},
}) I tried rewriting it to use lsp.buf.code_action({
apply = true,
filter = function(a)
return vim.tbl_get(a, 'command', 'command') == 'eslint.applyAllFixes'
end,
}) I would need to use the unexported lsp.buf.code_action({
apply = true,
context = {
diagnostics = diagnostic_vim_to_lsp(vim.diagnostic.get(0)),
},
filter = function(a)
return vim.tbl_get(a, 'command', 'command') == 'eslint.applyAllFixes'
end,
}) This feels like a lot of hoops to jump through to do something relatively simple. |
I'm not sure how to best deal with that but I'm reluctant to revert the change. One option that would work is to apply the same modified check as in the linked fix ( Another option could be to simplify the code_action API by:
In general it's a bit unusual that the server even requires a version parameter with the request. Can't it be set to |
It could live in
edit it can't live in changetracking due to circular dependencies.
This breaks if you open a file with existing issues.
Yes, this is true. I've tried to find other examples to strengthen my case and have came up empty handed.
This was the first thing I tried. No luck. |
I've been thinking about this a little more and I'm starting to question the use of local M = {}
---@type table<integer, integer>
local buf_versions = {}
---@param bufnr integer
---@return integer
function M.get(bufnr)
-- we default to 1 instead of 0 because some LSPs return 0
-- when they don't support versioning. Starting at 1 disambiguates
-- that case.
return buf_versions[bufnr] or 1
end
---@param bufnr integer
function M.changed(bufnr)
buf_versions[bufnr] = M.get(bufnr) + 1
end
return M |
I've noticed recently as well (even after the above follow up fix for the modify case) that semantic tokens aren't updating as reliably as they were. When I first implemented that feature, I remember looking into changedtick vs the way we were tracking versions and came to the conclusion at the time that we couldn't use changedtick and what we were doing was necessary, but I don't remember exactly how I came to that conclusion. I'll try to dig into that again at some point soon to see if I can remember why changedtick wasn't sufficient. |
I've also noticed some strangeness with semantic tokens, but I haven't been able to come up with a repro. |
Ok I haven't 100% confirmed this, but I think I know why I'm occasionally seeing some weird stuff with semantic tokens. So, the crux of the issue is that b:changedtick can change without an Specifically for semantic tokens this means that when a request comes back and the changedtick number changed without firing on_lines, we drop the response without ever sending a new request. Because we actually only care about changes to a buffer that occur with an on_lines callback, that is why we needed a separate version--one that only increments when we actually get that callback (so we send change notifications, request new tokens, etc). |
Reverted it for now #29217 |
Problem
#28943 changed the lsp code to use
vim.b[bufnr].changedtick
as the LSP document version.Now there’s no way to send the correct document version in a
workspace/executeCommand
after saving a modified buffer. This is becausechangedtick
is incremented when "Resetting modified when writing the buffer" https://neovim.io/doc/user/eval.html#b:changedtick but the server is not notified of this new version.Steps to reproduce
textDocument/didChange
(version 1)changedtick
is incremented to version 2textDocument/didSave
(no version sent):EslintFixAll
)workspace/executeCommand
(version 2)Expected behavior
Some way to get the last sent buffer version for
workspace/executeCommand
usage.Neovim version (nvim -v)
NVIM v0.11.0-dev-158+g05435a915
Vim (not Nvim) behaves the same?
N/A
Operating system/version
Ubuntu 22.04.4 LTS
Terminal name/version
alacritty 0.12.2
$TERM environment variable
alacritty
Installation
bob
The text was updated successfully, but these errors were encountered: