112 lines
3.1 KiB
Go
112 lines
3.1 KiB
Go
package sa2001
|
|
|
|
import (
|
|
"go/ast"
|
|
"go/types"
|
|
|
|
"honnef.co/go/tools/analysis/code"
|
|
"honnef.co/go/tools/analysis/lint"
|
|
"honnef.co/go/tools/analysis/report"
|
|
"honnef.co/go/tools/go/ast/astutil"
|
|
|
|
"golang.org/x/tools/go/analysis"
|
|
"golang.org/x/tools/go/analysis/passes/inspect"
|
|
)
|
|
|
|
var SCAnalyzer = lint.InitializeAnalyzer(&lint.Analyzer{
|
|
Analyzer: &analysis.Analyzer{
|
|
Name: "SA2001",
|
|
Run: run,
|
|
Requires: []*analysis.Analyzer{inspect.Analyzer},
|
|
},
|
|
Doc: &lint.RawDocumentation{
|
|
Title: `Empty critical section, did you mean to defer the unlock?`,
|
|
Text: `Empty critical sections of the kind
|
|
|
|
mu.Lock()
|
|
mu.Unlock()
|
|
|
|
are very often a typo, and the following was intended instead:
|
|
|
|
mu.Lock()
|
|
defer mu.Unlock()
|
|
|
|
Do note that sometimes empty critical sections can be useful, as a
|
|
form of signaling to wait on another goroutine. Many times, there are
|
|
simpler ways of achieving the same effect. When that isn't the case,
|
|
the code should be amply commented to avoid confusion. Combining such
|
|
comments with a \'//lint:ignore\' directive can be used to suppress this
|
|
rare false positive.`,
|
|
Since: "2017.1",
|
|
Severity: lint.SeverityWarning,
|
|
MergeIf: lint.MergeIfAny,
|
|
},
|
|
})
|
|
|
|
var Analyzer = SCAnalyzer.Analyzer
|
|
|
|
func run(pass *analysis.Pass) (any, error) {
|
|
if pass.Pkg.Path() == "sync_test" {
|
|
// exception for the sync package's tests
|
|
return nil, nil
|
|
}
|
|
|
|
// Initially it might seem like this check would be easier to
|
|
// implement using IR. After all, we're only checking for two
|
|
// consecutive method calls. In reality, however, there may be any
|
|
// number of other instructions between the lock and unlock, while
|
|
// still constituting an empty critical section. For example,
|
|
// given `m.x().Lock(); m.x().Unlock()`, there will be a call to
|
|
// x(). In the AST-based approach, this has a tiny potential for a
|
|
// false positive (the second call to x might be doing work that
|
|
// is protected by the mutex). In an IR-based approach, however,
|
|
// it would miss a lot of real bugs.
|
|
|
|
mutexParams := func(s ast.Stmt) (x ast.Expr, funcName string, ok bool) {
|
|
expr, ok := s.(*ast.ExprStmt)
|
|
if !ok {
|
|
return nil, "", false
|
|
}
|
|
call, ok := astutil.Unparen(expr.X).(*ast.CallExpr)
|
|
if !ok {
|
|
return nil, "", false
|
|
}
|
|
sel, ok := call.Fun.(*ast.SelectorExpr)
|
|
if !ok {
|
|
return nil, "", false
|
|
}
|
|
|
|
fn, ok := pass.TypesInfo.ObjectOf(sel.Sel).(*types.Func)
|
|
if !ok {
|
|
return nil, "", false
|
|
}
|
|
sig := fn.Type().(*types.Signature)
|
|
if sig.Params().Len() != 0 || sig.Results().Len() != 0 {
|
|
return nil, "", false
|
|
}
|
|
|
|
return sel.X, fn.Name(), true
|
|
}
|
|
|
|
fn := func(node ast.Node) {
|
|
block := node.(*ast.BlockStmt)
|
|
if len(block.List) < 2 {
|
|
return
|
|
}
|
|
for i := range block.List[:len(block.List)-1] {
|
|
sel1, method1, ok1 := mutexParams(block.List[i])
|
|
sel2, method2, ok2 := mutexParams(block.List[i+1])
|
|
|
|
if !ok1 || !ok2 || report.Render(pass, sel1) != report.Render(pass, sel2) {
|
|
continue
|
|
}
|
|
if (method1 == "Lock" && method2 == "Unlock") ||
|
|
(method1 == "RLock" && method2 == "RUnlock") {
|
|
report.Report(pass, block.List[i+1], "empty critical section")
|
|
}
|
|
}
|
|
}
|
|
code.Preorder(pass, fn, (*ast.BlockStmt)(nil))
|
|
return nil, nil
|
|
}
|