go/loader: make (*Config).Load() robust against I/O, scanner and parser errors.

Before, Load() would just fail.  Now, it gathers all frontend
errors (not just the first go/types error) in PackageInfo.Errors.

There are still cases where Load() can fail hard, e.g. errors in x_test.go
files.  That case is trickier to fix and remains a TODO item.

Also, make godoc display all scanner/parser/type errors in the source view.

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

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,
+		AllowErrors:   true,
 		SourceImports: true,
 	}
 
 	// Create an minimal custom build.Context
 	// that fakes the following packages:
 	//
-	// a --> b --> c!   c has a TypeError
+	// a --> b --> c!   c has an error
-	//   \              d and e are transitively error free.
+	//   \              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",
+				t.Errorf("Package %q has unexpected Errors: %v",
-					pkg.Path(), info.TypeError)
+					pkg.Path(), info.Errors)
 			}
 		}
 

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 AllowErrors is true, Load will return a Program even
-	// if some of the its packages contained type errors; such
+	// if some of the its packages contained I/O, parser or type
-	// errors are accessible via PackageInfo.TypeError.
+	// errors; such errors are accessible via PackageInfo.Errors.  If
-	// If false, Load will fail if any package had a type error.
+	// false, Load will fail if any package had an error.
-	AllowTypeErrors bool
+	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)
+	files, errs := parseFiles(conf.fset(), conf.build(), nil, ".", filenames, conf.ParserMode)
-	if err != nil {
+	if len(errs) > 0 {
-		return err
+		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
+// If AllowErrors is true, Load will return a Program even if some
-// fail, but is recorded in the PackageInfo.TypeError field.
+// 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
+			return nil, err // failed to create package
-			// because of a parse error in one package.
-			return nil, err // e.g. parse error (but not type error)
 		}
 		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')
+			bp, err := conf.findSourcePackage(path)
-			// Prefer the earlier error, if any.
+			if err != nil {
-			if err != nil && ii.err == nil {
+				// "Can't happen" because of previous loop.
-				ii.err = err // e.g. parse error.
+				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(bp *build.Package, which rune) ([]*ast.File, []error) {
-//
-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
-	}
-
 	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)
+	files, errs := parseFiles(conf.fset(), conf.build(), conf.DisplayPath, bp.Dir, filenames, conf.ParserMode)
-	if err != nil {
-		return nil, err
-	}
 
 	// 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...)
 		}
+	}
 
-	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.
+	// Ignore the returned (first) error since we already collect them all.
-		if info.TypeError == nil {
+	_ = info.checker.Files(files)
-			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{
 		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

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
 }
 

go/loader/util.go

@@ -15,13 +15,14 @@ import (
 	"sync"
 )
 
-// parseFiles parses the Go source files files within directory dir
+// parseFiles parses the Go source files within directory dir and
-// and returns their ASTs, or the first parse error if any.
+// 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()
 
+	// Eliminate nils, preserving order.
+	var o int
+	for _, f := range parsed {
+		if f != nil {
+			parsed[o] = f
+			o++
+		}
+	}
+	parsed = parsed[:o]
+
+	o = 0
 	for _, err := range errors {
 		if err != nil {
-			return nil, err
+			errors[o] = err
+			o++
 		}
 	}
-	return parsed, nil
+	errors = errors[:o]
+
+	return parsed, errors
 }
 
 // ---------- Internal helpers ----------

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.

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
+// ERRORS: for each locus of a frontend (scanner/parser/type) error, the
-// is highlighted in red and hover text provides the compiler error
+// location is highlighted in red and hover text provides the compiler
-// message.
+// 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
 }
@@ -302,14 +307,9 @@ func (a *analysis) posURL(pos token.Pos, len int) string {
 func Run(pta bool, result *Result) {
 	conf := loader.Config{
 		SourceImports: true,
-		AllowTypeErrors: true,
+		AllowErrors:   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)
 	}
+	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
+		log.Printf("Analysis failed: %s\n", err)
-		// of one parse error.
-		log.Print(err) // parse error in some package
 		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 {
+	errors := make(map[token.Position][]string)
-		fi, offset := a.fileAndOffset(pos)
+	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"),