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
Closed

cmd/vet: false positive printf-like function detection #26486

decathorpe opened this issue Jul 19, 2018 · 11 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@decathorpe
Copy link

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 ianlancetaylor added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 19, 2018
@ianlancetaylor ianlancetaylor added this to the Go1.11 milestone Jul 19, 2018
@ianlancetaylor
Copy link
Contributor

/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
Copy link
Contributor

cznic 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
Copy link
Contributor

@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
Copy link

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

@robpike
Copy link
Contributor

robpike 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
Copy link
Contributor

@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
Copy link
Contributor

bcmills commented Jul 20, 2018

CC: @alandonovan @josharian @mvdan

@robpike
Copy link
Contributor

robpike 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
Copy link
Contributor

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
Copy link
Member

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

@ianlancetaylor ianlancetaylor added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Jul 20, 2018
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 20, 2018
@robpike
Copy link
Contributor

robpike 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

blgm added a commit to onsi/gomega that referenced this issue Sep 6, 2018
- 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 pushed a commit to onsi/gomega that referenced this issue Sep 10, 2018
- 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)
@golang golang locked and limited conversation to collaborators Jul 23, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

7 participants