From cc87dda31a5b5637ade7ddcfe5199f2df5fd47df Mon Sep 17 00:00:00 2001 From: Sean Dewar Date: Fri, 4 Aug 2023 07:10:54 +0100 Subject: [PATCH] fix(lsp): do not assume client capability exists in watchfiles check (#24550) PR #23689 assumes `client.config.capabilities.workspace.didChangeWatchedFiles` exists when checking `dynamicRegistration`, but thats's true only if it was passed to `vim.lsp.start{_client}`. This caused #23806 (still an issue in v0.9.1; needs manual backport), but #23681 fixed it by defaulting `config.capabilities` to `make_client_capabilities` if not passed to `vim.lsp.start{_client}`. However, the bug resurfaces on HEAD if you provide a non-nil `capabilities` to `vim.lsp.start{_client}` with missing fields (e.g: not made via `make_client_capabilities`). From what I see, the spec says such missing fields should be interpreted as an absence of the capability (including those indicated by missing sub-fields): https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#clientCapabilities Also, suggest `vim.empty_dict()` for an empty dict in `:h vim.lsp.start_client()` (`{[vim.type_idx]=vim.types.dictionary}` no longer works anyway, probably since the cjson switch). --- runtime/doc/lsp.txt | 3 +- runtime/lua/vim/lsp.lua | 3 +- runtime/lua/vim/lsp/_watchfiles.lua | 15 +++-- test/functional/plugin/lsp_spec.lua | 99 +++++++++++++++++------------ 4 files changed, 71 insertions(+), 49 deletions(-) diff --git a/runtime/doc/lsp.txt b/runtime/doc/lsp.txt index 1b35219bb8..da624dba65 100644 --- a/runtime/doc/lsp.txt +++ b/runtime/doc/lsp.txt @@ -988,8 +988,7 @@ start_client({config}) *vim.lsp.start_client()* passed to the language server on initialization. Hint: use make_client_capabilities() and modify its result. • Note: To send an empty dictionary use - `{[vim.type_idx]=vim.types.dictionary}`, else it will be - encoded as an array. + |vim.empty_dict()|, else it will be encoded as an array. • handlers: Map of language server method names to |lsp-handler| diff --git a/runtime/lua/vim/lsp.lua b/runtime/lua/vim/lsp.lua index b4c853d3d4..ea6b386b28 100644 --- a/runtime/lua/vim/lsp.lua +++ b/runtime/lua/vim/lsp.lua @@ -1027,8 +1027,7 @@ end --- \|vim.lsp.protocol.make_client_capabilities()|, passed to the language --- server on initialization. Hint: use make_client_capabilities() and modify --- its result. ---- - Note: To send an empty dictionary use ---- `{[vim.type_idx]=vim.types.dictionary}`, else it will be encoded as an +--- - Note: To send an empty dictionary use |vim.empty_dict()|, else it will be encoded as an --- array. --- --- - handlers: Map of language server method names to |lsp-handler| diff --git a/runtime/lua/vim/lsp/_watchfiles.lua b/runtime/lua/vim/lsp/_watchfiles.lua index c271dc6e14..5dbd4a7199 100644 --- a/runtime/lua/vim/lsp/_watchfiles.lua +++ b/runtime/lua/vim/lsp/_watchfiles.lua @@ -121,12 +121,15 @@ M._poll_exclude_pattern = parse('**/.git/{objects,subtree-cache}/**') function M.register(reg, ctx) local client_id = ctx.client_id local client = vim.lsp.get_client_by_id(client_id) - if - -- Ill-behaved servers may not honor the client capability and try to register - -- anyway, so ignore requests when the user has opted out of the feature. - not client.config.capabilities.workspace.didChangeWatchedFiles.dynamicRegistration - or not client.workspace_folders - then + -- Ill-behaved servers may not honor the client capability and try to register + -- anyway, so ignore requests when the user has opted out of the feature. + local has_capability = vim.tbl_get( + client.config.capabilities or {}, + 'workspace', + 'didChangeWatchedFiles', + 'dynamicRegistration' + ) + if not has_capability or not client.workspace_folders then return end local watch_regs = {} --- @type table diff --git a/test/functional/plugin/lsp_spec.lua b/test/functional/plugin/lsp_spec.lua index 7e34946d95..6223c6b8d8 100644 --- a/test/functional/plugin/lsp_spec.lua +++ b/test/functional/plugin/lsp_spec.lua @@ -4402,58 +4402,79 @@ describe('LSP', function() it("ignores registrations by servers when the client doesn't advertise support", function() exec_lua(create_server_definition) - local result = exec_lua([[ - local server = _create_server() - local client_id = vim.lsp.start({ - name = 'watchfiles-test', - cmd = server.cmd, - root_dir = 'some_dir', - capabilities = { - workspace = { - didChangeWatchedFiles = { - dynamicRegistration = false, - }, - }, - }, - }) - - local watching = false + exec_lua([[ + server = _create_server() require('vim.lsp._watchfiles')._watchfunc = function(_, _, callback) -- Since the registration is ignored, this should not execute and `watching` should stay false watching = true return function() end end + ]]) - vim.lsp.handlers['client/registerCapability'](nil, { - registrations = { - { - id = 'watchfiles-test-kind', - method = 'workspace/didChangeWatchedFiles', - registerOptions = { - watchers = { - { - globPattern = '**/*', + local function check_registered(capabilities) + return exec_lua([[ + watching = false + local client_id = vim.lsp.start({ + name = 'watchfiles-test', + cmd = server.cmd, + root_dir = 'some_dir', + capabilities = ..., + }, { + reuse_client = function() return false end, + }) + + vim.lsp.handlers['client/registerCapability'](nil, { + registrations = { + { + id = 'watchfiles-test-kind', + method = 'workspace/didChangeWatchedFiles', + registerOptions = { + watchers = { + { + globPattern = '**/*', + }, }, }, }, }, - }, - }, { client_id = client_id }) + }, { client_id = client_id }) - -- Ensure no errors occur when unregistering something that was never really registered. - vim.lsp.handlers['client/unregisterCapability'](nil, { - unregisterations = { - { - id = 'watchfiles-test-kind', - method = 'workspace/didChangeWatchedFiles', + -- Ensure no errors occur when unregistering something that was never really registered. + vim.lsp.handlers['client/unregisterCapability'](nil, { + unregisterations = { + { + id = 'watchfiles-test-kind', + method = 'workspace/didChangeWatchedFiles', + }, + }, + }, { client_id = client_id }) + + vim.lsp.stop_client(client_id, true) + return watching + ]], capabilities) + end + + eq(true, check_registered(nil)) -- start{_client}() defaults to make_client_capabilities(). + eq(false, check_registered(vim.empty_dict())) + eq(false, check_registered({ + workspace = { + ignoreMe = true, + }, + })) + eq(false, check_registered({ + workspace = { + didChangeWatchedFiles = { + dynamicRegistration = false, }, }, - }, { client_id = client_id }) - - return watching - ]]) - - eq(false, result) + })) + eq(true, check_registered({ + workspace = { + didChangeWatchedFiles = { + dynamicRegistration = true, + }, + }, + })) end) end) end)