go/packages: reduce the number of export data files loaded

In ModeTypes, we only need the roots, in ModeSyntax we need the direct
dependancies.
We now do exactly what we need rather than the entire set of transitive
dependancies.
Without this change tools operating on source of a deep dependancy tree are far
to slow.

Change-Id: I3ddbe660709184758ad33ab7cdea860244433ce6
Reviewed-on: https://go-review.googlesource.com/129355
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>

go/packages/packages.go

@@ -38,7 +38,8 @@ const (
 	// Package fields added: Imports.
 	LoadImports
 
-	// LoadTypes adds type information for the package's exported symbols.
+	// LoadTypes adds type information for the package's exported symbols for
+	// packages matching the patterns.
 	// Package fields added: Types, Fset, IllTyped.
 	// This will use the type information provided by the build system if
 	// possible, and the ExportFile field may be filled in.
@@ -50,7 +51,8 @@ const (
 
 	// LoadAllSyntax adds typed syntax trees for the packages matching the patterns
 	// and all dependencies.
-	// Package fields added: Syntax, TypesInfo, for all packages in import graph.
+	// Package fields added: Types, Fset, IllTyped, Syntax, TypesInfo, for all
+	// packages in import graph.
 	LoadAllSyntax
 )
 
@@ -218,24 +220,22 @@ type Package struct {
 	Imports map[string]*Package
 
 	// Types is the type information for the package.
-	// Modes LoadTypes and above set this field for all packages.
+	// Modes LoadTypes and above set this field for packages matching the
-	//
+	// patterns; type information for dependencies may be missing or incomplete.
-	// TODO(adonovan): all packages? In Types mode this entails
+	// Mode LoadSyntaxAll sets this field for all packages, including dependencies.
-	// asymptotically more export data processing than is required
-	// to load the requested packages. Is that what we want?
 	Types *types.Package
 
 	// Fset provides position information for Types, TypesInfo, and Syntax.
-	// Modes LoadTypes and above set this field for all packages.
+	// It is set only when Types is set.
 	Fset *token.FileSet
 
 	// IllTyped indicates whether the package has any type errors.
-	// Modes LoadTypes and above set this field for all packages.
+	// It is set only when Types is set.
 	IllTyped bool
 
 	// Syntax is the package's syntax trees, for the files listed in GoFiles.
 	//
-	// Mode LoadSyntax set this field for packages matching the patterns.
+	// Mode LoadSyntax sets this field for packages matching the patterns.
 	// Mode LoadSyntaxAll sets this field for all packages, including dependencies.
 	Syntax []*ast.File
 
@@ -349,6 +349,7 @@ type loaderPackage struct {
 	loadOnce     sync.Once
 	color        uint8 // for cycle detection
 	needsrc      bool  // load from source (Mode >= LoadTypes)
+	needtypes    bool  // type information is either requested or depended on
 	initial      bool  // package was matched by a pattern
 }
 
@@ -413,9 +414,11 @@ func (ld *loader) refine(roots []string, list ...*Package) ([]*Package, error) {
 	for _, pkg := range list {
 		lpkg := &loaderPackage{
 			Package: pkg,
+			needtypes: ld.Mode >= LoadAllSyntax ||
+				ld.Mode >= LoadTypes && isRoot[pkg.ID],
 			needsrc: ld.Mode >= LoadAllSyntax ||
-				(ld.Mode >= LoadSyntax && isRoot[pkg.ID]) ||
+				ld.Mode >= LoadSyntax && isRoot[pkg.ID] ||
-				(pkg.ExportFile == "" && pkg.PkgPath != "unsafe"),
+				pkg.ExportFile == "" && pkg.PkgPath != "unsafe",
 		}
 		ld.pkgs[lpkg.ID] = lpkg
 		if isRoot[lpkg.ID] {
@@ -444,6 +447,7 @@ func (ld *loader) refine(roots []string, list ...*Package) ([]*Package, error) {
 	// for which we load source code.
 	var stack []*loaderPackage
 	var visit func(lpkg *loaderPackage) bool
+	var srcPkgs []*loaderPackage
 	visit = func(lpkg *loaderPackage) bool {
 		switch lpkg.color {
 		case black:
@@ -477,7 +481,9 @@ func (ld *loader) refine(roots []string, list ...*Package) ([]*Package, error) {
 			}
 			lpkg.Imports[importPath] = imp.Package
 		}
-
+		if lpkg.needsrc {
+			srcPkgs = append(srcPkgs, lpkg)
+		}
 		stack = stack[:len(stack)-1] // pop
 		lpkg.color = black
 
@@ -495,6 +501,14 @@ func (ld *loader) refine(roots []string, list ...*Package) ([]*Package, error) {
 			visit(lpkg)
 		}
 	}
+	for _, lpkg := range srcPkgs {
+		// 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
+		}
+	}
 	// Load type data if needed, starting at
 	// the initial packages (roots of the import DAG).
 	if ld.Mode >= LoadTypes {
@@ -557,6 +571,15 @@ func (ld *loader) loadPackage(lpkg *loaderPackage) {
 	// package declarations are inconsistent.
 	lpkg.Types = types.NewPackage(lpkg.PkgPath, lpkg.Name)
 
+	// Subtle: we populate all Types fields with an empty Package
+	// before loading export data so that export data processing
+	// never has to create a types.Package for an indirect dependency,
+	// which would then require that such created packages be explicitly
+	// inserted back into the Import graph as a final step after export data loading.
+	// The Diamond test exercises this case.
+	if !lpkg.needtypes {
+		return
+	}
 	if !lpkg.needsrc {
 		ld.loadFromExportData(lpkg)
 		return // not a source package, don't get syntax trees
@@ -783,7 +806,11 @@ func (ld *loader) loadFromExportData(lpkg *loaderPackage) (*types.Package, error
 	// package that might possibly be mentioned by the
 	// current package---its transitive closure.
 	//
-	// TODO(adonovan): it would be more simpler and more efficient
+	// In loadPackage, we unconditionally create a types.Package for
+	// each dependency so that export data loading does not
+	// create new ones.
+	//
+	// TODO(adonovan): it would be simpler and more efficient
 	// if the export data machinery invoked a callback to
 	// get-or-create a package instead of a map.
 	//
@@ -802,12 +829,16 @@ func (ld *loader) loadFromExportData(lpkg *loaderPackage) (*types.Package, error
 	}
 	visit(lpkg.Imports)
 
+	viewLen := len(view) + 1 // adding the self package
 	// Parse the export data.
-	// (May create/modify packages in view.)
+	// (May modify incomplete packages in view but not create new ones.)
 	tpkg, err := gcexportdata.Read(r, ld.Fset, view, lpkg.PkgPath)
 	if err != nil {
 		return nil, fmt.Errorf("reading %s: %v", lpkg.ExportFile, err)
 	}
+	if viewLen != len(view) {
+		log.Fatalf("Unexpected package creation during export data loading")
+	}
 
 	lpkg.Types = tpkg
 	lpkg.IllTyped = false

go/packages/packages_test.go

@@ -666,15 +666,16 @@ func TestLoadSyntaxOK(t *testing.T) {
 	// Can we simulate its existence?
 
 	for _, test := range []struct {
-		id         string
+		id           string
-		wantSyntax bool
+		wantSyntax   bool
+		wantComplete bool
 	}{
-		{"a", true},  // source package
+		{"a", true, true},   // source package
-		{"b", true},  // source package
+		{"b", true, true},   // source package
-		{"c", true},  // source package
+		{"c", true, true},   // source package
-		{"d", false}, // export data package
+		{"d", false, true},  // export data package
-		{"e", false}, // export data package
+		{"e", false, false}, // export data package
-		{"f", false}, // export data package
+		{"f", false, false}, // export data package
 	} {
 		if usesOldGolist && !test.wantSyntax {
 			// legacy go list always upgrades to LoadAllSyntax, syntax will be filled in.
@@ -688,8 +689,12 @@ func TestLoadSyntaxOK(t *testing.T) {
 		if p.Types == nil {
 			t.Errorf("missing types.Package for %s", p)
 			continue
-		} else if !p.Types.Complete() {
+		} else if p.Types.Complete() != test.wantComplete {
-			t.Errorf("incomplete types.Package for %s", p)
+			if test.wantComplete {
+				t.Errorf("incomplete types.Package for %s", p)
+			} else {
+				t.Errorf("unexpected complete types.Package for %s", p)
+			}
 		}
 		if (p.Syntax != nil) != test.wantSyntax {
 			if test.wantSyntax {
@@ -710,6 +715,62 @@ func TestLoadSyntaxOK(t *testing.T) {
 	}
 }
 
+func TestLoadDiamondTypes(t *testing.T) {
+	// We make a diamond dependency and check the type d.D is the same through both paths
+	tmp, cleanup := makeTree(t, map[string]string{
+		"src/a/a.go": `package a; import ("b"; "c"); var _ = b.B == c.C`,
+		"src/b/b.go": `package b; import "d"; var B d.D`,
+		"src/c/c.go": `package c; import "d"; var C d.D`,
+		"src/d/d.go": `package d; type D int`,
+	})
+	defer cleanup()
+
+	cfg := &packages.Config{
+		Mode: packages.LoadSyntax,
+		Env:  append(os.Environ(), "GOPATH="+tmp, "GO111MODULE=off"),
+		Error: func(err error) {
+			t.Errorf("Error during load: %v", err)
+		},
+	}
+	initial, err := packages.Load(cfg, "a")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	var visit func(pkg *packages.Package)
+	seen := make(map[string]bool)
+	visit = func(pkg *packages.Package) {
+		if seen[pkg.ID] {
+			return
+		}
+		seen[pkg.ID] = true
+		for _, err := range pkg.Errors {
+			t.Errorf("Error on package %v: %v", pkg.ID, err)
+		}
+		for _, imp := range pkg.Imports {
+			visit(imp)
+		}
+	}
+	for _, pkg := range initial {
+		visit(pkg)
+	}
+
+	graph, _ := importGraph(initial)
+	wantGraph := `
+* a
+  b
+  c
+  d
+  a -> b
+  a -> c
+  b -> d
+  c -> d
+`[1:]
+	if graph != wantGraph {
+		t.Errorf("wrong import graph: got <<%s>>, want <<%s>>", graph, wantGraph)
+	}
+}
+
 func TestLoadSyntaxError(t *testing.T) {
 	// A type error in a lower-level package (e) prevents go list
 	// from producing export data for all packages that depend on it