From e55e80d51ca5d85770981bffb9254badc3662e0c Mon Sep 17 00:00:00 2001 From: Chris AtLee Date: Tue, 1 Aug 2023 08:13:52 -0400 Subject: [PATCH] fix(lsp): inlay hints: "Failed to delete autocmd" when closing buffer #24469 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Problem: "Failed to delete autocmd" error when deleting LspNotify autocmd. #24456 Solution: Change a few things in the inlay_hint and diagnostic LSP code: 1. Re-introduce the `enabled` flag for the buffer state tables. Previously I was relying on the presence of an autocmd id in the state table to track whether inlay_hint / diagnostic was enabled for a buffer. There are two reasons why this doesn't work well: - Each time inlay_hint / diagnostic is enabled, we call `nvim_buf_attach` on the buffer, resulting in multiple `on_reload` or `on_detach` callbacks being registered. - Commands like `bwipeout` delete buffer local autocmds, sometimes before our `on_detach` callbacks have a chance to delete them first. This causes the - Use module local enabled state for diagnostic as well. bwipeout can race with on_detach callbacks for deleting autocmds. Error referenced in #24456. 2. Change the `LspDetach` autocmd to run each time (i.e., remove the `once` flag). Since we're only registering autocmds once per buffer now, we need to make sure that we set the enabled flag properly each time the LSP client detaches from the buffer. - Remove `once` from the LspDetach autocmds for inlay_hint and diagnostic. We only set up the autocmd once now. Gets removed when buffer is deleted. 3. Have the `LspNotify` handler also refresh the inlay_hint / diagnostics when receiving the `textDocument/didOpen` event. Before this point, the LSP backend doesn't have the contents of the buffer, so can't provide inlay hints or diagnostics. Downsides of this approach: * When inlay_hint / diagnostics are disabled on a buffer, it will continue to receive `LspNotify` events for that buffer. The callback exits early since the `enabled` flag is false. Alternatives: * Can we wrap the call to `nvim_del_autocmd` in `pcall` to swallow any errors resulting from trying to delete the autocmd? Fixes #24456 Helped-by: Maria José Solano --- runtime/lua/vim/lsp/diagnostic.lua | 85 ++++++++++++++++-------------- runtime/lua/vim/lsp/inlay_hint.lua | 35 +++++++----- 2 files changed, 67 insertions(+), 53 deletions(-) diff --git a/runtime/lua/vim/lsp/diagnostic.lua b/runtime/lua/vim/lsp/diagnostic.lua index 34be13096d..44bb90d985 100644 --- a/runtime/lua/vim/lsp/diagnostic.lua +++ b/runtime/lua/vim/lsp/diagnostic.lua @@ -392,19 +392,18 @@ local function clear(bufnr) end end ---- autocmd ids for LspNotify handlers per buffer ---- @private ---- @type table -local _autocmd_ids = {} +---@class lsp.diagnostic.bufstate +---@field enabled boolean Whether inlay hints are enabled for this buffer +---@type table +local bufstates = {} --- Disable pull diagnostics for a buffer --- @private local function disable(bufnr) - if not _autocmd_ids[bufnr] then - return + local bufstate = bufstates[bufnr] + if bufstate then + bufstate.enabled = false end - api.nvim_del_autocmd(_autocmd_ids[bufnr]) - _autocmd_ids[bufnr] = nil clear(bufnr) end @@ -416,38 +415,46 @@ function M._enable(bufnr) bufnr = api.nvim_get_current_buf() end - if _autocmd_ids[bufnr] then - return + if not bufstates[bufnr] then + bufstates[bufnr] = { enabled = true } + + api.nvim_create_autocmd('LspNotify', { + buffer = bufnr, + callback = function(opts) + if + opts.data.method ~= 'textDocument/didChange' + and opts.data.method ~= 'textDocument/didOpen' + then + return + end + if bufstates[bufnr] and bufstates[bufnr].enabled then + util._refresh('textDocument/diagnostic', { bufnr = bufnr, only_visible = true }) + end + end, + group = augroup, + }) + + api.nvim_buf_attach(bufnr, false, { + on_reload = function() + if bufstates[bufnr] and bufstates[bufnr].enabled then + util._refresh('textDocument/diagnostic', { bufnr = bufnr }) + end + end, + on_detach = function() + disable(bufnr) + end, + }) + + api.nvim_create_autocmd('LspDetach', { + buffer = bufnr, + callback = function() + disable(bufnr) + end, + group = augroup, + }) + else + bufstates[bufnr].enabled = true end - - _autocmd_ids[bufnr] = api.nvim_create_autocmd('LspNotify', { - buffer = bufnr, - callback = function(opts) - if opts.data.method ~= 'textDocument/didChange' then - return - end - util._refresh('textDocument/diagnostic', { bufnr = bufnr, only_visible = true }) - end, - group = augroup, - }) - - api.nvim_buf_attach(bufnr, false, { - on_reload = function() - util._refresh('textDocument/diagnostic', { bufnr = bufnr }) - end, - on_detach = function() - disable(bufnr) - end, - }) - - api.nvim_create_autocmd('LspDetach', { - buffer = bufnr, - callback = function() - disable(bufnr) - end, - once = true, - group = augroup, - }) end return M diff --git a/runtime/lua/vim/lsp/inlay_hint.lua b/runtime/lua/vim/lsp/inlay_hint.lua index 912ce6898e..2bf0fb9cb2 100644 --- a/runtime/lua/vim/lsp/inlay_hint.lua +++ b/runtime/lua/vim/lsp/inlay_hint.lua @@ -3,12 +3,12 @@ local log = require('vim.lsp.log') local api = vim.api local M = {} ----@class lsp._inlay_hint.bufstate +---@class lsp.inlay_hint.bufstate ---@field version integer ---@field client_hint table> client_id -> (lnum -> hints) ---@field applied table Last version of hints applied to this line ----@field autocmd_id integer The autocmd id for the buffer ----@type table +---@field enabled boolean Whether inlay hints are enabled for this buffer +---@type table local bufstates = {} local namespace = api.nvim_create_namespace('vim_lsp_inlayhint') @@ -31,6 +31,9 @@ function M.on_inlayhint(err, result, ctx, _) return end local bufstate = bufstates[bufnr] + if not bufstate or not bufstate.enabled then + return + end if not (bufstate.client_hint and bufstate.version) then bufstate.client_hint = vim.defaulttable() bufstate.version = ctx.version @@ -122,10 +125,9 @@ local function disable(bufnr) bufnr = api.nvim_get_current_buf() end clear(bufnr) - if bufstates[bufnr] and bufstates[bufnr].autocmd_id then - api.nvim_del_autocmd(bufstates[bufnr].autocmd_id) + if bufstates[bufnr] then + bufstates[bufnr] = { enabled = false, applied = {} } end - bufstates[bufnr] = nil end --- Enable inlay hints for a buffer @@ -136,24 +138,27 @@ local function enable(bufnr) end local bufstate = bufstates[bufnr] if not bufstate then - bufstates[bufnr] = { applied = {} } - bufstates[bufnr].autocmd_id = api.nvim_create_autocmd('LspNotify', { + bufstates[bufnr] = { applied = {}, enabled = true } + api.nvim_create_autocmd('LspNotify', { buffer = bufnr, callback = function(opts) - if opts.data.method ~= 'textDocument/didChange' then + if + opts.data.method ~= 'textDocument/didChange' + and opts.data.method ~= 'textDocument/didOpen' + then return end - if bufstates[bufnr] then + if bufstates[bufnr] and bufstates[bufnr].enabled then util._refresh('textDocument/inlayHint', { bufnr = bufnr }) end end, group = augroup, }) util._refresh('textDocument/inlayHint', { bufnr = bufnr }) - api.nvim_buf_attach(bufnr, true, { + api.nvim_buf_attach(bufnr, false, { on_reload = function(_, cb_bufnr) clear(cb_bufnr) - if bufstates[cb_bufnr] then + if bufstates[cb_bufnr] and bufstates[cb_bufnr].enabled then bufstates[cb_bufnr].applied = {} util._refresh('textDocument/inlayHint', { bufnr = cb_bufnr }) end @@ -167,9 +172,11 @@ local function enable(bufnr) callback = function() disable(bufnr) end, - once = true, group = augroup, }) + else + bufstate.enabled = true + util._refresh('textDocument/inlayHint', { bufnr = bufnr }) end end @@ -180,7 +187,7 @@ local function toggle(bufnr) bufnr = api.nvim_get_current_buf() end local bufstate = bufstates[bufnr] - if bufstate then + if bufstate and bufstate.enabled then disable(bufnr) else enable(bufnr)