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 <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>

Author: adonovan

go/packages/packages.go

@@ -258,13 +258,21 @@ type driverResponse struct {
 // 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 @@ type Package struct {
 	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 loaderPackage struct {
 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 @@ func (ld *loader) refine(response *driverResponse) ([]*Package, error) {
 		}
 	}
 
-	// Materialize the import graph.
+	// Materialize the import graph (if NeedImports).
 
 	const (
 		white = 0 // new
@@ -697,9 +704,8 @@ func (ld *loader) refine(response *driverResponse) ([]*Package, error) {
 	// 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 @@ func (ld *loader) refine(response *driverResponse) ([]*Package, error) {
 		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
+		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 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 @@ func (ld *loader) refine(response *driverResponse) ([]*Package, error) {
 		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 {

go/packages/packages_test.go

@@ -1217,6 +1217,34 @@ func testSizes(t *testing.T, exporter packagestest.Exporter) {
 	}
 }
 
+// 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.

gopls/internal/lsp/cache/load.go

@@ -413,6 +413,10 @@ func buildMetadata(updates map[PackageID]*source.Metadata, pkg *packages.Package
 		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,