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: false positives and wrong types across files, needs automatic recursive checking #20514

Closed
purpleidea opened this issue May 28, 2017 · 23 comments

Comments

Projects
None yet
5 participants
@purpleidea
Copy link

commented May 28, 2017

Please answer these questions before submitting your issue. Thanks!

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

go version go1.6.4 linux/amd64

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

GOOS="linux"
GOARCH="amd64"

What did you do?

ran go vet

Code looks like:

log.Printf("foo: %s", whatever)

Replacing with whatever.String() solves the issue, but is unnecessarily verbose.

What did you expect to see?

No error.

What did you see instead?

resources/foo.go:180: arg whatever for printf verb %s of wrong type: *foo.Whatever

The particularly strange thing is that the code is correct, and compiles and runs perfectly, however we get the warning from go vet, however not all cases are picked up. Some work without warning, and others don't.

The #go-nuts folks agreed this was an issue, but nobody knows if it was reported yet. It should be fairly easy to know if there is a String() string or fmt.Stringer declared.

Thanks!

@alercah

This comment has been minimized.

Copy link

commented May 28, 2017

I'm unable to reproduce with Go 1.7.4: https://play.golang.org/p/dtO4-q8n2R

None of these cause a vet warning. Interestingly, the third line doesn't, even though B does not implement Stringer, so that may be a separate bug.

@purpleidea

This comment has been minimized.

Copy link
Author

commented May 28, 2017

@alercah A strange issue is that as I mentioned this seems to be inconsistent. I can post a large repo of this with an example shortly.

@josharian

This comment has been minimized.

Copy link
Contributor

commented May 28, 2017

If you run "go install thepackage" first, does the error go away? If so, it's fixed in 1.9.

@purpleidea

This comment has been minimized.

Copy link
Author

commented May 28, 2017

@josharian Just using go build -i and go vet in the source dir. Is there a specific bug that's pointing to that you had in mind?

@josharian

This comment has been minimized.

Copy link
Contributor

commented May 28, 2017

Lots. This is a common problem. See e.g. #16086. Also #19332. (And sadly, I recall now that this is only fixed if you pass -source to vet.) See ddbee9a. Anyway, do 'go install yourpackages' before 'go vet'. Or use 1.9 when it comes out, and pass -source to vet

@josharian josharian closed this May 28, 2017

purpleidea added a commit to purpleidea/mgmt that referenced this issue May 29, 2017

test: Run go vet with -source flag in newer releases
This should hopefully eliminate some false positives.
golang/go#20514
@purpleidea

This comment has been minimized.

Copy link
Author

commented May 29, 2017

@josharian This seems to happen with current tip (according to travis). As promised here is a concrete example (with -source):

https://travis-ci.org/purpleidea/mgmt/jobs/237053430#L713

So AFAICT, I think we should re-open this issue.

@alercah

This comment has been minimized.

Copy link

commented May 29, 2017

Where's the String() method? I don't see it.

@josharian

This comment has been minimized.

Copy link
Contributor

commented May 29, 2017

resources/file.go:202: arg obj for printf verb %s of wrong type: *resources.FileRes

This does indeed look wrong. If I read correctly, the arguments to Logf in that line all appear to be strings, not *resources.FileRes.

cc @valyala

@josharian josharian reopened this May 29, 2017

@josharian josharian changed the title False positive on go vet cmd/vet: false positives in vet check May 29, 2017

@josharian josharian changed the title cmd/vet: false positives in vet check cmd/vet: false positives and wrong types in format check May 29, 2017

purpleidea added a commit to purpleidea/mgmt that referenced this issue May 29, 2017

test: Run go vet with -source flag in newer releases
This should hopefully eliminate some false positives.
golang/go#20514

purpleidea added a commit to purpleidea/mgmt that referenced this issue May 29, 2017

test: Run go vet with -source flag in newer releases
This should hopefully eliminate some false positives.
golang/go#20514
@valyala

This comment has been minimized.

Copy link
Contributor

commented May 29, 2017

resources/file.go:202: arg obj for printf verb %s of wrong type: *resources.FileRes

This does indeed look wrong. If I read correctly, the arguments to Logf in that line all appear to be strings, not *resources.FileRes.

The resources/file.go:202 at commit 35f3529 looks like:

log.Printf("%s: Watching: %s", obj, obj.path) // attempting to watch...

Where obj has *resources.FileRes type, which has no String() method. So go vet correctly warns about the issue.

Adding the following method to FileRes eliminates vet warnings:

func (obj *FileRes) String() string { return "" }
@purpleidea

This comment has been minimized.

Copy link
Author

commented May 29, 2017

@valyala Indeed, however if you look closely:

You'll see the BaseRes has:

https://github.com/purpleidea/mgmt/blob/feat/pgraph-cleanup/resources/resources.go#L444

func (obj *BaseRes) String() string {

and that FileRes has:

// FileRes is a file and directory resource.
type FileRes struct {
BaseRes `yaml:",inline"`
...
}

https://github.com/purpleidea/mgmt/blob/feat/pgraph-cleanup/resources/file.go#L48

So indeed the String method correctly exists. But go vet is not correctly detecting this since it's in a child (naming?) struct.

@gopherbot

This comment has been minimized.

Copy link

commented May 29, 2017

CL https://golang.org/cl/44375 mentions this issue.

@valyala

This comment has been minimized.

Copy link
Contributor

commented May 29, 2017

It's strange, but now I cannot reproduce the issue neither with go1.8, nor with go tip. Added a test case , so the issue could be caught automatically if it will appear again.

@purpleidea

This comment has been minimized.

Copy link
Author

commented May 29, 2017

@valyala As I said it's weird! If I run a simplified (what I think is) equivalent to the issue, I don't see the problem. But If I run go vet on the whole code base I see the issue...

@alercah

This comment has been minimized.

Copy link

commented May 29, 2017

@purpleidea

This comment has been minimized.

Copy link
Author

commented May 29, 2017

If it's still an issue, it's usually helpful to reduce a minimal testcase

@alercah We're all aware, thanks! Would you like to help out in this area?

@purpleidea

This comment has been minimized.

Copy link
Author

commented May 29, 2017

@valyala I think I might have found the issue... Just putting together a test to prove my suspicions...

@purpleidea

This comment has been minimized.

Copy link
Author

commented May 29, 2017

@valyala The remaining issue turned out to be my fault in terms of usage, although as far as I'm concerned this should be considered a golang tooling bug:

Go vet was getting called per file, so since the definition of String() string happened in a different file, it couldn't detect the issue. Changing my tests so that it runs go vet -source *.go in each package dir fixes the issue for now, however surely inter-package go vet issues won't be found!

Lastly, running go vet on a main package that includes many sub and sub-sub packages, doesn't recursively check all the contents! If this did, it would have avoided all of this nonsense to begin with.

The fix: purpleidea/mgmt@46bda97
Proof: https://travis-ci.org/purpleidea/mgmt/jobs/237258602#L719

Hopefully my analysis is correct! Thanks for your time.

@purpleidea purpleidea changed the title cmd/vet: false positives and wrong types in format check cmd/vet: false positives and wrong types across files, needs automatic recursive checking May 29, 2017

@valyala

This comment has been minimized.

Copy link
Contributor

commented May 30, 2017

@purpleidea, the issue is reproducible only if the package sources are put outside GOPATH.

The issue cannot be reproduced after go get github.com/purpleidea/mgmt, that correctly puts the package into $GOPATH:

$ go get github.com/purpleidea/mgmt
$ cd $GOPATH/src/github.com/purpleidea/mgmt
$ go vet ./resources/file.go

So I'm unsure whether this is a bug. @robpike and @josharian , what do you think?

@valyala

This comment has been minimized.

Copy link
Contributor

commented May 30, 2017

Oh, I forgot to checkout the revision 35f3529 after go get. Now the issue is reproducible on $GOPATH:

$ cd $GOPATH/src/github.com/purpleidea/mgmt
$ git checkout 35f3529
$ go vet ./resources/file.go
resources/file.go:202: arg obj for printf verb %s of wrong type: *resources.FileRes
resources/file.go:214: arg obj for printf verb %s of wrong type: *resources.FileRes
resources/file.go:639: arg obj for printf verb %s of wrong type: *resources.FileRes
resources/file.go:701: arg obj for printf verb %s of wrong type: *resources.FileRes
resources/file.go:747: arg obj for printf verb %s of wrong type: *resources.FileRes

go vet ./resources, go vet github.com/purpleidea/mgmt/resources and go vet github.com/purpleidea/mgmt/... correctly detect that BaseRes implements fmt.Stringer

@josharian

This comment has been minimized.

Copy link
Contributor

commented May 30, 2017

Running $ go doc cmd/vet says:

It can be invoked three ways:

By package, from the go tool:

    go vet package/path/name

vets the package whose path is provided.

By files:

    go tool vet source/directory/*.go

vets the files named, all of which must be in the same package.

By directory:

    go tool vet source/directory

recursively descends the directory, vetting each package it finds.

If you run go vet on individual files, I think it is reasonable that vet does not see the contents of other files in the same package, and that errors like this occur. Don't do that.

As for checking all packages, go vet toplevel/... or go tool vet toplevel should work. Maybe there's more we should do to document this, but it's not obvious to me what.

One could imagine adding a -transitive flag to go vet, such that go vet -transitive pkg runs vet on pkg and everything it depends on. But ./... and the go tool vet commands seem to be enough for most people in practice.

I do think that the difference in calling convention between go vet and go tool vet is confusing and underdocumented, but I don't think that that was the issue here.

So all in all, I'm not sure there is more to do for this specific issue, but I'm open to suggestions.

@purpleidea

This comment has been minimized.

Copy link
Author

commented May 30, 2017

@josharian Hmm.. I didn't realize it would work on a dir path. For some reason go vet does not, where as go tool vet does. Weird.

In any case, with this information, I just realized that this checks the vendor dir too, and something there always fails, so using it in this manner makes it difficult to use this as a test case, and we need to fall back to shell scripting to make this all work. I actually love shell, but I am a bit disappointed that the stock tools require shell wrapping to do what I think most users would want.

@purpleidea

This comment has been minimized.

Copy link
Author

commented May 30, 2017

I'll close this issue because my issue is worked-around with shell, and thanks for everyone's time and input. In a positive manner, if golang got rid of the CLA and the high barrier to getting code in, maybe more user patches would help round out the tools a bit better! Thanks again.

@purpleidea purpleidea closed this May 30, 2017

gopherbot pushed a commit that referenced this issue May 30, 2017

cmd/vet: add a test for embedded stringer
This should help narrowing down the possible cause of #20514.

Updates #20514.

Change-Id: Ie997400c9749aace7783bd585b23dbb4cefc181d
Reviewed-on: https://go-review.googlesource.com/44375
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>
Reviewed-by: Rob Pike <r@golang.org>
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@josharian

This comment has been minimized.

Copy link
Contributor

commented May 31, 2017

go vet works on package paths (intepreted relative to GOPATH). go tool vet works on directories.

As for your vendor problem, see #17058 and #18085.

maybe more user patches would help round out the tools a bit better!

I do agree that we should reduce the barrier to contribution. (The CLA isn't going away, though.)

However, I think the problem here is not willingness to implement a fix, but identifying exactly what needs to be fixed and what the fix is.

purpleidea added a commit to purpleidea/mgmt that referenced this issue May 31, 2017

test: Run go vet with -source flag in newer releases
This should hopefully eliminate some false positives.
golang/go#20514

@golang golang locked and limited conversation to collaborators May 31, 2018

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