Spec says "diagnostics are cleared by the server when the file is closed" but didOpen/didClose may not match what the user sees

[DanTup]

My understanding from this text:

As with the open notification the close notification is about managing the document's content. Receiving a close notification doesn't mean that the document was open in an editor before. A close notification requires a previous open notification to be sent. Note that a server's ability to fulfill requests is independent of whether a text document is open or closed.

and similar comments is that servers cannot assume that the textDocument/onDidClose means a document was just closed. Nor can it assume that onDidOpen actually relates to a document being open.

However, the spec also says this:

Diagnostics notification are sent from the server to the client to signal results of validation runs.

Diagnostics are “owned” by the server so it is the server’s responsibility to clear them if necessary. The following rule is used for VS Code servers that generate diagnostics:

• if a language is single file only (for example HTML) then diagnostics are cleared by the server when the file is closed.
• if a language has a project system (for example C#) diagnostics are not cleared when a file closes. When a project is opened all diagnostics for all files are recomputed (or read from a cache).

This two things appear to conflict. If a server is never able to tell when a file is (in the users opinion) opened or closed, it seems like the single-file mode cannot be implemented (or at least, the experience may result in errors appearing for files the user has not opened, and not disappearing after they close them).

Interestingly - I reviewed the TypeScript code because it seems to be clearing errors when I close files - and found that it's actually using the VS Code close event as a signal to hide the errors:

https://github.com/microsoft/vscode/blob/e41c19505142614f706ad853bd5c4c2ad0f74e89/extensions/typescript-language-features/src/features/bufferSyncSupport.ts#L500-L502

https://github.com/microsoft/vscode/blob/3841f78377fc02ab5ef514cbd0fa760ed4ac6f0d/extensions/typescript-language-features/src/typescriptServiceClient.ts#L159

It's not clear what should be done here. In Dart we're trying to provide some errors only when files are open (which the LSP spec describes, and the VS Code TS extension does), but it's been suggested (multiple times) that we should not assume this behaviour.

Is it possibly to clarify how the "single file mode" described above is intended to be implemented (if indeed, it should)? Thanks!

[kjeremy]

I'm running into this too. In addition when our server gets the didClose notification we immediately send out a publishDiagnostics for that file with an empty version. Should that version match the version number at the time of the close?

[dbaeumer]

Actually this has nothing to do with the spec. It is more an implementation problem in VS Code and the VS Code LSP libs. I am moving the issue.

[dbaeumer]

@kjeremy if you leave the version empty it will clear the errors for the current version of the file known on the client side.

To implement this correctly in VS Code (in terms what is visible to the user) we would need API from VS Code to get access to the tab model. Then by default we can filter non visual opens and allow clients in the middleware to override that default behavior.

[DanTup]

Actually this has nothing to do with the spec. It is more an implementation problem in VS Code and the VS Code LSP libs. I am moving the issue.

But right now the spec suggests the close event is not tied to the user closing the document (see quotes at the top of this issue). Based on the spec, even if VS Code gets a tab model, it's still valid for it not to send a Close event until minutes after the user closes the editor.

So I think the spec still needs updating. Either it should say that open/close do match what the user sees, or there should be other events. Otherwise this seems like it still will not work as expected.

[dbaeumer]

The open/close events (although named open /close) are ownership events no visual events. They say that an open transfers the ownership of the document to the client and a close back to the server. I see the point that in systems that have single file validation the current model is not sufficient. But IMO the solution can't be to tweak these ownership events. I also think that even a tab model might not help. An editor could decide to show content of a file in something different than a tab (e.g. an editable outline where you can rearrange methods using D&D).

The reason why I think a simple sentence in the spec will not help is: it doesn't fix the problem. This is why I am reluctant to write something. IMO we first need to understand what a better solution would be. The best I could come up with so far is a pull model for diagnostics since it will allow the client to pull depending on what it presents.

I will write a sentence in the spec that the open / close events do not necessarily reflect what the user sees in the UI.

[dbaeumer]

Added that sentence:

Please note that open / close events don't necessarily reflect what the user sees in the user interface. These events are ownership events. So with the current version of the specification it is possible that problems are not cleared although the file is not visible in the user interface since the client has not closed the file yet.

[DanTup]

@dbaeumer I agree with everything you said, though is the new note on the open/close event or in PublishDiagnostics? I guess the issue is that the text under PublishDiagnostics that says "diagnostics are cleared by the server when the file is closed" is a little misleading and maybe should also call it out and/or link to that new note:

diagnostics are cleared by the server when the files textDocument/didClose notification is received ([see note about close events](#link))

[dbaeumer]

The test I added is under PublishDiagnostics Notification

[DanTup]

Ah yes, I see it now. Much clearer - thanks!