Capture yet more bad defer / recover patterns (#719)

* Yet more bad defer / recover patterns

`recover()` is an eternal font of excitement.

* demonstrating another flavor of failure

* removing leftover code

* update documentation

* removes test not related to the rule itself

Co-authored-by: chavacava <[email protected]>

Author: Groxx

RULES_DESCRIPTIONS.md

@@ -233,12 +233,14 @@ _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|
+| immediate-recover | calling `recover` at the time a defer is registered, rather than as part of the deferred callback.  e.g. `defer recover()` or equivalent.|
 | 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)

rule/defer.go

@@ -45,11 +45,12 @@ func (*DeferRule) Name() string {
 func (*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,
+			"loop":              true,
+			"call-chain":        true,
+			"method-call":       true,
+			"return":            true,
+			"recover":           true,
+			"immediate-recover": true,
 		}
 
 		return allow
@@ -97,11 +98,29 @@ func (w lintDeferRule) Visit(node ast.Node) ast.Visitor {
 		}
 	case *ast.CallExpr:
 		if !w.inADefer && isIdent(n.Fun, "recover") {
+			// func fn() { recover() }
+			//
 			// 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")
+		} else if w.inADefer && !w.inAFuncLit && isIdent(n.Fun, "recover") {
+			// defer helper(recover())
+			//
+			// confidence is not truly 1 because this could be in a correctly-deferred func,
+			// but it is very likely to be a misunderstanding of defer's behavior around arguments.
+			w.newFailure("recover must be called inside a deferred function, this is executing recover immediately", n, 1, "logic", "immediate-recover")
 		}
 	case *ast.DeferStmt:
+		if isIdent(n.Call.Fun, "recover") {
+			// defer recover()
+			//
+			// confidence is not truly 1 because this could be in a correctly-deferred func,
+			// but normally this doesn't suppress a panic, and even if it did it would silently discard the value.
+			w.newFailure("recover must be called inside a deferred function, this is executing recover immediately", n, 1, "logic", "immediate-recover")
+		}
 		w.visitSubtree(n.Call.Fun, true, false, false)
+		for _, a := range n.Call.Args {
+			w.visitSubtree(a, true, false, false) // check arguments, they should not contain recover()
+		}
 
 		if w.inALoop {
 			w.newFailure("prefer not to defer inside loops", n, 1.0, "bad practice", "loop")
@@ -125,7 +144,7 @@ func (w lintDeferRule) Visit(node ast.Node) ast.Visitor {
 }
 
 func (w lintDeferRule) visitSubtree(n ast.Node, inADefer, inALoop, inAFuncLit bool) {
-	nw := &lintDeferRule{
+	nw := lintDeferRule{
 		onFailure:  w.onFailure,
 		inADefer:   inADefer,
 		inALoop:    inALoop,

testdata/defer.go

@@ -17,12 +17,19 @@ func deferrer() {
 	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/
+		return errors.New("error") // MATCH /return in a defer function has no effect/
 	}()
 
-	defer recover()
+	defer recover() // MATCH /recover must be called inside a deferred function, this is executing recover immediately/
 
-	recover() //MATCH /recover must be called inside a deferred function/
+	recover() // MATCH /recover must be called inside a deferred function/
 
 	defer deferrer()
+
+	helper := func(_ interface{}) {}
+
+	defer helper(recover()) // MATCH /recover must be called inside a deferred function, this is executing recover immediately/
+
+	// does not work, but not currently blocked.
+	defer helper(func() { recover() })
 }