Merge pull request #17857 from geoffw0/unreachable3

Rust: Fix rust/dead-code

rust/ql/src/queries/unusedentities/UnreachableCode.ql

@@ -13,63 +13,62 @@ import codeql.rust.controlflow.ControlFlowGraph
 import codeql.rust.controlflow.internal.ControlFlowGraphImpl as ControlFlowGraphImpl
 
 /**
- * Holds if `n` is an AST node that's unreachable.
+ * Successor relation that includes unreachable AST nodes.
  */
-private predicate unreachable(AstNode n) {
-  not n = any(CfgNode cfn).getAstNode() and // reachable nodes
-  exists(ControlFlowGraphImpl::ControlFlowTree cft |
-    // nodes intended to be part of the CFG
-    cft.succ(n, _, _)
-    or
-    cft.succ(_, n, _)
-  )
+private predicate succ(AstNode a, AstNode b) {
+  exists(ControlFlowGraphImpl::ControlFlowTree cft | cft.succ(a, b, _))
 }
 
 /**
- * Holds if `n` is an AST node that's unreachable, and is not the successor
- * of an unreachable node (which would be a duplicate result).
+ * Gets a node we'd prefer not to report as unreachable. These will be removed
+ * from the AST for the purposes of this query, with successor links being
+ * made across them where appropriate.
  */
-private predicate firstUnreachable(AstNode n) {
-  unreachable(n) and
-  (
-    // no predecessor -> we are the first unreachable node.
-    not ControlFlowGraphImpl::succ(_, n, _)
-    or
-    // reachable predecessor -> we are the first unreachable node.
-    exists(AstNode pred |
-      ControlFlowGraphImpl::succ(pred, n, _) and
-      not unreachable(pred)
-    )
-  )
+predicate hiddenNode(AstNode n) {
+  // isolated node (not intended to be part of the CFG)
+  not succ(n, _) and
+  not succ(_, n)
+  or
+  n instanceof ControlFlowGraphImpl::PostOrderTree // location is counter-intuitive
 }
 
 /**
- * Gets a node we'd prefer not to report as unreachable.
+ * Successor relation for edges out of `hiddenNode`s.
  */
-predicate skipNode(AstNode n) {
-  n instanceof ControlFlowGraphImpl::PostOrderTree or // location is counter-intuitive
-  not n instanceof ControlFlowGraphImpl::ControlFlowTree // not expected to be reachable
+private predicate succHidden(AstNode a, AstNode b) {
+  hiddenNode(a) and
+  succ(a, b)
 }
 
 /**
- * Gets the `ControlFlowTree` successor of a node we'd prefer not to report.
+ * Successor relation that removes / links over `hiddenNode`s.
  */
-AstNode skipSuccessor(AstNode n) {
-  skipNode(n) and
-  ControlFlowGraphImpl::succ(n, result, _)
+private predicate succWithHiding(AstNode a, AstNode b) {
+  exists(AstNode mid |
+    not hiddenNode(a) and
+    succ(a, mid) and
+    succHidden*(mid, b) and
+    not hiddenNode(b)
+  )
 }
 
 /**
- * Gets the node `n`, skipping past any nodes we'd prefer not to report.
+ * An AST node that is reachable.
  */
-AstNode skipSuccessors(AstNode n) {
-  result = skipSuccessor*(n) and
-  not skipNode(result)
+predicate reachable(AstNode n) { n = any(CfgNode cfn).getAstNode() }
+
+/**
+ * Holds if `n` is an AST node that's unreachable, and any predecessors
+ * of it are reachable (to avoid duplicate results).
+ */
+private predicate firstUnreachable(AstNode n) {
+  not reachable(n) and
+  not hiddenNode(n) and
+  forall(AstNode pred | succWithHiding(pred, n) | reachable(pred))
 }
 
-from AstNode first, AstNode report
+from AstNode n
 where
-  firstUnreachable(first) and
-  report = skipSuccessors(first) and
-  exists(report.getFile().getRelativePath()) // in source
-select report, "This code is never reached."
+  firstUnreachable(n) and
+  exists(n.getFile().getRelativePath()) // in source
+select n, "This code is never reached."

rust/ql/test/query-tests/unusedentities/UnreachableCode.expected

@@ -5,17 +5,12 @@
 | unreachable.rs:61:2:61:16 | ExprStmt | This code is never reached. |
 | unreachable.rs:107:16:107:23 | ExprStmt | This code is never reached. |
 | unreachable.rs:115:15:115:22 | ExprStmt | This code is never reached. |
-| unreachable.rs:131:2:131:16 | ExprStmt | This code is never reached. |
 | unreachable.rs:141:2:141:16 | ExprStmt | This code is never reached. |
 | unreachable.rs:148:3:148:17 | ExprStmt | This code is never reached. |
 | unreachable.rs:157:4:157:18 | ExprStmt | This code is never reached. |
 | unreachable.rs:163:3:163:17 | ExprStmt | This code is never reached. |
 | unreachable.rs:169:4:169:18 | ExprStmt | This code is never reached. |
 | unreachable.rs:177:4:177:18 | ExprStmt | This code is never reached. |
 | unreachable.rs:180:2:180:16 | ExprStmt | This code is never reached. |
-| unreachable.rs:197:2:197:16 | ExprStmt | This code is never reached. |
 | unreachable.rs:203:3:203:17 | ExprStmt | This code is never reached. |
-| unreachable.rs:206:2:206:16 | ExprStmt | This code is never reached. |
 | unreachable.rs:218:3:218:17 | ExprStmt | This code is never reached. |
-| unreachable.rs:233:2:233:16 | ExprStmt | This code is never reached. |
-| unreachable.rs:242:2:242:16 | ExprStmt | This code is never reached. |

rust/ql/test/query-tests/unusedentities/unreachable.rs

@@ -128,7 +128,7 @@ fn unreachable_match() {
 			do_something();
 		}
 	}
-	do_something(); // [unreachable FALSE POSITIVE]
+	do_something();
 
 	match get_a_number() {
 		1=>{
@@ -194,7 +194,7 @@ fn unreachable_let_1() {
 		do_something();
 	}
 
-	do_something(); // SPURIOUS: unreachable code
+	do_something();
 
 	if let _ = get_a_number() { // (always succeeds)
 		do_something();
@@ -203,7 +203,7 @@ fn unreachable_let_1() {
 		do_something(); // BAD: unreachable code
 	}
 
-	do_something(); // BAD: unreachable code
+	do_something();
 }
 
 fn unreachable_let_2() {
@@ -230,7 +230,7 @@ fn unreachable_if_2() {
 		do_something();
 	}
 
-	do_something(); // SPURIOUS: unreachable code
+	do_something();
 }
 
 fn unreachable_if_3() {
@@ -239,5 +239,5 @@ fn unreachable_if_3() {
 		return;
 	}
 
-	do_something(); // SPURIOUS: unreachable code
+	do_something();
 }