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.

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

cmd/go: list default vet flags in go test -help #26786

Open
iWdGo opened this Issue Aug 3, 2018 · 15 comments

Comments

Projects
None yet
6 participants
@iWdGo
Contributor

iWdGo commented Aug 3, 2018

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

go version go1.11beta2 windows/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

set GOBIN=
set GOCACHE=C:\Users\()\AppData\Local\go-build
set GOEXE=.exe
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOOS=windows
set GOPATH=C:\Users\()\Documents\Google\
set GOPROXY=
set GORACE=
set GOROOT=C:\Go
set GOTMPDIR=
set GOTOOLDIR=C:\Go\pkg\tool\windows_amd64
set GCCGO=gccgo
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOMOD=
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -mthreads -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=C:\Users\Costa\AppData\Local\Temp\go-build621759414=/tmp/go-build -gno-record-gcc-switches

What did you do?

This code trips go vet:

func UnreachableCode() {
	fmt.Println("this prints")
	return

	fmt.Println("this never prints")
}

A standard test like

func TestUnreachableCode() {
        UnreachableCode()
}

Of course, vet detects the issue correctly

>go vet
# _/c_/Users/()/sandbox/src/govetoff
.\govetoff.go:11: unreachable code

What did you expect to see?

>go test
# _/c_/Users/()/sandbox/src/govetoff
.\govetoff.go:11: unreachable code
FAIL    _/c_/Users/()/sandbox/src/govetoff [build failed]

What did you see instead?

c:\Users\()\sandbox\src\govetoff>go test
this prints
PASS
ok      _/c_/Users/()/sandbox/src/govetoff 2.602s

c:\Users\()\sandbox\src\govetoff>go test -vet
go test: missing argument for flag vet
run "go help test" or "go help testflag" for more information

The flag behavior is correct.

c:\Users\()\src\govetoff>go test -vet=off
this prints
PASS
ok      _/c_/Users/()/sandbox/src/govetoff 0.857s

c:\Users\()\sandbox\src\govetoff>go test -vet=all
# _/c_/Users/Costa/Documents/Google/CloudSDK/sandbox/src/govetoff
.\govetoff.go:11: unreachable code
FAIL    _/c_/Users/()/sandbox/src/govetoff [build failed]

Go vet behaves normally. The doc on go test flags states:

-vet list
Configure the invocation of "go vet" during "go test"
to use the comma-separated list of vet checks.
If list is empty, "go test" runs "go vet" with a curated list of
checks believed to be always worth addressing.
If list is "off", "go test" does not run "go vet" at all.

The present "curated" list seems empty but I did not try to locate it.
I supposed that unreachable code would be a prime choice for a curated list.
Behavior seems to contradict the current documentation of go test flags.

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Aug 3, 2018

You have written this bug report to say that go test does not invoke go vet, but as far as I can tell what you are actually saying is that go test does not pass -unreachable to go vet. That is true. The current set of flags that go test passes to go vet by default is -atomic -bool -buildtags -nilfunc -printf. I'll retitle this issue to suggest adding -unreachable.

@ianlancetaylor ianlancetaylor changed the title from cmd/go: go test does not run go vet unless explicitly requested to cmd/go: go test should pass -unreachable to vet by default Aug 3, 2018

@ianlancetaylor ianlancetaylor added this to the Go1.12 milestone Aug 3, 2018

@ysmolsky

This comment has been minimized.

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Aug 3, 2018

I'll add that one way to help determine whether this would be a good idea would be to run go test -vet=-unreachable on various packages and see if there are any false positives.

@iWdGo

This comment has been minimized.

Contributor

iWdGo commented Aug 3, 2018

It all started by speeding up a local build by removing vet during test in cmd/dist (#26472). My interpretation of documentation led me to this issue. The link above is helpful to understand what is veted where. It seems to me that supplementing documentation with the above link is helping.

@cznic

This comment has been minimized.

Contributor

cznic commented Aug 3, 2018

I'm afraid there may exist tests using build tags causing unreachable code.

I like go vet and I do use it, but I don't agree with it now being used as a non negotiable authority deciding if someone may or may not actually run go test. The only authorities with that much power should be the language specification and a human being, in that order.

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Aug 3, 2018

@cznic Yes, that is why we must be very conservative in adding additional vet checks. Although go test -vet=off is available, I agree that now that go test invokes vet, vet errors are in effect part of the language.

@iWdGo

This comment has been minimized.

Contributor

iWdGo commented Aug 4, 2018

I believe that the origin of my issue is that go test, even in verbose mode, does not display the flags that it passes to vet. I suggest to re-title with something like "go test verbose should display passed vet flags".

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Aug 5, 2018

go test -x does list the flags passed to vet. I assume by "verbose mode" you mean you the -v aka -test.v option, but that option doesn't mention vet at all. Changing the output of that option is likely to break existing scripts; I don't think that listing the vet arguments is a sufficient reason to risk that.

@iWdGo

This comment has been minimized.

Contributor

iWdGo commented Aug 6, 2018

Documentation about go test is not listing the -x flag as for other commands. It is referring to its usage. A line like -x prints the commands in the list of flags would help.

I ran the flag on the example above and go test -x and go test -x -vet=off list the same command for vet. Vet is really off in the test which means the the vet flags are passed correctly. I suppose that the line to find the list of flags that you're referring to is this one:

EOF
cd C:\Users\()\Documents\Google\CloudSDK\sandbox\src\govetoff
TERM='dumb' "C:\\Go\\pkg\\tool\\windows_amd64\\vet.exe" -atomic -bool -buildtags -nilfunc -printf "C:\\Users\\()\\AppData\\Local\\Temp\\go-build233594930\\b051\\vet.cfg"

To avoid disrupting scripts treating test results, I suggest to change the behavior of go test -vet which now returns

go test: missing argument for flag vet
run "go help test" or "go help testflag" for more information

Into the list of flags passed to vet like
default vet flags are -atomic -bool -buildtags -nilfunc -printf

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Aug 6, 2018

-x is one of the "build flags" that go test accepts.

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Aug 6, 2018

Probably we should just list the default vet flags in the output of go test -help.

@iWdGo

This comment has been minimized.

Contributor

iWdGo commented Aug 7, 2018

Fine by me. I looked around but could not locate where the help text is built to submit a related update.

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Aug 7, 2018

It's testFlag2 in cmd/go/internal/test/test.go.

@gopherbot

This comment has been minimized.

gopherbot commented Aug 22, 2018

Change https://golang.org/cl/130715 mentions this issue: cmd/go: display curated list of vet flags in help of go test

@iWdGo

This comment has been minimized.

Contributor

iWdGo commented Aug 22, 2018

GOFLAGS introduced recently allows to set -vet flag as deemed necessary.
With the fix, go test -h part about -vet would display as:

-vet list
            Configure the invocation of "go vet" during "go test"
            to use the comma-separated list of vet checks.
            If list is empty, "go test" runs "go vet" with a curated list of
            checks believed to be always worth addressing:
            -atomic -bool -buildtags -nilfunc -printf
            If list is "off", "go test" does not run "go vet" at all.

@bcmills bcmills changed the title from cmd/go: go test should pass -unreachable to vet by default to cmd/go: list default vet flags in go test -help Nov 14, 2018

@bcmills bcmills added the NeedsFix label Nov 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment