From 25f3e0fbdee99787c0895b8d5d5f537deefe9ee1 Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Mon, 15 Jul 2013 18:37:49 -0400 Subject: [PATCH] go.tools/cmd/vet: improvements to static checking of printf calls. Details: - added support for complex numbers as distinct from floats: %[efg] allows complex; %b does not. - %p: only Signature, Map, Chan, Slice, unsafe.Pointer allowed. - %s: allow []byte. - allow a verb to match map[K]V and []T if it matches K/V/T, e.g. %d now matches []int. i.e. matching is recursive. - use go/types' constant folding. literal() is gone. - group cases together. Added tests. R=gri, r CC=golang-dev https://golang.org/cl/10895043 --- cmd/vet/print.go | 81 +++++----------------- cmd/vet/testdata/print.go | 28 +++++--- cmd/vet/types.go | 138 ++++++++++++++++++++++++-------------- 3 files changed, 124 insertions(+), 123 deletions(-) diff --git a/cmd/vet/print.go b/cmd/vet/print.go index cf318cc09e..b09a5fcb5d 100644 --- a/cmd/vet/print.go +++ b/cmd/vet/print.go @@ -14,6 +14,8 @@ import ( "strconv" "strings" "unicode/utf8" + + "code.google.com/p/go.tools/go/exact" ) var printfuncs = flag.String("printfuncs", "", "comma-separated list of print function names to check") @@ -58,53 +60,6 @@ func (f *File) checkFmtPrintfCall(call *ast.CallExpr, Name string) { } } -// literal returns the literal value represented by the expression, or nil if it is not a literal. -func (f *File) literal(value ast.Expr) *ast.BasicLit { - switch v := value.(type) { - case *ast.BasicLit: - return v - case *ast.ParenExpr: - return f.literal(v.X) - case *ast.BinaryExpr: - if v.Op != token.ADD { - break - } - litX := f.literal(v.X) - litY := f.literal(v.Y) - if litX != nil && litY != nil { - lit := *litX - x, errX := strconv.Unquote(litX.Value) - y, errY := strconv.Unquote(litY.Value) - if errX == nil && errY == nil { - return &ast.BasicLit{ - ValuePos: lit.ValuePos, - Kind: lit.Kind, - Value: strconv.Quote(x + y), - } - } - } - case *ast.Ident: - // See if it's a constant or initial value (we can't tell the difference). - if v.Obj == nil || v.Obj.Decl == nil { - return nil - } - valueSpec, ok := v.Obj.Decl.(*ast.ValueSpec) - if ok && len(valueSpec.Names) == len(valueSpec.Values) { - // Find the index in the list of names - var i int - for i = 0; i < len(valueSpec.Names); i++ { - if valueSpec.Names[i].Name == v.Name { - if lit, ok := valueSpec.Values[i].(*ast.BasicLit); ok { - return lit - } - return nil - } - } - } - } - return nil -} - // formatState holds the parsed representation of a printf directive such as "%3.*[4]d". // It is constructed by parsePrintfVerb. type formatState struct { @@ -127,23 +82,20 @@ type formatState struct { // 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) { + f.Warn(call.Pos(), "too few arguments in call to", name) return } - lit := f.literal(call.Args[formatIndex]) + lit := f.pkg.values[call.Args[formatIndex]] if lit == nil { if *verbose { - f.Warn(call.Pos(), "can't check non-literal format in call to", name) + f.Warn(call.Pos(), "can't check non-constant format in call to", name) } return } - if lit.Kind != token.STRING { - f.Badf(call.Pos(), "literal %v not a string in call to", lit.Value, name) - } - format, err := strconv.Unquote(lit.Value) - if err != nil { - // Shouldn't happen if parser returned no errors, but be safe. - f.Badf(call.Pos(), "invalid quoted string literal") + if lit.Kind() != exact.String { + f.Badf(call.Pos(), "constant %v not a string in call to", lit, name) } + format := exact.StringVal(lit) firstArg := formatIndex + 1 // Arguments are immediately after format string. if !strings.Contains(format, "%") { if len(call.Args) > firstArg { @@ -325,6 +277,7 @@ const ( argRune argString argFloat + argComplex argPointer anyType printfArgType = ^0 ) @@ -356,12 +309,12 @@ var printVerbs = []printVerb{ {'b', numFlag, argInt | argFloat}, {'c', "-", argRune | argInt}, {'d', numFlag, argInt}, - {'e', numFlag, argFloat}, - {'E', numFlag, argFloat}, - {'f', numFlag, argFloat}, - {'F', numFlag, argFloat}, - {'g', numFlag, argFloat}, - {'G', numFlag, argFloat}, + {'e', numFlag, argFloat | argComplex}, + {'E', numFlag, argFloat | argComplex}, + {'f', numFlag, argFloat | argComplex}, + {'F', numFlag, argFloat | argComplex}, + {'g', numFlag, argFloat | argComplex}, + {'G', numFlag, argFloat | argComplex}, {'o', sharpNumFlag, argInt}, {'p', "-#", argPointer}, {'q', " -+.0#", argRune | argInt | argString}, @@ -410,7 +363,7 @@ func (f *File) okPrintfArg(call *ast.CallExpr, state *formatState) (ok bool) { return } arg := call.Args[argNum] - if !f.matchArgType(argInt, arg) { + if !f.matchArgType(argInt, nil, arg) { f.Badf(call.Pos(), "arg %s for * in printf format not of type int", f.gofmt(arg)) return false } @@ -423,7 +376,7 @@ func (f *File) okPrintfArg(call *ast.CallExpr, state *formatState) (ok bool) { return false } arg := call.Args[argNum] - if !f.matchArgType(v.typ, arg) { + if !f.matchArgType(v.typ, nil, arg) { typeString := "" if typ := f.pkg.types[arg]; typ != nil { typeString = typ.String() diff --git a/cmd/vet/testdata/print.go b/cmd/vet/testdata/print.go index fa6655265d..7a1b6714cf 100644 --- a/cmd/vet/testdata/print.go +++ b/cmd/vet/testdata/print.go @@ -53,21 +53,24 @@ func PrintfTests() { var s string var x float64 var p *int + var imap map[int]int + var fslice []float64 + var c complex64 // Some good format/argtypes fmt.Printf("") - fmt.Printf("%b %b", 3, i) + fmt.Printf("%b %b %b", 3, i, x) fmt.Printf("%c %c %c %c", 3, i, 'x', r) - fmt.Printf("%d %d", 3, i) - fmt.Printf("%e %e", 3e9, x) - fmt.Printf("%E %E", 3e9, x) - fmt.Printf("%f %f", 3e9, x) - fmt.Printf("%F %F", 3e9, x) - fmt.Printf("%g %g", 3e9, x) - fmt.Printf("%G %G", 3e9, x) + fmt.Printf("%d %d %d", 3, i, imap) + fmt.Printf("%e %e %e %e", 3e9, x, fslice, c) + fmt.Printf("%E %E %E %E", 3e9, x, fslice, c) + fmt.Printf("%f %f %f %f", 3e9, x, fslice, c) + fmt.Printf("%F %F %F %F", 3e9, x, fslice, c) + fmt.Printf("%g %g %g %g", 3e9, x, fslice, c) + fmt.Printf("%G %G %G %G", 3e9, x, fslice, c) fmt.Printf("%o %o", 3, i) fmt.Printf("%p %p", p, nil) fmt.Printf("%q %q %q %q", 3, i, 'x', r) - fmt.Printf("%s %s", "hi", s) + fmt.Printf("%s %s %s", "hi", s, []byte{65}) fmt.Printf("%t %t", true, b) fmt.Printf("%T %T", 3, i) fmt.Printf("%U %U", 3, i) @@ -78,8 +81,12 @@ func PrintfTests() { fmt.Printf("%s", &stringerv) fmt.Printf("%T", &stringerv) fmt.Printf("%*%", 2) // Ridiculous but allowed. + + fmt.Printf("%g", 1+2i) // Some bad format/argTypes fmt.Printf("%b", "hi") // ERROR "arg .hi. for printf verb %b of wrong type" + fmt.Printf("%b", c) // ERROR "arg c for printf verb %b of wrong type" + fmt.Printf("%b", 1+2i) // ERROR "arg 1 \+ 2i for printf verb %b of wrong type" fmt.Printf("%c", 2.3) // ERROR "arg 2.3 for printf verb %c of wrong type" fmt.Printf("%d", 2.3) // ERROR "arg 2.3 for printf verb %d of wrong type" fmt.Printf("%e", "hi") // ERROR "arg .hi. for printf verb %e of wrong type" @@ -87,15 +94,18 @@ func PrintfTests() { fmt.Printf("%f", "hi") // ERROR "arg .hi. for printf verb %f of wrong type" fmt.Printf("%F", 'x') // ERROR "arg 'x' for printf verb %F of wrong type" fmt.Printf("%g", "hi") // ERROR "arg .hi. for printf verb %g of wrong type" + fmt.Printf("%g", imap) // ERROR "arg imap for printf verb %g of wrong type" fmt.Printf("%G", i) // ERROR "arg i for printf verb %G of wrong type" fmt.Printf("%o", x) // ERROR "arg x for printf verb %o of wrong type" fmt.Printf("%p", 23) // ERROR "arg 23 for printf verb %p of wrong type" fmt.Printf("%q", x) // ERROR "arg x for printf verb %q of wrong type" fmt.Printf("%s", b) // ERROR "arg b for printf verb %s of wrong type" + fmt.Printf("%s", byte(65)) // ERROR "arg byte\(65\) for printf verb %s of wrong type" fmt.Printf("%t", 23) // ERROR "arg 23 for printf verb %t of wrong type" fmt.Printf("%U", x) // ERROR "arg x for printf verb %U of wrong type" fmt.Printf("%x", nil) // ERROR "arg nil for printf verb %x of wrong type" fmt.Printf("%X", 2.3) // ERROR "arg 2.3 for printf verb %X of wrong type" + fmt.Printf("%s", stringerv) // ERROR "arg stringerv for printf verb %s of wrong type" fmt.Printf("%t", stringerv) // ERROR "arg stringerv for printf verb %t of wrong type" fmt.Printf("%.*s %d %g", 3, "hi", 23, 'x') // ERROR "arg 'x' for printf verb %g of wrong type" fmt.Println() // not an error diff --git a/cmd/vet/types.go b/cmd/vet/types.go index 780c658e5d..b58e6e19c5 100644 --- a/cmd/vet/types.go +++ b/cmd/vet/types.go @@ -69,11 +69,21 @@ var ( stringerType = types.New("interface{ String() string }") ) -func (f *File) matchArgType(t printfArgType, arg ast.Expr) bool { - // TODO: for now, we can only test builtin types, untyped constants, and Stringer/Errors. - typ := f.pkg.types[arg] +// matchArgType reports an error if printf verb t is not appropriate +// for operand arg. +// +// typ is used only for recursive calls; external callers must supply nil. +// +// (Recursion arises from the compound types {map,chan,slice} which +// may be printed with %d etc. if that is appropriate for their element +// types.) +func (f *File) matchArgType(t printfArgType, typ types.Type, arg ast.Expr) bool { if typ == nil { - return true + // external call + typ = f.pkg.types[arg] + if typ == nil { + return true // probably a type check problem + } } // If we can use a string, does arg implement the Stringer or Error interface? if t&argString != 0 { @@ -81,48 +91,81 @@ func (f *File) matchArgType(t printfArgType, arg ast.Expr) bool { return true } } - basic, ok := typ.Underlying().(*types.Basic) - if !ok { + + switch typ := typ.Underlying().(type) { + case *types.Signature: + return t&argPointer != 0 + + case *types.Map: + // Recurse: map[int]int matches %d. + return t&argPointer != 0 || + (f.matchArgType(t, typ.Key(), arg) && f.matchArgType(t, typ.Elem(), arg)) + + case *types.Chan: + return t&argPointer != 0 + + case *types.Slice: + if types.IsIdentical(typ.Elem().Underlying(), types.Typ[types.Byte]) && t&argString != 0 { + return true // %s matches []byte + } + // Recurse: []int matches %d. + return t&argPointer != 0 || f.matchArgType(t, typ.Elem(), arg) + + case *types.Basic: + switch typ.Kind() { + case types.UntypedBool, + types.Bool: + return t&argBool != 0 + + case types.UntypedInt, + types.Int, + types.Int8, + types.Int16, + types.Int32, + types.Int64, + types.Uint, + types.Uint8, + types.Uint16, + types.Uint32, + types.Uint64, + types.Uintptr: + return t&argInt != 0 + + case types.UntypedFloat, + types.Float32, + types.Float64: + return t&argFloat != 0 + + case types.UntypedComplex, + types.Complex64, + types.Complex128: + return t&argComplex != 0 + + case types.UntypedString, + types.String: + return t&argString != 0 + + case types.UnsafePointer: + return t&(argPointer|argInt) != 0 + + case types.UntypedRune: + return t&(argInt|argRune) != 0 + + case types.UntypedNil: + return t&argPointer != 0 // TODO? + + case types.Invalid: + if *verbose { + f.Warnf(arg.Pos(), "printf argument %v has invalid or unknown type", f.gofmt(arg)) + } + return true // Probably a type check problem. + } + panic("unreachable") + + default: return true } - switch basic.Kind() { - case types.Bool: - return t&argBool != 0 - case types.Int, types.Int8, types.Int16, types.Int32, types.Int64: - fallthrough - case types.Uint, types.Uint8, types.Uint16, types.Uint32, types.Uint64, types.Uintptr: - return t&argInt != 0 - case types.Float32, types.Float64, types.Complex64, types.Complex128: - return t&argFloat != 0 - case types.String: - return t&argString != 0 - case types.UnsafePointer: - return t&(argPointer|argInt) != 0 - case types.UntypedBool: - return t&argBool != 0 - case types.UntypedComplex: - return t&argFloat != 0 - case types.UntypedFloat: - // If it's integral, we can use an int format. - switch f.pkg.values[arg].Kind() { - case exact.Int: - return t&(argInt|argFloat) != 0 - } - return t&argFloat != 0 - case types.UntypedInt: - return t&argInt != 0 - case types.UntypedRune: - return t&(argInt|argRune) != 0 - case types.UntypedString: - return t&argString != 0 - case types.UntypedNil: - return t&argPointer != 0 // TODO? - case types.Invalid: - if *verbose { - f.Warnf(arg.Pos(), "printf argument %v has invalid or unknown type", arg) - } - return true // Probably a type check problem. - } + return false } @@ -186,10 +229,5 @@ func (f *File) isErrorMethodCall(call *ast.CallExpr) bool { return false } // It must have return type "string" from the universe. - return isUniverseString(sig.Results().At(0).Type()) -} - -// isUniverseString reports whether the type is the predeclared type "string". -func isUniverseString(typ types.Type) bool { - return types.IsIdentical(typ, types.Typ[types.String]) + return sig.Results().At(0).Type() == types.Typ[types.String] }