Skip to content

Commit

Permalink
go/analysis: add Pass.ReadFile
Browse files Browse the repository at this point in the history
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 <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
  • Loading branch information
adonovan authored and gopherbot committed Apr 29, 2024
1 parent 5ef4fc9 commit 74c9cfe
Show file tree
Hide file tree
Showing 13 changed files with 177 additions and 24 deletions.
13 changes: 13 additions & 0 deletions go/analysis/analysis.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
30 changes: 14 additions & 16 deletions go/analysis/doc.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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)
Expand All @@ -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
Expand Down
4 changes: 4 additions & 0 deletions go/analysis/internal/checker/checker.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
90 changes: 90 additions & 0 deletions go/analysis/internal/checker/checker_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"os"
"path/filepath"
"reflect"
"strings"
"testing"

"golang.org/x/tools/go/analysis"
Expand All @@ -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) {
Expand Down Expand Up @@ -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")
}
}
2 changes: 1 addition & 1 deletion go/analysis/passes/asmdecl/asmdecl.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
2 changes: 1 addition & 1 deletion go/analysis/passes/buildtag/buildtag.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
2 changes: 1 addition & 1 deletion go/analysis/passes/buildtag/buildtag_old.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
2 changes: 1 addition & 1 deletion go/analysis/passes/directive/directive.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
2 changes: 1 addition & 1 deletion go/analysis/passes/framepointer/framepointer.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
11 changes: 8 additions & 3 deletions go/analysis/passes/internal/analysisutil/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -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
}
Expand Down
2 changes: 2 additions & 0 deletions go/analysis/unitchecker/unitchecker.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -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)
Expand Down
4 changes: 4 additions & 0 deletions gopls/internal/cache/analysis.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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.)
Expand Down
37 changes: 37 additions & 0 deletions internal/analysisinternal/analysis.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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
}

0 comments on commit 74c9cfe

Please sign in to comment.