From cd987a3c8316bc2b71733338491c3ed334986120 Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Tue, 11 Mar 2014 15:41:57 -0400 Subject: [PATCH] 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 | 2 +- go/loader/importer_test.go | 11 ++- go/loader/loader.go | 170 +++++++++++++++++++++---------------- go/loader/pkginfo.go | 6 +- go/loader/stdlib_test.go | 135 +++++++++++++++++++++++++++++ go/loader/util.go | 24 +++--- 6 files changed, 253 insertions(+), 95 deletions(-) create mode 100644 go/loader/stdlib_test.go diff --git a/go/loader/backdoor_test.go b/go/loader/backdoor_test.go index 816d8ef5..918546a0 100644 --- a/go/loader/backdoor_test.go +++ b/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 } diff --git a/go/loader/importer_test.go b/go/loader/importer_test.go index 8267b636..be35825d 100644 --- a/go/loader/importer_test.go +++ b/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. - if got, want := fmt.Sprint(pkgnames), "[fmt_test]"; got != want { + // All import paths may contribute tests. + 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) { - if !strings.Contains(which, "g") { + *pfn = func(_ *build.Context, fset *token.FileSet, path string, which rune) (files []*ast.File, err error) { + if which != 'g' { return nil, nil // no test/xtest files } var contents string diff --git a/go/loader/loader.go b/go/loader/loader.go index 0048be8e..c3db31f2 100644 --- a/go/loader/loader.go +++ b/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 -// 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. +// import graph over n augmented packages may contain cycles. // -// 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. +// First, all the (unaugmented) non-test packages and their +// dependencies are imported in the usual way; the loader reports an +// 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. - // - // Due to current type-checker limitations, at most one entry - // may be augmented (true). + // files in a second pass. 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 -// 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.) +// imports is the type-checker's package canonicalization map. // // 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 { - which := "g" // load *.go files - if augment { - which = "gt" // augment package by in-package *_test.go files - } - - ii.info, ii.err = imp.importFromSource(path, which) + if _, ok := imp.conf.ImportPkgs[path]; ok || imp.conf.SourceImports { + ii.info, ii.err = imp.importFromSource(path) } 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 -// package. +// located by go/build. // -func (imp *importer) importFromSource(path string, which string) (*PackageInfo, error) { - files, err := parsePackageFiles(imp.conf.build(), imp.conf.fset(), path, which) +func (imp *importer) importFromSource(path string) (*PackageInfo, error) { + 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 -// list of parsed files, importing their dependencies. It returns a -// PackageInfo containing the resulting types.Package, the ASTs, and -// other type information. -// +// typeCheckFiles adds the specified files to info and type-checks them. // The order of files determines the package initialization order. +// It may be called multiple times. // -// path will be the resulting package's Path(). -// For an ad-hoc package, this is not necessarily unique. -// -// The resulting package is accessible via AllPackages but is not -// importable, i.e. no 'import' spec can resolve to it. -// -// createPackage never fails, but the resulting package may contain type -// errors; the first of these is recorded in PackageInfo.TypeError. -// -func (imp *importer) createPackage(path string, files ...*ast.File) *PackageInfo { +// Any error is stored in the info.TypeError field. +func typeCheckFiles(info *PackageInfo, files ...*ast.File) { + info.Files = append(info.Files, files...) + if err := info.checker.Files(files); err != nil { + // Prefer the earlier error, if any. + if info.TypeError == nil { + info.TypeError = err + } + } +} + +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), }, } - - // 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 { - // 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 + info.checker = types.NewChecker(&tc, imp.conf.fset(), pkg, &info.Info) + imp.prog.AllPackages[pkg] = info return info } diff --git a/go/loader/pkginfo.go b/go/loader/pkginfo.go index 167da94d..fbd8ada6 100644 --- a/go/loader/pkginfo.go +++ b/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 { diff --git a/go/loader/stdlib_test.go b/go/loader/stdlib_test.go new file mode 100644 index 00000000..3a35dc64 --- /dev/null +++ b/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) +} diff --git a/go/loader/util.go b/go/loader/util.go index 1bc4ea0c..14bde951 100644 --- a/go/loader/util.go +++ b/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 { - var s []string - switch c { - case 'g': - s = bp.GoFiles - case 't': - s = bp.TestGoFiles - case 'x': - s = bp.XTestGoFiles - default: - panic(c) - } - filenames = append(filenames, s...) + switch which { + case 'g': + filenames = bp.GoFiles + case 't': + filenames = bp.TestGoFiles + case 'x': + filenames = bp.XTestGoFiles + default: + panic(which) } return parseFiles(fset, bp.Dir, filenames...) }