go/packages: use export data to load types info

Use export data in LoadTypes mode, when it is available to get the type information.

The lock can be moved back to loadFromExportData, as each package is
loaded in topological order, and all calls to loadFromExportData for a
particular package occur when that package is being loaded.

Fixes golang/go#26834

Change-Id: Ib6c28eb8642a473cc100d54d0aac7b90644d5d22
Reviewed-on: https://go-review.googlesource.com/128365
Reviewed-by: Michael Matloob <matloob@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>

go/packages/packages.go

@@ -128,9 +128,9 @@ type Config struct {
 
 	// TypeChecker provides additional configuration for type-checking syntax trees.
 	//
-	// The TypeCheck loader does not use the TypeChecker configuration
+	// It is used for all packages in LoadAllSyntax mode,
-	// for packages that have their type information provided by the
+	// and for the packages matching the patterns, but not their dependencies,
-	// underlying build system.
+	// in LoadSyntax mode.
 	//
 	// The TypeChecker.Error function is ignored:
 	// errors are reported using the Error function defined above.
@@ -138,9 +138,8 @@ type Config struct {
 	// The TypeChecker.Importer function is ignored:
 	// the loader defines an appropriate importer.
 	//
-	// The TypeChecker.Sizes are only used by the WholeProgram loader.
+	// TODO(rsc): TypeChecker.Sizes should use the same sizes as the main build.
-	// The TypeCheck loader uses the same sizes as the main build.
+	// Derive them from the runtime?
-	// TODO(rsc): At least, it should. Derive these from runtime?
 	TypeChecker types.Config
 }
 
@@ -232,7 +231,7 @@ type loaderPackage struct {
 	importErrors  map[string]error // maps each bad import to its error
 	loadOnce      sync.Once
 	color         uint8 // for cycle detection
-	mark, needsrc bool  // used in TypeCheck mode only
+	mark, needsrc bool  // used when Mode >= LoadTypes
 }
 
 func (lpkg *Package) String() string { return lpkg.ID }
@@ -317,7 +316,8 @@ func (ld *loader) loadFrom(roots []string, list ...*raw.Package) ([]*Package, er
 				GoFiles:    pkg.GoFiles,
 				OtherFiles: pkg.OtherFiles,
 			},
-			needsrc: ld.Mode >= LoadAllSyntax || pkg.Export == "",
+			needsrc: ld.Mode >= LoadAllSyntax ||
+				(pkg.Export == "" && pkg.PkgPath != "unsafe"),
 		}
 		ld.pkgs[lpkg.ID] = lpkg
 	}
@@ -421,11 +421,10 @@ func (ld *loader) loadFrom(roots []string, list ...*raw.Package) ([]*Package, er
 	return result, nil
 }
 
-// loadRecursive loads, parses, and type-checks the specified package and its
+// loadRecursive loads the specified package and its dependencies,
-// dependencies, recursively, in parallel, in topological order.
+// recursively, in parallel, in topological order.
 // It is atomic and idempotent.
-// Precondition: ld.mode != Metadata.
+// Precondition: ld.Mode >= LoadTypes.
-// In typeCheck mode, only needsrc packages are loaded.
 func (ld *loader) loadRecursive(lpkg *loaderPackage) {
 	lpkg.loadOnce.Do(func() {
 		// Load the direct dependencies, in parallel.
@@ -444,11 +443,10 @@ func (ld *loader) loadRecursive(lpkg *loaderPackage) {
 	})
 }
 
-// loadPackage loads, parses, and type-checks the
+// loadPackage loads the specified package.
-// files of the specified package, if needed.
 // It must be called only once per Package,
 // after immediate dependencies are loaded.
-// Precondition: ld.mode != Metadata.
+// Precondition: ld.Mode >= LoadTypes.
 func (ld *loader) loadPackage(lpkg *loaderPackage) {
 	if lpkg.raw.PkgPath == "unsafe" {
 		// Fill in the blanks to avoid surprises.
@@ -459,8 +457,14 @@ func (ld *loader) loadPackage(lpkg *loaderPackage) {
 		return
 	}
 
+	// Call NewPackage directly with explicit name.
+	// This avoids skew between golist and go/types when the files'
+	// package declarations are inconsistent.
+	lpkg.Types = types.NewPackage(lpkg.raw.PkgPath, lpkg.Name)
+
 	if !lpkg.needsrc {
-		return // not a source package
+		ld.loadFromExportData(lpkg)
+		return // not a source package, don't get syntax trees
 	}
 
 	hardErrors := false
@@ -482,11 +486,6 @@ func (ld *loader) loadPackage(lpkg *loaderPackage) {
 	lpkg.Fset = ld.Fset
 	lpkg.Syntax = files
 
-	// Call NewPackage directly with explicit name.
-	// This avoids skew between golist and go/types when the files'
-	// package declarations are inconsistent.
-	lpkg.Types = types.NewPackage(lpkg.raw.PkgPath, lpkg.Name)
-
 	lpkg.TypesInfo = &types.Info{
 		Types:      make(map[ast.Expr]types.TypeAndValue),
 		Defs:       make(map[*ast.Ident]types.Object),
@@ -516,16 +515,9 @@ func (ld *loader) loadPackage(lpkg *loaderPackage) {
 			return nil, fmt.Errorf("no metadata for %s", path)
 		}
 
-		ld.exportMu.Lock()
-		defer ld.exportMu.Unlock()
-
 		if ipkg.Types != nil && ipkg.Types.Complete() {
 			return ipkg.Types, nil
 		}
-		imp := ld.pkgs[ipkg.ID]
-		if !imp.needsrc {
-			return ld.loadFromExportData(imp)
-		}
 		log.Fatalf("internal error: nil Pkg importing %q from %q", path, lpkg)
 		panic("unreachable")
 	})
@@ -643,16 +635,8 @@ func (ld *loader) loadFromExportData(lpkg *loaderPackage) (*types.Package, error
 	// Not all accesses to Package.Pkg need to be protected by exportMu:
 	// graph ordering ensures that direct dependencies of source
 	// packages are fully loaded before the importer reads their Pkg field.
-	//
-	// TODO(matloob): Ask adonovan if this is safe. Because of diamonds in the
-	// dependency graph, it's possible that the same package is loaded in
-	// two separate goroutines and there's a race in the write of the Package's
-	// Types here and the read earlier checking if types is set before calling
-	// this function.
-	/*
 	ld.exportMu.Lock()
 	defer ld.exportMu.Unlock()
-	*/
 
 	if tpkg := lpkg.Types; tpkg != nil && tpkg.Complete() {
 		return tpkg, nil // cache hit
@@ -690,22 +674,9 @@ func (ld *loader) loadFromExportData(lpkg *loaderPackage) (*types.Package, error
 	//
 	// So, we must build a PkgPath-keyed view of the global
 	// (conceptually ID-keyed) cache of packages and pass it to
-	// gcexportdata, then copy back to the global cache any newly
+	// gcexportdata. The view must contain every existing
-	// created entries in the view map. The view must contain every
+	// package that might possibly be mentioned by the
-	// existing package that might possibly be mentioned by the
+	// current package---its transitive closure.
-	// current package---its reflexive transitive closure.
-	//
-	// (Yes, reflexive: although loadRecursive processes source
-	// packages in topological order, export data packages are
-	// processed only lazily within Importer calls. In the graph
-	// A->B->C, A->C where A is a source package and B and C are
-	// export data packages, processing of the A->B and A->C import
-	// edges may occur in either order, depending on the sequence
-	// of imports within A. If B is processed first, and its export
-	// data mentions C, an incomplete package for C will be created
-	// before processing of C.)
-	// We could do export data processing in topological order using
-	// loadRecursive, but there's no parallelism to be gained.
 	//
 	// TODO(adonovan): it would be more simpler and more efficient
 	// if the export data machinery invoked a callback to
@@ -713,22 +684,18 @@ func (ld *loader) loadFromExportData(lpkg *loaderPackage) (*types.Package, error
 	//
 	view := make(map[string]*types.Package) // view seen by gcexportdata
 	seen := make(map[*loaderPackage]bool)   // all visited packages
-	var copyback []*loaderPackage           // candidates for copying back to global cache
+	var visit func(pkgs map[string]*Package)
-	var visit func(p *loaderPackage)
+	visit = func(pkgs map[string]*Package) {
-	visit = func(p *loaderPackage) {
+		for _, p := range pkgs {
-		if !seen[p] {
+			lpkg := ld.pkgs[p.ID]
-			seen[p] = true
+			if !seen[lpkg] {
-			if p.Types != nil {
+				seen[lpkg] = true
-				view[p.raw.PkgPath] = p.Types
+				view[lpkg.raw.PkgPath] = lpkg.Types
-			} else {
+				visit(lpkg.Imports)
-				copyback = append(copyback, p)
-			}
-			for _, p := range p.Imports {
-				visit(ld.pkgs[p.ID])
 			}
 		}
 	}
-	visit(lpkg)
+	visit(lpkg.Imports)
 
 	// Parse the export data.
 	// (May create/modify packages in view.)
@@ -737,12 +704,6 @@ func (ld *loader) loadFromExportData(lpkg *loaderPackage) (*types.Package, error
 		return nil, fmt.Errorf("reading %s: %v", lpkg.raw.Export, err)
 	}
 
-	// For each newly created types.Package in the view,
-	// save it in the main graph.
-	for _, p := range copyback {
-		p.Types = view[p.raw.PkgPath] // may still be nil
-	}
-
 	lpkg.Types = tpkg
 	lpkg.IllTyped = false
 

go/packages/packages_test.go

@@ -432,13 +432,83 @@ func (ec *errCollector) add(err error) {
 	ec.mu.Unlock()
 }
 
+func TestLoadTypes(t *testing.T) {
+	// In LoadTypes and LoadSyntax modes, the compiler will
+	// fail to generate an export data file for c, because it has
+	// a type error.  The loader should fall back loading a and c
+	// from source, but use the export data for b.
+
+	tmp, cleanup := makeTree(t, map[string]string{
+		"src/a/a.go": `package a; import "b"; import "c"; const A = "a" + b.B + c.C`,
+		"src/b/b.go": `package b; const B = "b"`,
+		"src/c/c.go": `package c; const C = "c" + 1`,
+	})
+	defer cleanup()
+
+	cfg := &packages.Config{
+		Mode:  packages.LoadTypes,
+		Env:   append(os.Environ(), "GOPATH="+tmp),
+		Error: func(error) {},
+	}
+	initial, err := packages.Load(cfg, "a")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	graph, all := importGraph(initial)
+	wantGraph := `
+* a
+  b
+  c
+  a -> b
+  a -> c
+`[1:]
+	if graph != wantGraph {
+		t.Errorf("wrong import graph: got <<%s>>, want <<%s>>", graph, wantGraph)
+	}
+
+	for _, test := range []struct {
+		id         string
+		wantSyntax bool
+	}{
+		{"a", true},  // need src, no export data for c
+		{"b", false}, // use export data
+		{"c", true},  // need src, no export data for c
+	} {
+		if usesOldGolist && !test.wantSyntax {
+			// legacy go list always upgrades to LoadAllSyntax, syntax will be filled in.
+			// still check that types information is complete.
+			test.wantSyntax = true
+		}
+		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
+		} else if !p.Types.Complete() {
+			t.Errorf("incomplete types.Package for %s", p)
+		}
+		if (p.Syntax != nil) != test.wantSyntax {
+			if test.wantSyntax {
+				t.Errorf("missing ast.Files for %s", p)
+			} else {
+				t.Errorf("unexpected ast.Files for for %s", p)
+			}
+		}
+	}
+}
+
 func TestLoadSyntaxOK(t *testing.T) {
 	tmp, cleanup := makeTree(t, map[string]string{
 		"src/a/a.go": `package a; import "b"; const A = "a" + b.B`,
 		"src/b/b.go": `package b; import "c"; const B = "b" + c.C`,
 		"src/c/c.go": `package c; import "d"; const C = "c" + d.D`,
 		"src/d/d.go": `package d; import "e"; const D = "d" + e.E`,
-		"src/e/e.go": `package e; const E = "e"`,
+		"src/e/e.go": `package e; import "f"; const E = "e" + f.F`,
+		"src/f/f.go": `package f; const F = "f"`,
 	})
 	defer cleanup()
 
@@ -459,10 +529,12 @@ func TestLoadSyntaxOK(t *testing.T) {
 * c
   d
   e
+  f
   a -> b
   b -> c
   c -> d
   d -> e
+  e -> f
 `[1:]
 	if graph != wantGraph {
 		t.Errorf("wrong import graph: got <<%s>>, want <<%s>>", graph, wantGraph)
@@ -474,30 +546,29 @@ func TestLoadSyntaxOK(t *testing.T) {
 
 	for _, test := range []struct {
 		id         string
-		wantType   bool
 		wantSyntax bool
 	}{
-		{"a", true, true},   // source package
+		{"a", true},  // source package
-		{"b", true, true},   // source package
+		{"b", true},  // source package
-		{"c", true, true},   // source package
+		{"c", true},  // source package
-		{"d", true, false},  // export data package
+		{"d", false}, // export data package
-		{"e", false, false}, // no package
+		{"e", false}, // export data package
+		{"f", false}, // export data package
 	} {
-		if usesOldGolist && test.id == "d" || test.id == "e" {
+		if usesOldGolist && !test.wantSyntax {
-			// go list always upgrades  whole-program load.
+			// legacy go list always upgrades to LoadAllSyntax, syntax will be filled in.
-			continue
+			test.wantSyntax = true
 		}
 		p := all[test.id]
 		if p == nil {
 			t.Errorf("missing package: %s", test.id)
 			continue
 		}
-		if (p.Types != nil) != test.wantType {
+		if p.Types == nil {
-			if test.wantType {
 			t.Errorf("missing types.Package for %s", p)
-			} else {
+			continue
-				t.Errorf("unexpected types.Package for %s", p)
+		} else if !p.Types.Complete() {
-			}
+			t.Errorf("incomplete types.Package for %s", p)
 		}
 		if (p.Syntax != nil) != test.wantSyntax {
 			if test.wantSyntax {
@@ -513,7 +584,7 @@ func TestLoadSyntaxOK(t *testing.T) {
 
 	// Check value of constant.
 	aA := constant(all["a"], "A")
-	if got, want := fmt.Sprintf("%v %v", aA, aA.Val()), `const a.A untyped string "abcde"`; got != want {
+	if got, want := fmt.Sprintf("%v %v", aA, aA.Val()), `const a.A untyped string "abcdef"`; got != want {
 		t.Errorf("a.A: got %s, want %s", got, want)
 	}
 }
@@ -521,17 +592,15 @@ func TestLoadSyntaxOK(t *testing.T) {
 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
-	// [a-e]. Export data is only required for package d, so package
+	// [a-e]. Only f should be loaded from export data, and the rest
-	// c, which imports d, gets an error, and all packages above d
+	// should be IllTyped.
-	// are IllTyped. Package e is not ill-typed, because the user
-	// did not demand its type information (despite it actually
-	// containing a type error).
 	tmp, cleanup := makeTree(t, map[string]string{
 		"src/a/a.go": `package a; import "b"; const A = "a" + b.B`,
 		"src/b/b.go": `package b; import "c"; const B = "b" + c.C`,
 		"src/c/c.go": `package c; import "d"; const C = "c" + d.D`,
 		"src/d/d.go": `package d; import "e"; const D = "d" + e.E`,
-		"src/e/e.go": `package e; const E = "e" + 1`, // type error
+		"src/e/e.go": `package e; import "f"; const E = "e" + f.F + 1`, // type error
+		"src/f/f.go": `package f; const F = "f"`,
 	})
 	defer cleanup()
 
@@ -561,32 +630,30 @@ func TestLoadSyntaxError(t *testing.T) {
 
 	for _, test := range []struct {
 		id           string
-		wantTypes    bool
 		wantSyntax   bool
 		wantIllTyped bool
 	}{
-		{"a", true, true, true},
+		{"a", true, true},
-		{"b", true, true, true},
+		{"b", true, true},
-		{"c", true, true, true},
+		{"c", true, true},
-		{"d", false, false, true},  // missing export data
+		{"d", true, true},
-		{"e", false, false, false}, // type info not requested (despite type error)
+		{"e", true, true},
+		{"f", false, false},
 	} {
-		if usesOldGolist && test.id == "c" || test.id == "d" || test.id == "e" {
+		if usesOldGolist && !test.wantSyntax {
-			// Behavior is different for old golist because it upgrades to wholeProgram.
+			// legacy go list always upgrades to LoadAllSyntax, syntax will be filled in.
-			// TODO(matloob): can we run more of this test? Can we put export data into the test GOPATH?
+			test.wantSyntax = true
-			continue
 		}
 		p := all[test.id]
 		if p == nil {
 			t.Errorf("missing package: %s", test.id)
 			continue
 		}
-		if (p.Types != nil) != test.wantTypes {
+		if p.Types == nil {
-			if test.wantTypes {
+			t.Errorf("missing types.Package for %s", p)
-				t.Errorf("missing types.Package for %s", test.id)
+			continue
-			} else {
+		} else if !p.Types.Complete() {
-				t.Errorf("unexpected types.Package for %s", test.id)
+			t.Errorf("incomplete types.Package for %s", p)
-			}
 		}
 		if (p.Syntax != nil) != test.wantSyntax {
 			if test.wantSyntax {