cmd/vet: improve detecting printf-like format argument

Previously format argument was detected via scanning func type args.
This didn't work when func type couldn't be determined if the func
is declared in the external package. Fall back to scanning for
the first string call argument in this case.

Fixes #14754

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

src/cmd/vet/print.go

@@ -78,32 +78,63 @@ 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 {
+// formatString returns the format string argument and its index within
+// the given printf-like call expression.
+//
+// The last parameter before variadic arguments is assumed to be
+// a format string.
+//
+// The first string literal or string constant is assumed to be a format string
+// if the call's signature cannot be determined.
+//
+// If it cannot find any format string parameter, it returns  ("", -1).
+func formatString(f *File, call *ast.CallExpr) (string, int) {
 	typ := f.pkg.types[call.Fun].Type
-	if typ == nil {
-		return nil
+	if typ != nil {
+		if sig, ok := typ.(*types.Signature); ok {
+			if !sig.Variadic() {
+				// Skip checking non-variadic functions
+				return "", -1
+			}
+			idx := sig.Params().Len() - 2
+			if idx < 0 {
+				// Skip checking variadic functions without
+				// fixed arguments.
+				return "", -1
+			}
+			s, ok := stringLiteralArg(f, call, idx)
+			if !ok {
+				// The last argument before variadic args isn't a string
+				return "", -1
+			}
+			return s, idx
+		}
 	}
-	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
+	// Cannot determine call's signature. Fallback to scanning for the first
+	// string argument in the call
+	for idx := range call.Args {
+		if s, ok := stringLiteralArg(f, call, idx); ok {
+			return s, idx
+		}
 	}
-	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 "", -1
+}
+
+// stringLiteralArg returns call's string constant argument at the index idx.
+//
+// ("", false) is returned if call's argument at the index idx isn't a string
+// literal.
+func stringLiteralArg(f *File, call *ast.CallExpr, idx int) (string, bool) {
+	if idx >= len(call.Args) {
+		return "", false
 	}
+	arg := call.Args[idx]
+	lit := f.pkg.types[arg].Value
+	if lit != nil && lit.Kind() == constant.String {
+		return constant.StringVal(lit), true
 	}
-	return idx
+	return "", false
 }
 
 // checkCall triggers the print-specific checks if the call invokes a print function.
@@ -173,32 +204,15 @@ 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) {
-	idx := formatIndex(signature(f, call))
-
+	format, idx := formatString(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[idx]].Value
-	if lit == nil {
 		if *verbose {
 			f.Warn(call.Pos(), "can't check non-constant format in call to", name)
 		}
 		return
 	}
-	if lit.Kind() != constant.String {
-		f.Badf(call.Pos(), "constant %v not a string in call to %s", lit, name)
-		return
-	}
-	format := constant.StringVal(lit)
+
 	firstArg := idx + 1 // Arguments are immediately after format string.
 	if !strings.Contains(format, "%") {
 		if len(call.Args) > firstArg {

src/cmd/vet/testdata/print.go

@@ -11,6 +11,9 @@ import (
 	"math"
 	"os"
 	"unsafe" // just for test case printing unsafe.Pointer
+
+	// For testing printf-like functions from external package.
+	"github.com/foobar/externalprintf"
 )
 
 func UnsafePointerPrintfTest() {
@@ -215,6 +218,19 @@ func PrintfTests() {
 
 	Errorf(1, "%d", 3)    // OK
 	Errorf(1, "%d", "hi") // ERROR "arg .hi. for printf verb %d of wrong type: string"
+
+	// Multiple string arguments before variadic args
+	errorf("WARNING", "foobar")            // OK
+	errorf("INFO", "s=%s, n=%d", "foo", 1) // OK
+	errorf("ERROR", "%d")                  // ERROR "format reads arg 1, have only 0 args"
+
+	// Printf from external package
+	externalprintf.Printf("%d", 42) // OK
+	externalprintf.Printf("foobar") // OK
+	level := 123
+	externalprintf.Logf(level, "%d", 42)                        // OK
+	externalprintf.Errorf(level, level, "foo %q bar", "foobar") // OK
+	externalprintf.Logf(level, "%d")                            // ERROR "format reads arg 1, have only 0 args"
 }
 
 // A function we use as a function value; it has no other purpose.
@@ -242,6 +258,12 @@ func Errorf(i int, format string, args ...interface{}) {
 	panic("don't call - testing only")
 }
 
+// errorf is used by the test for a case in which the function accepts multiple
+// string parameters before variadic arguments
+func errorf(level, 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")