Is it fair to describe the proposal as basically make go test -vet=all ... effectively a synonym for go vet ...; go test ... (or something similar)?

If there was also a large uptick in folks[/tools] replacing go test with go test -vet=all, then adding a new checker would break everyone's tests. Having a division between tests must pass and vet must be clean is a reasonable distinction for most CI systems. Adding this flag would at the margin encourage folks to combine these two together. OTOH all of my CI systems do "go vet ...; go test ..." anyways so why not be more convenient?

The latter is a performance issue with large [recte monorepo] builds where a lot of source is being parsed.

Why would this be an issue with go vet ...; go test ... and not with go test -vet=all ...? I could imagine these having a different wallclock time via improved parallelism, but I do not know why the user time would differ much. I would assume the amount of parsing is the same for example.

enabling vets that produce true positives.

The true positive rate of the existing checkers is very very high. It is not 100% though.

The name all may be confusing if there are ever disabled vets added, like shadow, but as I read the docs, it seems like there was a move to purge ineffective vets.

A concern would be whether this will continue to be true moving forward. Adding a checker as initially disabled so folks can try it out and enabling it later is a lower barrier of entry.

Implementation note: printf is currently enabled in go test. So facts are enabled. So I do not expect to see a huge change in the performance caused by making go test -vet=all enable facts.

I agree that adding an "go vet -all" option in vet will be handy, but "go test -vet=all" is less convincing. One main issue is that the checkers not enabled by default such as the ShadowVariable checker may have high false positive rates, and if we combine "go test" and "go vet" then these false positives will fail the tests.
So "go test" uses only the high-confidence checkers:

"As part of building a test binary, go test runs go vet on the package and its test source files to identify significant problems. If go vet
finds any problems, go test reports those and does not run the test binary. Only a high-confidence subset of the default go vet checks are used."

The option "go vet -checker=all" looks better to me.

I agree that adding an "go vet -all" option in vet will be handy

to be clear, this would be a nop today? it would only do something different if we added a disabled by default vet like ShadowVariable.

One main issue is that the checkers not enabled by default such as the ShadowVariable checker may have high false positive rates, and if we combine "go test" and "go vet" then these false positives will fail the tests.

I feel comfortable saying some part of the community, myself included, wants such test failures as a feature, but I can understand doing this default may not be the best call.

The option "go vet -checker=all" looks better to me.

so this would also run tests? otherwise it is no different than go vet today, and does not meet the "one parse of the ast for vet and test" requirement from above.

We used to have a -all. go tool vet help says it is deprecated. Does someone want to look up the history?

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

Still pending someone looking up why the old -all became a no-op.

It looks to me that this was introduced in the change cmd/vet: switch to x/tools/go/analysis implementation. Relevant file.

Those flags look they might be mimicking the flags in cmd/vet/all/main.go. Just a guess.

cmd/vet/all was deleted in 86463c by rsc@.

Not sure when the -all flag changed behavior though.

We still need to figure out why -all was removed before.

alright, that took longer than I expected. it appears that this was added exclusively for backwards compatibility with old vet flags to unitchecker.Main via analysisflags.Parse in 8e5aba0. (cc @alandonovan / @adonovan):

Author: Alan Donovan <adonovan@google.com>
Date:   Fri Nov 16 13:36:49 2018 -0500

    go/analysis: harmonize flags across all checkers

    The -json and -c=N flags, formerly belonging only to the
    go/packages-based {single,multi}checkers, are now supported by
    unitchecker as well.

    The no-op -source, -v, -all, and -tags flags, formerly belonging only
    to unitchecker, have moved to the analysisflags package, which is
    common to all checkers.

    The -flags flag now reports all registered flags (except the
    {single,multi}checker-only debugging flags) rather than just those
    related to analyzers, allowing one to say: 'go vet -json' or 'go vet -c=1'.

    The code for printing diagnostics, either plain or in JSON, has been
    factored and moved into the common analysisflags package.

    This CL depends on https://go-review.googlesource.com/c/go/+/149960 to
    cmd/go, which causes 'go vet' to populate the ID field of the *.cfg.
    This field is used as a key in the JSON tree.

    Added basic tests of the new -json and -c unitchecker flags.

    Change-Id: Ia7a3a9adc86de067de060732d2c200c58be3945a
    Reviewed-on: https://go-review.googlesource.com/c/150038
    Reviewed-by: Michael Matloob <matloob@golang.org>

this can be manually confirmed:

package main

import "golang.org/x/tools/go/analysis/unitchecker"

func main() { unitchecker.Main() }

main is a tool for static analysis of Go programs.

main examines Go source code and reports suspicious constructs,
such as Printf calls whose arguments do not align with the format
string. It uses heuristics that do not guarantee all reports are
genuine problems, but it can find errors not caught by the compilers.

Registered analyzers:


By default all analyzers are run.
To select specific analyzers, use the -NAME flag for each one,
 or -NAME=false to run all analyzers not explicitly disabled.

Core flags:

  -V	print version and exit
  -all
    	no effect (deprecated)
  -c int
    	display offending line with this many lines of context (default -1)
  -flags
    	print analyzer flags in JSON
  -json
    	emit JSON output
  -source
    	no effect (deprecated)
  -tags string
    	no effect (deprecated)
  -v	no effect (deprecated)

To see details and flags of a specific analyzer, run 'main help name'.

OK, so it sounds like the history here was that we had go vet -all as the default, but that was turned off when you asked for anything specifically, like go vet -asmdecl. But then as part of the fix for #7422 (go vet -composite=false not turning off just the single check) we changed the way the flags are handled and made -all a no-op. Since "all" was the default, it didn't really matter that the flag was a no-op.

But then for "go test" we have been using a smaller test-safe subset. In the long term we want that to merge with "all" but it hasn't yet. Providing all would let people say 'go test -vet=all' to get the same checks as plain 'go vet'. That seems reasonable.

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

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

Change https://golang.org/cl/334873 mentions this issue: go/internal/test: add an all sentinel to -vet