go.tools/go/loader: allow loading of multiple test packages.

Now that go/types permits files to be added to a package
      incrementally (fixing bug 7114), this CL extends the loader
      to load and augment multiple test packages at once.

      TESTED:
      - go/loader/stdlib_test runs the type-checker on the entire
        standard library loaded from source in a single gulp, with
        each package augmented by tests.
      - Manually tested on:
        % ssadump -test -run unicode encoding/ascii85
            Both sets of tests are run (and pass).

LGTM=gri
R=gri
CC=golang-codereviews
https://golang.org/cl/61060043

go/loader/backdoor_test.go

@@ -12,6 +12,6 @@ import (
 
 // PackageLocatorFunc exposes the address of parsePackageFiles to tests.
 // This is a temporary hack until we expose a proper PackageLocator interface.
-func PackageLocatorFunc() *func(ctxt *build.Context, fset *token.FileSet, path string, which string) ([]*ast.File, error) {
+func PackageLocatorFunc() *func(ctxt *build.Context, fset *token.FileSet, path string, which rune) ([]*ast.File, error) {
 	return &parsePackageFiles
 }

go/loader/importer_test.go

@@ -10,7 +10,6 @@ import (
 	"go/build"
 	"go/token"
 	"sort"
-	"strings"
 	"testing"
 
 	"code.google.com/p/go.tools/go/loader"
@@ -56,8 +55,8 @@ func TestLoadFromArgs(t *testing.T) {
 	for _, info := range prog.Created {
 		pkgnames = append(pkgnames, info.Pkg.Path())
 	}
-	// Only the first import path (currently) contributes tests.
+	// All import paths may contribute tests.
-	if got, want := fmt.Sprint(pkgnames), "[fmt_test]"; got != want {
+	if got, want := fmt.Sprint(pkgnames), "[fmt_test errors_test]"; got != want {
 		t.Errorf("Created: got %s, want %s", got, want)
 	}
 
@@ -67,7 +66,7 @@ func TestLoadFromArgs(t *testing.T) {
 		pkgnames = append(pkgnames, path)
 	}
 	sort.Strings(pkgnames)
-	// Only the first import path (currently) contributes tests.
+	// All import paths may contribute tests.
 	if got, want := fmt.Sprint(pkgnames), "[errors fmt]"; got != want {
 		t.Errorf("Loaded: got %s, want %s", got, want)
 	}
@@ -124,8 +123,8 @@ func TestTransitivelyErrorFreeFlag(t *testing.T) {
 	// Temporary hack until we expose a principled PackageLocator.
 	pfn := loader.PackageLocatorFunc()
 	saved := *pfn
-	*pfn = func(_ *build.Context, fset *token.FileSet, path string, which string) (files []*ast.File, err error) {
+	*pfn = func(_ *build.Context, fset *token.FileSet, path string, which rune) (files []*ast.File, err error) {
-		if !strings.Contains(which, "g") {
+		if which != 'g' {
 			return nil, nil // no test/xtest files
 		}
 		var contents string

go/loader/loader.go

@@ -68,6 +68,16 @@
 // An AUGMENTED package is an importable package P plus all the
 // *_test.go files with same 'package foo' declaration as P.
 // (go/build.Package calls these files TestFiles.)
+//
+// The INITIAL packages are those specified in the configuration.  A
+// DEPENDENCY is a package loaded to satisfy an import in an initial
+// package or another dependency.
+//
+package loader
+
+// 'go test', in-package test files, and import cycles
+// ---------------------------------------------------
+//
 // An external test package may depend upon members of the augmented
 // package that are not in the unaugmented package, such as functions
 // that expose internals.  (See bufio/export_test.go for an example.)
@@ -77,24 +87,39 @@
 // The import graph over n unaugmented packages must be acyclic; the
 // import graph over n-1 unaugmented packages plus one augmented
 // package must also be acyclic.  ('go test' relies on this.)  But the
-// import graph over n augmented packages may contain cycles, and
+// import graph over n augmented packages may contain cycles.
-// currently, go/types is incapable of handling such inputs, so the
-// loader will only augment (and create an external test package
-// for) the first import path specified on the command-line.
 //
-// The INITIAL packages are those specified in the configuration.  A
+// First, all the (unaugmented) non-test packages and their
-// DEPENDENCY is a package loaded to satisfy an import in an initial
+// dependencies are imported in the usual way; the loader reports an
-// package or another dependency.
+// error if it detects an import cycle.
 //
-package loader
+// Then, each package P for which testing is desired is augmented by
+// the list P' of its in-package test files, by calling
+// (*types.Checker).Files.  This arrangement ensures that P' may
+// reference definitions within P, but P may not reference definitions
+// within P'.  Furthermore, P' may import any other package, including
+// ones that depend upon P, without an import cycle error.
+//
+// Consider two packages A and B, both of which have lists of
+// in-package test files we'll call A' and B', and which have the
+// following import graph edges:
+//    B  imports A
+//    B' imports A
+//    A' imports B
+// This last edge would be expected to create an error were it not
+// for the special type-checking discipline above.
+// Cycles of size greater than two are possible.  For example:
+//   compress/bzip2/bzip2_test.go (package bzip2)  imports "io/ioutil"
+//   io/ioutil/tempfile_test.go   (package ioutil) imports "regexp"
+//   regexp/exec_test.go          (package regexp) imports "compress/bzip2"
 
 // TODO(adonovan):
 // - (*Config).ParseFile is very handy, but feels like feature creep.
 //   (*Config).CreateFromFiles has a nasty precondition.
-// - Ideally some of this logic would move under the umbrella of
-//   go/types; see bug 7114.
 // - s/path/importPath/g to avoid ambiguity with other meanings of
 //   "path": a file name, a colon-separated directory list.
+// - cache the calls to build.Import so we don't do it three times per
+//   test package.
 // - Thorough overhaul of package documentation.
 
 import (
@@ -180,10 +205,7 @@ type Config struct {
 	// source.  The map keys are package import paths, used to
 	// locate the package relative to $GOROOT.  The corresponding
 	// values indicate whether to augment the package by *_test.go
-	// files.
+	// files in a second pass.
-	//
-	// Due to current type-checker limitations, at most one entry
-	// may be augmented (true).
 	ImportPkgs map[string]bool
 }
 
@@ -350,16 +372,8 @@ func (conf *Config) ImportWithTests(path string) error {
 	}
 	conf.Import(path)
 
-	// TODO(adonovan): due to limitations of the current type
-	// checker design, we can augment at most one package.
-	for _, augmented := range conf.ImportPkgs {
-		if augmented {
-			return nil // don't attempt a second
-		}
-	}
-
 	// Load the external test package.
-	xtestFiles, err := parsePackageFiles(conf.build(), conf.fset(), path, "x")
+	xtestFiles, err := parsePackageFiles(conf.build(), conf.fset(), path, 'x')
 	if err != nil {
 		return err
 	}
@@ -476,12 +490,29 @@ func (conf *Config) Load() (*Program, error) {
 		prog.Imported[path] = info
 	}
 
+	// Now augment those packages that need it.
+	for path, augment := range conf.ImportPkgs {
+		if augment {
+			ii := imp.imported[path]
+
+			// Find and create the actual package.
+			files, err := parsePackageFiles(imp.conf.build(), imp.conf.fset(), path, 't')
+			// Prefer the earlier error, if any.
+			if err != nil && ii.err == nil {
+				ii.err = err // e.g. parse error.
+			}
+			typeCheckFiles(ii.info, files...)
+		}
+	}
+
 	for _, create := range conf.CreatePkgs {
 		path := create.Path
 		if create.Path == "" && len(create.Files) > 0 {
 			path = create.Files[0].Name.Name
 		}
-		prog.Created = append(prog.Created, imp.createPackage(path, create.Files...))
+		info := imp.newPackageInfo(path)
+		typeCheckFiles(info, create.Files...)
+		prog.Created = append(prog.Created, info)
 	}
 
 	if len(prog.Imported)+len(prog.Created) == 0 {
@@ -491,8 +522,11 @@ func (conf *Config) Load() (*Program, error) {
 	// Create infos for indirectly imported packages.
 	// e.g. incomplete packages without syntax, loaded from export data.
 	for _, obj := range prog.ImportMap {
-		if prog.AllPackages[obj] == nil {
+		info := prog.AllPackages[obj]
+		if info == nil {
 			prog.AllPackages[obj] = &PackageInfo{Pkg: obj, Importable: true}
+		} else {
+			info.checker = nil // finished
 		}
 	}
 
@@ -567,10 +601,7 @@ func (conf *Config) build() *build.Context {
 // doImport imports the package denoted by path.
 // It implements the types.Importer signature.
 //
-// imports is the import map of the importing package, later
+// imports is the type-checker's package canonicalization map.
-// accessible as types.Package.Imports().  If non-nil, doImport will
-// update it to include this import.  (It may be nil in recursive
-// calls for prefetching.)
 //
 // It returns an error if a package could not be created
 // (e.g. go/build or parse error), but type errors are reported via
@@ -607,19 +638,12 @@ func (imp *importer) importPackage(path string) (*PackageInfo, error) {
 		// In preorder, initialize the map entry to a cycle
 		// error in case importPackage(path) is called again
 		// before the import is completed.
-		// TODO(adonovan): go/types should be responsible for
-		// reporting cycles; see bug 7114.
 		ii = &importInfo{err: fmt.Errorf("import cycle in package %s", path)}
 		imp.imported[path] = ii
 
 		// Find and create the actual package.
-		if augment, ok := imp.conf.ImportPkgs[path]; ok || imp.conf.SourceImports {
+		if _, ok := imp.conf.ImportPkgs[path]; ok || imp.conf.SourceImports {
-			which := "g" // load *.go files
+			ii.info, ii.err = imp.importFromSource(path)
-			if augment {
-				which = "gt" // augment package by in-package *_test.go files
-			}
-
-			ii.info, ii.err = imp.importFromSource(path, which)
 		} else {
 			ii.info, ii.err = imp.importFromBinary(path)
 		}
@@ -650,37 +674,49 @@ func (imp *importer) importFromBinary(path string) (*PackageInfo, error) {
 }
 
 // importFromSource implements package loading by parsing Go source files
-// located by go/build.  which indicates which files to include in the
+// located by go/build.
-// package.
 //
-func (imp *importer) importFromSource(path string, which string) (*PackageInfo, error) {
+func (imp *importer) importFromSource(path string) (*PackageInfo, error) {
-	files, err := parsePackageFiles(imp.conf.build(), imp.conf.fset(), path, which)
+	files, err := parsePackageFiles(imp.conf.build(), imp.conf.fset(), path, 'g')
 	if err != nil {
 		return nil, err
 	}
 	// Type-check the package.
-	return imp.createPackage(path, files...), nil
+	info := imp.newPackageInfo(path)
+	typeCheckFiles(info, files...)
+	return info, nil
 }
 
-// createPackage creates and type-checks a package from the specified
+// typeCheckFiles adds the specified files to info and type-checks them.
-// list of parsed files, importing their dependencies.  It returns a
-// PackageInfo containing the resulting types.Package, the ASTs, and
-// other type information.
-//
 // The order of files determines the package initialization order.
+// It may be called multiple times.
 //
-// path will be the resulting package's Path().
+// Any error is stored in the info.TypeError field.
-// For an ad-hoc package, this is not necessarily unique.
+func typeCheckFiles(info *PackageInfo, files ...*ast.File) {
-//
+	info.Files = append(info.Files, files...)
-// The resulting package is accessible via AllPackages but is not
+	if err := info.checker.Files(files); err != nil {
-// importable, i.e. no 'import' spec can resolve to it.
+		// Prefer the earlier error, if any.
-//
+		if info.TypeError == nil {
-// createPackage never fails, but the resulting package may contain type
+			info.TypeError = err
-// errors; the first of these is recorded in PackageInfo.TypeError.
+		}
-//
+	}
-func (imp *importer) createPackage(path string, files ...*ast.File) *PackageInfo {
+}
+
+func (imp *importer) newPackageInfo(path string) *PackageInfo {
+	// Use a copy of the types.Config so we can vary IgnoreFuncBodies.
+	tc := imp.conf.TypeChecker
+	tc.IgnoreFuncBodies = false
+	if f := imp.conf.TypeCheckFuncBodies; f != nil {
+		tc.IgnoreFuncBodies = !f(path)
+	}
+	if tc.Error == nil {
+		tc.Error = func(e error) { fmt.Fprintln(os.Stderr, e) }
+	}
+	tc.Import = imp.doImport // doImport wraps the user's importfn, effectively
+
+	pkg := types.NewPackage(path, "")
 	info := &PackageInfo{
-		Files: files,
+		Pkg: pkg,
 		Info: types.Info{
 			Types:      make(map[ast.Expr]types.TypeAndValue),
 			Defs:       make(map[*ast.Ident]types.Object),
@@ -690,19 +726,7 @@ func (imp *importer) createPackage(path string, files ...*ast.File) *PackageInfo
 			Selections: make(map[*ast.SelectorExpr]*types.Selection),
 		},
 	}
-
+	info.checker = types.NewChecker(&tc, imp.conf.fset(), pkg, &info.Info)
-	// Use a copy of the types.Config so we can vary IgnoreFuncBodies.
+	imp.prog.AllPackages[pkg] = info
-	tc := imp.conf.TypeChecker
-	tc.IgnoreFuncBodies = false
-	if f := imp.conf.TypeCheckFuncBodies; f != nil {
-		tc.IgnoreFuncBodies = !f(path)
-	}
-	if tc.Error == nil {
-		// TODO(adonovan): also save errors in the PackageInfo?
-		tc.Error = func(e error) { fmt.Fprintln(os.Stderr, e) }
-	}
-	tc.Import = imp.doImport // doImport wraps the user's importfn, effectively
-	info.Pkg, info.TypeError = tc.Check(path, imp.conf.fset(), files, &info.Info)
-	imp.prog.AllPackages[info.Pkg] = info
 	return info
 }

go/loader/pkginfo.go

@@ -15,7 +15,7 @@ import (
 // PackageInfo holds the ASTs and facts derived by the type-checker
 // for a single package.
 //
-// Not mutated once constructed.
+// Not mutated once exposed via the API.
 //
 type PackageInfo struct {
 	Pkg                   *types.Package
@@ -24,6 +24,10 @@ type PackageInfo struct {
 	Files                 []*ast.File // abstract syntax for the package's files
 	TypeError             error       // non-nil if the package had type errors
 	types.Info                        // type-checker deductions.
+
+	checker interface {
+		Files(files []*ast.File) error
+	} // transient type-checker state
 }
 
 func (info *PackageInfo) String() string {

go/loader/stdlib_test.go

@@ -0,0 +1,135 @@
+// Copyright 2013 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package loader_test
+
+// This file enumerates all packages beneath $GOROOT, loads them, plus
+// their external tests if any, runs the type checker on them, and
+// prints some summary information.
+//
+// Run test with GOMAXPROCS=8.
+
+import (
+	"fmt"
+	"go/ast"
+	"go/token"
+	"os"
+	"path/filepath"
+	"runtime"
+	"strings"
+	"testing"
+	"time"
+
+	"code.google.com/p/go.tools/go/loader"
+	"code.google.com/p/go.tools/go/types"
+)
+
+func allPackages() []string {
+	var pkgs []string
+	root := filepath.Join(runtime.GOROOT(), "src/pkg") + string(os.PathSeparator)
+	filepath.Walk(root, func(path string, info os.FileInfo, err error) error {
+		// Prune the search if we encounter any of these names:
+		switch filepath.Base(path) {
+		case "testdata", ".hg":
+			return filepath.SkipDir
+		}
+		if info.IsDir() {
+			pkg := strings.TrimPrefix(path, root)
+			switch pkg {
+			case "builtin", "pkg":
+				return filepath.SkipDir // skip these subtrees
+			case "":
+				return nil // ignore root of tree
+			}
+			pkgs = append(pkgs, pkg)
+		}
+
+		return nil
+	})
+	return pkgs
+}
+
+func TestStdlib(t *testing.T) {
+	runtime.GC()
+	t0 := time.Now()
+	var memstats runtime.MemStats
+	runtime.ReadMemStats(&memstats)
+	alloc := memstats.Alloc
+
+	// Load, parse and type-check the program.
+	var conf loader.Config
+	for _, path := range allPackages() {
+		if err := conf.ImportWithTests(path); err != nil {
+			t.Error(err)
+		}
+	}
+
+	prog, err := conf.Load()
+	if err != nil {
+		t.Fatalf("Load failed: %v", err)
+	}
+
+	t1 := time.Now()
+	runtime.GC()
+	runtime.ReadMemStats(&memstats)
+
+	numPkgs := len(prog.AllPackages)
+	if want := 206; numPkgs < want {
+		t.Errorf("Loaded only %d packages, want at least %d", numPkgs, want)
+	}
+
+	// Dump package members.
+	if false {
+		for pkg := range prog.AllPackages {
+			fmt.Printf("Package %s:\n", pkg.Path())
+			scope := pkg.Scope()
+			for _, name := range scope.Names() {
+				if ast.IsExported(name) {
+					fmt.Printf("\t%s\n", types.ObjectString(pkg, scope.Lookup(name)))
+				}
+			}
+			fmt.Println()
+		}
+	}
+
+	// Check that Test functions for io/ioutil, regexp and
+	// compress/bzip2 are all simultaneously present.
+	// (The apparent cycle formed when augmenting all three of
+	// these packages by their tests was the original motivation
+	// for reporting b/7114.)
+	//
+	// compress/bzip2.TestBitReader in bzip2_test.go    imports io/ioutil
+	// io/ioutil.TestTempFile       in tempfile_test.go imports regexp
+	// regexp.TestRE2Search         in exec_test.go     imports compress/bzip2
+	for _, test := range []struct{ pkg, fn string }{
+		{"io/ioutil", "TestTempFile"},
+		{"regexp", "TestRE2Search"},
+		{"compress/bzip2", "TestBitReader"},
+	} {
+		info := prog.Imported[test.pkg]
+		if info == nil {
+			t.Errorf("failed to load package %q", test.pkg)
+			continue
+		}
+		obj, _ := info.Pkg.Scope().Lookup(test.fn).(*types.Func)
+		if obj == nil {
+			t.Errorf("package %q has no func %q", test.pkg, test.fn)
+			continue
+		}
+	}
+
+	// Dump some statistics.
+
+	// determine line count
+	var lineCount int
+	prog.Fset.Iterate(func(f *token.File) bool {
+		lineCount += f.LineCount()
+		return true
+	})
+
+	t.Log("GOMAXPROCS:           ", runtime.GOMAXPROCS(0))
+	t.Log("#Source lines:        ", lineCount)
+	t.Log("Load/parse/typecheck: ", t1.Sub(t0))
+	t.Log("#MB:                  ", int64(memstats.Alloc-alloc)/1000000)
+}

go/loader/util.go

@@ -26,7 +26,7 @@ import (
 // "PackageLocator" interface for use proprietary build sytems that
 // are incompatible with "go test", and also for testing.
 //
-var parsePackageFiles = func(ctxt *build.Context, fset *token.FileSet, path string, which string) ([]*ast.File, error) {
+var parsePackageFiles = func(ctxt *build.Context, fset *token.FileSet, path string, which rune) ([]*ast.File, error) {
 	// Set the "!cgo" go/build tag, preferring (dummy) Go to
 	// native C implementations of net.cgoLookupHost et al.
 	ctxt2 := *ctxt
@@ -42,19 +42,15 @@ var parsePackageFiles = func(ctxt *build.Context, fset *token.FileSet, path stri
 	}
 
 	var filenames []string
-	for _, c := range which {
+	switch which {
-		var s []string
-		switch c {
 	case 'g':
-			s = bp.GoFiles
+		filenames = bp.GoFiles
 	case 't':
-			s = bp.TestGoFiles
+		filenames = bp.TestGoFiles
 	case 'x':
-			s = bp.XTestGoFiles
+		filenames = bp.XTestGoFiles
 	default:
-			panic(c)
+		panic(which)
-		}
-		filenames = append(filenames, s...)
 	}
 	return parseFiles(fset, bp.Dir, filenames...)
 }