Permalink
Browse files

Flag discarded results of pure functions

This check uses a combination of known pure functions and very basic
detection of simple pure functions.

Pureness is the third attribute a function can have (termination and VRP
are the other two). To keep all attributes of a function in one place we
introduce a function description mapping.

Closes gh-44
  • Loading branch information...
1 parent 539f279 commit 9bf66c2333eb33c36eb03c2be4360a2569998d52 @dominikh committed Dec 2, 2016
View
@@ -88,6 +88,7 @@ The following things are currently checked by staticcheck:
| SA4014 | An if/else if chain has repeated conditions and no side-effects; if the condition didn't match the first time, it won't match the second time, either |
| SA4015 | Calling functions like math.Ceil on floats converted from integers doesn't do anything useful |
| SA4016 | Certain bitwise operations, such as `x ^ 0`, do not do anything useful |
+| SA4017 | A pure function's return value is discarded, making the call pointless |
| | |
| **SA5???** | **Correctness issues** |
| SA5000 | Assignment to nil map |
View
@@ -19,14 +19,75 @@ import (
"honnef.co/go/lint"
"honnef.co/go/ssa"
+ "honnef.co/go/staticcheck/pure"
"honnef.co/go/staticcheck/vrp"
"golang.org/x/tools/go/ast/astutil"
)
+type Function struct {
+ // The function is known to be pure
+ Pure bool
+ // The function is known to never return (panics notwithstanding)
+ Infinite bool
+ // Variable ranges
+ Ranges vrp.Ranges
+}
+
+func (fn Function) Merge(other Function) Function {
+ r := fn.Ranges
+ if r == nil {
+ r = other.Ranges
+ }
+ return Function{
+ Pure: fn.Pure || other.Pure,
+ Infinite: fn.Infinite || other.Infinite,
+ Ranges: r,
+ }
+}
+
+var stdlibDescs = map[string]Function{
+ "strings.Map": Function{Pure: true},
+ "strings.Repeat": Function{Pure: true},
+ "strings.Replace": Function{Pure: true},
+ "strings.Title": Function{Pure: true},
+ "strings.ToLower": Function{Pure: true},
+ "strings.ToLowerSpecial": Function{Pure: true},
+ "strings.ToTitle": Function{Pure: true},
+ "strings.ToTitleSpecial": Function{Pure: true},
+ "strings.ToUpper": Function{Pure: true},
+ "strings.ToUpperSpecial": Function{Pure: true},
+ "strings.Trim": Function{Pure: true},
+ "strings.TrimFunc": Function{Pure: true},
+ "strings.TrimLeft": Function{Pure: true},
+ "strings.TrimLeftFunc": Function{Pure: true},
+ "strings.TrimPrefix": Function{Pure: true},
+ "strings.TrimRight": Function{Pure: true},
+ "strings.TrimRightFunc": Function{Pure: true},
+ "strings.TrimSpace": Function{Pure: true},
+ "strings.TrimSuffix": Function{Pure: true},
+}
+
+type FunctionDescriptions map[string]Function
+
+func (d FunctionDescriptions) Get(fn *ssa.Function) Function {
+ obj, ok := fn.Object().(*types.Func)
+ if !ok {
+ return Function{}
+ }
+ return d[obj.FullName()]
+}
+
+func (d FunctionDescriptions) Merge(fn *ssa.Function, desc Function) {
+ obj, ok := fn.Object().(*types.Func)
+ if !ok {
+ return
+ }
+ d[obj.FullName()] = d[obj.FullName()].Merge(desc)
+}
+
type Checker struct {
- terminatesCache map[*ssa.Function]bool
- ranges map[*ssa.Function]vrp.Ranges
+ funcDescs FunctionDescriptions
}
func NewChecker() *Checker {
@@ -81,6 +142,7 @@ func (c *Checker) Funcs() map[string]lint.Func {
"SA4014": c.CheckRepeatedIfElse,
"SA4015": c.CheckMathInt,
"SA4016": c.CheckSillyBitwiseOps,
+ "SA4017": c.CheckPureFunctions,
"SA5000": c.CheckNilMaps,
"SA5001": c.CheckEarlyDefer,
@@ -97,9 +159,33 @@ func (c *Checker) Funcs() map[string]lint.Func {
}
}
+// terminates reports whether fn is supposed to return, that is if it
+// has at least one theoretic path that returns from the function.
+// Explicit panics do not count as terminating.
+func terminates(fn *ssa.Function) (ret bool) {
+ if fn.Blocks == nil {
+ // assuming that a function terminates is the conservative
+ // choice
+ return true
+ }
+
+ for _, block := range fn.Blocks {
+ if len(block.Instrs) == 0 {
+ continue
+ }
+ if _, ok := block.Instrs[len(block.Instrs)-1].(*ssa.Return); ok {
+ return true
+ }
+ }
+ return false
+}
+
func (c *Checker) Init(prog *lint.Program) {
- c.terminatesCache = map[*ssa.Function]bool{}
- c.ranges = map[*ssa.Function]vrp.Ranges{}
+ c.funcDescs = FunctionDescriptions{}
+
+ for fn, desc := range stdlibDescs {
+ c.funcDescs[fn] = desc
+ }
var fns []*ssa.Function
for _, pkg := range prog.Packages {
@@ -125,15 +211,19 @@ func (c *Checker) Init(prog *lint.Program) {
}
}
+ s := pure.State{}
for _, fn := range fns {
if fn.Blocks == nil {
continue
}
detectInfiniteLoops(fn)
ssa.OptimizeBlocks(fn)
- g := vrp.BuildGraph(fn)
- c.ranges[fn] = g.Solve()
+ c.funcDescs.Merge(fn, Function{
+ Pure: s.IsPure(fn),
+ Ranges: vrp.BuildGraph(fn).Solve(),
+ Infinite: !terminates(fn),
+ })
}
}
@@ -223,34 +313,6 @@ func hasType(f *lint.File, expr ast.Expr, name string) bool {
return types.TypeString(f.Pkg.TypesInfo.TypeOf(expr), nil) == name
}
-// terminates reports whether fn is supposed to return, that is if it
-// has at least one theoretic path that returns from the function.
-// Explicit panics do not count as terminating.
-func (c *Checker) terminates(fn *ssa.Function) (ret bool) {
- if b, ok := c.terminatesCache[fn]; ok {
- return b
- }
- defer func() {
- c.terminatesCache[fn] = ret
- }()
- if fn.Blocks == nil {
- // assuming that a function terminates is the conservative
- // choice
- return true
- }
-
- // Detect ranging over a time.Tick channel
- for _, block := range fn.Blocks {
- if len(block.Instrs) == 0 {
- continue
- }
- if _, ok := block.Instrs[len(block.Instrs)-1].(*ssa.Return); ok {
- return true
- }
- }
- return false
-}
-
func (c *Checker) CheckUntrappableSignal(f *lint.File) {
fn := func(node ast.Node) bool {
call, ok := node.(*ast.CallExpr)
@@ -1049,8 +1111,9 @@ func (c *Checker) CheckDiffSizeComparison(f *lint.File) {
if !ok {
return true
}
- r1, ok1 := c.ranges[ssafn].Get(binop.X).(vrp.StringInterval)
- r2, ok2 := c.ranges[ssafn].Get(binop.Y).(vrp.StringInterval)
+ r := c.funcDescs.Get(ssafn).Ranges
+ r1, ok1 := r.Get(binop.X).(vrp.StringInterval)
+ r2, ok2 := r.Get(binop.Y).(vrp.StringInterval)
if !ok1 || !ok2 {
return true
}
@@ -2310,7 +2373,7 @@ func (c *Checker) CheckLeakyTimeTick(f *lint.File) {
if ssafn == nil {
return false
}
- if !c.terminates(ssafn) {
+ if c.funcDescs.Get(ssafn).Infinite {
return true
}
f.Errorf(node, "using time.Tick leaks the underlying ticker, consider using it only in endless functions, tests and the main package, and use time.NewTicker here")
@@ -2414,7 +2477,7 @@ func (c *Checker) CheckUnbufferedSignalChan(f *lint.File) {
if arg == nil {
return true
}
- r, ok := c.ranges[ssafn][arg].(vrp.ChannelInterval)
+ r, ok := c.funcDescs.Get(ssafn).Ranges[arg].(vrp.ChannelInterval)
if !ok || !r.IsKnown() {
return true
}
@@ -2563,3 +2626,43 @@ func (c *Checker) CheckStringsReplaceZero(f *lint.File) {
}
f.Walk(fn)
}
+
+func (c *Checker) CheckPureFunctions(f *lint.File) {
+ fn := func(node ast.Node) bool {
+ fn, ok := node.(*ast.FuncDecl)
+ if !ok {
+ return true
+ }
+ ssafn := f.EnclosingSSAFunction(fn)
+ if ssafn == nil {
+ return true
+ }
+ for _, b := range ssafn.Blocks {
+ for _, ins := range b.Instrs {
+ ins, ok := ins.(*ssa.Call)
+ if !ok {
+ continue
+ }
+ refs := ins.Referrers()
+ if refs == nil || len(filterDebug(*refs)) > 0 {
+ continue
+ }
+ callee := ins.Common().StaticCallee()
+ if callee == nil {
+ continue
+ }
+ obj, ok := callee.Object().(*types.Func)
+ if !ok {
+ continue
+ }
+ name := obj.FullName()
+ if c.funcDescs.Get(callee).Pure {
+ f.Errorf(ins, "%s is a pure function but its return value is ignored", name)
+ continue
+ }
+ }
+ }
+ return true
+ }
+ f.Walk(fn)
+}
View
@@ -0,0 +1,94 @@
+// Package pure analyzes functions for purity.
+//
+// It currently does a (very) conservative analysis, forbidding most
+// language constructs. It's in turn only able to detect simple pure
+// functions.
+//
+// The analysis may be improved in the future.
+package pure
+
+import (
+ "go/token"
+ "go/types"
+
+ "honnef.co/go/ssa"
+)
+
+type State struct {
+ purity map[*ssa.Function]bool
+}
+
+func (s *State) IsPure(fn *ssa.Function) (ret bool) {
+ if s.purity == nil {
+ s.purity = map[*ssa.Function]bool{}
+ }
+ if p, ok := s.purity[fn]; ok {
+ return p
+ }
+ // stop infinite recursion by considering recursive calls unpure.
+ s.purity[fn] = false
+ defer func() {
+ s.purity[fn] = ret
+ }()
+
+ for _, param := range fn.Params {
+ if _, ok := param.Type().Underlying().(*types.Basic); !ok {
+ return false
+ }
+ }
+
+ if fn.Blocks == nil {
+ return false
+ }
+ checkCall := func(c *ssa.CallCommon) bool {
+ if c.IsInvoke() {
+ return false
+ }
+ builtin, ok := c.Value.(*ssa.Builtin)
+ if !ok {
+ if c.StaticCallee() != fn {
+ if c.StaticCallee() == nil || !s.IsPure(c.StaticCallee()) {
+ return false
+ }
+ }
+ } else {
+ switch builtin.Name() {
+ case "len", "cap", "make", "new":
+ default:
+ return false
+ }
+ }
+ return true
+ }
+ for _, b := range fn.Blocks {
+ for _, ins := range b.Instrs {
+ switch ins := ins.(type) {
+ case *ssa.Call:
+ if !checkCall(ins.Common()) {
+ return false
+ }
+ case *ssa.Defer:
+ if !checkCall(&ins.Call) {
+ return false
+ }
+ case *ssa.Select:
+ return false
+ case *ssa.Send:
+ return false
+ case *ssa.Go:
+ return false
+ case *ssa.Panic:
+ return false
+ case *ssa.Store:
+ return false
+ case *ssa.FieldAddr:
+ return false
+ case *ssa.UnOp:
+ if ins.Op == token.MUL || ins.Op == token.AND {
+ return false
+ }
+ }
+ }
+ }
+ return true
+}
@@ -0,0 +1,15 @@
+package pkg
+
+import "strings"
+
+func fn() {
+ strings.Replace("", "", "", 1) // MATCH /is a pure function but its return value is ignored/
+ foo(1, 2) // MATCH /is a pure function but its return value is ignored/
+ bar(1, 2)
+}
+
+func foo(a, b int) int { return a + b }
+func bar(a, b int) int {
+ println(a + b)
+ return a + b
+}
@@ -20,7 +20,9 @@ func fn2(b1, b2 bool, ch chan string) {
func fn3() {
if gen() {
+ println()
} else if gen() {
+ println()
}
}
@@ -49,5 +49,7 @@ func fn9() {
s[0] = 1
}
-func fn(int) {}
+func fn(int) {
+ println() // make it unpure
+}
func ptr(*[]int) {}
Oops, something went wrong.

0 comments on commit 9bf66c2

Please sign in to comment.