nvim-lua / kickstart.nvim

A launch point for your personal nvim configuration
MIT License
16.93k stars 17.05k forks source link

Fix highlight errors when lsp crash or stop #864

Closed sudo-tee closed 2 months ago

sudo-tee commented 2 months ago

It adds a check wether the client is still available before highlighting.

If the client is not there anymore it returns true to unregister the autocommand

This fix the method textDocument/documentHighlight is not supported by any of the servers registered for the current buffer errors when doing a LspRestart or the server crashes

feoh commented 2 months ago

Please forgive my ignorance here but will this fix negatively affect people with different LSP configurations etc?

dam9000 commented 2 months ago

How do you test or reproduce this error? I tested :LspRestart and :LspStop and :LspStart and have not seen any errors and the behaviour worked as expected. My observed behaviour:

These cases seem to work normally. But I only tested with lua, perhaps some other LSP behaves differently. I don't know what happens in case of LSP crash or how to reproduce it, but I suspect in such a case the highlights will not be the only issue? Perhaps if LSP crashes the best is to just restart nvim, since it will not behave as expected? In any case I think we need to understand more details about this.

sudo-tee commented 2 months ago

I have a pretty unstable LSP (tsserver) on a couple projects. That requires me to frequently restart it. When the server crash I receive a load of errors method textDocument/documentHighlight is not supported by any of the servers registered for the current buffer

Theses errors are probably not an issue by themselves because they are silent, but I'm using Noice and nvim-notify so they are pretty noisy.

The issue is that the more I restart the LSP the more errors notifications I get, tsserver sometimes require 3 restart in a row. They only go away if a use the :e command or close nvim.

This led me to believe that there might be a small memory leak as the more you restart the lsp the more autocmds are created.

This might be an edge case but doing this trick completely resolved my issues and I can restart the LSP and continue working. I thought I'd share through this pull request.

dam9000 commented 2 months ago

Hmm I wonder if a more proper approach would be to listen to the LspDetach event and then call vim.lsp.buf.clear_references and unregister all Cursor* autocmds? Does the LspDetach event get triggered when the LSP crashes? Also, have you submitted a bug report on tsserver crash or perhaps it's already a known issue?

sudo-tee commented 2 months ago

It's is a good idea and it seems to work properly by removing the command in the LspDetatch. As per the crash I have to wait until it happens to see if the error is still there, but with this fix LspRestart will clear the commands the error will automatically be cleared as well. I would say it is not really an issue anymore.

I updated the PR to remove the the commands in the LspDetatch event like you suggested.

dam9000 commented 2 months ago

@sudo-tee It's looking much better now. I think it can be simplified further by using nvim_clear_autocmds like this:

diff --git a/init.lua b/init.lua
index f587aec..50e56d5 100644
--- a/init.lua
+++ b/init.lua
@@ -544,10 +544,7 @@ require('lazy').setup({
         group = vim.api.nvim_create_augroup('kickstart-lsp-detach', { clear = true }),
         callback = function(event)
           vim.lsp.buf.clear_references()
-          local cmds = vim.api.nvim_get_autocmds { group = 'kickstart-lsp-highlight', buffer = event.buf }
-          for _, cmd in ipairs(cmds) do
-            vim.api.nvim_del_autocmd(cmd.id)
-          end
+          vim.api.nvim_clear_autocmds { group = 'kickstart-lsp-highlight', buffer = event.buf }
         end,
       })

If you agree that this works and is cleaner go ahead and integrate it and I think we have a final solution.

sudo-tee commented 2 months ago

@dam9000

I confirm that it works with the vim.api.nvim_clear_autocmds. Thanks a lot

I updated the PR to use this method instead.

Thanks lot for your help on the issue :)

feoh commented 2 months ago

Thanks for working towards the best solution on this everyone!