Go: cmd/vet: possible to get a printf false positive with big.Int

Created on 26 Feb 2019 · 11Comments · Source: golang/go

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.

FrozenDueToAge NeedsFix

Source

karalabe

Most helpful comment

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 on 26 Feb 2019

👍4

All 11 comments

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

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 on 26 Feb 2019

👍4

@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 on 28 Feb 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.

mvdan on 28 Feb 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.

alandonovan on 28 Feb 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.

mvdan on 28 Feb 2019

The x/tools piece was merged over a month ago; this just needs someone to update the vendored copy into a Go 1.12.x release. Unassigning myself, as I'm not needed for that bit.

mvdan on 16 Apr 2019

(for completeness, the 1.12 tools CL was https://go-review.googlesource.com/c/tools/+/164657)

mvdan on 16 Apr 2019

Change https://golang.org/cl/174519 mentions this issue: [release-branch.go1.12] cmd/vendor/golang.org/x/tools/go/analysis: update from release-branch.go1.12