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

Jump to bottom

x/tools/gopls: with Go 1.17, certain invalid packages fail to re-import. #59179

Closed
findleyr opened this issue Mar 22, 2023 · 3 comments
Closed

x/tools/gopls: with Go 1.17, certain invalid packages fail to re-import. #59179

findleyr opened this issue Mar 22, 2023 · 3 comments
Assignees
@adonovan
Labels
gopls/incremental gopls Issues related to the Go language server, gopls. okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1 release-blocker Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
gopls/v0.12.0

Comments

@findleyr
Copy link
Contributor

As we work on making gopls incremental in #57987, we rely heavily on being able to export and then re-import both valid and invalid types.Packages.

I have just encountered the first instance where this fails, but only with Go 1.17:

The following code is invalid at 1.17, but valid at 1.18:

type A struct {
	X int
}

type B interface {
	A
}

(on 1.18, B is a valid constraint interface).

Go 1.17 contained much code supporting generics behind the scenes in go/types, and so reports an error at the embedding of A, but does not record the embedded type as Typ[Invalid]. Then importing fails here:
https://cs.opensource.google/go/go/+/refs/tags/go1.17.9:src/go/types/type.go;l=574;drc=66007e5cfc9e151485791d440b7e67d44af1b29f

This is an unfortunate instance where Go 1.17 is long since not supported by the Go project, but still supported by gopls until at least Go 1.21 comes out. I think it suffices for us to let the import fail, but we need to make sure we produce accurate diagnostics for this code at Go 1.17, and get reasonable behavior in importing packages.

CC @adonovan
@findleyr findleyr added this to the gopls/v0.12.0 milestone Mar 22, 2023
@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 Mar 22, 2023
@findleyr findleyr added release-blocker okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1 labels Apr 17, 2023
@adonovan
Copy link
Member

Fortunately this bug will soon be firmly in the past, but in general I don't know what we can really do fix future occurrences of similar problems. We could backport a fix to the go1.X branch, cut a point release, and suggest that affected users upgrade to it. That might be worthwhile for the most recent value of X.

In this case, I get a diagnostic on the import of the form cannot import %q (%v), possibly version skew - reinstall package, indicating that there was a recovered panic. The "version skew" is wrong and it's not clear what "reinstall package" means. If the user misinterprets this as a suggestion to install the Debian or Homebrew package for a newer version of Go, then by mere good fortune this should actual help.

Perhaps we should clarify the warning message for recovered panics going forward. I'm not sure what else there is to do here.

@findleyr
Copy link
Contributor Author

As discussed, let's just improve this error message to not be misleading. We should not hint at "version skew" as it's not possible for gopls, and it potentially very confusing to our users.

Reassigning to @adonovan for a suitable wording :)

@gopherbot
Copy link

Change https://go.dev/cl/494676 mentions this issue: internal/gcimporter: improve error handling

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@adonovan adonovan

Labels
gopls/incremental gopls Issues related to the Go language server, gopls. okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1 release-blocker Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
gopls/v0.12.0
Development

No branches or pull requests
3 participants
@adonovan @gopherbot @findleyr