1
0
mirror of https://github.com/mgechev/revive.git synced 2025-10-30 23:37:49 +02:00

fix #863:false positive on return statement in a func lit passed to the deferred function (#870)

This commit is contained in:
chavacava
2023-08-18 20:21:42 +02:00
committed by GitHub
parent 519ffbdd7a
commit 9acfcc8f86
2 changed files with 26 additions and 3 deletions

View File

@@ -97,18 +97,21 @@ func (w lintDeferRule) Visit(node ast.Node) ast.Visitor {
w.newFailure("return in a defer function has no effect", n, 1.0, "logic", "return") w.newFailure("return in a defer function has no effect", n, 1.0, "logic", "return")
} }
case *ast.CallExpr: case *ast.CallExpr:
if !w.inADefer && isIdent(n.Fun, "recover") { isCallToRecover := isIdent(n.Fun, "recover")
switch {
case !w.inADefer && isCallToRecover:
// func fn() { recover() } // func fn() { recover() }
// //
// confidence is not 1 because recover can be in a function that is deferred elsewhere // 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") 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") { case w.inADefer && !w.inAFuncLit && isCallToRecover:
// defer helper(recover()) // defer helper(recover())
// //
// confidence is not truly 1 because this could be in a correctly-deferred func, // 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. // 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") w.newFailure("recover must be called inside a deferred function, this is executing recover immediately", n, 1, "logic", "immediate-recover")
} }
case *ast.DeferStmt: case *ast.DeferStmt:
if isIdent(n.Call.Fun, "recover") { if isIdent(n.Call.Fun, "recover") {
// defer recover() // defer recover()
@@ -119,7 +122,12 @@ func (w lintDeferRule) Visit(node ast.Node) ast.Visitor {
} }
w.visitSubtree(n.Call.Fun, true, false, false) w.visitSubtree(n.Call.Fun, true, false, false)
for _, a := range n.Call.Args { for _, a := range n.Call.Args {
w.visitSubtree(a, true, false, false) // check arguments, they should not contain recover() switch a.(type) {
case *ast.FuncLit:
continue // too hard to analyze deferred calls with func literals args
default:
w.visitSubtree(a, true, false, false) // check arguments, they should not contain recover()
}
} }
if w.inALoop { if w.inALoop {
@@ -136,6 +144,7 @@ func (w lintDeferRule) Visit(node ast.Node) ast.Visitor {
w.newFailure("be careful when deferring calls to methods without pointer receiver", fn, 0.8, "bad practice", "method-call") w.newFailure("be careful when deferring calls to methods without pointer receiver", fn, 0.8, "bad practice", "method-call")
} }
} }
} }
return nil return nil
} }

14
testdata/defer.go vendored
View File

@@ -33,3 +33,17 @@ func deferrer() {
// does not work, but not currently blocked. // does not work, but not currently blocked.
defer helper(func() { recover() }) defer helper(func() { recover() })
} }
// Issue #863
func verify(fn func() error) {
if err := fn(); err != nil {
panic(err)
}
}
func f() {
defer verify(func() error {
return nil
})
}