Add support for pull diagnostics #2221

jwortmann · 2023-03-18T15:17:40Z

Closes #2095

Here is something, that I put together in an hour or two, so I haven't rigorously tested everything. But it can be tested with LSP-css and seems to work well so far.

Workspace diagnostics (workspace/diagnostic request) are not yet implemented, and it would have to be discussed and decided on what occasion this request would be useful. I don't think there is a point in testing this with LSP-css, because afaik that server doesn't rely on a project/workspace. If I understand it correctly, it might be useful to send this request if a configuration file changes, e.g. pyproject.toml for Python. When editing the regular files which have a session attached, then the textDocument/diagnostic request should already be sufficient (the response can contain diagnostics for other files as well), and there is also the workspace/diagnostic/refresh server request implemented in this PR.

I don't really see a big advantage in the pull diagnostics model as it is described in the specs, but it would be needed if a server decides to drop support for the regular diagnostics notification in the future (see referenced issue).

rchl · 2023-03-20T08:03:28Z

I suppose the most tricky part with pull diagnostics is to know when to request them. It's clear that this should happen on file edit but what about background files? The diagnostics might change in those when editing the current file. Is server gonna explicitly refresh those in that case or will it rely on the client?

Also, after brief look, I think we might be missing update after capability is registered. Unless, again, the server will trigger refresh in that case?

jwortmann · 2023-03-20T15:49:55Z

but what about background files?

I think there are 3 possible ways how diagnostics for background files work:

The server just sends the diagnostics for other affected files via the relatedDocuments field in the response. We have signalized support for this through "relatedDocumentSupport": True. This should be the simplest and most effective way (no additional request required).
If other files are affected, the server requests workspace/diagnostic/refresh. We will then send new requests for all files which are opened in the editor (immediately for visible files, and for all other files only when their tab gets activated). This is not very effective (client doesn't know which files are affected and which have unchanged diagnostics), and diagnostics shown (or not shown) in the diagnostics panel & Goto Diagnostic quick panel might be outdated for tabs which are currently not visible.
The server does neither 1. nor 2. and instead it expects the client to predict the files for which diagnostics should be updated. This would be the second point under "Implementation considerations" in the specs, but implemented in the least effective way:
- 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.
This is not implemented in this PR at the moment, and I'm not sure whether this would really be a good solution. How often would you do requests for background files? After a fixed time, say e.g. every 30 seconds, or just trigger it after each buffer change, but debounced and with a really big debounce time (maybe like 10 or 30 seconds)? If the user has lots of files open, it might be a lot of requests.

I think we might be missing update after capability is registered. Unless, again, the server will trigger refresh in that case?

Right. It should be fixed in 7f656c8. But I think the other features (color boxes, semantic highlighting, inlay hints) miss update after dynamic registration too.

LSP-json also supports pull diagnostics, but both LSP-json and LSP-css use static registration. And they do send workspace/diagnostic/refresh after initialization.

predragnikolic · 2023-03-22T23:06:38Z

+1 for this PR

Pretty happy with the PR, great work!

For me this PR is mergeable,
but I will continue testing this more in the following days.

jwortmann · 2023-03-24T17:45:07Z

It would be interesting to see how it works with a server which actually depends on a workspace and where changes in one file can affect diagnostics in other files. But I'm not aware of any project-based server with support for pull diagnostics. If anyone knows such a server, a hint would be welcome.

As long as there is no server that entirely drops support for the regular publishDiagnostics, I think there is no urgent need to merge this, so I would have no problems with leaving this PR open for a while.

I also wonder whether this client capability should perhaps be disabled by default, so that servers which support both pull and push diagnostics would still use the old publishDiagnostics. This might prevent the potential drawback of outdated diagnostics for background files / inactive tabs, that I mentioned above (but I don't know how likely this might actually occur in practice).
And then only activate the capability for servers which will drop support for the old push diagnostics, e.g. in LSP-eslint.sublime-settings.

rchl · 2023-03-24T21:07:31Z

I think I agree that we could disable the capability for now since I don't really see any benefit with the servers I'm using at least (which is quite limited subset of servers, I admit). Then we could merge it to avoid conflicts piling up.

I still want to have a close look into the changes but it looks good on cursory look.

I guess we could do some cool stuff with it like debouncing diagnostics request on typing to give priority to completions but that would require servers that support pull model and also might not be universally wanted from users.

rchl · 2023-03-25T21:25:55Z

plugin/session_buffer.py

+            # Retrigger the request after a short delay, but only if there were no additional changes to the buffer (in
+            # that case the request will be retriggered automatically anyway)
+            version = view.change_count()
+            sublime.set_timeout_async(self._if_view_unchanged(self.do_document_diagnostic_async, version), 500)


I've updated it to look more readable by replacing Callable that can't express optional arguments with a custom type.

But now version is not passed to self.do_document_diagnostic_async, no? That's why I appended (version) and need a lamda here (I think).

Actually I see that I made version optional. So it's not needed and this should be okay.
I think the reason I put it here, is to save one ST API call.

Should we just remove the version argument? It looks like we are not really consistent and sometimes passing it and sometimes not. And it is able to infer it automatically itself.

I see your argument but I think we can do that extra api request in this case for the sake of clarity in our code.

I think it's there to save some duplicate API calls, but I only added this advantage with version = view.change_count() in _check_did_open in this PR. But yeah, it's only used inconsistently by a few methods.

Should probably okay to remove it, since I assume that View.change_count() is not very expensive.

To be clear, what I've meant is to remove the version argument from all functions then. I've re-added it for now, and mabye it looks ugly, but that's because of the way how _if_view_unchanged is defined to return a callable and accept arguments. If it should not be called with arguments, in order to make the code look nicer, then I think in the first place the _if_view_unchanged function should be adjusted to prevent that.

rchl · 2023-03-25T21:30:14Z

plugin/core/sessions.py

@@ -1745,13 +1758,22 @@ def m_workspace_inlayHint_refresh(self, params: None, request_id: Any) -> None:
        for sv in not_visible_session_views:
            sv.session_buffer.set_inlay_hints_pending_refresh()

+    def m_workspace_diagnostic_refresh(self, params: None, request_id: Any) -> None:
+        """handles the workspace/diagnostic/refresh request"""
+        self.send_response(Response(request_id, None))


It's perhaps a bit unclear in the LSP spec whether the response should be sent after or before the related requests are finished but I guess we are currently consistent with ourselves at least.

I would think that if it was expected to postpone the response until the other requests were sent, then this would explicitly be mentioned in the specs. So I think the requests are independent and it should be fine to immediately send the response here.

rchl · 2023-03-25T21:34:28Z

LSP-json seems to respond with error to the diagnostic request but still publishes diagnostics on its own so it looks like it either doesn't support it after all or some configuration needs to be changed.

rchl · 2023-03-25T21:41:12Z

No, wait. That's because I forgot to re-enable capabilities. But that shows the problem that when capabilities are disabled then we are still sending those requests.

jwortmann · 2023-03-25T21:43:47Z

LSP-json seems to respond with error to the diagnostic request but still publishes diagnostics on its own so it looks like it either doesn't support it after all or some configuration needs to be changed.

I think it's because of d8c4ac2

But that looks like a bug in LSP-json to me, because it should not announce support for diagnosticProvider then. Probably the same in LSP-css and other vscode-node based servers.

rchl · 2023-03-25T21:50:09Z

But that looks like a bug in LSP-json to me, because it should not announce support for diagnosticProvider then.

Hmm, right.

jwortmann · 2023-03-25T21:51:16Z

This is the relevant code in LSP-json:
https://github.com/microsoft/vscode/blob/0e530c3e078af61e883f15d1608bcad677f278bc/extensions/json-language-features/server/src/jsonServer.ts#L166-L190

It still advertises support in the initialize response, but internally only enables push support.

jwortmann · 2023-03-25T21:55:42Z

Anyways, it's probably not expected to send requests if we don't advertise support ourselves, and I guess it won't be fixed in the vscode servers.

So we should either temporarily disable the do_document_diagnostic_async method too, or just try with this feature and the capabilities enabled.

rchl · 2023-03-27T18:38:07Z

We can try it

* main: Clear pull diagnostics on file closed (#2224) html-escape diagnostic-related strings (#2228) Allow style overrides for inlay_hints.css (#2232) Fix exception for null response id (#2233) Add "outline" as an option for "document_highlight_style" (#2234) remove accidentally committed files Improve label detail support in completions (#2212) Update clojure-lsp docs (#2226) Add support for pull diagnostics (#2221) update test MockManager after API change add a client option for hiding non-project diagnostics (#2218) Fix some features might not work with dynamical registration (#2222)

jwortmann added 2 commits March 18, 2023 15:57

Add support for pull diagnostics

b087a38

Use capability from SessionBuffer

e71ac94

jwortmann mentioned this pull request Mar 19, 2023

Fix some features might not work with dynamical registration #2222

Merged

Update after dynamic registration

7f656c8

jwortmann and others added 4 commits March 25, 2023 20:10

Merge branch 'main' into pull-diagnostics

4a5df00

Disable capability for now

d8c4ac2

Merge branch 'main' into pull-diagnostics

c2892a8

better type with optional arguments

4587b66

rchl reviewed Mar 25, 2023

View reviewed changes

more clarity

6a721c4

Revert some changes

3285685

rchl approved these changes Mar 27, 2023

View reviewed changes

rchl merged commit 27a5861 into sublimelsp:main Mar 27, 2023

jwortmann deleted the pull-diagnostics branch March 28, 2023 06:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for pull diagnostics #2221

Add support for pull diagnostics #2221

jwortmann commented Mar 18, 2023

rchl commented Mar 20, 2023

jwortmann commented Mar 20, 2023

predragnikolic commented Mar 22, 2023

jwortmann commented Mar 24, 2023 •

edited

Loading

rchl commented Mar 24, 2023 •

edited

Loading

rchl Mar 25, 2023

jwortmann Mar 25, 2023

jwortmann Mar 25, 2023 •

edited

Loading

rchl Mar 25, 2023

rchl Mar 25, 2023

jwortmann Mar 25, 2023 •

edited

Loading

jwortmann Mar 27, 2023

rchl Mar 25, 2023 •

edited

Loading

jwortmann Mar 27, 2023 •

edited

Loading

rchl commented Mar 25, 2023

rchl commented Mar 25, 2023

jwortmann commented Mar 25, 2023

rchl commented Mar 25, 2023

jwortmann commented Mar 25, 2023

jwortmann commented Mar 25, 2023

rchl commented Mar 27, 2023

Add support for pull diagnostics #2221

Add support for pull diagnostics #2221

Conversation

jwortmann commented Mar 18, 2023

rchl commented Mar 20, 2023

jwortmann commented Mar 20, 2023

predragnikolic commented Mar 22, 2023

jwortmann commented Mar 24, 2023 • edited Loading

rchl commented Mar 24, 2023 • edited Loading

rchl Mar 25, 2023

Choose a reason for hiding this comment

jwortmann Mar 25, 2023

Choose a reason for hiding this comment

jwortmann Mar 25, 2023 • edited Loading

Choose a reason for hiding this comment

rchl Mar 25, 2023

Choose a reason for hiding this comment

rchl Mar 25, 2023

Choose a reason for hiding this comment

jwortmann Mar 25, 2023 • edited Loading

Choose a reason for hiding this comment

jwortmann Mar 27, 2023

Choose a reason for hiding this comment

rchl Mar 25, 2023 • edited Loading

Choose a reason for hiding this comment

jwortmann Mar 27, 2023 • edited Loading

Choose a reason for hiding this comment

rchl commented Mar 25, 2023

rchl commented Mar 25, 2023

jwortmann commented Mar 25, 2023

rchl commented Mar 25, 2023

jwortmann commented Mar 25, 2023

jwortmann commented Mar 25, 2023

rchl commented Mar 27, 2023

jwortmann commented Mar 24, 2023 •

edited

Loading

rchl commented Mar 24, 2023 •

edited

Loading

jwortmann Mar 25, 2023 •

edited

Loading

jwortmann Mar 25, 2023 •

edited

Loading

rchl Mar 25, 2023 •

edited

Loading

jwortmann Mar 27, 2023 •

edited

Loading