go/analysis/checker: a go/packages-based driver library

The {single,multi}checker packages provide the main function
for a complete application, as a black box. Many users want
the ability to customize the analyzer behavior with additional
logic, as described in the attached issues.

This change creates a new package, go/analysis/checker, that
exposes an Analyze pure function---one that avoids global flags,
os.Exit, logging, profiling, and other side effects---that
runs a set of analyzers on a set of packages loaded (by the
client) using go/packages, and presents the graph of results
in a form that allows postprocessing.

  package checker

  func Analyze([]*analysis.Analyzer, []*packages.Package, *Options) (*Graph, error)
  type Graph struct {
	Roots []*Action
  }
  type Action struct { ... } // information about each step

  func (*Graph) WriteJSONDiagnostics(io.Writer) error
  func (*Graph) WriteTextDiagnostics(io.Writer, contextLines int) error
  func (*Graph) All() iter.Seq[*Action] // (was Visit in the proposal)

See the example_test.go file for typical API usage.
API feedback welcome.

This change should have no effect on the behavior of
existing programs. Logic changes have been kept to a
minimum, so the large diffs are mostly code motion.
Auxiliary functions for printing, flags, fixes,
and so on has been kept to a minimum so that we
can make progress on this change.
They will be dealt with in follow-up changes.

Detailed list of changes to ease review:

analysistest:
- Run: we report Load errors to t.Log up front
  (unless RunDespiteErrors); this was previously in loadPackages.
- Run: the Validate call is now done by checker.Analyze.
- internal/checker.TestAnalyzer has melted away
- the logic to construct the "legacy" Result slice is new.
- Result: this type used to be an alias for
  internal/checker.checker.TestAnalyzerResult (regrettably
  exposing more than intended); now it is a plain public struct.
- check: the logic to check fact expectations is new.
- The buildtag and directive analyzers both used a similar
  hack w.r.t. IgnoredFiles; this hack is now in analysistest.
  Better ideas welcome.

checker:
- checker.go is mostly moved from internal/checker/checker.go.
- print.go is mostly split from old printDiagnostics in
  internal/checker/checker.go.
- Action.execOnce: the pass.Result/Err logic was simplified
  using an immediately-applied lambda.
- inheritFacts: the comments on package-fact handling are new.
- Graph.Visit is "born deprecated".
  Clients using go1.23 should use Graph.All (iter.Seq).
- Example: this is new code.

internal/checker:
- applyFixes: now accepts only the actions to apply, without
  calling visitAll over the action graph.
  The previous code was wrong, a latent bug.

Some files outside go/analysis were updated, not out of
necessity (it's not a breaking change) but to modernize
them (e.g. avoiding analysistest.Result.Pass).

Updates golang/go#61324		(new API proposal)
Fixes golang/go#53215		(feature proposal)
Fixes golang/go#31897
Fixes golang/go#50265
Fixes golang/go#53336
Fixes golang/go#66745
Updates golang/go#30231
Updates golang/go#30219
Updates golang/go#31007
Updates golang/go#66745

Change-Id: I745d319a587dca506564a4624b52a7f1eb5f4751
Reviewed-on: https://go-review.googlesource.com/c/tools/+/411907
Reviewed-by: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

go/analysis/analysis.go

@@ -50,7 +50,7 @@ type Analyzer struct {
 	// RunDespiteErrors allows the driver to invoke
 	// the Run method of this analyzer even on a
 	// package that contains parse or type errors.
-	// The Pass.TypeErrors field may consequently be non-empty.
+	// The [Pass.TypeErrors] field may consequently be non-empty.
 	RunDespiteErrors bool
 
 	// Requires is a set of analyzers that must run successfully

go/analysis/analysistest/analysistest.go

@@ -23,7 +23,8 @@ import (
 	"text/scanner"
 
 	"golang.org/x/tools/go/analysis"
-	"golang.org/x/tools/go/analysis/internal/checker"
+	"golang.org/x/tools/go/analysis/checker"
+	"golang.org/x/tools/go/analysis/internal"
 	"golang.org/x/tools/go/packages"
 	"golang.org/x/tools/internal/diff"
 	"golang.org/x/tools/internal/testenv"
@@ -137,7 +138,7 @@ type Testing interface {
 // analyzers that offer alternative fixes are advised to put each fix
 // in a separate .go file in the testdata.
 func RunWithSuggestedFixes(t Testing, dir string, a *analysis.Analyzer, patterns ...string) []*Result {
-	r := Run(t, dir, a, patterns...)
+	results := Run(t, dir, a, patterns...)
 
 	// If the immediate caller of RunWithSuggestedFixes is in
 	// x/tools, we apply stricter checks as required by gopls.
@@ -162,7 +163,9 @@ func RunWithSuggestedFixes(t Testing, dir string, a *analysis.Analyzer, patterns
 	// Validating the results separately means as long as the two analyses
 	// don't produce conflicting suggestions for a single file, everything
 	// should match up.
-	for _, act := range r {
+	for _, result := range results {
+		act := result.Action
+
 		// file -> message -> edits
 		fileEdits := make(map[*token.File]map[string][]diff.Edit)
 		fileContents := make(map[*token.File][]byte)
@@ -185,14 +188,14 @@ func RunWithSuggestedFixes(t Testing, dir string, a *analysis.Analyzer, patterns
 					if start > end {
 						t.Errorf(
 							"diagnostic for analysis %v contains Suggested Fix with malformed edit: pos (%v) > end (%v)",
-							act.Pass.Analyzer.Name, start, end)
+							act.Analyzer.Name, start, end)
 						continue
 					}
-					file, endfile := act.Pass.Fset.File(start), act.Pass.Fset.File(end)
+					file, endfile := act.Package.Fset.File(start), act.Package.Fset.File(end)
 					if file == nil || endfile == nil || file != endfile {
 						t.Errorf(
 							"diagnostic for analysis %v contains Suggested Fix with malformed spanning files %v and %v",
-							act.Pass.Analyzer.Name, file.Name(), endfile.Name())
+							act.Analyzer.Name, file.Name(), endfile.Name())
 						continue
 					}
 					if _, ok := fileContents[file]; !ok {
@@ -275,7 +278,7 @@ func RunWithSuggestedFixes(t Testing, dir string, a *analysis.Analyzer, patterns
 			}
 		}
 	}
-	return r
+	return results
 }
 
 // applyDiffsAndCompare applies edits to src and compares the results against
@@ -355,24 +358,76 @@ func Run(t Testing, dir string, a *analysis.Analyzer, patterns ...string) []*Res
 		return nil
 	}
 
-	if err := analysis.Validate([]*analysis.Analyzer{a}); err != nil {
-		t.Errorf("Validate: %v", err)
+	// Print parse and type errors to the test log.
+	// (Do not print them to stderr, which would pollute
+	// the log in cases where the tests pass.)
+	if t, ok := t.(testing.TB); ok && !a.RunDespiteErrors {
+		packages.Visit(pkgs, nil, func(pkg *packages.Package) {
+			for _, err := range pkg.Errors {
+				t.Log(err)
+			}
+		})
+	}
+
+	res, err := checker.Analyze([]*analysis.Analyzer{a}, pkgs, nil)
+	if err != nil {
+		t.Errorf("Analyze: %v", err)
 		return nil
 	}
 
-	results := checker.TestAnalyzer(a, pkgs)
-	for _, result := range results {
-		if result.Err != nil {
-			t.Errorf("error analyzing %s: %v", result.Pass, result.Err)
+	var results []*Result
+	for _, act := range res.Roots {
+		if act.Err != nil {
+			t.Errorf("error analyzing %s: %v", act, act.Err)
 		} else {
-			check(t, dir, result.Pass, result.Diagnostics, result.Facts)
+			check(t, dir, act)
 		}
+
+		// Compute legacy map of facts relating to this package.
+		facts := make(map[types.Object][]analysis.Fact)
+		for _, objFact := range act.AllObjectFacts() {
+			if obj := objFact.Object; obj.Pkg() == act.Package.Types {
+				facts[obj] = append(facts[obj], objFact.Fact)
+			}
+		}
+		for _, pkgFact := range act.AllPackageFacts() {
+			if pkgFact.Package == act.Package.Types {
+				facts[nil] = append(facts[nil], pkgFact.Fact)
+			}
+		}
+
+		// Construct the legacy result.
+		results = append(results, &Result{
+			Pass:        internal.Pass(act),
+			Diagnostics: act.Diagnostics,
+			Facts:       facts,
+			Result:      act.Result,
+			Err:         act.Err,
+			Action:      act,
+		})
 	}
 	return results
 }
 
 // A Result holds the result of applying an analyzer to a package.
-type Result = checker.TestAnalyzerResult
+//
+// Facts contains only facts associated with the package and its objects.
+//
+// This internal type was inadvertently and regrettably exposed
+// through a public type alias. It is essentially redundant with
+// [checker.Action], but must be retained for compatibility. Clients may
+// access the public fields of the Pass but must not invoke any of
+// its "verbs", since the pass is already complete.
+type Result struct {
+	Action *checker.Action
+
+	// legacy fields
+	Facts       map[types.Object][]analysis.Fact // nil key => package fact
+	Pass        *analysis.Pass
+	Diagnostics []analysis.Diagnostic // see Action.Diagnostics
+	Result      any                   // see Action.Result
+	Err         error                 // see Action.Err
+}
 
 // loadPackages uses go/packages to load a specified packages (from source, with
 // dependencies) from dir, which is the root of a GOPATH-style project tree.
@@ -421,16 +476,6 @@ func loadPackages(a *analysis.Analyzer, dir string, patterns ...string) ([]*pack
 		}
 	}
 
-	// Do NOT print errors if the analyzer will continue running.
-	// It is incredibly confusing for tests to be printing to stderr
-	// willy-nilly instead of their test logs, especially when the
-	// errors are expected and are going to be fixed.
-	if !a.RunDespiteErrors {
-		if packages.PrintErrors(pkgs) > 0 {
-			return nil, fmt.Errorf("there were package loading errors (and RunDespiteErrors is false)")
-		}
-	}
-
 	if len(pkgs) == 0 {
 		return nil, fmt.Errorf("no packages matched %s", patterns)
 	}
@@ -441,7 +486,7 @@ func loadPackages(a *analysis.Analyzer, dir string, patterns ...string) ([]*pack
 // been run, and verifies that all reported diagnostics and facts match
 // specified by the contents of "// want ..." comments in the package's
 // source files, which must have been parsed with comments enabled.
-func check(t Testing, gopath string, pass *analysis.Pass, diagnostics []analysis.Diagnostic, facts map[types.Object][]analysis.Fact) {
+func check(t Testing, gopath string, act *checker.Action) {
 	type key struct {
 		file string
 		line int
@@ -468,7 +513,7 @@ func check(t Testing, gopath string, pass *analysis.Pass, diagnostics []analysis
 	}
 
 	// Extract 'want' comments from parsed Go files.
-	for _, f := range pass.Files {
+	for _, f := range act.Package.Syntax {
 		for _, cgroup := range f.Comments {
 			for _, c := range cgroup.List {
 
@@ -491,7 +536,7 @@ func check(t Testing, gopath string, pass *analysis.Pass, diagnostics []analysis
 				// once outside the loop, but it's
 				// incorrect because it can change due
 				// to //line directives.
-				posn := pass.Fset.Position(c.Pos())
+				posn := act.Package.Fset.Position(c.Pos())
 				filename := sanitize(gopath, posn.Filename)
 				processComment(filename, posn.Line, text)
 			}
@@ -500,7 +545,17 @@ func check(t Testing, gopath string, pass *analysis.Pass, diagnostics []analysis
 
 	// Extract 'want' comments from non-Go files.
 	// TODO(adonovan): we may need to handle //line directives.
-	for _, filename := range pass.OtherFiles {
+	files := act.Package.OtherFiles
+
+	// Hack: these two analyzers need to extract expectations from
+	// all configurations, so include the files are are usually
+	// ignored. (This was previously a hack in the respective
+	// analyzers' tests.)
+	if act.Analyzer.Name == "buildtag" || act.Analyzer.Name == "directive" {
+		files = append(files[:len(files):len(files)], act.Package.IgnoredFiles...)
+	}
+
+	for _, filename := range files {
 		data, err := os.ReadFile(filename)
 		if err != nil {
 			t.Errorf("can't read '// want' comments from %s: %v", filename, err)
@@ -553,45 +608,38 @@ func check(t Testing, gopath string, pass *analysis.Pass, diagnostics []analysis
 	}
 
 	// Check the diagnostics match expectations.
-	for _, f := range diagnostics {
+	for _, f := range act.Diagnostics {
 		// TODO(matloob): Support ranges in analysistest.
-		posn := pass.Fset.Position(f.Pos)
+		posn := act.Package.Fset.Position(f.Pos)
 		checkMessage(posn, "diagnostic", "", f.Message)
 	}
 
 	// Check the facts match expectations.
-	// Report errors in lexical order for determinism.
+	// We check only facts relating to the current package.
+	//
+	// We report errors in lexical order for determinism.
 	// (It's only deterministic within each file, not across files,
 	// because go/packages does not guarantee file.Pos is ascending
 	// across the files of a single compilation unit.)
-	var objects []types.Object
-	for obj := range facts {
-		objects = append(objects, obj)
-	}
-	sort.Slice(objects, func(i, j int) bool {
-		// Package facts compare less than object facts.
-		ip, jp := objects[i] == nil, objects[j] == nil // whether i, j is a package fact
-		if ip != jp {
-			return ip && !jp
-		}
-		return objects[i].Pos() < objects[j].Pos()
-	})
-	for _, obj := range objects {
-		var posn token.Position
-		var name string
-		if obj != nil {
-			// Object facts are reported on the declaring line.
-			name = obj.Name()
-			posn = pass.Fset.Position(obj.Pos())
-		} else {
-			// Package facts are reported at the start of the file.
-			name = "package"
-			posn = pass.Fset.Position(pass.Files[0].Pos())
-			posn.Line = 1
+
+	// package facts: reported at start of first file
+	for _, pkgFact := range act.AllPackageFacts() {
+		if pkgFact.Package == act.Package.Types {
+			posn := act.Package.Fset.Position(act.Package.Syntax[0].Pos())
+			posn.Line, posn.Column = 1, 1
+			checkMessage(posn, "fact", "package", fmt.Sprint(pkgFact))
 		}
+	}
 
-		for _, fact := range facts[obj] {
-			checkMessage(posn, "fact", name, fmt.Sprint(fact))
+	// object facts: reported at line of object declaration
+	objFacts := act.AllObjectFacts()
+	sort.Slice(objFacts, func(i, j int) bool {
+		return objFacts[i].Object.Pos() < objFacts[j].Object.Pos()
+	})
+	for _, objFact := range objFacts {
+		if obj := objFact.Object; obj.Pkg() == act.Package.Types {
+			posn := act.Package.Fset.Position(obj.Pos())
+			checkMessage(posn, "fact", obj.Name(), fmt.Sprint(objFact.Fact))
 		}
 	}