x/tools/gopls: completion incorrectly offers embedded field shadowed by outer field

[myitcv]

What version of Go are you using (go version)?

$ go version
go version go1.20.2 linux/arm64
$ go list -m golang.org/x/tools
golang.org/x/tools v0.9.0
$ go list -m golang.org/x/tools/gopls
golang.org/x/tools/gopls v0.0.0-20230508221120-0809ec2e45f6

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output

$ go env
GO111MODULE="on"
GOARCH="arm64"
GOBIN=""
GOCACHE="/home/myitcv/.cache/go-build"
GOENV="/home/myitcv/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="arm64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/myitcv/gostuff/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/myitcv/gostuff"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/myitcv/gos"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/myitcv/gos/pkg/tool/linux_arm64"
GOVCS=""
GOVERSION="go1.20.2"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/myitcv/gostuff/src/github.com/myitcv/govim/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 -pthread -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build2292349384=/tmp/go-build -gno-record-gcc-switches"

What did you do?

With the following input file:

package main

import (
	"fmt"
	"log"
)

type outer struct {
	// outer logger
	logger *log.Logger

	inner
}

type inner struct {
	// inner logger
	logger *log.Logger
}

func main() {
	var v outer
	fmt.Println(v.lo_)
}

The cursor at the position _.

Attempt a completion.

What did you expect to see?

The single candidate outer.logger.

What did you see instead?

Two candidates:

outer.logger
inner.logger

The completion text for both is the plain logger. So this can't be correct, as the outer.logger shadows the embedded inner.logger.

[findleyr]

Thanks for the report. You're right that this is inaccurate. We should instead offer the deep completion of v.inner.logger.

[myitcv]

Thanks for the report. You're right that this is inaccurate. We should instead offer the deep completion of v.inner.logger.

I'm not clear that in this case you should. Because I don't have deep completions turned on (intentionally), and hopefully my config reflects that.

[findleyr]

@myitcv yes, I mean only in the case where deep completions are enabled.

[findleyr]

BTW: @myitcv I'm curious why you have deep completions turned off, if you don't mind providing feedback.

[myitcv]

BTW: @myitcv I'm curious why you have deep completions turned off, if you don't mind providing feedback.

Of course!

Please bear in mind that what follows is largely a function of my personal experience and preference using Vim.

I have deep completions turned off for largely the same reason as I'm advocating for search scopes in #37237. 90% of the time I don't want deep completions. So any results that are deep would be noise, and I find that noise incredibly distracting. If I could have a keystroke to opt a particular completion into deep completion I would take that for sure! But much like different search scopes, I don't want the default to be "I'll include everything and you keep typing to be more precise". In both cases, I know the scope of the completion or the search, and am happy to assign a key binding to change the scope. So all or nothing config options don't work for me.

[findleyr]

Thanks!

I think proper sorting can come close to providing the best of both worlds. Gopls could improve a lot in this regard.

[myitcv]

I think proper sorting can come close to providing the best of both worlds.

I'm not entirely sure I understand what you mean by "proper sorting". Not least because I'm not clear that in this or the the symbol case "proper sorting" (or indeed any ranking algorithm) can ever know the intended scope of my completion/search, and therefore segment the results accordingly.

Per #37236 (comment), the visual noise is, in any case, a major part of the problem for me.

[muirdm]

I don't see duplicate "logger" candidates anymore, but I still don't see "inner.logger" with deep completions enabled. I think there is some code somewhere that assumes embedded structs don't need to be searched because their fields were already included in the containing struct's fields, but that is not true for fields with conflicting names.

[gopherbot]

Change https://go.dev/cl/592975 mentions this issue: gopls/completion: fix completion of ambiguous fields

[gopherbot]

Change https://go.dev/cl/594242 mentions this issue: gopls/completion: switch to new field/method search