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
Milestone

Comments

@ChrisHines
Copy link
Contributor

@ChrisHines ChrisHines commented Aug 6, 2019

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

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Aug 8, 2019

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 label Aug 8, 2019
@wangfenjin

This comment has been minimized.

Copy link

@wangfenjin wangfenjin commented Oct 15, 2019

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

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Oct 15, 2019

@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

This comment has been minimized.

Copy link

@wangfenjin wangfenjin commented Oct 16, 2019

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

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Oct 16, 2019

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

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 22, 2019

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

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 22, 2019

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

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
4 participants
You can’t perform that action at this time.