Support textDocument/diagnostic specification (Pull diagnostics)

[SofusA]

Closes #7757, if workspace diagnostics is not a requirement. I have not yet found a language server which supports this capability.

Handling of response was originally done by @woojiq in #7900. I was not able to include their git history since their branch has been deleted.

Tested with language servers:

• eslint version >4.10.0
• csharp-language-sever version >4.12.0
• ruby-lsp
• rust-analyzer version >2025-03-03

Diagnostics are pulled when a document is changed for each language server with pull diagnostics feature with an async hook after a debounce period.

Diagnostics are also pulled when a document is opened, when changed to buffer and when a language server is successfully initiated. This is done without debounce.

[SofusA]

Thanks again for the reviews! I added your comments and did some refactoring. I closed the conversations, i am sorry if it is usually the reviewer who does this.

The request is currently only being send on document changes, which makes a terrible user experience :smile:

Is there a way to send this request when a document is opened, or should a workspace diagnostic event be send when the language server (which supports this feature) is started?

Edit: By the way, i am using this branch daily for C# development, since it allows me to use the same language server as the C# vscode extension. So i am keeping the branch up to date with master branch. Let me know if the merge commits is making too much noise.

[SofusA]

I did some experiments with this on roslyn language server. It might be that this language server just behaves weird (which it does on other stuff), but sending workspace/diagnostic after the server is initiated always resulted in and empty diagnostic list. Also with some seconds delay.

I also noticed that responses can either be a full or an related unchanged document report. At the moment, since we only listening on changes, we only get the full reports. This means that when i eg. rename something, i will not see diagnostics on related documents until i make a change.

I am not sure if the specification suggest to pull diagnostics every time a document is viewed, or if we should pull diagnostics for all open documents on changes to a document.

Edit: Pulling diagnostics for all open documents seems to work fine actually.

[SofusA]

My plan is to send pull diagnostics request:

1. When current document is edited for all open documents, if possible where other documents has same active language server by id as edited document
2. When a new document is opened

[SofusA]

Edit: This is no outdated. I now pull diagnostics when a document has changed for that document for each language server with pull diagnostics feature. This is done with a async hook with a debounce.

I also pull blocking when a document is opened or changed to buffer.

I chose this model because i thought pulling for all open documents on edit was too aggressive.

I have now added two events: *Pull diagnostics for documents* Trigger: When documents are opened Document ids are collected in a hashset After a short debounce all documents pull from all its language servers with pull diagnostics support.

Pull diagnostics for language servers Trigger: When document are saved Language server ids with pull diagnostics support from saved document are collected in a hashset. After a longer debounce all language servers for all open document are pulled

Document ids are being passed around, and i do some cloning of ids.

Overall it appears to work well. At least better than the first implementation with idle timeout.

[lizclipse]

I've been trying this PR to see if it'll solve some issues using the ESLint LS from vscode-langservers-extracted, and I've run into a couple of bugs. Firstly, if you open a file directly using the CLI args, then the diagnostics won't show, and instead you have to close the buffer and re-open via the file picker. Secondly, and most noticable I think, if I then close this buffer and try to re-open it again then I get a crash:

thread 'main' panicked at helix-term/src/handlers/diagnostics.rs:144:19:
no entry found for key
stack backtrace:
   0: rust_begin_unwind
             at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/std/src/panicking.rs:597:5
   1: core::panicking::panic_fmt
             at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/core/src/panicking.rs:72:14
   2: core::panicking::panic_display
             at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/core/src/panicking.rs:168:5
   3: core::panicking::panic_str
             at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/core/src/panicking.rs:152:5
   4: core::option::expect_failed
             at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/core/src/option.rs:1988:5
   5: helix_term::handlers::diagnostics::pull_diagnostics_for_documents
   6: helix_term::job::Jobs::handle_callback
   7: helix_term::application::Application::run::{{closure}}
   8: tokio::runtime::park::CachedParkThread::block_on
   9: tokio::runtime::runtime::Runtime::block_on
  10: hx::main
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

Of note, you do have to be using vscode-langservers-extracted@>=4.9.0 (I'm testing on 4.10.0), as that's when they changed something to stop the LS working originally, and is why I'm trying this.

[SofusA]

Thanks for testing!

Firstly, if you open a file directly using the CLI args, then the diagnostics won't show, and instead you have to close the buffer and re-open via the file picker.

I see the same behaviour with roslyn language server. I think this is because we are requesting diagnostics before the language server is ready. I was hoping that this would be the language servers responsibility. I am not sure what to do about this. Diagnostics will be requested if you make a change to the document

Secondly, and most noticable I think, if I then close this buffer and try to re-open it again then I get a crash

I fixed this by skipping the diagnostics request if the document id is not in the editor. I am not sure if this is the best solution.

Edit:

Of note, you do have to be using vscode-langservers-extracted@>=4.9.0 (I'm testing on 4.10.0), as that's when they changed something to stop the LS working originally, and is why I'm trying this.

I am actually (without knowing) been using vscode-eslint-language-server version 4.10.0 on this branch. It is working for me with pull diagnostics. @lizclipse Can you check if it work with my latest changes?

[lizclipse]

It looks like the crash is fixed now, thank you. I'd be curious on why the diagnostic request is trying to happen on a missing document ID (is there a memory leak somewhere?), but a little defensive programming didn't hurt anyone.

I've been testing this PR against v4.10.0 and so far it's been working (unlike the master branch, since eslint needs this now I think), although I am still getting the issue where it doesn't pick up the first open file. With my testing just now, it seems that it also doesn't pick up if you open helix blank and then open a file via the picker, you then have to re-open the file (or make an edit) before it starts showing anything. IMO I think functionally it's fine for this PR, but it may be worth opening an issue afterwards as a follow-up to at least document that this is known dodgey behaviour.

[SofusA]

I looked into it, and i am not sending the pull diagnostics request, because of a race between the event, and the launching the language server (and registering capabilities). The event wins :smile:
In sequent events (open or edit) the language server will be initiated and capabilities are registered.

I have not found a good solution for this, which i find very annoying. I have found a somewhat hacky solution, where i dispatch the DocumentDidOpen again after a language sever is initialized.

It looks like the crash is fixed now, thank you. I'd be curious on why the diagnostic request is trying to happen on a missing document ID (is there a memory leak somewhere?), but a little defensive programming didn't hurt anyone.

I am not completely sure why. I think part of it was because i was doing this asynchronous, where i collected all document_ids in a hashset, and then send all notifications after a debounce. I changed this to a synchronous hook instead.

[SofusA]

I did some experiments with diagnostics refresh, but it does not look like the language servers that I use send this notification. I was hoping this would be sent when the language server is ready to receive pull diagnostic requests.

[SofusA]

I started to implement workspace diagnostics, but the language servers that i use don't responds to this request. Eslint returns an error and roslyn returns an empty array. So not much to work with here.

I also think that this will be difficult to get to work in helix, because we would need the diagnostics picker to await the async request before displaying, or we would need a way to refresh diagnostics in the picker when the request has finished. So i am leaving this feature, at least unless someone can guide me in the right direction.

I got refresh diagnostics working though

[ssiltanen]

eslint version 4.10.0 Eslint returns an error

Have you tried building a newer version of Microsoft/vscode-eslint? Since you referred to version 4.10.0, I imagine you are using the build from hrsh7th/vscode-langservers-extracted, which took a non-release commit from main at its build time.

[SofusA]

eslint version 4.10.0 Eslint returns an error

Have you tried building a newer version of Microsoft/vscode-eslint? Since you referred to version 4.10.0, I imagine you are using the build from hrsh7th/vscode-langservers-extracted, which took a non-release commit from main at its build time.

No i have only tried vscode-langservers-extracted from nixpkgs.

But this would require some refactoring of the diagnostics picker, and i think it should be in a separate pull request

[the-mikedavis]

See the symbol picker for an example of a picker that waits on an LSP request. The workspace symbol picker is maybe also a good example but it's a sort of "dynamic picker" that re-requests symbols as you type with follow-up requests to the language server.

[SofusA]

See the symbol picker for an example of a picker that waits on an LSP request. The workspace symbol picker is maybe also a good example but it's a sort of "dynamic picker" that re-requests symbols as you type with follow-up requests to the language server.

Okay it does not look that difficult. I have however not been able to get response from roslyn language server. I will try again with a newer version of eslint language server next week.

Edit: Okay i gave it a very low effort shot using this fork which has a nix flake i could use.

I looked at the capability responses from the 3 language servers i know of that supports pull diagnostics: ruby-lsp, vscode-eslint´ and Microsoft.CodeAnalysis.LanguageServer(akaroslyn-ls`. Unfortunately only the roslyn language server supports this at the time of writing, and it does not give me a reliable response.

A workaround could be to request diagnostics for all open buffers, for all language servers which support pull diagnostics, before opening workspace diagnostics. Edit again: I did give this a try and i am not sure if i will be able to get it working. There will be a double for looping which will gives me issues. That would be the case for workspace diagnostics since it is just one request to the server. A better rust developer might be able to pull it off :smile:

I suggest to wait for language servers to support this capability, and maybe handle pulling diagnostics for all documents for workspace diagnostics picker in a separate pull request.

[the-mikedavis]

I ported out the change to move the diagnostic handling from Application to Editor to master in https://github.com/helix-editor/helix/commit/62625eda460c7c5e7cbd76db92d2d141aa2f9650 and rebased this.

I dropped a change though in https://github.com/SofusA/helix-pull-diagnostics/blob/3304a54ed45b42e5b3819fb8dd165386e854b3a7/helix-view/src/editor.rs#L2236-L2242: that was discarding diagnostics that came in without an open document. For some language servers that's ok since they only ever emit diagnostics for the opened documents but others like rust-analyzer can send documents for the whole workspace. For example if you change a function's name in one file, rust-analyzer should send diagnostics for all other files in the workspace that need to update the name.

[SofusA]

I ported out the change to move the diagnostic handling from Application to Editor to master in 62625ed and rebased this.

I dropped a change though in https://github.com/SofusA/helix-pull-diagnostics/blob/3304a54ed45b42e5b3819fb8dd165386e854b3a7/helix-view/src/editor.rs#L2236-L2242: that was discarding diagnostics that came in without an open document. For some language servers that's ok since they only ever emit diagnostics for the opened documents but others like rust-analyzer can send documents for the whole workspace. For example if you change a function's name in one file, rust-analyzer should send diagnostics for all other files in the workspace that need to update the name.

Awesome. Thanks!

Is there anything I could do to help complete this pull request? I am a little lost in the process.

[the-mikedavis]

For now no - I need to go throgh the spec and sit with the changes for a while and then I'll leave some review. I'd also like @pascalkuthe's review on the hook/event stuff but he's quite busy with work at the moment so in the meantime I'd like to minimize the changes with master so it's easy to rebase.

[SofusA]

For now no - I need to go throgh the spec and sit with the changes for a while and then I'll leave some review. I'd also like @pascalkuthe's review on the hook/event stuff but he's quite busy with work at the moment so in the meantime I'd like to minimize the changes with master so it's easy to rebase.

Great :+1:

I think I have found an issue.

Diagnostics comes in one by one in an annoying way when language server responds slower than denounce value. See attached video. I can only reproduce in bigger dotnet codebase with StyleCop and Sonar diagnostics providers with the C# language server.

It don't think it is possible to cancel a pull diagnostic request, so maybe this just indicates that the debounce should be higher. Maybe 250 ms.

Screencast From 2025-02-10 15-27-27.webm

Do you want me to keep rebaseing and tuning the debounce value on this branch?

[the-mikedavis]

Yeah feel free to push/rebase/force-push this branch as you see fit - I only force-pushed to resolve the conflicts and keep it to one commit as you had been doing

[SofusA]

I noticed, that latest rust-analyzer does provide pull diagnostic capabilities. But only some diagnostics are included in pull diagnostic responses. This partial diagnostic is then replacing all the diagnostics from the publish diagnostics.

I am not sure what to do here, and it should be handled before merge. Any ideas? My current best solution would be to store pull and push diagnostics separately, and render a joined list of diagnostics.

[SofusA]

I managed to solve this by changing DiagnosticProvider to an enum. Coincidentally, there was already a todo for this. I use this enum instead of LanguageServerId when something is related to diagnostics from a server.

Now, a language server can provide both types of diagnostics. Both rust-analyzer and the C# language server work :tada:

[SofusA]

It don't think it is possible to cancel a pull diagnostic request, so maybe this just indicates that the debounce should be higher. Maybe 250 ms.

I solved this issue by pull diagnostics on the first change, and then increase the debounce to 500 ms. This both fixes the issue and the first diagnostic is now rendered 120 ms faster.

The debounce could easily be lower, but the perceived responsiveness is fine at 500.

[the-mikedavis]

There's some extra context in the LSP issue tracker about whether you can support push and pull diagnostics simultaneously https://github.com/microsoft/language-server-protocol/issues/1743. ~It sounds like rust-analyzer's behavior currently is a bug.~ It's also mentioned in comments after the pull diagnostic PR was merged: https://github.com/rust-lang/rust-analyzer/pull/18404#issuecomment-2525047165

(Continuing in https://github.com/rust-lang/rust-analyzer/issues/18709)

[SofusA]

I have tried to address comments in separate commits. Some changes has made this pull request large. I am sorry about that :smile:

let me know if it needs some refactoring

[the-mikedavis]

I ported out the change to refactor the diagnostic provider as an enum and the change to add DocumentDidOpen (as well as some others) and gave this branch a rebase: https://github.com/helix-editor/helix/compare/2d4c2a170cab5f625139b829b2d3f00b303a8550...6da1a79d80d90d13cb4821362a52fa0d6b9bdee6

Those changes will be necessary for some other patches like #11660 and DocumentDidOpen I believe was already needed by a few open PRs.

W.r.t. rust-analyzer, one option to limit the impact of the broken versions is to turn off pull diagnostics for a while by default:

[[languages]]
name = "rust"
# ...
language-servers = [{ name = "rust-analyzer", except-features = ["pull-diagnostics"] }]
# ...

Then once 2025-03-03 gets older we can eventually go back to using them.

[sandiks]

Hi, is it possible integrate this lang server with zed editor?

[SofusA]

Hi, is it possible integrate this lang server with zed editor?

This is a pull request for implementing a protocol in Helix editor.

If you are referring to the csharp language server that I use (and is depending on this or for helix support), please add an issue there instead 😊

[SofusA]

@pascalkuthe Thanks for the review. It sounds like this needs some more features:

1. Ability to cancel ongoing requests
2. Implementation of workspace diagnostics
3. Tracking of dispatched revisions

While i do agree that this would be great features to have, i am personally only invested in basic pull diagnostic support. I absolutely understand why this should be implemented, i just do not have the time (or skills :smile: ).

I have been updating this branch for quite some time and will continue to do so, but i don't think i will be able to implement these features in the near future.

[the-mikedavis]

Pascal and I can take the branch forward as we find time to work on it. I have some changes locally I was working on to track the ongoing requests anyways. We wouldn't mind getting something relatively simple in as a first step but there are some changes we'll want to make to ensure we don't spam the language server or allow duplicate in-flight requests

[SofusA]

Pascal and I can take the branch forward as we find time to work on it. I have some changes locally I was working on to track the ongoing requests anyways. We wouldn't mind getting something relatively simple in as a first step but there are some changes we'll want to make to ensure we don't spam the language server or allow duplicate in-flight requests

Thanks. I appreciate your work 😃

I did play around with the cancel request. The result is great. I was able to reduce the debounce to 125 ms without issues, but I am not sure if the code holding ongoing requests is ideal.

edit: Fat fingered the close with comment button.