x/tools/gopls: test completions duplicates text

[vikblom]

Hi,
I have the same problem as @vadimi reported here: emacs-lsp/lsp-mode#3699

In brief, when editing a test, writing

func TestFoo

suggests the completion

func TestFoo(t *t.Testing)

but when goign through with that completion the actual result is

func TestFoo(t *t.Testing)TestFoo

If my fault tracing is correct, I would like to make a patch for this.

gopls version

Build info
----------
golang.org/x/tools/gopls v0.10.1
    golang.org/x/tools/gopls@v0.10.1 h1:JoHe17pdZ8Vsa24/GUO8iTVTKPh0EOBiWpPop7XJybI=
    github.com/BurntSushi/toml@v1.2.0 h1:Rt8g24XnyGTyglgET/PRUNlrUeu9F5L+7FilkXfZgs0=
    github.com/google/go-cmp@v0.5.8 h1:e6P7q2lk1O+qJJb4BtCQXlK8vWEO8V1ZeuEdJNOqZyg=
    github.com/sergi/go-diff@v1.1.0 h1:we8PVUC3FE2uYfodKH/nBHMSetSfHDR6scGdBi+erh0=
    golang.org/x/exp@v0.0.0-20220722155223-a9213eeb770e h1:+WEEuIdZHnUeJJmEUjyYC2gfUMj69yZXw17EnHg/otA=
    golang.org/x/exp/typeparams@v0.0.0-20220722155223-a9213eeb770e h1:7Xs2YCOpMlNqSQSmrrnhlzBXIE/bpMecZplbLePTJvE=
    golang.org/x/mod@v0.6.0 h1:b9gGHsz9/HhJ3HF5DHQytPpuwocVTChQJK3AvoLRD5I=
    golang.org/x/sync@v0.0.0-20220722155255-886fb9371eb4 h1:uVc8UZUe6tr40fFVnUP5Oj+veunVezqYl9z7DYw9xzw=
    golang.org/x/sys@v0.1.0 h1:kunALQeHf1/185U1i0GOB/fy1IPRDDpuoOOqRReG57U=
    golang.org/x/text@v0.4.0 h1:BrVqGRd7+k1DiOgtnFvAkoQEWQvBc25ouMJM6429SFg=
    golang.org/x/tools@v0.2.1-0.20221101170700-b5bc717366b2 h1:KBm+UwBaO/tdQ35tfGvxH1FUCiXRg4MoTzkznsdeab8=
    golang.org/x/vuln@v0.0.0-20221010193109-563322be2ea9 h1:KaYZQUtEEaV8aVADIHAuYBTjo77aUcCvC7KTGKM3J1I=
    honnef.co/go/tools@v0.3.3 h1:oDx7VAwstgpYpb3wv0oxiZlxY+foCpRAwY7Vk6XpAgA=
    mvdan.cc/gofumpt@v0.3.1 h1:avhhrOmv0IuvQVK7fvwV91oFSGAk5/6Po8GXTzICeu8=
    mvdan.cc/xurls/v2@v2.4.0 h1:tzxjVAj+wSBmDcF6zBB7/myTy3gX9xvi8Tyr28AuQgc=
go: go1.19.3

go env

GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/viktor/.cache/go-build"
GOENV="/home/viktor/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/viktor/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/viktor/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/viktor/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/viktor/local/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.19.3"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/dev/null"
GOWORK=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build1297134621=/tmp/go-build -gno-record-gcc-switches"

What did you do?

I tried finding the root cause and think it can be condensed into this unit test:

func TestCompleteSomeTest(t *testing.T) {
	const mod = `
-- go.mod --
module mod.com

go 1.18
`

	c := struct {
		name          string
		before, after string
	}{
		name: "TestSome",
		before: `
package foo_test

func TestSome
`,
		after: `
package foo_test

func TestSome(t *testing.T)
`,
	}

	r := WithOptions()
	r.Run(t, mod, func(t *testing.T, env *Env) {

		c.before = strings.Trim(c.before, "\n")
		c.after = strings.Trim(c.after, "\n")

		env.CreateBuffer("foo_test.go", c.before)

		pos := env.RegexpSearch("foo_test.go", "Some")
		completions := env.Completion("foo_test.go", pos)
		if len(completions.Items) != 1 {
			t.Errorf("expected one completion, got %v", completions.Items)
		}

		t.Logf("%+v", completions.Items[0].TextEdit)

		env.AcceptCompletion("foo_test.go", pos, completions.Items[0])
		if buf := env.Editor.BufferText("foo_test.go"); buf != c.after {
			t.Errorf("\nGOT:\n%s\nEXPECTED:\n%s", buf, c.after)
		}
	})
}

which fails with

...
--- FAIL: TestCompleteSomeTest (0.30s)
    --- FAIL: TestCompleteSomeTest/default (0.08s)
        completion_test.go:689: completion.TextEdit: &{Range:2:5-2:5 NewText:TestSome(t *testing.T)}
        completion_test.go:693:
            GOT:
            package foo_test

            func TestSome(t *testing.T)TestSome
            EXPECTED:
            package foo_test

            func TestSome(t *testing.T)
...

Since the completions TextEdit has start == end, the suggestion is just inserted.

Editor and settings

This is GNU Emacs 28.1 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.34, cairo version 1.16.0)
lsp-mode 20221115.1951 f5d521d56cfef54d0f102680e956a856347d2c96
company-mode 20221007.2145 48fea7a905b3bcc6d97609316beced666da89b1f

Logs

Emacs logs the interaction like this:

[Trace - 20:55:34.651 PM] Sending request 'textDocument/completion - (74)'.
Params: {"textDocument":{"uri":"file:///home/viktor/tmp/complete/complete_test.go"},"position":{"line":8,"character":13},"context":{"triggerKind":3}}


[Trace - 20:55:34.651 PM] Received response 'textDocument/completion - (74)' in 0ms.
Result: {"isIncomplete":true,"items":[{"label":"TestSome(t *testing.T)","kind":3,"documentation":"complete the parameter","preselect":true,"sortText":"00000","filterText":"TestSome(t *testing.T)","insertTextFormat":2,"textEdit":{"range":{"start":{"line":8,"character":5},"end":{"line":8,"character":5}},"newText":"TestSome(t *testing.T)"}}]}

[findleyr]

CC @pjweinb

[jamalc]

That sounds right, the selection range for this type of completion is set here. I think we would welcome a patch.

[gopherbot]

Change https://go.dev/cl/453335 mentions this issue: gopls/internal/lsp: Replace input text when completing a definition

[vikblom]

@jamalc I opened a CR, maybe you can assign me.

Let me know if there is anything else to consider issue-wise.

[vikblom]

I stumbled across another problem when my test failed in go <= 1.16.

With older versions of go, most of the completions here will not add function parameters.
(Edit: Specifically when completing something like TestFoo. Existing tests get around this by completing TestFoo(), but then the result is TestFoo(t *testing.T)().)

definition.go uses the following logic to decide if (t *testing.T) (or equivalent) should be appended to the completion:

	fp := fd.Type.Params
	if fp != nil && len(fp.List) > 0 {
		// signature already there, minimal suggestion
		return name
	}

From go 1.17.1 and later

fd.Type.Params = &{Opening:60 List:[] Closing:60}

but in go 1.16.15 or earlier,

fd.Type.Params = &{Opening:64 List:[0xc00017a400] Closing:64}
fd.Type.Params.List[0] = &ast.Field{Doc:(*ast.CommentGroup)(nil), Names:[]*ast.Ident(nil), Type:(*ast.BadExpr)(0xc0003e99c0), Tag:(*ast.BasicLit)(nil), Comment:(*ast.CommentGroup)(nil)}

Any advise on how to approach this would be most welcome.
Should I try to fix this or file it as a separate issue?

edit: For now I just added a go1.17 build tag to the tests I introduced.

[findleyr]

Thanks for filing this issue, and sending a fix. Per discussion at #57384 (comment), I think what gopls does is technically correct, but as it appears multiple LSP clients disagree, we should do something like the associated CL.

[findleyr]

Err, I just tested this in VS Code and it is broken:

The inserted text I get is this:

func TestXxx(t *testing.T) {

}(t *testing.T) {

}

Reassigning to pjw to investigate.

[findleyr]

Correction: it works as expected if typing func Te_, but if typing func Te_(t *testing.T) {} (i.e. modifying the test func name), the completion duplicates the test body. I'm not sure if this bug already existed, or if it was introduced by the switch to provide a non-insertion text edit.

[vikblom]

Oh that's not good.

I see the same problem in my editor before and after the patch.

pjw probably has things covered but let me know if I can assist in fixing this.

edit: I installed VSCode (1.73.1) and got similar wonky results with and without gopls patched. I tried to make extra sure that VSCode was using my latest local Go installed gopls but maybe someone used to VSCode can double check.

[findleyr]

I see the same problem in my editor before and after the patch.

Yep, I think I only noticed it as I was re-testing the feature. May have existed for a while. In that case, I'll open a separate bug.