Add support for explicitly requesting diagnostics
w0rp opened this issue · 32 comments
The Language Server Protocol sends diagnostics to clients as notifications. These notifications are typically sent in response to textDocument/didChange or textDocument/didSave, but they could arrive at any time. Clients have no control over when or if diagnostics will be sent to them. This lack of control causes problems for clients.
- Clients cannot indicate if there there are pending diagnostics. Because they can arrive at any time, or may never arrive, you cannot indicate if there are diagnostics being calculated or not.
- Clients cannot easily control when diagnostics should be displayed. A document update in order to retrieve accurate completion results may result in diagnostics being sent, for example.
- Servers can send clients diagnostics for files they might not be interested in, such as files that are not open in client editors. This results in clients having to read more data than they need.
I think it would be a good idea to add an optional mode to the protocol to disable sending diagnostics to clients unless they are explicitly requested for a file. An optional attribute could be added to InitializeParams which when enabled would tell servers not to send diagnostics until a new message type textDocument/getDiagonistics is sent. ServerCapabilities could include an attribute for indicating if this mode is supported. Servers would then be required to respond to clients with textDocument/publishDiagnostics with either an empty diagnostics array, or previously computed results.
For my LSP client ALE, this would mean that ALE will be able to update the Vim status bar to indicate that diagnostic results from language servers are pending, and to avoid requesting diagnostics when the user only wants to get some completion results. Currently ALE is able to do this for tsserver, because the tsserver protocol requires the use of a geterr command, but this is unfortunately not possible with language servers.
Let me know what you think. Thank you for the protocol.
@aeschli we discussed something like this awhile back as well.
@w0rp the guarding of such a capability would need to be done in the client and server capabilities, not in the InitializeParams. However if servers support that mode they shouldn't push any diagnostics anymore.
One tricky part could be is how to clear diagnostics on document close. Clients would need to send a corresponding request in this case.
IMO we need:
a. a new request textDocument/getDiagonistics for the client to request diagnostics for a specific URL from the server
b. a way to configure the server whether and for which URLs the server it should publish diagnostics. This will likely be a selector as the client doesn't know all URIs that can get diagnostics.
However if servers support that mode they shouldn't push any diagnostics anymore.
I don't think a. and b. should be coupled. The user can configure it that way, but you can also think of a usage where the problems view is fed by published diagnostics (as now) but there are also programmatic requests for diagnosics of a specific document for other another use cases.
I don't think a. and b. should be coupled.
After reading this again I agree.
I agree that the explicit diagnostic requests and disabling automatic publishing of diagnostics in response to notifications like textDocument/didChange can be implemented as two separate features.
FWIW: The setup where server 'pushes' diagnostics works just fine from our point of view. And this also fits with situations where the server performs something like workspace builds, and then pushes any build-related problems as 'diagnostics' to the client. Such workflow is not driven by the client, but by the server.
I do also agree that there may be situations where a pull model makes more sense. I'm fine with adding that as long as its a 'opt-in' sort of thing and we also keep the 'push' model as it exists now.
I think both are valid use cases, for different purposes.
Just to clarify: it is not our intention to deprecate the push model. Pull would be simply another way to get to diagnostics.
I have another use case for this. For https://github.com/apexskier/nova-typescript/, the editor manages the language server entirely, and intercepts diagnostic notifications. When requesting code actions, I need to pass diagnostics to the server to receive all actions, but I can't because I currently don't have a way to know the diagnostics. I'd say this is more of a Nova issue - it should provide a way for me to get a list of diagnostics, since it's already managing them, but this would be another way to solve it.
Would it make sense if the client could "filter" diagnostics, in addition to adding the "client pull" capability?
For example, the client could ask the server to not pull diagnostics matching tag = "Linting" and code ∈ (E328, E089), and then could later specifically pull an unfiltered diagnostic when the user wants it.
This is like the difference between a "signal" and a "method" in D-Bus.
If there's ever the ability to filter which type of diagnostics are received, I think it's worth implementing that after being able to just pull everything at first, so we can see that by itself finished sooner.
It made the VSCode release notes now, and we're getting some questions about it!
https://code.visualstudio.com/updates/v1_54#_language-server-protocol
It would be great to have some details in the upcoming docs :-)
From reading https://github.com/microsoft/vscode-languageserver-node/blob/main/protocol/src/common/proposed.diagnostic.ts, a couple of questions:
- the request returns a diagonstic list | null, or an error as usual. What's the effect of returning null? (when reparsing includes, clangd goes into a low-fidelity state where it doesn't publish diagnostics. It would be useful to us to return null and then request
refreshonce we're done, but I can't tell if that design is valid) - as a language server that supports real-time diagnostics, I'm not really sure how to populate the pull flags. We'd prefer to push if possible (as it gives us more flexibility in scheduling), so I'm not sure whether we should set all the flags or none of them.
@sam-mccall as said this is currently proposed and might very likely change again. To clarify some things:
- if a server can't fulfil a request right now because it is busy the server can cancel the request and the client is supposed to retrigger it.
- we also will provide a workspace pull model which will allow to compute all errors for the workspace. We are working on details but it will consider the scheduling of the server.
Push will not be removed. So if we server wants to use push it will be able to do so. However currently it is hard to tell for a server if and when it should compute diagnostic for a single document.
Thanks!
if a server can't fulfil a request right now because it is busy the server can cancel the request and the client is supposed to retrigger it.
If the client retriggers, we'll likely (quickly) cancel the request again, and it can take up to a minute to become ready. It'd be nice to be able to have the client wait for a refresh request to retrigger to avoid a retry loop, but probably not a huge deal in practice.
FWIW we'd been (independently) looking at extensions for pull-diagnostics to better support distributed web clients, where pull seems to work better than push. I'm not sure if you've had this case in mind, but the pull+refresh model in this extension (and semanticTokens) looks good from that perspective.
Yes, being able to explicitly pull for diagnostics (even programmatically via API) is the use case we want to support as well.
@dbaeumer since an implementation "got published", is there any documentation about how Diagnostic Pull proposal can be used?
Would be interesting for extensions such as Error Lens usernamehw/vscode-error-lens#77
Since the implementation is still proposed I leep the doc with the implementation. It is here: https://github.com/microsoft/vscode-languageserver-node/blob/main/protocol/src/common/proposed.diagnostics.md#L1
I've noticed a problem with the current protocol: a client is not able to know for sure if a diagnostics message reflects the latest document changes. I was following this ticket in hopes it would be a solution, but upon revisiting the proposal, I don't think it will.
As with other pull requests the server is asked to compute the diagnostics for the currently synced version of the document.
"Currently synced version" is imprecise. Is there no way for the server to communicate which version of the document the diagnostics are for? Consider the following scheme:
- Client detects changes to the document
- Client updates the document and generates a new ID for latest document version
- Client notifies the server
- Server computes the diagnostics for the document
- Server responds with diagnostics + the document ID for which the diagnostics were computed
That way the client can ignore the diagnostics if it is not matching the latest ID (effectively means the diagnostics are outdated).
Without a way of precisely knowing which version of the document the diagnostics are for, the client is forced to rely on the assumptions of:
The latest diagnostics are up-to-date because
- The server should have cancelled any previous diagnostic processing as soon as it was notified of the document update and
- The server should have received the latest document updates before it responded
Therefore this message can't possibly refer to an old version
As far as I am aware those assumptions are not guaranteed by the protocol, nor it means the latest diagnostics surely incorporate the latest document changes, as demonstrated by the following scheme:
- Client detects changes to the document
- Client updates the document
- Server is already computing some diagnostics in the meantime
- Client notifies the server
- Before the client's message arrives, the server responds with outdated diagnostics
I was thinking the pull model could be a solution to this problem because the client could request diagnostics again if the document had changed before the server replied. Even despite of that, as I understood the proposal, however, the major flaw of "a client is not able to know if the diagnostics incorporate the latest document changes" still exists and is subject to transport delays and failures in the client <-> server communication.
"Currently synced version" is imprecise. Is there no way for the server to communicate which version of the document the diagnostics are for?
With all request/response pairs on a document, the response pertains to the version of the file that was valid when the request was sent (e.g. the last didChange notification before the request).
As far as I am aware those assumptions are not guaranteed by the protocol
I assure you that implementations rely on them. If that guarantee isn't clear enough then it should be made clearer, but adding a second way to determine the version is going to create confusing.
With all request/response pairs on a document, the response pertains to the version of the file that was valid when the request was sent (e.g. the last didChange notification before the request).
So the client first has to flush all change events related to the document before sending the request. Makes sense. Now here's the question, does the client always know that the changes' messages are successfully delivered before the diagnostics request is made? Please bear in mind I'm no expert in the supported network protocols, but as far as I'm aware not all of them have an "ACK" mechanism like TCP does.
I assure you that implementations rely on them. If that guarantee isn't clear enough then it should be made clearer, but adding a second way to determine the version is going to create confusing.
On the server pushes model, even if "The server should have cancelled any previous diagnostic processing as soon as it was notified of the document update" is being followed in the wild (unclear if it is; server implementers might think it is fine to send the ongoing ones first and queue the latest ones for later), in the scenario I brought up above:
- Client detects changes to the document
- Client updates the document
- Server is already computing some diagnostics in the meantime
- Client notifies the server
- Before the client's message arrives, the server responds with outdated diagnostics
Suppose the client is able to work around this scenario through the following strategy: "As soon as I detect document changes locally, then ignore all diagnostics the server pushes for this document until I have flushed all changes notifications" which sounds fine but then, again, do we have this "ACK" delivery guarantee on all supported transport methods?
For all the problems I brought up so far, a document content ID seems like the simplest solution. Perhaps it's not required though.
So the client first has to flush all change events related to the document before sending the request. Makes sense. Now here's the question, does the client always know that the changes' messages are successfully delivered before the diagnostics request is made? Please bear in mind I'm no expert in the supported network protocols, but as far as I'm aware not all of them have an "ACK" mechanism like TCP does.
My interpretation: the client gets to assume that the change messages are delivered before the diagnostics request is delivered.
This requires that:
- the client sends the change messages first.
- the network protocol preserves the order of client->server messages, and guarantees delivery or an error state.
- the server handles messages in order. (Even if it ultimately does async work and responds out-of-order)
The spec doesn't explicitly say what protocols are supported, but it mentions stdin/stdout, windows pipes, unix domain sockets, and network sockets. These protocols do preserve order of messages in each direction (unless we're talking about UDP sockets). Note that there's no requirement that the client->server message stream and the server->client message stream are synchronized in any way.
I think it would be great if the spec said something about the constraints on the transport protocol, because JSON-RPC itself doesn't have this requirement (it seems perfectly sensible to have a stateless JSON-RPC server over UDP or something).
Here's one more more interesting scenario in the absence of a content ID: a client sends two diagnostic pulls but then it does not know if the response refers to the first or second request.
Workarounds:
- Not send more requests until a response arrives
- Keep a count of sent requests, decrement when a response arrives, then process the payload when the count reaches 0
- This model assumes the server will always respond once for each request and not give up on responding to the first one if the second was received while the first was ongoing
Notably, the diagnostic responses for pull should be distinguishable from the ones in server pushes for any of those strategies to work.
does not know if the response refers to the first or second request.
JSON-RPC requests include a mandatory request ID, and responses include the ID of the request they're associated with.
JSON-RPC requests include a mandatory request ID, and responses include the ID of the request they're associated with.
I was not aware of that. Thank you.
All explanations considered, my takeaway for the time being is that it'll be possible to get away without content IDs. I'll keep thinking about the scenarios to see if there's one which is not covered by the current proposal.
@sam-mccall thanks a lot for helping @resolritter with this!
@dbaeumer Hey Dirk, do you have any idea when 3.17 might land in VSCode?
(Going with the request forwarding method for embedded languages for my extension that splits a page into server-side JS and Svelte virtual documents and was just wondering when-ish I could expect to implement diagnostics with this approach.)
@dbaeumer Hey Dirk, do you have any idea when 3.17 might land in VSCode?
There is a client/src/common/proposed.diagnostic.ts file. Are you encountering bugs with the implementation?
Added to 3.17 which shipped today :-)
I just saw this now, and this is great news. This will be a game changer for editors.
That's great! IIUC however the Document Diagnostic request doesn't include a document point.
Do you think that is something that could be added in 3.18? It would be useful for us as to be able to check the document part that is near the cursor (a common use case in our context, due to slow checking times)
You usually can infer that from the latest changes made to the document.
In general, in LSP we stay away from syncing UI state (cursor, open views, ...)
I had understood from #718 (comment) that the current visible range would be acceptable; IMHO that's a good hint. [So I assume the range here would be akin to the position that is sent in most other requests, like hover]
Inferring the view from changes is actually quite brittle, systems that implement viewport diagnostics usually want them refreshed on scroll, and for the use cases of Isabelle and Coq that's what you want, as the user moves ahead in a document you want to schedule the checking for that part.
@ejgallego yes, that is something we can add since we sent this in other situations as well.