proposal: cmd/vet: check for deferring test cleanup

[uhthomas]

Proposal Details

I noticed recently during a code review that t.Cleanup was being deferred, which is almost certainly unintentional.

func TestABC(t *testing.T) {
    defer t.Cleanup(func() {
        // ...
    })
}

A naive search shows hundreds of results, and it's possible a more accurate search would show even more.

https://github.com/search?q=%22defer+t.Cleanup%28func%22+language%3AGo&type=code

It would be nice to have an analyser to catch this as I am not sure I see any scenario where this is actually desirable.

[uhthomas]

It's worth noting that this will unintentionally reverse the order in which the cleanup functions are run.

The docs say:

Cleanup functions will be called in last added, first called order.
https://pkg.go.dev/testing#T.Cleanup

Therefore, these will have different behaviour:

t.Cleanup(a)
t.Cleanup(b)
// b() -> a()

defer t.Cleanup(a)
defer t.Cleanup(b)
// cleanup(b) -> cleanup(a)
// a() -> b()

[adonovan]

I agree these cases probably arise from misunderstanding, but I wonder how often the defer is actually incorrect, and, conversely, whether there are any cases in which the defer is actually crucial for correctness. If the defer is occasionally critical, we can't reliably suggest not to use it.

[uhthomas]

That's a good point, though I did skim a few pages and have yet to see a case where it makes sense.