From f21f74b8e18245c7ed0ed671a9e33bc1c50725d4 Mon Sep 17 00:00:00 2001 From: chavacava Date: Sat, 16 Apr 2022 13:30:55 +0000 Subject: [PATCH] add rule datarace --- README.md | 1 + RULES_DESCRIPTIONS.md | 6 ++ config/config.go | 1 + rule/datarace.go | 142 ++++++++++++++++++++++++++++++++++++++++++ test/datarace_test.go | 11 ++++ testdata/datarace.go | 30 +++++++++ 6 files changed, 191 insertions(+) create mode 100644 rule/datarace.go create mode 100644 test/datarace_test.go create mode 100644 testdata/datarace.go diff --git a/README.md b/README.md index fded58b06..49f17304d 100644 --- a/README.md +++ b/README.md @@ -482,6 +482,7 @@ List of all available rules. The rules ported from `golint` are left unchanged a | [`banned-characters`](./RULES_DESCRIPTIONS.md#banned-characters) | n/a | Checks banned characters in identifiers | no | no | | [`optimize-operands-order`](./RULES_DESCRIPTIONS.md#optimize-operands-order) | n/a | Checks inefficient conditional expressions | no | no | | [`use-any`](./RULES_DESCRIPTIONS.md#use-any) | n/a | Proposes to replace `interface{}` with its alias `any` | no | no | +| [`datarace`](./RULES_DESCRIPTIONS.md#datarace) | n/a | Spots potential dataraces | no | no | ## Configurable rules diff --git a/RULES_DESCRIPTIONS.md b/RULES_DESCRIPTIONS.md index a788e4a69..b60e35ecd 100644 --- a/RULES_DESCRIPTIONS.md +++ b/RULES_DESCRIPTIONS.md @@ -19,6 +19,7 @@ List of all available rules. - [context-as-argument](#context-as-argument) - [context-keys-type](#context-keys-type) - [cyclomatic](#cyclomatic) + - [datarace](#datarace) - [deep-exit](#deep-exit) - [defer](#defer) - [dot-imports](#dot-imports) @@ -217,6 +218,11 @@ Example: [rule.cyclomatic] arguments =[3] ``` +## datarace + +_Description_: This rule spots potential dataraces caused by go-routines capturing (by-reference) particular identifiers of the function from which go-routines are created. The rule is able to spot two of such cases: go-routines capturing named return values, and capturing `for-range` values. + +_Configuration_: N/A ## deep-exit diff --git a/config/config.go b/config/config.go index 388517a30..e959707d5 100644 --- a/config/config.go +++ b/config/config.go @@ -85,6 +85,7 @@ var allRules = append([]lint.Rule{ &rule.BannedCharsRule{}, &rule.OptimizeOperandsOrderRule{}, &rule.UseAnyRule{}, + &rule.DataRaceRule{}, }, defaultRules...) var allFormatters = []lint.Formatter{ diff --git a/rule/datarace.go b/rule/datarace.go new file mode 100644 index 000000000..26fcadcdc --- /dev/null +++ b/rule/datarace.go @@ -0,0 +1,142 @@ +package rule + +import ( + "fmt" + "go/ast" + + "github.com/mgechev/revive/lint" +) + +// DataRaceRule lints assignments to value method-receivers. +type DataRaceRule struct{} + +// Apply applies the rule to given file. +func (*DataRaceRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure { + var failures []lint.Failure + onFailure := func(failure lint.Failure) { + failures = append(failures, failure) + } + w := lintDataRaces{onFailure: onFailure} + + ast.Walk(w, file.AST) + + return failures +} + +// Name returns the rule name. +func (*DataRaceRule) Name() string { + return "datarace" +} + +type lintDataRaces struct { + onFailure func(failure lint.Failure) +} + +func (w lintDataRaces) Visit(n ast.Node) ast.Visitor { + node, ok := n.(*ast.FuncDecl) + if !ok { + return w // not function declaration + } + if node.Body == nil { + return nil // empty body + } + + results := node.Type.Results + + returnIDs := map[*ast.Object]struct{}{} + if results != nil { + returnIDs = w.ExtractReturnIDs(results.List) + } + fl := &lintFunctionForDataRaces{onFailure: w.onFailure, returnIDs: returnIDs, rangeIDs: map[*ast.Object]struct{}{}} + ast.Walk(fl, node.Body) + + return nil +} + +func (w lintDataRaces) ExtractReturnIDs(fields []*ast.Field) map[*ast.Object]struct{} { + r := map[*ast.Object]struct{}{} + for _, f := range fields { + for _, id := range f.Names { + r[id.Obj] = struct{}{} + } + } + + return r +} + +type lintFunctionForDataRaces struct { + _ struct{} + onFailure func(failure lint.Failure) + returnIDs map[*ast.Object]struct{} + rangeIDs map[*ast.Object]struct{} +} + +func (w lintFunctionForDataRaces) Visit(node ast.Node) ast.Visitor { + switch n := node.(type) { + case *ast.RangeStmt: + if n.Body == nil { + return nil + } + + getIds := func(exprs ...ast.Expr) []*ast.Ident { + r := []*ast.Ident{} + for _, expr := range exprs { + if id, ok := expr.(*ast.Ident); ok { + r = append(r, id) + } + } + return r + } + + ids := getIds(n.Key, n.Value) + for _, id := range ids { + w.rangeIDs[id.Obj] = struct{}{} + } + + ast.Walk(w, n.Body) + + for _, id := range ids { + delete(w.rangeIDs, id.Obj) + } + + return nil // do not visit the body of the range, it has been already visited + case *ast.GoStmt: + f := n.Call.Fun + funcLit, ok := f.(*ast.FuncLit) + if !ok { + return nil + } + selectIDs := func(n ast.Node) bool { + _, ok := n.(*ast.Ident) + return ok + } + + ids := pick(funcLit.Body, selectIDs, nil) + for _, id := range ids { + id := id.(*ast.Ident) + _, isRangeID := w.rangeIDs[id.Obj] + _, isReturnID := w.returnIDs[id.Obj] + + switch { + case isRangeID: + w.onFailure(lint.Failure{ + Confidence: 1, + Node: id, + Category: "logic", + Failure: fmt.Sprintf("datarace: range value %s is captured (by-reference) in goroutine", id.Name), + }) + case isReturnID: + w.onFailure(lint.Failure{ + Confidence: 0.8, + Node: id, + Category: "logic", + Failure: fmt.Sprintf("potential datarace: return value %s is captured (by-reference) in goroutine", id.Name), + }) + } + } + + return nil + } + + return w +} diff --git a/test/datarace_test.go b/test/datarace_test.go new file mode 100644 index 000000000..c7bd6bfc4 --- /dev/null +++ b/test/datarace_test.go @@ -0,0 +1,11 @@ +package test + +import ( + "testing" + + "github.com/mgechev/revive/rule" +) + +func TestDatarace(t *testing.T) { + testRule(t, "datarace", &rule.DataRaceRule{}) +} diff --git a/testdata/datarace.go b/testdata/datarace.go new file mode 100644 index 000000000..37e94a3f6 --- /dev/null +++ b/testdata/datarace.go @@ -0,0 +1,30 @@ +package fixtures + +func datarace() (r int, c char) { + for _, p := range []int{1, 2} { + go func() { + print(r) // MATCH /potential datarace: return value r is captured (by-reference) in goroutine/ + print(p) // MATCH /datarace: range value p is captured (by-reference) in goroutine/ + }() + for i, p1 := range []int{1, 2} { + a := p1 + go func() { + print(r) // MATCH /potential datarace: return value r is captured (by-reference) in goroutine/ + print(p) // MATCH /datarace: range value p is captured (by-reference) in goroutine/ + print(p1) // MATCH /datarace: range value p1 is captured (by-reference) in goroutine/ + print(a) + print(i) // MATCH /datarace: range value i is captured (by-reference) in goroutine/ + }() + print(i) + print(p) + go func() { + _ = c // MATCH /potential datarace: return value c is captured (by-reference) in goroutine/ + }() + } + print(p1) + } + go func() { + print(r) // MATCH /potential datarace: return value r is captured (by-reference) in goroutine/ + }() + print(r) +}