defer FP on return statement in a function literal passed to the deferred function

[feldgendler]

Describe the bug The defer check misfires on a return statement inside a function literal passed to the deferred function.

To Reproduce Steps to reproduce the behavior:

1. I updated revive go install github.com/mgechev/revive@latest
2. I run it with the following flags & configuration file:

revive -config revive.toml test.go

revive.toml:

[rule.defer]

test.go:

package test

func verify(fn func() error) {
    if err := fn(); err != nil {
        panic(err)
    }
}

func f() {
    defer verify(func() error {
        return nil
    })
}

Here, verify executes the given function and verifies that it has succeeded. We use this pattern in our project, and I don't see anything wrong with it.

Although return has no effect in a deferred function, this one is not in the deferrede function; it's in a function that's passed as an argument to the deferred function.

Expected behavior No defer issue is reported.

Logs

test.go:11:3: return in a defer function has no effect

Desktop (please complete the following information):

• Ubuntu 23.04
• go version go1.20.6 linux/amd64

[chavacava]

Hi @feldgendler thanks for filling the issue.

[chavacava]

@feldgendler, I've created a PR with a fix to avoid the false positive. Please check if the fix works for you (and if is an acceptable approach)

[feldgendler]

Yes, thank you! It fixes our case. It's definitely a step in the right direction (towards being less aggressive).

I see what kind of bug you are trying to prevent, though, so here is a suggestion. I would completely replace this particular rule (“return in a defer function has no effect”) with an entirely different one:

case *ast.DeferStmt:
    if fl, ok := n.Call.Fun.(*ast.FuncLit); ok && fl.Type.Results != nil {
        w.newFailure("deferred function with useless return values", 1, "logic", "return")
    }

That's it. You don't need to look inside for a return statement because defer func() T { ... }() is just always wrong; whatever the author might think it does, it definitely doesn't do.

And if n.Call.Fun is not a function literal, then it's OK. For example, defer f.Close() is OK even though f.Close returns en error. Likewise defer w.Write(...) where two values are ignored. This might not be great style, but there are other linters to detect that.

[feldgendler]

While we're at it: while reading fix/863, I noticed something else.

I'm not sure I understand correctly the exact logic behind the recover and immediate-recover subrules, but in my view, the “area where a recover() call is expected” should be only the body of n.Call.Fun.(*ast.FuncLit). Anywhere else, including named functions and methods, function literals in deferred arguments, and function literals lexically nested inside n.Call.Fun.(*ast.FuncLit), is not a place where recover() is expected.

This simple rule will flag all of these, which definitely look like bugs:

func main() {
    e := recover() // not in a deferred function
    ...
}

...

defer recover() // doesn't do what one might naively guess

defer f(recover()) // doesn't do what one might naively guess

defer func() {
    e := func() any {
        return recover() // Go reference manual says this won't work
    }()
    ...
}()

defer f(func() any {
    return recover() // won't work even if f calls its argument
})

The only kind of false positive that I can think of is

func f() {
    e := recover()
    ...
}

...

defer f()

This might be correct code, but I'd say it's bad style, and anyone who thinks otherwise should disable the defer rule.

Final note: you might want to extend the rule from just recover() calls to any mentions of recover. Although defer f(recover) might be technically correct if f calls its argument, it's shady as shit.