x/tools/go/analysis: add field Pass.ReadFile func

[adonovan]

Analyzers in the golang.org/x/tools/go/analysis framework sometimes need to read the content of one of the package's Go source files, or perhaps one of the other files (e.g. assembly, cgo, etc). For example, the inlining tool under construction in https://go.dev/cl/519715 needs to read the source of a file so that it can provide a suggested fix for inlining a function. (It is infeasible to do this by working at the AST level as it is lossy.)

Although it is trivial for an analyzer to call os.ReadFile, it may be subtly incorrect, as the source from which the analysis.Pass was generated may not be consistent with the source in the file system if, for example, the analysis driver operates on unsaved editor buffers, as gopls does.

We propose to add a ReadFile field to analysis.Pass that is a function with the same signature as os.ReadFile. Analyzers should use this function (if present) instead of os.ReadFile. This allows the analysis driver to provide the correct file contents for any of the files mentioned by the Pass.

@findleyr

[adonovan]

An alternative approach would be to expose one of the new family of io.FS interfaces instead of a single ReadFile operation. It's not obvious to me what practical benefit it would provide, and it certainly adds a complexity cost, but I mention it in case someone with more experience than me of io.FS sees an opportunity.

[rsc]

This doesn't seem like enough to me. If we can support replacing files, why not adding or removing files? To do that, you need ReadDir in addition to ReadFile. See go/build.Context for example, although ReadDir should now probably return []DirEntry not []FileInfo.

[seankhliao]

cmd/go already has an -overlay flag, does analysis need to support that?

[rsc]

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

[lfolger]

I think for some of our use cases it might be difficult (not impossible) to provide more than the source files of the packages. I would appreciate if this could be limited to the source files of the package under analysis unless there is evidence that more is needed. Maybe the go.mod file is needed as well.

I don't have an opinion if this should be an io.FS or a simple ReadFile function.

[adonovan]

I should have stated explicitly my intent that the only files that should be accessible through this API would be those belonging to the pass's package, in other words, those named in the Pass.{,CompiledGo,Ignored,Other}Files fields.

[adonovan]

[rsc] This doesn't seem like enough to me. If we can support replacing files, why not adding or removing files?

What do you mean by "support replacing"? Are you referring to the overlay mechanism that allows an analysis driver to work on a virtualized file tree? Or to the ability of an analyzer to offer fixes that replace portions of existing files? (The API provides no way for an analyzer to create a new file.)

To do that, you need ReadDir in addition to ReadFile.

There's an elegance to the idea of using fs.FS to define a read-only view of the portion of the file tree relevant to the current package, but it's hard to see what additional useful information it could provide beyond ReadFile. Directory information is not especially useful, since it has already been heavily processed by the build tool to populate the existing fields of Pass. It's also too much information, because in some build systems the other files in the directory may belong to different packages, so arguably we should redact them from the FS view, which again raises the question of what value it offers over Pass.*Files. (If we don't then dependency analysis becomes more complex because analysis of a package may depend on things outside the package.)

I still think ReadFile is the correct interface.

[timothy-king]

Pass.{,CompiledGo,Ignored,Other}Files fields.

If the is Go module file path is added to Pass, this would be another good candidate.

[adonovan]

If the is Go module file path is added to Pass, this would be another good candidate.

Could you clarify?

[findleyr]

I agree with @adonovan that ReadFile is correct, with the documentation that analyzers should not access files outside of the package. In other words, analyzers should only read the content of Files, OtherFiles, or IgnoredFiles. If we do decide to expose module information, I suppose it should be OK to read the go.mod or go.work file as well, but that is a separate discussion.

I don't think it would ever be "correct" for an analyzer to care about directories or files in another package. That would violate the modular nature of analysis. I suppose it's even debatable whether analyzers should be able to access the content of, say, IgnoredFiles, but since the IgnoredFiles field exists, it must be OK for analyzers to care about them.

Other alternatives that I'd reject:

• Adding a pass.FileContents [][]byte (or similar) field containing the materialized content of each file. This is perhaps more consistent with pass.Files (after all, we don't have pass.ParseFile), but would result in too much wasted memory, and would be harder for drivers to implement.
• Add an API more similar to go/build.Context (OpenFile). This is more cumbersome for both drivers and analyzers.

[adonovan]

analyzers should not access files outside of the package. In other words, analyzers should only read the content of Files, OtherFiles, or IgnoredFiles.

s/should not/cannot/

I think we should state that ReadFile returns an error for any file not named in the Pass.

[rsc]

Having ReadFile to get back to the original source makes perfect sense. I thought this was about pretending that pending work had been written out.

[rsc]

Have all remaining concerns about this proposal been addressed?

The proposal is to add a new field in struct Pass:

// ReadFile returns the content of the named file.
// Valid names to pass to ReadFile are the names obtained from Fset
// or listed in OtherFiles or IgnoredFiles.
ReadFile func(name string) ([]byte, error)

[rsc]

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

The proposal is to add a new field in struct Pass:

// ReadFile returns the content of the named file.
// Valid names to pass to ReadFile are the names obtained from Fset
// or listed in OtherFiles or IgnoredFiles.
ReadFile func(name string) ([]byte, error)

[findleyr]

// Valid names to pass to ReadFile are the names obtained from Fset

This makes it sound like any name in the Fset is OK, and that's not quite right. FileSets tend to contain extra files, depending on the scope of the pass, and we shouldn't support reading everything in the FileSet.

We could say "Valid names to pass to ReadFile are the names of Files (which may be obtained from Fset) or names in OtherFiles or IgnoredFiles.".

[adonovan]

Agreed. In particular, there may be an unbounded number of 'alt' files in the FileSet due to //line directives, and there is no way to even enumerate them (short of calling fset.Position on every integer).

[rsc]

Note that Files has no strings. It has to be that the names are taken from Fset but only for a lookup from a File.Start position.

[rsc]

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

The proposal is to add a new field in struct Pass:

// ReadFile returns the content of the named file.
// Valid names to pass to ReadFile are the names obtained from Fset
// or listed in OtherFiles or IgnoredFiles.
ReadFile func(name string) ([]byte, error)

[gopherbot]

Change https://go.dev/cl/581555 mentions this issue: go/analysis: add Pass.ReadFile