From 3bbc63016b1c00619a8ddab1d92b31162a740df0 Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Thu, 7 Aug 2014 12:50:15 -0400 Subject: [PATCH] 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 | 50 ++++++---- .../{importer_test.go => loader_test.go} | 94 +++++++++++++++---- 2 files changed, 107 insertions(+), 37 deletions(-) rename go/loader/{importer_test.go => loader_test.go} (75%) diff --git a/go/loader/loader.go b/go/loader/loader.go index 14b9a395..3800389f 100644 --- a/go/loader/loader.go +++ b/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 @@ -271,11 +271,21 @@ type PackageInfo struct { Errors []error // non-nil if the package had errors types.Info // type-checker deductions. - checker *types.Checker // transient type-checker state + 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 - - // 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) - } + tc.Import = imp.doImport // doImport wraps the user's importfn, effectively + tc.Error = info.appendError // appendError wraps the user's Error function info.checker = types.NewChecker(&tc, imp.conf.fset(), pkg, &info.Info) imp.prog.AllPackages[pkg] = info diff --git a/go/loader/importer_test.go b/go/loader/loader_test.go similarity index 75% rename from go/loader/importer_test.go rename to go/loader/loader_test.go index 83fe24b2..5843c5d7 100644 --- a/go/loader/importer_test.go +++ b/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) { - conf := loader.Config{ - AllowErrors: true, - SourceImports: true, +func fakeContext(pkgs map[string]string) *build.Context { + ctxt := build.Default // copy + ctxt.GOROOT = "/go" + 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 - ctxt.GOROOT = "/go" - 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 nopCloser{bytes.NewBufferString(pkgs[path[0:1]])}, nil + conf := loader.Config{ + AllowErrors: true, + SourceImports: true, + Build: fakeContext(pkgs), } - 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) + } +}