Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
ianthehat opened this issue Apr 17, 2019 · 6 comments
Closed

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

ianthehat opened this issue Apr 17, 2019 · 6 comments
Labels
FeatureRequest FrozenDueToAge gopls Issues related to the Go language server, gopls. NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@ianthehat
Copy link

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.

@ianthehat ianthehat added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. FeatureRequest gopls Issues related to the Go language server, gopls. labels Apr 17, 2019
@ianthehat ianthehat self-assigned this Apr 17, 2019
@stamblerre stamblerre changed the title s/tools/internal/lsp: error handling x/tools/internal/lsp: error handling Apr 17, 2019
@gopherbot gopherbot added this to the Unreleased milestone Apr 17, 2019
@myitcv
Copy link
Member

myitcv commented Apr 18, 2019

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
Copy link
Contributor

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
Copy link
Member

myitcv commented Apr 18, 2019

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
Copy link
Contributor

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
Copy link
Member

myitcv commented Apr 19, 2019

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 stamblerre changed the title x/tools/internal/lsp: error handling x/tools/internal/lsp: define approach for returning errors to client Jun 5, 2019
@stamblerre stamblerre changed the title x/tools/internal/lsp: define approach for returning errors to client x/tools/gopls: define approach for returning errors to client Jul 2, 2019
@stamblerre
Copy link
Contributor

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.

@golang golang locked and limited conversation to collaborators Aug 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FeatureRequest FrozenDueToAge gopls Issues related to the Go language server, gopls. NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

4 participants