x/tools/gopls: improve the 'implementation' query on interfaces

[koonix]

gopls version

Build info
----------
golang.org/x/tools/gopls v0.16.1
    golang.org/x/tools/[email protected] h1:1hO/dCeUvjEYx3V0rVvCtOkwnpEpqS29paE+Jw4dcAc=
    github.com/BurntSushi/[email protected] h1:9F2/+DoOYIOksmaJFPw1tGFy1eDnIJXg+UHjuD8lTak=
    github.com/google/[email protected] h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI=
    golang.org/x/exp/[email protected] h1:2O2DON6y3XMJiQRAS1UWU+54aec2uopH3x7MAiqGW6Y=
    golang.org/x/[email protected] h1:5+9lSbEzPSdWkH32vYPBwEpX8KwDbM52Ud9xBUvNlb0=
    golang.org/x/[email protected] h1:YsImfSBoP9QPYL0xyKJPq0gcaJdG3rInoqxTWbfQu9M=
    golang.org/x/[email protected] h1:3Wt8mZlbFwG8llny+t18kh7AXxyWePFycXMuVdHxnyM=
    golang.org/x/[email protected] h1:a94ExnEXNtEwYLGJSIUxnWoxoRz/ZcCsV63ROupILh4=
    golang.org/x/[email protected] h1:Kd+Z5Pm6uwYx3T2KEkeHMHUMZxDPb/q6b1m+zEcy62c=
    golang.org/x/[email protected] h1:SP0mPeg2PmGCu03V+61EcQiOjmpri2XijexKdzv8Z1I=
    honnef.co/go/[email protected] h1:9MDAWxMoSnB6QoSqiVr7P5mtkT9pOc1kSxchzPCnqJs=
    mvdan.cc/[email protected] h1:G3QvahNDmpD+Aek/bNOLrFR2XC6ZAdo62dZu65gmwGo=
    mvdan.cc/xurls/[email protected] h1:lyBNOm8Wo71UknhUs4QTFUNNMyxy2JEIaKKo0RWOh+8=
go: go1.22.5

go env

GO111MODULE=''
GOARCH='amd64'
GOBIN='/home/koonix/.local/bin-go'
GOCACHE='/home/koonix/.cache/go-build'
GOENV='/home/koonix/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/koonix/.local/share/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/koonix/.local/share/go'
GOPRIVATE=''
GOPROXY='direct'
GOROOT='/usr/lib/golang'
GOSUMDB='off'
GOTMPDIR=''
GOTOOLCHAIN='local'
GOTOOLDIR='/usr/lib/golang/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.22.5'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/home/koonix/Repositories/other/gotest/go.mod'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build3909477011=/tmp/go-build -gno-record-gcc-switches'

What did you do?

package main

type A interface {
	Read([]byte) (int, error)
}

type B interface {
	Read([]byte) (int, error)
	Extra()
}

type X int

func (x X) Read([]byte) (int, error) {
	return 0, nil
}

func fn(param A) {}

func main() {

	// this is fine
	var a A
	fn(a)

	// this is also fine
	var b B
	fn(b)

	// so is this
	var x X
	fn(x)
}

What did you see happen?

implementation behaves like so:

• For interface A, it returns all types that implement it (including X but not B).
• For type X, it returns all interfaces that it implements (including A).
• For interface B, it returns no results.

What did you expect to see?

Much like the relationship between A and X, A is implemented by B, and B implements A. Therefore:

• I expect to see B among A's implementations.
• I expect to see A among B's implementations.

Editor and settings

No response

Logs

No response

[gabyhelp]

Related Issues and Documentation

• x/tools/gopls: Autocompletion implementation should contain comments for the interface. #65543 (closed)
• x/tools/gopls: Automatic completion of functions using interface annotations #59594
• x/tools/gopls: no results for implementations #36219 (closed)
• x/tools/gopls: code action "Implement" is not listed #64114 (closed)
• x/tools/gopls: support module-local implementation request #32973 (closed)
• x/tools/gopls: Implement interface methods cannot handle type if it was defined inside type() #56825 (closed)
• x/tools/gopls: missing interfaces in 'implementations' #67041 (closed)
• x/tools/gopls: regression in implementationsV2 for concrete methods #58506 (closed)
• x/tools/gopls: cannot find implementation for a particular interface in a vendored module #46894 (closed)
• x/tools/gopls: Implement action should ignore importing the current package #61830 (closed)

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

[adonovan]

Thanks for the bug report; I can reproduce it with your test case. (The functions fn and main are superfluous, BTW.)

According to the documentation, implementation(A) should indeed include B, since B implements A, so this is definitely a bug. However, implementation(B) is correct not to include A, since A does not implement B.

type A interface { // impls={X} (should include B)
	Read([]byte) (int, error)
}

type B interface { // impls={} (correct)
	Read([]byte) (int, error)
	Extra()
}

type X int // impls={A} (correct)

func (x X) Read([]byte) (int, error) { return 0, nil }

[koonix]

Thanks for your reply.

implementation(B) is correct not to include A, since A does not implement B

I understand that this is true according to the documentation, but could we change this?

What I was trying to show in main() was that the relationship between B and A is practically similar to that of X and A, and if implementation(X) contains A, perhaps implementation(B) should too.

In other words, concrete types can only implement, but interfaces can both implement and be implemented.

I understand that the textDocument/implementation request might not be a good place for this (backwards compatibility? not matching Microsoft's LSP spec?), but it would be nice to somehow be able to see what other interfaces an interface implements.

[tttoad]

@adonovan Is this a problem I can try to fix? I've been idle lately. 0.0

[adonovan]

[@koonix] I understand that this is true according to the documentation, but could we change this?
[...] concrete types can only implement, but interfaces can both implement and be implemented.

Certainly we could, and potentially even within the existing textDocument/implementation operation, if there's a consensus that this is the right change. The LSP spec gives very little guidance on what the implementation of a symbol actually means and what the operation is supposed to do. For example, the wording is loose enough to permit reporting the definition (as opposed to declaration) of a function symbol in C++, which distinguishes them. But I'm not yet convinced it is the right change: when I ask for all implementations of an interface, I would be surprised to see all interfaces that it implements as well. Unfortunately the LSP does not provide a pair of "upwards" and "downwards" operations for the respective halves of the query, and there is currently no way to generalize references-like queries. Perhaps this warrants a separate discussion.
[See https://github.com/microsoft/language-server-protocol/issues/2037]

[@tttoad] Is this a problem I can try to fix?

If you're referring to the bug portion of the issue (that implementations(A) should include B), then yes, we would welcome a fix. Thanks.

[gopherbot]

Change https://go.dev/cl/603075 mentions this issue: gopls/implementation: Fix the query function when the implementation on the interface is a different interface.

[tttoad]

Too much noise? Maybe we should sort, `local.struct`>`other.struct`>`local.interface`>`local.interface` ? @adonovan

[adonovan]

Based on the description of the Go to Implementation feature in the VS Code manual, and despite my comment in the source, I'm no longer convinced that the gopls behavior is wrong. The manual says that when triggered by an interface type, it reports concrete types (subtypes), and when triggered by a concrete type, it reports interfaces (supertypes). That's what gopls does.

Of course, in Go, interfaces may have supertypes (also interfaces). And in languages like Java, concrete types may have subtypes (also concrete types). It would be useful in both these languages for LSP to separate the queries for supertypes and subtypes. But they do need to be separate queries: it makes no sense to report a mixture within a single operation.

One way for LSP to do this would be to support generalized reference queries.

[Update: a better one would be:]

• Add "Go to Interfaces" counterpart of "Go to Implementations" microsoft/language-server-protocol#2037

[gopherbot]

Change https://go.dev/cl/605395 mentions this issue: gopls/references: Fix the query function when the implementation on the interface is a different interface.

[gopherbot]

Change https://go.dev/cl/619719 mentions this issue: gopls/internal/golang: implementation: report iface/iface relations