cmd/vet: GO111MODULE=off go test -count=1 cmd/vet fails

[cuonglm]

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

$ go version
go version devel +9be2d46422 Fri May 17 06:01:17 2019 +0000 linux/amd64

Does this issue reproduce with the latest release?

No

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

go env Output

$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/cuonglm/.cache/go-build"
GOENV="/home/cuonglm/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/cuonglm/go"
GOPROXY="direct"
GOROOT="/home/cuonglm/sources/go"
GOSUMDB="off"
GOTMPDIR=""
GOTOOLDIR="/home/cuonglm/sources/go/pkg/tool/linux_amd64"
GCCGO="/usr/local/bin/gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build714996541=/tmp/go-build -gno-record-gcc-switches"

What did you do?

$ GO111MODULE=off go vet cmd/vet/testdata/unsafeptr
$ GO111MODULE=on go vet cmd/vet/testdata/unsafeptr
# cmd/vet/testdata/unsafeptr
cmd/vet/testdata/unsafeptr/unsafeptr.go:12:6: possible misuse of unsafe.Pointer

What did you expect to see?

go vet produces output regardless of GO111MODULE value.

What did you see instead?

go vet does not produce output when GO111MODULE=off.

[bcmills]

@rsc has been doing some refactoring in cmd/vet. Perhaps he knows?

[cuonglm]

@bradfitz I think maybe because I set GO111MODULE=off

Hope anyone haven't run go mod tidy can try this.

[cuonglm]

@bcmills @bradfitz Anyway, since when go mod tidy changes the go.sum, should I send a CL to update it?

[cuonglm]

@bcmills @bradfitz The error occurs again in my local

ok  	cmd/trace	0.064s
--- FAIL: TestVet (0.90s)
    --- FAIL: TestVet/print (0.27s)
        vet_test.go:162: error check failed: 
            print.go:169: missing error "Warn call has possible formatting directive %s"
            print.go:170: missing error "Warnf call needs 1 arg but has 2 args"
            print.go:171: missing error "Warnf format %r has unknown verb r"
            print.go:172: missing error "Warnf format %#s has unrecognized flag #"
            print.go:173: missing error "Warn2 call has possible formatting directive %s"
            print.go:174: missing error "Warnf2 call needs 1 arg but has 2 args"
            print.go:175: missing error "Warnf2 format %r has unknown verb r"
            print.go:176: missing error "Warnf2 format %#s has unrecognized flag #"
    --- FAIL: TestVet/unsafeptr (0.27s)
        vet_test.go:153: vet output:
        vet_test.go:154: <nil>
FAIL
FAIL	cmd/vet	1.939s
FAIL
2019/05/20 14:30:35 Failed: exit status 1

[cuonglm]

@bcmills @bradfitz I think I found the culprit, updated the issue.

[bradfitz]

@bcmills @bradfitz Anyway, since when go mod tidy changes the go.sum, should I send a CL to update it?

No, no need.

[cuonglm]

I believe the problem comes from this line https://github.com/golang/tools/blob/9558fe4a7893c491f71cd402ea24d4a9366b8918/go/analysis/internal/analysisflags/flags.go#L56

When GO111MODULE=on, cmd/vet/testdata/unsafeptr is treated as a package path, so the flag for unsafeptr is not set. When GO111MODULE=off, cmd/vet/testdata/unsafeptr is treated as a flag toggle, the flag for unsafeptr set to false, causing it never run.

Is it a flag.Parse() bug? cc @bradfitz @bcmills

[rsc]

It's unclear that go vet cmd/vet/testdata/unsafeptr was ever meant to be useful. The testdata directories are explicitly excluded from matching patterns like all and cmd and cmd/.... If this bug is only about the behavior on that specific argument, I'm not particularly worried.

If this is really about a real package, please give an example that is not testing a testdata directory.

[rsc]

OK, I see that the real problem is GO111MODULE=off go test -count=1 cmd/vet failing. I will adjust the title here.

[rsc]

Separately, it may be that we should special-case GO111MODULE and include it in the test cache key hash. I am seeing GO111MODULE=on go test cmd/vet apparently fill a cache result used by GO111MODULE=off go test cmd/vet.

[cuonglm]

@rsc GO111MODULE=off go test -count=1 cmd/vet is actually what I reported in the first time. I changed to go vet cmd/vet/testdata/unsafeptr because when I did some debug, I saw that the flag in this line https://github.com/golang/tools/blob/9558fe4a7893c491f71cd402ea24d4a9366b8918/go/analysis/internal/analysisflags/flags.go#L56 was set to false for unsafeptr if GO111MODULE=off.

I did not look much into it since #32107 (comment), would take another look when I have time.

[cuonglm]

Separately, it may be that we should special-case GO111MODULE and include it in the test cache key hash. I am seeing GO111MODULE=on go test cmd/vet apparently fill a cache result used by GO111MODULE=off go test cmd/vet.

Filled #32285

[bcmills]

Separately, it may be that we should special-case GO111MODULE and include it in the test cache key hash.

The test's dependency on GO111MODULE comes from the combination of being in the standard library (and hence not having any module dependencies), plus executing cmd/go as a subprocess (which adds the dependency on module-mode behavior).

Instead of hard-coding GO111MODULE in particular, I think we should make internal/testenv.GoToolPath touch all of the environment variables listed in cmd/go/internal/cfg.knownEnv. That makes the connection between the variables and the test results more explicit and much more robust: tests that execute the go command are potentially sensitive to all of the variables that the go command reads.

[cuonglm]

This was fixed in 902d5aa