Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
adonovan opened this issue Apr 25, 2024 · 9 comments
Closed

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

adonovan opened this issue Apr 25, 2024 · 9 comments
Assignees
Labels
gopls Issues related to the Go language server, gopls. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@adonovan
Copy link
Member

adonovan commented Apr 25, 2024

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

@gopherbot gopherbot added Tools This label describes issues relating to any tools in the x/tools repository. gopls Issues related to the Go language server, gopls. labels Apr 25, 2024
@gopherbot gopherbot added this to the Unreleased milestone Apr 25, 2024
@adonovan adonovan added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 25, 2024
@adonovan adonovan self-assigned this Apr 25, 2024
@adonovan adonovan modified the milestones: Unreleased, gopls/v0.16.0 Apr 25, 2024
@adonovan
Copy link
Member Author

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

@findleyr
Copy link
Contributor

🍿 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
Copy link
Contributor

findleyr commented Apr 25, 2024

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.

@findleyr findleyr assigned findleyr and unassigned adonovan Apr 26, 2024
@gopherbot
Copy link

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

@findleyr
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Member Author

adonovan commented Apr 26, 2024

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
Copy link
Contributor

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
Copy link
Member Author

adonovan commented Apr 26, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gopls Issues related to the Go language server, gopls. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

3 participants