Go: cmd/vet: go vet -vettool=$(which shadow) errors in go1.13 only (flag provided but not defined: -unsafeptr)

[leighmcculloch]

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

$ go version
go version go1.13 linux/amd64

Does this issue reproduce with the latest release?

Yes

Does this issue reproduce with the previous release?

No

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

go env Output

$ go env

GO111MODULE=""

GOARCH="amd64"

GOBIN="/home/leighmcculloch/local/bin"

GOCACHE="/home/leighmcculloch/.cache/go-build"

GOENV="/home/leighmcculloch/.config/go/env"

GOEXE=""

GOFLAGS=""

GOHOSTARCH="amd64"

GOHOSTOS="linux"

GONOPROXY=""

GONOSUMDB=""

GOOS="linux"

GOPATH="/home/leighmcculloch/go"

GOPRIVATE=""

GOPROXY="https://proxy.golang.org,direct"

GOROOT="/home/leighmcculloch/local/bin/go/1.13"

GOSUMDB="sum.golang.org"

GOTMPDIR=""

GOTOOLDIR="/home/leighmcculloch/local/bin/go/1.13/pkg/tool/linux_amd64"

GCCGO="gccgo"

AR="ar"

CC="gcc"

CXX="g++"

CGO_ENABLED="1"

GOMOD="/home/leighmcculloch/devel/stellar-go/go.mod"

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-build252896639=/tmp/go-build -gno-record-gcc-switches"

What did you do?

$ mkdir test
$ cd test
$ go mod init test
$ cat > main.go <<-EOF
package main

import "fmt"

func main() {
        v := "bye"
        for {
                v := "hello"
                fmt.Println(v)
                break
        }
        fmt.Println(v)
}
EOF
$ go get -u golang.org/x/tools/go/analysis/passes/shadow/cmd/shadow
$ go vet -vettool=$(which shadow)

What did you expect to see?

I expected to see the output indicating the shadow variable:

$ go vet -vettool=$(which shadow)
# test
./main.go:8:3: declaration of "v" shadows declaration at line 6

What did you see instead?

The output was huge, 1541 lines of output. It was repeatedly dumping out an error flag provided but not defined: -unsafeptr once for some number of standard library packages along with the usage documentation for the shadow command. At the very end was the output I expected.

$ go vet -vettool=$(which shadow)
# internal/cpu
flag provided but not defined: -unsafeptr
shadow: check for possible unintended shadowing of variables

Usage: shadow [-flag] [package]

This analyzer check for shadowed variables.
...
# test
./main.go:8:3: declaration of "v" shadows declaration at line 6

To condense the output down a bit we can see the packages it is displaying the error about:

$ go vet -vettool=$(which shadow) 2>&1 | grep '^#'
# internal/cpu
# runtime/internal/sys
# internal/bytealg
# runtime/internal/math
# unicode/utf8
# internal/race
# sync/atomic
# unicode
# math/bits
# internal/testlog
# runtime/internal/atomic
# math
# runtime
# internal/reflectlite
# sync
# sort
# errors
# io
# internal/oserror
# strconv
# syscall
# reflect
# internal/syscall/unix
# time
# internal/fmtsort
# internal/poll
# os
# fmt
# test

Does this issue reproduce with the previous release? (details)

No. This doesn't happen with Go 1.12.9. If I run the above commands with that version of Go, it behaves as expected and outputs the shadow errors. Example:

$ go1.12.9 vet -vettool=$(which shadow)           
# test
./main.go:8:3: declaration of "v" shadows declaration at line 6

[leighmcculloch]

The process I'm following for installing shadow is in the Go 1.12 release docs here:
https://golang.org/doc/go1.12#vet

If you run go help vet the install instructions are captured there also:

$ go1.13 help vet
usage: go vet [-n] [-x] [-vettool prog] [build flags] [vet flags] [packages]
...
The -vettool=prog flag selects a different analysis tool with alternative
or additional checks.
For example, the 'shadow' analyzer can be built and run using these commands:

  go install golang.org/x/tools/go/analysis/passes/shadow/cmd/shadow
  go vet -vettool=$(which shadow)
...

So I believe I am using the tool appropriately.

[bcmills]

The -unsafeptr argument likely comes from here:
https://github.com/golang/go/blob/407010ef0b858a7fa6e6e95abe652fdff923da9a/src/cmd/go/internal/work/exec.go#L1017-L1043

Probably the shadow command needs to be updated to be flag-compatible with the actual vet command.

CC @ianthehat @matloob

[leighmcculloch]

Could we instead not pass this flag down to custom tools? It seems unnecessary to require all vettool programs to support this flag.

[gyuho]

Is there any workaround? etcd https://github.com/etcd-io/etcd/pull/11110 is experiencing the same issue...

[matloob]

Expanding on @leighmcculloch's proposal Would it be reasonable to redefine VetExplicit to mean whether flags are passed to vet or a custom vet tool is used? I think users of vet with a custom tool would be okay filtering out the unsafeptr errors in goroot if their tool included it.

One workaround (which might not be workable in your case) is to add a unsafeptr flag to your custom tool

[viswajithiii]

If it helps anyone, I used the following workaround (building on @matloob's suggestion):

Save this in a file called, say, customvettool/customvettool.go. Run go install ./customvettool to install the binary, and then you can invoke as go vet -vettool=$(which customvettool).

package main

import (
    "flag"

    "golang.org/x/tools/go/analysis/passes/shadow"
    "golang.org/x/tools/go/analysis/unitchecker"
)

func main() {
    unitchecker.Main(
        shadow.Analyzer,
                // ... other custom analyzers
    )
}

func init() {
    // go vet always adds this flag for certain packages in the standard library,
    // which causes "flag provided but not defined" errors when running with
    // custom vet tools.
    // So we just declare it here and swallow the flag.
    // See https://github.com/golang/go/issues/34053 for details.
    // TODO: Remove this once above issue is resolved.
    flag.String("unsafeptr", "", "")
}

[kevinburke]

Previously, this check was also broken in the Go 1.12 release. https://github.com/golang/go/issues/29260

[leighmcculloch]

It also seems like we can work around this for the moment by running shadow independently of go vet. In our workflow we can replace go vet -vettool=$(which shadow) ./... with shadow ./....

[agnivade]

By the time I found this issue, I already spent too much time trying to figure out what was wrong in my codebase. Now I am bothered enough to fix this.

There seems to be a separate VetTool flag which indicates a presence of an alternate binary.

// VetTool is the path to an alternate vet tool binary.
// The caller is expected to set it (if needed) before executing any vet actions.
var VetTool string

Adding this to the if condition should do it. Will send a CL.

[gopherbot]

Change https://golang.org/cl/200957 mentions this issue: cmd/go/internal/work: fix error while passing custom vet tool

[hanzei]

Will this fix get back ported into 1.13?

[agnivade]

It's definitely a backport candidate. But will leave it to @bcmills for a decision.

[bcmills]

@gopherbot, please backport to 1.13: this was an early regression and the fix is trivial.

[gopherbot]

Backport issue(s) opened: #34922 (for 1.13).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

[gopherbot]

Change https://golang.org/cl/201237 mentions this issue: [release-branch.go1.13] cmd/go/internal/work: fix error while passing custom vet tool