Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
LeGamerDc opened this issue Feb 18, 2024 · 2 comments
Open

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

LeGamerDc opened this issue Feb 18, 2024 · 2 comments
Labels
Proposal
Milestone
Proposal

Comments

@LeGamerDc
Copy link

Proposal Details

checker always load packages with empty BuildFlags. however, analysis code in other build tags are needed
@gopherbot gopherbot added this to the Proposal milestone Feb 18, 2024
@ianlancetaylor
Copy link
Contributor

CC @alandonovan

@seankhliao seankhliao changed the title proposal: x/tools/go/analysis/internal/checker/check.go: check should accept custom build flags proposal: x/tools/go/analysis/internal/checker: check should accept custom build flags Feb 19, 2024
@adonovan
Copy link
Member

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.

@mneverov mneverov mentioned this issue Apr 9, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Proposal
Projects
Proposals
Status: Incoming
Milestone
Proposal
Development

No branches or pull requests
4 participants
@ianlancetaylor @adonovan @gopherbot @LeGamerDc