diff --git a/go/loader/importer_test.go b/go/loader/importer_test.go index c6f04450..83fe24b2 100644 --- a/go/loader/importer_test.go +++ b/go/loader/importer_test.go @@ -131,15 +131,15 @@ var justXgo = [1]os.FileInfo{fakeFileInfo{}} // ["x.go"] func TestTransitivelyErrorFreeFlag(t *testing.T) { conf := loader.Config{ - AllowTypeErrors: true, - SourceImports: true, + AllowErrors: true, + SourceImports: true, } // Create an minimal custom build.Context // that fakes the following packages: // - // a --> b --> c! c has a TypeError - // \ d and e are transitively error free. + // a --> b --> c! c has an error + // \ d and e are transitively error-free. // e --> d // // Each package [a-e] consists of one file, x.go. @@ -184,12 +184,12 @@ func TestTransitivelyErrorFreeFlag(t *testing.T) { continue } - if (info.TypeError != nil) != wantErr { + if (info.Errors != nil) != wantErr { if wantErr { - t.Errorf("Package %q.TypeError = nil, want error", pkg.Path()) + t.Errorf("Package %q.Error = nil, want error", pkg.Path()) } else { - t.Errorf("Package %q has unexpected TypeError: %s", - pkg.Path(), info.TypeError) + t.Errorf("Package %q has unexpected Errors: %v", + pkg.Path(), info.Errors) } } diff --git a/go/loader/loader.go b/go/loader/loader.go index 534cfd1f..2a15359a 100644 --- a/go/loader/loader.go +++ b/go/loader/loader.go @@ -121,6 +121,10 @@ package loader // - cache the calls to build.Import so we don't do it three times per // test package. // - Thorough overhaul of package documentation. +// - Certain errors (e.g. parse error in x_test.go files, or failure to +// import an initial package) still cause Load() to fail hard. +// Fix that. (It's tricky because of the way x_test files are parsed +// eagerly.) import ( "errors" @@ -199,11 +203,11 @@ type Config struct { // leaking into the user interface. DisplayPath func(path string) string - // 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 + // If AllowErrors is true, Load will return a Program even + // if some of the its packages contained I/O, parser or type + // errors; such errors are accessible via PackageInfo.Errors. If + // false, Load will fail if any package had an error. + AllowErrors bool // CreatePkgs specifies a list of non-importable initial // packages to create. Each element specifies a list of @@ -348,12 +352,13 @@ func (conf *Config) FromArgs(args []string, xtest bool) (rest []string, err erro // specified *.go files and adds a package entry for them to // conf.CreatePkgs. // +// It fails if any file could not be loaded or parsed. +// func (conf *Config) CreateFromFilenames(path string, filenames ...string) error { - files, err := parseFiles(conf.fset(), conf.build(), nil, ".", filenames, conf.ParserMode) - if err != nil { - return err + files, errs := parseFiles(conf.fset(), conf.build(), nil, ".", filenames, conf.ParserMode) + if len(errs) > 0 { + return errs[0] } - conf.CreateFromFiles(path, files...) return nil } @@ -388,9 +393,15 @@ func (conf *Config) ImportWithTests(path string) error { conf.Import(path) // Load the external test package. - xtestFiles, err := conf.parsePackageFiles(path, 'x') + bp, err := conf.findSourcePackage(path) if err != nil { - return err + return err // package not found + } + xtestFiles, errs := conf.parsePackageFiles(bp, 'x') + if len(errs) > 0 { + // TODO(adonovan): fix: parse errors in x_test.go files + // are still catastrophic to Load(). + return errs[0] // I/O or parse error } if len(xtestFiles) > 0 { conf.CreateFromFiles(path+"_test", xtestFiles...) @@ -460,7 +471,7 @@ type importer struct { // importInfo tracks the success or failure of a single import. type importInfo struct { - info *PackageInfo // results of typechecking (including type errors) + info *PackageInfo // results of typechecking (including errors) err error // reason for failure to make a package } @@ -470,8 +481,10 @@ type importInfo struct { // 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. +// If AllowErrors is true, Load will return a Program even if some +// packages contained I/O, parser or type errors, or if dependencies +// were missing. (Such errors are accessible via PackageInfo.Errors. If +// false, Load will fail if any package had an error. // // It is an error if no packages were loaded. // @@ -498,9 +511,7 @@ 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) + return nil, err // failed to create package } prog.Imported[path] = info } @@ -508,15 +519,17 @@ func (conf *Config) Load() (*Program, error) { // 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 := imp.conf.parsePackageFiles(path, 't') - // Prefer the earlier error, if any. - if err != nil && ii.err == nil { - ii.err = err // e.g. parse error. + bp, err := conf.findSourcePackage(path) + if err != nil { + // "Can't happen" because of previous loop. + return nil, err // package not found } - typeCheckFiles(ii.info, files...) + + info := imp.imported[path].info // must be non-nil, see above + files, errs := imp.conf.parsePackageFiles(bp, 't') + info.Errors = append(info.Errors, errs...) + typeCheckFiles(info, files...) } } @@ -545,16 +558,16 @@ func (conf *Config) Load() (*Program, error) { } } - if !conf.AllowTypeErrors { + if !conf.AllowErrors { // Report errors in indirectly imported packages. var errpkgs []string for _, info := range prog.AllPackages { - if info.TypeError != nil { + if len(info.Errors) > 0 { errpkgs = append(errpkgs, info.Pkg.Path()) } } if errpkgs != nil { - return nil, fmt.Errorf("couldn't load packages due to type errors: %s", + return nil, fmt.Errorf("couldn't load packages due to errors: %s", strings.Join(errpkgs, ", ")) } } @@ -592,7 +605,7 @@ func markErrorFreePackages(allPackages map[*types.Package]*PackageInfo) { } } for _, info := range allPackages { - if info.TypeError != nil { + if len(info.Errors) > 0 { visit(info.Pkg) } } @@ -613,26 +626,28 @@ func (conf *Config) build() *build.Context { return &build.Default } +// findSourcePackage locates the specified (possibly empty) package +// using go/build logic. It returns an error if not found. +// +func (conf *Config) findSourcePackage(path string) (*build.Package, error) { + // Import(srcDir="") disables local imports, e.g. import "./foo". + bp, err := conf.build().Import(path, "", 0) + if _, ok := err.(*build.NoGoError); ok { + return bp, nil // empty directory is not an error + } + return bp, err +} + // parsePackageFiles enumerates the files belonging to package path, -// then loads, parses and returns them. +// then loads, parses and returns them, plus a list of I/O or parse +// errors that were encountered. // // 'which' indicates which files to include: // 'g': include non-test *.go source files (GoFiles + processed CgoFiles) // 't': include in-package *_test.go source files (TestGoFiles) // 'x': include external *_test.go source files. (XTestGoFiles) // -// TODO(adonovan): don't fail just because some files contain parse errors. -// -func (conf *Config) parsePackageFiles(path string, which rune) ([]*ast.File, error) { - // Import(srcDir="") disables local imports, e.g. import "./foo". - bp, err := conf.build().Import(path, "", 0) - if _, ok := err.(*build.NoGoError); ok { - return nil, nil // empty directory - } - if err != nil { - return nil, err // import failed - } - +func (conf *Config) parsePackageFiles(bp *build.Package, which rune) ([]*ast.File, []error) { var filenames []string switch which { case 'g': @@ -645,21 +660,19 @@ func (conf *Config) parsePackageFiles(path string, which rune) ([]*ast.File, err panic(which) } - files, err := parseFiles(conf.fset(), conf.build(), conf.DisplayPath, bp.Dir, filenames, conf.ParserMode) - if err != nil { - return nil, err - } + files, errs := parseFiles(conf.fset(), conf.build(), conf.DisplayPath, bp.Dir, filenames, conf.ParserMode) // Preprocess CgoFiles and parse the outputs (sequentially). if which == 'g' && bp.CgoFiles != nil { cgofiles, err := processCgoFiles(bp, conf.fset(), conf.ParserMode) if err != nil { - return nil, err + errs = append(errs, err) + } else { + files = append(files, cgofiles...) } - files = append(files, cgofiles...) } - return files, nil + return files, errs } // doImport imports the package denoted by path. @@ -694,6 +707,9 @@ func (imp *importer) doImport(imports map[string]*types.Package, path string) (* // importPackage imports the package with the given import path, plus // its dependencies. // +// On success, it returns a PackageInfo, possibly containing errors. +// importPackage returns an error if it couldn't even create the package. +// // Precondition: path != "unsafe". // func (imp *importer) importPackage(path string) (*PackageInfo, error) { @@ -741,12 +757,14 @@ func (imp *importer) importFromBinary(path string) (*PackageInfo, error) { // located by go/build. // func (imp *importer) importFromSource(path string) (*PackageInfo, error) { - files, err := imp.conf.parsePackageFiles(path, 'g') + bp, err := imp.conf.findSourcePackage(path) if err != nil { - return nil, err + return nil, err // package not found } // Type-check the package. info := imp.newPackageInfo(path) + files, errs := imp.conf.parsePackageFiles(bp, 'g') + info.Errors = append(info.Errors, errs...) typeCheckFiles(info, files...) return info, nil } @@ -755,29 +773,15 @@ func (imp *importer) importFromSource(path string) (*PackageInfo, error) { // The order of files determines the package initialization order. // It may be called multiple times. // -// Any error is stored in the info.TypeError field. +// Any error is stored in the info.Error 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 - } - } + + // Ignore the returned (first) error since we already collect them all. + _ = info.checker.Files(files) } 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{ Pkg: pkg, @@ -790,6 +794,26 @@ func (imp *importer) newPackageInfo(path string) *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) + } + tc.Import = imp.doImport // doImport wraps the user's importfn, effectively + + // The default error reporter just prints the errors. + dfltError := tc.Error + if dfltError == nil { + dfltError = func(e error) { fmt.Fprintln(os.Stderr, e) } + } + // Wrap the error reporter to also collect them. + tc.Error = func(e error) { + info.Errors = append(info.Errors, e) + dfltError(e) + } + 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 77e4b6af..467ae5ae 100644 --- a/go/loader/pkginfo.go +++ b/go/loader/pkginfo.go @@ -22,9 +22,9 @@ type PackageInfo struct { 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 + Errors []error // non-nil if the package had errors types.Info // type-checker deductions. - *types.Checker + checker *types.Checker // transient type-checker state } diff --git a/go/loader/util.go b/go/loader/util.go index a7057171..467a74ce 100644 --- a/go/loader/util.go +++ b/go/loader/util.go @@ -15,13 +15,14 @@ import ( "sync" ) -// parseFiles parses the Go source files files within directory dir -// and returns their ASTs, or the first parse error if any. +// parseFiles parses the Go source files within directory dir and +// returns the ASTs of the ones that could be at least partially parsed, +// along with a list of I/O and parse errors encountered. // // I/O is done via ctxt, which may specify a virtual file system. // displayPath is used to transform the filenames attached to the ASTs. // -func parseFiles(fset *token.FileSet, ctxt *build.Context, displayPath func(string) string, dir string, files []string, mode parser.Mode) ([]*ast.File, error) { +func parseFiles(fset *token.FileSet, ctxt *build.Context, displayPath func(string) string, dir string, files []string, mode parser.Mode) ([]*ast.File, []error) { if displayPath == nil { displayPath = func(path string) string { return path } } @@ -51,22 +52,38 @@ func parseFiles(fset *token.FileSet, ctxt *build.Context, displayPath func(strin } else { rd, err = os.Open(file) } - defer rd.Close() if err != nil { - errors[i] = err + errors[i] = err // open failed return } + + // ParseFile may return both an AST and an error. parsed[i], errors[i] = parser.ParseFile(fset, displayPath(file), rd, mode) + rd.Close() }(i, file) } wg.Wait() - for _, err := range errors { - if err != nil { - return nil, err + // Eliminate nils, preserving order. + var o int + for _, f := range parsed { + if f != nil { + parsed[o] = f + o++ } } - return parsed, nil + parsed = parsed[:o] + + o = 0 + for _, err := range errors { + if err != nil { + errors[o] = err + o++ + } + } + errors = errors[:o] + + return parsed, errors } // ---------- Internal helpers ---------- diff --git a/godoc/analysis/README b/godoc/analysis/README index a31fefc6..6913b979 100644 --- a/godoc/analysis/README +++ b/godoc/analysis/README @@ -26,9 +26,6 @@ Testing: how does one test that a web page "looks right"? Bugs ---- -(*go/loader.Program).Load fails if it encounters a single parse error. -Make this more robust. - (*ssa.Program).Create requires transitively error-free packages. We can make this more robust by making the requirement transitively free of "hard" errors; soft errors are fine. diff --git a/godoc/analysis/analysis.go b/godoc/analysis/analysis.go index ddf4dcd1..936872cc 100644 --- a/godoc/analysis/analysis.go +++ b/godoc/analysis/analysis.go @@ -36,15 +36,16 @@ // CHANNEL PEERS: for each channel operation make/<-/close, the set of // other channel ops that alias the same channel(s). // -// ERRORS: for each locus of a static (go/types) error, the location -// is highlighted in red and hover text provides the compiler error -// message. +// ERRORS: for each locus of a frontend (scanner/parser/type) error, the +// location is highlighted in red and hover text provides the compiler +// error message. // package analysis import ( "fmt" "go/build" + "go/scanner" "go/token" "html" "io" @@ -274,9 +275,13 @@ type analysis struct { pcgs map[*ssa.Package]*packageCallGraph } -// fileAndOffset returns the file and offset for a given position. +// fileAndOffset returns the file and offset for a given pos. func (a *analysis) fileAndOffset(pos token.Pos) (fi *fileInfo, offset int) { - posn := a.prog.Fset.Position(pos) + return a.fileAndOffsetPosn(a.prog.Fset.Position(pos)) +} + +// fileAndOffsetPosn returns the file and offset for a given position. +func (a *analysis) fileAndOffsetPosn(posn token.Position) (fi *fileInfo, offset int) { url := a.path2url[posn.Filename] return a.result.fileInfo(url), posn.Offset } @@ -301,15 +306,10 @@ func (a *analysis) posURL(pos token.Pos, len int) string { // func Run(pta bool, result *Result) { conf := loader.Config{ - SourceImports: true, - AllowTypeErrors: true, - } - - errors := make(map[token.Pos][]string) - conf.TypeChecker.Error = func(e error) { - err := e.(types.Error) - errors[err.Pos] = append(errors[err.Pos], err.Msg) + SourceImports: true, + AllowErrors: true, } + conf.TypeChecker.Error = func(e error) {} // silence the default error handler var roots, args []string // roots[i] ends with os.PathSeparator @@ -333,7 +333,7 @@ func Run(pta bool, result *Result) { //args = []string{"fmt"} if _, err := conf.FromArgs(args, true); err != nil { - log.Print(err) // import error + log.Printf("Analysis failed: %s\n", err) // import error return } @@ -343,9 +343,7 @@ func Run(pta bool, result *Result) { log.Printf("Loaded %d packages.", len(iprog.AllPackages)) } if err != nil { - // TODO(adonovan): loader: don't give up just because - // of one parse error. - log.Print(err) // parse error in some package + log.Printf("Analysis failed: %s\n", err) return } @@ -398,12 +396,28 @@ func Run(pta bool, result *Result) { } } - // Add links for type-checker errors. + // Add links for scanner, parser, type-checker errors. // TODO(adonovan): fix: these links can overlap with // identifier markup, causing the renderer to emit some // characters twice. - for pos, errs := range errors { - fi, offset := a.fileAndOffset(pos) + errors := make(map[token.Position][]string) + for _, info := range iprog.AllPackages { + for _, err := range info.Errors { + switch err := err.(type) { + case types.Error: + posn := a.prog.Fset.Position(err.Pos) + errors[posn] = append(errors[posn], err.Msg) + case scanner.ErrorList: + for _, e := range err { + errors[e.Pos] = append(errors[e.Pos], e.Msg) + } + default: + log.Printf("Error (%T) without position: %v\n", err, err) + } + } + } + for posn, errs := range errors { + fi, offset := a.fileAndOffsetPosn(posn) fi.addLink(errorLink{ start: offset, msg: strings.Join(errs, "\n"),