From 7acdfd2cdbb71a0cc675ed7f0e0544adcba32bab Mon Sep 17 00:00:00 2001 From: Michael Urman Date: Thu, 1 Sep 2022 22:13:14 -0500 Subject: [PATCH 1/3] varfmt: reimplement with SSA --- varfmt/testdata/src/a/a.go | 41 +++-- varfmt/varfmt.go | 312 +++++++++---------------------------- 2 files changed, 97 insertions(+), 256 deletions(-) diff --git a/varfmt/testdata/src/a/a.go b/varfmt/testdata/src/a/a.go index 7048a40..ad9a548 100644 --- a/varfmt/testdata/src/a/a.go +++ b/varfmt/testdata/src/a/a.go @@ -22,26 +22,31 @@ const ( ) func bad() { - // note: plusses, square brackets, other regex chars replaced with . - fmt.Sprintf(v1) // want "variable `v1` used for fmt.Sprintf format parameter" - passthrough(v1 + v2) // want "variable `v1 . v2` used for passthrough format parameter" - fmt.Sprintf(v1 + v2 + v3) // want "variable `v1 . v2 . v3` used for fmt.Sprintf format parameter" - fmt.Sprintf(v1 + c1) // want "variable `v1` used for fmt.Sprintf format parameter" - fmt.Sprintf((v1 + c1)) // want "non-constant expression used for fmt.Sprintf format parameter" - fmt.Sprintf(ar[0]) // want "variable `ar.0.` used for fmt.Sprintf format parameter" - fmt.Sprintf(*pv1) // want "variable `.pv1` used for fmt.Sprintf format parameter" - fmt.Sprintf(v3[:2]) // want "variable `v3` used for fmt.Sprintf format parameter" - fmt.Sprintf(v3[2:]) // want "variable `v3` used for fmt.Sprintf format parameter" - fmt.Sprintf(v3[0:2]) // want "variable `v3` used for fmt.Sprintf format parameter" + passthrough(v1 + v2) // want "variable `v1 \\+ v2` used for passthrough format parameter" + fmt.Sprintf(v1 + v2 + v3) // want "variable `v1 \\+ v2 \\+ v3` used for fmt.Sprintf format parameter" + fmt.Sprintf(v1 + c1) // want "variable `v1 \\+ c1` used for fmt.Sprintf format parameter" + fmt.Sprintf((v1 + c1)) // want "variable `\\(v1 \\+ c1\\)` used for fmt.Sprintf format parameter" + fmt.Sprintf(ar[0]) // want "variable `ar\\[0\\]` used for fmt.Sprintf format parameter" + fmt.Sprintf(*pv1) // want "variable `\\*pv1` used for fmt.Sprintf format parameter" + fmt.Sprintf(v3[:2]) // want "variable `v3\\[:2\\]` used for fmt.Sprintf format parameter" + fmt.Sprintf(v3[2:]) // want "variable `v3\\[2:\\]` used for fmt.Sprintf format parameter" + fmt.Sprintf(v3[0:2]) // want "variable `v3\\[0:2\\]` used for fmt.Sprintf format parameter" fmt.Sprintf(pkg.V) // want "variable `pkg.V` used for fmt.Sprintf format parameter" lv := "lv" + fmt.Sprintf(lv) + if len(os.Args) > 2 { + lv = "lv2" + } + // TODO:should we accept conditional between two constant values? fmt.Sprintf(lv) // want "variable `lv` used for fmt.Sprintf format parameter" lookup := map[bool]string{false: "false", true: "true"} - fmt.Sprintf(lookup[false]) // want "variable `lookup.false.` used for fmt.Sprintf format parameter" + fmt.Sprintf(lookup[false]) // want "variable `lookup\\[false\\]` used for fmt.Sprintf format parameter" + // long literals are replaced with 'non-constant expression' fmt.Sprintf(map[bool]string{false: "false", true: "true"}[false]) // want "non-constant expression used for fmt.Sprintf format parameter" - complicated(v1, v2, v3) // want "variable `v3` used for complicated format parameter" - fmt.Fprintf(os.Stdout, os.Args[0]) // want "variable `os.Args.0.` used for fmt.Fprintf format parameter" + + complicated(v1, v2, v3) // want "variable `v3` used for complicated format parameter" + fmt.Fprintf(os.Stdout, os.Args[0]) // want "variable `os\\.Args\\[0\\]` used for fmt.Fprintf format parameter" } func goodf(format string, args ...interface{}) { @@ -73,9 +78,11 @@ func passthrough(format string, args ...interface{}) { } func modifiedpassthrough(format string, args ...interface{}) { - fmt.Sprintf(format, args...) - format = "blah" - // but not after they modify the format + fmt.Sprintf(format, args...) // don't warn for passthrough format strings + format = c1 + fmt.Sprintf(format, args...) // nor if set to a new constant + format = v1 + // but do warn if modified to a variable fmt.Sprintf(format, args...) // want "variable `format` used for fmt.Sprintf format parameter" } diff --git a/varfmt/varfmt.go b/varfmt/varfmt.go index cfc08ab..b7074d1 100644 --- a/varfmt/varfmt.go +++ b/varfmt/varfmt.go @@ -9,13 +9,13 @@ import ( "go/ast" "go/printer" "go/types" - "reflect" "strings" "golang.org/x/tools/go/analysis" - "golang.org/x/tools/go/analysis/passes/inspect" + "golang.org/x/tools/go/analysis/passes/buildssa" "golang.org/x/tools/go/analysis/passes/printf" - "golang.org/x/tools/go/ast/inspector" + "golang.org/x/tools/go/ast/astutil" + "golang.org/x/tools/go/ssa" ) const doc = `varfmt reports variables passed as format strings in printf-like functions @@ -36,7 +36,7 @@ func Analyzer() *varfmtAnalyzer { Analyzer: &analysis.Analyzer{ Name: "varfmt", Doc: doc, - Requires: []*analysis.Analyzer{inspect.Analyzer, printf.Analyzer}, + Requires: []*analysis.Analyzer{buildssa.Analyzer, printf.Analyzer}, }, } v.Flags.BoolVar(&v.SuppressNoArgs, "no-args", false, "suppress varfmt reports when formatted args are passed") @@ -51,257 +51,91 @@ func Analyzer() *varfmtAnalyzer { } func (v *varfmtAnalyzer) run(pass *analysis.Pass) (interface{}, error) { - pp = func(x ast.Node) string { + pp := func(x ast.Node) string { var sb strings.Builder printer.Fprint(&sb, pass.Fset, x) return sb.String() } - // track objects of parameters that are themselves format strings so as to - // ignore them when their variable is passed a format string. - passthru := make(map[types.Object]struct{}) - - inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) + prog := pass.ResultOf[buildssa.Analyzer].(*buildssa.SSA) printers := pass.ResultOf[printf.Analyzer].(*printf.Result) - callFilter := []ast.Node{ - (*ast.CallExpr)(nil), - (*ast.FuncDecl)(nil), - // (*ast.FuncLit)(nil), // Can we handle a printf-wrapper FuncLit? - (*ast.AssignStmt)(nil), - } - inspect.Preorder(callFilter, func(n ast.Node) { - switch n := n.(type) { - case *ast.FuncDecl: // validate wrapper pass-through - obj := pass.TypesInfo.ObjectOf(n.Name) - if obj != nil { - if i := fmtarg(printers, obj); i >= 0 { - id := nthName(n.Type.Params.List, i) - if id != nil { - obj := pass.TypesInfo.ObjectOf(id) - if obj != nil { - passthru[obj] = struct{}{} - } - } + for _, fn := range prog.SrcFuncs { + if tfun, ok := fn.Object().(*types.Func); ok { + var fmtParam *ssa.Parameter + if printers.Kind(tfun) != printf.KindNone { + if len(fn.Params) >= 2 { + fmtParam = fn.Params[len(fn.Params)-2] } } - case *ast.AssignStmt: // invalidate wrapper pass-through - for _, lh := range n.Lhs { - if id, ok := lh.(*ast.Ident); ok { - obj := pass.TypesInfo.ObjectOf(id) - if obj != nil { - delete(passthru, obj) - } - } - } - case *ast.CallExpr: - checkarg := -1 - switch callfun := n.Fun.(type) { - case *ast.Ident: - obj := pass.TypesInfo.ObjectOf(callfun) - checkarg = fmtarg(printers, obj) - case *ast.SelectorExpr: - obj := selobj(pass.TypesInfo, callfun) - checkarg = fmtarg(printers, obj) - case *ast.CallExpr, *ast.TypeAssertExpr, *ast.ParenExpr, *ast.IndexExpr: - case *ast.FuncLit, *ast.ArrayType, *ast.InterfaceType, *ast.MapType, *ast.ChanType, *ast.StructType: - default: - pt("unhandled callfun "+pp(n.Fun), n.Fun) - return - } - - if checkarg < 0 { - return - } - if v.SuppressNoArgs && len(n.Args)-1 > checkarg { - return - } - - switch arg := n.Args[checkarg].(type) { - case *ast.Ident: - switch obj := pass.TypesInfo.ObjectOf(arg).(type) { - case *types.Var: - // ignore validated wrapper pass-through - if _, ok := passthru[obj]; !ok { - pass.Report(analysis.Diagnostic{ - Pos: arg.Pos(), - End: arg.End(), - Message: fmt.Sprintf("variable `%s` used for %s format parameter", pp(arg), pp(n.Fun)), - Related: []analysis.RelatedInformation{{ - Pos: obj.Pos(), - Message: "defined here", - }}, - }) - } - case *types.Const: - default: - pt("unhandled argid "+pp(arg), obj) - } + for _, blk := range fn.Blocks { + for _, inst := range blk.Instrs { + if call, ok := inst.(ssa.CallInstruction); ok { + com := call.Common() + if com.IsInvoke() { + pass.Reportf(com.Pos(), "invoke") + } else { + callee := com.StaticCallee() + if callee == nil { + continue + } - default: - bad := make(map[ast.Node]struct{}) - if !constlit(pass.TypesInfo, arg, bad) { - // fmt.Printf("%0.20q math formatting contains %v\n", pp(n), bad) - var related []analysis.RelatedInformation - for x := range bad { - related = append(related, analysis.RelatedInformation{ - Pos: x.Pos(), - End: x.End(), - Message: fmt.Sprintf("Non-constant: %s", pp(x)), - }) - } - msg := "non-constant expression" - if len(bad) == 1 { - for x := range bad { - if s := pp(x); len(s) < 32 { - msg = "variable `" + pp(x) + "`" + if tfun, ok := callee.Object().(*types.Func); ok { + if printers.Kind(tfun) == printf.KindNone { + continue + } + if len(com.Args) < 2 { + continue + } + + msg := "non-constant expression" + + fmtarg := com.Args[len(com.Args)-2] + switch v := fmtarg.(type) { + case *ssa.Const: + continue // Constants are fine + case *ssa.Slice: + if _, ok := v.X.(*ssa.Const); ok { + continue // Slices of const strings are ok + } + case *ssa.Parameter: + if v == fmtParam { + continue // pass-through format params are ok. + } + } + + var n ast.Node + for _, f := range pass.Files { + if f.Pos() <= call.Pos() && f.End() >= call.Pos() { + path, _ := astutil.PathEnclosingInterval(f, call.Pos(), call.Pos()) + for _, p := range path { + if c, ok := p.(*ast.CallExpr); ok { + if len(c.Args) < len(com.Args)-2 { + break + } + n = c.Args[len(com.Args)-2] + } + } + } + } + + m := fmt.Sprintf("variable `%s`", pp(n)) + if len(m) < 2*len(msg) { + msg = m + } + + name := tfun.FullName() + if tfun.Pkg() == pass.Pkg { + name = strings.TrimPrefix(name, pass.Pkg.Name()+".") + } + pass.Reportf(call.Pos(), "%s used for %s format parameter", msg, name) } } } - pass.Report(analysis.Diagnostic{ - Pos: arg.Pos(), - End: arg.End(), - Message: fmt.Sprintf("%s used for %s format parameter", msg, pp(n.Fun)), - Related: related, - }) } } } - }) - return nil, nil -} - -func fmtarg(printers *printf.Result, obj types.Object) int { - fun, ok := obj.(*types.Func) - if !ok { - return -1 } - switch printers.Kind(fun) { - case printf.KindPrintf, printf.KindErrorf: - sig := fun.Type().(*types.Signature) - params := sig.Params() - return params.Len() - 2 // assumes func(⋯, format, args...) - default: // includes printf.KindNone, printf.KindPrint - return -1 - } -} - -func constlit(ti *types.Info, x ast.Expr, bad map[ast.Node]struct{}) bool { - switch x := x.(type) { - case *ast.BinaryExpr: - return combine(ti, x, bad, x.X, x.Y) - case *ast.KeyValueExpr: - return combine(ti, x, bad, x.Key, x.Value) - case *ast.ParenExpr: - return combine(ti, x, bad, x.X) - case *ast.SliceExpr: - exprs := append(make([]ast.Expr, 0, 4), x.X) - if x.Low != nil { - exprs = append(exprs, x.Low) - } - if x.High != nil { - exprs = append(exprs, x.High) - } - if x.Max != nil { - exprs = append(exprs, x.Max) - } - return combine(ti, x, bad, exprs...) - case *ast.StarExpr: - return combine(ti, x, bad, x.X) - case *ast.TypeAssertExpr: - return combine(ti, x, bad, x.X) - case *ast.UnaryExpr: - return combine(ti, x, bad, x.X) - case *ast.SelectorExpr: - if o := selobj(ti, x); o != nil && constobj(o) { - return true - } - - case *ast.BasicLit: - return true - case *ast.Ident: - if constobj(ti.ObjectOf(x)) { - return true - } - - case *ast.CallExpr: - if len(x.Args) == 1 && typeexpr(ti, x.Fun) && constlit(ti, x.Args[0], bad) { - return true - } - case *ast.IndexExpr: - - default: - pt("unhandled constlit "+pp(x), x) - } - - bad[x] = struct{}{} - return false -} - -func combine(ti *types.Info, parent ast.Expr, bad map[ast.Node]struct{}, xs ...ast.Expr) bool { - allgood := true - allbad := true - for _, x := range xs { - if constlit(ti, x, bad) { - allbad = false - } else { - allgood = false - } - } - if allbad { - for _, x := range xs { - delete(bad, x) - } - bad[parent] = struct{}{} - } - return allgood -} - -func pt(label string, x interface{}) { - println(label, reflect.TypeOf(x).String()) -} - -var pp func(ast.Node) string - -func constobj(obj types.Object) bool { - _, ok := obj.(*types.Const) - return ok -} - -func typeobj(obj types.Object) bool { - _, ok := obj.(*types.TypeName) - return ok -} - -func typeexpr(ti *types.Info, x ast.Expr) bool { - if sel, ok := x.(*ast.SelectorExpr); ok { - x = sel.Sel - } - if id, ok := x.(*ast.Ident); ok { - return typeobj(ti.ObjectOf(id)) - } - pt("typeexpr "+pp(x), x) - return false -} - -func nthName(flds []*ast.Field, nth int) *ast.Ident { - i := 0 - for _, fld := range flds { - for _, n := range fld.Names { - if i == nth { - return n - } - i++ - } - } - return nil -} - -func selobj(ti *types.Info, x *ast.SelectorExpr) types.Object { - if obj := ti.ObjectOf(x.Sel); obj != nil { - return obj - } - sel := ti.Selections[x] - return sel.Obj() + return nil, nil } From c0d627e7c433529cd52b1aaa8fe0d8a070f1db97 Mon Sep 17 00:00:00 2001 From: Michael Urman Date: Thu, 1 Sep 2022 23:16:47 -0500 Subject: [PATCH 2/3] varfmt: Don't flag print-like functions --- varfmt/varfmt.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/varfmt/varfmt.go b/varfmt/varfmt.go index b7074d1..85562c8 100644 --- a/varfmt/varfmt.go +++ b/varfmt/varfmt.go @@ -63,7 +63,7 @@ func (v *varfmtAnalyzer) run(pass *analysis.Pass) (interface{}, error) { for _, fn := range prog.SrcFuncs { if tfun, ok := fn.Object().(*types.Func); ok { var fmtParam *ssa.Parameter - if printers.Kind(tfun) != printf.KindNone { + if k := printers.Kind(tfun); k != printf.KindNone && k != printf.KindPrint { if len(fn.Params) >= 2 { fmtParam = fn.Params[len(fn.Params)-2] } @@ -73,7 +73,7 @@ func (v *varfmtAnalyzer) run(pass *analysis.Pass) (interface{}, error) { if call, ok := inst.(ssa.CallInstruction); ok { com := call.Common() if com.IsInvoke() { - pass.Reportf(com.Pos(), "invoke") + // pass.Reportf(com.Pos(), "invoke") } else { callee := com.StaticCallee() if callee == nil { @@ -81,7 +81,7 @@ func (v *varfmtAnalyzer) run(pass *analysis.Pass) (interface{}, error) { } if tfun, ok := callee.Object().(*types.Func); ok { - if printers.Kind(tfun) == printf.KindNone { + if k := printers.Kind(tfun); k == printf.KindNone || k == printf.KindPrint { continue } if len(com.Args) < 2 { From a78437e8fc61c147705206c25047160d8be67803 Mon Sep 17 00:00:00 2001 From: Michael Urman Date: Fri, 2 Sep 2022 09:38:27 -0500 Subject: [PATCH 3/3] varfmt: handle interface calls --- varfmt/varfmt.go | 101 +++++++++++++++++++++++------------------------ 1 file changed, 50 insertions(+), 51 deletions(-) diff --git a/varfmt/varfmt.go b/varfmt/varfmt.go index 85562c8..b38fe23 100644 --- a/varfmt/varfmt.go +++ b/varfmt/varfmt.go @@ -18,13 +18,16 @@ import ( "golang.org/x/tools/go/ssa" ) -const doc = `varfmt reports variables passed as format strings in printf-like functions +const ( + maxSourceWidth = 60 + doc = `varfmt reports variables passed as format strings in printf-like functions While this isn't necessarily a problem, and sometimes is very intentional, accidental use of potentially unvetted strings can result in ugly tokens like %!x(MISSING) showing up in the middle of your formatted strings. Varfmt reports all uses of variables as format strings, except those that are merely pass- throughs in a wrapper of a printf-like function.` +) type varfmtAnalyzer struct { *analysis.Analyzer @@ -72,65 +75,61 @@ func (v *varfmtAnalyzer) run(pass *analysis.Pass) (interface{}, error) { for _, inst := range blk.Instrs { if call, ok := inst.(ssa.CallInstruction); ok { com := call.Common() - if com.IsInvoke() { - // pass.Reportf(com.Pos(), "invoke") - } else { - callee := com.StaticCallee() - if callee == nil { - continue + tfun := com.Method + if tfun == nil { + if callee := com.StaticCallee(); callee != nil { + tfun, _ = callee.Object().(*types.Func) } + } + if tfun == nil { + continue + } + if k := printers.Kind(tfun); k == printf.KindNone || k == printf.KindPrint { + continue + } + if len(com.Args) < 2 { + continue + } - if tfun, ok := callee.Object().(*types.Func); ok { - if k := printers.Kind(tfun); k == printf.KindNone || k == printf.KindPrint { - continue - } - if len(com.Args) < 2 { - continue - } - - msg := "non-constant expression" - - fmtarg := com.Args[len(com.Args)-2] - switch v := fmtarg.(type) { - case *ssa.Const: - continue // Constants are fine - case *ssa.Slice: - if _, ok := v.X.(*ssa.Const); ok { - continue // Slices of const strings are ok - } - case *ssa.Parameter: - if v == fmtParam { - continue // pass-through format params are ok. - } - } + fmtarg := com.Args[len(com.Args)-2] + switch v := fmtarg.(type) { + case *ssa.Const: + continue // Constants are fine + case *ssa.Slice: + if _, ok := v.X.(*ssa.Const); ok { + continue // Slices of const strings are ok + } + case *ssa.Parameter: + if v == fmtParam { + continue // pass-through format params are ok. + } + } - var n ast.Node - for _, f := range pass.Files { - if f.Pos() <= call.Pos() && f.End() >= call.Pos() { - path, _ := astutil.PathEnclosingInterval(f, call.Pos(), call.Pos()) - for _, p := range path { - if c, ok := p.(*ast.CallExpr); ok { - if len(c.Args) < len(com.Args)-2 { - break - } - n = c.Args[len(com.Args)-2] - } + var n ast.Node + for _, f := range pass.Files { + if f.Pos() <= call.Pos() && f.End() >= call.Pos() { + path, _ := astutil.PathEnclosingInterval(f, call.Pos(), call.Pos()) + for _, p := range path { + if c, ok := p.(*ast.CallExpr); ok { + if len(c.Args) < len(com.Args)-2 { + break } + n = c.Args[len(com.Args)-2] } } + } + } - m := fmt.Sprintf("variable `%s`", pp(n)) - if len(m) < 2*len(msg) { - msg = m - } + msg := fmt.Sprintf("variable `%s`", pp(n)) + if len(msg) > maxSourceWidth { + msg = "non-constant expression" + } - name := tfun.FullName() - if tfun.Pkg() == pass.Pkg { - name = strings.TrimPrefix(name, pass.Pkg.Name()+".") - } - pass.Reportf(call.Pos(), "%s used for %s format parameter", msg, name) - } + name := tfun.FullName() + if tfun.Pkg() == pass.Pkg { + name = strings.TrimPrefix(name, pass.Pkg.Name()+".") } + pass.Reportf(call.Pos(), "%s used for %s format parameter", msg, name) } } }