go/analysis: exit nonzero upon diagnostics

This change causes singlechecker and multichecker to exit with the
correct error code: 0 for success, 1 for load/analysis and other
errors, 3 for diagnostics.  (We avoid 2 because the flag package uses
it.)

In JSON mode, errors in package loading, parsing, typechecking and
analysis are successfully in JSON format, with exit code 0.

+ Test.

Change-Id: Iaf130ed3d4cb3e747a628af6da8dc97d065aa869
Reviewed-on: https://go-review.googlesource.com/c/149603
Run-TryBot: Alan Donovan <adonovan@google.com>
Reviewed-by: Michael Matloob <matloob@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>

Author: adonovan

go/analysis/internal/checker/checker.go

@@ -64,7 +64,8 @@ func RegisterFlags() {
 // Analysis flags must already have been set.
 // It provides most of the logic for the main functions of both the
 // singlechecker and the multi-analysis commands.
-func Run(args []string, analyzers []*analysis.Analyzer) error {
+// It returns the appropriate exit code.
+func Run(args []string, analyzers []*analysis.Analyzer) (exitcode int) {
 	if CPUProfile != "" {
 		f, err := os.Create(CPUProfile)
 		if err != nil {
@@ -131,15 +132,14 @@ func Run(args []string, analyzers []*analysis.Analyzer) error {
 	allSyntax := needFacts(analyzers)
 	initial, err := load(args, allSyntax)
 	if err != nil {
-		return err
+		log.Print(err)
+		return 1 // load errors
 	}
 
-	roots := analyze(initial, analyzers)
-
 	// Print the results.
-	printDiagnostics(roots)
+	roots := analyze(initial, analyzers)
 
-	return nil
+	return printDiagnostics(roots)
 }
 
 // load loads the initial packages.
@@ -160,6 +160,9 @@ func load(patterns []string, allSyntax bool) ([]*packages.Package, error) {
 			err = fmt.Errorf("error during loading")
 		}
 	}
+	if len(initial) == 0 {
+		err = fmt.Errorf("%s matched no packages", strings.Join(patterns, " "))
+	}
 	return initial, err
 }
 
@@ -263,7 +266,12 @@ func analyze(pkgs []*packages.Package, analyzers []*analysis.Analyzer) []*action
 // printDiagnostics prints the diagnostics for the root packages in either
 // plain text or JSON format. JSON format also includes errors for any
 // dependencies.
-func printDiagnostics(roots []*action) {
+//
+// It returns the exitcode: in plain mode, 0 for success, 1 for analysis
+// errors, and 3 for diagnostics. We avoid 2 since the flag package uses
+// it. JSON mode always succeeds at printing errors and diagnostics in a
+// structured form to stdout.
+func printDiagnostics(roots []*action) (exitcode int) {
 	// Print the output.
 	//
 	// Print diagnostics only for root packages,
@@ -341,6 +349,7 @@ func printDiagnostics(roots []*action) {
 		print = func(act *action) {
 			if act.err != nil {
 				fmt.Fprintf(os.Stderr, "%s: %v\n", act.a.Name, act.err)
+				exitcode = 1 // analysis failed, at least partially
 				return
 			}
 			if act.isroot {
@@ -372,6 +381,10 @@ func printDiagnostics(roots []*action) {
 			}
 		}
 		visitAll(roots)
+
+		if exitcode == 0 && len(seen) > 0 {
+			exitcode = 3 // successfuly produced diagnostics
+		}
 	}
 
 	// Print timing info.
@@ -399,6 +412,8 @@ func printDiagnostics(roots []*action) {
 			}
 		}
 	}
+
+	return exitcode
 }
 
 // needFacts reports whether any analysis required by the specified set

go/analysis/multichecker/multichecker.go

@@ -19,13 +19,6 @@ import (
 	"golang.org/x/tools/go/analysis/internal/checker"
 )
 
-// TODO(adonovan): document (and verify) the exit codes:
-// "Vet's exit code is 2 for erroneous invocation of the tool, 1 if a
-// problem was reported, and 0 otherwise. Note that the tool does not
-// check every possible problem and depends on unreliable heuristics
-// so it should be used as guidance only, not as a firm indicator of
-// program correctness."
-
 func Main(analyzers ...*analysis.Analyzer) {
 	progname := filepath.Base(os.Args[0])
 	log.SetFlags(0)
@@ -53,7 +46,5 @@ func Main(analyzers ...*analysis.Analyzer) {
 		os.Exit(0)
 	}
 
-	if err := checker.Run(args, analyzers); err != nil {
-		log.Fatal(err)
-	}
+	os.Exit(checker.Run(args, analyzers))
 }

go/analysis/multichecker/multichecker_test.go

@@ -0,0 +1,82 @@
+// +build go1.12
+
+package multichecker_test
+
+import (
+	"fmt"
+	"os"
+	"os/exec"
+	"runtime"
+	"testing"
+
+	"golang.org/x/tools/go/analysis"
+	"golang.org/x/tools/go/analysis/multichecker"
+	"golang.org/x/tools/go/analysis/passes/findcall"
+)
+
+func main() {
+	fail := &analysis.Analyzer{
+		Name: "fail",
+		Doc:  "always fail on a package 'sort'",
+		Run: func(pass *analysis.Pass) (interface{}, error) {
+			if pass.Pkg.Path() == "sort" {
+				return nil, fmt.Errorf("failed")
+			}
+			return nil, nil
+		},
+	}
+	multichecker.Main(findcall.Analyzer, fail)
+}
+
+// TestExitCode ensures that analysis failures are reported correctly.
+// This test fork/execs the main function above.
+func TestExitCode(t *testing.T) {
+	if runtime.GOOS != "linux" {
+		t.Skipf("skipping fork/exec test on this platform")
+	}
+
+	if os.Getenv("MULTICHECKER_CHILD") == "1" {
+		// child process
+
+		// replace [progname -test.run=TestExitCode -- ...]
+		//      by [progname ...]
+		os.Args = os.Args[2:]
+		os.Args[0] = "vet"
+		main()
+		panic("unreachable")
+	}
+
+	for _, test := range []struct {
+		args []string
+		want int
+	}{
+		{[]string{"nosuchdir/..."}, 1},                      // matched no packages
+		{[]string{"nosuchpkg"}, 1},                          // matched no packages
+		{[]string{"-unknownflag"}, 2},                       // flag error
+		{[]string{"-findcall.name=panic", "io"}, 3},         // finds diagnostics
+		{[]string{"-findcall=0", "io"}, 0},                  // no checkers
+		{[]string{"-findcall.name=nosuchfunc", "io"}, 0},    // no diagnostics
+		{[]string{"-findcall.name=panic", "sort", "io"}, 1}, // 'fail' failed on 'sort'
+
+		// -json: exits zero even in face of diagnostics or package errors.
+		{[]string{"-findcall.name=panic", "-json", "io"}, 0},
+		{[]string{"-findcall.name=panic", "-json", "io"}, 0},
+		{[]string{"-findcall.name=panic", "-json", "sort", "io"}, 0},
+	} {
+		args := []string{"-test.run=TestExitCode", "--"}
+		args = append(args, test.args...)
+		cmd := exec.Command(os.Args[0], args...)
+		cmd.Env = append(os.Environ(), "MULTICHECKER_CHILD=1")
+		out, err := cmd.CombinedOutput()
+		if len(out) > 0 {
+			t.Logf("%s: out=<<%s>>", test.args, out)
+		}
+		var exitcode int
+		if err, ok := err.(*exec.ExitError); ok {
+			exitcode = err.ExitCode() // requires go1.12
+		}
+		if exitcode != test.want {
+			t.Errorf("%s: exited %d, want %d", test.args, exitcode, test.want)
+		}
+	}
+}

go/analysis/singlechecker/singlechecker.go

@@ -67,7 +67,5 @@ func Main(a *analysis.Analyzer) {
 		os.Exit(1)
 	}
 
-	if err := checker.Run(args, analyzers); err != nil {
-		log.Fatal(err)
-	}
+	os.Exit(checker.Run(args, analyzers))
 }