x/tools/gopls: field completion on the target variable of a type switch is flakey

alecthomas commented 10 months ago

What version of Go, VS Code & VS Code Go extension are you using?

Version Information

* Run `go version` to get version of Go from _the VS Code integrated terminal_. - go version go1.21.5 darwin/arm64 * Run `gopls -v version` to get version of Gopls from _the VS Code integrated terminal_. - ``` Build info ---------- golang.org/x/tools/gopls v0.14.2 golang.org/x/tools/gopls@v0.14.2 h1:sIw6vjZiuQ9S7s0auUUkHlWgsCkKZFWDHmrge8LYsnc= 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+54aec2uopH3x7MAiqGW6Y= golang.org/x/mod@v0.14.0 h1:dGoOF9QVLYng8IHTm7BAyWqCqSheQ5pYWGhzW00YJr0= golang.org/x/sync@v0.4.0 h1:zxkM55ReGkDlKSM+Fu41A+zmbZuaPVbGMzvvdUPznYQ= golang.org/x/sys@v0.14.0 h1:Vz7Qs629MkJkGyHxUlRHizWJRG2j8fbQKjELVSNhy7Q= golang.org/x/telemetry@v0.0.0-20231114163143-69313e640400 h1:brbkEFfGwNGAEkykUOcryE/JiHUMMJouzE0fWWmz/QU= golang.org/x/text@v0.13.0 h1:ablQoSUd0tRdKxZewP80B+BaqeKJuVhuRxj/dkrun3k= golang.org/x/tools@v0.14.1-0.20231114185516-c9d3e7de13fd h1:Oku7E+OCrXHyst1dG1z10etCTxewCHXNFLRlyMPbh3w= 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.21.3 ``` * Run `code -v` or `code-insiders -v` to get version of VS Code or VS Code Insiders. - 1.85.0 af28b32d7e553898b2a91af498b1fb666fdebe0c arm64 * Check your installed extensions to get the version of the VS Code Go extension - v0.40.0 * Run Ctrl+Shift+P (Cmd+Shift+P on Mac OS) > `Go: Locate Configured Go Tools` command. ``` Checking configured tools.... GOBIN: /Users/alec/dev/ftl/.hermit/go/bin toolsGopath: gopath: /Users/alec/go GOROOT: /Users/alec/Library/Caches/hermit/pkg/go-1.21.5 PATH: /Users/alec/Library/Caches/hermit/pkg/go-1.21.5/bin:/Users/alec/dev/ftl/scripts:/Users/alec/dev/ftl/frontend/node_modules/.bin:/Users/alec/dev/ftl/node_modules/.bin:/Users/alec/dev/ftl/.hermit/node/bin:/Users/alec/dev/ftl/.hermit/go/bin:/Users/alec/dev/ftl/bin:/Users/alec/.local/bin:/Users/alec/bin:/opt/homebrew/bin:/usr/local/bin:/System/Cryptexes/App/usr/bin:/usr/bin:/bin:/usr/sbin:/sbin:/var/run/com.apple.security.cryptexd/codex.system/bootstrap/usr/local/bin:/var/run/com.apple.security.cryptexd/codex.system/bootstrap/usr/bin:/var/run/com.apple.security.cryptexd/codex.system/bootstrap/usr/appleinternal/bin:/Library/Apple/usr/bin:/Applications/kitty.app/Contents/MacOS:/Users/alec/.orbstack/bin:/opt/homebrew/opt/fzf/bin PATH (vscode launched with): /Users/alec/dev/ftl/scripts:/Users/alec/dev/ftl/frontend/node_modules/.bin:/Users/alec/dev/ftl/node_modules/.bin:/Users/alec/dev/ftl/.hermit/node/bin:/Users/alec/dev/ftl/.hermit/go/bin:/Users/alec/dev/ftl/bin:/Users/alec/.local/bin:/Users/alec/bin:/opt/homebrew/bin:/usr/local/bin:/System/Cryptexes/App/usr/bin:/usr/bin:/bin:/usr/sbin:/sbin:/var/run/com.apple.security.cryptexd/codex.system/bootstrap/usr/local/bin:/var/run/com.apple.security.cryptexd/codex.system/bootstrap/usr/bin:/var/run/com.apple.security.cryptexd/codex.system/bootstrap/usr/appleinternal/bin:/Library/Apple/usr/bin:/Applications/kitty.app/Contents/MacOS:/Users/alec/.orbstack/bin:/opt/homebrew/opt/fzf/bin go: /Users/alec/Library/Caches/hermit/pkg/go-1.21.5/bin/go: go version go1.21.5 darwin/arm64 gopls: /Users/alec/dev/ftl/.hermit/go/bin/gopls (version: v0.14.2 built with go: go1.21.3) gotests: /Users/alec/dev/ftl/.hermit/go/bin/gotests (version: v1.6.0 built with go: go1.21.3) gomodifytags: /Users/alec/dev/ftl/.hermit/go/bin/gomodifytags (version: v1.16.0 built with go: go1.21.3) impl: /Users/alec/dev/ftl/.hermit/go/bin/impl (version: v1.1.0 built with go: go1.21.3) goplay: /Users/alec/dev/ftl/.hermit/go/bin/goplay (version: v1.0.0 built with go: go1.21.3) dlv: /Users/alec/dev/ftl/.hermit/go/bin/dlv (version: v1.21.1 built with go: go1.21.3) staticcheck: /Users/alec/dev/ftl/.hermit/go/bin/staticcheck (version: v0.4.6 built with go: go1.21.3) go env Workspace Folder (ftl): /Users/alec/dev/ftl GO111MODULE='' GOARCH='arm64' GOBIN='/Users/alec/dev/ftl/.hermit/go/bin' GOCACHE='/Users/alec/Library/Caches/go-build' GOENV='/Users/alec/Library/Application Support/go/env' GOEXE='' GOEXPERIMENT='' GOFLAGS='' GOHOSTARCH='arm64' GOHOSTOS='darwin' GOINSECURE='' GOMODCACHE='/Users/alec/go/pkg/mod' GONOPROXY='*.sqcorp.co,github.com/squareup' GONOSUMDB='*.sqcorp.co,github.com/squareup' GOOS='darwin' GOPATH='/Users/alec/go' GOPRIVATE='*.sqcorp.co,github.com/squareup' GOPROXY='https://proxy.golang.org,direct' GOROOT='/Users/alec/Library/Caches/hermit/pkg/go-1.21.5' GOSUMDB='sum.golang.org' GOTMPDIR='' GOTOOLCHAIN='local' GOTOOLDIR='/Users/alec/Library/Caches/hermit/pkg/go-1.21.5/pkg/tool/darwin_arm64' GOVCS='' GOVERSION='go1.21.5' GCCGO='gccgo' AR='ar' CC='clang' CXX='clang++' CGO_ENABLED='1' GOMOD='/Users/alec/dev/ftl/go.mod' GOWORK='/Users/alec/dev/ftl/go.work' 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 -ffile-prefix-map=/var/folders/h2/_9k30gds1dbdjsv1m3yw834w0000gn/T/go-build2176859146=/tmp/go-build -gno-record-gcc-switches -fno-common' ```

Share the Go related settings you have added/edited

Run Preferences: Open Settings (JSON) command to open your settings.json file. Share all the settings with the go. or ["go"] or gopls prefixes.

Describe the bug

Steps to reproduce the behavior:

Field completion on the target variable of a type switch seems to be flakey. I've had it work before, but under some circumstances (such as with the code below) it doesn't seem to, instead it repeatedly shows completions for the top-level variable.

The following code seems to reproduce this:

package main

import (
    "fmt"
    "strings"
    "text/template/parse"
)

func main() {
    trees, err := parse.Parse("template", `{{ range .Modules }}{{.Name}}{{ . | branch }}{{ end }}`, `{{`, `}}`, map[string]any{"branch": func(b any) string { return "..." }})
    if err != nil {
        panic(err)
    }
    template := trees["template"]
    visit(template.Root, 0)
}

func visit(node parse.Node, indent int) {
    if node == nil {
        return
    }
    fmt.Printf("%s%T\n", strings.Repeat(" ", indent*2), node)
    switch n := node.(type) {
    case *parse.ActionNode:
        visit(n.Pipe, indent+1)

    case *parse.ListNode:
        for _, child := range n.Nodes {
            visit(child, indent+1)
        }

    case *parse.RangeNode:
        visit(n.Pipe, indent+1)
        visit(n.List, indent+1)
        visit(&n.BranchNode, indent+1)
        n.n.n.n.

    case *parse.IfNode:

    case *parse.WithNode:

    case *parse.TemplateNode:

    case *parse.TextNode:

    case *parse.PipeNode:
        for _, child := range n.Decl {
            visit(child, indent+1)
        }
        for _, child := range n.Cmds {
            visit(child, indent+1)
        }

    case *parse.CommandNode:

    case *parse.ChainNode:

    case *parse.IdentifierNode:

    case *parse.VariableNode:

    case *parse.DotNode:

    case *parse.NilNode:

    case *parse.FieldNode:

    case *parse.StringNode:

    case *parse.BoolNode:

    case *parse.BranchNode:

    case *parse.NumberNode:

    case *parse.BreakNode:

    case *parse.ContinueNode:

    default:
        fmt.Printf("%T\n", n)
    }
}

Screenshots or recordings

alecthomas commented 10 months ago

Actually it seems that they do show up if I happen to type a character that matches one of the field prefixes, but of course I have to know this in advance.

eg. with just .:

But if I type P then matching fields show up:

hyangah commented 10 months ago

Thanks for the report and the repro case. Looks like gopls has trouble handling completion when struct embedding is involved.

And, including n, indent, ... in the completion response in this case doesn't make sense. That needs investigation too. Transferring to the gopls issue tracker for further investigation.

findleyr commented 10 months ago

See @adonovan's comment here: https://cs.opensource.google/go/x/tools/+/master:gopls/internal/lsp/source/completion/util.go;l=32;drc=28b92af2866ab2bc225795ba13f5a1ae765ffec5

There he cites types.NewSelectionSet (an API that does not exist). The lack of said API is exactly why we have this problem. We should propose it.

adonovan commented 10 months ago

Much though I would love to claim prescience, I have no recollection of writing this comment--not even the foggiest. So I checked, and indeed I didn't write it, @stamblerre did, in https://go.dev/173779 (see internal/lsp/source/util.go#32). So she deserves credit, both for the comment and for sneakily assigning it to me. ;-)

muirdm commented 10 months ago

This is the classic trailing "." breaking parsing, I think? i.e. it is parsed as n.case which is invalid due to the keyword.

I know I've tried to hack around similar issues in the past. Maybe we need another special "fix AST" case that looks for dangling selector + newline + keyword and rewrites the selector with a phantom _ to make it parse normally (e.g. a.\ncase becomes a._\ncase. Or maybe the stdlib parser getting more robust soon?

golang / go