x/tools/go/analysis: feature request: need a way of accessing analysis results from another go package

[adammw]

What version of Go are you using (go version)?

$ go version
go version go1.17.3 darwin/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output

$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/amalcontenti-wilson/Library/Caches/go-build"
GOENV="/Users/amalcontenti-wilson/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/amalcontenti-wilson/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/amalcontenti-wilson/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/Cellar/go/1.17.3/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.17.3/libexec/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="go1.17.3"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/dev/null"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/yq/18f4vk0j2m95ycmk2wk8j4qh0000gq/T/go-build1149757957=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I am looking to write a generator that statically analyses some code, and based on the code paths being used generates a manifest for AWS permissions, similar to how kubebuilder controller-gen walks the directory tree and analyses code comments, then produces a result.

What did you see instead?

x/tools/go/analysis package provides a Analyzer package that is great for writing single use code analyzers, but the only documented way of using the analyzers is via the singlechecker or multichecker packages which expose a Main() method and exit with a status code. This makes it hard to write any code that uses the analysis result directly.

The internal checker package does sort of what I would expect, having a "Run" method that returns an exit code, but is both internal (so cannot be imported) and does not return the analysis results.

What did you expect to see?

An exported method that was capable of running analyzers against a set of packages/files, and returning the result interface{} to the program execution flow.

My current workaround is to copy parts of golang.org/x/tools/go/analysis/checker/internal/checker into my own codebase, such as load() to load packages, and the action code to create and run an analysis pass, to be able to get the result directly, which is a fair amount of extra duplicated code to maintain.

[timothy-king]

There are 3 ways Analyzers can communicate at the moment: results, facts, and diagnostics. This gives a lot of flexibility for how information can be passed around. OTOH the analysis package is not suitable for all needs (notice that none of the x/tools/callgraph commands use it).

1. What are you trying to do? What kind of information needs to leave the analyzer and why? I need some detail to confirm that is this is not practical to serialize. How is information passed between packages?
2. What is the suggested new interface in detail? Which Passes are exposed? All executed? All top level? What is the function signature?
3. Are such Analyzers going to be compatible with unitchecker? (Related to which Passes are exported.)

FWIW this may eventually need a proposal for acceptance. IMO this is exposing more internal details than were previously. Older Analyzers might not be considered backwards compatible. We can try to iron out the above questions first though.

[adammw]

In the simplest form, what I'm trying to do is analyse a package's method calls, and serialize that into a separate YAML file. The type i'm using at the moment to move the results around is map[string]bool, where the string keys are the method call names. From these results, I then use that map to read/update/save a separate YAML file which lists what service calls are used by the software.

Perhaps using an Analyzer isn't appropriate for this case - what I'm essentially trying to do here is closer to code generation rather than code analysis or linting, which is what the analysis package appears to be geared towards. Are there more suitable packages for this, or is the suggestion that I "roll my own" and duplicate some of what the analyzer package is doing?

I did try a couple of approaches using the supplied facts and diagnostics APIs:
firstly, using a custom fact struct containing that map, and then using multichecker to run both analyzers, however there wasn't a clean way to "analyze" the yaml code since it's expecting the same package to be used for all analyzers.
secondly, using suggestedfixes to update the yaml file, however that didn't seemed to work in the default config (my guess is it doesn't like adding a non-go file to the Fset after the fact).

[timothy-king]

Perhaps using an Analyzer isn't appropriate for this case - what I'm essentially trying to do here is closer to code generation rather than code analysis or linting, which is what the analysis package appears to be geared towards. Are there more suitable packages for this, or is the suggestion that I "roll my own" and duplicate some of what the analyzer package is doing?

It is still not very clear to me what you are trying to do. Maybe you can zoom out a bit more? Or explain what you have tried?

FWIW I find it is much easier to think of analysis by thinking about the communication mechanisms: Facts, Results, Diagnostics, and the scheduling relationships of passes than "being about linting/code analysis". This is an insider view but this is an under the hood feature request.

Another tactic that may help is to describe what features you are trying to get out of being an Analyzer? This might indicate if it is simpler to roll your own infra.

Responses and questions about more minor points:

what I'm trying to do is analyse a package's method calls,

Which packages do you want to analyze the methods of? If you run the checker on P and P imports Q, do you know you need the results of Q? Is just P okay? What about Q's imports?

The type i'm using at the moment to move the results around is map[string]bool, where the string keys are the method call names.

This will admit very simple serializations that are stable over time. Facts may be appropriate.

From these results, I then use that map to read/update/save a separate YAML file which lists what service calls are used by the software.

{single,multi}checker do not guarantee each package is analyzed in the same process. This allows reducing to unitchecker to be picked up by go vet -vettool=.... Keep this in mind as locking reads/writes to a shared file needs to be done at a different level, the filesystem (or an external database) may suffice. You can also shard writes to different non-overlapping locations for each pass.

(my guess is it doesn't like adding a non-go file to the Fset after the fact).

This is not supported. The assumption is that the packages.Packages can be loaded all at once before any Passes are run.

I did try a couple of approaches using the supplied facts and diagnostics APIs:
firstly, using a custom fact struct containing that map, and then using multichecker to run both analyzers, however there wasn't a clean way to "analyze" the yaml code since it's expecting the same package to be used for all analyzers.

If you are communicating with an external file/DB that will need to be aware of different Passes, an (Analyzer, Package)-pairs. Potentially different executions of the same (Analyzer, Package).

where the string keys are the method call names

2cents: This may have type incorrect call relationships. Not sure if this impacts your application

[gopherbot]

Change https://go.dev/cl/411907 mentions this issue: go/analysis/checker: a go/packages-based driver library

[adammw]

Probably a bit late but I got permission to share concrete code of how we're using the analysis library here: https://gist.github.com/adammw/941d15c8b1730e3e89fc61138a6a4f24

Note that I had to essentially copy out the implementation of the internal load() method and create/run an analysis.Pass myself. The API sketch above seems to solve some of the problem of creating and running analysis passes in dependency order as needed, however package loading is still left to the user implementation (which might be fine, users like us want the ability to exclude tests for example).

[timothy-king]

Thank you for the example.

To summarize your use case a bit:

1. You have a dynamic sequence of top level packages combined with an Analyzer. Future package P is computed as a side effect of a result from another package Q (p=Foo(Q) where p is the pattern for P). A framework would not be able to predict this.
2. Diagnostics and Results for Passes are used.
3. Facts and Requires are not used.

The first bullet is going to be hard for a framework to schedule. We can debate whether there is some way of shoving this into the expectations of analysis/{single,mulit,unit}checker, but I am not sure that would amount to much.

We will take this case into account when considering #53215 .

[josebalius]

Perhaps this needs a new issue altogether, but this is the closest thing I could find to what I am running into.

We have a linter that we use today but cannot integrate with the existing analysis tools (we currently use golang.org/x/tools/go/packages and process the AST directly).

We essentially have a linter that ensures specific structs are being initialized with all of their fields. Currently, certain community linters do this by taking in a "pattern" of struct name to match against, we've found this to be cumbersome and inefficient, and instead, our linter allows us to add a comment to the structs we wish this linting rule enforced for. Then for any literal we find, we ensure the rule is applied.

Currently, the limitation of only processing one pkg at a time and comments not being attached to types (or anything we can grab in a pass) keeps us from being able to adapt this otherwise. Is there an alternative approach here?

[timothy-king]

@josebalius Please open a new issue. It is a different discussion.

[adonovan]

@addmmw It seems to me that if your goal is to compute some property of a package and all its transitive dependencies, for the purpose of code generation, you would be better off starting from the go/packages.Load function, which returns a graph of packages that you can analyze using custom logic.

The analysis framework is oriented towards checkers, i.e. functions that produce diagnostics, which is not your desired output, and towards incremental analysis of large codebases, analogous to separate compilation in a compiler. But if your goal is code generation---something done relatively infrequently in the workflow---this optimization doesn't seem very important.

No doubt it is possible to express your problem in the analysis framework (perhaps by encoding the result information into a diagnostics message and then parsing it) but it's forcing a round peg into a square hole. There have been requests for us to provide a more piecemeal set of components for building analysis tools, instead of the inflexible unitchecker and singlechecker packages which provide the complete main function for the application (everything but the checker plugins). Such an API might reduce the needed amount of force. But still it seems to me that what you're doing is most naturally addressed by starting from golang.org/x/tools.go/packages.

[adonovan]

We essentially have a linter that ensures specific structs are being initialized with all of their fields. Currently, certain community linters do this by taking in a "pattern" of struct name to match against, we've found this to be cumbersome and inefficient, and instead, our linter allows us to add a comment to the structs we wish this linting rule enforced for. Then for any literal we find, we ensure the rule is applied.

Nice to hear from you @josebalius, I hope you are well.

I think your problem should be very cleanly solvable using Facts in the go/analysis framework. Basically, your analyzer needs to do two things:
(1) figure out which named struct types declared in this package are "special", and for each one, call ExportObjectFact on the TypeName. You can do this by looking for a TypeSpec enclosing a StructType that has a special comment. And:
(2) each time you see a struct CompositeLit, test to see whether its type is a special type, and if so, apply the check.

A type is special if it was discovered by step (1) in this package, or if it was discovered when step (1) was previously applied to one of this package's dependencies, so it's a memoized call to ImportObjectFact. The printf checker should serve as a guide.

In future we may come up with a more standard way to annotate declarations such as your struct type, but for now, looking for a special doc comment seems fine. Best of luck.

[adonovan]

I'm going to close this issue because I think the analysis framework is not appropriate; use go/packages instead as described in #50265 (comment).

[josebalius]

@adonovan hope you are doing well also! Yep that's exactly what I ended up doing after figuring it out by looking at some other linters' implementation.