x/tools/gopls: completion offers too many irrelevant candidates in struct literal

[myitcv]

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!

---

cc @stamblerre @ianthehat

[muirdm]

My argument is that completion should follow the language spec and not be influenced by linters (this particular vet check is not a fatal error, but more of a warning). The language lets you use implicit field names, so gopls completions should support that case. And it isn't some esoteric language feature that no one uses. Many people disable the composite key vet check because they feel there is nothing wrong with using implicit field names in their own code.

Note that gopls lists the field names first (so the noise should be less impactful), and once you have one key-value field in your literal it will stop suggesting lexical completions for later keys.

One easy improvement would be to list a "stricter" set of lexical completions. Instead of including every object and type name, it only lists objects and types that match the struct field. That would cut your list down to:

Name
Age
string

string is debatable, but you might want that completion to perform a type conversion.

[myitcv]

this particular vet check is not a fatal error, but more of a warning

I'm not aware of any such distinction in go vet; go vet to exit with a non-zero exit code, so that's an error to my mind. This particular check is not, however, part of the suite of checks that are part of go test.

Many people disable the composite key vet check because they feel there is nothing wrong with using implicit field names in their own code.

My understanding is that this is a current check, largely for reasons of backward compatibility. If you don't use a keyed literal, then new fields can't be added; it's a breaking change. With that response, I'll chicken out and say that we should probably avoid debating the merits of this particular vet check here 😄

The language lets you use implicit field names, so gopls completions should support that case.

I'm looking to provide an out-of-the-box sensible default behaviour for gopls. So that's what I'm primarily solving for here. And I'm looking to base the decision on what a sensible default is upon preexisting best practice.

At the risk of providing a config soup, it's possible this behaviour could remain but behind a config option.

That would cut your list down to:

I edited my original post because I failed to mention package-level candidates. So the list could potentially be much longer, assuming their respective types match the struct's fields.

[muirdm]

I'm not aware of any such distinction in go vet

go vet runs them all by default, but you can disable them e.g. go vet -composites=false. go test doesn't run "composites" because it is not a definite source of problems/bugs. Here is a gopls issue with some people wanting to disable the go vet composites diagnostic messages: #31717

I'm looking to provide an out-of-the-box sensible default behaviour for gopls

I would rather the default gopls behavior not exclude "legitimate" completions. People use implicit field names so I think it is better to have some false positive "noise" than false negatives.

[stamblerre]

I agree with @muir in this case. I think we actually had this conversation on a code review once - at some point, gopls did refuse anything but keyed literals. I think that I would rather that gopls err on the side of providing too many results rather than too few (which could potentially not be the ones that users want). Maybe as the completion implementation matures we can do more exclusion of results, but for now I think ranking should be enough to accomplish this.

@myitcv, for the record, the gopls tests specifically exclude builtin results to avoid this noise in test cases.

[muir]

@stamblerre, I think you meant @muirrn rather than me.

[myitcv]

@stamblerre @muirrn - ok well I've done my bit/best trying to convince you 😄. For structs outside of the current package I still believe it's the correct thing to do (only provide field names) given the current default behaviour of go vet.

For now at the very least we need to exclude candidates that are not the correct type. @muirrn is that the intention of https://go-review.googlesource.com/c/tools/+/183941?

or the record, the gopls tests specifically exclude builtin results to avoid this noise in test cases.

Not sure what you mean here - can you expand?

[stamblerre]

All I meant is that, when testing, we don't check against builtin candidates because it's too much noise (see https://github.com/golang/tools/blob/fb37f6ba82613749b0b522aa509da78361849fc3/internal/lsp/lsp_test.go#L163).

I think that I'd be willing to revisit this discussion at some point in the future when things are a bit more stable, and we have a better sense of what users want. However, you are completely right that something like println should not be suggested. We definitely need a better approach for handling builtins.

[quasilyte]

It's possible to avoid that issue by setting appropriate min candidate score inside the client (VSCode, for example).

In order to make that safe, users may need a guarantee that candidates that satisfy matchingCandidate(cand *candidate) scored higher than 1.0 (or other sensible min threshold that clients can use).

If we set that threshold to 1.01, this is a list for completion for the given example:

Name candidate score=100.000000
Age candidate score=10.000000
fmt.Sprint candidate score=9.000000
fmt.Sprintf candidate score=9.000000
fmt.Sprintln candidate score=9.000000

Field names have highest score, other candidates are for the case where you want to use unkeyed literals.

As a side note, I believe things like println that are not valid expressions (or anything that is void-typed) shouldn't ever be a completion candidate for a place that expects an expression that produces a value. Right now they have score of 1, which is unfortunate. This is why 1.01 threshold is used instead of 1.0, to drop irrelevant things from the candidates list.

[muirdm]

Since the decision was to include lexical completions in this case, can we close out this issue?

[stamblerre]

Yes, absolutely. Please open a new issue if something else comes up.

[mvdan]

I think that I'd be willing to revisit this discussion at some point in the future when things are a bit more stable, and we have a better sense of what users want.

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.

go test doesn't run "composites" because it is not a definite source of problems/bugs. Here is a gopls issue with some people wanting to disable the go vet composites diagnostic messages: #31717

Personally, I don't think this is a good argument. go test runs 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 having go 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.

People use implicit field names so I think it is better to have some false positive "noise" than false negatives.

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.

[cespare]

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.

[muirdm]

@mvdan Are you arguing against the additional noise of superfluous completion candidates, or against gopls offering completions that fail go vet?

[mvdan]

@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.

[muirdm]

Thanks for clarifying.

Slightly tangential, but with the advent of modules could the "composites" vet check be changed to allow implicit field names that cross package boundaries within the same module?

Anyway, I think it makes sense in this case for gopls to not offer completions that fail vet, as configured. I would still want to be able to disable the "composites" vet analyzer in gopls (or have the analyzer relax its check to module scope).

[myitcv]

Slightly tangential, but with the advent of modules could the "composites" vet check be changed to allow implicit field names that cross package boundaries within the same module?

Continuing the slightly tangential point 😄 but I'm not sure how the advent of modules changes the vet rule?

Anyway, I think it makes sense in this case for gopls to not offer completions that fail vet, as configured.

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?

I would still want to be able to disable the "composites" vet analyzer in gopls (or have the analyzer relax its check to module scope).

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, vet etc, or start using gopls as a way of adding additional controls that aren't in the underlying tool. In many respects, an option in gopls (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 within gopls.

[muirdm]

I'm not sure how the advent of modules changes the vet rule?

The "composites" vet check flags implicit field names used in struct literals only if the struct type is imported from another package. I presume it doesn't flag struct types defined in your current package since if you add another field to the struct you will certainly be able to update composite literals in the struct's package accordingly. The same could not be said for someone else using your package as a library and using implicit field names (i.e. adding a new struct field causes a compilation error for them that you cannot avoid). However, within a single module it seems like we now have the same certainty that we can fix all broken composite literals within the module when adding a new field to the struct.

[mvdan]

However, within a single module it seems like we now have the same certainty that we can fix all broken composite literals within the module when adding a new field to the struct.

I think that's reasonable. Modules came long after these vet checks were designed. It's also true that vet checks are only aware of packages, not modules, so I'm not sure how feasible this restriction would be in practice.

Could you file a cmd/vet issue about it? It should be considered and tracked separately, I think. We could then make gopls follow whatever vet decides to do.