Skip to content

LSP diagnostics inside nvim-tree may flicker #2954

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
des-b opened this issue Oct 12, 2024 · 12 comments · Fixed by #2980 or #3007
Closed

LSP diagnostics inside nvim-tree may flicker #2954

des-b opened this issue Oct 12, 2024 · 12 comments · Fixed by #2980 or #3007
Labels
bug Something isn't working

Comments

@des-b
Copy link
Collaborator

des-b commented Oct 12, 2024

Description

When diagnostics.enable = true and a filetype-generic LSP (e.g. typos-lsp) is running, one may see flickering of diagnostics in the nvim-tree buffer/window.

On a more complex neovim setup than "Clean room replication" I also observed moderate to high CPU load.

A possible workaround for me was to add a check for vim.bo[bufnr].buflisted in view.lua:491:is_buf_valid. I am new to this stuff and not sure if this would be a valid solution.

Neovim version

NVIM v0.10.2
Build type: RelWithDebInfo
LuaJIT 2.1.1727870382

Operating system and version

Linux 6.11.3

Windows variant

nvim-tree version

master

Clean room replication

vim.g.loaded_netrw = 1
vim.g.loaded_netrwPlugin = 1

vim.cmd([[set runtimepath=$VIMRUNTIME]])
vim.cmd([[set packpath=/tmp/nvt-min/site]])
local package_root = "/tmp/nvt-min/site/pack"
local install_path = package_root .. "/packer/start/packer.nvim"
local function load_plugins()
	require("packer").startup({
		{
			"wbthomason/packer.nvim",
			"nvim-tree/nvim-tree.lua",
			"nvim-tree/nvim-web-devicons",
			-- ADD PLUGINS THAT ARE _NECESSARY_ FOR REPRODUCING THE ISSUE
		},
		config = {
			package_root = package_root,
			compile_path = install_path .. "/plugin/packer_compiled.lua",
			display = { non_interactive = true },
		},
	})
end
if vim.fn.isdirectory(install_path) == 0 then
	print("Installing nvim-tree and dependencies.")
	vim.fn.system({ "git", "clone", "--depth=1", "https://github.com/wbthomason/packer.nvim", install_path })
end
load_plugins()
require("packer").sync()
vim.cmd([[autocmd User PackerComplete ++once echo "Ready!" | lua setup()]])
vim.opt.termguicolors = true
vim.opt.cursorline = true

-- MODIFY NVIM-TREE SETTINGS THAT ARE _NECESSARY_ FOR REPRODUCING THE ISSUE
_G.setup = function()
	require("nvim-tree").setup({
		diagnostics = {
			enable = true,
		},
	})
	require("nvim-tree.api").tree.open()
end

-- UNCOMMENT this block for diagnostics issues, substituting pattern and cmd as appropriate.
-- Requires diagnostics.enable = true in setup.

vim.api.nvim_create_autocmd("FileType", {
	pattern = "*",
	callback = function()
                -- please adjust path to `typos-lsp`
		vim.lsp.start({ cmd = { "/home/des-b/.local/share/nvim/mason/bin/typos-lsp" } })
	end,
})

Steps to reproduce

  1. Put a file with an invalid name: som.txt
  2. Run neovim

Expected behavior

Diagnostic on som.txt shown in nvim-tree should just show ''typos: som should be some"

Actual behavior

The diagnostic is shown but flickers every few ~100ms

@des-b des-b added the bug Something isn't working label Oct 12, 2024
@alex-courtis
Copy link
Member

I've seen a bit of flickering on and off for the diagnostic status with som, ede and others in files.

With the above setup you'll notice that there are multiple language servers running - it starts a new one whech every file is opened.

Open nvim-tree and two other files:

: ; ps aux | grep typos
alex       13574  0.0  0.0 2323388 21600 ?       Ssl  09:41   0:00 typos-lsp
alex       13609  0.0  0.0 2323380 20756 ?       Ssl  09:41   0:00 typos-lsp
alex       13650  0.0  0.0 2323420 21588 ?       Ssl  09:41   0:00 typos-lsp
alex       13703  0.0  0.0   6396  3840 pts/3    S+   09:42   0:00 grep --color=auto typos
: ;

@alex-courtis
Copy link
Member

For other language servers, adding an identifying name is enough to result in the one instance being shared e.g. lua-language-server: wiki: Development: Native neovim Configuration

That doesn't seem to be the case for typos-lsp.

Using the "official" language server manager nvim-lspconfig did work nicely:

local lspconfig = require("lspconfig")
lspconfig.typos_lsp.setup()

You'll need typos-lsp in your $PATH

Please let me know if that works and I'll convert this to a discussion.

@alex-courtis
Copy link
Member

alex-courtis commented Oct 13, 2024

@alex-courtis
Copy link
Member

@des-b
Copy link
Collaborator Author

des-b commented Oct 15, 2024

You are right. With the lspconfig config as you suggested there is no flickering.

I have to check my actual neovim config. (where I already use lspconfig with mason btw. 😅) There I observed the flickering as well. When I find the cause I will post it.

Sorry for the noise and thanks for the rapid response and the effort on nvim-tree!

@alex-courtis
Copy link
Member

Sorry for the noise and thanks for the rapid response and the effort on nvim-tree!

No apologies needed; there are some issues here that we need to get to the bottom of.

@des-b
Copy link
Collaborator Author

des-b commented Oct 19, 2024

After a lot of stripping my configuration down I found the cause and the (already existing 🫣) solution. The underlying issue was "fixed" in neovim/nvim-lspconfig@b2c7317. Before this change a FileType autocmd is used to detect if a buffer should be lsp-attached. (normally lspconfig uses BufReadPost)

With the following config I am able to reproduce the issue. Notice I have re-enabled filetypes. (also notice that this still not always causes flickering; see below the example)

local lazypath = vim.fn.stdpath("data") .. "/lazy/lazy.nvim"
load(vim.fn.system("curl -s https://raw.githubusercontent.com/folke/lazy.nvim/main/bootstrap.lua"))()

require("lazy.minit").setup({
    spec = {
    {
       "neovim/nvim-lspconfig"
    },
    {
         "kyazdani42/nvim-tree.lua",
         opts = {
            diagnostics = {
            enable = true,
         },
      },
    },
   },
})

require("lspconfig").typos_lsp.setup({
   cmd = { "PATH-TO-typos-lsp" },
   filetypes = { "*" }, -- or 'NvimTree' if you like
})

vim.keymap.set("n", "<F2>", function()
 require("nvim-tree.api").tree.toggle()
end)

With the configuration above lspconfig attaches the buffer if both criteria are met:

  • the filetype matches
  • buftype is not nofile (checked in lspconfig/manager.lua:try_add())

Now nvim-tree.lua sets BUFFER_OPTIONS (create_buffer()) in non-deterministic order. I played with this a little bit.

  • buftype = "nofile" before filetype = "NvimTree": no flickering
  • buftype = "nofile" after filetype = "NvimTree": always flickering

So with the second order the behavior is reproducible since typos-lsp is always attached on nvim-tree.

Flickering itself is caused by lsp diagnostics I guess. When typos-lsp produces diagnostics in nvim-tree for invalid file names this will be detected as a buffer change which in turn will be checked by typos-lsp and this cycles forever.

With this monkey-patched version of nvim-tree's is_buf_valid I can avoid flickering.

local view = require("nvim-tree.view")
local is_buf_valid_orig = view.is_buf_valid
---@diagnostic disable-next-line: duplicate-set-field
view.is_buf_valid = function(bufnr)
 return is_buf_valid_orig(bufnr) and vim.bo[bufnr].buflisted
end

I would like to be able to use typos-lsp on nvim-tree.lua to detect typos in my file names. But I could also see that this would be fragile and might cause trouble in the future. (by setting buftype after filetype in nvim-tree buffer set-up I would be able to)

I am interested in your opinions on how to continue. At least it would make sense to ensure BUFFER_OPTIONS are set in a deterministic order. I could imagine there are other configs/plugins which would be troubled by this non-determinism.

@alex-courtis
Copy link
Member

alex-courtis commented Oct 20, 2024

Now nvim-tree.lua sets BUFFER_OPTIONS (create_buffer()) in non-deterministic order. I played with this a little bit.

Wow, great find! We should do this regardless.

Would this deterministic order resolve all the above issues?

An array something like this. Guessed at a sensible order.

---@type { name: string, value: any }[]
local BUFFER_OPTIONS = {
  { name = "buftype",    value = "nofile" },
  { name = "filetype",   value = "NvimTree" },
  { name = "buflisted",  value = false },
  { name = "bufhidden",  value = "wipe" },
  { name = "modifiable", value = false },
  { name = "swapfile",   value = false },
}
---
  bufnr = M.get_bufnr()
  for _, option in ipairs(BUFFER_OPTIONS) do
    vim.api.nvim_set_option_value(option.name, option.value, { buf = bufnr })
  end

@alex-courtis
Copy link
Member

With this monkey-patched version of nvim-tree's is_buf_valid I can avoid flickering.

local view = require("nvim-tree.view")
local is_buf_valid_orig = view.is_buf_valid
---@diagnostic disable-next-line: duplicate-set-field
view.is_buf_valid = function(bufnr)
 return is_buf_valid_orig(bufnr) and vim.bo[bufnr].buflisted
end

That is reasonable; we can safely ignore unlisted buffers.

view.is_buf_valid doesn't make much sense: the name is misleading and it's only used in one place. Let's inline it into diagnostics so it is very clear what's happening.

@alex-courtis
Copy link
Member

I'd be most grateful for a PR with both these changes; you're the best one to validate the solution ;)

@alex-courtis alex-courtis reopened this Oct 20, 2024
des-b added a commit to des-b/nvim-tree.lua that referenced this issue Nov 1, 2024
This ensures related autocmd's (e.g. on FileType) will be called in a
similar environment.
des-b added a commit to des-b/nvim-tree.lua that referenced this issue Nov 1, 2024
…'buflisted'

is_buf_valid has been inlined since it is only used for diagnostics
and its name is misleading.
alex-courtis pushed a commit that referenced this issue Nov 2, 2024
… buffer options in deterministic order (#2980)

* fix(#2954): set buffer options in deterministic order

This ensures related autocmd's (e.g. on FileType) will be called in a
similar environment.

* fix(#2954): redraw only for diagnostics if source buffer is 'buflisted'

is_buf_valid has been inlined since it is only used for diagnostics
and its name is misleading.
@alex-courtis alex-courtis reopened this Nov 16, 2024
@alex-courtis
Copy link
Member

#2990 temporarily reverted this change.

@alex-courtis
Copy link
Member

alex-courtis commented Nov 17, 2024

Experimenting with DiagnosticsChanged event:

  • on start, one event is sent per buffer
    • buffer will be loaded by lsp, hidden
  • on new or changed diagnostic, one event is sent per buffer
    • there will likely be many, as the user types
  • on resolution of a diagnostic, one event is sent for the buffer with data.diagnostics = nil
  • on disabling of lsp, empty events are sent for each buffer, clearing

Event data.bufnr is the one and only buffer changed.
data.diagnostics contains the same list as from_nvim_lsp vim.diagnostic.get, however it is only for the one bufnr.

It appears that we can use the events themselves to determine the state for each node, rather than enumerating. We can detect a delta, and do nothing if there is no delta.

Simplest Approach

When an event is received, set diag_status on the node, for the maximum severity. Note whether a change occurred.
On a change, a debounced update will be scheduled, however that will only be a render, top show the new diag_status.
Events are synchronous and very unlikely to become asynchronous as a lot of plugins would break.

Performant Alternative

Node lookup by path is expensive.
Maintain a cache of buffer names to severities and map them to the node diag_status only when a change occurs.

alex-courtis added a commit that referenced this issue Nov 21, 2024
…_delay from 50ms to 500ms (#3007)

* fix(#2954): use LSP diagnostic data deltas from events instead of a full query

* fix(#2954): use LSP diagnostic data deltas from events instead of a full query
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug Something isn't working
Projects
None yet
2 participants