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

vitallium · 2024-10-15T16:12:16Z

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:

The existing functionality works as before
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

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

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:

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

CleanShot.2024-10-13.at.13.27.22.mp4

vitallium · 2024-10-15T16:13:15Z

crates/language/src/language.rs

@@ -201,6 +201,7 @@ pub struct CachedLspAdapter {
    pub name: LanguageServerName,
    pub disk_based_diagnostic_sources: Vec<String>,
    pub disk_based_diagnostics_progress_token: Option<String>,
+    pub previous_document_diagnostic_result_id: Option<String>,


Still not sure about the place to store this token that Zed should store and use between sending/receiving textDocument/diagnostic commands.

Moved the previous_document_diagnostic_result_id to LanguageServerState enum. Let me know if that's ok.

crates/lsp/src/lsp.rs

crates/project/src/lsp_store.rs

vitallium · 2024-10-15T16:19:56Z

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

vitallium · 2024-10-15T16:21:13Z

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

Done.

zed-industries-bot · 2024-10-15T16:21:34Z

	Messages
📖	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 🚫 dangerJS against 9e79ecd

crates/project/src/lsp_command.rs

vitallium · 2024-10-18T18:16:07Z

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

Hi. While working on #19230 I noticed that some servers send a request to unregistered the `textDocument/rename` capability. I thought it would be good to handle that message in Zed: ```plaintext [2024-10-18T21:25:07+02:00 WARN project::lsp_store] unhandled capability unregistration: Unregistration { id: "biome_rename", method: "textDocument/rename" } ``` So this pull request implements that. Thanks. Release Notes: - N/A

…ndustries#19427) Hi. While working on zed-industries#19230 I noticed that some servers send a request to unregistered the `textDocument/rename` capability. I thought it would be good to handle that message in Zed: ```plaintext [2024-10-18T21:25:07+02:00 WARN project::lsp_store] unhandled capability unregistration: Unregistration { id: "biome_rename", method: "textDocument/rename" } ``` So this pull request implements that. Thanks. Release Notes: - N/A

vitallium · 2024-10-21T17:28:59Z

Resolved merge conflicts.

SomeoneToIgnore

One thing I do not understand is how both diagnostics pull and diagnostics push models are supposed to live together.

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?
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.

crates/lsp/src/lsp.rs

crates/project/src/lsp_command.rs

crates/project/src/lsp_store.rs

vitallium · 2024-10-23T10:40:03Z

@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 · 2024-11-04T19:52:02Z

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

vitallium · 2024-12-10T19:05:41Z

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 · 2024-12-26T09:15:34Z

crates/editor/src/editor.rs

+                    } else if let project::Event::LanguageServerAdded(_, _, _) = event {
+                        editor.tasks_pull_diagnostics_task = Some(editor.refresh_diagnostics(cx));
+                    } else if let project::Event::LanguageServerRemoved(_) = event {
+                        editor.tasks_pull_diagnostics_task = Some(editor.refresh_diagnostics(cx));


Question: Not sure if triggering the task for pulling diagnostics here is the correct approach. My thinking is with the MultiLspQuery we want to pull new diagnostics sostics every time new LSP is added or removed.

vitallium · 2024-12-26T09:23:45Z

@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 · 2024-12-26T19:42:55Z

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

lsp: use task-based approach for pulling diagnostics

Requesting diagnostics for a document can return an empty result meaning there are no new diagnostics for the current document. Returning an empty vector could indicate that. It would be good to avoid that and return `None` if diagnostics haven't changed.

cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label Oct 15, 2024

vitallium commented Oct 15, 2024

View reviewed changes

crates/lsp/src/lsp.rs Show resolved Hide resolved

vitallium commented Oct 15, 2024

View reviewed changes

crates/project/src/lsp_store.rs Outdated Show resolved Hide resolved

vitallium commented Oct 15, 2024

View reviewed changes

crates/project/src/lsp_store.rs Outdated Show resolved Hide resolved

vitallium force-pushed the vs/lsp-pull-diagnostic-support-with-lsp-command branch from 2180cc7 to 04e102e Compare October 15, 2024 16:21

vitallium commented Oct 15, 2024

View reviewed changes

crates/project/src/lsp_command.rs Show resolved Hide resolved

vitallium force-pushed the vs/lsp-pull-diagnostic-support-with-lsp-command branch 2 times, most recently from 5f35960 to 2e2e437 Compare October 15, 2024 21:12

vitallium marked this pull request as ready for review October 15, 2024 21:12

vitallium force-pushed the vs/lsp-pull-diagnostic-support-with-lsp-command branch 3 times, most recently from 0e98542 to 3ab5b2c Compare October 17, 2024 19:26

vitallium commented Oct 18, 2024

View reviewed changes

crates/project/src/lsp_command.rs Outdated Show resolved Hide resolved

vitallium force-pushed the vs/lsp-pull-diagnostic-support-with-lsp-command branch from 5c1e305 to 2d3d38c Compare October 18, 2024 18:40

vitallium mentioned this pull request Oct 18, 2024

lsp: Handle unregistration "textDocument/rename" from a server #19427

Merged

vitallium force-pushed the vs/lsp-pull-diagnostic-support-with-lsp-command branch from 2d3d38c to 4e6c9fd Compare October 21, 2024 17:28

vitallium force-pushed the vs/lsp-pull-diagnostic-support-with-lsp-command branch from 4e6c9fd to b7b3be1 Compare October 23, 2024 04:41

SomeoneToIgnore reviewed Oct 23, 2024

View reviewed changes

SomeoneToIgnore self-assigned this Oct 23, 2024

SomeoneToIgnore marked this pull request as draft October 23, 2024 11:07

vitallium force-pushed the vs/lsp-pull-diagnostic-support-with-lsp-command branch 2 times, most recently from 5b9ee27 to 9de1d98 Compare December 4, 2024 16:52

vitallium force-pushed the vs/lsp-pull-diagnostic-support-with-lsp-command branch from 9de1d98 to 9ddc70c Compare December 7, 2024 20:51

osiewicz mentioned this pull request Dec 12, 2024

Switch diagnosticMode from workspace to openFileOnly when a lot of files #19840

Open

1 task

vitallium force-pushed the vs/lsp-pull-diagnostic-support-with-lsp-command branch 4 times, most recently from e7fe69e to fa673d1 Compare December 15, 2024 10:55

vitallium force-pushed the vs/lsp-pull-diagnostic-support-with-lsp-command branch 4 times, most recently from 93c99eb to 7bfaf2d Compare December 25, 2024 20:22

vitallium commented Dec 26, 2024

View reviewed changes

vitallium requested a review from SomeoneToIgnore December 26, 2024 09:18

vitallium force-pushed the vs/lsp-pull-diagnostic-support-with-lsp-command branch from 7bfaf2d to 26f6751 Compare December 26, 2024 10:25

vitallium added 11 commits December 31, 2024 07:43

lsp: Implement support for the textDocument/diagnostic command

8228d98

lsp: store and use previous result of document diagnostics

345e317

lsp: Remove the version field from GetDocumentDiagnostics

b2f4137

lsp: switch to DiagnosticEntry for pull diagnostics

5c72953

lsp: use task-based approach for pulling diagnostics

3510398

lsp: use task-based approach for pulling diagnostics

lsp: add struct for LSP pull-based diagnostics

77ac0b2

lsp: improve task for pulling document diagnostics

6731c19

lsp: make LspDiagnostics response optional

ae4eb88

lsp: handle related_information and tags

8a7c6d5

lsp: handle pull-based diagnostics model for related documents

9e79ecd

vitallium force-pushed the vs/lsp-pull-diagnostic-support-with-lsp-command branch from 26f6751 to 9e79ecd Compare December 31, 2024 06:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

vitallium commented Oct 15, 2024 •

edited

Loading

vitallium Oct 15, 2024

vitallium Oct 15, 2024

vitallium commented Oct 15, 2024

vitallium commented Oct 15, 2024

zed-industries-bot commented Oct 15, 2024 •

edited

Loading

vitallium commented Oct 18, 2024

vitallium commented Oct 21, 2024

SomeoneToIgnore left a comment

vitallium commented Oct 23, 2024

vitallium commented Nov 4, 2024

vitallium commented Dec 10, 2024

vitallium Dec 26, 2024

vitallium commented Dec 26, 2024 •

edited

Loading

SomeoneToIgnore commented Dec 26, 2024

lsp: Implement support for the textDocument/diagnostic command #19230

Are you sure you want to change the base?

lsp: Implement support for the textDocument/diagnostic command #19230

Conversation

vitallium commented Oct 15, 2024 • edited Loading

Brief

TODO

References

In action

vitallium Oct 15, 2024

Choose a reason for hiding this comment

vitallium Oct 15, 2024

Choose a reason for hiding this comment

vitallium commented Oct 15, 2024

vitallium commented Oct 15, 2024

zed-industries-bot commented Oct 15, 2024 • edited Loading

vitallium commented Oct 18, 2024

vitallium commented Oct 21, 2024

SomeoneToIgnore left a comment

Choose a reason for hiding this comment

vitallium commented Oct 23, 2024

vitallium commented Nov 4, 2024

vitallium commented Dec 10, 2024

vitallium Dec 26, 2024

Choose a reason for hiding this comment

vitallium commented Dec 26, 2024 • edited Loading

SomeoneToIgnore commented Dec 26, 2024

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

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

vitallium commented Oct 15, 2024 •

edited

Loading

zed-industries-bot commented Oct 15, 2024 •

edited

Loading

vitallium commented Dec 26, 2024 •

edited

Loading