internal/checker: run despite all errors Previously, we would run an analysis when RunDespiteErrors=true only if all package errors were syntax or type errors. We should run the analysis on all errors. The previous design choice was motivated by a concern that list and unknown errors might indicate something unexpectedly wrong about the program. If RunDespiteErrors=true, then this would go under the users' radar. This design choice seems somewhat arbitrary and creates problems when a list error is a duplicate of a type error; the analysis is not executed despite a conceptually single type error. Further, package errors are always printed to stderr, so they are visible to users. Lastly, we return exit code 3 if there are any diagnostics. Otherwise, we return 1 exit code, if there are any packages errors. Fixes golang/go#67790 Change-Id: Ie3186a08ed3c8292033344aec35fc755a6d256f2 Reviewed-on: https://go-review.googlesource.com/c/tools/+/591117 LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Alan Donovan <adonovan@google.com>
diff --git a/go/analysis/internal/checker/checker.go b/go/analysis/internal/checker/checker.go index 52f0e55..5a71465 100644 --- a/go/analysis/internal/checker/checker.go +++ b/go/analysis/internal/checker/checker.go
@@ -11,7 +11,6 @@ import ( "bytes" "encoding/gob" - "errors" "flag" "fmt" "go/format" @@ -134,16 +133,21 @@ allSyntax := needFacts(analyzers) initial, err := load(args, allSyntax) if err != nil { - if _, ok := err.(typeParseError); !ok { - // Fail when some of the errors are not - // related to parsing nor typing. - log.Print(err) - return 1 - } - // TODO: filter analyzers based on RunDespiteError? + log.Print(err) + return 1 } - // Run the analysis. + pkgsExitCode := 0 + // Print package errors regardless of RunDespiteErrors. + // Do not exit if there are errors, yet. + if n := packages.PrintErrors(initial); n > 0 { + pkgsExitCode = 1 + } + + // Run the analyzers. On each package with (transitive) + // errors, we run only the subset of analyzers that are + // marked (and whose transitive requirements are also + // marked) with RunDespiteErrors. roots := analyze(initial, analyzers) // Apply fixes. @@ -155,18 +159,18 @@ } } - // Print the results. - return printDiagnostics(roots) + // Print the results. If !RunDespiteErrors and there + // are errors in the packages, this will have 0 exit + // code. Otherwise, we prefer to return exit code + // indicating diagnostics. + if diagExitCode := printDiagnostics(roots); diagExitCode != 0 { + return diagExitCode // there were diagnostics + } + return pkgsExitCode // package errors but no diagnostics } -// typeParseError represents a package load error -// that is related to typing and parsing. -type typeParseError struct { - error -} - -// load loads the initial packages. If all loading issues are related to -// typing and parsing, the returned error is of type typeParseError. +// load loads the initial packages. Returns only top-level loading +// errors. Does not consider errors in packages. func load(patterns []string, allSyntax bool) ([]*packages.Package, error) { mode := packages.LoadSyntax if allSyntax { @@ -178,44 +182,12 @@ Tests: IncludeTests, } initial, err := packages.Load(&conf, patterns...) - if err == nil { - if len(initial) == 0 { - err = fmt.Errorf("%s matched no packages", strings.Join(patterns, " ")) - } else { - err = loadingError(initial) - } + if err == nil && len(initial) == 0 { + err = fmt.Errorf("%s matched no packages", strings.Join(patterns, " ")) } return initial, err } -// loadingError checks for issues during the loading of initial -// packages. Returns nil if there are no issues. Returns error -// of type typeParseError if all errors, including those in -// dependencies, are related to typing or parsing. Otherwise, -// a plain error is returned with an appropriate message. -func loadingError(initial []*packages.Package) error { - var err error - if n := packages.PrintErrors(initial); n > 1 { - err = fmt.Errorf("%d errors during loading", n) - } else if n == 1 { - err = errors.New("error during loading") - } else { - // no errors - return nil - } - all := true - packages.Visit(initial, nil, func(pkg *packages.Package) { - for _, err := range pkg.Errors { - typeOrParse := err.Kind == packages.TypeError || err.Kind == packages.ParseError - all = all && typeOrParse - } - }) - if all { - return typeParseError{err} - } - return err -} - // TestAnalyzer applies an analyzer to a set of packages (and their // dependencies if necessary) and returns the results. // The analyzer must be valid according to [analysis.Validate].
diff --git a/go/analysis/internal/checker/checker_test.go b/go/analysis/internal/checker/checker_test.go index 69ea944..46c74d7 100644 --- a/go/analysis/internal/checker/checker_test.go +++ b/go/analysis/internal/checker/checker_test.go
@@ -68,10 +68,11 @@ } var renameAnalyzer = &analysis.Analyzer{ - Name: "rename", - Requires: []*analysis.Analyzer{inspect.Analyzer}, - Run: run, - Doc: "renames symbols named bar to baz", + Name: "rename", + Requires: []*analysis.Analyzer{inspect.Analyzer}, + Run: run, + Doc: "renames symbols named bar to baz", + RunDespiteErrors: true, } var otherAnalyzer = &analysis.Analyzer{ // like analyzer but with a different Name. @@ -140,13 +141,28 @@ func Foo(s string) int { return s + 1 } -`} +`, + "cperr/test.go": `package copyerr + +import "sync" + +func bar() { } // for renameAnalyzer + +type T struct{ mu sync.Mutex } +type T1 struct{ t *T } + +func NewT1() *T1 { return &T1{T} } +`, + } testdata, cleanup, err := analysistest.WriteFiles(files) if err != nil { t.Fatal(err) } - path := filepath.Join(testdata, "src/rderr/test.go") + defer cleanup() + + rderrFile := "file=" + filepath.Join(testdata, "src/rderr/test.go") + cperrFile := "file=" + filepath.Join(testdata, "src/cperr/test.go") // A no-op analyzer that should finish regardless of // parse or type errors in the code. @@ -159,8 +175,8 @@ RunDespiteErrors: true, } - // A no-op analyzer that should finish regardless of - // parse or type errors in the code. + // A no-op analyzer, with facts, that should finish + // regardless of parse or type errors in the code. noopWithFact := &analysis.Analyzer{ Name: "noopfact", Requires: []*analysis.Analyzer{inspect.Analyzer}, @@ -178,30 +194,34 @@ code int }{ // parse/type errors - {name: "skip-error", pattern: []string{"file=" + path}, analyzers: []*analysis.Analyzer{renameAnalyzer}, code: 1}, + {name: "skip-error", pattern: []string{rderrFile}, analyzers: []*analysis.Analyzer{renameAnalyzer}, code: 1}, // RunDespiteErrors allows a driver to run an Analyzer even after parse/type errors. // // The noop analyzer doesn't use facts, so the driver loads only the root // package from source. For the rest, it asks 'go list' for export data, // which fails because the compiler encounters the type error. Since the // errors come from 'go list', the driver doesn't run the analyzer. - {name: "despite-error", pattern: []string{"file=" + path}, analyzers: []*analysis.Analyzer{noop}, code: 1}, + {name: "despite-error", pattern: []string{rderrFile}, analyzers: []*analysis.Analyzer{noop}, code: 1}, // The noopfact analyzer does use facts, so the driver loads source for // all dependencies, does type checking itself, recognizes the error as a // type error, and runs the analyzer. - {name: "despite-error-fact", pattern: []string{"file=" + path}, analyzers: []*analysis.Analyzer{noopWithFact}, code: 0}, + {name: "despite-error-fact", pattern: []string{rderrFile}, analyzers: []*analysis.Analyzer{noopWithFact}, code: 1}, // combination of parse/type errors and no errors - {name: "despite-error-and-no-error", pattern: []string{"file=" + path, "sort"}, analyzers: []*analysis.Analyzer{renameAnalyzer, noop}, code: 1}, + {name: "despite-error-and-no-error", pattern: []string{rderrFile, "sort"}, analyzers: []*analysis.Analyzer{renameAnalyzer, noop}, code: 1}, // non-existing package error {name: "no-package", pattern: []string{"xyz"}, analyzers: []*analysis.Analyzer{renameAnalyzer}, code: 1}, {name: "no-package-despite-error", pattern: []string{"abc"}, analyzers: []*analysis.Analyzer{noop}, code: 1}, {name: "no-multi-package-despite-error", pattern: []string{"xyz", "abc"}, analyzers: []*analysis.Analyzer{noop}, code: 1}, // combination of type/parsing and different errors - {name: "different-errors", pattern: []string{"file=" + path, "xyz"}, analyzers: []*analysis.Analyzer{renameAnalyzer, noop}, code: 1}, + {name: "different-errors", pattern: []string{rderrFile, "xyz"}, analyzers: []*analysis.Analyzer{renameAnalyzer, noop}, code: 1}, // non existing dir error {name: "no-match-dir", pattern: []string{"file=non/existing/dir"}, analyzers: []*analysis.Analyzer{renameAnalyzer, noop}, code: 1}, // no errors {name: "no-errors", pattern: []string{"sort"}, analyzers: []*analysis.Analyzer{renameAnalyzer, noop}, code: 0}, + // duplicate list error with no findings + {name: "list-error", pattern: []string{cperrFile}, analyzers: []*analysis.Analyzer{noop}, code: 1}, + // duplicate list errors with findings (issue #67790) + {name: "list-error-findings", pattern: []string{cperrFile}, analyzers: []*analysis.Analyzer{renameAnalyzer}, code: 3}, } { if test.name == "despite-error" && testenv.Go1Point() < 20 { // The behavior in the comment on the despite-error test only occurs for Go 1.20+. @@ -211,8 +231,6 @@ t.Errorf("got incorrect exit code %d for test %s; want %d", got, test.name, test.code) } } - - defer cleanup() } type EmptyFact struct{}