go/packages: make sure TypesSizes are requested when Types are

This code fixes a bug when a user specifies NeedTypes, which
implicitly requires TypesSizes, but TypesSizes isn't fetched,
which causes typechecking to explode.

Also fix a similar issue where NeedImports isn't implicitly
fetched for NeedDeps.

I added a TODO for a better fix, which is to have an "implicitMode"
in the loader type containing all the data that's needed as a prerequisite
for other fields. Then we can use implicitMode when fetching data,
and cfg.Mode to clear out the fields the user didn't request.

Fixes golang/go#31163

Change-Id: If3506765470af43dfb24d06fcbd31b66a623f2e0
Reviewed-on: https://go-review.googlesource.com/c/tools/+/170342
Reviewed-by: Ian Cottrell <[email protected]>

Author: matloob

go/packages/golist.go

@@ -78,7 +78,7 @@ func goListDriver(cfg *Config, patterns ...string) (*driverResponse, error) {
 	var sizes types.Sizes
 	var sizeserr error
 	var sizeswg sync.WaitGroup
-	if cfg.Mode&NeedTypesSizes != 0 {
+	if cfg.Mode&NeedTypesSizes != 0 || cfg.Mode&NeedTypes != 0 {
 		sizeswg.Add(1)
 		go func() {
 			sizes, sizeserr = getSizes(cfg)

go/packages/packages.go

@@ -420,6 +420,12 @@ type loader struct {
 	Config
 	sizes    types.Sizes
 	exportMu sync.Mutex // enforces mutual exclusion of exportdata operations
+
+	// TODO(matloob): Add an implied mode here and use that instead of mode.
+	// Implied mode would contain all the fields we need the data for so we can
+	// get the actually requested fields. We'll zero them out before returning
+	// packages to the user. This will make it easier for us to get the conditions
+	// where we need certain modes right.
 }
 
 func newLoader(cfg *Config) *loader {
@@ -563,7 +569,7 @@ func (ld *loader) refine(roots []string, list ...*Package) ([]*Package, error) {
 		return lpkg.needsrc
 	}
 
-	if ld.Mode&NeedImports == 0 {
+	if ld.Mode&(NeedImports|NeedDeps) == 0 {
 		// We do this to drop the stub import packages that we are not even going to try to resolve.
 		for _, lpkg := range initial {
 			lpkg.Imports = nil
@@ -574,7 +580,7 @@ func (ld *loader) refine(roots []string, list ...*Package) ([]*Package, error) {
 			visit(lpkg)
 		}
 	}
-	if ld.Mode&NeedDeps != 0 {
+	if ld.Mode&NeedDeps != 0 { // TODO(matloob): This is only the case if NeedTypes is also set, right?
 		for _, lpkg := range srcPkgs {
 			// Complete type information is required for the
 			// immediate dependencies of each source package.
@@ -602,46 +608,48 @@ func (ld *loader) refine(roots []string, list ...*Package) ([]*Package, error) {
 	importPlaceholders := make(map[string]*Package)
 	for i, lpkg := range initial {
 		result[i] = lpkg.Package
+	}
+	for i := range ld.pkgs {
 		// Clear all unrequested fields, for extra de-Hyrum-ization.
 		if ld.Mode&NeedName == 0 {
-			result[i].Name = ""
-			result[i].PkgPath = ""
+			ld.pkgs[i].Name = ""
+			ld.pkgs[i].PkgPath = ""
 		}
 		if ld.Mode&NeedFiles == 0 {
-			result[i].GoFiles = nil
-			result[i].OtherFiles = nil
+			ld.pkgs[i].GoFiles = nil
+			ld.pkgs[i].OtherFiles = nil
 		}
 		if ld.Mode&NeedCompiledGoFiles == 0 {
-			result[i].CompiledGoFiles = nil
+			ld.pkgs[i].CompiledGoFiles = nil
 		}
 		if ld.Mode&NeedImports == 0 {
-			result[i].Imports = nil
+			ld.pkgs[i].Imports = nil
 		}
 		if ld.Mode&NeedExportsFile == 0 {
-			result[i].ExportFile = ""
+			ld.pkgs[i].ExportFile = ""
 		}
 		if ld.Mode&NeedTypes == 0 {
-			result[i].Types = nil
-			result[i].Fset = nil
-			result[i].IllTyped = false
+			ld.pkgs[i].Types = nil
+			ld.pkgs[i].Fset = nil
+			ld.pkgs[i].IllTyped = false
 		}
 		if ld.Mode&NeedSyntax == 0 {
-			result[i].Syntax = nil
+			ld.pkgs[i].Syntax = nil
 		}
 		if ld.Mode&NeedTypesInfo == 0 {
-			result[i].TypesInfo = nil
+			ld.pkgs[i].TypesInfo = nil
 		}
 		if ld.Mode&NeedTypesSizes == 0 {
-			result[i].TypesSizes = nil
+			ld.pkgs[i].TypesSizes = nil
 		}
 		if ld.Mode&NeedDeps == 0 {
-			for j, pkg := range result[i].Imports {
+			for j, pkg := range ld.pkgs[i].Imports {
 				ph, ok := importPlaceholders[pkg.ID]
 				if !ok {
 					ph = &Package{ID: pkg.ID}
 					importPlaceholders[pkg.ID] = ph
 				}
-				result[i].Imports[j] = ph
+				ld.pkgs[i].Imports[j] = ph
 			}
 		}
 	}

go/packages/packages_test.go

@@ -576,6 +576,84 @@ func testLoadTypes(t *testing.T, exporter packagestest.Exporter) {
 	}
 }
 
+// TestLoadTypesBits is equivalent to TestLoadTypes except that it only requests
+// the types using the NeedTypes bit.
+func TestLoadTypesBits(t *testing.T) { packagestest.TestAll(t, testLoadTypesBits) }
+func testLoadTypesBits(t *testing.T, exporter packagestest.Exporter) {
+	exported := packagestest.Export(t, exporter, []packagestest.Module{{
+		Name: "golang.org/fake",
+		Files: map[string]interface{}{
+			"a/a.go": `package a; import "golang.org/fake/b"; const A = "a" + b.B`,
+			"b/b.go": `package b; import "golang.org/fake/c"; const B = "b" + c.C`,
+			"c/c.go": `package c; import "golang.org/fake/d"; const C = "c" + d.D`,
+			"d/d.go": `package d; import "golang.org/fake/e"; const D = "d" + e.E`,
+			"e/e.go": `package e; import "golang.org/fake/f"; const E = "e" + f.F`,
+			"f/f.go": `package f; const F = "f"`,
+		}}})
+	defer exported.Cleanup()
+
+	exported.Config.Mode = packages.NeedTypes | packages.NeedDeps | packages.NeedImports
+	initial, err := packages.Load(exported.Config, "golang.org/fake/a", "golang.org/fake/c")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	graph, all := importGraph(initial)
+	wantGraph := `
+* golang.org/fake/a
+  golang.org/fake/b
+* golang.org/fake/c
+  golang.org/fake/d
+  golang.org/fake/e
+  golang.org/fake/f
+  golang.org/fake/a -> golang.org/fake/b
+  golang.org/fake/b -> golang.org/fake/c
+  golang.org/fake/c -> golang.org/fake/d
+  golang.org/fake/d -> golang.org/fake/e
+  golang.org/fake/e -> golang.org/fake/f
+`[1:]
+	if graph != wantGraph {
+		t.Errorf("wrong import graph: got <<%s>>, want <<%s>>", graph, wantGraph)
+	}
+
+	for _, test := range []struct {
+		id string
+	}{
+		{"golang.org/fake/a"},
+		{"golang.org/fake/b"},
+		{"golang.org/fake/c"},
+		{"golang.org/fake/d"},
+		{"golang.org/fake/e"},
+		{"golang.org/fake/f"},
+	} {
+		p := all[test.id]
+		if p == nil {
+			t.Errorf("missing package: %s", test.id)
+			continue
+		}
+		if p.Types == nil {
+			t.Errorf("missing types.Package for %s", p)
+			continue
+		}
+		// We don't request the syntax, so we shouldn't get it.
+		if p.Syntax != nil {
+			t.Errorf("Syntax unexpectedly provided for %s", p)
+		}
+		if p.Errors != nil {
+			t.Errorf("errors in package: %s: %s", p, p.Errors)
+		}
+	}
+
+	// Check value of constant.
+	aA := constant(all["golang.org/fake/a"], "A")
+	if aA == nil {
+		t.Fatalf("a.A: got nil")
+	}
+	if got, want := fmt.Sprintf("%v %v", aA, aA.Val()), `const golang.org/fake/a.A untyped string "abcdef"`; got != want {
+		t.Errorf("a.A: got %s, want %s", got, want)
+	}
+}
+
 func TestLoadSyntaxOK(t *testing.T) { packagestest.TestAll(t, testLoadSyntaxOK) }
 func testLoadSyntaxOK(t *testing.T, exporter packagestest.Exporter) {
 	exported := packagestest.Export(t, exporter, []packagestest.Module{{