x/tools/gopls: missing interfaces in 'implementations'
adonovan opened this issue · 9 comments
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
Intriguingly this case reduces to types.AssignableTo("*types.Struct", "types.Type") returning false. The plot thickens...
🍿 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.
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.
Change https://go.dev/cl/581875 mentions this issue: gopls/internal/golang: fix resolution of in-package implementations
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).
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.
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.
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.
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.