x/tools/gopls: line offset out of range in codeAction response #37702
Comments
gopherbot
added
Tools
|
Thank you for filing a gopls issue! Please take a look at the Troubleshooting guide, and make sure that you have provided all of the relevant information here. |
stamblerre
changed the title
|
I tried to
I couldn't check what's going on with the remaining ~100 commits, because, for the minimal repro, they all hit this line and then gopls just dies. |
|
Thanks for the report. This actually looks related to #33717, which I had just closed. This does actually seem like a bug though because here there is never a terminal newline on L2. @ianthehat: This is unexpected behavior, right? I want to confirm with you first because of your comment here: #33717 (comment). |
|
For reference, the way we protect users from this bug is by dropping every code action that has the end of range out of bounds. |
|
I would suggest the approach that sublimelsp took: sublimelsp/LSP#717 - that way users still get the correct formatting and code actions applied. The out of bounds line always seems to be an off-by-one. |
stamblerre
added
the
NeedsInvestigation
|
@stamblerre I'm not sure that's the right approach. I mean in the sense that sublime have (regrettably) now made themselves bug-compatible with VSCode. The only eventual result of that is a de facto standard outside of the actual protocol. I don't feel that it's appropriate for all client authors to have to work around issues in servers just because they don't affect VScode. Unfortunately there seems to be a growing trend amongst language server vendors that the only thing that matters is that it works in vscode, and the protocol is secondary. Of course, that's not compatible with the stated goal of solving the matrix problem. Of course we realise that gopls is in an experimental state and so accept that there are issues, and that everyone has a backlog/prioirities, etc.. So, can can confirm that this behaviour is incorrect and that solving it in gopls is probably the right approach, rather all client authors working around it? If so, then we'll either just wait for a resolution or perhaps do the fix/investigation ourselves. I'd rather spend time doing that than working around it in my client. |
|
@puremourning: I absolutely agree that we should focus on matching the spec rather than the behavior of VS Code. I closed the original issue (#33717) because it was out of date and had a workaround, but this issue does indicate that the edits I suggest using Sublime's workaround in the meantime because it seems like dropping code actions would be a pretty negative experience for the user. |
Emphasis mine. I'm guessing that was a typo.
In my opinion, dropping malformed code actions is a safer bet for client implementers, since it makes fewer assumptions. Specifically, doing a |
|
Is this definitely still a problem? Because given the steps in the initial description I can't reproduce this (using |
|
The bug is definitely there on v0.4.0. I'll give it a try with the git HEAD. |
|
My guess is that this has to do with how editors deal (or don't deal) with non-zero-length files with no |
|
I still see this bug, both on the current HEAD and on c07e33e. The file contents as seen by gopls are in the original post. Yes, the file ends with a new line, but that still doesn't mean there are 3 lines. |
|
Then I think we need to focus on why we are seeing different responses from |
|
Can you post the |
|
@myitcv do you have the full log of messages to-from gopls ? I think Boris posted our logs in the OP |
|
Here's the gopls log: That's with |
|
Here is my log: govim.log |
|
It might make comparison a bit easier if you're able to provide a log from a minimal repo; there appear to be diagnostics sent for a whole load of unrelated files. |
|
Here's the log with a minimal repo. |
|
Change https://golang.org/cl/244762 mentions this issue: |
|
Ok, after bringing myself up to speed on this a bit, I'm not sure I fully understand the problem. Per the spec:
So if we have a file
Also, what are we supposed to specify if we want to append So what exactly is correct here, per the spec? I can't say that what Gopls is doing is definitely correct, but it also doesn't seem to be incontrovertibly wrong. I can do some research to see what other servers do, but thought I'd first comment to see if anyone had additional evidence on hand. |
|
@findleyr - per our offline conversation, from my perspective (FWIW)
You can't - I think it's intended you specify an edit that (in your example) is equivalent to setting the range #38274 and microsoft/language-server-protocol#376 are obviously relevant here. |
|
Thanks for looking into this, @findleyr and @myitcv. I agree that, based on the spec, it seems like |
stamblerre
added
WaitingForInfo
|
I think you’re right. The new line is right there in the message. Embarrassingly we can no longer repro the problem anymore so pls feel free to close with our thanks for your input and support. |
|
Sounds good - glad this has been resolved! |
What version of Go are you using (
go version)?$ go version golang.org/x/tools/gopls v0.3.3 golang.org/x/tools/gopls@(devel)Does this issue reproduce with the latest release?
Version 0.3.3 is the latest gopls version. I haven't tried the latest master, to be honest.
What operating system and processor architecture are you using (
go env)?go envOutputWhat did you do?
mkdir test && cd testgo mod init techo -en 'package main\nimport ( "fmt" )' > main.gomain.goin your favourite editor and make atextDocument/codeActionrequest from cursor position(0, 0).What did you expect to see?
"Organize imports" code action, with valid
Rangeobjects.What did you see instead?
The
"range"keys in the response contain"line":2, which, considering 0-indexing of the protocol, doesn't exist for themain.goin question. This can cause a lot of troubles in some clients.Additional info:
edit:[ { "range": { "start": { "line": 0, "character": 0 }, "end": { "line": 2, "character": 0 } }, "newText": "" }, { "range": { "start": { "line": 2, "character": 0 }, "end": { "line": 2, "character": 0 } }, "newText": "package main" } ]In case the 3rd line exists in the file (actually 1st line after
import- not necessarily 3rd), the line offset of2is valid and doesn't cause issues.For reference, this was noticed during the review of a ycmd pull request.
The text was updated successfully, but these errors were encountered: