-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Support textDocument/diagnostic specification (Pull diagnostics) #11315
base: master
Are you sure you want to change the base?
Conversation
dbed6e4
to
13f048b
Compare
85600d2
to
29c5318
Compare
Thanks again for the reviews! The request is currently only being send on document changes, which makes a terrible user experience 😄 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 |
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 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. |
My plan is to send pull diagnostics request:
|
ac7aabf
to
9e667bd
Compare
Edit: This is no outdated. 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. Pull diagnostics for language servers Document ids are being passed around, and i do some cloning of ids.
|
97a7124
to
32b2077
Compare
32b2077
to
1df9b76
Compare
I've been trying this PR to see if it'll solve some issues using the ESLint LS from
Of note, you do have to be using |
1df9b76
to
4177ba6
Compare
Thanks for testing!
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.
I fixed this by skipping the diagnostics request if the document id is not in the editor. Edit:
I am actually (without knowing) been using |
70daf02
to
44c5453
Compare
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. |
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 😄
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. |
fc2895b
to
74a9e9e
Compare
74a9e9e
to
69025dd
Compare
fec66f2
to
19f7d64
Compare
e8957a5
to
3304a54
Compare
3304a54
to
3fb3904
Compare
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. |
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 👍 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 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.webmDo you want me to keep rebaseing and tuning the debounce value on this branch? |
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 |
981b505
to
28e3cac
Compare
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. |
2053a6a
to
2154e19
Compare
I managed to solve this by changing Now, a language server can provide both types of diagnostics. Both rust-analyzer and the C# language server work 🎉 |
2154e19
to
708d8f9
Compare
I solved this issue by pull diagnostics on the first change, and then increase the debounce to 500 ms. The debounce could easily be lower, but the perceived responsiveness is fine at 500. |
21bd5a9
to
46b6ddf
Compare
46b6ddf
to
0a9b068
Compare
There's some extra context in the LSP issue tracker about whether you can support push and pull diagnostics simultaneously microsoft/language-server-protocol#1743. (Continuing in rust-lang/rust-analyzer#18709) |
if event | ||
.doc | ||
.has_language_server_with_feature(LanguageServerFeature::PullDiagnostics) | ||
{ | ||
let document_id = event.doc.id(); | ||
job::dispatch_blocking(move |editor, _| { | ||
let Some(doc) = editor.document(document_id) else { | ||
return; | ||
}; | ||
|
||
let language_servers = | ||
doc.language_servers_with_feature(LanguageServerFeature::PullDiagnostics); | ||
|
||
for language_server in language_servers { | ||
pull_diagnostics_for_document(doc, language_server); | ||
} | ||
}) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we should be able to call pull_diagnostics_for_document
directly and avoid the extra job dispatch
for language_server in event
.doc
.language_servers_with_feature(LanguageServerFeature::PullDiagnostics)
{
pull_diagnostics_for_document(event.doc, language_server);
}
pub enum DiagnosticProvider { | ||
PublishDiagnosticProvider(LanguageServerId), | ||
PullDiagnosticProvider(LanguageServerId), | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the discussions I've read it sounds like this is meant to be done with the identifier
value included on the server capabilities. R-a should add one in a future version rust-lang/rust-analyzer#19266
So instead what I'm thinking for this enum is
pub enum DiagnosticProvider {
Lsp {
server_id: LanguageServerId,
identifier: Option<String>,
},
// In the future, other non-LSP providers like spell checking go here...
}
where pull diagnostics would use that field from the capabilities and push diagnostics would always be None
. (At least for now - the LSP spec could add an identifier to push diagnostics in the future.)
}) | ||
.await | ||
} | ||
Err(err) => log::error!("Pull diagnostic request failed: {err}"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we should also handle the error case here and look for ServerCancelled
and retry based on the cancellation data: https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#diagnosticServerCancellationData
Lack of cancel+retry support for this seems to have bit the neovim+rust-analyzer combination lately: neovim/neovim#30985 / neovim/neovim#31345. I can reproduce getting some error logs from this line about the server cancelling the request when modifying a document shortly after opening a rust file (starting rust-analyzer)
fn handle_pull_diagnostics_response( | ||
editor: &mut Editor, | ||
response: lsp::DocumentDiagnosticReport, | ||
provider: DiagnosticProvider, | ||
uri: Uri, | ||
document_id: DocumentId, | ||
) { | ||
let Some(doc) = editor.document_mut(document_id) else { | ||
return; | ||
}; | ||
|
||
match response { | ||
lsp::DocumentDiagnosticReport::Full(report) => { | ||
// Diagnostic for requested file | ||
editor.handle_lsp_diagnostics( | ||
provider, | ||
uri, | ||
None, | ||
report.full_document_diagnostic_report.items, | ||
report.full_document_diagnostic_report.result_id, | ||
); | ||
|
||
// Diagnostic for related files | ||
handle_document_diagnostic_report_kind( | ||
editor, | ||
document_id, | ||
report.related_documents, | ||
provider, | ||
); | ||
} | ||
lsp::DocumentDiagnosticReport::Unchanged(report) => { | ||
doc.previous_diagnostic_id = | ||
Some(report.unchanged_document_diagnostic_report.result_id); | ||
|
||
handle_document_diagnostic_report_kind( | ||
editor, | ||
document_id, | ||
report.related_documents, | ||
provider, | ||
); | ||
} | ||
} | ||
} | ||
|
||
fn handle_document_diagnostic_report_kind( | ||
editor: &mut Editor, | ||
document_id: DocumentId, | ||
report: Option<HashMap<lsp::Url, lsp::DocumentDiagnosticReportKind>>, | ||
provider: DiagnosticProvider, | ||
) { | ||
for (url, report) in report.into_iter().flatten() { | ||
match report { | ||
lsp::DocumentDiagnosticReportKind::Full(report) => { | ||
let Ok(uri) = Uri::try_from(url) else { | ||
return; | ||
}; | ||
|
||
editor.handle_lsp_diagnostics(provider, uri, None, report.items, report.result_id); | ||
} | ||
lsp::DocumentDiagnosticReportKind::Unchanged(report) => { | ||
let Some(doc) = editor.document_mut(document_id) else { | ||
return; | ||
}; | ||
doc.previous_diagnostic_id = Some(report.result_id); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can eliminate the extra helper function here and make this a bit shorter
fn handle_pull_diagnostics_response(
editor: &mut Editor,
response: lsp::DocumentDiagnosticReport,
provider: DiagnosticProvider,
uri: Uri,
document_id: DocumentId,
) {
let (result_id, related_documents) = match response {
lsp::DocumentDiagnosticReport::Full(report) => {
editor.handle_lsp_diagnostics(
provider,
uri,
None,
report.full_document_diagnostic_report.items,
);
(
report.full_document_diagnostic_report.result_id,
report.related_documents,
)
}
lsp::DocumentDiagnosticReport::Unchanged(report) => (
Some(report.unchanged_document_diagnostic_report.result_id),
report.related_documents,
),
};
if let Some(doc) = editor.document_mut(document_id) {
doc.previous_diagnostic_id = result_id;
};
for (url, report) in related_documents.into_iter().flatten() {
let result_id = match report {
lsp::DocumentDiagnosticReportKind::Full(report) => {
let Ok(uri) = Uri::try_from(url) else {
continue;
};
editor.handle_lsp_diagnostics(provider, uri, None, report.items);
report.result_id
}
lsp::DocumentDiagnosticReportKind::Unchanged(report) => Some(report.result_id),
};
if let Some(doc) = editor.document_mut(document_id) {
doc.previous_diagnostic_id = result_id;
}
}
}
I think we should also try to apply as much of the report (continue rather than return).
I also changed the signature of Editor::handle_lsp_diagnostics
here so that the calling code here sets the result_id
rather than passing it. I think that makes the option handling a little cleaner and we don't need to worry about the meaning of this field for push diagnostics.
Instead of passing the result_id though I believe we will need to pass the identifier
(or at least the DiagnosticProvider
).
impl PullDiagnosticsHandler { | ||
pub fn new() -> PullDiagnosticsHandler { | ||
PullDiagnosticsHandler { | ||
document_ids: [].into(), | ||
} | ||
} | ||
} | ||
|
||
impl helix_event::AsyncHook for PullDiagnosticsHandler { | ||
type Event = PullDiagnosticsEvent; | ||
|
||
fn handle_event( | ||
&mut self, | ||
event: Self::Event, | ||
existing_debounce: Option<tokio::time::Instant>, | ||
) -> Option<tokio::time::Instant> { | ||
if existing_debounce.is_none() { | ||
dispatch_pull_diagnostic_for_document(event.document_id); | ||
} | ||
|
||
self.document_ids.insert(event.document_id); | ||
Some(Instant::now() + Duration::from_millis(500)) | ||
} | ||
|
||
fn finish_debounce(&mut self) { | ||
for document_id in self.document_ids.clone() { | ||
dispatch_pull_diagnostic_for_document(document_id); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the handler I think we will want some more nuanced logic. There are some tips at the bottom of the "Diagnostics Refresh" section in the spec that I think are meant to apply to pull diagnostics generally and are just out of place in the spec at the moment:
Implementation Considerations
Generally the language server specification doesn’t enforce any specific client implementation since those usually depend on how the client UI behaves. However since diagnostics can be provided on a document and workspace level here are some tips:
- a client should pull actively for the document the users types in.
- if the server signals inter file dependencies a client should also pull for visible documents to ensure accurate diagnostics. However the pull should happen less frequently.
- if the server signals workspace pull support a client should also pull for workspace diagnostics. It is recommended for clients to implement partial result progress for the workspace pull to allow servers to keep the request open for a long time. If a server closes a workspace diagnostic pull request the client should re-trigger the request.
So the behavior should depend on whether the server declares that the language has inter-file dependencies. For language servers with inter-file dependencies we want to query for all "visible" documents attached to that language server. And for languages without inter-file dependencies we can request diagnostics only when that document changes, and we can pull on a lower debounce.
As for what is a "visible" document I think it's meant to be left up to the editor. In our case I think it's fine to query any open document (so we could look at Editor
's documents instead of holding a hashset here).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the behavior should depend on whether the server declares that the language has inter-file dependencies. For language servers with inter-file dependencies we want to query for all "visible" documents attached to that language server. And for languages without inter-file dependencies we can request diagnostics only when that document changes, and we can pull on a lower debounce.
I think this would require two separate async hooks: One for servers with and without interfile dependencies. Is this something we want?
As for what is a "visible" document I think it's meant to be left up to the editor. In our case I think it's fine to query any open document (so we could look at
Editor
's documents instead of holding a hashset here).
I think this requires that i add Editor
to the DocumentDidChange
and DocumentDidOpen
in helix-view/src/events.rs
. Not that this is a problem, it will just require a lot of changes
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.0roslyn language sever
for C# version 4.12.0ruby-lsp
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.