Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

lsp: Add limited implementation of pull-based diagnostics #23639

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cartercanedy
Copy link

@cartercanedy cartercanedy commented Jan 25, 2025

Limited implementation of the diagnostic pull strategy using textDocument/diagnostic rpc call

Lsp server implementors are starting to support a new diagnostic pull strategy. In the case of the most recent release of rust-analyzer, this improves diagnostic reporting in the active document substantially.

Without commenting on the implementation details of #19230, I agree that using the diagnostic pull strategy is useful, but this implementation significantly reduces the complexity of the changes by utilizing existing code paths that normally handle 'textDocument/publishDiagnostics'.

Some work is still needed around implementing this for downstream clients, but I'm definitely open to feedback/support in the current implementation and further developing the feature set. My implementation is high enough above the collab buffer abstractions that I don't need to implement any new rpc features to enable it


I'm open to this being modified/rejected, but from my experience using this branch locally, it drastically decreases latency in diagnostic reporting when working in the zed codebase, in combination with bumping the rust toolchain channel to 1.84 (necessary since RA devs only implemented the diagnostic pull rpcs 3 months ago)

Closes #23566

Release Notes:

  • N/A

@cla-bot cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label Jan 25, 2025
@cartercanedy
Copy link
Author

@SomeoneToIgnore @osiewicz would love to get some feedback from you guys on this when you get the chance to take a look

@kucho
Copy link

kucho commented Jan 25, 2025

There's another PR trying to implement the same strategy. Perhaps you can collaborate on the implementation? I've been waiting for this functionality for a while!

@maxdeviant maxdeviant changed the title lsp: limited implementation of diagnostic pull strategy lsp: Add limited implementation of pull-based diagnostics Jan 26, 2025
@cartercanedy
Copy link
Author

From what I can tell, all of the code paths here are hit by both local and remote buffer events so I shouldn't have to implement any additional rpc features.

@cartercanedy cartercanedy marked this pull request as ready for review January 26, 2025 21:45
@cartercanedy cartercanedy marked this pull request as draft January 27, 2025 00:40
@cartercanedy
Copy link
Author

it turns out there's some more complicated logic with RA that results in an empty diagnostic report being returned if the textDocument/diagnostic request arrives while any fs change handling is still in-flight. Tbh, I'm honestly questioning if there's ever a situation where pull diagnostics from RA return valid diagnostics.

It seems like I jumped to the conclusion that the improvement in RA diagnostic reporting performance when jumping from 1.81 to 1.84 was related to pull diagnostics, but that was very likely mistaken. I'll try to test the changes with other language servers to see if there's an improvement in other scenarios

@cartercanedy
Copy link
Author

ruby-lsp is confirmed working with this implementation, but it likely would benefit from implementing workspace diagnostic refresh ssr handling

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed The user has signed the Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rust-analyzer: diagnostics can take a long time to be updated
2 participants