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.

Sign up for GitHub

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

Jump to bottom

x/tools/gopls: gopls can't "Go to implementation" for generic interface methods #61801

Closed
bimmlerd opened this issue Aug 7, 2023 · 3 comments
Closed

x/tools/gopls: gopls can't "Go to implementation" for generic interface methods #61801

bimmlerd opened this issue Aug 7, 2023 · 3 comments
Labels
gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
gopls/v0.14.0

Comments

@bimmlerd
Copy link

bimmlerd commented Aug 7, 2023 •
edited

What did you do?

In the following program, I would like to find implementations of Resource[T].Store(), but gopls doesn't find the two implementations.

Repro: https://go.dev/play/p/E74QMj99O8z

(asked on gopher slack before filing, sticking around for a bit to answer questions: https://gophers.slack.com/archives/CJZH85XCZ/p1691048076576599)

What did you expect to see?

Placing the cursor on Resource[T].Store, I pressed the shortcut for go to implementation. I expected to see both implementations listed:

func (r *resource[T]) Store() (Store[T], error) {
	return &store[T]{}, nil
}

and

func (fii *fakeSomeObjectResource) Store() (Store[*someObject], error) {
	return &store[*someObject]{}, nil
}

What did you see instead?

an LSP error, saying Not found for: Store

Internal errors

Note that these internal errors are only tangentially related. The reproduction is a minified version of the cilium/cilium/pkg/k8s/resource package implicated below, but the error below doesn't occur on the minimal repro - something else is going on.

Gopls detected 3 internal errors, 1 distinct:

/build/gopls/src/pkg/mod/golang.org/x/tools@v0.11.2-0.20230801165449-23c7f589706c/internal/gcimporter/iexport.go:934: unable to encode object "Events" in package "github.com/cilium/cilium/pkg/k8s/resource": can't find path for func (github.com/cilium/cilium/pkg/k8s/resource.Resource[*github.com/cilium/cilium/pkg/k8s/apis/cilium.io/v2.CiliumNode]).Events(ctx context.Context, opts ...github.com/cilium/cilium/pkg/k8s/resource.EventsOpt) <-chan github.com/cilium/cilium/pkg/k8s/resource.Event[*github.com/cilium/cilium/pkg/k8s/apis/cilium.io/v2.CiliumNode] in github.com/cilium/cilium/pkg/k8s/resource (3/3)

goroutine 283162 [running]:
runtime/debug.Stack()
	/usr/lib/go/src/runtime/debug/stack.go:24 +0x64
golang.org/x/tools/gopls/internal/bug.report({0x402c2b0c00, 0x1e3})
	/build/gopls/src/tools-gopls-v0.13.1/gopls/internal/bug/bug.go:83 +0xd8
golang.org/x/tools/gopls/internal/bug.Reportf({0xaaab5d8e8e3f?, 0x29?}, {0x405193f470?, 0x40034536be?, 0x404ee10500?})
	/build/gopls/src/tools-gopls-v0.13.1/gopls/internal/bug/bug.go:49 +0x28
golang.org/x/tools/internal/gcimporter.(*exportWriter).objectPath(0x404afbd9a0, {0xaaab5dd11f78, 0x404ffb00c0})
	/build/gopls/src/pkg/mod/golang.org/x/tools@v0.11.2-0.20230801165449-23c7f589706c/internal/gcimporter/iexport.go:934 +0x250
golang.org/x/tools/internal/gcimporter.(*exportWriter).doTyp(0x404afbd9a0, {0xaaab5dd05a20?, 0x404ff9a780?}, 0x40489299f0)
	/build/gopls/src/pkg/mod/golang.org/x/tools@v0.11.2-0.20230801165449-23c7f589706c/internal/gcimporter/iexport.go:865 +0x75c
golang.org/x/tools/internal/gcimporter.(*iexporter).typOff(0x404ee10500, {0xaaab5dd05a20, 0x404ff9a780}, 0x0?)
	/build/gopls/src/pkg/mod/golang.org/x/tools@v0.11.2-0.20230801165449-23c7f589706c/internal/gcimporter/iexport.go:723 +0x90
golang.org/x/tools/internal/gcimporter.(*exportWriter).typ(0x404ee29130, {0xaaab5dd05a20?, 0x404ff9a780?}, 0x5?)
	/build/gopls/src/pkg/mod/golang.org/x/tools@v0.11.2-0.20230801165449-23c7f589706c/internal/gcimporter/iexport.go:706 +0x30
golang.org/x/tools/internal/gcimporter.(*iexporter).doDecl(0x404ee10500, {0xaaab5dd121f8, 0x404ff9a050})
	/build/gopls/src/pkg/mod/golang.org/x/tools@v0.11.2-0.20230801165449-23c7f589706c/internal/gcimporter/iexport.go:554 +0x578
golang.org/x/tools/internal/gcimporter.iexportCommon({0xaaab5dd01be0, 0x404e0d4d20}, 0x403d06e080, 0x0, 0x1, 0x2, {0x4003453c98, 0x1, 0x2037b9028adf20a4?}, 0xaaab5dcfd890)
	/build/gopls/src/pkg/mod/golang.org/x/tools@v0.11.2-0.20230801165449-23c7f589706c/internal/gcimporter/iexport.go:154 +0x2bc
golang.org/x/tools/internal/gcimporter.IExportShallow(0xaaab5dbf8080?, 0x40489299f0, 0xaaab5d8aa003?)
	/build/gopls/src/pkg/mod/golang.org/x/tools@v0.11.2-0.20230801165449-23c7f589706c/internal/gcimporter/iexport.go:49 +0x70
golang.org/x/tools/gopls/internal/lsp/cache.(*typeCheckBatch).checkPackage.func1()
	/build/gopls/src/tools-gopls-v0.13.1/gopls/internal/lsp/cache/check.go:686 +0x248
created by golang.org/x/tools/gopls/internal/lsp/cache.(*typeCheckBatch).checkPackage
	/build/gopls/src/tools-gopls-v0.13.1/gopls/internal/lsp/cache/check.go:678 +0x228

Build info

golang.org/x/tools/gopls v0.13.1
    golang.org/x/tools/gopls@(devel)
    github.com/BurntSushi/toml@v1.2.1 h1:9F2/+DoOYIOksmaJFPw1tGFy1eDnIJXg+UHjuD8lTak=
    github.com/google/go-cmp@v0.5.9 h1:O2Tfq5qg4qc4AmwVlvv0oLiVAGB7enBSJ2x2DqQFi38=
    github.com/sergi/go-diff@v1.1.0 h1:we8PVUC3FE2uYfodKH/nBHMSetSfHDR6scGdBi+erh0=
    golang.org/x/exp@v0.0.0-20220722155223-a9213eeb770e h1:+WEEuIdZHnUeJJmEUjyYC2gfUMj69yZXw17EnHg/otA=
    golang.org/x/exp/typeparams@v0.0.0-20221212164502-fae10dda9338 h1:2O2DON6y3XMJiQRAS1UWU+54aec2uopH3x7MAiqGW6Y=
    golang.org/x/mod@v0.12.0 h1:rmsUpXtvNzj340zd98LZ4KntptpfRHwpFOHG188oHXc=
    golang.org/x/sync@v0.3.0 h1:ftCYgMx6zT/asHUrPw8BLLscYtGznsLAnjq5RH9P66E=
    golang.org/x/sys@v0.10.0 h1:SqMFp9UcQJZa+pmYuAKjd9xq1f0j5rLcDIk0mj4qAsA=
    golang.org/x/text@v0.11.0 h1:LAntKIrcmeSKERyiOh0XMV39LXS8IE9UL2yP7+f5ij4=
    golang.org/x/tools@v0.11.2-0.20230801165449-23c7f589706c h1:YilyjKn3EDn6X+x+kP+4REeEtG3WeM52dJAr7AWzVrQ=
    golang.org/x/vuln@v0.0.0-20230110180137-6ad3e3d07815 h1:A9kONVi4+AnuOr1dopsibH6hLi1Huy54cbeJxnq4vmU=
    honnef.co/go/tools@v0.4.2 h1:6qXr+R5w+ktL5UkwEbPp+fEvfyoMPche6GkOpGHZcLc=
    mvdan.cc/gofumpt@v0.4.0 h1:JVf4NN1mIpHogBj7ABpgOyZc65/UUOkKQFkoURsz4MM=
    mvdan.cc/xurls/v2@v2.4.0 h1:tzxjVAj+wSBmDcF6zBB7/myTy3gX9xvi8Tyr28AuQgc=
go: go1.20.7
@gopherbot gopherbot added Tools This label describes issues relating to any tools in the x/tools repository. gopls Issues related to the Go language server, gopls. labels Aug 7, 2023
@gopherbot gopherbot added this to the Unreleased milestone Aug 7, 2023
@findleyr
Copy link
Contributor

findleyr commented Aug 7, 2023

Thank you for the issue.

First of all, the bug reports are a red herring. It turns out that the reported failure can be expected in certain situations, and this report was removed in https://go.dev/cl/514575. Where did you observe these reports?

I was aware of the incompleteness of implementations for generic interfaces, and thought we already had an open issue, but can't find it. Thank you for writing it up so diligently.

There is a bit of an open question about what it means for an type to implement a generic interface: does it mean that there's a theoretical instantiation of the type that implements the interface? Does it mean that the type implements all instantiations of the interface? Does it mean that the type is generic, and pairwise instantiations of the type and interface implement eachother?

Nevertheless, I think we can come up with a reasonable definition, and do better than we do now. Probably the following is what we want:

A (possibly generic) type T implements a (possibly generic) interface I if:

  • T and I are not generic, and T implements I (the normal case)
  • T is generic, and there is an implementation of T_0 that implements I.
  • I is generic, and there is an implementation I_0 of I where T implements I_0
  • T and I are generic, and there are implementations (T_0, I_0) where T_0 implements I_0.

Answering these correctly will require type unification, but we could perhaps do something weaker that is close enough.

@findleyr findleyr modified the milestones: Unreleased, gopls/v0.14.0 Aug 7, 2023
@bimmlerd
Copy link
Author

bimmlerd commented Aug 8, 2023

Thanks for the speedy answer!

First of all, the bug reports are a red herring. It turns out that the reported failure can be expected in certain situations, and this report was removed in https://go.dev/cl/514575. Where did you observe these reports?

I ran gopls bug and it printed those. I don't know how that command works, so it seems possible to me that these reports were produced by older versions of gopls and the newer version simply discovered them. Especially so as no report was created regarding the above example.

I was aware of the incompleteness of implementations for generic interfaces, and thought we already had an open issue, but can't find it.

I was sure there was going to be one, but also couldn't find it. Happy to close this once we find it to be a duplicate.

There is a bit of an open question about what it means for an type to implement a generic interface: does it mean that there's a theoretical instantiation of the type that implements the interface?

Yeah, it wasn't entirely clear to me what to expect, either. In practice, though, it seems to me that I'm interested in the following:

  1. For a generic interface I, if Ts exist which implement all instantiations of I, those are the implementations I want. In the real example of cilium's Resource[T], I'm interested to go to the generic implementation (which holds all the complexity), not fake/mock implementations for tests. (Interestingly, I've just noticed that across package boundaries, implementations which instantiate the interface for a specific type, like the second example in the playground code above, seem to be found! Ironically, those are exactly the test mocks that I don't care about 😬)
  2. For a new type which instantiates Resource[T], say a type NodeResource Resource[Node], I care about both the generic implementation(s) and implementations for that specific instantiation. I don't, however, care about Resource[Pod] mocks.

Not sure how general those preferences are, however.

I've also just realised that going from the (generic) implementation to the interface already works.

@findleyr
Copy link
Contributor

Aha, I found the existing issue: #59224

Let's consolidate discussion there.

@findleyr findleyr closed this as not planned Won't fix, can't repro, duplicate, stale Aug 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
gopls/v0.14.0
Development

No branches or pull requests
3 participants
@bimmlerd @gopherbot @findleyr