proposal: cmd/go,cmd/vet: declarative external vettools

[seankhliao]

Proposal Details

api changes

go.mod

A new vettol type would be added, acting like the existing tool definitions,
but they must be vet tools invocable by the current go vet -vettool prog.
That is, they must be built on top of golang.org/x/tools/go/analysis/unitchecker.

// go.mod

vettool (
        example.com/my/analysis/one
        example.com/my/analysis/two
        github.com/other/analysis
)

go vet

Running go vet would run the standard vet checks in cmd/vet,
in addition to any vet tools defined in go.mod as described above.

The -vettool flag currently only accepts absolute file paths.
It would change to work like go tool, resolving:

• all to mean all declared vettools
• vet to cmd/vet,
• single element names and import paths to definitions in vettool
• absolute paths work as it currently does

go test

Currently, by default, go test invokes a subset of vet checks.
This proposal would not change the default subset of checks run during a test.

go test -vet=all would change include these externally defined checks.

A new go test -vet=vet would represent the standard vet checks (equivalent to the current -vet=all)

why

The bar for the Go project to add a check in to cmd/vet is quite high,
we require correctness, frequency, and precision across pretty much all of written Go code.

Individual projects are much more tightly scoped,
they can afford to be stricter, and more opinionated in the style and checks enforced.
And they frequently are, as seen by the proliferation of custom analyzers
and tools to run them in both OSS and enterprise.

All this means there's a fragmented ecosystem
with no standard way of running all the checks a project will require of its contributors.
For these projects, the additional analyzers they want to run will often be required and enforced in CI.
I think it would be great to eliminate the mess of having to install and run
potentially multiple external analysis tools and have them all under the umbrella of go vet,
since for the end user, there isn't much functional difference in which checks must pass.

I previously filed #71478, but running the analysis as tests seems wrong,
as the scope for tests is a package,
while analysis should be on the entire module.

security

The defaults I chose above mean go vet would now execute in control of the module being vetted (through the analyzers it declares) where it previously didn't.

To me, this is an ok tradeoff in terms of security vs convenience, as vet is commonly followed by test which by nature executes code.

If we wish to maintain the original security boundary, then vettool would default to -vettool=vet and the new behavior would be opt in with -vettool=all.

[gabyhelp]

Related Issues

• proposal: x/tools/go/analysis/vet: export cmd/vet's []*Analyzer suite #35487
• proposal: cmd/vet: expose list of analyzers #75227
• proposal: x/tools/go/analysis/testchecker: analysis driver for tests #71478 (closed)
• cmd/go: go vet should take vet's flags #19350 (closed)
• cmd/vet/all: get golang.org/x/tools/go/analysis/cmd/vet via a go.mod file, not via coordinator #31040 (closed)
• vet: Enable multiple linters with -vettool #70041 (closed)
• cmd/go: run vet automatically during 'go test' #18084 (closed)
• cmd/go: fix: apply fixes from modernizers, inline, and other analyzers #71859 (closed)
• proposal: cmd/go: add go doctor command for unified health aggregator #76719 (closed)
• proposal: cmd/go: fix: include selected staticcheck analyzers #76918

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

[prattmic]

Just to clarify, assuming this is enabled by default, do I understand correctly that go vet all would also run all vet tools declared in dependencies?

[seankhliao]

No, only vet tools in the main module are used, just like go tool only taking tools from the main module.

go vet all means run go vet, selecting all the vet tools in the main module (-vettool=all), run against the all package pattern (main module and dependencies).

[adonovan]

I'm sure there are details to work out or at least articulate, but this is an interesting idea. The security story seems workable since go vet has a more similar role in the workflow to go test (which trusts the test code in the main module and all its dependencies) than does, say, gopls, which is used to read code that you definitely do not trust, hence the need for more elaborate security precautions in #59869. But if we add a vettool directive, the very next feature request will be "make it just work in gopls too", so perhaps the two ideas are not so easily separated.

[seankhliao]

Perhaps we could copy what GOVCS did in #41730 to do module pattern matching for allowed analyzers.

GOVETTOOL=module-prefix1:allowed|tools,module-prefix2:vet,...

[crhntr]

To permit arguments to be passed to the tool (and to reduce the syntax change if that matters), it might be nice if, like // indirect, the interface was a comment at the end of the line: fields after the vet comment prefix are passed to the tool as arguments. This would also allow exiting tools to add flags to output the analysis result in a standard format so the vet tool can handle pretty printing.

module example.com

require (
    github.com/typelate/muxt v0.18.3 // indirect
)

tool (
    github.com/typelate/muxt/cmd/muxt // vet -C ./internal/hypertext check --output=vet --use-templates-variable=myTemplates
)

[seankhliao]

I was thinking about the difference in performance of cmd/vet vs directly invoking analyzers through one of the x/tools/go/analysis drivers.

on my own codebase, go vet all takes:

• empty cache: go vet all 2175.19s user 137.80s system 1519% cpu 2:32.23 total
  • running go build before does not seem to impact run time
• subsequent run: go vet all 5.62s user 1.10s system 390% cpu 1.721 total

running the same set of analyzers via golang.org/x/tools/go/analysis/checker in a test takes:

• empty cache: go test -vet=off -run TestAnalysisVet 209.68s user 19.86s system 1156% cpu 19.846 total
• subsequent run: go test -vet=off -run TestAnalysisVet 189.85s user 18.99s system 1214% cpu 17.200 total

cmd/vet uses golang.org/x/tools/go/analysis/unitchecker, and it invokes cmd/compile.
checker doesn't invoke cmd/compile, but it's consistent in run time (I think I matched most of the config?)

test code

var vetAnalyzers = []*analysis.Analyzer{
	appends.Analyzer,
	asmdecl.Analyzer,
	assign.Analyzer,
	atomic.Analyzer,
	bools.Analyzer,
	buildtag.Analyzer,
	cgocall.Analyzer,
	composite.Analyzer,
	copylock.Analyzer,
	defers.Analyzer,
	directive.Analyzer,
	errorsas.Analyzer,
	framepointer.Analyzer,
	hostport.Analyzer,
	httpresponse.Analyzer,
	ifaceassert.Analyzer,
	loopclosure.Analyzer,
	lostcancel.Analyzer,
	nilfunc.Analyzer,
	printf.Analyzer,
	shift.Analyzer,
	sigchanyzer.Analyzer,
	slog.Analyzer,
	stdmethods.Analyzer,
	stdversion.Analyzer,
	stringintconv.Analyzer,
	structtag.Analyzer,
	testinggoroutine.Analyzer,
	tests.Analyzer,
	timeformat.Analyzer,
	unmarshal.Analyzer,
	unreachable.Analyzer,
	unsafeptr.Analyzer,
	unusedresult.Analyzer,
	waitgroup.Analyzer,
}

func TestAnalysisVet(t *testing.T) {
	// bi, ok := debug.ReadBuildInfo()
	// if !ok {
	// 	t.Fatal("failed to read build info")
	// }
	// module := bi.Main.Path + "/..."
	module := "all"

	pkgs, err := packages.Load(&packages.Config{
		Mode:    packages.LoadAllSyntax,
		Tests:   true,
		Context: t.Context(),
	}, module)
	if err != nil {
		t.Fatalf("load packages %v: %v", module, err)
	}

	analyzers := vetAnalyzers
	g, err := checker.Analyze(analyzers,
		pkgs,
		&checker.Options{})
	if err != nil {
		t.Fatalf("run analyzers: %v", err)
	}

	for act := range g.All() {
		// if !strings.HasPrefix(act.Package.PkgPath, bi.Main.Path) {
		// 	continue
		// }
		name := fmt.Sprintf("%s/%s", act.Analyzer, act.Package)

		generated := make(map[string]bool)

		t.Run(name, func(t *testing.T) {
			if act.Err != nil {
				t.Error(act.Err)
			}

			for _, diag := range act.Diagnostics {
				pos := act.Package.Fset.Position(diag.Pos)

				if _, ok := generated[pos.Filename]; !ok {
					b, err := os.ReadFile(pos.Filename)
					if err != nil {
						t.Errorf("read file %v: %v", pos.Filename, err)
						continue
					}
					generated[pos.Filename] = generatedRe.Match(b)
				}
				if generated[pos.Filename] {
					continue
				}

				t.Errorf("[%v] %s", pos, diag.Message)
			}
		})
	}
}

[adonovan]

I was thinking about the difference in performance of cmd/vet vs directly invoking analyzers through one of the x/tools/go/analysis drivers.

Terminological nitpick: cmd/vet is actually just a set of analyzers plus x/tools/go/analysis/unitchecker, i.e. one of those very drivers. The difference is that go vet runs cmd/vet like a compiler, doing one unit (package) of work, reading and writing files; whereas the {single,multi}checker tools load the whole program into memory. The latter has worse asymptotic scaling (to process a big monorepo demands a big machine) but avoids the serialization overhead, which I suspect is what you are seeing below.

on my own codebase, go vet all takes:

• empty cache: go vet all 2175.19s user 137.80s system 1519% cpu 2:32.23 total

Wow, that's a lot! (But great parallel utilization!) It would be good to get a break down of where this time is being spent. (My guess is repeated export data parsing, and possibly also fact decoding/encoding.)

Would you be able to make a CPU profile of one of the slower (=later, probably) cmd/vet runs? Thanks.

cmd/vet uses golang.org/x/tools/go/analysis/unitchecker, and it invokes cmd/compile. checker doesn't invoke cmd/compile, but it's consistent in run time (I think I matched most of the config?)

Are you sure about that? I don't think cmd/vet (or even go vet) should invoke cmd/compile. I think cmd/vet (unitchecker) acts as the compiler frontend (doing parsing, importing, type checking, and exporting), for all dependencies.

Update: Never mind, I was confusing facts, for which cmd/vet of course does all export/import, with types, for which cmd/compile is used to generate export data. We should change cmd/vet to export types and facts and skip the compiler entirely. I've filed #79454.