x/tools/gopls: staticcheck QF1003 quickfix produces corrupted textedit

[tttoad]

gopls version

Build info

golang.org/x/tools/gopls v0.14.1 golang.org/x/tools/gopls@v0.14.1 h1:XaTETpi7Q67XO8nftquJitcx+9c2bPclO8Kz2sBVvec= github.com/BurntSushi/toml@v1.2.1 h1:9F2/+DoOYIOksmaJFPw1tGFy1eDnIJXg+UHjuD8lTak= github.com/google/go-cmp@v0.5.9 h1:O2Tfq5qg4qc4AmwVlvv0oLiVAGB7enBSJ2x2DqQFi38= github.com/sergi/go-diff@v1.1.0 h1:we8PVUC3FE2uYfodKH/nBHMSetSfHDR6scGdBi+erh0= golang.org/x/exp/typeparams@v0.0.0-20221212164502-fae10dda9338 h1:2O2DON6y3XMJiQRAS1UWU+54aec2uopH3x7MAiqGW 6Y= golang.org/x/mod@v0.13.0 h1:I/DsJXRlw/8l/0c24sM9yb0T4z9liZTduXvdAWYiysY= golang.org/x/sync@v0.4.0 h1:zxkM55ReGkDlKSM+Fu41A+zmbZuaPVbGMzvvdUPznYQ= golang.org/x/sys@v0.13.0 h1:Af8nKPmuFypiUBjVoU9V20FiaFXOcuZI21p0ycVYYGE= golang.org/x/telemetry@v0.0.0-20231011160506-788d5629a052 h1:1baVNneD/IRxmu8JQdBuki78zUqBtZxq8smZXQj0X2Y= golang.org/x/text@v0.13.0 h1:ablQoSUd0tRdKxZewP80B+BaqeKJuVhuRxj/dkrun3k= golang.org/x/tools@v0.14.1-0.20231026192422-8b5abd452b28 h1:5YgdZAe2w0x3Xrjv0+GXrI0jvm7qCQK/ySGFfiEHMfU= golang.org/x/vuln@v1.0.1 h1:KUas02EjQK5LTuIx1OylBQdKKZ9jeugs+HiqO5HormU= honnef.co/go/tools@v0.4.5 h1:YGD4H+SuIOOqsyoLOpZDWcieM28W47/zRO7f+9V3nvo= mvdan.cc/gofumpt@v0.4.0 h1:JVf4NN1mIpHogBj7ABpgOyZc65/UUOkKQFkoURsz4MM= mvdan.cc/xurls/v2@v2.4.0 h1:tzxjVAj+wSBmDcF6zBB7/myTy3gX9xvi8Tyr28AuQgc= go: go1.20.7

go env

GO111MODULE="" GOARCH="arm64" GOBIN="" GOCACHE="/Users/toad/Library/Caches/go-build" GOENV="/Users/toad/Library/Application Support/go/env" GOEXE="" GOEXPERIMENT="" GOFLAGS="" GOHOSTARCH="arm64" GOHOSTOS="darwin" GOINSECURE="" GOMODCACHE="/Users/toad/work/pkg/mod" GONOPROXY="" GONOSUMDB="" GOOS="darwin" GOPATH="/Users/toad/work" GOPRIVATE="" GOPROXY="https://goproxy.io,direct" GOROOT="/Users/toad/go/go1.20.7" GOSUMDB="sum.golang.org" GOTMPDIR="" GOTOOLDIR="/Users/toad/go/go1.20.7/pkg/tool/darwin_arm64" GOVCS="" GOVERSION="go1.20.7" GCCGO="gccgo" AR="ar" CC="clang" CXX="clang++" CGO_ENABLED="1" GOMOD="/Users/toad/work/demo5/go.mod" GOWORK="" CGO_CFLAGS="-O2 -g" CGO_CPPFLAGS="" CGO_CXXFLAGS="-O2 -g" CGO_FFLAGS="-O2 -g" CGO_LDFLAGS="-O2 -g" PKG_CONFIG="pkg-config" GOGCCFLAGS="-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-pre fix-map=/var/folders/g1/tgmnlrdn3vxgv08kdgh9vpkw0000gn/T/go-build1758453573=/tmp/go-build -gno-record-gcc-switc hes -fno-common"

What did you do?

https://github.com/golang/go/assets/14021602/5c47c32c-c56d-416c-bf32-2b4b8e3056d7

What did you expect to see?

https://github.com/golang/go/assets/14021602/243dcbf8-f4f7-4559-b848-2c8ff2c902be

I found the problem on "honnef.co/go/tools" and I will commit PR to that repository for fixing the problem.

[hyangah]

Thanks for the report. This functionality comes from staticcheck. Can you please open an issue in the staticcheck issue tracker, and once fixed, we can update gopls's dependency. cc @dominikh

[dominikh]

@hyangah Are we sure this isn't an issue with how gopls applies edits?

For the code in the OP, Staticcheck emits 6 code edits. The first 5 replace if <cond> { with case <cond>: and delete the closing } of the block. The sixth edit inserts switch <x> {\n in front of the first if keyword. I admit that this order of edits is non-trivial, but IMO it should work fine, assuming correct tracking of how positions change due to insertions and deletions.

The go/analysis docs do say that

Diagnostics should not contain SuggestedFixes that overlap.

but I'm not sure that replace 0..10 with text and insert text at 0 can be considered to be overlapping.

[hyangah]

Thanks @dominikh

@findleyr @adonovan Can you please take a look whether the quick fix translation gopls is doing is sane? EDIT: ~I could reproduce it~ I cannot reproduce it any more. :-(

@tttoad You said you already found an issue in honnef.co/go/tools. Can you share the PR you have?

package foo

import "fmt"

func fn() {
    x := 0
    if x == 1 || x == 2 {
    } else if x == 3 {
        fmt.Println(x)
    } else {
        fmt.Println(x)
    }
}

[dominikh]

@hyangah the proposed PR is https://github.com/dominikh/go-tools/pull/1466 — it merges the first and last edit into one.

[findleyr]

@dominikh for LSP edits it looks like inserts must precede replace edits, when all start at the same position: https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textEditArray.

We're naively translating go/analysis edits to LSP edits, and so that could explain the breakage.

Probably we should keep the edit semantics for go/analysis aligned with LSP, and so should clarify the go/analysis documentation for this case.

[dominikh]

@findleyr can you test if https://github.com/dominikh/go-tools/commit/4136a8489ea1bb8745c75158e04f5e1c9708c328 fixes the issue? This should respect the order required by the spec.

[tttoad]

@hyangah https://github.com/dominikh/go-tools/pull/1466. It works on my mac.

[tttoad]

@findleyr @hyangah Any progress on the issue?

[findleyr]

@tttoad thanks for testing -- I haven't yet had an opportunity to test for myself.

@tttoad what progress are you expecting? I think the correct path forward is to update staticcheck. This will take some time.

[dominikh]

There are currently two competing fixes for the issue in Staticcheck. @tttoad's PR, that combines some of the edits, and my change that attempts to fix the issue by reordering edits.

I'm waiting to hear if my change fixes the issue before merging it to master (or before merging tttoad's PR.)

[findleyr]

@dominikh thanks for clarifying.

It will likely take a few days before I can confirm. But I will do so.

@adonovan should we update the go/analysis documentation?

[adonovan]

LSP ... inserts must precede replace edits, when all start at the same position We're naively translating go/analysis edits to LSP edits, and so that could explain the breakage. Probably we should keep the edit semantics for go/analysis aligned with LSP, and so should clarify the go/analysis documentation for this case.

That new requirement would represent a breaking change to the analysis API. The analysis API's specification is clear enough; let's just take more care in the logic that translates edits to LSP.

(Why does the LSP have that restriction? There's an unambiguous interpretation of any list of non-overlapping edits.)

[adonovan]

There are at least a dozen places in gopls that loop over TextEdits, typically converting one form to another (e.g. protocol, analysis, gob). In many cases it's locally obvious that the edits are already ordered (e.g. because they came directly from diff.Strings), but it seems like a thorough audit is necessary. I think source.ApplyFix is the most obvious place where the invariant needs to be considered (and perhaps also lsp.refactorRewrite).

It would be nice if we could simplify those (at least a dozen) loops at the same time, and perhaps eliminate some of the conversions.

[findleyr]

Sorry it took so long, but I tested https://github.com/dominikh/go-tools/commit/4136a8489ea1bb8745c75158e04f5e1c9708c328 and confirmed that it fixes the issue. (the resulting code is not gofmt'ed, but that's OK).

[dominikh]

Alright, thanks for confirming. I've pushed a version of that commit to Staticcheck's master, or you can cherry-pick the old version of that commit on top of the latest release (or you can wait until the next Staticcheck release, of course.)

[findleyr]

Thanks. I think we'll wait for the next staticcheck release, as this has existed for a while. (EDIT: milestoning for v0.16.0 (N+2), as a reminder to update staticcheck and add a test).

[adonovan]

I just confirmed empirically that both Emacs+eglot and VS Code are able to correctly apply a SuggestedFix whose TextEdits are out of order, so long as they are non-overlapping and non-adjacent.

I also confirmed that VS Code does the right thing for an insert of "A" at point p followed by a replacement of (p-1, p) by "B": these edits adjoin but do not overlap; the result is "BA". But Emacs+eglot misapplies the edits and yields "AB".

Both of these cases are consistent with the LSP spec wording, so I think Eglot has bug, but VS Code does not, and nor does staticcheck.