lsp: Implement support for the `textDocument/diagnostic` command

[vitallium]

Closes #13107

Release Notes:

• Added support for the LSP textDocument/diagnostic command.

Brief

This is draft PR that implements the LSP textDocument/diagnostic command. The goal is to receive your feedback and establish further steps towards fully implementing this command. I tried to re-use existing method and structures to ensure:

1. The existing functionality works as before
2. There is no interference between the diagnostics sent by a server and the diagnostics requested by a client.

The current implementation is done via a new LSP command GetDocumentDiagnostics that is sent when a buffer is saved and when a buffer is edited. There is a new method called pull_diagnostic that is called for such events. It has debounce to ensure we don't spam a server with commands every time the buffer is edited. Probably, we don't need the debounce when the buffer is saved.

All in all, the goal is basically to get your feedback and ensure I am on the right track. Thanks!

TODO

• [x] Support for related documents diagnostics.
• [ ] Support for re-using previous results. The spec describes that as storing and sending a token between LSP commands.

References

1. https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_pullDiagnostics

In action

You can clone any Ruby repo since the ruby-lsp supports the pull diagnostics only.

Steps to reproduce:

1. Clone this repo https://github.com/vitallium/stimulus-lsp-error-zed
2. Install Ruby (via asdf or `mise).
3. Install Ruby gems via bundle install
4. Install Ruby LSP with gem install ruby-lsp
5. Check out this PR and build Zed
6. Open any file and start editing to see diagnostics in realtime.

https://github.com/user-attachments/assets/0ef6ec41-e4fa-4539-8f2c-6be0d8be4129

[vitallium]

Whoops, conflicts. I will resolve them in a bit.

[vitallium]

Whoops, conflicts. I will resolve them in a bit.

Done.

[zed-industries-bot]

	Messages
:book:	This PR includes links to the following GitHub Issues: #13107 If this PR aims to close an issue, please include a Closes #ISSUE line at the top of the PR body.

Generated by :no_entry_sign: dangerJS against 749a4ea1331aac47502f9d7eeb707fc95459bad7

[vitallium]

Ok, this is ready for the first round of review. Thanks!

[vitallium]

Resolved merge conflicts.

[vitallium]

@SomeoneToIgnore Thanks for this amazing review! This is what I've been waiting for. I will start working on your comments and answering your questions about handling both push and pull models for diagnostics. Do you want me to convert this PR to draft to mark it as non-ready?

[vitallium]

Small update: I am still working on it. I was OOO last week.

[vitallium]

2024-11-09 Update

With the great review from @SomeoneToIgnore, I've managed to utilize the MultiLspQuery approach instead of querying the primary language server.
In its current state, I can see Zed sends textDocument/diagnostics requests, but it does not display them yet.

One major blocker is the RPC implementation, which I am struggling with. There are two different structures for diagnostics: lsp::Diagnostic and Diagnostic. Despite being similar, they are different. The Diagnostic structure has fields like group_id and is_disk_based that do not exist in the lsp::Diagnostic structure, which is used for sending and receiving LSP commands. The RPC implementation requires serializing and deserializing diagnostics with protobuf, and I do not know how to achieve that without adding messages to the protocol for lsp::Diagnostic.

There is a method, update_diagnostics, in the LspStore that fills fields in the Diagnostic structure. I will check if I can use it. Additionally, this PR is missing pieces for the dynamic registration of LSP capabilities for pulling diagnostics. I also suspect there are missing pieces for the RPC stuff. I will have a look into both things too.

TODO list:

• Finish support for the RPC.
• Check dynamic registrations.
• Check how to use the previous_result_id that an LSP server sends back to indicate diagnostics haven't changed since the last request.
• Implement handling of diagnostics for relevant documents. I think the easiest way is to create a new method like handle_relative_documents_diagnostics in the LspStore and call it from the response_from_lsp method in GetDocumentDiagnostics.
• Check the refresh_support thing.

Thanks!

[SomeoneToIgnore]

To respond to the RPC part, it's always so that RPC clients have no access to LSP, it runs on the host machine only (either an RPC owner, or on a headless ssh part).

So, we always pass the LSP data over the wire and derive the additional properties on the client, hence you can (and should) use existing methods that fill those properties. If that's not the case, let's have a look together at some point — ping me when more attention is needed to this PR.

[vitallium]

There have been a lot of changes recently in the related crates. I will double-check all the changes here. Also, I may need to review the implementation approach in this commit

The reason why I went this way is that the LspStore::update_diagnostics function accepts lsp::PublishDiagnosticsParams as one of its parameters. Diagnostics pulled via textDocument/diagnostic are not fully compatible with PublishDiagnosticsParams; for instance, there is no uri: Url field. So, I have had to introduce separate structs for pulled diagnostics.

[vitallium]

@SomeoneToIgnore Hi! Can you please do a smoke check of this PR? Just to guide me if I am moving towards the right direction with the task-based approach and additional protobuf messages. The latter is the major question to me since I have had to do it to properly pass LSP diagnostics between Zed and LSPs. The main difference is the structure of push-based and pull-based diagnostics is different. Thanks for the suggestion about using MultiLspQuery, the implementation looks way better than before. I use Ruby LSP for testing because this is the only server with pull-based diagnostics that I can use currently. I know that rust-analyzer also supports it but I wasn't able to enable them.

For instance, rust-analyzer has its own diagnostics, fast to update, and the cargo check-based ones, which may take seconds to update on large projects. If we start to wait for all diagnostics or nothing, it might get slower — right now it's ok, as r-a pushes slow and fast ones separately. How does this feature work on Zed? How should the LSP server and the client agree what to use? It seems, that the push method is the default, and pull is an opt-in one, that will be used right after we declare this new capability?

AFAIU, it depends on the LSP. I wonder if it makes sense to provide a configuration option in Zed to switch between push and pull-based models?

Is it ok to handle both push and pull at the same time, as we do now? We have to include potential refresh requests that we either have to support (as declared currently in the capabilities) or declare incapable.

I think it is, but I am not sure. In my tests, I used Ruby LSP with pull-based diagnostics and rubocop with push-based diagnostics and I didn't notice problems with mixing two modes. I will double-check that.

Thanks again!

[SomeoneToIgnore]

Thank you for working on this. I'll be able to look at this somewhere after 9th of Jan.

[vitallium]

I see the following error message in the logs:

[2025-01-14T08:53:35+01:00 ERROR project::lsp_store] No LSP snapshots found for buffer with path "app/frontend/javascripts/parse_responses.js"

Not sure what that means exactly but I will have a look.

[SomeoneToIgnore]

Sorry, still crawling towards your PR, which I remember about, but other tasks are always popping.

That error is https://github.com/zed-industries/zed/pull/22934

and not necessarily related to your changes, but it is something good to fix indeed.

[vitallium]

Sorry, still crawling towards your PR, which I remember about, but other tasks are always popping.

No worries! I understand how it goes. There's always a long list of tasks and priorities can shift. I appreciate you keeping my PR in mind. Thanks!

[SomeoneToIgnore]

Just a small note about that [2025-01-14T08:53:35+01:00 ERROR project::lsp_store] No LSP snapshots found for buffer with path "app/frontend/javascripts/parse_responses.js" log message: it turned out to be overly pedantic and triggering for Zig and our settings.json out of the blue, so we're either forward-fixing this or reverting, so seems like nothing broken in the current PR.

[vitallium]

Just a small note about that [2025-01-14T08:53:35+01:00 ERROR project::lsp_store] No LSP snapshots found for buffer with path "app/frontend/javascripts/parse_responses.js" log message: it turned out to be overly pedantic and triggering for Zig and our settings.json out of the blue, so we're either forward-fixing this or reverting, so seems like nothing broken in the current PR.

Aha, good to know. Thank you!

[vitallium]

Sorry for taking so long, now I'm back to business and hopefully able to respond to updates more quickly.

No worries and welcome back! I really appreciate any input you provide here!

Main concerns I have for this PR:

• I want to preserve the existing diagnostics pushes, just in case certain servers are not able to do other things and just in case we mess up this impl, so people can use a fall back. Also, I think I've raised it in previous reviews, we do not want both modes to work simultaneously? I see, nothing is removed for pushing handle code, so that's possible? Given all that, I expected somewhere in the language/language server-related logic, a piece of if-based logic to determine which path are we going with.

My bad, I forgot about that. For some reason, I thought that LSP servers decide which mode to use based on given capabilities, but given how strange LSP servers can sometimes be, having such a switch makes sense. I will add it.

• I think, we do not query enough diagnostics, but maybe I'm wrong? Voiced that thought in the editor.rs comments.

Thank you for the thorough review! I will address your points one by one, answering them and composing a TODO list based on your feedback.

• We still have some new code added for diagnostics [de]serialization, what was the issue with the existing one that handled pushes via LSP?

Hi, both functions operate on the DiagnosticEntry<Anchor> struct, but pull-based diagnostics have the type Diagnostic. The implementation of the LspCommand trait for the GetDocumentDiagnostics type operates with Diagnostic type to have the correct return type. Changing the implementation response type to DiagnosticEntry<T> will lead to additional functions, AFAIU, because response_from_lsp should return a collection of DiagnosticEntry. When diagnostics arrive in GetDocumentDiagnostics, in the report report.full_document_diagnostic_report.items, they have the Diagnostic type, requiring conversion to DiagnosticEntry<T>. This conversion is not straightforward because most fields are assigned in the update_diagnostics function on the LspStore struct. In theory, it is possible to call update_diagnostics from the LspCommand but that will leave the following question open: what should the response_from_lsp return?

I hope I described the situation properly. Let me know if you have any questions. Thanks!

[vitallium]

Passing by with a few responses and noting that there's #23639 trying to do the same thing with a bit different primitives, maybe something interesting can be united.

#23639 looks interesting, thanks for linking it! I probably went into over-engineering mode, and the only things required are in #23639. I am totally fine with giving priority to the linked PR over the somewhat complicated solution in this pull request.