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

I like the proposal and have two questions about this.

1. Do you have an opinion how error handling works in with this package, especially in the light of panics?
   Imagine you are running multiple analyzers on multiple packages . If one of these analyzers panics during the analysis does the Analyze recover the panic?
   If not, it would prevent you from getting any result for any of the analyzers on any package.

2. Is is possible to make the facts part of the output/input?
   This would allow users to shard the execution of many packages into batches and distribute them between workers (potentially in different processes on different machines).

1. Do you have an opinion how error handling works in with this package, especially in the light of panics?
   Imagine you are running multiple analyzers on multiple packages . If one of these analyzers panics during the analysis does the Analyze recover the panic?

The go/analysis/checker driver uses defer + recover around Analyzer.Run, so if the analyzer is single-threaded (as ~all are), it will catch the panic and treat it as a failure of that Action. Like compilation, all downstream actions will not be executed, but independent actions will continue as usual. All this information can be gleaned from the action graph returned by checker.Analyze.

2. Is is possible to make the facts part of the output/input?

The Fact data structures are part of the output: see the All{Object,Package}Facts and {Object,Package}Fact methods. (Their encoding isn't, but the point of internal/checker is that it loads, parses, type-checks and analyzes everything from source, so the encoding of types and facts don't come into play.)

This would allow users to shard the execution of many packages into batches and distribute them between workers (potentially in different processes on different machines).

If you want to distribute units of analysis, you should be using unitchecker, or something like it (it's not a lot of code), as it assumes that intermediate results between units are serialized. Perhaps I should add an Example in go/analysis (but not checker) that shows how to construct a system analogous to go vet + unitchecker: in other words, the parent task constructs the graph of units of work and the child task(s) do the actual work, using serialized intermediaries for types and facts.

I see. I think I missed this part:

For each analyzer that uses facts, Analyze also runs it on all the dependencies of the initial packages.

Thanks for clarifying (I should learn to read comment 😄 )

I think checker is a fine name.

Since this is a non-main package, I don't think it should log anything. Consider instead providing that functionality as callbacks. For instance, replace Verbose with OnStep func(s string) (or maybe something richer than a string).

Since this is a non-main package, I don't think it should log anything.

Good point. On closer inspection, it only logs one message, a constant, at the very beginning. We can simply get rid of Verbose. Thanks for pointing this out.

@adonovan Hello. Thank u for proposal!

My question is still the same – how I can reuse helpful flags from single/multichecker and append to them my custom flags?

how I can reuse helpful flags from single/multichecker and append to them my custom flags?

This proposal doesn't include a "convenience" API for flags, but it does allow you to control all the relevant pieces of the driver and the analyzers from the outside:

• The various verbosity flags (-debug) can be be set through fields of checker.Options;
• Profiling (e.g. -cpuprofile) can be enabled by making the usual calls to the runtime;
• Tests (-test) can be loaded by setting packages.Config.Tests;
• Fixes (-fix) can be applied by calling checker.ApplyFixes.
• Flags for each analyzer can be registered using some variation on :

	for _, a := range analyzers {
		a.Flags.VisitAll(func(f *flag.Flag) {
			flag.Var(f.Value, a.Name + "." + f.Name, f.Usage) // --analyzer.flag=...
		})
	}

	flag.Parse()

We can always add more convenience functions later as clear needs arise, but the primary goal here is to make the tricky logic of the driver available in a form other than a complete application Main function.

Thank u, I see.

checker.ApplyFixes and checker.Print* is a great steps to solving the problem I'm talking about.

At the moment we are hostages of multi/singlechecker.Main.

My linter help example:

Flags:
  -V    print version and exit
  -all
        no effect (deprecated)
  -c int
        display offending line with this many lines of context (default -1)
  -checked-types value
        coma separated list (default chan,func,iface,map,ptr)
  -cpuprofile string
        write CPU profile to this file
  -debug string
        debug flags, any subset of "fpstv"
  -fix
        apply all suggested fixes
  -flags
        print analyzer flags in JSON
  -json
        emit JSON output
  -memprofile string
        write memory profile to this file
  -source
        no effect (deprecated)
  -tags string
        no effect (deprecated)
  -test
        indicates whether test files should be analyzed, too (default true)
  -trace string
        write trace log to this file
  -v    no effect (deprecated)

Issues:

1. You have no possibility to tune flag set. E.g. if linter works only with tests then -test doesn't make sence.

2. You have no possibility remove a lot of deprecated flags needed only for govet backward compatibility.

3. It's not obvious what flags are "service" (e.g. -trace, -cpuprofile) and what are related to linter functionality (e.g. -checked-types).

4. If you want to make custom flag set and don't use singlechecker.Main but you are interested in useful flags, you should write (or copy-paste) on yourself own:

• profiling code
• tracing code
• fixing code
• printing code (version, flags, diagnostic – plain and json)

Thanks @Antonboom, I think you are saying that the proposal addresses the four issues you listed and that you are in favor of accepting it. I hope we can get it approved soon. (Please do correct me if I have misunderstood or if you think we should change the proposed API in some way.)

Please do correct me if I have misunderstood

No, no. I'm waiting this change a lot.

I attached this example just with the hope that maybe it will push you to new thoughts.

For example:

1. Existing checker.Run contains some logic around package loading. But in this proposal it is a headache of package user?
2. Profiling/tracing code should be copypasted from checker.Run? Maybe it's ok...
3. Similar for print flags and version functionality.
4. etc?

I attached this example just with the hope that maybe it will push you to new thoughts.

Great, thanks. Yes, in all three cases the new API makes these problems the client responsibility:

For example:

1. Existing checker.Run contains some logic around package loading. But in this proposal it is a headache of package user?

The amount of code in the load function is very small, and client applications are likely going to want fine-grained control over it, so I'd rather not introduce a helper function just yet. (The proposed API does state its requirements for the Mode flag, but that's all it needs to say.)

2. Profiling/tracing code should be copypasted from checker.Run? Maybe it's ok...

This logic is unrelated to analysis; it's there because singlechecker.Main is a main function, and any significant application has logic like this in its main function.

3. Similar for print flags and version functionality.

We may want to add some helper functions for flags, but it's surprisingly fiddly to come up with a good API for it, so I thought I would leave it out for now and see what needs arise. The important thing is that clients are unstuck, even if they have to copy a little code.

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

I am in favor of this proposal. However, I think we should consider having this package operate on an fs.FS, rather than the os files system itself. It seems possible that certain applications (e.g. gopls, or a tool like it) might want to operate on a file state other than the file system. Do you think this would be a reasonable use of this new package?

I suppose we can add FS support later as an Options.FS field, but wanted to pose the question. For example, the ApplyFixes helper is necessarily tightly coupled to the OS. Perhaps we should elide that helper for now, or give it a more extensible API?

In general, I'm not sure about adding all the helpers in this package. They seem more related to a particular command-line interface than they do to the code of the analysis algorithm. But I don't feel strongly.

we should consider having this package operate on an fs.FS, rather than the os files system itself.
I suppose we can add FS support later as an Options.FS field

I assume you are primarily referring to ApplyFixes. It does seem reasonable to parameterize it over the file system in some way. An FS would be suitable for reading, but we still need a means to write the results. (There's no writable equivalent of FS, and proposal #45757 was declined.) I'll have a think.

As for Analyze, the client is expected to have already obtained the packages.Packages, so it's too late to affect how files are read. That said, some analyzers do call os.OpenFile and I recently filed a proposal #62292 to add an OpenFile operation to analysis.Pass, in which case this interface would want an FS field so that reads done within the analyzer are consistent with whatever overlays were passed to go/packages.

In general, I'm not sure about adding all the helpers in this package. They seem more related to a particular command-line interface than they do to the code of the analysis algorithm.

The three Print functions aren't fundamental. They could be left for a later proposal, but I've included them to make it easy to build something like unitchecker without having to copy tons of code. The PrintDiagnostics function could also be usefully parameterized over an FS since it prints lines of context which it must read from somewhere.

As for Analyze, the client is expected to have already obtained the packages.Packages, so it's too late to affect how files are read.

FWIW go/packages accepts overlays -- that's how gopls uses it.

go/packages accepts overlays -- that's how gopls uses it.

Understood, but my point is that the client does the packages.Load call before the call to Analyze, so Analyze has no need for the file system (at least until #62292 is accepted).

Understood, but my point is that the client does the packages.Load call before the call to Analyze, so Analyze has no need for the file system (at least until #62292 is accepted).

Ack, sorry I misinterpreted your statement as saying that go/packages would have necessarily read from the OS. In that case, yes it does appear that this is only relevant to ApplyFixes. Should we accept a ReadFile/WriteFile interface?

Should we accept a ReadFile/WriteFile interface?

Yes, those two would be almost sufficient, but the implementation of ApplyFixes currently uses the robustio.FileID type (as a map key) to make it safe in the presence of file-system level aliasing: symbolic links, hard links, case insensitivity, path uncleanness. The algorithm could be expressed in terms of os.SameFile but only at the expense of a quadratic number of calls. So in addition to {Read,Write}File we'd need a FileID(filename) (any, error) method that returns an opaque comparable value whose equivalence relation corresponds to "same inode". Seems like a can of worms.

The quadratic term can be reduced to a trivial coefficient by using the content as the primary distinguishing key, calling SameFile only when the contents match, which should be rare for Go source files. The FS interfaces (even StatFS) don't seem to have an abstraction of os.SameFile, so we would need to users to provide it. Or we could say that, if users provide a custom FS, then file-system level aliasing becomes their problem.

After discussion with Russ, I have removed a number of functions and simplified some of the names and signatures. See revised first note in this issue, or updated CL.

Understood, but my point is that the client does the packages.Load call before the call to Analyze, so Analyze has no need for the file system (at least until #62292 is accepted).

Ack, sorry I misinterpreted your statement as saying that go/packages would have necessarily read from the OS. In that case, yes it does appear that this is only relevant to ApplyFixes. Should we accept a ReadFile/WriteFile interface?

ApplyFixes has been dropped from the proposal for now, in part because of the confusion of abstraction levels: the input is provided as a data structure produced from files read by the client calling packages.Load, but the ApplyFixes operation interacts with the file system directly. We should probably consider all these overlay and FS-related issues holistically in #62292.

Dropping the comments to make the API easier to skim, I think we are down to:

package checker // golang.org/x/tools/go/analysis/checker

func Analyze(analyzers []*analysis.Analyzer, pkgs []*packages.Package, opts *Options) (*Graph, error)

type Options struct {
	Sequential  bool // -p: disable parallelism
	SanityCheck bool // -s: perform additional sanity checks
	ShowFacts   bool // -f: log each exported fact
}

type Graph struct {
	Roots []*Action
}

func (*Graph) Visit(f func(*Action) error) error) 
func (*Graph) JSONDiagnostics(w.io.Writer) 
func (*Graph) TextDiagnostics(w.io.Writer, contextLines int) 

type Action struct {
	Analyzer    *analysis.Analyzer
	Package         *packages.Package // NOTE: was Pkg in earlier draft.
	IsRoot      bool // whether this is a root node of the graph
	Deps        []*Action
	Result      interface{} // computed result of Analyzer.run, if any (and if IsRoot)
	Err         error       // error result of Analyzer.run
	Diagnostics []analysis.Diagnostic
	Duration    time.Duration // execution time of this step
}

func (a *Action) AllObjectFacts() []analysis.ObjectFact
func (a *Action) AllPackageFacts() []analysis.PackageFact
func (a *Action) ObjectFact(obj types.Object, ptr analysis.Fact) bool
func (a *Action) PackageFact(pkg *types.Package, ptr analysis.Fact) bool

The JSONDiagnostics and TextDiagnostics names seem wrong to me. I had suggested PrintJSON and PrintText, which make the action clearer and sort next to each other.

For Options, I am still not sure about mentioning the flags. When we spoke last week I thought I understood you to say that unitchecker or multichecker define those flags, but I can't find any evidence of that in the x/tools repo:

% git grep -n 'flag.*"[psf]"'
cmd/godex/godex.go:20:	source  = flag.String("s", "", "only consider packages from src, where src is one of the supported compilers")
cmd/goyacc/yacc.go:167:	flag.StringVar(&prefix, "p", "yy", "name prefix to use in generated code")
cmd/stress/stress.go:35:	flagP       = flag.Int("p", runtime.NumCPU(), "run `N` processes in parallel")
% git grep -n 'Bool.*"[psf]"'
% 

The JSONDiagnostics and TextDiagnostics names seem wrong to me. I had suggested PrintJSON and PrintText, which make the action clearer and sort next to each other.

I can change the names easily enough. The fact that they don't call fmt.Print, but write to a writer, made me think Print wasn't quite the right verb, but I don't feel strongly.

More importantly, they write to an io.Writer but don't return an error. Should I make them return an error, or return their result as a []byte? I am inclined toward the former.

For Options, I am still not sure about mentioning the flags. When we spoke last week I thought I understood you to say that unitchecker or multichecker define those flags, but I can't find any evidence of that in the x/tools repo:

I misspoke, and misremembered when I wrote the doc comments "-p: ..." etc above: they are not regular flags, but single-letter arguments to a single -debug=[fpstv]* flag.

Returning an error is fine. Better not to assume one giant []byte.

@dominikh's feedback may be useful here.

Here is what the standard library has to say about writing to an io.Writer. I've removed names like Encode and Execute that are clearly wrong.

Write is the most common prefix, though I think To also works.

bufio.Reader.WriteTo(w io.Writer) (n int64, err error)
bytes.Buffer.WriteTo(w io.Writer) (n int64, err error)
bytes.Reader.WriteTo(w io.Writer) (n int64, err error)
strings.Reader.WriteTo(w io.Writer) (n int64, err error)
index/suffixarray.Index.Write(w io.Writer) error
go/types.Scope.WriteTo(w io.Writer, n int, recurse bool)
net.Buffers.WriteTo(w io.Writer) (n int64, err error)
net/http.Request.Write(w io.Writer) error
net/http.Response.Write(w io.Writer) error
net/http.Header.Write(w io.Writer) error
runtime/pprof.Profile.WriteTo(w io.Writer, debug int) error
go/scanner.PrintError(w io.Writer, err error)
go/doc.ToHTML(w io.Writer, text string, words map[string]string)
go/doc.ToText(w io.Writer, text string, prefix, codePrefix string, width int)
encoding/binary.Write(w io.Writer, order ByteOrder, data any) error
runtime/coverage.WriteMeta(w io.Writer) error
runtime/coverage.WriteCounters(w io.Writer) error
runtime/pprof.WriteHeapProfile(w io.Writer) error

scanner.PrintError seems like sufficient justification for PrintJSON and PrintText.

Ok, Print it is.

package checker // golang.org/x/tools/go/analysis/checker

func Analyze(analyzers []*analysis.Analyzer, pkgs []*packages.Package, opts *Options) (*Graph, error)

type Options struct {
	Sequential  bool // disable parallelism
	SanityCheck bool // perform additional sanity checks
	ShowFacts   bool // log each exported fact
}

type Graph struct {
	Roots []*Action
}

func (*Graph) Visit(f func(*Action) error) error) 
func (*Graph) PrintJSON(w.io.Writer) error
func (*Graph) PrintText(w.io.Writer, contextLines int)  error

type Action struct {
	Analyzer    *analysis.Analyzer
	Package         *packages.Package // NOTE: was Pkg in earlier draft.
	IsRoot      bool // whether this is a root node of the graph
	Deps        []*Action
	Result      interface{} // computed result of Analyzer.run, if any (and if IsRoot)
	Err         error       // error result of Analyzer.run
	Diagnostics []analysis.Diagnostic
	Duration    time.Duration // execution time of this step
}

func (a *Action) AllObjectFacts() []analysis.ObjectFact
func (a *Action) AllPackageFacts() []analysis.PackageFact
func (a *Action) ObjectFact(obj types.Object, ptr analysis.Fact) bool
func (a *Action) PackageFact(pkg *types.Package, ptr analysis.Fact) bool

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

package checker // golang.org/x/tools/go/analysis/checker

func Analyze(analyzers []*analysis.Analyzer, pkgs []*packages.Package, opts *Options) (*Graph, error)

type Options struct {
	Sequential  bool // disable parallelism
	SanityCheck bool // perform additional sanity checks
	ShowFacts   bool // log each exported fact
}

type Graph struct {
	Roots []*Action
}

func (*Graph) Visit(f func(*Action) error) error) 
func (*Graph) PrintJSON(w.io.Writer) error
func (*Graph) PrintText(w.io.Writer, contextLines int)  error

type Action struct {
	Analyzer    *analysis.Analyzer
	Package     *packages.Package
	IsRoot      bool // whether this is a root node of the graph
	Deps        []*Action
	Result      interface{} // computed result of Analyzer.run, if any (and if IsRoot)
	Err         error       // error result of Analyzer.run
	Diagnostics []analysis.Diagnostic
	Duration    time.Duration // execution time of this step
}

func (a *Action) AllObjectFacts() []analysis.ObjectFact
func (a *Action) AllPackageFacts() []analysis.PackageFact
func (a *Action) ObjectFact(obj types.Object, ptr analysis.Fact) bool
func (a *Action) PackageFact(pkg *types.Package, ptr analysis.Fact) bool

I'm slightly concerned about the proposed Analyze function. It requires having loaded all packages ahead of time, including their type information and ASTs. If checking a large amount of packages, this amounts to a lot of flat memory usage.

In Staticcheck's analysis runner, we instead construct the package graph from packages loaded with

packages.NeedName |
	packages.NeedImports |
	packages.NeedDeps |
	packages.NeedExportFile |
	packages.NeedFiles |
	packages.NeedCompiledGoFiles |
	packages.NeedTypesSizes |
	packages.NeedModule

and then compute ASTs and type information on demand when a package is ready to be analyzed, i.e. all of its dependencies have been analyzed. Once we're done, we discard the AST and type information. This results in overall much lower memory usage.

One downside of our approach is that we load type information for dependencies from export data repeatedly, leading to more garbage and more CPU time spent on parsing export data and GC.

(We do the same discarding and reloading for facts, but that part probably contributes much less to overall memory usage.)

Also, are there any plans to add callbacks to support caching? That is, avoid recomputing facts and diagnostics for packages that haven't changed.

Hi @dominikh.

I'm slightly concerned about the proposed Analyze function. It requires having loaded all packages ahead of time, including their type information and ASTs. If checking a large amount of packages, this amounts to a lot of flat memory usage.

You're right that it isn't the most asymptotically efficient solution. It would be more efficient to have Analyze produce (and then evict) type-annotated syntax as it goes, and for {single,multi}checker to take advantage of this optimization too. I suppose nothing stops us from retrofitting this optimization into the API proposed here: if syntax and/or types are missing, then Analyze could parse and type-check as it goes, using a parallel structure to avoid mutating the go/packages graph.

But the motivation for this new API is for users that want to load and analyze a large program, re-using the tricky bits of {single,multi}checker.Main while inserting their own logic in between package.Load and Analyze and between Analyze and Exit, which the existing API makes impossible. So I expect that for them, the cost of holding everything in memory at once is quite acceptable.

As always, asymptotically efficient separate analysis with persistent memoization of subproblems is best done using the unitchecker approach (go vet -vettool=myexecutable), which works like a build system. This makes the cost of repeat analysis proportional to what has changed, though, as you point out, in the "clean build" scenario it does mean that export data is decoded repeatedly, just as in a clean go build. But the unitchecker approach is no good for tools that want to see the whole program before analysis, or all the results after analysis.

Result interface{} // computed result of Analyzer.run, if any (and if IsRoot)

Are we sure we want to keep the "and if IsRoot" part? Keeping it does encourage compatibility with unitchecker and facts, but it will make the Actions distinct from how the execution happened.

If we have analyzers A and B where B uses the results of A and a package P and we request the roots [B]x[P], we will have the Actions [(A, P), (B, P)]. The current proposal requires that (A,P).Result == nil despite this potentially not being nil during the computation of (B,P).

Why is Result special in this way and not say Diagnostics?

If we have analyzers A and B where B uses the results of A and a package P and we request the roots [B]x[P], we will have the Actions [(A, P), (B, P)]. The current proposal requires that (A,P).Result == nil despite this potentially not being nil during the computation of (B,P).

Correct.

Why is Result special in this way and not say Diagnostics?

Because Results are potentially very large data structures, such as the SSA representation of all the function bodies, and we want to evict them from memory. By contrast, Diagnostics are typically small and few in number.

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

package checker // golang.org/x/tools/go/analysis/checker

func Analyze(analyzers []*analysis.Analyzer, pkgs []*packages.Package, opts *Options) (*Graph, error)

type Options struct {
	Sequential  bool // disable parallelism
	SanityCheck bool // perform additional sanity checks
	ShowFacts   bool // log each exported fact
}

type Graph struct {
	Roots []*Action
}

func (*Graph) Visit(f func(*Action) error) error) 
func (*Graph) PrintJSON(w.io.Writer) error
func (*Graph) PrintText(w.io.Writer, contextLines int)  error

type Action struct {
	Analyzer    *analysis.Analyzer
	Package     *packages.Package
	IsRoot      bool // whether this is a root node of the graph
	Deps        []*Action
	Result      interface{} // computed result of Analyzer.run, if any (and if IsRoot)
	Err         error       // error result of Analyzer.run
	Diagnostics []analysis.Diagnostic
	Duration    time.Duration // execution time of this step
}

func (a *Action) AllObjectFacts() []analysis.ObjectFact
func (a *Action) AllPackageFacts() []analysis.PackageFact
func (a *Action) ObjectFact(obj types.Object, ptr analysis.Fact) bool
func (a *Action) PackageFact(pkg *types.Package, ptr analysis.Fact) bool