Clarify how to support both publish and pull diagnostics

[tristanlabelle]

The spec is not clear on whether diagnostics from publish and pull models interact with one another. If a language server reports supporting pull diagnostics, but always returns an empty array from textDocument/diagnostic and keeps publishing diagnostics using textDocument/publishDiagnostic, will the client get confused as to whether there are diagnostics or not? Or would the client manage the two sets of diagnostics independently?

Related: if a server sees the client capabilities support for pull diagnostics and reports supporting pull diagnostics itself, can it then assume that textDocument/diagnostic requests will be coming and it need not ever use textDocument/publishDiagnostic?

This comes into play because I'm working on adding pull diagnostics support to the Swift language server to replace publish diagnostics because it fixes some stale diagnostics issues we're running into. The Swift language server is behind a proxy language server (SourceKit-LSP) serving both Swift and Clang, which makes it more difficult to report capabilities because the capabilities of either language-specific servers may differ, and I want to make sure not to break the Clang server if it only supports publish diagnostics.

[dbaeumer]

A server can use both pull and push and the client will manage two independent sets for them (unless they are folded onto the same ID which I think most servers don't use).

If a server opts into providing pull then the client pulls for diagnostics and the server doesn't need to push.

Hope that helps.

[jwortmann]

@dbaeumer Could you clarify a bit more what the expected behavior is, because at least to me this answer raises more questions than it solves.

In particular, those two sentences are a bit contradictory (or at least they don't seem to clarify much). First you say "server can use both pull and push", but then in the second sentence that "server doesn't need to push" if it already provides pull diagnostics. So in other words, it is entirely up to the server whether it will additionally send the push notification, if it already responds to pull diagnostics, right?

A server can use both pull and push and the client will manage two independent sets for them

At least in the client for Sublime Text, this is not the case, and the pull and push diagnostics for the same document will just overwrite each other. So to answer the question from the opening post

If a language server reports supporting pull diagnostics, but always returns an empty array from textDocument/diagnostic and keeps publishing diagnostics using textDocument/publishDiagnostic, will the client get confused as to whether there are diagnostics or not?

this should indeed work with the Sublime Text client, provided that the push notification arrives after the empty pull diagnostics response. However, and without having tested it, I assume that this could lead to an ugly flickering of squiggly underlines in the editor UI, dependent on the time difference between those messages. And in general this client implementation was made with the assumtion in mind that servers won't send additional publishDiagnostics notifications if they already support pull diagnostics and clients also signalize pull diagnostics support in the initialization options.

If - in contrast to that - "the client will manage two independent sets for them" is the intended behavior, then this should definitely be added to the LSP specs, because at the moment and as far as I can tell, there is nothing written in the specs which would mention it. And I don't think one can take it as a given presumption, because there should be no rational reason to expect servers also to send push notifications, if they already support and respond to pull diagnostics.

And could you please clarify what exactly you mean with

(unless they are folded onto the same ID which I think most servers don't use)

The "ID" can't be the

	/**
	 * An optional identifier under which the diagnostics are
	 * managed by the client.
	 */
	identifier?: string;

from the DiagnosticOptions server capability, because there exist no such corresponding property for publishDiagnostics notifications.

Also what should happen in the editor UI if a server uses both pull and push at the same time; should the editor display the same diagnostic twice to the user, or are the sets of diagnostics from pull and push expected to be mutually exclusive / non overlapping?

[dbaeumer]

The "ID" can't be the

Actually the ID is that. The ID allows to create different sets of diagnostics which a client should manage independently. This allows a server to manage independent stream of diagnostics (for example one the can be computed fast and one that is slower).

The push case indeed has no identifier in the spec which indeed makes it hard to understand what happens since the ID at least in VS Code is managed by the client.

So I opt to simply add to the spec that a server if opting into pull it shouldn't push diagnostics for the same set of resources.

[jwortmann]

@dbaeumer

So I opt to simply add to the spec that a server if opting into pull it shouldn't push diagnostics for the same set of resources.

It seems that this has been forgotten and we have seen some behavior mismatch about this recently, so it would be good if you could still add this sentence to the specs explicitly to clear things up.

Also it would be great and I would recommend to add one or two sentences about the general purpose of the identifier to the specs. Basically what you wrote above about independent diagnostic streams, i.e. that servers can register multiple identifiers and that clients should manage diagnostics from different identifiers independently and that those should not overwrite each other. Currently there is only in the specs that the identifier exists and that it should be sent back to the server when doing pull diagnostic requests, but nothing more.

[rchl]

@dbaeumer rust-analyzer now depends on pull and push diagnostics being two independent streams that shouldn't override each other. That means that clients are kinda forced to support that even though it's not expected given your comment above.