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

Add pullDiagnostics lsp feature #7757

Open
Stanislav-Lapata opened this issue Jul 26, 2023 · 8 comments · May be fixed by #11315
Open

Add pullDiagnostics lsp feature #7757

Stanislav-Lapata opened this issue Jul 26, 2023 · 8 comments · May be fixed by #11315
Labels
A-language-server Area: Language server client C-enhancement Category: Improvements E-medium Call for participation: Experience needed to fix: Medium / intermediate

Comments

@Stanislav-Lapata
Copy link

Some of LSP support only pull diagnostics
source here #7725

@Stanislav-Lapata Stanislav-Lapata added the C-enhancement Category: Improvements label Jul 26, 2023
@Stanislav-Lapata Stanislav-Lapata changed the title Add pullDiagnostics Add pullDiagnostics lsp feature Jul 26, 2023
@pascalkuthe pascalkuthe added E-medium Call for participation: Experience needed to fix: Medium / intermediate A-language-support Area: Support for programming/text languages labels Jul 26, 2023
@the-mikedavis the-mikedavis added A-language-server Area: Language server client and removed A-language-support Area: Support for programming/text languages labels Jul 26, 2023
@woojiq
Copy link
Contributor

woojiq commented Aug 5, 2023

We need to wait until gluon-lang/lsp-types#258 is merged. Right now library has no primitives to implement textDocument/diagnostic.

@pascalkuthe
Copy link
Member

The new lsp types version contains the necessary PR so this is not blocked anymore. It might be a bit of work to implement tougnh

@woojiq
Copy link
Contributor

woojiq commented Aug 8, 2023

Are there any other language servers that you know of that support pull diagnostics? I've tried omnisharp, pyright, RA, zls, and a few others, but none have implemented this feature. Some of them will probably support this later because they also depend on lsp-types as nil. So it will even be hard to test decently with different ls 😄

@pascalkuthe
Copy link
Member

pascalkuthe commented Aug 8, 2023

This issue was motivated by ruby-lsp. It's a fairly new feature tough so I am not surprised it's not widely supported yet

@woojiq
Copy link
Contributor

woojiq commented Aug 9, 2023

I played around with the implementation a bit and here are some notes.
It's actually very easy to implement if you make this request on a key press (like hover or goto-definition). This is because all such commands are executed with a commands::Context passed in, so we can use cx.callback, which gives as mut Editor in the closure, and we can extract documents and other stuff:

cx.callback(
future,
move |editor, compositor, response: Option<lsp::Hover>| {

This is how all commands interact with lsp (via future + callback). On the other hand, the current diagnosis (publish) comes to us in the form of notifications, so we no longer need to interact with async, just change the document and listen for additional notifications. So my question is how can we simultaneously process an asynchronous request and then make changes to the document doc.replace_diagnostics(). For example, we can't paste code here:

if let Some(notification) =
language_server.text_document_did_save(identifier.clone(), &text)
{
notification.await?;
}

because mut Doc cannot be passed between threads. By code I mean lsp call + await + handle the returned data. Do we need to refactor the code and implement a callback? I hope you understand what I mean. 🤞😃

P.S. if you also want to try to implement this feature, ruby-lsp works with pullDiagnostic, but you also need to download robocup and look carefully at the docs. This lsp works very limited compared to RA.

@pascalkuthe
Copy link
Member

We have mechanismis for exactly this: You need to create a job. This gets run in the main loop mutable access to the editor. This should likely run on idle timeout (and need to use the event system in the future)

@SofusA
Copy link

SofusA commented Jul 18, 2024

What would be required now that #8021 is merged?

I could give this a shot, if it is not too difficult.

I would like to try out Roslyn Language Server (successor to OmniSharp) for C# and it only supports pull diagnostics.

@woojiq
Copy link
Contributor

woojiq commented Jul 18, 2024

I could give this a shot, if it is not too difficult.

I think that would be not very difficult, because all building blocks are probably in the code base already. But you need to carefully read and understand lsp specification to support ?all? capabilities, etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-language-server Area: Language server client C-enhancement Category: Improvements E-medium Call for participation: Experience needed to fix: Medium / intermediate
Projects
None yet
5 participants