From 74c9cfe4d22faa696baabeea02df6493b15e8c79 Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Wed, 24 Apr 2024 15:20:14 -0400 Subject: [PATCH] go/analysis: add Pass.ReadFile The ReadFile function enables an analyzer to read the contents of a file through the driver. The allows the driver to provide the analyzer with a view of the file system that is consistent with that seen by the parser and type checker, and enables drivers to reject attempts to read arbitrary files, so that their dependency analysis (if any) can be sound and precise. + a test Fixes golang/go#62292 Change-Id: I9f301cbfd4a705a259f9e213fc2e63d74424294a Reviewed-on: https://go-review.googlesource.com/c/tools/+/581555 Reviewed-by: Robert Findley LUCI-TryBot-Result: Go LUCI Auto-Submit: Alan Donovan --- go/analysis/analysis.go | 13 +++ go/analysis/doc.go | 30 +++---- go/analysis/internal/checker/checker.go | 4 + go/analysis/internal/checker/checker_test.go | 90 +++++++++++++++++++ go/analysis/passes/asmdecl/asmdecl.go | 2 +- go/analysis/passes/buildtag/buildtag.go | 2 +- go/analysis/passes/buildtag/buildtag_old.go | 2 +- go/analysis/passes/directive/directive.go | 2 +- .../passes/framepointer/framepointer.go | 2 +- .../passes/internal/analysisutil/util.go | 11 ++- go/analysis/unitchecker/unitchecker.go | 2 + gopls/internal/cache/analysis.go | 4 + internal/analysisinternal/analysis.go | 37 ++++++++ 13 files changed, 177 insertions(+), 24 deletions(-) diff --git a/go/analysis/analysis.go b/go/analysis/analysis.go index 5da33c7e6e1..52117736574 100644 --- a/go/analysis/analysis.go +++ b/go/analysis/analysis.go @@ -112,6 +112,19 @@ type Pass struct { // analysis's ResultType. ResultOf map[*Analyzer]interface{} + // ReadFile returns the contents of the named file. + // + // The only valid file names are the elements of OtherFiles + // and IgnoredFiles, and names returned by + // Fset.File(f.FileStart).Name() for each f in Files. + // + // Analyzers must use this function (if provided) instead of + // accessing the file system directly. This allows a driver to + // provide a virtualized file tree (including, for example, + // unsaved editor buffers) and to track dependencies precisely + // to avoid unnecessary recomputation. + ReadFile func(filename string) ([]byte, error) + // -- facts -- // ImportObjectFact retrieves a fact associated with obj. diff --git a/go/analysis/doc.go b/go/analysis/doc.go index 44867d599e4..2a0aa577126 100644 --- a/go/analysis/doc.go +++ b/go/analysis/doc.go @@ -32,7 +32,7 @@ bases, and so on. # Analyzer -The primary type in the API is Analyzer. An Analyzer statically +The primary type in the API is [Analyzer]. An Analyzer statically describes an analysis function: its name, documentation, flags, relationship to other analyzers, and of course, its logic. @@ -72,7 +72,7 @@ help that describes the analyses it performs. The doc comment contains a brief one-line summary, optionally followed by paragraphs of explanation. -The Analyzer type has more fields besides those shown above: +The [Analyzer] type has more fields besides those shown above: type Analyzer struct { Name string @@ -114,7 +114,7 @@ instance of the Pass type. # Pass -A Pass describes a single unit of work: the application of a particular +A [Pass] describes a single unit of work: the application of a particular Analyzer to a particular package of Go code. The Pass provides information to the Analyzer's Run function about the package being analyzed, and provides operations to the Run function for @@ -135,16 +135,14 @@ reporting diagnostics and other information back to the driver. The Fset, Files, Pkg, and TypesInfo fields provide the syntax trees, type information, and source positions for a single package of Go code. -The OtherFiles field provides the names, but not the contents, of non-Go -files such as assembly that are part of this package. See the "asmdecl" -or "buildtags" analyzers for examples of loading non-Go files and reporting -diagnostics against them. - -The IgnoredFiles field provides the names, but not the contents, -of ignored Go and non-Go source files that are not part of this package -with the current build configuration but may be part of other build -configurations. See the "buildtags" analyzer for an example of loading -and checking IgnoredFiles. +The OtherFiles field provides the names of non-Go +files such as assembly that are part of this package. +Similarly, the IgnoredFiles field provides the names of Go and non-Go +source files that are not part of this package with the current build +configuration but may be part of other build configurations. +The contents of these files may be read using Pass.ReadFile; +see the "asmdecl" or "buildtags" analyzers for examples of loading +non-Go files and reporting diagnostics against them. The ResultOf field provides the results computed by the analyzers required by this one, as expressed in its Analyzer.Requires field. The @@ -177,7 +175,7 @@ Diagnostic is defined as: The optional Category field is a short identifier that classifies the kind of message when an analysis produces several kinds of diagnostic. -The Diagnostic struct does not have a field to indicate its severity +The [Diagnostic] struct does not have a field to indicate its severity because opinions about the relative importance of Analyzers and their diagnostics vary widely among users. The design of this framework does not hold each Analyzer responsible for identifying the severity of its @@ -191,7 +189,7 @@ and buildtag, inspect the raw text of Go source files or even non-Go files such as assembly. To report a diagnostic against a line of a raw text file, use the following sequence: - content, err := os.ReadFile(filename) + content, err := pass.ReadFile(filename) if err != nil { ... } tf := fset.AddFile(filename, -1, len(content)) tf.SetLinesForContent(content) @@ -216,7 +214,7 @@ addition, it records which functions are printf wrappers for use by later analysis passes to identify other printf wrappers by induction. A result such as “f is a printf wrapper” that is not interesting by itself but serves as a stepping stone to an interesting result (such as -a diagnostic) is called a "fact". +a diagnostic) is called a [Fact]. The analysis API allows an analysis to define new types of facts, to associate facts of these types with objects (named entities) declared diff --git a/go/analysis/internal/checker/checker.go b/go/analysis/internal/checker/checker.go index 3c893500890..52f0e55aaae 100644 --- a/go/analysis/internal/checker/checker.go +++ b/go/analysis/internal/checker/checker.go @@ -31,6 +31,7 @@ import ( "golang.org/x/tools/go/analysis" "golang.org/x/tools/go/analysis/internal/analysisflags" "golang.org/x/tools/go/packages" + "golang.org/x/tools/internal/analysisinternal" "golang.org/x/tools/internal/diff" "golang.org/x/tools/internal/robustio" ) @@ -432,6 +433,8 @@ func applyFixes(roots []*action) error { // Now we've got a set of valid edits for each file. Apply them. for path, edits := range editsByPath { + // TODO(adonovan): this should really work on the same + // gulp from the file system that fed the analyzer (see #62292). contents, err := os.ReadFile(path) if err != nil { return err @@ -766,6 +769,7 @@ func (act *action) execOnce() { AllObjectFacts: act.allObjectFacts, AllPackageFacts: act.allPackageFacts, } + pass.ReadFile = analysisinternal.MakeReadFile(pass) act.pass = pass var err error diff --git a/go/analysis/internal/checker/checker_test.go b/go/analysis/internal/checker/checker_test.go index b383f29a985..69ea944d888 100644 --- a/go/analysis/internal/checker/checker_test.go +++ b/go/analysis/internal/checker/checker_test.go @@ -10,6 +10,7 @@ import ( "os" "path/filepath" "reflect" + "strings" "testing" "golang.org/x/tools/go/analysis" @@ -18,6 +19,8 @@ import ( "golang.org/x/tools/go/analysis/passes/inspect" "golang.org/x/tools/go/ast/inspector" "golang.org/x/tools/internal/testenv" + "golang.org/x/tools/internal/testfiles" + "golang.org/x/tools/txtar" ) func TestApplyFixes(t *testing.T) { @@ -254,3 +257,90 @@ func TestURL(t *testing.T) { t.Errorf("Expected Diagnostics.URLs %v. got %v", want, urls) } } + +// TestPassReadFile exercises the Pass.ReadFile function. +func TestPassReadFile(t *testing.T) { + cwd, _ := os.Getwd() + + const src = ` +-- go.mod -- +module example.com + +-- p/file.go -- +package p + +-- p/ignored.go -- +//go:build darwin && mips64 + +package p + +hello from ignored + +-- p/other.s -- +hello from other +` + + // Expand archive into tmp tree. + tmpdir := t.TempDir() + if err := testfiles.ExtractTxtar(tmpdir, txtar.Parse([]byte(src))); err != nil { + t.Fatal(err) + } + + ran := false + a := &analysis.Analyzer{ + Name: "a", + Requires: []*analysis.Analyzer{inspect.Analyzer}, + Doc: "doc", + Run: func(pass *analysis.Pass) (any, error) { + if len(pass.OtherFiles)+len(pass.IgnoredFiles) == 0 { + t.Errorf("OtherFiles and IgnoredFiles are empty") + return nil, nil + } + + for _, test := range []struct { + filename string + want string // substring of file content or error message + }{ + { + pass.OtherFiles[0], // [other.s] + "hello from other", + }, + { + pass.IgnoredFiles[0], // [ignored.go] + "hello from ignored", + }, + { + "nonesuch", + "nonesuch is not among OtherFiles, ", // etc + }, + { + filepath.Join(cwd, "checker_test.go"), + "checker_test.go is not among OtherFiles, ", // etc + }, + } { + content, err := pass.ReadFile(test.filename) + var got string + if err != nil { + got = err.Error() + } else { + got = string(content) + if len(got) > 100 { + got = got[:100] + "..." + } + } + if !strings.Contains(got, test.want) { + t.Errorf("Pass.ReadFile(%q) did not contain %q; got:\n%s", + test.filename, test.want, got) + } + } + ran = true + return nil, nil + }, + } + + analysistest.Run(t, tmpdir, a, "example.com/p") + + if !ran { + t.Error("analyzer did not run") + } +} diff --git a/go/analysis/passes/asmdecl/asmdecl.go b/go/analysis/passes/asmdecl/asmdecl.go index f2ca95aa9eb..3417232ce35 100644 --- a/go/analysis/passes/asmdecl/asmdecl.go +++ b/go/analysis/passes/asmdecl/asmdecl.go @@ -173,7 +173,7 @@ func run(pass *analysis.Pass) (interface{}, error) { Files: for _, fname := range sfiles { - content, tf, err := analysisutil.ReadFile(pass.Fset, fname) + content, tf, err := analysisutil.ReadFile(pass, fname) if err != nil { return nil, err } diff --git a/go/analysis/passes/buildtag/buildtag.go b/go/analysis/passes/buildtag/buildtag.go index 55bdad78b76..51ba2a91e5b 100644 --- a/go/analysis/passes/buildtag/buildtag.go +++ b/go/analysis/passes/buildtag/buildtag.go @@ -89,7 +89,7 @@ func checkOtherFile(pass *analysis.Pass, filename string) error { // We cannot use the Go parser, since this may not be a Go source file. // Read the raw bytes instead. - content, tf, err := analysisutil.ReadFile(pass.Fset, filename) + content, tf, err := analysisutil.ReadFile(pass, filename) if err != nil { return err } diff --git a/go/analysis/passes/buildtag/buildtag_old.go b/go/analysis/passes/buildtag/buildtag_old.go index 0001ba53639..19ef6b9bce4 100644 --- a/go/analysis/passes/buildtag/buildtag_old.go +++ b/go/analysis/passes/buildtag/buildtag_old.go @@ -83,7 +83,7 @@ func checkGoFile(pass *analysis.Pass, f *ast.File) { } func checkOtherFile(pass *analysis.Pass, filename string) error { - content, tf, err := analysisutil.ReadFile(pass.Fset, filename) + content, tf, err := analysisutil.ReadFile(pass, filename) if err != nil { return err } diff --git a/go/analysis/passes/directive/directive.go b/go/analysis/passes/directive/directive.go index 2691f189aae..f6727c5ada0 100644 --- a/go/analysis/passes/directive/directive.go +++ b/go/analysis/passes/directive/directive.go @@ -90,7 +90,7 @@ func checkGoFile(pass *analysis.Pass, f *ast.File) { func checkOtherFile(pass *analysis.Pass, filename string) error { // We cannot use the Go parser, since is not a Go source file. // Read the raw bytes instead. - content, tf, err := analysisutil.ReadFile(pass.Fset, filename) + content, tf, err := analysisutil.ReadFile(pass, filename) if err != nil { return err } diff --git a/go/analysis/passes/framepointer/framepointer.go b/go/analysis/passes/framepointer/framepointer.go index 0b3ded47eaf..6eff3a20fea 100644 --- a/go/analysis/passes/framepointer/framepointer.go +++ b/go/analysis/passes/framepointer/framepointer.go @@ -48,7 +48,7 @@ func run(pass *analysis.Pass) (interface{}, error) { } for _, fname := range sfiles { - content, tf, err := analysisutil.ReadFile(pass.Fset, fname) + content, tf, err := analysisutil.ReadFile(pass, fname) if err != nil { return nil, err } diff --git a/go/analysis/passes/internal/analysisutil/util.go b/go/analysis/passes/internal/analysisutil/util.go index 89291602a5b..f7f071dc8be 100644 --- a/go/analysis/passes/internal/analysisutil/util.go +++ b/go/analysis/passes/internal/analysisutil/util.go @@ -14,6 +14,7 @@ import ( "go/types" "os" + "golang.org/x/tools/go/analysis" "golang.org/x/tools/internal/aliases" "golang.org/x/tools/internal/analysisinternal" ) @@ -60,12 +61,16 @@ func HasSideEffects(info *types.Info, e ast.Expr) bool { // ReadFile reads a file and adds it to the FileSet // so that we can report errors against it using lineStart. -func ReadFile(fset *token.FileSet, filename string) ([]byte, *token.File, error) { - content, err := os.ReadFile(filename) +func ReadFile(pass *analysis.Pass, filename string) ([]byte, *token.File, error) { + readFile := pass.ReadFile + if readFile == nil { + readFile = os.ReadFile + } + content, err := readFile(filename) if err != nil { return nil, nil, err } - tf := fset.AddFile(filename, -1, len(content)) + tf := pass.Fset.AddFile(filename, -1, len(content)) tf.SetLinesForContent(content) return content, tf, nil } diff --git a/go/analysis/unitchecker/unitchecker.go b/go/analysis/unitchecker/unitchecker.go index 1fa0d1f68f9..d77fb203d85 100644 --- a/go/analysis/unitchecker/unitchecker.go +++ b/go/analysis/unitchecker/unitchecker.go @@ -49,6 +49,7 @@ import ( "golang.org/x/tools/go/analysis" "golang.org/x/tools/go/analysis/internal/analysisflags" + "golang.org/x/tools/internal/analysisinternal" "golang.org/x/tools/internal/facts" "golang.org/x/tools/internal/versions" ) @@ -377,6 +378,7 @@ func run(fset *token.FileSet, cfg *Config, analyzers []*analysis.Analyzer) ([]re ExportPackageFact: facts.ExportPackageFact, AllPackageFacts: func() []analysis.PackageFact { return facts.AllPackageFacts(factFilter) }, } + pass.ReadFile = analysisinternal.MakeReadFile(pass) t0 := time.Now() act.result, act.err = a.Run(pass) diff --git a/gopls/internal/cache/analysis.go b/gopls/internal/cache/analysis.go index 79ce29d3dce..e2ea8cbb90a 100644 --- a/gopls/internal/cache/analysis.go +++ b/gopls/internal/cache/analysis.go @@ -44,6 +44,7 @@ import ( "golang.org/x/tools/gopls/internal/util/bug" "golang.org/x/tools/gopls/internal/util/frob" "golang.org/x/tools/gopls/internal/util/maps" + "golang.org/x/tools/internal/analysisinternal" "golang.org/x/tools/internal/event" "golang.org/x/tools/internal/facts" "golang.org/x/tools/internal/gcimporter" @@ -1363,6 +1364,9 @@ func (act *action) exec() (interface{}, *actionSummary, error) { AllObjectFacts: func() []analysis.ObjectFact { return factset.AllObjectFacts(factFilter) }, AllPackageFacts: func() []analysis.PackageFact { return factset.AllPackageFacts(factFilter) }, } + // TODO(adonovan): integrate this into the snapshot's file + // cache and its dependency analysis. + pass.ReadFile = analysisinternal.MakeReadFile(pass) // Recover from panics (only) within the analyzer logic. // (Use an anonymous function to limit the recover scope.) diff --git a/internal/analysisinternal/analysis.go b/internal/analysisinternal/analysis.go index eb830888aad..2c406ded0c9 100644 --- a/internal/analysisinternal/analysis.go +++ b/internal/analysisinternal/analysis.go @@ -12,8 +12,10 @@ import ( "go/ast" "go/token" "go/types" + "os" "strconv" + "golang.org/x/tools/go/analysis" "golang.org/x/tools/internal/aliases" ) @@ -393,3 +395,38 @@ 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) { + return func(filename string) ([]byte, error) { + if err := checkReadable(pass, filename); err != nil { + return nil, err + } + return os.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) { + return nil + } + for _, f := range pass.Files { + // TODO(adonovan): use go1.20 f.FileStart + if pass.Fset.File(f.Pos()).Name() == filename { + return nil + } + } + 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 +}