From ed45af74ff09a280906089e383aca5d202ba6698 Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Tue, 18 Feb 2014 19:43:14 -0500 Subject: [PATCH] go.tools/go/loader: add AllowTypeErrors flag. If this flag is set, (*Config).Load will not return an error even if some packages had type errors. Each individual PackageInfo can be queried for its error state, now exposed as TypeError. In addition, each PackageInfo exposes whether it is "transitively error-free". ssa.Create skips packages without this flag since it is required for SSA construction. + Test. LGTM=gri R=gri CC=golang-codereviews https://golang.org/cl/62000045 --- go/loader/importer_test.go | 81 ++++++++++++++++++++++++++++++++ go/loader/loader.go | 94 ++++++++++++++++++++++++++++++-------- go/loader/pkginfo.go | 11 +++-- go/loader/util.go | 7 ++- go/ssa/create.go | 8 ++-- 5 files changed, 174 insertions(+), 27 deletions(-) diff --git a/go/loader/importer_test.go b/go/loader/importer_test.go index 54aec5b0..8267b636 100644 --- a/go/loader/importer_test.go +++ b/go/loader/importer_test.go @@ -6,7 +6,11 @@ package loader_test import ( "fmt" + "go/ast" + "go/build" + "go/token" "sort" + "strings" "testing" "code.google.com/p/go.tools/go/loader" @@ -103,3 +107,80 @@ func TestLoadFromArgsSource(t *testing.T) { t.Errorf("loadFromArgs(%q): got %v, want [P]", prog.Created) } } + +func TestTransitivelyErrorFreeFlag(t *testing.T) { + conf := loader.Config{ + AllowTypeErrors: true, + SourceImports: true, + } + conf.Import("a") + + // Fake the following packages: + // + // a --> b --> c! c has a TypeError + // \ d and e are transitively error free. + // e --> d + + // 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") { + return nil, nil // no test/xtest files + } + var contents string + switch path { + case "a": + contents = `package a; import (_ "b"; _ "e")` + case "b": + contents = `package b; import _ "c"` + case "c": + contents = `package c; func f() { _ = int(false) }` // type error within function body + case "d": + contents = `package d;` + case "e": + contents = `package e; import _ "d"` + default: + return nil, fmt.Errorf("no such package %q", path) + } + f, err := conf.ParseFile(fmt.Sprintf("%s/x.go", path), contents, 0) + return []*ast.File{f}, err + } + defer func() { *pfn = saved }() + + prog, err := conf.Load() + if err != nil { + t.Errorf("Load failed: %s", err) + } + if prog == nil { + t.Fatalf("Load returnd nil *Program") + } + + for pkg, info := range prog.AllPackages { + var wantErr, wantTEF bool + switch pkg.Path() { + case "a", "b": + case "c": + wantErr = true + case "d", "e": + wantTEF = true + default: + t.Errorf("unexpected package: %q", pkg.Path()) + continue + } + + if (info.TypeError != nil) != wantErr { + if wantErr { + t.Errorf("Package %q.TypeError = nil, want error", pkg.Path()) + } else { + t.Errorf("Package %q has unexpected TypeError: %s", + pkg.Path(), info.TypeError) + } + } + + if info.TransitivelyErrorFree != wantTEF { + t.Errorf("Package %q.TransitivelyErrorFree=%t, want %t", + info.TransitivelyErrorFree, wantTEF) + } + } +} diff --git a/go/loader/loader.go b/go/loader/loader.go index 3fcd7f87..16c330a3 100644 --- a/go/loader/loader.go +++ b/go/loader/loader.go @@ -95,6 +95,7 @@ package loader // 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. +// - Thorough overhaul of package documentation. import ( "errors" @@ -158,6 +159,12 @@ type Config struct { // Otherwise &build.Default is used. Build *build.Context + // If AllowTypeErrors is true, Load will return a Program even + // if some of the its packages contained type errors; such + // errors are accessible via PackageInfo.TypeError. + // If false, Load will fail if any package had a type error. + AllowTypeErrors bool + // CreatePkgs specifies a list of non-importable initial // packages to create. Each element specifies a list of // parsed files to be type-checked into a new package, and a @@ -385,7 +392,7 @@ func (conf *Config) Import(path string) { // up to the AST root. It searches all ast.Files of all packages in prog. // exact is defined as for astutil.PathEnclosingInterval. // -// The result is (nil, nil, false) if not found. +// The zero value is returned if not found. // func (prog *Program) PathEnclosingInterval(start, end token.Pos) (pkg *PackageInfo, path []ast.Node, exact bool) { for _, info := range prog.AllPackages { @@ -431,8 +438,11 @@ type importInfo struct { // Load creates the initial packages specified by conf.{Create,Import}Pkgs, // loading their dependencies packages as needed. // -// On success, it returns a Program containing a PackageInfo for each -// package; all are well-typed. +// On success, Load returns a Program containing a PackageInfo for +// each package. On failure, it returns an error. +// +// If conf.AllowTypeErrors is set, a type error does not cause Load to +// fail, but is recorded in the PackageInfo.TypeError field. // // It is an error if no packages were loaded. // @@ -459,6 +469,8 @@ func (conf *Config) Load() (*Program, error) { for path := range conf.ImportPkgs { info, err := imp.importPackage(path) if err != nil { + // TODO(adonovan): don't abort Load just + // because of a parse error in one package. return nil, err // e.g. parse error (but not type error) } prog.Imported[path] = info @@ -473,19 +485,7 @@ func (conf *Config) Load() (*Program, error) { } if len(prog.Imported)+len(prog.Created) == 0 { - return nil, errors.New("no *.go source files nor packages were specified") - } - - // Report errors in indirectly imported packages. - var errpkgs []string - for _, info := range prog.AllPackages { - if info.err != nil { - errpkgs = append(errpkgs, info.Pkg.Path()) - } - } - if errpkgs != nil { - return nil, fmt.Errorf("couldn't load packages due to type errors: %s", - strings.Join(errpkgs, ", ")) + return nil, errors.New("no initial packages were specified") } // Create infos for indirectly imported packages. @@ -496,9 +496,66 @@ func (conf *Config) Load() (*Program, error) { } } + if !conf.AllowTypeErrors { + // Report errors in indirectly imported packages. + var errpkgs []string + for _, info := range prog.AllPackages { + if info.TypeError != nil { + errpkgs = append(errpkgs, info.Pkg.Path()) + } + } + if errpkgs != nil { + return nil, fmt.Errorf("couldn't load packages due to type errors: %s", + strings.Join(errpkgs, ", ")) + } + } + + markErrorFreePackages(prog.AllPackages) + return prog, nil } +// markErrorFreePackages sets the TransitivelyErrorFree flag on all +// applicable packages. +func markErrorFreePackages(allPackages map[*types.Package]*PackageInfo) { + // Build the transpose of the import graph. + importedBy := make(map[*types.Package]map[*types.Package]bool) + for P := range allPackages { + for _, Q := range P.Imports() { + clients, ok := importedBy[Q] + if !ok { + clients = make(map[*types.Package]bool) + importedBy[Q] = clients + } + clients[P] = true + } + } + + // Find all packages reachable from some error package. + reachable := make(map[*types.Package]bool) + var visit func(*types.Package) + visit = func(p *types.Package) { + if !reachable[p] { + reachable[p] = true + for q := range importedBy[p] { + visit(q) + } + } + } + for _, info := range allPackages { + if info.TypeError != nil { + visit(info.Pkg) + } + } + + // Mark the others as "transitively error-free". + for _, info := range allPackages { + if !reachable[info.Pkg] { + info.TransitivelyErrorFree = true + } + } +} + // build returns the effective build context. func (conf *Config) build() *build.Context { if conf.Build != nil { @@ -619,7 +676,7 @@ func (imp *importer) importFromSource(path string, which string) (*PackageInfo, // 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.err. +// errors; the first of these is recorded in PackageInfo.TypeError. // func (imp *importer) createPackage(path string, files ...*ast.File) *PackageInfo { info := &PackageInfo{ @@ -640,10 +697,11 @@ func (imp *importer) createPackage(path string, files ...*ast.File) *PackageInfo 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.err = tc.Check(path, imp.conf.fset(), files, &info.Info) + info.Pkg, info.TypeError = tc.Check(path, imp.conf.fset(), files, &info.Info) imp.prog.AllPackages[info.Pkg] = info return info } diff --git a/go/loader/pkginfo.go b/go/loader/pkginfo.go index bb43ff18..dba4f0c7 100644 --- a/go/loader/pkginfo.go +++ b/go/loader/pkginfo.go @@ -18,11 +18,12 @@ import ( // Not mutated once constructed. // type PackageInfo struct { - Pkg *types.Package - Importable bool // true if 'import "Pkg.Path()"' would resolve to this - Files []*ast.File // abstract syntax for the package's files - err error // non-nil if the package had static errors - types.Info // type-checker deductions. + Pkg *types.Package + Importable bool // true if 'import "Pkg.Path()"' would resolve to this + TransitivelyErrorFree bool // true if Pkg and all its dependencies are free of errors + 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. } func (info *PackageInfo) String() string { diff --git a/go/loader/util.go b/go/loader/util.go index 164ee66a..1bc4ea0c 100644 --- a/go/loader/util.go +++ b/go/loader/util.go @@ -21,7 +21,12 @@ import ( // 't': include in-package *_test.go source files (TestGoFiles) // 'x': include external *_test.go source files. (XTestGoFiles) // -func parsePackageFiles(ctxt *build.Context, fset *token.FileSet, path string, which string) ([]*ast.File, error) { +// This function is stored in a var as a concession to testing. +// TODO(adonovan): we plan to replace this hook with an exposed +// "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) { // Set the "!cgo" go/build tag, preferring (dummy) Go to // native C implementations of net.cgoLookupHost et al. ctxt2 := *ctxt diff --git a/go/ssa/create.go b/go/ssa/create.go index a16dc74f..cab8ba81 100644 --- a/go/ssa/create.go +++ b/go/ssa/create.go @@ -29,8 +29,8 @@ const ( GlobalDebug // Enable debug info for all packages ) -// Create returns a new SSA Program, creating all packages and all -// their members. +// Create returns a new SSA Program. An SSA Package is created for +// each transitively error-free package of iprog. // // Code for bodies of functions is not built until Build() is called // on the result. @@ -48,7 +48,9 @@ func Create(iprog *loader.Program, mode BuilderMode) *Program { } for _, info := range iprog.AllPackages { - prog.CreatePackage(info) + if info.TransitivelyErrorFree { + prog.CreatePackage(info) + } } return prog