sa2001.go raw

   1  package sa2001
   2  
   3  import (
   4  	"go/ast"
   5  	"go/types"
   6  
   7  	"honnef.co/go/tools/analysis/code"
   8  	"honnef.co/go/tools/analysis/lint"
   9  	"honnef.co/go/tools/analysis/report"
  10  	"honnef.co/go/tools/go/ast/astutil"
  11  
  12  	"golang.org/x/tools/go/analysis"
  13  	"golang.org/x/tools/go/analysis/passes/inspect"
  14  )
  15  
  16  var SCAnalyzer = lint.InitializeAnalyzer(&lint.Analyzer{
  17  	Analyzer: &analysis.Analyzer{
  18  		Name:     "SA2001",
  19  		Run:      run,
  20  		Requires: []*analysis.Analyzer{inspect.Analyzer},
  21  	},
  22  	Doc: &lint.RawDocumentation{
  23  		Title: `Empty critical section, did you mean to defer the unlock?`,
  24  		Text: `Empty critical sections of the kind
  25  
  26      mu.Lock()
  27      mu.Unlock()
  28  
  29  are very often a typo, and the following was intended instead:
  30  
  31      mu.Lock()
  32      defer mu.Unlock()
  33  
  34  Do note that sometimes empty critical sections can be useful, as a
  35  form of signaling to wait on another goroutine. Many times, there are
  36  simpler ways of achieving the same effect. When that isn't the case,
  37  the code should be amply commented to avoid confusion. Combining such
  38  comments with a \'//lint:ignore\' directive can be used to suppress this
  39  rare false positive.`,
  40  		Since:    "2017.1",
  41  		Severity: lint.SeverityWarning,
  42  		MergeIf:  lint.MergeIfAny,
  43  	},
  44  })
  45  
  46  var Analyzer = SCAnalyzer.Analyzer
  47  
  48  func run(pass *analysis.Pass) (interface{}, error) {
  49  	if pass.Pkg.Path() == "sync_test" {
  50  		// exception for the sync package's tests
  51  		return nil, nil
  52  	}
  53  
  54  	// Initially it might seem like this check would be easier to
  55  	// implement using IR. After all, we're only checking for two
  56  	// consecutive method calls. In reality, however, there may be any
  57  	// number of other instructions between the lock and unlock, while
  58  	// still constituting an empty critical section. For example,
  59  	// given `m.x().Lock(); m.x().Unlock()`, there will be a call to
  60  	// x(). In the AST-based approach, this has a tiny potential for a
  61  	// false positive (the second call to x might be doing work that
  62  	// is protected by the mutex). In an IR-based approach, however,
  63  	// it would miss a lot of real bugs.
  64  
  65  	mutexParams := func(s ast.Stmt) (x ast.Expr, funcName string, ok bool) {
  66  		expr, ok := s.(*ast.ExprStmt)
  67  		if !ok {
  68  			return nil, "", false
  69  		}
  70  		call, ok := astutil.Unparen(expr.X).(*ast.CallExpr)
  71  		if !ok {
  72  			return nil, "", false
  73  		}
  74  		sel, ok := call.Fun.(*ast.SelectorExpr)
  75  		if !ok {
  76  			return nil, "", false
  77  		}
  78  
  79  		fn, ok := pass.TypesInfo.ObjectOf(sel.Sel).(*types.Func)
  80  		if !ok {
  81  			return nil, "", false
  82  		}
  83  		sig := fn.Type().(*types.Signature)
  84  		if sig.Params().Len() != 0 || sig.Results().Len() != 0 {
  85  			return nil, "", false
  86  		}
  87  
  88  		return sel.X, fn.Name(), true
  89  	}
  90  
  91  	fn := func(node ast.Node) {
  92  		block := node.(*ast.BlockStmt)
  93  		if len(block.List) < 2 {
  94  			return
  95  		}
  96  		for i := range block.List[:len(block.List)-1] {
  97  			sel1, method1, ok1 := mutexParams(block.List[i])
  98  			sel2, method2, ok2 := mutexParams(block.List[i+1])
  99  
 100  			if !ok1 || !ok2 || report.Render(pass, sel1) != report.Render(pass, sel2) {
 101  				continue
 102  			}
 103  			if (method1 == "Lock" && method2 == "Unlock") ||
 104  				(method1 == "RLock" && method2 == "RUnlock") {
 105  				report.Report(pass, block.List[i+1], "empty critical section")
 106  			}
 107  		}
 108  	}
 109  	code.Preorder(pass, fn, (*ast.BlockStmt)(nil))
 110  	return nil, nil
 111  }
 112