go: x/tools/gopls: completion offers too many irrelevant candidates in struct literal
What version of Go are you using (go version)?
$ go version go version devel +44c9354c5a Fri Jun 21 05:21:30 2019 +0000 linux/amd64 $ go list -m golang.org/x/tools golang.org/x/tools v0.0.0-20190620191750-1fa568393b23 $ go list -m golang.org/x/tools/gopls golang.org/x/tools/gopls v0.0.0-20190620191750-1fa568393b23
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="amd64" GOBIN="/home/myitcv/gostuff/src/github.com/myitcv/govim/cmd/govim/.bin" GOCACHE="/home/myitcv/.cache/go-build" GOENV="/home/myitcv/.config/go/env" GOEXE="" GOFLAGS="" GOHOSTARCH="amd64" GOHOSTOS="linux" 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_amd64" GCCGO="gccgo" AR="ar" CC="gcc" CXX="g++" CGO_ENABLED="1" GOMOD="/home/myitcv/gostuff/src/github.com/myitcv/govim/go.mod" 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 -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build637283533=/tmp/go-build -gno-record-gcc-switches"
What did you do?
Consider the following example:
package main
import (
"fmt"
"playground.com/p"
)
func main() {
s := p.S{
// attempt completion
}
fmt.Println(s)
}
-- go.mod --
module playground.com
-- p/p.go --
package p
type S struct {
Name string
Age int
}
If completion is attempted at the position of the comment // attempt completion the following list of candidates is returned:
Name
Age
fmt
p
main()
string
append
bool
byte
cap
close
complex
complex128
complex64
copy
delete 78% ☰ 11/14 ㏑ : 7
error
false
float32
float64
imag
int
int16
int32
int64
int8
iota
len
make
new
nil
panic
print
println
real
recover
rune
true
uint
uint16
uint32
uint64
uint8
uintptr
In this case, because S is declared in another package, vet will enforce that use of the struct type in a composite literal must be keyed:
https://play.golang.org/p/-H5Fnm5c9zj
Hence I believe the only valid candidates are:
Name
Age
In any case, if S were declared in the same package, the list of candidates is not actually correct: it appears to be the valid key names plus all the predeclared identifiers, plus package-scope identifiers, regardless of whether they are applicable.
My proposal would be that regardless of whether S is declared in the current package or not, the list of candidates be limited to the valid key names. This feels like a more sensible default; far less noise in the majority of cases.
I realise this is subjective… so other thoughts welcomed!
About this issue
- Original URL
- State: open
- Created 5 years ago
- Reactions: 11
- Comments: 18 (11 by maintainers)
Could I gently nudge you to reopen or reconsider this issue? 😃 It’s been over a year, and as someone who has started using gopls recently (I know I’m late), I’m surprised at the outcome of this issue.
Personally, I don’t think this is a good argument.
go testruns a subset of checks, yes, but that does not mean one can or should choose to ignore the others. I think we can all agree that a codebase havinggo vet ./...warnings is generally not a good sign.I’d even go as far as saying that gopls should not have easy or well-documented ways to disable vet checks. The whole point of vet is that it has zero false positives, and practically everyone should follow its advice. If a check is not useful enough, or has too many false positives, it wouldn’t have met vet’s criteria.
This change is for unkeyed composite literals for exported structs declared in other packages. Unkeyed composite literals with local types would continue to work the same.
Continuing the slightly tangential point 😄 but I’m not sure how the advent of modules changes the vet rule?
I’ll re-open this issue for now then, because there doesn’t seem like much point opening a duplicate, and it saves us discussing on a closed issue.
@stamblerre - what are your thoughts on reducing the scope of the completion candidates, according to
vet’s default rules?As I’ve mentioned before, and @mvdan mentions above, I think we need to take care to not undo the (almost) zero-option nature of
gofmt,vetetc, or start usinggoplsas a way of adding additional controls that aren’t in the underlying tool. In many respects, an option ingopls(more specifically the editor one is using) is easier to adjust, easier to share with colleagues and more persistent. Whether that makes it more likely people will start to configure these things I don’t know. But I think we should be referring back to those original decision makers when considering this sort of change/option withingopls.@muirdm I’m arguing against gopls offering completions which fail go vet, yes. I made some extra arguments in the comment above against making vet configurable at a high level, but the reason I want to reopen this issue is what you said.
I filed #43374 for a similar issue.
For me, the experience of using completion with gopls is somewhat bimodal. In some contexts, you get a short list of sensible completions. In other contexts (such as this and #43374), you are presented with a very long list of mostly-irrelevant suggestions.
@stamblerre, I think you meant @muirrn rather than me.