proposal: x/tools/go/analysis/internal/checker: check should accept custom build flags

[LeGamerDc]

Proposal Details

checker always load packages with empty BuildFlags. however, analysis code in other build tags are needed

[ianlancetaylor]

CC @alandonovan

[adonovan]

This feature is already supported by the unitchecker analysis driver, as shown by this example based on the printf checker built in to go vet (which is based on unitchecker):

xtools$ cat p/p.go
package p

$ cat p/p_foo.go
//go:build mytag
// +build mytag

package p

import "fmt"

func _() { fmt.Printf("%d", "") }

$ go vet ./p

$ go vet -tags=mytag ./p
p/p_foo.go:8:12: fmt.Printf format %d has arg "" of wrong type string

This works even with the -vettool flag, which specifies an arbitrary unitchecker-based driver. For example:

xtools$ go build -o nilness ./go/analysis/passes/nilness/cmd/nilness/
xtools$ go vet  -vettool=./nilness ./p
...shows nilness diagnostics in p_foo.go...

So you may be able to use that instead. (Bonus: it's more efficient, because it does an "incremental build", just like go build).

You are correct that the {single,multi}checker driver (whose common implementation is internal/checker) does not currently support build tags, even though it declares a non-functional flag named -tags for historical reasons to do with vet compatibility.

We could revive this flag to support the desired behavior with a small patch such as this:

diff --git a/go/analysis/internal/analysisflags/flags.go b/go/analysis/internal/analysisflags/flags.go
index ff14ff58f9..366d69db85 100644
--- a/go/analysis/internal/analysisflags/flags.go
+++ b/go/analysis/internal/analysisflags/flags.go
@@ -26,6 +26,7 @@ import (
 var (
        JSON    = false // -json
        Context = -1    // -c=N: if N>0, display offending line plus N lines of context
+       Tags    string
 )
 
 // Parse creates a flag for each of the analyzer's flags,
@@ -80,7 +81,7 @@ func Parse(analyzers []*analysis.Analyzer, multi bool) []*analysis.Analyzer {
        _ = flag.Bool("source", false, "no effect (deprecated)")
        _ = flag.Bool("v", false, "no effect (deprecated)")
        _ = flag.Bool("all", false, "no effect (deprecated)")
-       _ = flag.String("tags", "", "no effect (deprecated)")
+       flag.StringVar(&Tags, "tags", "", "comma-separated list of build tags")
        for old, new := range vetLegacyFlags {
                newFlag := flag.Lookup(new)
                if newFlag != nil && flag.Lookup(old) == nil {
diff --git a/go/analysis/internal/checker/checker.go b/go/analysis/internal/checker/checker.go
index 3c89350089..4b06a2a265 100644
--- a/go/analysis/internal/checker/checker.go
+++ b/go/analysis/internal/checker/checker.go
@@ -175,6 +175,8 @@ func load(patterns []string, allSyntax bool) ([]*packages.Package, error) {
        conf := packages.Config{
                Mode:  mode,
                Tests: IncludeTests,
+               // TODO: assumes no GOPACKAGESDRIVER
+               BuildFlags: []string{"-tags=" + analysisflags.Tags},
        }
        initial, err := packages.Load(&conf, patterns...)
        if err == nil {

But it would need some extra care to address alternative go/packages drivers such as Blaze, Bazel, Pants, Buck, and so on; the packages.Config.BuildFlags field doesn't feel like it was designed with portability in mind.