Merge pull request #22679 from owen-mc/go/fix/unreachable-statement-allowlist

Go: fix some duplicate results in go/unreachable-statement

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.
  */
@@ -78,6 +87,8 @@ predicate isAllowedReturnValue(Expr retval) {
  * Matches if `s` is an allowed unreachable statement.
  */
 predicate allowlist(Stmt s) {
+  s instanceof EmptyStmt
+  or
   // `panic("unreachable")` and similar
   exists(CallExpr ce | ce = s.(ExprStmt).getExpr() or ce = s.(ReturnStmt).getExpr() |
     ce.getTarget().mustPanic() or ce.getCalleeName().toLowerCase() = "error"
@@ -87,14 +98,28 @@ 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())
+}
+
+Stmt firstNonAllowlisted(Stmt s) {
+  not isReachable(s) and
+  (
+    not allowlist(s) and result = s
+    or
+    allowlist(s) and
+    exists(Stmt next | getPreviousStmt(next) = s | result = firstNonAllowlisted(next))
+  )
+}
+
+/** Holds if `s` is the first non-allowlisted statement in a run of unreachable statements. */
+predicate firstNonAllowlistedUnreachableStmt(Stmt s) {
+  exists(Stmt unreachable |
+    firstUnreachableStmt(unreachable) and
+    s = firstNonAllowlisted(unreachable)
+  )
 }
 
 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/CONSISTENCY/CfgConsistency.expected

@@ -1,5 +1,5 @@
 consistencyOverview
-| deadEnd | 10 |
+| deadEnd | 11 |
 deadEnd
 | main.go:17:2:17:10 | select statement |
 | main.go:109:2:109:10 | select statement |
@@ -11,3 +11,4 @@ deadEnd
 | main.go:151:2:151:10 | select statement |
 | main.go:157:2:157:10 | select statement |
 | main.go:164:2:164:10 | select statement |
+| main.go:168:2:168:10 | select statement |

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

@@ -164,4 +164,26 @@ func test20() {
 	select {} // OK: reachable after starting the goroutine
 }
 
+func test21() {
+	select {}
+	panic("unreachable") // OK: allowlisted statement
+	// OK: empty statement
+	unreachable() // $ Alert
+}
+
+func test22() {
+	goto reachableLabel
+	panic("unreachable") // OK: allowlisted statement
+reachableLabel:
+	reachable() // OK: reachable through the goto
+}
+
+func test23() {
+	if true {
+		return
+	}
+	unreachable() // OK: deliberately unreachable
+	unreachable() // OK: deliberately unreachable
+}
+
 func main() {}