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

@matloob per owners

FWIW, the go vet subcommand currently scrapes the flags here:

Running go tool vet -flags locally, I see that it actually has a fair amount of structure in the Usage field.

[
	{
		"Name": "V",
		"Bool": true,
		"Usage": "print version and exit"
	},
	{
		"Name": "all",
		"Bool": true,
		"Usage": "no effect (deprecated)"
	},
	{
		"Name": "asmdecl",
		"Bool": true,
		"Usage": "enable asmdecl analysis"
	},
	{
		"Name": "assign",
		"Bool": true,
		"Usage": "enable assign analysis"
	},
	{
		"Name": "atomic",
		"Bool": true,
		"Usage": "enable atomic analysis"
	},
	{
		"Name": "bool",
		"Bool": true,
		"Usage": "deprecated alias for -bools"
	},
	{
		"Name": "bools",
		"Bool": true,
		"Usage": "enable bools analysis"
	},
	{
		"Name": "buildtag",
		"Bool": true,
		"Usage": "enable buildtag analysis"
	},
	{
		"Name": "buildtags",
		"Bool": true,
		"Usage": "deprecated alias for -buildtag"
	},
	{
		"Name": "c",
		"Bool": false,
		"Usage": "display offending line with this many lines of context"
	},
	{
		"Name": "cgocall",
		"Bool": true,
		"Usage": "enable cgocall analysis"
	},
	{
		"Name": "composites",
		"Bool": true,
		"Usage": "enable composites analysis"
	},
	{
		"Name": "composites.whitelist",
		"Bool": true,
		"Usage": "use composite white list; for testing only"
	},
	{
		"Name": "compositewhitelist",
		"Bool": true,
		"Usage": "deprecated alias for -composites.whitelist"
	},
	{
		"Name": "copylocks",
		"Bool": true,
		"Usage": "enable copylocks analysis"
	},
	{
		"Name": "errorsas",
		"Bool": true,
		"Usage": "enable errorsas analysis"
	},
	{
		"Name": "flags",
		"Bool": true,
		"Usage": "print analyzer flags in JSON"
	},
	{
		"Name": "framepointer",
		"Bool": true,
		"Usage": "enable framepointer analysis"
	},
	{
		"Name": "httpresponse",
		"Bool": true,
		"Usage": "enable httpresponse analysis"
	},
	{
		"Name": "ifaceassert",
		"Bool": true,
		"Usage": "enable ifaceassert analysis"
	},
	{
		"Name": "json",
		"Bool": true,
		"Usage": "emit JSON output"
	},
	{
		"Name": "loopclosure",
		"Bool": true,
		"Usage": "enable loopclosure analysis"
	},
	{
		"Name": "lostcancel",
		"Bool": true,
		"Usage": "enable lostcancel analysis"
	},
	{
		"Name": "methods",
		"Bool": true,
		"Usage": "deprecated alias for -stdmethods"
	},
	{
		"Name": "nilfunc",
		"Bool": true,
		"Usage": "enable nilfunc analysis"
	},
	{
		"Name": "printf",
		"Bool": true,
		"Usage": "enable printf analysis"
	},
	{
		"Name": "printf.funcs",
		"Bool": false,
		"Usage": "comma-separated list of print function names to check"
	},
	{
		"Name": "printfuncs",
		"Bool": false,
		"Usage": "deprecated alias for -printf.funcs"
	},
	{
		"Name": "rangeloops",
		"Bool": true,
		"Usage": "deprecated alias for -loopclosure"
	},
	{
		"Name": "shift",
		"Bool": true,
		"Usage": "enable shift analysis"
	},
	{
		"Name": "sigchanyzer",
		"Bool": true,
		"Usage": "enable sigchanyzer analysis"
	},
	{
		"Name": "source",
		"Bool": true,
		"Usage": "no effect (deprecated)"
	},
	{
		"Name": "stdmethods",
		"Bool": true,
		"Usage": "enable stdmethods analysis"
	},
	{
		"Name": "stringintconv",
		"Bool": true,
		"Usage": "enable stringintconv analysis"
	},
	{
		"Name": "structtag",
		"Bool": true,
		"Usage": "enable structtag analysis"
	},
	{
		"Name": "tags",
		"Bool": false,
		"Usage": "no effect (deprecated)"
	},
	{
		"Name": "testinggoroutine",
		"Bool": true,
		"Usage": "enable testinggoroutine analysis"
	},
	{
		"Name": "tests",
		"Bool": true,
		"Usage": "enable tests analysis"
	},
	{
		"Name": "unmarshal",
		"Bool": true,
		"Usage": "enable unmarshal analysis"
	},
	{
		"Name": "unreachable",
		"Bool": true,
		"Usage": "enable unreachable analysis"
	},
	{
		"Name": "unsafeptr",
		"Bool": true,
		"Usage": "enable unsafeptr analysis"
	},
	{
		"Name": "unusedfuncs",
		"Bool": false,
		"Usage": "deprecated alias for -unusedresult.funcs"
	},
	{
		"Name": "unusedresult",
		"Bool": true,
		"Usage": "enable unusedresult analysis"
	},
	{
		"Name": "unusedresult.funcs",
		"Bool": false,
		"Usage": "comma-separated list of functions whose results must be used"
	},
	{
		"Name": "unusedresult.stringmethods",
		"Bool": false,
		"Usage": "comma-separated list of names of methods of type func() string whose results must be used"
	},
	{
		"Name": "unusedstringmethods",
		"Bool": false,
		"Usage": "deprecated alias for -unusedresult.stringmethods"
	},
	{
		"Name": "v",
		"Bool": true,
		"Usage": "no effect (deprecated)"
	}
]

Perhaps we should only allow flags whose Usage string is of the form enable %s analysis?

If we are committing to allowing full configuration of vet

I don't think we should allow configuration of flags like -c and -v in go test -vet. If users want that much control, they can run go vet separately from go test.

Sweet, I too do not see the benefit in having -vet be a generic way to pass arbitrary flags, so I have struck the following section from the description: (persisted for historical purposes)

Otherwise, there are three main problematic cases.

distinguished

This may be resolved in CL 334873, but basically we should really not be accepting distinguished values and other things in the list of checks: e.g. -vet=off,anything.

deprecated

Because the contract that -vet provides is "run these checks", it is really confusing how -vet=v runs more linters than -vet="" (or not passing -vet). Since the list of flags labelled as no effect (deprecated) is finite and (hopefully) fixed, we can could ignore or error if any are passed.

value

If we are committing to allowing full configuration of vet, which may well be a bad idea, things like -c, takes values as input and should be able to be configured. This could be something like -vet=asmdecl,c=3,atomic, which is disabled today, by the failure to parse flag values containing =s. -vet=c3 would also be fine and is quite coreutils.

Also worth noting are deprecated check flags like -compositewhitelist, and their non-deprecated alternatives, -composites.whitelist, but that should work with the -c fix: -vet=asmdecl,composites.whitelist=false,atomic.

four

The -V flag causes output to be very long and unreadable, since it is runs for every package that is compiled for testing. Even if we allow arbitrary flags, I support ignoring this one explicitly.

Perhaps we should only allow flags whose Usage string is of the form enable %s analysis?

I have some concerns about the complexity of needing to keep these two components in sync, but if there is a clean way to do that, or somehow share the canonical list of vet checks: I am game. I only mention a block list, since I assume this flag list is frozen, except for new checks, meaning it should be pretty future proof.

@colin-sitehost: Somewhat tangential/unrelated to the main issue. But it could lead to a separate issue in which go test isn't following documented behavior.

In this issue description, the output you saw for go test -vet=all . was:

# trash/five
./main.go:7:8: using r before checking for errors
FAIL	trash/five [build failed]
testing: warning: no tests to run
PASS
ok  	trash/five	0.101s

However go help test documents that:

If go vet finds any problems, go test reports those and does not run the test binary.

So if the test binary did run (indicated by the PASS ok trash/five part) despite the vet failure, then go test didn't work as documented.

Can you confirm the actual output you saw/are seeing? Thanks. I haven't been able to reproduce on macOS.

// main.go

package main

import "net/http"

func main() {
	r, err := http.Get("example")
	defer r.Body.Close()
	if err != nil {
		panic(err)
	}
	_ = r
}

// main_test.go

package main

However, now I cannot reproduce it, so I have moved the old text from the description to not confuse the issue further:

# trash/five
./main.go:7:8: using r before checking for errors
FAIL	trash/five [build failed]
testing: warning: no tests to run
PASS
ok  	trash/five	0.101s

I have some concerns about the complexity of needing to keep these two components in sync, but if there is a clean way to do that, or somehow share the canonical list of vet checks: I am game.

I can think of at least two such ways.

One is to write a go:generate script that updates a map or function, and then verify that that map or function is up to date in a unit test. (That's the approach I used for #18682.)

Another is to invoke the vet tool once at run-time before running tests, as is currently done in go vet. But that only seems preferable if the vet tool itself can be specified at run-time, which is certainly true for go vet today but I think not for go test.

For my understanding, in go test -vet=a,b,c we propagate -a -b -c to vet during go test? But we should ideally do filtering of flag being passed to go vet?

@zpavlinovic, we currently propagate -vet=a,b,c as -a -b -c, yes. What I think we should do instead is not filtering, but validation: go test should fail outright if any of the -vet parameters does not name a valid (or, perhaps, a once-valid) analyzer flag.

One is to write a go:generate script that updates a map or function, and then verify that that map or function is up to date in a unit test. (That's the approach I used for #18682.)

This is then probably the most robust way going forward.

#35487 suggests having an exported list of analyzers available somewhere. This would be robust (but it does mean initializing all of the vet packages just to get their names).

Should we then block this issue until #35487 is resolved?

I don't think we need to block it on #35487. We can already scrape go tool vet -flags for Usage matching ^enable .* analysis$ or ^deprecated alias for -.*$ to get a list of the supported analysis flags.

That issue seems to be asking for a way to list the analyses used by default by go test and go vet, which are a subset of the analyses supported by the vet tool itself — and cmd/go already knows that subset anyway.

Ok, so the plan forward is to rely on go tool vet -flags to get the list of supported analysis flags.

Change https://golang.org/cl/341334 mentions this issue: cmd/go/internal/test: pass only analysis flags to vet