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

Jump to bottom

x/tools/gopls: test completions duplicates text #56852

Closed
vikblom opened this issue Nov 19, 2022 · 10 comments
Closed

x/tools/gopls: test completions duplicates text #56852

vikblom opened this issue Nov 19, 2022 · 10 comments
Assignees
@pjweinb @vikblom
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. NeedsFix The path to resolution is known, but the work has not been done. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
gopls/v0.12.0

Comments

@vikblom
Copy link

vikblom commented Nov 19, 2022 •
edited

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)"}}]}
@gopherbot gopherbot added Tools This label describes issues relating to any tools in the x/tools repository. gopls Issues related to the Go language server, gopls. labels Nov 19, 2022
@gopherbot gopherbot added this to the Unreleased milestone Nov 19, 2022
@findleyr
Copy link
Contributor

CC @pjweinb

@jamalc jamalc added the NeedsFix The path to resolution is known, but the work has not been done. label Nov 22, 2022
@jamalc jamalc modified the milestones: Unreleased, gopls/v0.11.0 Nov 22, 2022
@jamalc
Copy link

jamalc commented Nov 22, 2022

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

@gopherbot
Copy link

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

@vikblom
Copy link
Author

vikblom commented Nov 24, 2022

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

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

@vikblom
Copy link
Author

vikblom commented Nov 28, 2022 •
edited

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 findleyr mentioned this issue Dec 20, 2022
Closed
@findleyr
Copy link
Contributor

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
Copy link
Contributor

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 findleyr reopened this Dec 22, 2022
@findleyr
Copy link
Contributor

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
Copy link
Author

vikblom commented Dec 23, 2022 •
edited

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
Copy link
Contributor

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.

@findleyr findleyr mentioned this issue Dec 27, 2022
Closed
@vikblom vikblom mentioned this issue Apr 19, 2023
Closed
@golang golang locked and limited conversation to collaborators Dec 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@pjweinb pjweinb

@vikblom vikblom

Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. NeedsFix The path to resolution is known, but the work has not been done. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
gopls/v0.12.0
Development

No branches or pull requests
5 participants
@pjweinb @gopherbot @jamalc @vikblom @findleyr