From: Spencer Nelson Date: Wed, 2 Mar 2016 20:29:30 +0000 (-0500) Subject: cmd/vet: Use function signature to find format string index. X-Git-Tag: go1.7beta1~1569 X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=867910ea170cad67ef929ae472f1e19052da7d7c;p=gostls13.git 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 TryBot-Result: Gobot Gobot Reviewed-by: Rob Pike --- diff --git a/src/cmd/vet/print.go b/src/cmd/vet/print.go index 61139fdc11..5968141417 100644 --- 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 c5faa36e89..d0b5821e77 100644 --- 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")