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 positive printf-like function detection #26486

Closed
decathorpe opened this issue Jul 19, 2018 · 11 comments

Comments

Projects
None yet
7 participants
@decathorpe
Copy link

commented Jul 19, 2018

go version: 1.11 beta 1 (regression compared to 1.10 branch)
OS version: fedora rawhide

The go package github.com/cznic/ql can't compile its tests with go 1.11 beta 1, which is a regression compared to go 1.10.

I've reported this issue originally here:
cznic/ql#203

However, I agree with the developer that this vet error looks like a false-positive, since the function in question (included below) doesn't actually need formatting directives.

function triggering this vet error:

func dbg(s string, va ...interface{}) {
	if s == "" {
		s = strings.Repeat("%v ", len(va))
	}
	_, fn, fl, _ := runtime.Caller(1)
	fmt.Printf("dbg %s:%d: ", path.Base(fn), fl)
	fmt.Printf(s, va...)
	fmt.Println()
}

error message:

(...)/src/github.com/cznic/ql/storage_test.go:423: dbg call has arguments but no formatting directives
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2018

/src/github.com/cznic/ql/storage_test.go:423: dbg call has arguments but no formatting directives

Looking at the current repo, line 423 is

dbg("", err)

That will cause the dbg function is to call fmt.Printf("", err). So this looks like a true positive to me: the dbg call does indeed have arguments but no formatting directives.

@cznic

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2018

That will cause the dbg function is to call fmt.Printf("", err)

AFAICT, dbg("", err) will instead execute fmt.Printf("%v ", err) due to this preceding if statement.

        if s == "" {
		s = strings.Repeat("%v ", len(va))
	}
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2018

@cznic Quite right, sorry for missing that.

So what is happening here is that vet is seeing that the arguments to dbg are being passed to fmt.Printf, and that therefore dbg is a formatter, and that it therefore deserves the warning.

Perhaps it is possible to modify the formatting checks to not attempt to warn if the function modifies its parameters. Let me see how difficult that would be to implement.

@gopherbot

This comment has been minimized.

Copy link

commented Jul 19, 2018

Change https://golang.org/cl/125039 mentions this issue: cmd/vet: if a function modifies its args, it's not a print wrapper

@robpike

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2018

I don't think it's that simple. Couldn't this could turn off checking for too many benign functions, such as this one:

func printf(format string, args ...interface{}) {
   format = "pkg: " + format
   fmt.Printf(format, args...)
}
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2018

@robpike I'm more or less assuming that false positives must be avoided. I don't see how to reasonably handle your example while still not issuing a warning for the dbg example here.

We could decide that we are going to warn for dbg, and accept the false positive. However I'm somewhat reluctant to do that for a vet check that is run automatically by go test. My sense is that for those tests we need to push the false positive rate as low as possible. If the dbg example were just absurd, that would be one thing, but to me it seems like reasonable code for its purpose.

@bcmills

This comment has been minimized.

Copy link
Member

commented Jul 20, 2018

@robpike

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2018

@ianlancetaylor I'm not convinced your starting point is the right one. I want to keep false positives low but not at the point of disabling checks too broadly. The dbg function seems misguided to me, and your fix turns off checking for any formats passed to it. That seems wrong. I would rewrite the code to have dbg and dbgf, fixing the code rather than vet.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2018

Because we run go vet for every use of go test, we are going to be breaking existing, working, correct code. We don't have to adopt my CL but I don't think we should require this kind of code to change in order to use Go 1.11.

@mvdan

This comment has been minimized.

Copy link
Member

commented Jul 20, 2018

What about temporarily applying a conservative fix for 1.11, and looking for a better solution for 1.12? If I remember correctly, the printf checks were completely disabled for a similar reason in 1.10 and are coming back in 1.11.

@robpike

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2018

I came here to suggest what @mvdan says. If you want to make progress, I suppose the CL should land, but:

  • it needs a TODO saying it's too dismissive.
  • there should be an issue reminding us of the TODO.
  • there should be a new issue about vet's philosophy here and then a discussion about it, to be resolved in 1.12

@gopherbot gopherbot closed this in 240ae7e Jul 23, 2018

blgm added a commit to onsi/gomega that referenced this issue Sep 6, 2018

Work around go vet issue with Go v1.11
- go vet in Go v1.11 identifies `Say()` as a print wrapper
  - go vet will then fail for `Say("%")` because this is not a valid
  Sprintf template
- Because of golang/go#26486, changing the way
  that the functon is written will work around the vet issue
- I have added a comment documenting that this is not ideal in
  golang/go#26555 (comment)

nodo added a commit to onsi/gomega that referenced this issue Sep 10, 2018

Work around go vet issue with Go v1.11 (#300)
- go vet in Go v1.11 identifies `Say()` as a print wrapper
  - go vet will then fail for `Say("%")` because this is not a valid
  Sprintf template
- Because of golang/go#26486, changing the way
  that the functon is written will work around the vet issue
- I have added a comment documenting that this is not ideal in
  golang/go#26555 (comment)
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.