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

koonix commented 1 month ago

gopls version

Build info
----------
golang.org/x/tools/gopls v0.16.1
    golang.org/x/tools/gopls@v0.16.1 h1:1hO/dCeUvjEYx3V0rVvCtOkwnpEpqS29paE+Jw4dcAc=
    github.com/BurntSushi/toml@v1.2.1 h1:9F2/+DoOYIOksmaJFPw1tGFy1eDnIJXg+UHjuD8lTak=
    github.com/google/go-cmp@v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI=
    golang.org/x/exp/typeparams@v0.0.0-20221212164502-fae10dda9338 h1:2O2DON6y3XMJiQRAS1UWU+54aec2uopH3x7MAiqGW6Y=
    golang.org/x/mod@v0.18.0 h1:5+9lSbEzPSdWkH32vYPBwEpX8KwDbM52Ud9xBUvNlb0=
    golang.org/x/sync@v0.7.0 h1:YsImfSBoP9QPYL0xyKJPq0gcaJdG3rInoqxTWbfQu9M=
    golang.org/x/telemetry@v0.0.0-20240607193123-221703e18637 h1:3Wt8mZlbFwG8llny+t18kh7AXxyWePFycXMuVdHxnyM=
    golang.org/x/text@v0.16.0 h1:a94ExnEXNtEwYLGJSIUxnWoxoRz/ZcCsV63ROupILh4=
    golang.org/x/tools@v0.22.1-0.20240628205440-9c895dd76b34 h1:Kd+Z5Pm6uwYx3T2KEkeHMHUMZxDPb/q6b1m+zEcy62c=
    golang.org/x/vuln@v1.0.4 h1:SP0mPeg2PmGCu03V+61EcQiOjmpri2XijexKdzv8Z1I=
    honnef.co/go/tools@v0.4.7 h1:9MDAWxMoSnB6QoSqiVr7P5mtkT9pOc1kSxchzPCnqJs=
    mvdan.cc/gofumpt@v0.6.0 h1:G3QvahNDmpD+Aek/bNOLrFR2XC6ZAdo62dZu65gmwGo=
    mvdan.cc/xurls/v2@v2.5.0 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 commented 1 month ago

Related Issues and Documentation

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

adonovan commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

adonovan commented 1 month ago

[@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.

[@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 commented 1 month ago

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 commented 1 month ago

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

adonovan commented 1 month ago

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.

gopherbot commented 3 weeks ago

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.

golang / go