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 incorrectly warns of "no formatting directive in Errorf call" for non core methods with same name #12294

Closed
piersy opened this issue Aug 24, 2015 · 7 comments

Comments

Projects
None yet
8 participants
@piersy
Copy link

commented Aug 24, 2015

We have a log object it provides a method Errorf

Go vet prints a warning - "no formatting directive in Errorf call"

I would expect go vet to be able to determine that this Errorf call is not a call to the standard library Errorf function.

go version go1.5 linux/amd64

@ianlancetaylor ianlancetaylor changed the title go vet incorrectly warns of "no formatting directive in Errorf call" for non core methods with same name cmd/vet: go vet incorrectly warns of "no formatting directive in Errorf call" for non core methods with same name Aug 24, 2015

@ianlancetaylor ianlancetaylor added this to the Unplanned milestone Aug 24, 2015

@bradfitz

This comment has been minimized.

Copy link
Member

commented Aug 24, 2015

What is the signature of your Errorf method?

govet doesn't care about the standard library; it enforces the rule that if your function is named one of the popular -f ending functions, then it should probably be acting like them:

var printfList = map[string]int{
    "errorf":  0,
    "fatalf":  0,
    "fprintf": 1,
    "logf":    0,
    "panicf":  0,
    "printf":  0,
    "sprintf": 0,
}

This is probably working as intended, but please share more details.

@Setomidor

This comment has been minimized.

Copy link

commented Sep 3, 2015

I have exactly the same problem, and my Fatalf looks as follows:

func Fatalf(tag string, fullID string, format string, a ...interface{}) {

Is it the extra arguments that matter? The signature of the original method is:

func Fatalf(format string, v ...interface{})

@m3ng9i

This comment has been minimized.

Copy link

commented Sep 15, 2015

Similar problem. Here is the code:

// testvet.go
package main

import "fmt"

type S struct {
}

func (this *S) Printf(i int) {
    fmt.Println(i)
}

func main() {
    s := &S{}
    s.Printf(1)
}

After run go vet testvet.go, I got an error: testvet.go:16: constant 1 not a string in call to Printf.

But parameter of S.Printf() is exactly int, not string.

My go version: go1.5.1 windows/386

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Sep 15, 2015

@m3ng9i This is working as expected. Your choices are 1) rename your Printf function; 2) don't run vet.

@spenczar

This comment has been minimized.

Copy link
Contributor

commented Feb 23, 2016

I'd like to revive this topic, motivated by gRPC's Errorf function which triggers vet.

gRPC has a public function with this signature:

func Errorf(c codes.Code, format string, a ...interface{}) error

Using this function causes vet to shriek, as noted in grpc/grpc-go#90. Given that grpc is maintained by core Go authors who are unwilling to rename their function, I think it's fair to say some more effort in go vet's side here would be worthwhile to expand its set of covered programs.

It seems to me that vet could be a little more intelligent here. It should be easily possible to identify the function's call signature and figure out whether its first parameter is a string. Vet could merely warn that it's unable to handle non-standard Printf calls, or perhaps it could try to suss out which parameter is a format string, and appropriately vet the arguments passed in.

This comment is a proposal to gauge interest; I'm happy to be the one to do the work of writing the CL but I want to make sure that this change to vet's behavior is desirable.

@gopherbot

This comment has been minimized.

Copy link

commented Mar 3, 2016

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

@valyala

This comment has been minimized.

Copy link
Contributor

commented Mar 10, 2016

The CL https://golang.org/cl/20163 breaks go vet -printfuncs=FooBarf if FooBarf is defined outside the current package. It looks like signature() returns nil in this case, which results in no formatting directive in FooBarf call warnings for each FooBarf(format, <args>) call.
I'd suggest falling back to the previous behavior if signature() returns nil.

tamird added a commit to tamird/cockroach that referenced this issue Aug 2, 2016

tamird added a commit to tamird/cockroach that referenced this issue Aug 2, 2016

Bump to go1.7rc3
I'd go to rc4, but I don't feel like waiting for docker. See
docker-library/official-images#2013.

Also unignore grpc.Errorf in `go vet`, now that it's fixed upstream.
See golang/go#12294.

Also remove TLS handshakes from leaktest's whitelist, since these leaks
should be gone in 1.7. See golang/go#14539.

Closes cockroachdb#8053.
Closes cockroachdb#8136.
Closes cockroachdb#8215.
Closes cockroachdb#8217.
Closes cockroachdb#8218.
Closes cockroachdb#8219.
Closes cockroachdb#8237.
Closes cockroachdb#8250.
Closes cockroachdb#8255.

tamird added a commit to tamird/cockroach that referenced this issue Aug 2, 2016

Bump to go1.7rc3
I'd go to rc4, but I don't feel like waiting for docker. See
docker-library/official-images#2013.

Also unignore grpc.Errorf in `go vet`, now that it's fixed upstream.
See golang/go#12294.

Also remove TLS handshakes from leaktest's whitelist, since these leaks
should be gone in 1.7. See golang/go#14539.

Closes cockroachdb#8053.
Closes cockroachdb#8136.
Closes cockroachdb#8215.
Closes cockroachdb#8217.
Closes cockroachdb#8218.
Closes cockroachdb#8219.
Closes cockroachdb#8237.
Closes cockroachdb#8250.
Closes cockroachdb#8255.

tamird added a commit to tamird/cockroach that referenced this issue Aug 2, 2016

Bump to go1.7rc3
I'd go to rc4, but I don't feel like waiting for docker. See
docker-library/official-images#2013.

Also unignore grpc.Errorf in `go vet`, now that it's fixed upstream.
See golang/go#12294.

Also remove TLS handshakes from leaktest's whitelist, since these leaks
should be gone in 1.7. See golang/go#14539.

Closes cockroachdb#8053.
Closes cockroachdb#8136.
Closes cockroachdb#8215.
Closes cockroachdb#8217.
Closes cockroachdb#8218.
Closes cockroachdb#8219.
Closes cockroachdb#8237.
Closes cockroachdb#8250.
Closes cockroachdb#8255.

tamird added a commit to tamird/cockroach that referenced this issue Aug 3, 2016

Bump to go1.7rc3
I'd go to rc4, but I don't feel like waiting for docker. See
docker-library/official-images#2013.

Also unignore grpc.Errorf in `go vet`, now that it's fixed upstream.
See golang/go#12294.

Also remove TLS handshakes from leaktest's whitelist, since these leaks
should be gone in 1.7. See golang/go#14539.

Closes cockroachdb#8053.
Closes cockroachdb#8136.
Closes cockroachdb#8215.
Closes cockroachdb#8217.
Closes cockroachdb#8218.
Closes cockroachdb#8219.
Closes cockroachdb#8237.
Closes cockroachdb#8250.
Closes cockroachdb#8255.

tamird added a commit to tamird/cockroach that referenced this issue Aug 3, 2016

Bump to go1.7rc3
I'd go to rc4, but I don't feel like waiting for docker. See
docker-library/official-images#2013.

Also unignore grpc.Errorf in `go vet`, now that it's fixed upstream.
See golang/go#12294.

Also remove TLS handshakes from leaktest's whitelist, since these leaks
should be gone in 1.7. See golang/go#14539.

Closes cockroachdb#8053.
Closes cockroachdb#8136.
Closes cockroachdb#8215.
Closes cockroachdb#8217.
Closes cockroachdb#8218.
Closes cockroachdb#8219.
Closes cockroachdb#8237.
Closes cockroachdb#8250.
Closes cockroachdb#8255.

tamird added a commit to tamird/cockroach that referenced this issue Aug 3, 2016

Bump to go1.7rc3
I'd go to rc5, but I don't feel like waiting for docker. See
docker-library/official-images#2013.

Also unignore grpc.Errorf in `go vet`, now that it's fixed upstream.
See golang/go#12294.

Also remove TLS handshakes from leaktest's whitelist, since these leaks
should be gone in 1.7. See golang/go#14539.

Closes cockroachdb#8053.
Closes cockroachdb#8136.
Closes cockroachdb#8215.
Closes cockroachdb#8217.
Closes cockroachdb#8218.
Closes cockroachdb#8219.
Closes cockroachdb#8237.
Closes cockroachdb#8250.
Closes cockroachdb#8255.

tamird added a commit to tamird/cockroach that referenced this issue Aug 3, 2016

Bump to go1.7rc3
I'd go to rc5, but I don't feel like waiting for docker. See
docker-library/official-images#2013.

Also unignore grpc.Errorf in `go vet`, now that it's fixed upstream.
See golang/go#12294.

Also remove TLS handshakes from leaktest's whitelist, since these leaks
should be gone in 1.7. See golang/go#14539.

Closes cockroachdb#8053.
Closes cockroachdb#8136.
Closes cockroachdb#8215.
Closes cockroachdb#8217.
Closes cockroachdb#8218.
Closes cockroachdb#8219.
Closes cockroachdb#8237.
Closes cockroachdb#8250.
Closes cockroachdb#8255.

@golang golang locked and limited conversation to collaborators Mar 13, 2017

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.