UnreachableStatement: tolerate more harmless unreachable return statements#246
Conversation
max-schaefer
left a comment
max-schaefer
left a comment
There was a problem hiding this comment.
LGTM, modulo one minor nit. Could you also add a change note? See https://github.com/github/codeql-go/blob/master/change-notes/2020-05-01-bad-redirect-check.md for an example.
| or | ||
| retval instanceof BasicLit | ||
| or | ||
| isAllowedReturnValue(retval.(UnaryExpr).getOperand()) |
There was a problem hiding this comment.
Could you add a test for this? Presumably it's for handling return &SomeStruct{}?
There was a problem hiding this comment.
I don't know what this intends to catch, it was pre-existing
There was a problem hiding this comment.
What an excellent point. Can you add a test for return &SomeStruct{} anyway?
There was a problem hiding this comment.
It actually wouldn't have caught that (and I'd argue, should not -- they should surely always return nil for a type that supports it), as it previously specified unary-op-of-basic-literal. Troublingly, it looks like -1 is being parsed as unary-minus-of-literal-one rather than literal-negative-one. I'll tighten it up to catch only that case.
There was a problem hiding this comment.
Troublingly, it looks like -1 is being parsed as unary-minus-of-literal-one
Ah, right. That is beyond our control (being a decision made by the Go frontend), but definitely worth tightening the predicate.
…ments The Golang compiler isn't particularly good at spotting paths that don't need a return statement due to a dominating noreturn statement (e.g. os.Exit(1)), so dead return statements are common. We already tried to tolerate some instances of this pattern; this additionally allows 'true' and 'false' literals, and anything of type 'error'. The carte-blanche for error values aims to accommodate the pattern "abort(); return whateverErrorWouldOtherwiseBeAppropriate();", which is probably preferable to "return nil", a misleading no-error indication.
smowton
force-pushed
the
smowton/feature/nuisance-dead-code-warnings
branch
from
a5d0d26 to
5b34c05
Compare
2 participants
The Golang compiler isn't particularly good at spotting paths that don't need a return statement due to a dominating noreturn statement (e.g. os.Exit(1)), so dead return statements are common. We already tried to tolerate some instances of this pattern; this additionally allows 'true' and 'false' literals, and anything of type 'error'.
The carte-blanche for error values aims to accommodate the pattern "abort(); return whateverErrorWouldOtherwiseBeAppropriate();", which is probably preferable to "return nil", a misleading no-error indication.
Intentionally excluded from this PR: unreachable 'break' and 'continue' statements, which misleadingly indicate control-flow will go somewhere it doesn't.