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: types.IsInterface "Named.check == nil but type is incomplete" panic in methodsets.EnsurePointer #69537

Open
adonovan opened this issue Sep 19, 2024 · 5 comments
Labels
gopls/telemetry-wins 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 Sep 19, 2024

#!stacks
"runtime.gopanic" && "types.IsInterface" && "methodsets.EnsurePointer:+1"

Issue created by stacks.

The Named type passed to IsInterface has an Underlying type that is also a Named (indicating type checking is incomplete) yet the original Named (which may or may not be the same) has Named.check==nil (indicating type checking should be complete). This seems like a go/types bug.

This stack pY1Q7g was reported by telemetry:

golang.org/x/tools/gopls@v0.16.1 go1.23.0 linux/amd64 neovim (1)
@adonovan adonovan added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository. gopls/telemetry-wins labels Sep 19, 2024
@gopherbot gopherbot added this to the Unreleased milestone Sep 19, 2024
@adonovan adonovan changed the title x/tools/gopls: types.Interface "Named.check == nil but type is incomplete" panic in methodsets.EnsurePointer x/tools/gopls: types.IsInterface "Named.check == nil but type is incomplete" panic in methodsets.EnsurePointer Sep 19, 2024
@adonovan
Copy link
Member Author

cc: @griesemer @timothy-king

@findleyr
Copy link
Contributor

This is liable to be very hard to track down without a reproducer. I'm glad we have this assertion.

Some brief notes, from refreshing my memory:

  • Named types are only created in go/types via Checker.newNamed and Checker.newNamedInstance, both of which should register the type as needing cleanup.
  • At cleanup time (at the end of type checking), non-instantiated named types should have under called with a non-nil checker. Unresolved instances are skipped if their underlying has not yet been resolved, but their origin should be cleaned up, so when they do resolve their underlying it should be substituted in the origin underlying, which will not be a Named type.
  • Importers use types.NewNamed, which panic if the underlying is named.

So, once type checking or importing exits, all types should either have non-Named underlying, or be an unexpanded instance of a type that has a non-Named underlying. That's how it's supposed to work. Aliases add a layer of complexity, but shouldn't be relevant to gopls@v0.16.1 (which does not have the Go 1.23.1 language version).

Therefore, it's concerning that there must really be something we've missed here. One possibility is a path to newNamed or newNamedInstance during type checking where the Checker is actually nil (unfortunately, many methods on Checker accept a nil receiver). However I do not see such a path.

A good next step might be to add some earlier assertions of the invariants described above.

@findleyr
Copy link
Contributor

Another potential category of bug would be some sort of ordering bug, such that the instance is expanded after it has been cleaned up. But this logic is meant to prevent such a scenario: whenever the named type is expanded, it is verified that it either has non-named underlying OR is still yet to be cleaned up (check != nil => not cleaned up yet).

@findleyr
Copy link
Contributor

Ah, I wonder if this could actually be a dupe of #68877. Could it be that someone built gopls with the explicit GODEBUG=gotypesalias=1 setting?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gopls/telemetry-wins 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

4 participants