x/tools/gopls: define approach for returning errors to client

[ianthehat]

We need to think about what the rules for error handling should be in gopls, specifically what kinds of things result in rpc errors vs just logging.
For instance:

• errors in producing diagnostics, which happen without the user requesting them, cannot result in an rpc error but should not be invisible. Should they appear as a diagnostic, or just be logged to the client console?
• errors in formatting, should they fail the rpc or just result in no edits?
• errors in attempting to generate code actions, which happen inside an rpc but without the user asking for them, should they fail the rpc or just log to the client console?

I am currently looking into using xerrors extensively through gopls to both improve the error messages, give us call traces, and also give us the type information to make decisions in the lsp layer about things that happened way down inside the source layer.
Once that is done it will get much easier to think about and experiment with the rules, but we should probably have some kind of overall logic we apply to make these decision.

[myitcv]

FWIW my answers would be:

errors in producing diagnostics, which happen without the user requesting them, cannot result in an rpc error but should not be invisible. Should they appear as a diagnostic, or just be logged to the client console?

Appear as a diagnostic.

errors in formatting because the file/range has syntax errors, should they fail the rpc or just result in no edits?

Just result in no edits.

[stamblerre]

I agree about diagnostics, but @myitcv, I think we disagree about formatting. I think it's on the client side to make the decision of how to present formatting errors. If we respond with no edits in the case of both a correctly formatted file and a broken file that cannot be formatted, we are not giving the client all of the information it needs. As a user, I know that I would appreciate an error message if I keep trying to format a file that has a syntax error that maybe I didn't notice.

[myitcv]

I for one am not clear on where the line between errors and diagnostics starts/ends, so please to continue to consider my answers in that light.

If I have a syntax error in my file, should I expect to see a diagnostics about that? I think the answer is, yes, diagnostics are reported to the client (not least because that's what I'm seeing today). So, assuming the user is looking at those diagnostics, I think it's fair for the server to return no edits for a file with syntax error: there is nothing that can be done, the user needs to fix the syntax error first.

Otherwise, we would need to start encoding detail in the error returned to the client, detail that is already available in the diagnostics.

Or am I missing something?

[stamblerre]

I just don't think that we can rely on diagnostics for this. VSCode already has a configuration to turn off diagnostics from the language server, so we have no guarantee that just because a user is asking the server for formatting, that the user is also seeing our diagnostics.

Additionally, even though I recently disabled this, I think we should do our best to format files with syntax errors (even if imperfectly), so then that case will become ambiguous. In the case completion or signature help, an error means that we have no results to show the user - so it's fine if we just reply with no results. But with formatting, no results has multiple meanings - either my file is already perfectly formatted or my file is totally broken, and if I've disabled diagnostics, or even if I'm not paying attention and I didn't notice a red squiggle, I can't distinguish between the two.

To be honest, I think I'd prefer to send all of the errors and let the client decide. It'd be nice if we could differentiate between a fundamental failure and an error that could be user-facing. I'm imagining a scenario where you made a typo and are trying to complete "fm." instead of "fmt." -- it might be nice to get a popup that "fm" is undefined, even in addition to the diagnostic.

[myitcv]

VSCode already has a configuration to turn off diagnostics from the language server, so we have no guarantee that just because a user is asking the server for formatting, that the user is also seeing our diagnostics.

But didn't you also mention (on our call?) that VSCode ignores errors from calls to the server?

if I've disabled diagnostics, or even if I'm not paying attention and I didn't notice a red squiggle, I can't distinguish between the two.

Good point. But at this point it feels like we're about to start defining an entirely new error mechanism with structure being added to the errors returned by gopls. Unless structured errors (that are above JSON RPC in the "stack") already form part of the LSP spec?

Even if VSCode didn't ignore these errors (assuming my recollection above is correct), it's presumably not going to be able to do anything more than print the string version of the structured error. i.e. there is no logic to "do X in case of structure Y"?

The ResponseError type looks fairly low-level. But again, yours is the greater experience with the LSP spec here so I'll defer 😄

[stamblerre]

I updated https://github.com/golang/go/wiki/gopls-integrator-FAQ#rpc-response-errors to reflect the current state of affairs regarding this issue. Things seem to have stabilized a bit, so I am thinking that I will close this issue for now, and we can re-open discussion on a case-by-case basis as new problems come up.