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

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