Skip to content

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

Closed
@findleyr

Description

@findleyr

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

Activity

added this to the gopls/v0.12.0 milestone on Mar 22, 2023
added
ToolsThis label describes issues relating to any tools in the x/tools repository.
goplsIssues related to the Go language server, gopls.
on Mar 22, 2023
added
okay-after-beta1Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1
on Apr 17, 2023
adonovan

adonovan commented on May 11, 2023

@adonovan
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

findleyr commented on May 12, 2023

@findleyr
MemberAuthor

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

gopherbot commented on May 12, 2023

@gopherbot
Contributor

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

added a commit that references this issue on Feb 16, 2024
12a0517
locked and limited conversation to collaborators on May 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

FrozenDueToAgeToolsThis label describes issues relating to any tools in the x/tools repository.goplsIssues related to the Go language server, gopls.gopls/incrementalokay-after-beta1Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1release-blocker

Type

No type

Projects

No projects

Relationships

None yet

    Development

    No branches or pull requests

      Participants

      @adonovan@gopherbot@findleyr

      Issue actions

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