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{}
