Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
myitcv opened this issue Jun 25, 2019 · 7 comments

Comments

@myitcv
Copy link
Member

@myitcv myitcv commented Jun 25, 2019

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

@gopherbot gopherbot added this to the Unreleased milestone Jun 25, 2019
@muirdm

This comment has been minimized.

Copy link

@muirdm muirdm commented Jun 25, 2019

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 comment has been minimized.

Copy link
Member Author

@myitcv myitcv commented Jun 25, 2019

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

This comment has been minimized.

Copy link

@muirdm muirdm commented Jun 25, 2019

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

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Jun 27, 2019

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

This comment has been minimized.

Copy link

@muir muir commented Jun 28, 2019

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

@myitcv

This comment has been minimized.

Copy link
Member Author

@myitcv myitcv commented Jun 29, 2019

@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

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Jul 1, 2019

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.

@stamblerre stamblerre changed the title x/tools/cmd/gopls: completion offers too many irrelevant candidates in struct literal x/tools/gopls: completion offers too many irrelevant candidates in struct literal Jul 2, 2019
@stamblerre stamblerre added help wanted and removed Suggested labels Aug 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.