x/tools/gopls: `TestXxx()` completions not working when location followed by newline, due to parser error recovery

[muchzill4]

gopls version

Affects versions >=0.12.0, including current master.

go env

GO111MODULE=""
GOARCH="arm64"
GOBIN=""
GOCACHE="/Users/muchzill4/Library/Caches/go-build"
GOENV="/Users/muchzill4/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="arm64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/muchzill4/.asdf/installs/golang/1.20.5/packages/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/muchzill4/.asdf/installs/golang/1.20.5/packages"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/Users/muchzill4/.asdf/installs/golang/1.20.5/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/Users/muchzill4/.asdf/installs/golang/1.20.5/go/pkg/tool/darwin_arm64"
GOVCS=""
GOVERSION="go1.20.5"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/muchzill4/Dev/vcs/go-tools/gopls/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-prefix-map=/var/folders/8b/9q2y2mm51yb6qmjm0pv9sjhm0000gn/T/go-build2972635718=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

When triggering completions in test files, such as:

package main

import "testing"

func T<caret, where completion should be triggered>

func TestSomething(t *testing.T) {
}

What did you expect to see?

TestXxx as one of the completions.

What did you see instead?

TestMain and Test

Editor and settings

n/a

Logs

[Trace - 11:49:34.710 AM] Sending request 'textDocument/completion - (581)'.
Params: {"textDocument":{"uri":"file:///Users/muchzill4/Dev/my/ttt/foo_test.go"},"position":{"line":4,"character":6},"context":{"triggerKind":1}}
...
[Trace - 11:49:34.711 AM] Received response 'textDocument/completion - (581)' in 1ms.
Result: {"isIncomplete":true,"items":[{"label":"TestMain","kind":3,"documentation":{"kind":"markdown","value":"complete the function name\n"},"preselect":true,"sortText":"00000","filterText":"TestMain","insertTextFormat":2,"textEdit":{"range":{"start":{"line":4,"character":5},"end":{"line":4,"character":6}},"newText":"TestMain"}},{"label":"Test","kind":3,"documentation":{"kind":"markdown","value":"complete the function name\n"},"sortText":"00001","filterText":"Test","insertTextFormat":2,"textEdit":{"range":{"start":{"line":4,"character":5},"end":{"line":4,"character":6}},"newText":"Test"}}]}

Debugging journey

My debugging indicates that this started happening since https://go-review.googlesource.com/c/tools/+/459559.
It seems that t.Closing != t.Opening unless the completion is triggered at the end of the file.

To replicate with existing tests:

diff --git a/gopls/internal/regtest/completion/completion_test.go b/gopls/internal/regtest/completion/completion_test.go
index 117e940e0..e3e62b47e 100644
--- a/gopls/internal/regtest/completion/completion_test.go
+++ b/gopls/internal/regtest/completion/completion_test.go
@@ -729,7 +729,7 @@ package foo
 		env.OpenFile(fname)
 		env.Await(env.DoneWithOpen())
 		for _, test := range tests {
-			env.SetBufferContent(fname, "package foo\n"+test.line)
+			env.SetBufferContent(fname, "package foo\n"+test.line+"\n")
 			loc := env.RegexpSearch(fname, test.pat)
 			loc.Range.Start.Character += uint32(protocol.UTF16Len([]byte(test.pat)))
 			completions := env.Completion(loc)

[adonovan]

This is yet another case of poor parser error recovery. The parser parses the input above to

0 *ast.FuncDecl {
1 . Doc: nil
2 . Recv: nil
3 . Name: *ast.Ident {
4 . . NamePos: play.go:5:6
5 . . Name: "T"
6 . . Obj: *ast.Object {
7 . . . Kind: func
8 . . . Name: "T"
9 . . . Decl: *(obj @ 0)
10 . . . Data: nil
11 . . . Type: nil
12 . . }
13 . }
14 . Type: *ast.FuncType {
15 . . Func: play.go:5:1
16 . . TypeParams: nil
17 . . Params: *ast.FieldList {
18 . . . Opening: play.go:5:7
19 . . . List: []*ast.Field (len = 1) {
20 . . . . 0: *ast.Field {
21 . . . . . Doc: nil
22 . . . . . Names: nil
23 . . . . . Type: *ast.FuncType {
24 . . . . . . Func: play.go:7:1
25 . . . . . . TypeParams: nil
26 . . . . . . Params: *ast.FieldList {
27 . . . . . . . Opening: play.go:7:6
28 . . . . . . . List: []*ast.Field (len = 1) {
29 . . . . . . . . 0: *ast.Field {
30 . . . . . . . . . Doc: nil
31 . . . . . . . . . Names: nil
32 . . . . . . . . . Type: *ast.ParenExpr {
33 . . . . . . . . . . Lparen: play.go:7:19
34 . . . . . . . . . . X: *ast.Ident {
35 . . . . . . . . . . . NamePos: play.go:7:20
36 . . . . . . . . . . . Name: "t"
37 . . . . . . . . . . . Obj: nil
38 . . . . . . . . . . }
39 . . . . . . . . . . Rparen: play.go:7:22
40 . . . . . . . . . }
41 . . . . . . . . . Tag: nil
42 . . . . . . . . . Comment: nil
43 . . . . . . . . }
44 . . . . . . . }
45 . . . . . . . Closing: play.go:7:32
46 . . . . . . }
47 . . . . . . Results: nil
48 . . . . . }
49 . . . . . Tag: nil
50 . . . . . Comment: nil
51 . . . . }
52 . . . }
53 . . . Closing: play.go:8:2
54 . . }
55 . . Results: nil
56 . }
57 . Body: nil
58 }

which pretty-prints to:

func T(

func(t),
)

In other words, the first func decl swallowed the second.

See also: gopls/parsing Issues related to parsing / poor parser recovery.

[findleyr]

Thanks for investigating. With forward compatibility in particular, I think we should fix this upstream (in go/parser).

That will take longer. Moving this to the next milestone.