go/packages: ensure that types.Sizes is correct
This change ensures that types.Sizes information can
be computed from compiler+GOARCH if it is needed;
if not, it causes the Load to fail.
This fixes a crash in gopls whereby a bad GOARCH
causes the types.Sizes to be silently nil, in violation
of the Packages.TypesSizes contract. Gopls would
then dereference this nil.
The problem only manifests with a file=foo.go
query, as this suppresses go list's usual eager check for
valid GOOS/GOARCH during its build tag computation.
gopls relies on the file=... mode as a fall back.
Note that this change reverts my earlier doc
change today that allowed TypesSizes to be nil
if unknown. This was the wrong fix, as it
creates both a nil-dereference hazard (the
original bug), plus, if the nil Sizes is fed to
go/types.Config, it would trigger the default
(wrong) size computation.
(This is less significant to gopls because
in file=... mode, size errors are the least of
your type-error worries, but still...)
Plus, a test.
Also: simplify two trivially true conditions
in the packages.Load control flow.
Fixes golang/go#63701
Fixes golang/vscode-go#3021
Change-Id: I8d519d0d8a8c7bce6b3206a8116a150d37e74e45
Reviewed-on: https://go-review.googlesource.com/c/tools/+/537118
Reviewed-by: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
diff --git a/go/packages/packages.go b/go/packages/packages.go
index 3b6e58a..40af0ad 100644
--- a/go/packages/packages.go
+++ b/go/packages/packages.go
@@ -258,13 +258,21 @@
// proceeding with further analysis. The PrintErrors function is
// provided for convenient display of all errors.
func Load(cfg *Config, patterns ...string) ([]*Package, error) {
- l := newLoader(cfg)
- response, err := defaultDriver(&l.Config, patterns...)
+ ld := newLoader(cfg)
+ response, err := defaultDriver(&ld.Config, patterns...)
if err != nil {
return nil, err
}
- l.sizes = types.SizesFor(response.Compiler, response.Arch)
- return l.refine(response)
+
+ // If type size information is needed but unavailable.
+ // reject the whole Load since the error is the same for every package.
+ ld.sizes = types.SizesFor(response.Compiler, response.Arch)
+ if ld.sizes == nil && ld.Config.Mode&(NeedTypes|NeedTypesSizes|NeedTypesInfo) != 0 {
+ return nil, fmt.Errorf("can't determine type sizes for compiler %q on GOARCH %q",
+ response.Compiler, response.Arch)
+ }
+
+ return ld.refine(response)
}
// defaultDriver is a driver that implements go/packages' fallback behavior.
@@ -373,7 +381,6 @@
TypesInfo *types.Info
// TypesSizes provides the effective size function for types in TypesInfo.
- // It may be nil if, for example, the compiler/architecture pair is not known.
TypesSizes types.Sizes
// forTest is the package under test, if any.
@@ -554,7 +561,7 @@
type loader struct {
pkgs map[string]*loaderPackage
Config
- sizes types.Sizes // nil => unknown
+ sizes types.Sizes // non-nil if needed by mode
parseCache map[string]*parseValue
parseCacheMu sync.Mutex
exportMu sync.Mutex // enforces mutual exclusion of exportdata operations
@@ -679,7 +686,7 @@
}
}
- // Materialize the import graph.
+ // Materialize the import graph (if NeedImports).
const (
white = 0 // new
@@ -697,9 +704,8 @@
// visit returns whether the package needs src or has a transitive
// dependency on a package that does. These are the only packages
// for which we load source code.
- var stack []*loaderPackage
+ var stack, srcPkgs []*loaderPackage
var visit func(lpkg *loaderPackage) bool
- var srcPkgs []*loaderPackage
visit = func(lpkg *loaderPackage) bool {
switch lpkg.color {
case black:
@@ -710,35 +716,34 @@
lpkg.color = grey
stack = append(stack, lpkg) // push
stubs := lpkg.Imports // the structure form has only stubs with the ID in the Imports
- // If NeedImports isn't set, the imports fields will all be zeroed out.
- if ld.Mode&NeedImports != 0 {
- lpkg.Imports = make(map[string]*Package, len(stubs))
- for importPath, ipkg := range stubs {
- var importErr error
- imp := ld.pkgs[ipkg.ID]
- if imp == nil {
- // (includes package "C" when DisableCgo)
- importErr = fmt.Errorf("missing package: %q", ipkg.ID)
- } else if imp.color == grey {
- importErr = fmt.Errorf("import cycle: %s", stack)
- }
- if importErr != nil {
- if lpkg.importErrors == nil {
- lpkg.importErrors = make(map[string]error)
- }
- lpkg.importErrors[importPath] = importErr
- continue
- }
-
- if visit(imp) {
- lpkg.needsrc = true
- }
- lpkg.Imports[importPath] = imp.Package
+ lpkg.Imports = make(map[string]*Package, len(stubs))
+ for importPath, ipkg := range stubs {
+ var importErr error
+ imp := ld.pkgs[ipkg.ID]
+ if imp == nil {
+ // (includes package "C" when DisableCgo)
+ importErr = fmt.Errorf("missing package: %q", ipkg.ID)
+ } else if imp.color == grey {
+ importErr = fmt.Errorf("import cycle: %s", stack)
}
+ if importErr != nil {
+ if lpkg.importErrors == nil {
+ lpkg.importErrors = make(map[string]error)
+ }
+ lpkg.importErrors[importPath] = importErr
+ continue
+ }
+
+ if visit(imp) {
+ lpkg.needsrc = true
+ }
+ lpkg.Imports[importPath] = imp.Package
}
if lpkg.needsrc {
srcPkgs = append(srcPkgs, lpkg)
}
+ // NeedTypeSizes causes TypeSizes to be set even
+ // on packages for which types aren't needed.
if ld.Mode&NeedTypesSizes != 0 {
lpkg.TypesSizes = ld.sizes
}
@@ -758,17 +763,18 @@
for _, lpkg := range initial {
visit(lpkg)
}
- }
- if ld.Mode&NeedImports != 0 && ld.Mode&NeedTypes != 0 {
- for _, lpkg := range srcPkgs {
+
+ if ld.Mode&NeedTypes != 0 {
// Complete type information is required for the
// immediate dependencies of each source package.
- for _, ipkg := range lpkg.Imports {
- imp := ld.pkgs[ipkg.ID]
- imp.needtypes = true
+ for _, lpkg := range srcPkgs {
+ for _, ipkg := range lpkg.Imports {
+ ld.pkgs[ipkg.ID].needtypes = true
+ }
}
}
}
+
// Load type data and syntax if needed, starting at
// the initial packages (roots of the import DAG).
if ld.Mode&NeedTypes != 0 || ld.Mode&NeedSyntax != 0 {
diff --git a/go/packages/packages_test.go b/go/packages/packages_test.go
index 60fdf9f..4fb7f0b 100644
--- a/go/packages/packages_test.go
+++ b/go/packages/packages_test.go
@@ -1217,6 +1217,34 @@
}
}
+// This is a regression test for the root cause of
+// github.com/golang/vscode-go/issues/3021.
+// If types are needed (any of NeedTypes{,Info,Sizes}
+// and the types.Sizes cannot be obtained (e.g. due to a bad GOARCH)
+// then the Load operation must fail. It must not return a nil
+// TypesSizes, or use the default (wrong) size.
+//
+// We use a file=... query because it suppresses the bad-GOARCH check
+// that the go command would otherwise perform eagerly.
+// (Gopls relies on this as a fallback.)
+func TestNeedTypeSizesWithBadGOARCH(t *testing.T) {
+ testAllOrModulesParallel(t, func(t *testing.T, exporter packagestest.Exporter) {
+ exported := packagestest.Export(t, exporter, []packagestest.Module{{
+ Name: "testdata",
+ Files: map[string]interface{}{"a/a.go": `package a`}}})
+ defer exported.Cleanup()
+
+ exported.Config.Mode = packages.NeedTypesSizes // or {,Info,Sizes}
+ exported.Config.Env = append(exported.Config.Env, "GOARCH=286")
+ _, err := packages.Load(exported.Config, "file=./a/a.go")
+ got := fmt.Sprint(err)
+ want := "can't determine type sizes"
+ if !strings.Contains(got, want) {
+ t.Errorf("Load error %q does not contain substring %q", got, want)
+ }
+ })
+}
+
// TestContainsFallbackSticks ensures that when there are both contains and non-contains queries
// the decision whether to fallback to the pre-1.11 go list sticks across both sets of calls to
// go list.
diff --git a/gopls/internal/lsp/cache/load.go b/gopls/internal/lsp/cache/load.go
index 331e0e7..0ac4e5d 100644
--- a/gopls/internal/lsp/cache/load.go
+++ b/gopls/internal/lsp/cache/load.go
@@ -413,6 +413,10 @@
return
}
+ if pkg.TypesSizes == nil {
+ panic(id + ".TypeSizes is nil")
+ }
+
// Recreate the metadata rather than reusing it to avoid locking.
m := &source.Metadata{
ID: id,
