Clear diagnostics for non-project files

[ulugbekna]

When one opens a source file of a library they use (with go-to-definition, for example), merlin reports errors in that file. When the file is closed, the errors remain in the problems pane (see the screenshot). This PR clears the reported errors when such a non-project source is closed. I define non-project source file as a file with URI that contains ".opam", "_opam", or "_esy".

[ulugbekna]

MacOS tests are failing for me locally but even with master -- not sure why.

[rgrinberg]

When one opens a source file of a library they use (with go-to-definition, for example), merlin reports errors in that file. When the file is closed, the errors remain in the problems pane (see the screenshot)

That's the bigger problem I think. When a document is closed we should republish the diagnostics without the closed file.

[ulugbekna]

When a document is closed we should republish the diagnostics without the closed file.

I am not sure about this because having errors for project files to persist after they are closed:

• helps to see that there are still errors that may affect the other files that the user switch to work on (for example, to keep a note to come back to that file to fix the issues -- vscode marks filenames red of files that contain errors)
• signals that the build will likely fail
• is "recommended" by lsp spec:

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

[rgrinberg]

The spec recommendation certainly makes sense if diagnostics are project wide, but our diagnostics are local to a single compilation unit. Let's consider what it would take for the user to clear out stale diagnostics: he would have to open every stale file and recompute them. That seems like a poor experience to me, and that is not how merlin did diagnostics for vim and emacs in its traditional front end.

We will have project wide diagnostics in the future, but they will come from dune rather than merlin. In this case, it would make sense to keep them lingering around. That's because we'll know for sure they're applicable, and we'll be able to clean them up automatically once they are stale.

[ulugbekna]

Okay, let's clear diagnostics on close. Someone will probably create an issue if they miss the previous behavior for project files. Thanks!

…

On Sun, 25 Oct 2020 at 21:07, Rudi Grinberg ***@***.***> wrote: The spec recommendation certainly makes sense if diagnostics are project wide, but our diagnostics are local to a single compilation unit. Let's consider what it would take for the user to clear out stale diagnostics: he would have to open every stale file and recompute them. That seems like a poor experience to me, and that is not how merlin did diagnostics for vim and emacs in its traditional front end. We will have project wide diagnostics in the future, but they will come from dune rather than merlin. In this case, it would make sense to keep them lingering around. That's because we'll know for sure they're applicable, and we'll be able to clean them up automatically once they are stale. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#271 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AD4YR65WKL7LRFPYXZIPZ63SMSARXANCNFSM4S5U4RIQ> .

[rgrinberg]

I must have misread the diff in the web UI. Code looks good. Some minor adjustments:

• I got rid of @@. It's confusing to read the extend of the functions it applies
• I fixed the code to wait for canceling the document on close
• I moved more code outside of the merlin thread. We should only run code that modifies the typechecker state in here.
• I added a CHANGES entry.