feat(lsp): implement textDocument/diagnostic

[catlee]

I'm trying to implement the textDocument/diagnostic capability introduced in LSP 3.17.0 (#22838)

The client initiates a request for diagnostics to the server, instead of having the server push diagnostics to the client.

I took a lot of the code from the inlayHints implementation since that also seemed to poll the server in similar way.

Current status:

• pull diagnostics are enabled when servers advertise the diagnosticProvider capability

TODO:

• Tests
• Refactor common code (e.g. timer related stuff) between inlay_hints and diagnostic
• Docs
• Should it be enabled by default? Figure out how to get it enabled when ruby-lsp attaches. Does this go into lspconfig?
• Not sure if the clear function is doing the right thing. I couldn't figure out how to get a client id.
• File issues for follow-up:
  • Support dynamic registration, and using identifier from [DiagnosticRegistrationOptions](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#diagnosticRegistrationOptions) for namespace determination - LSP diagnostics: support dynamic registration #24412
  • Switch inlay_hint to use LspNotify - refactor(lsp): use LspNotify for inlay_hint #24411
  • Expose namespaces under lsp.Client as per this comment below.

[catlee]

I wonder if the requests for diagnostics is better done within the changetracking functionality in lua.lsp?

[justinmk]

Refactor common code (e.g. timer related stuff) between inlay_hints and diagnostic

I would consider this a blocker. We can't keep duplicating non-trivial caching/debounce/timer logic.

[mfussenegger]

Refactor common code (e.g. timer related stuff) between inlay_hints and diagnostic

I would consider this a blocker. We can't keep duplicating non-trivial caching/debounce/timer logic.

I was thinking that we could trigger a autocmd when the client sends a didChange to the server. That's already debounced and is the moment when the server gets updated about the document state.
Ideally we'd be able to re-use LspRequest, and have pattern = lsp-method. But I think bufnr and pattern can't be specified at the same time?

[justinmk]

Ideally we'd be able to re-use LspRequest, and have pattern = lsp-method. But I think bufnr and pattern can't be specified at the same time?

Could add a v:event field.

[catlee]

Does this approach of using LspDidChange look good? If so, I can tackle porting inlay_hint.lua over as well.

[catlee]

Ideally we'd be able to re-use LspRequest, and have pattern = lsp-method. But I think bufnr and pattern can't be specified at the same time?

Could add a v:event field.

Is there a way to set something like v:event.scope from lua?

[justinmk]

Does this approach of using LspDidChange look good?

Not really. Why can't LspRequest be re-used?

Is there a way to set something like v:event.scope from lua?

Not currently. But we already are attaching stuff in the data field:

neovim/runtime/lua/vim/lsp.lua

Line 1506 in 8a788e2

data = { client_id = client_id, request_id = request_id, request = request },

, so we could add another data field instead of bothering with v:event.

[catlee]

Does this approach of using LspDidChange look good?

Not really. Why can't LspRequest be re-used?

Is there a way to set something like v:event.scope from lua?

Not currently. But we already are attaching stuff in the data field:

neovim/runtime/lua/vim/lsp.lua

Line 1506 in 8a788e2

data = { client_id = client_id, request_id = request_id, request = request },

, so we could add another data field instead of bothering with v:event.

Would that mean that any autocmds created for LspRequest would have to manually filter out invocations based on e.g. data.method? They would be triggered for all completed LSP requests on the buffer, and then need to ignore any except for ones with data.method == 'didChange'. Is that ok?

[justinmk]

any autocmds created for LspRequest would have to manually filter out invocations based on e.g. data.method?

That seems fine to me. LspRequest after all is a very generic (noisy/expensive) event.

Ideally the event system would allow subscribers to opt-in more granularly, but that's a bigger topic.

[mfussenegger]

Is there a way to set something like v:event.scope from lua?

Not currently. But we already are attaching stuff in the data field:

+1 for adding the method to the data field

Would that mean that any autocmds created for LspRequest would have to manually filter out invocations based on e.g. data.method?

Also keep in mind that LspRequest is triggered multiple times for the same request (for state changes from pending to either canceled or done)

If so, I can tackle porting inlay_hint.lua over as well.

I think that can also happen in a separate PR to keep the scope of this one smaller. Helps with review and getting it merged faster.

[catlee]

should we sort these entries alphabetically?

[mfussenegger]

Looks like this breaks inlay hints, (even) if not enabled:

Error executing vim.schedule lua callback: .../dev/neovim/neovim/runtime/lua/vim/lsp/inlay_hint.lua:49:
 attempt to index local 'bufstate' (a nil value)
stack traceback:
        .../dev/neovim/neovim/runtime/lua/vim/lsp/inlay_hint.lua:49: in function 'handler'
        /dev/neovim/neovim/runtime/lua/vim/lsp.lua:1518: in function ''
        vim/_editor.lua: in function <vim/_editor.lua:0>
Error executing vim.schedule lua callback: .../dev/neovim/neovim/runtime/lua/vim/lsp/inlay_hint.lua:49:
 attempt to index local 'bufstate' (a nil value)
stack traceback:
        .../dev/neovim/neovim/runtime/lua/vim/lsp/inlay_hint.lua:49: in function 'handler'
        /dev/neovim/neovim/runtime/lua/vim/lsp.lua:1518: in function ''
        vim/_editor.lua: in function <vim/_editor.lua:0>

[catlee]

Looks like this breaks inlay hints, (even) if not enabled

Ok, I think I fixed that up, and tested it with rust-analyzer.

The problem was that on_refresh was always calling into the new util._refresh function, whether or not the buffer had inlayHints enabled or not.

[catlee]

Is there anything left to do before we can merge this in?

[justinmk]

If result is falsy (i.e. rpc.notify failed?) is that reflected in the logs somehow? If not, should it be?

[mfussenegger]

The result is false if the client stopped. In that case no notification is sent.

Other failures would point to client bugs and probably bubble up, or if the payload is wrong, hopefully get handled in a decent way on the server side - since there are no responses to notifications the client probably won't receive an error

[justinmk]

maybe client_active is a better name for this variable

Suggested change

	if result then
	if client_active then

[catlee]

I applied that suggestion in the PR here: #24411

[catlee]

echasnovski/mini.nvim#429

Lately keep getting this kind issues.

This should be fixed by #24469