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

foldingRange: clarify the meaning of [] vs null responses #1200

Closed
hyangah opened this issue Feb 13, 2021 · 8 comments
Closed

foldingRange: clarify the meaning of [] vs null responses #1200

hyangah opened this issue Feb 13, 2021 · 8 comments
Milestone

Comments

@hyangah
Copy link

hyangah commented Feb 13, 2021

According to the spec can return FoldingRange[] | null, but it does not explicitly mention whether [] (empty FoldingRange) and null are semantically distinct yet.

Empirically, I am guessing [] means there is no folding range (so editor may remove all existing folding ranges) while null translates into provideFoldingRanges returning null or undefined (so editor will not use the info from the language server) . It would be nice if the spec explicitly clarifies the semantic differences.

And, VS Code question while we are here: if a language server responds with 'null' or an error, what does VS Code do - remove all the folding ranges from the previous responses, or keep the folding range info from the previous successful, non-null response until the next successful response is received?

@dbaeumer
Copy link
Member

In general null is always equal to []. There is currently no support in LSP that a server can say: I am busy and can't generate a result so please ask me later again. This would need to be expressed differently than using null

@dbaeumer dbaeumer added this to the 3.17 milestone Feb 15, 2021
@hyangah
Copy link
Author

hyangah commented Feb 15, 2021

Thanks!

We are currently trying to deal with folding range info disappearing because the go language server returns a partial or an empty response as a result of temporary parsing issues. I am proposing to return an error as a workaround if the language server cannot compute the complete folding range at the moment. Until the future LSP supports the "I am busy and can't generate a result" mode response. That may create noise to clients that treat every error as significant, but at least in the vscode node client, this doesn't seem too bad. Does this sound reasonable?

@rwols
Copy link
Contributor

rwols commented Feb 15, 2021

Would client-initiated progress reporting solve your problem of "I am busy and can't generate a result" ?

@hyangah
Copy link
Author

hyangah commented Feb 15, 2021

@rwols In our case, "The source code is not in the state that I can work with yet. Next time after user edits more I may be able to participate in the folding range computation" may be more precise description of the condition.

In case of VS Code, I see the client (vscode) issue folding range requests for almost every key stroke, so progress reporting doesn't seem like a good fit for it. :-(

@dbaeumer
Copy link
Member

Resolving this using an error is the right approach right now. We started to support Busy please ask again for semantic tokens by allowing returning a special cancel error response that will retrigger the computation.

@dbaeumer
Copy link
Member

What you can do right now is the following:

  • use the middle ware on the client to intercept the folding result range
  • interpret the result
  • retrigger the request if you know you server can handle it know
  • return the result from the retrigger.

@hyangah
Copy link
Author

hyangah commented Feb 17, 2021

Thanks @dbaeumer!

The middleware approach is interesting - does it imply we (the extension) are responsible to hold the request until we have the correct answer? In our specific case, knowing whether the server is ready to handle the request is hard because the root cause is not the overload. The code is in a state where the server can't work with nicely. The only possible resolution now is the user edits the code.

I observed that VS Code keep issuing foldingRange requests as the user edits the code. I didn't try, but if the middleware holds the request for retry, I am guessing VS Code would cancel the previous outstanding foldingRange request before sending another foldingRange request for the new version of the code. If that's the case, isn't the middleware logic for hold&retrigger redundant?

@dbaeumer
Copy link
Member

@hyangah yes, VS Code (and all other LSP clients should) cancels a request for which the result is not needed anymore. If your server can handle that correctly you don't need to have a middleware. However you can in the middleware detect if the request is canceled as well and react accordingly

gopherbot pushed a commit to golang/tools that referenced this issue Mar 8, 2021
When parse errors occur, go's parse package cannot recover nicely.
gopls tried to compute folding ranges based on the partial info
in this case, but returning partial folding range info confuses
editors (vscode) and results in dropping previous folding range
info from the region after the parse error location.

This CL makes gopls not to return anything - so the editor can
tell the result is not believable and ignore it.

The ideal solution is to return a response explicitly surfacing
this case, but currently LSP (3.16, as of today) does not have
a way to describe this condition. See the discussion in
microsoft/language-server-protocol#1200.

We also tried to make gopls return an error. While it worked
nicely in VSCode, we are not sure about how other editors handle
errors from foldingRange. So, instead, we just let gopls return
an empty result - since foldingRange is already broken in this
case, we hope it doesn't add a lot of noise to existing users.

VSCode Go will check the response from the middleware. If the
response is empty but the file is not empty, VSCode Go will
ignore the response.
(https://go-review.googlesource.com/c/vscode-go/+/299569)

Updates golang/vscode-go#1224
Updates golang/go#41281

Change-Id: I917d6667508aabbca1906137eb5e21a97a6cfdaf
Reviewed-on: https://go-review.googlesource.com/c/tools/+/291569
Trust: Hyang-Ah Hana Kim <hyangah@gmail.com>
Run-TryBot: Hyang-Ah Hana Kim <hyangah@gmail.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants