Fix bug relating to allowlist

Author: owen-mc

go/ql/src/RedundantCode/UnreachableStatement.ql

@@ -37,7 +37,6 @@ Stmt getPreviousStmt(Stmt s) {
  */
 predicate firstUnreachableStmt(Stmt s) {
   not isReachable(s) and
-  not s instanceof EmptyStmt and
   (
     // a statement whose preceding statement in the same list is reachable
     isReachable(getPreviousStmt(s))
@@ -47,6 +46,16 @@ predicate firstUnreachableStmt(Stmt s) {
   )
 }
 
+/** Holds if `s` is in a run of unreachable statements following a constant condition. */
+predicate isInUnreachableRunAfterConstantCondition(Stmt s) {
+  not isReachable(s) and
+  (
+    exists(getPreviousStmt(s).(IfStmt).getCondition().getBoolValue())
+    or
+    isInUnreachableRunAfterConstantCondition(getPreviousStmt(s))
+  )
+}
+
 /**
  * Matches if `retval` is a constant or a struct composed wholly of constants.
  */
@@ -87,14 +96,32 @@ predicate allowlist(Stmt s) {
   exists(ReturnStmt ret | ret = s |
     forall(Expr retval | retval = ret.getAnExpr() | isAllowedReturnValue(retval))
   )
-  or
-  // statements deliberately made unreachable by a constant condition, such as the code
-  // following `if true { return }`
-  exists(getPreviousStmt(s).(IfStmt).getCondition().getBoolValue())
+}
+
+/** Holds if `s` is part of a non-reportable prefix of a run of unreachable statements. */
+predicate isInNonReportableUnreachablePrefix(Stmt s) {
+  (allowlist(s) or s instanceof EmptyStmt) and
+  (
+    firstUnreachableStmt(s)
+    or
+    isInNonReportableUnreachablePrefix(getPreviousStmt(s))
+  )
+}
+
+/** Holds if `s` is the first non-allowlisted statement in a run of unreachable statements. */
+predicate firstNonAllowlistedUnreachableStmt(Stmt s) {
+  not isReachable(s) and
+  not s instanceof EmptyStmt and
+  not allowlist(s) and
+  (
+    firstUnreachableStmt(s)
+    or
+    isInNonReportableUnreachablePrefix(getPreviousStmt(s))
+  )
 }
 
 from Stmt s
 where
-  firstUnreachableStmt(s) and
-  not allowlist(s)
+  firstNonAllowlistedUnreachableStmt(s) and
+  not isInUnreachableRunAfterConstantCondition(s)
 select s, "This statement is unreachable."

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

@@ -10,3 +10,4 @@
 | main.go:147:2:147:17 | return statement | This statement is unreachable. |
 | main.go:153:2:153:22 | return statement | This statement is unreachable. |
 | main.go:159:2:159:43 | return statement | This statement is unreachable. |
+| main.go:171:2:171:14 | expression statement | This statement is unreachable. |

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

@@ -168,7 +168,7 @@ func test21() {
 	select {}
 	panic("unreachable") // OK: allowlisted statement
 	// OK: empty statement
-	unreachable() // $ MISSING: Alert
+	unreachable() // $ Alert
 }
 
 func test23() {