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

Closed
karalabe opened this issue Feb 26, 2019 · 11 comments
Closed

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

karalabe opened this issue Feb 26, 2019 · 11 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@karalabe
Copy link
Contributor

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
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 The path to resolution is known, but the work has not been done. 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
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
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
Copy link
Contributor

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
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.

@mvdan
Copy link
Member

mvdan commented Apr 16, 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 mvdan removed their assignment Apr 16, 2019
@mvdan
Copy link
Member

mvdan commented Apr 16, 2019

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

@gopherbot
Copy link

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

@gopherbot
Copy link

Change https://golang.org/cl/174520 mentions this issue: [release-branch.go1.12] cmd/vet: add tests for point-release issues

@gopherbot
Copy link

Closed by merging dc6db5f to release-branch.go1.12.

@gopherbot
Copy link

Closed by merging 9ac7093 to release-branch.go1.12.

gopherbot pushed a commit that referenced this issue May 1, 2019
…date from release-branch.go1.12

$ ./update-xtools.sh
Copied /Users/rsc/src/golang.org/x/tools@aa829657 to .
$ cd ~/src/golang.org/x/tools
$ git log -n1 aa829657
commit aa82965741a9fecd12b026fbb3d3c6ed3231b8f8 (HEAD -> release-branch.go1.12, origin/release-branch.go1.12)
Author:     Daniel Martí <mvdan@mvdan.cc>
AuthorDate: Fri Mar 1 11:00:19 2019 +0000
Commit:     Brad Fitzpatrick <bradfitz@golang.org>
CommitDate: Wed Mar 13 21:06:03 2019 +0000
...
$

Picks up cmd/vet fixes that have been inadvertently missed in point releases so far.

Fixes #30399.
Fixes #30465.

Change-Id: Ibcfaac51d134205b986b32f857d54006b19c896a
Reviewed-on: https://go-review.googlesource.com/c/go/+/174519
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
gopherbot pushed a commit that referenced this issue May 1, 2019
Add explicit tests for:

 #30465	cmd/vet: Consider reverting tag conflict for embedded fields
 #30399	cmd/vet: possible to get a printf false positive with big.Int

because we have managed not to fix them in the last
couple point releases, and it will be too embarrassing
to do that yet again.

Change-Id: Ib1da5df870348b6eb9bfc8a87c507ecc6d44b8dd
Reviewed-on: https://go-review.googlesource.com/c/go/+/174520
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@golang golang locked and limited conversation to collaborators Apr 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

5 participants