cmd/vet: Use function signature to find format string index.

cmd/vet's printf checker currently uses a hardcoded map of function
names to expected positions of format strings. We can be a bit more
precise than this by looking up the signature of the function, which
helps when libraries implement functions like Errorf or Logf with
extra arguments like log levels or error codes.

Specifically, the format string param is assumed to be the last string
parameter of the called function.

Fixes #12294.

Change-Id: Icf10ebb819bba91fa1c4109301417042901e34c7
Reviewed-on: https://go-review.googlesource.com/20163
Run-TryBot: Rob Pike <r@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rob Pike <r@golang.org>

diff --git a/src/cmd/vet/print.go b/src/cmd/vet/print.go
index 61139fdc115e0b00031c07319568850d54348cdb..596814141733114d7f2efa9586880fb184814f92 100644 (file)
--- a/src/cmd/vet/print.go
+++ b/src/cmd/vet/print.go
@@ -46,24 +46,23 @@ func initPrintFlags() {
                }
                name = strings.ToLower(name)
                if name[len(name)-1] == 'f' {
-                       printfList[name] = skip
+                       isFormattedPrint[name] = true
                } else {
                        printList[name] = skip
                }
        }
 }
 
-// printfList records the formatted-print functions. The value is the location
-// of the format parameter. Names are lower-cased so the lookup is
-// case insensitive.
-var printfList = map[string]int{
-       "errorf":  0,
-       "fatalf":  0,
-       "fprintf": 1,
-       "logf":    0,
-       "panicf":  0,
-       "printf":  0,
-       "sprintf": 0,
+// isFormattedPrint records the formatted-print functions. Names are
+// lower-cased so the lookup is case insensitive.
+var isFormattedPrint = map[string]bool{
+       "errorf":  true,
+       "fatalf":  true,
+       "fprintf": true,
+       "logf":    true,
+       "panicf":  true,
+       "printf":  true,
+       "sprintf": true,
 }
 
 // printList records the unformatted-print functions. The value is the location
@@ -79,6 +78,34 @@ var printList = map[string]int{
        "sprint": 0, "sprintln": 0,
 }
 
+// signature returns the types.Signature of a call. If it is unable to
+// identify the call's signature, it can return nil.
+func signature(f *File, call *ast.CallExpr) *types.Signature {
+       typ := f.pkg.types[call.Fun].Type
+       if typ == nil {
+               return nil
+       }
+       sig, _ := typ.(*types.Signature)
+       return sig
+}
+
+// formatIndex returns the index of the format string parameter within
+// a signature. If it cannot find any format string parameter, it
+// returns -1.
+func formatIndex(sig *types.Signature) int {
+       if sig == nil {
+               return -1
+       }
+       idx := -1
+       for i := 0; i < sig.Params().Len(); i++ {
+               p := sig.Params().At(i)
+               if typ, ok := p.Type().(*types.Basic); ok && typ.Kind() == types.String {
+                       idx = i
+               }
+       }
+       return idx
+}
+
 // checkCall triggers the print-specific checks if the call invokes a print function.
 func checkFmtPrintfCall(f *File, node ast.Node) {
        if d, ok := node.(*ast.FuncDecl); ok && isStringer(f, d) {
@@ -109,8 +136,8 @@ func checkFmtPrintfCall(f *File, node ast.Node) {
        }
 
        name := strings.ToLower(Name)
-       if skip, ok := printfList[name]; ok {
-               f.checkPrintf(call, Name, skip)
+       if _, ok := isFormattedPrint[name]; ok {
+               f.checkPrintf(call, Name)
                return
        }
        if skip, ok := printList[name]; ok {
@@ -147,12 +174,20 @@ type formatState struct {
 
 // checkPrintf checks a call to a formatted print routine such as Printf.
 // call.Args[formatIndex] is (well, should be) the format argument.
-func (f *File) checkPrintf(call *ast.CallExpr, name string, formatIndex int) {
-       if formatIndex >= len(call.Args) {
+func (f *File) checkPrintf(call *ast.CallExpr, name string) {
+       idx := formatIndex(signature(f, call))
+
+       if idx < 0 {
+               f.Badf(call.Pos(), "no formatting directive in %s call", name)
+               return
+       }
+
+       if idx >= len(call.Args) {
                f.Bad(call.Pos(), "too few arguments in call to", name)
                return
        }
-       lit := f.pkg.types[call.Args[formatIndex]].Value
+
+       lit := f.pkg.types[call.Args[idx]].Value
        if lit == nil {
                if *verbose {
                        f.Warn(call.Pos(), "can't check non-constant format in call to", name)
@@ -164,7 +199,7 @@ func (f *File) checkPrintf(call *ast.CallExpr, name string, formatIndex int) {
                return
        }
        format := constant.StringVal(lit)
-       firstArg := formatIndex + 1 // Arguments are immediately after format string.
+       firstArg := idx + 1 // Arguments are immediately after format string.
        if !strings.Contains(format, "%") {
                if len(call.Args) > firstArg {
                        f.Badf(call.Pos(), "no formatting directive in %s call", name)

diff --git a/src/cmd/vet/testdata/print.go b/src/cmd/vet/testdata/print.go
index c5faa36e897a957d57e237b4fea8113ecdc89382..d0b5821e7742e1a93055672f1850a16ae871ef3b 100644 (file)
--- a/src/cmd/vet/testdata/print.go
+++ b/src/cmd/vet/testdata/print.go
@@ -211,8 +211,10 @@ func PrintfTests() {
        Log(3)       // OK
        Log("%d", 3) // ERROR "possible formatting directive in Log call"
        Logf("%d", 3)
-       Logf("%d", "hi") // ERROR "arg .hi. for printf verb %d of wrong type: untyped string"
+       Logf("%d", "hi") // ERROR "arg .hi. for printf verb %d of wrong type: string"
 
+       Errorf(1, "%d", 3)    // OK
+       Errorf(1, "%d", "hi") // ERROR "arg .hi. for printf verb %d of wrong type: string"
 }
 
 // A function we use as a function value; it has no other purpose.
@@ -224,11 +226,22 @@ func Printf(format string, args ...interface{}) {
        panic("don't call - testing only")
 }
 
+// Logf is used by the test so we must declare it.
+func Logf(format string, args ...interface{}) {
+       panic("don't call - testing only")
+}
+
 // printf is used by the test so we must declare it.
 func printf(format string, args ...interface{}) {
        panic("don't call - testing only")
 }
 
+// Errorf is used by the test for a case in which the first parameter
+// is not a format string.
+func Errorf(i int, format string, args ...interface{}) {
+       panic("don't call - testing only")
+}
+
 // multi is used by the test.
 func multi() []interface{} {
        panic("don't call - testing only")