Skip to content

Commit ae39b13

Browse files
committed
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 <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]>
1 parent c3a6283 commit ae39b13

File tree

17 files changed

+1113
-752
lines changed

17 files changed

+1113
-752
lines changed

go/analysis/analysis.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ type Analyzer struct {
5050
// RunDespiteErrors allows the driver to invoke
5151
// the Run method of this analyzer even on a
5252
// package that contains parse or type errors.
53-
// The Pass.TypeErrors field may consequently be non-empty.
53+
// The [Pass.TypeErrors] field may consequently be non-empty.
5454
RunDespiteErrors bool
5555

5656
// Requires is a set of analyzers that must run successfully

go/analysis/analysistest/analysistest.go

+106-58
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,8 @@ import (
2323
"text/scanner"
2424

2525
"golang.org/x/tools/go/analysis"
26-
"golang.org/x/tools/go/analysis/internal/checker"
26+
"golang.org/x/tools/go/analysis/checker"
27+
"golang.org/x/tools/go/analysis/internal"
2728
"golang.org/x/tools/go/packages"
2829
"golang.org/x/tools/internal/diff"
2930
"golang.org/x/tools/internal/testenv"
@@ -137,7 +138,7 @@ type Testing interface {
137138
// analyzers that offer alternative fixes are advised to put each fix
138139
// in a separate .go file in the testdata.
139140
func RunWithSuggestedFixes(t Testing, dir string, a *analysis.Analyzer, patterns ...string) []*Result {
140-
r := Run(t, dir, a, patterns...)
141+
results := Run(t, dir, a, patterns...)
141142

142143
// If the immediate caller of RunWithSuggestedFixes is in
143144
// x/tools, we apply stricter checks as required by gopls.
@@ -162,7 +163,9 @@ func RunWithSuggestedFixes(t Testing, dir string, a *analysis.Analyzer, patterns
162163
// Validating the results separately means as long as the two analyses
163164
// don't produce conflicting suggestions for a single file, everything
164165
// should match up.
165-
for _, act := range r {
166+
for _, result := range results {
167+
act := result.Action
168+
166169
// file -> message -> edits
167170
fileEdits := make(map[*token.File]map[string][]diff.Edit)
168171
fileContents := make(map[*token.File][]byte)
@@ -185,14 +188,14 @@ func RunWithSuggestedFixes(t Testing, dir string, a *analysis.Analyzer, patterns
185188
if start > end {
186189
t.Errorf(
187190
"diagnostic for analysis %v contains Suggested Fix with malformed edit: pos (%v) > end (%v)",
188-
act.Pass.Analyzer.Name, start, end)
191+
act.Analyzer.Name, start, end)
189192
continue
190193
}
191-
file, endfile := act.Pass.Fset.File(start), act.Pass.Fset.File(end)
194+
file, endfile := act.Package.Fset.File(start), act.Package.Fset.File(end)
192195
if file == nil || endfile == nil || file != endfile {
193196
t.Errorf(
194197
"diagnostic for analysis %v contains Suggested Fix with malformed spanning files %v and %v",
195-
act.Pass.Analyzer.Name, file.Name(), endfile.Name())
198+
act.Analyzer.Name, file.Name(), endfile.Name())
196199
continue
197200
}
198201
if _, ok := fileContents[file]; !ok {
@@ -275,7 +278,7 @@ func RunWithSuggestedFixes(t Testing, dir string, a *analysis.Analyzer, patterns
275278
}
276279
}
277280
}
278-
return r
281+
return results
279282
}
280283

281284
// 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
355358
return nil
356359
}
357360

358-
if err := analysis.Validate([]*analysis.Analyzer{a}); err != nil {
359-
t.Errorf("Validate: %v", err)
361+
// Print parse and type errors to the test log.
362+
// (Do not print them to stderr, which would pollute
363+
// the log in cases where the tests pass.)
364+
if t, ok := t.(testing.TB); ok && !a.RunDespiteErrors {
365+
packages.Visit(pkgs, nil, func(pkg *packages.Package) {
366+
for _, err := range pkg.Errors {
367+
t.Log(err)
368+
}
369+
})
370+
}
371+
372+
res, err := checker.Analyze([]*analysis.Analyzer{a}, pkgs, nil)
373+
if err != nil {
374+
t.Errorf("Analyze: %v", err)
360375
return nil
361376
}
362377

363-
results := checker.TestAnalyzer(a, pkgs)
364-
for _, result := range results {
365-
if result.Err != nil {
366-
t.Errorf("error analyzing %s: %v", result.Pass, result.Err)
378+
var results []*Result
379+
for _, act := range res.Roots {
380+
if act.Err != nil {
381+
t.Errorf("error analyzing %s: %v", act, act.Err)
367382
} else {
368-
check(t, dir, result.Pass, result.Diagnostics, result.Facts)
383+
check(t, dir, act)
369384
}
385+
386+
// Compute legacy map of facts relating to this package.
387+
facts := make(map[types.Object][]analysis.Fact)
388+
for _, objFact := range act.AllObjectFacts() {
389+
if obj := objFact.Object; obj.Pkg() == act.Package.Types {
390+
facts[obj] = append(facts[obj], objFact.Fact)
391+
}
392+
}
393+
for _, pkgFact := range act.AllPackageFacts() {
394+
if pkgFact.Package == act.Package.Types {
395+
facts[nil] = append(facts[nil], pkgFact.Fact)
396+
}
397+
}
398+
399+
// Construct the legacy result.
400+
results = append(results, &Result{
401+
Pass: internal.Pass(act),
402+
Diagnostics: act.Diagnostics,
403+
Facts: facts,
404+
Result: act.Result,
405+
Err: act.Err,
406+
Action: act,
407+
})
370408
}
371409
return results
372410
}
373411

374412
// A Result holds the result of applying an analyzer to a package.
375-
type Result = checker.TestAnalyzerResult
413+
//
414+
// Facts contains only facts associated with the package and its objects.
415+
//
416+
// This internal type was inadvertently and regrettably exposed
417+
// through a public type alias. It is essentially redundant with
418+
// [checker.Action], but must be retained for compatibility. Clients may
419+
// access the public fields of the Pass but must not invoke any of
420+
// its "verbs", since the pass is already complete.
421+
type Result struct {
422+
Action *checker.Action
423+
424+
// legacy fields
425+
Facts map[types.Object][]analysis.Fact // nil key => package fact
426+
Pass *analysis.Pass
427+
Diagnostics []analysis.Diagnostic // see Action.Diagnostics
428+
Result any // see Action.Result
429+
Err error // see Action.Err
430+
}
376431

377432
// loadPackages uses go/packages to load a specified packages (from source, with
378433
// 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
421476
}
422477
}
423478

424-
// Do NOT print errors if the analyzer will continue running.
425-
// It is incredibly confusing for tests to be printing to stderr
426-
// willy-nilly instead of their test logs, especially when the
427-
// errors are expected and are going to be fixed.
428-
if !a.RunDespiteErrors {
429-
if packages.PrintErrors(pkgs) > 0 {
430-
return nil, fmt.Errorf("there were package loading errors (and RunDespiteErrors is false)")
431-
}
432-
}
433-
434479
if len(pkgs) == 0 {
435480
return nil, fmt.Errorf("no packages matched %s", patterns)
436481
}
@@ -441,7 +486,7 @@ func loadPackages(a *analysis.Analyzer, dir string, patterns ...string) ([]*pack
441486
// been run, and verifies that all reported diagnostics and facts match
442487
// specified by the contents of "// want ..." comments in the package's
443488
// source files, which must have been parsed with comments enabled.
444-
func check(t Testing, gopath string, pass *analysis.Pass, diagnostics []analysis.Diagnostic, facts map[types.Object][]analysis.Fact) {
489+
func check(t Testing, gopath string, act *checker.Action) {
445490
type key struct {
446491
file string
447492
line int
@@ -468,7 +513,7 @@ func check(t Testing, gopath string, pass *analysis.Pass, diagnostics []analysis
468513
}
469514

470515
// Extract 'want' comments from parsed Go files.
471-
for _, f := range pass.Files {
516+
for _, f := range act.Package.Syntax {
472517
for _, cgroup := range f.Comments {
473518
for _, c := range cgroup.List {
474519

@@ -491,7 +536,7 @@ func check(t Testing, gopath string, pass *analysis.Pass, diagnostics []analysis
491536
// once outside the loop, but it's
492537
// incorrect because it can change due
493538
// to //line directives.
494-
posn := pass.Fset.Position(c.Pos())
539+
posn := act.Package.Fset.Position(c.Pos())
495540
filename := sanitize(gopath, posn.Filename)
496541
processComment(filename, posn.Line, text)
497542
}
@@ -500,7 +545,17 @@ func check(t Testing, gopath string, pass *analysis.Pass, diagnostics []analysis
500545

501546
// Extract 'want' comments from non-Go files.
502547
// TODO(adonovan): we may need to handle //line directives.
503-
for _, filename := range pass.OtherFiles {
548+
files := act.Package.OtherFiles
549+
550+
// Hack: these two analyzers need to extract expectations from
551+
// all configurations, so include the files are are usually
552+
// ignored. (This was previously a hack in the respective
553+
// analyzers' tests.)
554+
if act.Analyzer.Name == "buildtag" || act.Analyzer.Name == "directive" {
555+
files = append(files[:len(files):len(files)], act.Package.IgnoredFiles...)
556+
}
557+
558+
for _, filename := range files {
504559
data, err := os.ReadFile(filename)
505560
if err != nil {
506561
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
553608
}
554609

555610
// Check the diagnostics match expectations.
556-
for _, f := range diagnostics {
611+
for _, f := range act.Diagnostics {
557612
// TODO(matloob): Support ranges in analysistest.
558-
posn := pass.Fset.Position(f.Pos)
613+
posn := act.Package.Fset.Position(f.Pos)
559614
checkMessage(posn, "diagnostic", "", f.Message)
560615
}
561616

562617
// Check the facts match expectations.
563-
// Report errors in lexical order for determinism.
618+
// We check only facts relating to the current package.
619+
//
620+
// We report errors in lexical order for determinism.
564621
// (It's only deterministic within each file, not across files,
565622
// because go/packages does not guarantee file.Pos is ascending
566623
// across the files of a single compilation unit.)
567-
var objects []types.Object
568-
for obj := range facts {
569-
objects = append(objects, obj)
570-
}
571-
sort.Slice(objects, func(i, j int) bool {
572-
// Package facts compare less than object facts.
573-
ip, jp := objects[i] == nil, objects[j] == nil // whether i, j is a package fact
574-
if ip != jp {
575-
return ip && !jp
576-
}
577-
return objects[i].Pos() < objects[j].Pos()
578-
})
579-
for _, obj := range objects {
580-
var posn token.Position
581-
var name string
582-
if obj != nil {
583-
// Object facts are reported on the declaring line.
584-
name = obj.Name()
585-
posn = pass.Fset.Position(obj.Pos())
586-
} else {
587-
// Package facts are reported at the start of the file.
588-
name = "package"
589-
posn = pass.Fset.Position(pass.Files[0].Pos())
590-
posn.Line = 1
624+
625+
// package facts: reported at start of first file
626+
for _, pkgFact := range act.AllPackageFacts() {
627+
if pkgFact.Package == act.Package.Types {
628+
posn := act.Package.Fset.Position(act.Package.Syntax[0].Pos())
629+
posn.Line, posn.Column = 1, 1
630+
checkMessage(posn, "fact", "package", fmt.Sprint(pkgFact))
591631
}
632+
}
592633

593-
for _, fact := range facts[obj] {
594-
checkMessage(posn, "fact", name, fmt.Sprint(fact))
634+
// object facts: reported at line of object declaration
635+
objFacts := act.AllObjectFacts()
636+
sort.Slice(objFacts, func(i, j int) bool {
637+
return objFacts[i].Object.Pos() < objFacts[j].Object.Pos()
638+
})
639+
for _, objFact := range objFacts {
640+
if obj := objFact.Object; obj.Pkg() == act.Package.Types {
641+
posn := act.Package.Fset.Position(obj.Pos())
642+
checkMessage(posn, "fact", obj.Name(), fmt.Sprint(objFact.Fact))
595643
}
596644
}
597645

0 commit comments

Comments
 (0)