go/analysis/internal/checker: be silent with -fix

Just apply the fixes without listing the diagnostics.

Also: be verbose with -v. The -v flag exists for historical
reasons to do with the vet CLI, but it had no effect. This
change makes it effectively an alias for -debug=v, which
no-one can remember how to spell.

Also, list the number of fixes and fixes updated when -fix and -v.

Change-Id: Ic2342ad4868b6c8649077bf13c48e8727dbeba37
Reviewed-on: https://go-review.googlesource.com/c/tools/+/647698
LUCI-TryBot-Result: Go LUCI <[email protected]>
Auto-Submit: Alan Donovan <[email protected]>
Reviewed-by: Robert Findley <[email protected]>

Author: adonovan

go/analysis/internal/checker/checker.go

@@ -86,7 +86,36 @@ func RegisterFlags() {
 // It provides most of the logic for the main functions of both the
 // singlechecker and the multi-analysis commands.
 // It returns the appropriate exit code.
-func Run(args []string, analyzers []*analysis.Analyzer) int {
+//
+// TODO(adonovan): tests should not call this function directly;
+// fiddling with global variables (flags) is error-prone and hostile
+// to parallelism. Instead, use unit tests of the actual units (e.g.
+// checker.Analyze) and integration tests (e.g. TestScript) of whole
+// executables.
+func Run(args []string, analyzers []*analysis.Analyzer) (exitcode int) {
+	// Instead of returning a code directly,
+	// call this function to monotonically increase the exit code.
+	// This allows us to keep going in the face of some errors
+	// without having to remember what code to return.
+	//
+	// TODO(adonovan): interpreting exit codes is like reading tea-leaves.
+	// Insted of wasting effort trying to encode a multidimensional result
+	// into 7 bits we should just emit structured JSON output, and
+	// an exit code of 0 or 1 for success or failure.
+	exitAtLeast := func(code int) {
+		exitcode = max(code, exitcode)
+	}
+
+	// When analysisflags is linked in (for {single,multi}checker),
+	// then the -v flag is registered for complex legacy reasons
+	// related to cmd/vet CLI.
+	// Treat it as an undocumented alias for -debug=v.
+	if v := flag.CommandLine.Lookup("v"); v != nil &&
+		v.Value.(flag.Getter).Get() == true &&
+		!strings.Contains(Debug, "v") {
+		Debug += "v"
+	}
+
 	if CPUProfile != "" {
 		f, err := os.Create(CPUProfile)
 		if err != nil {
@@ -142,17 +171,14 @@ func Run(args []string, analyzers []*analysis.Analyzer) int {
 	initial, err := load(args, allSyntax)
 	if err != nil {
 		log.Print(err)
-		return 1
+		exitAtLeast(1)
+		return
 	}
 
-	// TODO(adonovan): simplify exit code logic by using a single
-	// exit code variable and applying "code = max(code, X)" each
-	// time an error of code X occurs.
-	pkgsExitCode := 0
 	// Print package and module errors regardless of RunDespiteErrors.
 	// Do not exit if there are errors, yet.
 	if n := packages.PrintErrors(initial); n > 0 {
-		pkgsExitCode = 1
+		exitAtLeast(1)
 	}
 
 	var factLog io.Writer
@@ -172,7 +198,8 @@ func Run(args []string, analyzers []*analysis.Analyzer) int {
 	graph, err := checker.Analyze(analyzers, initial, opts)
 	if err != nil {
 		log.Print(err)
-		return 1
+		exitAtLeast(1)
+		return
 	}
 
 	// Don't print the diagnostics,
@@ -181,22 +208,22 @@ func Run(args []string, analyzers []*analysis.Analyzer) int {
 		if err := applyFixes(graph.Roots, Diff); err != nil {
 			// Fail when applying fixes failed.
 			log.Print(err)
-			return 1
+			exitAtLeast(1)
+			return
 		}
-		// TODO(adonovan): don't proceed to print the text or JSON output
-		// if we applied fixes; stop here.
-		//
-		// return pkgsExitCode
+		// Don't proceed to print text/JSON,
+		// and don't report an error
+		// just because there were diagnostics.
+		return
 	}
 
 	// 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(graph); diagExitCode != 0 {
-		return diagExitCode // there were diagnostics
-	}
-	return pkgsExitCode // package errors but no diagnostics
+	exitAtLeast(printDiagnostics(graph))
+
+	return
 }
 
 // printDiagnostics prints diagnostics in text or JSON form
@@ -541,6 +568,10 @@ fixloop:
 		}
 	}
 
+	if dbg('v') {
+		log.Printf("applied %d fixes, updated %d files", len(fixes), filesUpdated)
+	}
+
 	return nil
 }
 

go/analysis/internal/checker/checker_test.go

@@ -49,8 +49,10 @@ func Foo() {
 		t.Fatal(err)
 	}
 	path := filepath.Join(testdata, "src/rename/test.go")
+
 	checker.Fix = true
 	checker.Run([]string{"file=" + path}, []*analysis.Analyzer{renameAnalyzer})
+	checker.Fix = false
 
 	contents, err := os.ReadFile(path)
 	if err != nil {
@@ -138,31 +140,33 @@ func NewT1() *T1 { return &T1{T} }
 		// 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{rderrFile}, analyzers: []*analysis.Analyzer{noop}, code: 1},
+		{name: "despite-error", pattern: []string{rderrFile}, analyzers: []*analysis.Analyzer{noop}, code: exitCodeFailed},
 		// 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{rderrFile}, analyzers: []*analysis.Analyzer{noopWithFact}, code: 1},
+		{name: "despite-error-fact", pattern: []string{rderrFile}, analyzers: []*analysis.Analyzer{noopWithFact}, code: exitCodeFailed},
 		// combination of parse/type errors and no errors
-		{name: "despite-error-and-no-error", pattern: []string{rderrFile, "sort"}, analyzers: []*analysis.Analyzer{renameAnalyzer, noop}, code: 1},
+		{name: "despite-error-and-no-error", pattern: []string{rderrFile, "sort"}, analyzers: []*analysis.Analyzer{renameAnalyzer, noop}, code: exitCodeFailed},
 		// 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},
+		{name: "no-package", pattern: []string{"xyz"}, analyzers: []*analysis.Analyzer{renameAnalyzer}, code: exitCodeFailed},
+		{name: "no-package-despite-error", pattern: []string{"abc"}, analyzers: []*analysis.Analyzer{noop}, code: exitCodeFailed},
+		{name: "no-multi-package-despite-error", pattern: []string{"xyz", "abc"}, analyzers: []*analysis.Analyzer{noop}, code: exitCodeFailed},
 		// combination of type/parsing and different errors
-		{name: "different-errors", pattern: []string{rderrFile, "xyz"}, analyzers: []*analysis.Analyzer{renameAnalyzer, noop}, code: 1},
+		{name: "different-errors", pattern: []string{rderrFile, "xyz"}, analyzers: []*analysis.Analyzer{renameAnalyzer, noop}, code: exitCodeFailed},
 		// non existing dir error
-		{name: "no-match-dir", pattern: []string{"file=non/existing/dir"}, analyzers: []*analysis.Analyzer{renameAnalyzer, noop}, code: 1},
+		{name: "no-match-dir", pattern: []string{"file=non/existing/dir"}, analyzers: []*analysis.Analyzer{renameAnalyzer, noop}, code: exitCodeFailed},
 		// no errors
-		{name: "no-errors", pattern: []string{"sort"}, analyzers: []*analysis.Analyzer{renameAnalyzer, noop}, code: 0},
+		{name: "no-errors", pattern: []string{"sort"}, analyzers: []*analysis.Analyzer{renameAnalyzer, noop}, code: exitCodeSuccess},
 		// duplicate list error with no findings
-		{name: "list-error", pattern: []string{cperrFile}, analyzers: []*analysis.Analyzer{noop}, code: 1},
+		{name: "list-error", pattern: []string{cperrFile}, analyzers: []*analysis.Analyzer{noop}, code: exitCodeFailed},
 		// duplicate list errors with findings (issue #67790)
-		{name: "list-error-findings", pattern: []string{cperrFile}, analyzers: []*analysis.Analyzer{renameAnalyzer}, code: 3},
+		{name: "list-error-findings", pattern: []string{cperrFile}, analyzers: []*analysis.Analyzer{renameAnalyzer}, code: exitCodeDiagnostics},
 	} {
-		if got := checker.Run(test.pattern, test.analyzers); got != test.code {
-			t.Errorf("got incorrect exit code %d for test %s; want %d", got, test.name, test.code)
-		}
+		t.Run(test.name, func(t *testing.T) {
+			if got := checker.Run(test.pattern, test.analyzers); got != test.code {
+				t.Errorf("got incorrect exit code %d for test %s; want %d", got, test.name, test.code)
+			}
+		})
 	}
 }
 

go/analysis/internal/checker/fix_test.go

@@ -52,9 +52,9 @@ func TestMain(m *testing.M) {
 }
 
 const (
-	exitCodeSuccess     = 0 // success (no diagnostics)
+	exitCodeSuccess     = 0 // success (no diagnostics, or successful -fix)
 	exitCodeFailed      = 1 // analysis failed to run
-	exitCodeDiagnostics = 3 // diagnostics were reported
+	exitCodeDiagnostics = 3 // diagnostics were reported (and no -fix)
 )
 
 // TestReportInvalidDiagnostic tests that a call to pass.Report with

go/analysis/internal/checker/start_test.go

@@ -40,6 +40,7 @@ package comment
 	path := filepath.Join(testdata, "src/comment/doc.go")
 	checker.Fix = true
 	checker.Run([]string{"file=" + path}, []*analysis.Analyzer{commentAnalyzer})
+	checker.Fix = false
 
 	contents, err := os.ReadFile(path)
 	if err != nil {

go/analysis/internal/checker/testdata/diff.txt

@@ -8,8 +8,7 @@
 
 skip GOOS=windows
 checker -rename -fix -diff example.com/p
-exit 3
-stderr renaming "bar" to "baz"
+exit 0
 
 -- go.mod --
 module example.com

go/analysis/internal/checker/testdata/fixes.txt

@@ -2,9 +2,9 @@
 # particular when processing duplicate fixes for overlapping packages
 # in the same directory ("p", "p [p.test]", "p_test [p.test]").
 
-checker -rename -fix example.com/p
-exit 3
-stderr renaming "bar" to "baz"
+checker -rename -fix -v example.com/p
+stderr applied 8 fixes, updated 3 files
+exit 0
 
 -- go.mod --
 module example.com

go/analysis/internal/checker/testdata/importdup.txt

@@ -1,8 +1,9 @@
 # Test that duplicate imports--and, more generally, duplicate
 # identical insertions--are coalesced.
 
-checker -marker -fix example.com/a
-exit 3
+checker -marker -fix -v example.com/a
+stderr applied 2 fixes, updated 1 files
+exit 0
 
 -- go.mod --
 module example.com

go/analysis/internal/checker/testdata/importdup2.txt

@@ -19,8 +19,9 @@
 # In more complex examples, the result
 # may be more subtly order-dependent.
 
-checker -marker -fix example.com/a example.com/b
-exit 3
+checker -marker -fix -v example.com/a example.com/b
+stderr applied 6 fixes, updated 2 files
+exit 0
 
 -- go.mod --
 module example.com

go/analysis/internal/checker/testdata/noend.txt

@@ -2,8 +2,7 @@
 # interpreted as if equal to SuggestedFix.Pos (see issue #64199).
 
 checker -noend -fix example.com/a
-exit 3
-stderr say hello
+exit 0
 
 -- go.mod --
 module example.com

go/analysis/internal/checker/testdata/overlap.txt

@@ -15,9 +15,12 @@
 # (This is a pretty unlikely situation, but it corresponds
 # to a historical test, TestOther, that used to check for
 # a conflict, and it seemed wrong to delete it without explanation.)
+#
+# The fixes are silently and successfully applied.
 
-checker -rename -marker -fix example.com/a
-exit 3
+checker -rename -marker -fix -v example.com/a
+stderr applied 2 fixes, updated 1 file
+exit 0
 
 -- go.mod --
 module example.com