Repository: mgechev/revive (go)

Issue:
rule warning on defer gotchas
It could be useful to have a rule that warns on [`defer` gotchas](https://blog.learngoprogramming.com/5-gotchas-of-defer-in-go-golang-part-iii-36a1ab3d6ef1) 

Pull request description:
Defer rule
Closes #429 


Patch:
diff --git a/README.md b/README.md
index 882dec3..fe9ceac 100644
--- a/README.md
+++ b/README.md
@@ -353,6 +353,7 @@ List of all available rules. The rules ported from `golint` are left unchanged a
 | [`early-return`](./RULES_DESCRIPTIONS.md#early-return)          |  n/a   | Spots if-then-else statements that can be refactored to simplify code reading            |    no    |  no   |
 | [`unconditional-recursion`](./RULES_DESCRIPTIONS.md#unconditional-recursion)          |  n/a   | Warns on function calls that will lead to (direct) infinite recursion |    no    |  no   |
 | [`identical-branches`](./RULES_DESCRIPTIONS.md#identical-branches)          |  n/a   | Spots if-then-else statements with identical `then` and `else` branches       |    no    |  no   |
+| [`defer`](./RULES_DESCRIPTIONS.md#defer)          |  map   |  Warns on some [defer gotchas](https://blog.learngoprogramming.com/5-gotchas-of-defer-in-go-golang-part-iii-36a1ab3d6ef1)       |    no    |  no   |
 
 ## Configurable rules
 
diff --git a/RULES_DESCRIPTIONS.md b/RULES_DESCRIPTIONS.md
index afdcd18..62fd2a5 100644
--- a/RULES_DESCRIPTIONS.md
+++ b/RULES_DESCRIPTIONS.md
@@ -19,6 +19,7 @@ List of all available rules.
   - [context-keys-type](#context-keys-type)
   - [cyclomatic](#cyclomatic)
   - [deep-exit](#deep-exit)
+  - [defer](#defer)
   - [dot-imports](#dot-imports)
   - [duplicated-imports](#duplicated-imports)
   - [early-return](#early-return)
@@ -192,6 +193,26 @@ _Description_: Packages exposing functions that can stop program execution by ex
 
 _Configuration_: N/A
 
+## defer
+
+_Description_: This rule warns on some common mistakes when using `defer` statement. It currently alerts on the following situations:
+| name | description |
+|------|-------------|
+| call-chain| even if deferring call-chains of the form `foo()()` is valid, it does not helps code understanding (only the last call is deferred)|
+|loop  | deferring inside loops can be misleading (deferred functions are not executed at the end of the loop iteration but of the current function) and it could lead to exhausting the execution stack |
+| method-call| deferring a call to a method can lead to subtle bugs if the method does not have a pointer receiver|
+| recover | calling `recover` outside a deferred function has no effect|
+| return | returning values form a deferred function has no effect|
+
+These gotchas are described [here](https://blog.learngoprogramming.com/gotchas-of-defer-in-go-1-8d070894cb01)
+
+_Configuration_: by default all warnings are enabled but it is possible selectively enable them through configuration. For example to enable only `call-chain` and `loop`:
+
+```toml
+[rule.defer]
+  arguments=[["call-chain","loop"]]
+```
+
 ## dot-imports
 
 _Description_: Importing with `.` makes the programs much harder to understand because it is unclear whether names belong to the current package or to an imported package.
diff --git a/config.go b/config.go
index cf1a175..65f5bf9 100644
--- a/config.go
+++ b/config.go
@@ -87,6 +87,7 @@ var allRules = append([]lint.Rule{
 	&rule.EarlyReturnRule{},
 	&rule.UnconditionalRecursionRule{},
 	&rule.IdenticalBranchesRule{},
+	&rule.DeferRule{},
 }, defaultRules...)
 
 var allFormatters = []lint.Formatter{
diff --git a/go.mod b/go.mod
index e1942f8..1b46858 100644
--- a/go.mod
+++ b/go.mod
@@ -10,5 +10,5 @@ require (
 	github.com/mitchellh/go-homedir v1.1.0
 	github.com/olekukonko/tablewriter v0.0.4
 	github.com/pkg/errors v0.9.1
-	golang.org/x/tools v0.0.0-20200603170713-0310561d584d
+	golang.org/x/tools v0.0.0-20200606014950-c42cb6316fb6
 )
diff --git a/go.sum b/go.sum
index 8eecb36..af74fc9 100644
--- a/go.sum
+++ b/go.sum
@@ -72,6 +72,8 @@ golang.org/x/tools v0.0.0-20200601175630-2caf76543d99 h1:deddXmhOJb/bvD/4M/j2AUM
 golang.org/x/tools v0.0.0-20200601175630-2caf76543d99/go.mod h1:EkVYQZoAsY45+roYkvgYkIh4xh/qjgUK9TdY2XT94GE=
 golang.org/x/tools v0.0.0-20200603170713-0310561d584d h1:uHd7B+jdDraEUYC/fseWgiZgUT1x2uD43ii19+bZYb4=
 golang.org/x/tools v0.0.0-20200603170713-0310561d584d/go.mod h1:EkVYQZoAsY45+roYkvgYkIh4xh/qjgUK9TdY2XT94GE=
+golang.org/x/tools v0.0.0-20200606014950-c42cb6316fb6 h1:5Y8c5HBW6hBYnGEE3AbJPV0R8RsQmg1/eaJrpvasns0=
+golang.org/x/tools v0.0.0-20200606014950-c42cb6316fb6/go.mod h1:EkVYQZoAsY45+roYkvgYkIh4xh/qjgUK9TdY2XT94GE=
 golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
 golang.org/x/xerrors v0.0.0-20191011141410-1b5146add898/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
 golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543 h1:E7g+9GITq07hpfrRu66IVDexMakfv52eLZ2CXBWiKr4=
diff --git a/rule/defer.go b/rule/defer.go
new file mode 100644
index 0000000..2ec7ef4
--- /dev/null
+++ b/rule/defer.go
@@ -0,0 +1,137 @@
+package rule
+
+import (
+	"fmt"
+	"go/ast"
+
+	"github.com/mgechev/revive/lint"
+)
+
+// DeferRule lints unused params in functions.
+type DeferRule struct{}
+
+// Apply applies the rule to given file.
+func (r *DeferRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure {
+	allow := r.allowFromArgs(arguments)
+
+	var failures []lint.Failure
+	onFailure := func(failure lint.Failure) {
+		failures = append(failures, failure)
+	}
+
+	w := lintDeferRule{onFailure: onFailure, allow: allow}
+
+	ast.Walk(w, file.AST)
+
+	return failures
+}
+
+// Name returns the rule name.
+func (r *DeferRule) Name() string {
+	return "defer"
+}
+
+func (r *DeferRule) allowFromArgs(args lint.Arguments) map[string]bool {
+	if len(args) < 1 {
+		allow := map[string]bool{
+			"loop":        true,
+			"call-chain":  true,
+			"method-call": true,
+			"return":      true,
+			"recover":     true,
+		}
+
+		return allow
+	}
+
+	aa, ok := args[0].([]interface{})
+	if !ok {
+		panic(fmt.Sprintf("Invalid argument '%v' for 'defer' rule. Expecting []string, got %T", args[0], args[0]))
+	}
+
+	allow := make(map[string]bool, len(aa))
+	for _, subcase := range aa {
+		sc, ok := subcase.(string)
+		if !ok {
+			panic(fmt.Sprintf("Invalid argument '%v' for 'defer' rule. Expecting string, got %T", subcase, subcase))
+		}
+		allow[sc] = true
+	}
+
+	return allow
+}
+
+type lintDeferRule struct {
+	onFailure  func(lint.Failure)
+	inALoop    bool
+	inADefer   bool
+	inAFuncLit bool
+	allow      map[string]bool
+}
+
+func (w lintDeferRule) Visit(node ast.Node) ast.Visitor {
+	switch n := node.(type) {
+	case *ast.ForStmt:
+		w.visitSubtree(n.Body, w.inADefer, true, w.inAFuncLit)
+		return nil
+	case *ast.RangeStmt:
+		w.visitSubtree(n.Body, w.inADefer, true, w.inAFuncLit)
+		return nil
+	case *ast.FuncLit:
+		w.visitSubtree(n.Body, w.inADefer, false, true)
+		return nil
+	case *ast.ReturnStmt:
+		if len(n.Results) != 0 && w.inADefer && w.inAFuncLit {
+			w.newFailure("return in a defer function has no effect", n, 1.0, "logic", "return")
+		}
+	case *ast.CallExpr:
+		if isIdent(n.Fun, "recover") && !w.inADefer {
+			// confidence is not 1 because recover can be in a function that is deferred elsewhere
+			w.newFailure("recover must be called inside a deferred function", n, 0.8, "logic", "recover")
+		}
+	case *ast.DeferStmt:
+		w.visitSubtree(n.Call.Fun, true, false, false)
+
+		if w.inALoop {
+			w.newFailure("prefer not to defer inside loops", n, 1.0, "bad practice", "loop")
+		}
+
+		switch fn := n.Call.Fun.(type) {
+		case *ast.CallExpr:
+			w.newFailure("prefer not to defer chains of function calls", fn, 1.0, "bad practice", "call-chain")
+		case *ast.SelectorExpr:
+			if id, ok := fn.X.(*ast.Ident); ok {
+				isMethodCall := id != nil && id.Obj != nil && id.Obj.Kind == ast.Typ
+				if isMethodCall {
+					w.newFailure("be careful when deferring calls to methods without pointer receiver", fn, 0.8, "bad practice", "method-call")
+				}
+			}
+		}
+		return nil
+	}
+
+	return w
+}
+
+func (w lintDeferRule) visitSubtree(n ast.Node, inADefer, inALoop, inAFuncLit bool) {
+	nw := &lintDeferRule{
+		onFailure:  w.onFailure,
+		inADefer:   inADefer,
+		inALoop:    inALoop,
+		inAFuncLit: inAFuncLit,
+		allow:      w.allow}
+	ast.Walk(nw, n)
+}
+
+func (w lintDeferRule) newFailure(msg string, node ast.Node, confidence float64, cat string, subcase string) {
+	if !w.allow[subcase] {
+		return
+	}
+
+	w.onFailure(lint.Failure{
+		Confidence: confidence,
+		Node:       node,
+		Category:   cat,
+		Failure:    msg,
+	})
+}


Test patch:
diff --git a/test/defer_test.go b/test/defer_test.go
new file mode 100644
index 0000000..81a7b0d
--- /dev/null
+++ b/test/defer_test.go
@@ -0,0 +1,25 @@
+package test
+
+import (
+	"testing"
+
+	"github.com/mgechev/revive/lint"
+	"github.com/mgechev/revive/rule"
+)
+
+// Defer rule.
+func TestDefer(t *testing.T) {
+	testRule(t, "defer", &rule.DeferRule{})
+}
+
+func TestDeferLoopDisabled(t *testing.T) {
+	testRule(t, "defer-loop-disabled", &rule.DeferRule{}, &lint.RuleConfig{
+		Arguments: []interface{}{[]interface{}{"return", "recover", "call-chain", "method-call"}},
+	})
+}
+
+func TestDeferOthersDisabled(t *testing.T) {
+	testRule(t, "defer-only-loop-enabled", &rule.DeferRule{}, &lint.RuleConfig{
+		Arguments: []interface{}{[]interface{}{"loop"}},
+	})
+}
diff --git a/testdata/defer-loop-disabled.go b/testdata/defer-loop-disabled.go
new file mode 100644
index 0000000..1abd83c
--- /dev/null
+++ b/testdata/defer-loop-disabled.go
@@ -0,0 +1,28 @@
+package fixtures
+
+import "errors"
+
+type tt int
+
+func (t tt) m() {}
+
+func deferrer1() {
+	for {
+		go func() {
+			defer println()
+		}()
+		defer func() {}()
+	}
+
+	defer tt.m() // MATCH /be careful when deferring calls to methods without pointer receiver/
+
+	defer func() error {
+		return errors.New("error") //MATCH /return in a defer function has no effect/
+	}()
+
+	defer recover()
+
+	recover() //MATCH /recover must be called inside a deferred function/
+
+	defer deferrer()
+}
diff --git a/testdata/defer-only-loop-enabled.go b/testdata/defer-only-loop-enabled.go
new file mode 100644
index 0000000..99c8b62
--- /dev/null
+++ b/testdata/defer-only-loop-enabled.go
@@ -0,0 +1,28 @@
+package fixtures
+
+import "errors"
+
+type tt int
+
+func (t tt) m() {}
+
+func deferrer3() {
+	for {
+		go func() {
+			defer println()
+		}()
+		defer func() {}() // MATCH /prefer not to defer inside loops/
+	}
+
+	defer tt.m()
+
+	defer func() error {
+		return errors.New("error")
+	}()
+
+	defer recover()
+
+	recover()
+
+	defer deferrer()
+}
diff --git a/testdata/defer.go b/testdata/defer.go
new file mode 100644
index 0000000..d5b864f
--- /dev/null
+++ b/testdata/defer.go
@@ -0,0 +1,28 @@
+package fixtures
+
+import "errors"
+
+type tt int
+
+func (t tt) m() {}
+
+func deferrer() {
+	for {
+		go func() {
+			defer println()
+		}()
+		defer func() {}() // MATCH /prefer not to defer inside loops/
+	}
+
+	defer tt.m() // MATCH /be careful when deferring calls to methods without pointer receiver/
+
+	defer func() error {
+		return errors.New("error") //MATCH /return in a defer function has no effect/
+	}()
+
+	defer recover()
+
+	recover() //MATCH /recover must be called inside a deferred function/
+
+	defer deferrer()
+}