x/tools/gopls: missing interfaces in 'implementations'

[adonovan]

An implementations query on *types.Struct does not report the types.Type interface, nor vice versa.

xtools$ cat a.go
package tools

import "go/types"

var _ *types.Struct
var _ types.Type


xtools$ gopls implementation ./a.go:#48      # Struct
Log: Loading packages...
Info: Finished loading packages.
/Users/adonovan/w/goroot/src/context/context.go:518:6-14
/Users/adonovan/w/goroot/src/expvar/expvar.go:41:6-9
/Users/adonovan/w/goroot/src/fmt/print.go:63:6-14
/Users/adonovan/w/goroot/src/runtime/error.go:210:6-14
/Users/adonovan/w/xtools/gopls/internal/test/integration/fake/glob/glob.go:182:6-13

xtools$ gopls implementation ./a.go:#67    # Type
Log: Loading packages...
Info: Finished loading packages.
/Users/adonovan/go/pkg/mod/honnef.co/go/tools@v0.4.7/go/types/typeutil/ext.go:8:6-14
/Users/adonovan/w/goroot/src/go/internal/gcimporter/support.go:154:6-13
/Users/adonovan/w/xtools/go/ssa/builder.go:90:6-16
/Users/adonovan/w/xtools/go/ssa/interp/reflect.go:24:6-16
/Users/adonovan/w/xtools/internal/gcimporter/bimport.go:147:6-13

[adonovan]

Intriguingly this case reduces to types.AssignableTo("*types.Struct", "types.Type") returning false. The plot thickens...

[findleyr]

🍿 yet type Type2 types.Type is found.

It must be the case that the types.Type returned by Type.Underlying is from a different realm.
This is why all the stringer interfaces in the standard library are found (because string is a basic type), yet types.Type is not.

[findleyr]

It must have been broken by https://go.dev/cl/518896. obj and localpkgs are no longer in the same realm 😞

How this was not caught by any of our tests, I don't know.

That's quite an oversight.

[gopherbot]

Change https://go.dev/cl/581875 mentions this issue: gopls/internal/golang: fix resolution of in-package implementations

[findleyr]

Aha: the local query works if it originates from the declaring package, because of the way caching works.
This is probably why it wasn't noticed: users may have grown used to jumping to the package and then running the implements query (I believe @hyangah mentioned that she does this).

[findleyr]

Interestingly, objects were already being compared from different realms, in test variants. Perhaps I saw that and therefore assumed that the object identity didn't matter... (being generous to myself).

The CL above also fixes the pre-existing bug finding local implementations declared in test variants.

[adonovan]

I meant to follow-up this morning but you beat me to it. (Thanks!) I still don't understand why realms (my first thought too) could explain the AssignableTo result in this specific case where the RHS is an interface type with only a public method (Underlying) whose signature doesn't mention a named type (only string). The assignability check shouldn't involve any considerations of Named type identity.

[findleyr]

where the RHS is an interface type with only a public method (Underlying) whose signature doesn't mention a named type (only string).

types.Type is interface{ String() string; Underlying() Type }. The Named type identity that matters is the result of Underlying: a types.Type.

The CL I sent you contains a fix, along with a test for this regression as well as the preexisting inconsistency I described, and should make the oversight clear.

[adonovan]

types.Type is interface{ String() string; Underlying() Type }. The Named type identity that matters is the result of Underlying: a types.Type.

Of course, Type. I need more coffee. Or just coffee. Thanks.