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?

2023-11-03.18.33.31.mov

What did you expect to see?

2023-11-03.18.18.41.mov

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 dominikh/go-tools#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.