Merge pull request #246 from smowton/smowton/feature/nuisance-dead-code-warnings

UnreachableStatement: tolerate more harmless unreachable return statements

Author: max-schaefer

change-notes/2020-07-06-tolerate-more-unreachable-returns.md

@@ -0,0 +1,2 @@
+lgtm,codescanning
+* The query "Unreachable statement" (`go/unreachable-statement`) now tolerates more unreachable return statements, which can often be required in Go following a function call that cannot return. Newly tolerated statements include `return true`, `return MyStruct{0, true}`, and any return when the return value has type `error`. This eliminates some nuisance results.

ql/src/RedundantCode/UnreachableStatement.ql

@@ -21,6 +21,36 @@ ControlFlow::Node nonGuardPredecessor(ControlFlow::Node nd) {
   )
 }
 
+/**
+ * Matches if `retval` is a constant or a struct composed wholly of constants.
+ */
+predicate isAllowedReturnValue(Expr retval) {
+  retval = Builtin::nil().getAReference()
+  or
+  retval = Builtin::true_().getAReference()
+  or
+  retval = Builtin::false_().getAReference()
+  or
+  retval instanceof BasicLit
+  or
+  // Allow -1 (which parses as unary-minus-of-literal) or !true, but not &somestruct,
+  // for which we would usually prefer `return nil`
+  isAllowedReturnValue(retval.(UnaryExpr).getOperand()) and
+  not retval.getType().getUnderlyingType() instanceof PointerType
+  or
+  // Allow structs composed of allowed values
+  retval instanceof StructLit and
+  forall(Expr element | element = retval.(StructLit).getAnElement() | isAllowedReturnValue(element))
+  or
+  // Allow anything of type `error`, as `abort(); return constructError(...);`
+  // is preferable to insisting on a misleading `return nil` that suggests
+  // successful return:
+  retval.getType().getEntity() = Builtin::error()
+}
+
+/**
+ * Matches if `s` is an allowed unreachable statement.
+ */
 predicate allowlist(Stmt s) {
   // `panic("unreachable")` and similar
   exists(CallExpr ce | ce = s.(ExprStmt).getExpr() or ce = s.(ReturnStmt).getExpr() |
@@ -29,11 +59,7 @@ predicate allowlist(Stmt s) {
   or
   // `return nil` and similar
   exists(ReturnStmt ret | ret = s |
-    forall(Expr retval | retval = ret.getAnExpr() |
-      retval = Builtin::nil().getAReference() or
-      retval instanceof BasicLit or
-      retval.(UnaryExpr).getOperand() instanceof BasicLit
-    )
+    forall(Expr retval | retval = ret.getAnExpr() | isAllowedReturnValue(retval))
   )
   or
   // statements in an `if false { ... }` and similar

ql/test/query-tests/RedundantCode/UnreachableStatement/UnreachableStatement.expected

@@ -1,8 +1,12 @@
 | UnreachableStatement.go:5:27:5:29 | increment statement | This statement is unreachable. |
-| main.go:9:2:9:14 | expression statement | This statement is unreachable. |
-| main.go:14:2:14:14 | expression statement | This statement is unreachable. |
-| main.go:18:22:18:34 | expression statement | This statement is unreachable. |
-| main.go:26:2:26:14 | expression statement | This statement is unreachable. |
-| main.go:45:2:45:14 | expression statement | This statement is unreachable. |
-| main.go:51:3:51:15 | expression statement | This statement is unreachable. |
-| main.go:53:2:53:14 | expression statement | This statement is unreachable. |
+| main.go:11:2:11:14 | expression statement | This statement is unreachable. |
+| main.go:16:2:16:14 | expression statement | This statement is unreachable. |
+| main.go:20:22:20:34 | expression statement | This statement is unreachable. |
+| main.go:28:2:28:14 | expression statement | This statement is unreachable. |
+| main.go:47:2:47:14 | expression statement | This statement is unreachable. |
+| main.go:53:3:53:15 | expression statement | This statement is unreachable. |
+| main.go:55:2:55:14 | expression statement | This statement is unreachable. |
+| main.go:139:2:139:26 | return statement | This statement is unreachable. |
+| main.go:145:2:145:17 | return statement | This statement is unreachable. |
+| main.go:151:2:151:22 | return statement | This statement is unreachable. |
+| main.go:157:2:157:43 | return statement | This statement is unreachable. |

ql/test/query-tests/RedundantCode/UnreachableStatement/main.go

@@ -1,5 +1,7 @@
 package main
 
+import ("errors")
+
 func unreachable() {}
 
 func reachable() {}
@@ -101,4 +103,58 @@ func test11() {
 	}
 }
 
+func test12() bool {
+	select {}
+	// Not flagged as this is a primitive return
+	return true
+}
+
+func test13() bool {
+	select {}
+	// Not flagged as this is a primitive return
+	return false
+}
+
+type mystruct struct {
+	x int
+	y bool
+}
+
+func test14() mystruct {
+	select {}
+	// Not flagged as this is a struct literal composed of primitives
+	return mystruct{0, true}
+}
+
+func test15() error {
+	select {}
+	// Not flagged as any expression is acceptable for type `error`
+	return errors.New("unreachable")
+}
+
+func test16() *mystruct {
+	select {}
+	// Flagged, as `return nil` is possible and preferable when the
+	// return site is unreachable.
+	return &mystruct{0, true}
+}
+
+func test17() int {
+	select {}
+	// Flagged, as a nontrivial unreachable return
+	return test10(1)
+}
+
+func test18() bool {
+	select {}
+	// Flagged, as a nontrivial unreachable return
+	return test10(1) == 1
+}
+
+func test19() mystruct {
+	select {}
+	// Flagged, as a nontrivial unreachable return
+	return mystruct{test10(1), test10(2) == 2}
+}
+
 func main() {}