Go: cmd/vet: false positive printf-like function detection

[decathorpe]

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:
https://github.com/cznic/ql/issues/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]

/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]

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]

@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]

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

[robpike]

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]

@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]

CC: @alandonovan @josharian @mvdan

[robpike]

@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]

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]

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]

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