Skip to content

Commit

Permalink
fix #863:false positive on return statement in a func lit passed to t…
Browse files Browse the repository at this point in the history
…he deferred function (#870)
  • Loading branch information
chavacava authored Aug 18, 2023
1 parent 519ffbd commit 9acfcc8
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 3 deletions.
15 changes: 12 additions & 3 deletions rule/defer.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
case *ast.CallExpr:
if !w.inADefer && isIdent(n.Fun, "recover") {
isCallToRecover := isIdent(n.Fun, "recover")
switch {
case !w.inADefer && isCallToRecover:
// 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") {
case w.inADefer && !w.inAFuncLit && isCallToRecover:
// 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()
Expand All @@ -119,7 +122,12 @@ func (w lintDeferRule) Visit(node ast.Node) ast.Visitor {
}
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()
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 {
Expand All @@ -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")
}
}

}
return nil
}
Expand Down
14 changes: 14 additions & 0 deletions testdata/defer.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,3 +33,17 @@ func deferrer() {
// does not work, but not currently blocked.
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
})
}

0 comments on commit 9acfcc8

Please sign in to comment.