From db56db0b6a11404963d53472d5a40d90cc0868a6 Mon Sep 17 00:00:00 2001 From: Steven L Date: Sun, 24 Jul 2022 00:34:16 -0700 Subject: [PATCH] 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 --- RULES_DESCRIPTIONS.md | 2 ++ rule/defer.go | 31 +++++++++++++++++++++++++------ testdata/defer.go | 13 ++++++++++--- 3 files changed, 37 insertions(+), 9 deletions(-) diff --git a/RULES_DESCRIPTIONS.md b/RULES_DESCRIPTIONS.md index e51e5f12c..f003c0ab1 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 4165b6205..f8224fd4d 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, diff --git a/testdata/defer.go b/testdata/defer.go index d5b864fcf..a32b0cca0 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() }) }