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: experimental semantic tokenizing of suggested argument names in user defined function types #45233

Closed
soypat opened this issue Mar 25, 2021 · 10 comments
Assignees
Milestone

Comments

@soypat
Copy link

@soypat soypat commented Mar 25, 2021

What did you do?

Define some function types:

type Shape func(xyz ...float64) float64
type ABC func(uvw float64) (err error)

func (a Node) Sh(abc ...int) {}

gopls highlights xyz and uvw (suggested function argument names) as semantic tokens. This may be intentional, but I believe it is worth discussing if these "variable names" should be categorized as types, after all abc (in the same example) is not categorized as a semantic token.

Version info/env

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

$ go version
go version go1.16.2 linux/amd64

Does this issue reproduce with the latest release?

I am in the latest release.

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

go env Output
$  go env
GO111MODULE=""
GOARCH="amd64"
GOBIN="/home/pato/go/bin"
GOCACHE="/home/pato/.cache/go-build"
GOENV="/home/pato/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/pato/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/pato/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.16.2"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="0"
GOMOD="/home/pato/src/gofeas/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 -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build2731818870=/tmp/go-build -gno-record-gcc-switches"
@hyangah hyangah changed the title gopls experimental semantic tokenizing of suggested argument names in user defined function types x/tools/gopls: experimental semantic tokenizing of suggested argument names in user defined function types Mar 25, 2021
@gopherbot gopherbot added this to the Unreleased milestone Mar 25, 2021
@hyangah
Copy link
Contributor

@hyangah hyangah commented Mar 25, 2021

/cc @pjweinb

@pjweinb
Copy link

@pjweinb pjweinb commented Mar 25, 2021

That seems to be what it is doing. What should it do? gopls semtok produces
/⇒4,keyword,[]/type /⇒5,type,[definition]/Shape /⇒4,keyword,[]/func(/⇒3,type,[definition]/xyz /⇒3,operator,[]/.../⇒7,type,[]/float64) /⇒7,type,[]/float64
/⇒4,keyword,[]/type /⇒3,type,[definition]/ABC /⇒4,keyword,[]/func(/⇒3,type,[definition]/uvw /⇒7,type,[]/float64) (/⇒3,type,[definition]/err /⇒5,type,[]/error)

/⇒4,keyword,[]/func (/⇒1,variable,[]/a /⇒4,variable,[definition]/Node) /⇒2,member,[definition]/Sh(/⇒3,parameter,[definition]/abc /⇒3,operator,[]/.../⇒3,type,[]/int) {}

@soypat
Copy link
Author

@soypat soypat commented Mar 25, 2021

So the part that seems off to me is /⇒3,type,[definition]/xyz, as xyz is not really a definition, but really just an argument name suggestion. The user of the Shape type can choose to use xyz as the argument name or just any other name within the Go rules for identifier naming. Which brings me to the next point, xyz is closer to an identifier (parameter semtok).

@pjweinb
Copy link

@pjweinb pjweinb commented Mar 25, 2021

thanks. let me think about it overnight. (or over-weekend).

@pjweinb
Copy link

@pjweinb pjweinb commented Mar 26, 2021

There's the same issue in type X struct{A int}, which syntactically just has a StructDecl instead of a FuncDecl, so it need to change both places, i think.

@soypat
Copy link
Author

@soypat soypat commented Mar 26, 2021

Yes, I was about to create an issue complaining on that! The following declaration is confusing since all of the contents are colored the same!

type rocket struct {
	// detalles constructivos
	caliber, length, massEmpty float64
	// esp. tecnicas
	thrustMax, burnTime, propInitialMass float64
	// rocket state
	apogee, empty, mainParachute bool
}

I believe apogee, empty, mainParachute should be colored differently than bool for clarity.

@soypat
Copy link
Author

@soypat soypat commented Mar 26, 2021

Inline type definition seems to work as expected. The following example has the expected behaviour in gopls v0.6.9.

v := struct{A int}{}
var w struct{A float64}
var u = struct{A complex128}{}

Does color A and its type differently.

@pjweinb
Copy link

@pjweinb pjweinb commented Mar 26, 2021

I will produce a fix. Thanks for reporting this.

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 29, 2021

Change https://golang.org/cl/305209 mentions this issue: internal/lsp/semantic: fix some type definitions

@pjweinb pjweinb self-assigned this Mar 29, 2021
@stamblerre stamblerre modified the milestones: Unreleased, gopls/v0.6.10 Mar 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants