Skip to content
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/vet: go vet -vettool=$(which shadow) errors in go1.13 only (flag provided but not defined: -unsafeptr) #34053

Open
leighmcculloch opened this issue Sep 3, 2019 · 6 comments

Comments

@leighmcculloch
Copy link
Contributor

commented Sep 3, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2019

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.

leighmcculloch added a commit to stellar/go that referenced this issue Sep 4, 2019
all: drop support for go1.11, add support for go1.13 (#1700)
Add testing and support for Go 1.13 and discontinue support for Go 1.11.

To maintain consistency with supported and released versions of Go. Go provides security updates for the last two versions of Go, this means the current release (1.13) and the previous release (1.12). Go 1.13 was released today meaning security updates will no longer be released for Go 1.11.

Go 1.13 also comes with several new features relating to Modules including more efficient and reliable downloading of dependencies. As we switch to using Modules it will be a better experience for all and we'll be making the most of the change to promote and use Go 1.13.

Close #1696 

Summary of changes:
- Update 1.13 builds to use latest release of 1.13.
- Remove 1.11 from builds.
- Update all documentation references of supported versions of Go.
- Update `govet.sh` to use `-vettool` option that was added in Go 1.12. This was an outstanding TODO item in the file already, but we needed to do it to make the file work with Go 1.12+. Closes #1038.
- Add notes to changelogs.

Known limitations & issues:
There is a bug in the Go 1.13's `go vet` tool that prevents it from being used with shadow. For this reason only the standard vet checks run on Go 1.13 builds, and the additional shadow check runs on Go 1.12 only. I stumbled onto this issue and opened an issue here for it: golang/go#34053.
@bcmills

This comment has been minimized.

Copy link
Member

commented Sep 4, 2019

The -unsafeptr argument likely comes from here:

// In GOROOT, we enable all the vet tests during 'go test',
// not just the high-confidence subset. This gets us extra
// checking for the standard library (at some compliance cost)
// and helps us gain experience about how well the checks
// work, to help decide which should be turned on by default.
// The command-line still wins.
//
// Note that this flag change applies even when running vet as
// a dependency of vetting a package outside std.
// (Otherwise we'd have to introduce a whole separate
// space of "vet fmt as a dependency of a std top-level vet"
// versus "vet fmt as a dependency of a non-std top-level vet".)
// This is OK as long as the packages that are farther down the
// dependency tree turn on *more* analysis, as here.
// (The unsafeptr check does not write any facts for use by
// later vet runs.)
if a.Package.Goroot && !VetExplicit {
// Note that $GOROOT/src/buildall.bash
// does the same for the misc-compile trybots
// and should be updated if these flags are
// changed here.
//
// There's too much unsafe.Pointer code
// that vet doesn't like in low-level packages
// like runtime, sync, and reflect.
vetFlags = []string{"-unsafeptr=false"}
}

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

CC @ianthehat @matloob

@bcmills bcmills added this to the Go1.14 milestone Sep 4, 2019

@leighmcculloch

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2019

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

@gyuho

This comment has been minimized.

Copy link
Contributor

commented Sep 6, 2019

Is there any workaround? etcd etcd-io/etcd#11110 is experiencing the same issue...

@matloob

This comment has been minimized.

Copy link
Contributor

commented Sep 6, 2019

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

This comment has been minimized.

Copy link

commented Sep 17, 2019

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", "", "")
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.