Capture yet more bad defer / recover patterns #719
Conversation
`recover()` is an eternal font of excitement.
Groxx
changed the title
test/defer_test.go
Outdated
| @@ -23,3 +23,82 @@ func TestDeferOthersDisabled(t *testing.T) { | |||
| Arguments: []interface{}{[]interface{}{"loop"}}, | |||
| }) | |||
| } | |||
|
|
|||
| func TestDemonstrateIneffectiveDefers(t *testing.T) { | |||
There was a problem hiding this comment.
Usually this kind of tests is not part of rules tests. You could add all these cases to testdata/defer.go
There was a problem hiding this comment.
yea, my goal was to provide executable evidence that the patterns being detected behave the way the code / warnings describe. these aren't linted, they're run as part of the test suite.
I'm entirely fine just removing them if you'd prefer to keep the tests more focused on running the linter or something, I wrote them half to re-prove it to myself :)
|
Hi @Groxx thanks for the PR, I've left one comment on it. |
|
Thanks @Groxx ! |
…ind more defer/recover badness. mgechev/revive#719
…ind more defer/recover badness. mgechev/revive#719
* Update to latest version of revive that includes Steven's change to find more defer/recover badness. mgechev/revive#719 * Ignore 158 new warnings about package-comments. * Add Steven's new immediate-recover linter. - Verified this would have caught the previous mistakes I made (woohoo!)
* Update to latest version of revive that includes Steven's change to find more defer/recover badness. mgechev/revive#719 * Ignore 158 new warnings about package-comments. * Add Steven's new immediate-recover linter. - Verified this would have caught the previous mistakes I made (woohoo!)
defer+recover()is an eternal font of excitement.This adds a new category under
defer, "immediate-recover", which warns about cases like these:because they do not do what they appear to do. Neither will recover a panic (in normal use).
Tests and lint-tests include full demonstrations of ^ those, as well as some fairly exotic example where those do work.
I have not ever seen these in practice, so for the moment I've set the confidence level to
1- IMO both of these are very strong signals of flawed code, and worth running by default.I'm fairly sure I'm missing some things from this PR, e.g. documentation.
I'll try to look tomorrow, but I'd be happy for any pointers :) I'm happy to flesh it out.
Closes #718, which also contains motivation and other details.