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: possible to get a printf false positive with big.Int #30399

Open
karalabe opened this Issue Feb 26, 2019 · 5 comments

Comments

Projects
None yet
4 participants
@karalabe
Copy link
Contributor

karalabe commented Feb 26, 2019

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

$ go version
go version go1.12 linux/amd64

Does this issue reproduce with the latest release?

Yes.

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

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/karalabe/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/tmp/go"
GOPROXY=""
GORACE=""
GOROOT="/opt/google/go"
GOTMPDIR=""
GOTOOLDIR="/opt/google/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
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-build576186895=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Here's a full repro:

$ mkdir /tmp/go
$ GOPATH=/tmp/go
$ go get github.com/karalabe/govetrepro2/...
$ cd /tmp/go/src/github.com/karalabe/govetrepro2/
$ go test ./...
# github.com/karalabe/govetrepro2/bn256/google
bn256/google/main_test.go:16:3: Logf format %d has arg n of wrong type *math/big.Int
ok  	github.com/karalabe/govetrepro2/bn256/cloudflare	0.065s
FAIL	github.com/karalabe/govetrepro2/bn256/google [build failed]

What did you expect to see?

The exact same test code (main_test.go) is present in both bn256/cloudflare and bn256/google. I expect either both to fail with the format error, or neither of them.

What did you see instead?

Cloudflare always passes, Google always fails.

@mvdan

This comment has been minimized.

Copy link
Member

mvdan commented Feb 26, 2019

Thanks for the detailed report! This is a false positive, as math/big.Int implements fmt.Formatter, so the printf vet check should skip it.

This tiny program reproduces the bug:

$ cat f.go
package p

import (
        "math/big"
        "testing"
)

func f(t *testing.T) {
        t.Logf("%d\n", big.NewInt(4))
}
$ go vet f.go
# command-line-arguments
./f.go:9:9: Logf format %d has arg big.NewInt(4) of wrong type *math/big.Int

It seems like the issue is that vet assumes that a program must import fmt to implement fmt.Formatter. Which is correct, but not the way it's used in the vet check. In this case, it's imported in the package that implements fmt.Formatter, which is math/big, and not the current package.

You can see that this is the cause because adding an import _ "fmt" makes the false positive go away.

I'll send a CL to the tools repo later today, and I'm sure we can include a backport for 1.12.1's cmd/vet. /cc @alandonovan

@mvdan mvdan self-assigned this Feb 26, 2019

@mvdan mvdan added the NeedsFix label Feb 26, 2019

@mvdan mvdan changed the title Inconsistent (test) build failures on Go 1.12 cmd/vet: possible to get a printf false positive with big.Int Feb 26, 2019

@mvdan mvdan added this to the Go1.12.1 milestone Feb 28, 2019

@mvdan

This comment has been minimized.

Copy link
Member

mvdan commented Feb 28, 2019

@alandonovan now that cmd/vet is out of repo, how can we easily backport single commits? With git it should be possible to cherry-pick the x/tools commit into cmd/vendor/golang.org/x/tools, but I don't know if you have a better plan.

@mvdan

This comment has been minimized.

Copy link
Member

mvdan commented Feb 28, 2019

I also messed up the "Fixes" line in the x/tools commit, as usual. But that's actually good, as the issue isn't fixed in neither master nor 1.12.

@alandonovan

This comment has been minimized.

Copy link
Contributor

alandonovan commented Feb 28, 2019

I don't have any better ideas than simply applying the same change to both repos. The x/tools/go/analysis tree is subject to the release freeze, so I don't think there's any situation in which they should deviate.

@mvdan

This comment has been minimized.

Copy link
Member

mvdan commented Feb 28, 2019

Ah right, I keep forgetting that x/tools has a 1.12 branch as well. I presume we can just cherry-pick into its 1.12 branch, and update Go's 1.12 branch to vendor x/tools' updated 1.12 branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.