Go: Add query for redundant calls to recover#147
Conversation
max-schaefer
left a comment
max-schaefer
left a comment
There was a problem hiding this comment.
LGTM overall, I just have some minor stylistic comments and suggestions. We won't need a performance evaluation for this query, since it is very simple and syntactic. Can I just confirm that you have reviewed results on LGTM.com and are satisfied that most of them are true positives?
| </p> | ||
| <sample src="RedundantRecover1.go" /> | ||
| <p> | ||
| This problem can be deferring the call to the function which calls<code>recover</code>: |
There was a problem hiding this comment.
I can't parse this sentence. Is there a "fixed by" missing somewhere?
| f = recoverCall.getEnclosingCallable() and | ||
| ( | ||
| isDeferred(recoverCall) and | ||
| msg = "Deferred calls to 'recover' have no effect" |
There was a problem hiding this comment.
For consistency:
| msg = "Deferred calls to 'recover' have no effect" | |
| msg = "Deferred calls to 'recover' have no effect." |
| not isDeferred(f.getACall()) and | ||
| msg = "This call to 'recover' has no effect because $@ is never called using a defer statement." | ||
| ) | ||
| select recoverCall, msg, f, f.getName() |
There was a problem hiding this comment.
Not all functions have a name, so f.getName() may be undefined, causing the alert to be suppressed. I'd suggest instead using "the enclosing function":
| select recoverCall, msg, f, f.getName() | |
| select recoverCall, msg, f, "the enclosing function" |
| @@ -0,0 +1,3 @@ | |||
| | RedundantRecover1.go:6:5:6:13 | call to recover | This call to 'recover' has no effect because $@ is never called using a defer statement. | RedundantRecover1.go:5:1:9:1 | function declaration | callRecover1 | | |||
There was a problem hiding this comment.
| | RedundantRecover1.go:6:5:6:13 | call to recover | This call to 'recover' has no effect because $@ is never called using a defer statement. | RedundantRecover1.go:5:1:9:1 | function declaration | callRecover1 | | |
| | RedundantRecover1.go:6:5:6:13 | call to recover | This call to 'recover' has no effect because $@ is never called using a defer statement. | RedundantRecover1.go:5:1:9:1 | function declaration | the enclosing function | |
| @@ -0,0 +1,3 @@ | |||
| | RedundantRecover1.go:6:5:6:13 | call to recover | This call to 'recover' has no effect because $@ is never called using a defer statement. | RedundantRecover1.go:5:1:9:1 | function declaration | callRecover1 | | |||
| | RedundantRecover2.go:4:8:4:16 | call to recover | Deferred calls to 'recover' have no effect | RedundantRecover2.go:3:1:6:1 | function declaration | fun2 | | |||
There was a problem hiding this comment.
| | RedundantRecover2.go:4:8:4:16 | call to recover | Deferred calls to 'recover' have no effect | RedundantRecover2.go:3:1:6:1 | function declaration | fun2 | | |
| | RedundantRecover2.go:4:8:4:16 | call to recover | Deferred calls to 'recover' have no effect. | RedundantRecover2.go:3:1:6:1 | function declaration | fun2 | |
| @@ -0,0 +1,3 @@ | |||
| | RedundantRecover1.go:6:5:6:13 | call to recover | This call to 'recover' has no effect because $@ is never called using a defer statement. | RedundantRecover1.go:5:1:9:1 | function declaration | callRecover1 | | |||
| | RedundantRecover2.go:4:8:4:16 | call to recover | Deferred calls to 'recover' have no effect | RedundantRecover2.go:3:1:6:1 | function declaration | fun2 | | |||
| | tst.go:8:5:8:13 | call to recover | This call to 'recover' has no effect because $@ is never called using a defer statement. | tst.go:5:1:11:1 | function declaration | callRecover3 | | |||
There was a problem hiding this comment.
| | tst.go:8:5:8:13 | call to recover | This call to 'recover' has no effect because $@ is never called using a defer statement. | tst.go:5:1:11:1 | function declaration | callRecover3 | | |
| | tst.go:8:5:8:13 | call to recover | This call to 'recover' has no effect because $@ is never called using a defer statement. | tst.go:5:1:11:1 | function declaration | the enclosing function | |
|
I think the docs look good, but would it also be possible to see an HTML preview of the query help as a sanity check? (I'm not sure where those previews usually come from 👀 ) |
|
HTML preview is on its way (you're too fast 🙂 ). |
|
I've added a commit to fix address your comments, @max-schaefer . (Let me know if I should squash it.) Yes, I've seen the results on LGTM.com and they seemed to be almost entirely true positives. I put the precision as 'high' because I wasn't sure what the bar for 'very high' is. |
shati-patel
left a comment
shati-patel
left a comment
There was a problem hiding this comment.
Thanks, the query help looks good 😊 I've just added a small suggestion to the change note.
Co-authored-by: Shati Patel <42641846+shati-patel@users.noreply.github.com>
4 participants
It's very easy to think you are using
recovercorrectly when you aren't. The two patterns I've found are (a) deferring a call torecoverdirectly, (b) not deferring the call torecoverand (c) deferring a function which calls a function which calls recover - the panic doesn't propagate down the stack, only back up it. This query finds direct calls to recover and calls to recover in functions which are never called in a defer statement (but which are called at least once). I haven't done a perfomance comparison.