lsp: Where to put errors of imported files?

[flofriday]

While reviewing the recent LSP PR we came across this issue (here is my comment).

At the moment we put all the errors of the importee (files we import) also in the importer (current file), at the position of the first character in the file.
In practise I think this reads poorly:

Screenshot from IDE:

In most cases the importer isn't responsible for such errors and as such I don't think they need to be shown in the importer.

Nuances in OpenVADL:
However, you can break a file by they way you import it. In that case the importee is correct on it's own but by how you imported it is no longer valid:

While this is possible it 1) is not that common, I had to think quite hard to come up with an example that behaves like that and 2) the error shown still doesn't really help the user and they still have to invoke the compiler by hand.

Suggestion:
I think, we should only show the errors in the files we are in. In the obscure cases that the importer brakes the importee I think it will be fine if only a actual compilation will show that.

This is also what other LSPs do like rusts amazing rust-analyzer:

@AndreasKrall do you have any thoughts on this.

[jaraonthe-dot-net]

A little clarification:

• The error at the top of the importer doesn't show the full error that occurred in the imported file. Only a short notice about in which file an error occurred (as shown in your second screenshot).

• The intention is the following: Assuming an error in an imported file interrupts the parsing procedure (e.g. a simple syntax error should already do that), then you wouldn't get any errors reported in the importer (that the user is currently looking at in their IDE) because the Parser doesn't get that far. Even if you expect an error to be shown. I think that might be very confusing for the user, and that's why there's this notice at the top of the file.

• Further (at least in VSCode), if a file hasn't been opened in the editor, it will not be marked as having errors (on the editor tab nor in the file tree), so unless the user manually opens all possibly relevant .vadl files, they won't know where there is something wrong. The notice is supposed to pinpoint them in the right direction.

• Your second case, where the importee itself is fine, only producing an error when parsed via the importer, is an edge case that was never considered for this mechanism.

I'm aware that this is a clunky solution; it is a quick fix/safeguard to work around the technical limitations of the parser. I'm happy to drop it if you want, or to improve it if you have any suggestions how.

[flofriday]

Assuming an error in an imported file interrupts the parsing procedure [...], then you wouldn't get any errors reported in the importer [...] because the Parser doesn't get that far.

I wasn't aware of that behavior, seems like a relict of a past time when compilation aborted at the first error. I've created #999 to address that.

Further [...] if a file hasn't been opened in the editor, it will not be marked as having errors

That's how many Language Servers work. They help you while you write and navigate the code but it isn't a tool to give you a health check across all files in a project. (Mostly because that's quite expensive). For that you simply have to revert to a full compilation.

However it also could be that this is a bug in the current Langauage Server because textDocument/publishDiagnostics should only be used to report errors in the currently openend files while workspace/diagnostic should be used to report diagnostics in the complete project including unopened files.

Since OpenVADL (like Python) doesn't have an obvious project structure, or entry points / main file and it depends on the developer to define it with each compilation, I think it does make much more sense to limit our LSP to the currently opened files.

[jaraonthe-dot-net]

However it also could be that this is a bug in the current Langauage Server because textDocument/publishDiagnostics should only be used to report errors in the currently openend files while workspace/diagnostic should be used to report diagnostics in the complete project including unopened files.

These are part of two different approaches: publishDiagnostics is pushing from the server to client (without being asked for it), and workspace/diagnostic is part of the pull model, where the client asks for diagnostics.

We are using the push approach. Re-reading the docs, it occurs to me that there is nothing prohibiting the server from pushing diagnostics for unopened files. I can try this and see what the client does with this data.

This of course wouldn't change the core issue that no errors are reported for a file if an error occurred in an imported file, so #999 is certainly the right way forward.