Document diagnostics not cleared on file close when workspace diagnostics enabled

[JohnnyMorganz]

This follows on from #1013, where it was fixed that document diagnostics are cleared when a file closed.

This works fine when a server does not support workspace diagnostics.

There, it was mentioned by @aeschli:

@JohnnyMorganz You should looking into using workspace diagnostics

• Workspace diagnostics are shown for all resources, unless the ones open in an editor
• Document diagnostics are shown for the resources in open editors.
  Your workspace diagnostics would not contain diagnostics for ignored resources, but the your document diagnostics would.

So I followed these steps. I implemented workspace diagnostics, and my workspace diagnostics do not follow include ignored resources - but document diagnostics do.

However, if I have workspace diagnostics enabled, the document diagnostics do not clear on file close.

I updated my pull diagnostics minimal repro (https://github.com/JohnnyMorganz/pull-diagnostics-bug) with the following diff:

diff --git a/server/src/server.ts b/server/src/server.ts
index 1069d4a..6a8646b 100644
--- a/server/src/server.ts
+++ b/server/src/server.ts
@@ -30,7 +30,7 @@ connection.onInitialize((params: InitializeParams) => {
       textDocumentSync: TextDocumentSyncKind.Incremental,
       diagnosticProvider: {
         interFileDependencies: true,
-        workspaceDiagnostics: false,
+        workspaceDiagnostics: true,
       },
       workspace: {
         workspaceFolders: {
@@ -80,6 +80,12 @@ connection.languages.diagnostics.on((params) => {
   };
 });

+connection.languages.diagnostics.onWorkspace(() => {
+  return {
+    items: [],
+  };
+});
+
 // Make the text document manager listen on the connection
 // for open, change and close text document events
 documents.listen(connection);

[dbaeumer]

However, if I have workspace diagnostics enabled, the document diagnostics do not clear on file close.

Actually this is on purpose. It is assumed that workspace diagnostics is a super set of document diagnostics. Is this incorrect for you?

[JohnnyMorganz]

Actually this is on purpose. It is assumed that workspace diagnostics is a super set of document diagnostics. Is this incorrect for you?

Yeah, since it was recommended I should make workspace diagnostics empty for ignored files, and use document diagnostics instead:

@aeschli said

Your workspace diagnostics would not contain diagnostics for ignored resources, but the your document diagnostics would.

in #1013 (comment), so it cannot be a superset.

This makes sense to me, and it would be useful to me if this worked

[JohnnyMorganz]

Also, if workspace diagnostics is meant to be a superset, is there any purpose in recomputing document diagnostics then? I would've assumed workspace diagnostics would then just be sufficient

[dbaeumer]

I am nit sure I understand this: why would document diagnostics be shown for ignored resource but workspace diagnostics not. Can you give me an example where this is useful?

[JohnnyMorganz]

Our usecase:
Users configure files to ignore (typically these are 3rd party dependencies / vendored files). Generally, they do not want diagnostics shown for this file, so that they don't pollute things like the problems window or make everything in the tree go red. Therefore, we do not provide workspace diagnostics for these files.

In the cases when they open an ignored file, it is useful to provide temporary diagnostics. Users normally open up these files when they are debugging things, we can therefore highlight type errors, or lint things like undefined variables. This can help them spot a problem faster when they have the file open. Once they close it, they don't want those diagnostics anymore.
This is where document diagnostics help, since they only show for active files.

#848 is another issue which has the same use case

[dbaeumer]

I see the use case but this is currently not possible. I will mark it as a feature request since the tricky case here is to avoid problem flickering since the library doesn't know whether the file is ignored or not.

[JohnnyMorganz]

Thank you for taking the time to look into this

The main thing for me here is only on the "file close" (i.e. file becomes not active) event for the client.
It seems to be doing 2 different things:

• If ServerCapabilities.diagnosticProvider.workspaceDiagnostics = false, then diagnostics provided by textDocument/diagnostic are cleared on file close (as added in Fixes #1013: Document diagnostics report not used when text document closed #1020)
• If ServerCapabilities.diagnosticProvider.workspaceDiagnostics = true, then textDocument/diagnostics are not cleared on file close.

My expectation would be that the same thing would happen regardless of whether workspaceDiagnostics capability has been enabled or not by the server.

The way the client currently handles workspace vs. document diagnostics generally is not an issue (so no flickering), it is just the inconsistency when a file becomes inactive.
AIUI, the library doesn't need any concept of ignored files. I would think that, once a file becomes inactive, the document diagnostics are discarded, and the client falls back to showing any workspace diagnostics provided - which is what workspaceDiagnostics = false does.

[dbaeumer]

There is actually more to it: when workspace diagnostics are enabled the client still send document diagnostic request to let the server know which files have priority. The assumption behind that is that workspace diagnostics is a super set of document diagnostics which is not true for your scenario. This is why this needs more work than simply clearing the diagnostics.

[pisv]

@JohnnyMorganz When trying to understand the issue, I have noticed the following comment made by @dbaeumer in #1013 (comment):

if the server supports workspace diagnostics then leave the diagnostics as is. The one from the workspace diagnostics will then catchup with any changes.

Could not, then, your problem be solved by explicitly reporting an empty list of workspace diagnostics for the ignored files (regardless whether they are open or not)?

It seems that, since document pull results are favoured over workspace pull results by the client, the document diagnostics will still be shown for the ignored files in the open editors, but when an ignored file gets closed, the (empty) workspace diagnostics will then replace the document diagnostics for the file.

[JohnnyMorganz]

Aha @pisv, thank you, this seems to work!
Originally, I didn't include the ignored files in the workspace report, but instead if you include the file but with empty diagnostics, it will correctly clear.

It won't work immediately though, there is one thing where you need workspace/diagnostic to refresh when the file is closed. So to handle this, with partial result support, on textDocument/close event I send a progress with an empty report for the now-closed file, and the diagnostics clear!

[pisv]

@JohnnyMorganz, great to hear that!

So, IIUC, the additional problem is that with workspace diagnostics there might be a delay in refreshing, but you would like the diagnostics for an ignored file be cleared immediately after the file is closed. Therefore, if the workspace diagnostics request with partial result support is still open at the moment the file is closed, you just add a partial result with empty diagnostics for the now-closed file, right?

[JohnnyMorganz]

Yep, that is correct. Since I keep the request open for as long as possible, I can do a partial request update on the close event.

[Not entirely sure if this is an antipattern though, since I'm essentially going back to pushing a diagnostic update from the server (except doing it using a partial response to a workspace pull diagnostic request). Also textDocument/didClose doesn't necessarily equate to a user-visible change in open documents, but it is sufficient for this case]

This works well for my use case, so I am happy to close this issue if there is nothing else that needs doing

[pisv]

As far as I can tell, your implementation is in line with the specification:

In contrast to the document diagnostic request the workspace request can be long running and is not bound to a specific workspace or document state. If the client supports streaming for the workspace diagnostic pull it is legal to provide a document diagnostic report multiple times for the same document URI. The last one reported will win over previous reports.

and

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.

However, I'm also a bit puzzled, because now there seems to be two ways of doing essentially the same thing -- streaming diagnostics from the server to the client -- by textDocument/publishDiagnostics notifications or by partial result progress for the workspace/diagnostic request. I wonder what is the relationship between them. Are they supposed to be mutually exclusive? Should the latter be preferred to the former by a server as a newer way of doing things?

I also think that this issue may be closed. There seems to be no need for changes in the client, because the problem can (should?) be solved on the server side, since the language server has complete control over workspace diagnostics.

@dbaeumer, can you please share your thoughts on this?

[dbaeumer]

The benefit comes with pull on documents. The old push model had no knowledge about which files to priorities. Hence it could easily happen that you saw stale errors for some time. The pull model allows to combine a long running pull with single document pulls hence knowing which once to prioritize.

The solution you implemented is in line with the specification. I will close the issue.

[pisv]

@dbaeumer Thank you for the clarification!