Repository: mgechev/revive (go)

Issue:
Capture more bad / risky defer+recover patterns
While cleaning up some code in a repo I work on, and attempting to lock down our `defer` rules, the first round of fixing the problem led to code like this:
```go
func something() {
  defer logPanics()
}
func logPanics() {
  if r := recover() ...  // we want to enforce the lint that blocks this
}

// because this does not work
func helper() {
  logPanics()
}
func other() {
  defer helper()
}
```
turning into code like this:
```go
func something() {
  defer logPanics(recover())  // oops
}
```
... which trades one issue for another.

---

Since our underlying goal is to block all non-deferred recovers because it's error-prone (there are a few flaws in our codebase motivating this cleanup), and helper funcs are great for many reasons, the _intent_ was to do stuff like this:
```go
defer func() { logPanics(recover()) }()  // verbose but impossible to screw up helper funcs
```
Since `defer logPanics(recover())` is simpler looking, the eternal battle with working on a tired brain combined with the eternal seduction of seemingly-straightforward code led to the simpler-but-wrong one being selected.

Eh, it happens.  
But this is a _great_ signal for something worth locking down.

With reliable "must defer" and "must recover in deferred func, not arguments" lints, I think ^ both of those bad patterns can be made impossible.

Pull request:
Capture yet more bad defer / recover patterns
`defer` + `recover()` is an eternal font of excitement.

This adds a new category under `defer`, "immediate-recover", which warns about cases like these:
```go
defer recover()
defer helper(recover())
```
because they do not do what they appear to do.  Neither will recover a panic (in normal use).

Tests and lint-tests include full demonstrations of ^ those, as well as some fairly exotic example where those _do_ work.
I have not ever seen these in practice, so for the moment I've set the confidence level to `1` - IMO both of these are very strong signals of flawed code, and worth running by default.

I'm fairly sure I'm missing some things from this PR, e.g. documentation.
I'll try to look tomorrow, but I'd be happy for any pointers :)  I'm happy to flesh it out.

Closes #718, which also contains motivation and other details.

Patch:
diff --git a/RULES_DESCRIPTIONS.md b/RULES_DESCRIPTIONS.md
index e51e5f1..f003c0a 100644
--- a/RULES_DESCRIPTIONS.md
+++ b/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)
diff --git a/rule/defer.go b/rule/defer.go
index 4165b62..f8224fd 100644
--- a/rule/defer.go
+++ b/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,


Test patch:
diff --git a/testdata/defer.go b/testdata/defer.go
index d5b864f..a32b0cc 100644
--- a/testdata/defer.go
+++ b/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() })
 }