go.tools/go/loader: report parser errors too.

Previously these were recorded in the PackageInfo, but not
reported to the user's error handler (types.Config.Error),
which is typically what prints them.

Minor subtlety: that function must now be able to handle error
values that are not of type types.Error.

+ Test (and renamed it).

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

go/loader/loader.go

@@ -181,7 +181,7 @@ type Config struct {
 	// instead.  Since that typically supplies only the types of
 	// package-level declarations and values of constants, but no
 	// code, it will not yield a whole program.  It is intended
-	// for analyses that perform intraprocedural analysis of a
+	// for analyses that perform modular analysis of a
 	// single package, e.g. traditional compilation.
 	//
 	// The initial packages (CreatePkgs and ImportPkgs) are always
@@ -272,10 +272,20 @@ type PackageInfo struct {
 	types.Info                        // type-checker deductions.
 
 	checker   *types.Checker // transient type-checker state
+	errorFunc func(error)
 }
 
 func (info *PackageInfo) String() string { return info.Pkg.Path() }
 
+func (info *PackageInfo) appendError(err error) {
+	if info.errorFunc != nil {
+		info.errorFunc(err)
+	} else {
+		fmt.Fprintln(os.Stderr, err)
+	}
+	info.Errors = append(info.Errors, err)
+}
+
 func (conf *Config) fset() *token.FileSet {
 	if conf.Fset == nil {
 		conf.Fset = token.NewFileSet()
@@ -513,6 +523,11 @@ func (conf *Config) Load() (*Program, error) {
 		conf.TypeChecker.Packages = make(map[string]*types.Package)
 	}
 
+	// Create a simple default error handler for parse/type errors.
+	if conf.TypeChecker.Error == nil {
+		conf.TypeChecker.Error = func(e error) { fmt.Fprintln(os.Stderr, e) }
+	}
+
 	prog := &Program{
 		Fset:        conf.fset(),
 		Imported:    make(map[string]*PackageInfo),
@@ -546,7 +561,9 @@ func (conf *Config) Load() (*Program, error) {
 
 			info := imp.imported[path].info // must be non-nil, see above
 			files, errs := imp.conf.parsePackageFiles(bp, 't')
-			info.Errors = append(info.Errors, errs...)
+			for _, err := range errs {
+				info.appendError(err)
+			}
 			typeCheckFiles(info, files...)
 		}
 	}
@@ -572,7 +589,9 @@ func (conf *Config) Load() (*Program, error) {
 		if info == nil {
 			prog.AllPackages[obj] = &PackageInfo{Pkg: obj, Importable: true}
 		} else {
-			info.checker = nil // finished
+			// finished
+			info.checker = nil
+			info.errorFunc = nil
 		}
 	}
 
@@ -782,7 +801,9 @@ func (imp *importer) importFromSource(path string) (*PackageInfo, error) {
 	// Type-check the package.
 	info := imp.newPackageInfo(path)
 	files, errs := imp.conf.parsePackageFiles(bp, 'g')
-	info.Errors = append(info.Errors, errs...)
+	for _, err := range errs {
+		info.appendError(err)
+	}
 	typeCheckFiles(info, files...)
 	return info, nil
 }
@@ -791,7 +812,7 @@ 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.Error field.
+// Errors are stored in the info.Errors field.
 func typeCheckFiles(info *PackageInfo, files ...*ast.File) {
 	info.Files = append(info.Files, files...)
 
@@ -811,26 +832,17 @@ func (imp *importer) newPackageInfo(path string) *PackageInfo {
 			Scopes:     make(map[ast.Node]*types.Scope),
 			Selections: make(map[*ast.SelectorExpr]*types.Selection),
 		},
+		errorFunc: imp.conf.TypeChecker.Error,
 	}
 
-	// Use a copy of the types.Config so we can vary IgnoreFuncBodies.
+	// Copy the types.Config so we can vary it across PackageInfos.
 	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
-
+	tc.Error = info.appendError // appendError wraps the user's Error function
-	// 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

go/loader/loader_test.go

@@ -9,8 +9,10 @@ import (
 	"fmt"
 	"go/build"
 	"io"
+	"io/ioutil"
 	"os"
 	"sort"
+	"strings"
 	"testing"
 	"time"
 
@@ -114,10 +116,6 @@ func TestLoadFromArgsSource(t *testing.T) {
 	}
 }
 
-type nopCloser struct{ *bytes.Buffer }
-
-func (nopCloser) Close() error { return nil }
-
 type fakeFileInfo struct{}
 
 func (fakeFileInfo) Name() string       { return "x.go" }
@@ -129,12 +127,20 @@ func (fakeFileInfo) Mode() os.FileMode  { return 0644 }
 
 var justXgo = [1]os.FileInfo{fakeFileInfo{}} // ["x.go"]
 
-func TestTransitivelyErrorFreeFlag(t *testing.T) {
+func fakeContext(pkgs map[string]string) *build.Context {
-	conf := loader.Config{
+	ctxt := build.Default // copy
-		AllowErrors:   true,
+	ctxt.GOROOT = "/go"
-		SourceImports: true,
+	ctxt.GOPATH = ""
+	ctxt.IsDir = func(path string) bool { return true }
+	ctxt.ReadDir = func(dir string) ([]os.FileInfo, error) { return justXgo[:], nil }
+	ctxt.OpenFile = func(path string) (io.ReadCloser, error) {
+		path = path[len("/go/src/pkg/"):]
+		return ioutil.NopCloser(bytes.NewBufferString(pkgs[path[0:1]])), nil
 	}
+	return &ctxt
+}
 
+func TestTransitivelyErrorFreeFlag(t *testing.T) {
 	// Create an minimal custom build.Context
 	// that fakes the following packages:
 	//
@@ -150,17 +156,11 @@ func TestTransitivelyErrorFreeFlag(t *testing.T) {
 		"d": `package d;`,
 		"e": `package e; import _ "d"`,
 	}
-	ctxt := build.Default // copy
+	conf := loader.Config{
-	ctxt.GOROOT = "/go"
+		AllowErrors:   true,
-	ctxt.GOPATH = ""
+		SourceImports: true,
-	ctxt.IsDir = func(path string) bool { return true }
+		Build:         fakeContext(pkgs),
-	ctxt.ReadDir = func(dir string) ([]os.FileInfo, error) { return justXgo[:], nil }
-	ctxt.OpenFile = func(path string) (io.ReadCloser, error) {
-		path = path[len("/go/src/pkg/"):]
-		return nopCloser{bytes.NewBufferString(pkgs[path[0:1]])}, nil
 	}
-	conf.Build = &ctxt
-
 	conf.Import("a")
 
 	prog, err := conf.Load()
@@ -199,3 +199,61 @@ func TestTransitivelyErrorFreeFlag(t *testing.T) {
 		}
 	}
 }
+
+// Test that both syntax (scan/parse) and type errors are both recorded
+// (in PackageInfo.Errors) and reported (via Config.TypeChecker.Error).
+func TestErrorReporting(t *testing.T) {
+	pkgs := map[string]string{
+		"a": `package a; import _ "b"; var x int = false`,
+		"b": `package b; 'syntax error!`,
+	}
+	conf := loader.Config{
+		AllowErrors:   true,
+		SourceImports: true,
+		Build:         fakeContext(pkgs),
+	}
+	var allErrors []error
+	conf.TypeChecker.Error = func(err error) {
+		allErrors = append(allErrors, err)
+	}
+	conf.Import("a")
+
+	prog, err := conf.Load()
+	if err != nil {
+		t.Errorf("Load failed: %s", err)
+	}
+	if prog == nil {
+		t.Fatalf("Load returned nil *Program")
+	}
+
+	hasError := func(errors []error, substr string) bool {
+		for _, err := range errors {
+			if strings.Contains(err.Error(), substr) {
+				return true
+			}
+		}
+		return false
+	}
+
+	// TODO(adonovan): test keys of ImportMap.
+
+	// Check errors recorded in each PackageInfo.
+	for pkg, info := range prog.AllPackages {
+		switch pkg.Path() {
+		case "a":
+			if !hasError(info.Errors, "cannot convert false") {
+				t.Errorf("a.Errors = %v, want bool conversion (type) error", info.Errors)
+			}
+		case "b":
+			if !hasError(info.Errors, "rune literal not terminated") {
+				t.Errorf("b.Errors = %v, want unterminated literal (syntax) error", info.Errors)
+			}
+		}
+	}
+
+	// Check errors reported via error handler.
+	if !hasError(allErrors, "cannot convert false") ||
+		!hasError(allErrors, "rune literal not terminated") {
+		t.Errorf("allErrors = %v, want both syntax and type errors", allErrors)
+	}
+}