Still shows diagnostics on closed files

[reagle]

As first documented in sublimelsp/LSP#1925, when I close a file, its diagnostics continue to pollute the diagnostic panel until I exit ST. This unfortunately obscures any problems I might have in the python files that I keep linted. (Similarly, it'd be nice if the diagnostic panel view moved to the start of the diagnostics for the current visible pane/tab.)

Open a file with many diagnostics.
Close the file and the diagnostics persist.
macOS 12.1 21C52 arm64 using ST 4126 with LSP 1.15.0 and LSP-pylsp 2.3.1.

[krassowski]

Indeed the LSP specification says that:

Diagnostics are “owned” by the server so it is the server’s responsibility to clear them if necessary.

For the other part however:

(Similarly, it'd be nice if the diagnostic panel view moved to the start of the diagnostics for the current visible pane/tab.)

I don't think this is the server issue; it does not know anything about the panel; I would recommend going back to sublimelsp with this one.

[reagle]

@krassowski, would you mind responding to @rwols on the LSP issue? I'm just a user that doesn't know what any of this means and don't want to confuse things.

[krassowski]

I don't believe there is anything to respond - the interface behaviour like moving diagnostic panel is just an unrelated issue, and I think you are in much better position to open a new issue about it than myself as I do not use sublime ;)

[ccordoba12]

@krassowski, I don't understand what we should do to clear diagnostics on our side. Perhaps cancelling all pending requests for a file after it's closed?

[krassowski]

My understanding is that when we receive textDocument/didClose from the client we should just send a textDocument/publishDiagnostics notification with an empty array like: { uri: 'current-uri', 'diagnostics: [] }

[krassowski]

This is a similar pattern to the server notifying the client about the need to close signature hover by sending null (which we do not implement either).

[krassowski]

Looking further into this, the specification does not have a detailed normative text on it, just an example of how VSCode handles it:

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. 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.
• 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).

My understanding is that the differences are on client level: if client supports "projects", it will "open" all files with the server and not send the close notification (unless file is deleted); if the file is not part of the project, it the client will only open the file when it actually gets open and send "close" notification as soon as the files is actually closed in the user interface. This seems to be to allow to show diagnostics for all files even if only one or few are actually open in the main editing area (e.g. to highlight files with errors).

[rchl]

if client supports "projects", it will "open" all files with the server and not send the close notification (unless file is deleted);

Haven't seen such behavior in any client. Especially not in VSCode which can be seen as a "reference" implementation.

Some servers just clear the diagnostics after textDocument/didClose and some don't.

As far as I understand it, the servers can be divided into those that handle "projects" (for example clang) and those that don't have a concept of projects and work on individual files instead (for example JSON server). So then it's the choice of the server whether it wants to clear the diagnostics on closing a file or not but with that in mind, I would expect the JSON server to clear the diagnostics on closing a file while I would not necessarily expect the clang server to do that.

But in the end it's still a choice the server has to make and I'm not sure which behavior is more correct for the pylsp server. It's not unlikely that changing the behavior will result in some other person complaining that he/she liked the previous behavior.

[ccordoba12]

But in the end it's still a choice the server has to make and I'm not sure which behavior is more correct for the pylsp server.

Do you depend on the server clearing diagnostics to clear them also from the Sublime panel that shows them? If that's the case, we can make the change here (even if not mandatory) to make things easier for you.

I agree that perhaps other clients don't expect that, but (to me) it's a simple change so they could adapt to it.

[rchl]

Yes, the ST panel only shows the diagnostics that the server has reported and hasn't cleared. But I don't think this is in any way specific to ST. VSCode would do the same in its Problems panel.

[reagle]

It's not unlikely that changing the behavior will result in some other person complaining that he/she liked the previous behavior.

I don't anything is ever cleared presently, so I don't think there'd be complaints between file/project people in any case. If things were cleared, I'd prefer the diagnostics only show for currently open files.

[rchl]

It's not unlikely that changing the behavior will result in some other person complaining that he/she liked the previous behavior.

I don't anything is ever cleared presently, so I don't think there'd be complaints between file/project people in any case. If things were cleared, I'd prefer the diagnostics only show for currently open files.

Not sure what you mean.

pylsp diagnostics of closed files are not currently cleared in ST but that's because pylsp doesn't clear them. If it would then ST would clear them also.

[jwortmann]

But in the end it's still a choice the server has to make and I'm not sure which behavior is more correct for the pylsp server. It's not unlikely that changing the behavior will result in some other person complaining that he/she liked the previous behavior.

FWIW, https://github.com/microsoft/pyright for example has a setting for this:

python.analysis.diagnosticMode ["openFilesOnly", "workspace"]: Determines whether pyright analyzes (and reports errors for) all files in the workspace, as indicated by the config file. If this option is set to "openFilesOnly", pyright analyzes only open files.

If a server chooses not to clear diagnostics after textDocument/didClose, then I would expected that the server will report diagnostics for all applicable files which it can find in the workspace / project after it initializes, not just for the files which were opened in the editor. Indeed, pyright behaves exactly like this when the diagnosticMode is set to "workspace".

[ccordoba12]

Hey @reagle, @rchl and @jwortmann, I created PR #165 to fix this. Could you apply that small change to your local installations of this project and check if it solves the problem for you?

I'm almost sure it will, but I'd just like your help to be extra confident about it.

[reagle]

Thanks for doing this! This is beyond me at the moment, though, as I know nothing about LSP other than it is a package that is installed. Hopefully someone else can test it.

[rchl]

I've got around to testing it and it works as advertised.

[ccordoba12]

Great! Thanks for your help with that @rchl.