go/analysis: preparatory cleanups

adonovan · gopherbot · commit 114ac8273d6a · 2025-01-24T13:09:30.000-08:00
This change contains some minor cleanups split out of the forthcoming three-way merge work. 1. Plumb pass.ReadFile down from a (hidden) checker option. Factor CheckedReadFile helper. 2. In "assign" checker, improve SuggestedFix title. 3. Fix bug in error handling in fix_test.go. 4. Define testenv.RedirectStderr helper to temporarily redirect stderr and log output to t.Log, to declutter the test output. Update golang/go#68765 Update golang/go#67049 Change-Id: Icac62afdeb160a2dfa3cc3637b79fe7d89e92272 Reviewed-on: https://go-review.googlesource.com/c/tools/+/643695 LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Robert Findley <rfindley@google.com> Commit-Queue: Alan Donovan <adonovan@google.com> Auto-Submit: Alan Donovan <adonovan@google.com>
diff --git a/go/analysis/analysistest/analysistest.go b/go/analysis/analysistest/analysistest.go
@@ -167,6 +167,7 @@ func RunWithSuggestedFixes(t Testing, dir string, a *analysis.Analyzer, patterns
 		act := result.Action
 
 		// file -> message -> edits
+		// TODO(adonovan): this mapping assumes fix.Messages are unique across analyzers.
 		fileEdits := make(map[*token.File]map[string][]diff.Edit)
 		fileContents := make(map[*token.File][]byte)
 
@@ -179,6 +180,8 @@ func RunWithSuggestedFixes(t Testing, dir string, a *analysis.Analyzer, patterns
 					t.Errorf("missing Diagnostic.Category for SuggestedFix without TextEdits (gopls requires the category for the name of the fix command")
 				}
 
+				// TODO(adonovan): factor in common with go/analysis/internal/checker.validateEdits.
+
 				for _, edit := range fix.TextEdits {
 					start, end := edit.Pos, edit.End
 					if !end.IsValid() {
@@ -275,7 +278,7 @@ func RunWithSuggestedFixes(t Testing, dir string, a *analysis.Analyzer, patterns
 				}
 			} else {
 				// all suggested fixes are represented by a single file
-
+				// TODO(adonovan): fix: this makes no sense if len(fixes) > 1.
 				var catchallEdits []diff.Edit
 				for _, edits := range fixes {
 					catchallEdits = append(catchallEdits, edits...)
diff --git a/go/analysis/checker/checker.go b/go/analysis/checker/checker.go
@@ -35,6 +35,7 @@ import (
 	"go/types"
 	"io"
 	"log"
+	"os"
 	"reflect"
 	"sort"
 	"strings"
@@ -55,9 +56,10 @@ type Options struct {
 	SanityCheck bool      // check fact encoding is ok and deterministic
 	FactLog     io.Writer // if non-nil, log each exported fact to it
 
-	// TODO(adonovan): add ReadFile so that an Overlay specified
+	// TODO(adonovan): expose ReadFile so that an Overlay specified
 	// in the [packages.Config] can be communicated via
 	// Pass.ReadFile to each Analyzer.
+	readFile analysisinternal.ReadFileFunc
 }
 
 // Graph holds the results of a round of analysis, including the graph
@@ -344,7 +346,11 @@ func (act *Action) execOnce() {
 		AllObjectFacts:    act.AllObjectFacts,
 		AllPackageFacts:   act.AllPackageFacts,
 	}
-	pass.ReadFile = analysisinternal.MakeReadFile(pass)
+	readFile := os.ReadFile
+	if act.opts.readFile != nil {
+		readFile = act.opts.readFile
+	}
+	pass.ReadFile = analysisinternal.CheckedReadFile(pass, readFile)
 	act.pass = pass
 
 	act.Result, act.Err = func() (any, error) {
diff --git a/go/analysis/diagnostic.go b/go/analysis/diagnostic.go
@@ -65,7 +65,9 @@ type RelatedInformation struct {
 // user can choose to apply to their code. Usually the SuggestedFix is
 // meant to fix the issue flagged by the diagnostic.
 //
-// The TextEdits must not overlap, nor contain edits for other packages.
+// The TextEdits must not overlap, nor contain edits for other
+// packages. Edits need not be totally ordered, but the order
+// determines how insertions at the same point will be applied.
 type SuggestedFix struct {
 	// A verb phrase describing the fix, to be shown to
 	// a user trying to decide whether to accept it.
diff --git a/go/analysis/internal/checker/checker.go b/go/analysis/internal/checker/checker.go
@@ -20,6 +20,7 @@ import (
 	"go/token"
 	"io"
 	"io/ioutil"
+
 	"log"
 	"os"
 	"runtime"
@@ -139,6 +140,9 @@ func Run(args []string, analyzers []*analysis.Analyzer) int {
 		return 1
 	}
 
+	// 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.
diff --git a/go/analysis/internal/checker/checker_test.go b/go/analysis/internal/checker/checker_test.go
@@ -25,6 +25,7 @@ import (
 
 func TestApplyFixes(t *testing.T) {
 	testenv.NeedsGoPackages(t)
+	testenv.RedirectStderr(t) // associated checker.Run output with this test
 
 	files := map[string]string{
 		"rename/test.go": `package rename
@@ -114,10 +115,12 @@ func run(pass *analysis.Pass) (interface{}, error) {
 					{Pos: ident.Pos(), End: ident.End(), NewText: []byte("lorem ipsum")},
 				}...)
 			case duplicate:
+				// Duplicate (non-insertion) edits are disallowed,
+				// so this is a buggy analyzer, and validatedFixes should reject it.
 				edits = append(edits, edits...)
 			case other:
 				if pass.Analyzer.Name == other {
-					edits[0].Pos = edits[0].Pos + 1 // shift by one to mismatch analyzer and other
+					edits[0].Pos++ // shift by one to mismatch analyzer and other
 				}
 			}
 			pass.Report(analysis.Diagnostic{
@@ -133,6 +136,7 @@ func run(pass *analysis.Pass) (interface{}, error) {
 
 func TestRunDespiteErrors(t *testing.T) {
 	testenv.NeedsGoPackages(t)
+	testenv.RedirectStderr(t) // associate checker.Run output with this test
 
 	files := map[string]string{
 		"rderr/test.go": `package rderr
@@ -360,4 +364,7 @@ hello from other
 	if !ran {
 		t.Error("analyzer did not run")
 	}
+
+	// TODO(adonovan): test that fixes are applied to the
+	// pass.ReadFile virtual file tree.
 }
diff --git a/go/analysis/internal/checker/fix_test.go b/go/analysis/internal/checker/fix_test.go
@@ -77,16 +77,23 @@ func fix(t *testing.T, dir, analyzers string, wantExit int, patterns ...string)
 		return strings.ReplaceAll(s, os.TempDir(), "os.TempDir/")
 	}
 	outBytes, err := cmd.CombinedOutput()
-	out := clean(string(outBytes))
-	t.Logf("$ %s\n%s", clean(fmt.Sprint(cmd)), out)
-	if err, ok := err.(*exec.ExitError); !ok {
-		t.Fatalf("failed to execute multichecker: %v", err)
-	} else if err.ExitCode() != wantExit {
-		// plan9 ExitCode() currently only returns 0 for success or 1 for failure
-		if !(runtime.GOOS == "plan9" && wantExit != exitCodeSuccess && err.ExitCode() != exitCodeSuccess) {
-			t.Errorf("exit code was %d, want %d", err.ExitCode(), wantExit)
+	switch err := err.(type) {
+	case nil:
+		// success
+	case *exec.ExitError:
+		if code := err.ExitCode(); code != wantExit {
+			// plan9 ExitCode() currently only returns 0 for success or 1 for failure
+			if !(runtime.GOOS == "plan9" && wantExit != exitCodeSuccess && code != exitCodeSuccess) {
+				t.Errorf("exit code was %d, want %d", code, wantExit)
+			}
 		}
+	default:
+		t.Fatalf("failed to execute multichecker: %v", err)
 	}
+
+	out := clean(string(outBytes))
+	t.Logf("$ %s\n%s", clean(fmt.Sprint(cmd)), out)
+
 	return out
 }
 
@@ -253,6 +260,7 @@ func Foo() {
 	}
 	defer cleanup()
 
+	// The 'rename' and 'other' analyzers suggest conflicting fixes.
 	out := fix(t, dir, "rename,other", exitCodeFailed, "other")
 
 	pattern := `.*conflicting edits from other and rename on .*foo.go`
diff --git a/go/analysis/internal/checker/start_test.go b/go/analysis/internal/checker/start_test.go
@@ -22,6 +22,7 @@ import (
 // of the file takes effect.
 func TestStartFixes(t *testing.T) {
 	testenv.NeedsGoPackages(t)
+	testenv.RedirectStderr(t) // associated checker.Run output with this test
 
 	files := map[string]string{
 		"comment/doc.go": `/* Package comment */
diff --git a/go/analysis/passes/assign/assign.go b/go/analysis/passes/assign/assign.go
@@ -63,10 +63,12 @@ func run(pass *analysis.Pass) (any, error) {
 			if le == re {
 				pass.Report(analysis.Diagnostic{
 					Pos: stmt.Pos(), Message: fmt.Sprintf("self-assignment of %s to %s", re, le),
-					SuggestedFixes: []analysis.SuggestedFix{
-						{Message: "Remove", TextEdits: []analysis.TextEdit{
-							{Pos: stmt.Pos(), End: stmt.End(), NewText: []byte{}},
-						}},
+					SuggestedFixes: []analysis.SuggestedFix{{
+						Message: "Remove self-assignment",
+						TextEdits: []analysis.TextEdit{{
+							Pos: stmt.Pos(),
+							End: stmt.End(),
+						}}},
 					},
 				})
 			}
diff --git a/go/analysis/unitchecker/unitchecker.go b/go/analysis/unitchecker/unitchecker.go
@@ -386,7 +386,7 @@ func run(fset *token.FileSet, cfg *Config, analyzers []*analysis.Analyzer) ([]re
 				AllPackageFacts:   func() []analysis.PackageFact { return facts.AllPackageFacts(factFilter) },
 				Module:            module,
 			}
-			pass.ReadFile = analysisinternal.MakeReadFile(pass)
+			pass.ReadFile = analysisinternal.CheckedReadFile(pass, os.ReadFile)
 
 			t0 := time.Now()
 			act.result, act.err = a.Run(pass)
diff --git a/go/analysis/unitchecker/unitchecker_test.go b/go/analysis/unitchecker/unitchecker_test.go
@@ -133,7 +133,7 @@ func _() {
 				"message": "self-assignment of i to i",
 				"suggested_fixes": \[
 					\{
-						"message": "Remove",
+						"message": "Remove self-assignment",
 						"edits": \[
 							\{
 								"filename": "([/._\-a-zA-Z0-9]+[\\/]fake[\\/])?c/c.go",
diff --git a/internal/analysisinternal/analysis.go b/internal/analysisinternal/analysis.go
@@ -14,7 +14,6 @@ import (
 	"go/scanner"
 	"go/token"
 	"go/types"
-	"os"
 	pathpkg "path"
 	"slices"
 	"strings"
@@ -178,20 +177,25 @@ func equivalentTypes(want, got types.Type) bool {
 	return types.AssignableTo(want, got)
 }
 
-// MakeReadFile returns a simple implementation of the Pass.ReadFile function.
-func MakeReadFile(pass *analysis.Pass) func(filename string) ([]byte, error) {
+// A ReadFileFunc is a function that returns the
+// contents of a file, such as [os.ReadFile].
+type ReadFileFunc = func(filename string) ([]byte, error)
+
+// CheckedReadFile returns a wrapper around a Pass.ReadFile
+// function that performs the appropriate checks.
+func CheckedReadFile(pass *analysis.Pass, readFile ReadFileFunc) ReadFileFunc {
 	return func(filename string) ([]byte, error) {
 		if err := CheckReadable(pass, filename); err != nil {
 			return nil, err
 		}
-		return os.ReadFile(filename)
+		return readFile(filename)
 	}
 }
 
 // CheckReadable enforces the access policy defined by the ReadFile field of [analysis.Pass].
 func CheckReadable(pass *analysis.Pass, filename string) error {
-	if slicesContains(pass.OtherFiles, filename) ||
-		slicesContains(pass.IgnoredFiles, filename) {
+	if slices.Contains(pass.OtherFiles, filename) ||
+		slices.Contains(pass.IgnoredFiles, filename) {
 		return nil
 	}
 	for _, f := range pass.Files {
@@ -202,16 +206,6 @@ func CheckReadable(pass *analysis.Pass, filename string) error {
 	return fmt.Errorf("Pass.ReadFile: %s is not among OtherFiles, IgnoredFiles, or names of Files", filename)
 }
 
-// TODO(adonovan): use go1.21 slices.Contains.
-func slicesContains[S ~[]E, E comparable](slice S, x E) bool {
-	for _, elem := range slice {
-		if elem == x {
-			return true
-		}
-	}
-	return false
-}
-
 // AddImport checks whether this file already imports pkgpath and
 // that import is in scope at pos. If so, it returns the name under
 // which it was imported and a zero edit. Otherwise, it adds a new
diff --git a/internal/testenv/testenv.go b/internal/testenv/testenv.go
@@ -7,10 +7,12 @@
 package testenv
 
 import (
+	"bufio"
 	"bytes"
 	"context"
 	"fmt"
 	"go/build"
+	"log"
 	"os"
 	"os/exec"
 	"path/filepath"
@@ -553,3 +555,40 @@ func NeedsGOROOTDir(t *testing.T, dir string) {
 		}
 	}
 }
+
+// RedirectStderr causes os.Stderr (and the global logger) to be
+// temporarily replaced so that writes to it are sent to t.Log.
+// It is restored at test cleanup.
+func RedirectStderr(t testing.TB) {
+	t.Setenv("RedirectStderr", "") // side effect: assert t.Parallel wasn't called
+
+	// TODO(adonovan): if https://go.dev/issue/59928 is accepted,
+	// simply set w = t.Output() and dispense with the pipe.
+	r, w, err := os.Pipe()
+	if err != nil {
+		t.Fatalf("pipe: %v", err)
+	}
+	go func() {
+		for sc := bufio.NewScanner(r); sc.Scan(); {
+			t.Log(sc.Text())
+		}
+		r.Close()
+	}()
+
+	// Also do the same for the global logger.
+	savedWriter, savedPrefix, savedFlags := log.Writer(), log.Prefix(), log.Flags()
+	log.SetPrefix("log: ")
+	log.SetOutput(w)
+	log.SetFlags(0)
+
+	oldStderr := os.Stderr
+	os.Stderr = w
+	t.Cleanup(func() {
+		w.Close() // ignore error
+		os.Stderr = oldStderr
+
+		log.SetOutput(savedWriter)
+		log.SetPrefix(savedPrefix)
+		log.SetFlags(savedFlags)
+	})
+}

Original file line number	Diff line number	Diff line change
`@@ -386,7 +386,7 @@ func run(fset token.FileSet, cfg Config, analyzers []*analysis.Analyzer) ([]re`
`386`	`386`	`AllPackageFacts: func() []analysis.PackageFact { return facts.AllPackageFacts(factFilter) },`
`387`	`387`	`Module: module,`
`388`	`388`	`}`
`389`		`- pass.ReadFile = analysisinternal.MakeReadFile(pass)`
	`389`	`+ pass.ReadFile = analysisinternal.CheckedReadFile(pass, os.ReadFile)`
`390`	`390`
`391`	`391`	`t0 := time.Now()`
`392`	`392`	`act.result, act.err = a.Run(pass)`