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: incorrect placeholders for aliased types #33500

Closed
ChrisHines opened this issue Aug 6, 2019 · 8 comments
Closed

x/tools/gopls: incorrect placeholders for aliased types #33500

ChrisHines opened this issue Aug 6, 2019 · 8 comments
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. help wanted
Milestone

Comments

@ChrisHines
Copy link
Contributor

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

golang.org/x/tools/gopls v0.1.3
    golang.org/x/tools/gopls@v0.1.4-0.20190806194950-6743d4095d4b h1:2RQrzcrEYVmfqqLlivm/WQAKgrN3esjyGgkI4UhN/a0=
    golang.org/x/sync@v0.0.0-20190423024810-112230192c58 h1:8gQV6CLnAEikrhgkHFbMAEhagSSnXWGV915qUMm9mrU=
    golang.org/x/tools@v0.0.0-20190723021737-8bb11ff117ca h1:SqwJrz6xPBlCUltcEHz2/p01HRPR+VGD+aYLikk8uas=

Go info
-------
go version go1.12.7 darwin/amd64

GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/chines209/Library/Caches/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/chines209/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/go"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
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 -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/zs/qv8flyxx1zl_nmdkklb5p15sz6zd1t/T/go-build867357531=/tmp/go-build -gno-record-gcc-switches -fno-common"

Does this issue reproduce with the latest release?

Yes.

What did you do?

I declared a function type struct field. The function has a parameter and return value with a type that is an exported alias of a type in an internal package my code doesn't have access to. For example:

import "golang.org/x/net/ipv4"

type mockAppender struct {
	appendMessages func(now time.Time, ms []ipv4.Message) ([]ipv4.Message, bool)
	stopped        func()
}

I then declare type literals for mockAppender in VSCode with the gopls.usePlaceholders option enabled.

What did you expect to see?

When I type a struct literal for the above type and add the appendMessages field, I expect the generated placeholder to reference types my code has access to, specifically:

ma := mockAppender{
	appendMessages: // placeholder -> func(now time.Time, ms []ipv4.Message) ([]ipv4.Message, bool, StepTime),
}

What did you see instead?

A placeholder that referenced types from a package my code cannot import that is type that ipv4.Message aliases. Specifically:

ma := mockAppender{
	appendMessages: // placeholder -> func(now time.Time, ms []socket.Message) ([]socket.Message, bool, StepTime),
}
@gopherbot gopherbot added this to the Unreleased milestone Aug 6, 2019
@gopherbot gopherbot added the gopls Issues related to the Go language server, gopls. label Aug 6, 2019
@stamblerre stamblerre changed the title x/tools/gopls: placeholders for aliased types not right x/tools/gopls: incorrect placeholders for aliased types Aug 7, 2019
@stamblerre stamblerre added Suggested Issues that may be good for new contributors looking for work to do. help wanted labels Aug 8, 2019
@stamblerre
Copy link
Contributor

Thanks for filing this issue! This is lower priority for us, so while I will try to fix this when I get a chance, if anyone else is interested in contributing a fix for this, please go ahead!

@stamblerre stamblerre removed the Suggested Issues that may be good for new contributors looking for work to do. label Aug 8, 2019
@wangfenjin
Copy link

Hi @stamblerre ,

I'm interested to look into this issue.

I think the problem is related to types.TypeString() function in this line completion_format.go

Am I in the right direction? Thanks.

@stamblerre
Copy link
Contributor

@wangfenjin: The placeholders for a function are actually determined in the functionCallSnippet function in completion_snippet.go. I would suggest getting started here by adding a test case and running the gopls tests. Examples of the completion snippet tests can be found here.

@wangfenjin
Copy link

Hi @stamblerre ,

After some investment, I find it's a placeholder for struct field which determined by structFieldSnippet, which in turn relies on types.TypeString(). I think I should fix this in golang/go package.

@wangfenjin
Copy link

wangfenjin commented Oct 16, 2019

Hi @stamblerre ,

Seems it's intended to return the underlining type for alias, please refer to types/decl.go. I verified this issue will be fixed by changing the if alias to false, but I think we can't change the logic here.

Do you have any suggestions?

@stamblerre
Copy link
Contributor

Ah I see, thanks for investigating this. Looks like there's no way for us to to get back the alias information from the go/types package. An alternative solution would be for us to always use the file's AST when constructing the placeholders. That would mean replacing the logic that uses types.TypeString. This would be similar to the logic that gets the item's documentation (see https://github.com/golang/tools/blob/9c6d90b5a7d0c5de88404edf63ae60750868bf0a/internal/lsp/source/completion_format.go#L119).

@gopherbot
Copy link

Change https://golang.org/cl/201677 mentions this issue: internal/lsp: use file AST to construct field placeholders

@gopherbot
Copy link

Change https://golang.org/cl/208497 mentions this issue: internal/lsp: use AST to construct placeholders

@golang golang locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. help wanted
Projects
None yet
Development

No branches or pull requests

4 participants