Merge pull request github#17525 from geoffw0/unreachable

Rust: Unreachable code query

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

@@ -0,0 +1,24 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>This rule finds code that is never reached. Unused code should be removed to increase readability and avoid confusion.</p>
+</overview>
+
+<recommendation>
+<p>Remove any unreachable code.</p>
+</recommendation>
+
+<example>
+<p>In the following example, the final <code>return</code> statement can never be reached:</p>
+<sample src="UnreachableCodeBad.rs" />
+<p>The problem can be fixed simply by removing the unreachable code:</p>
+<sample src="UnreachableCodeGood.rs" />
+</example>
+
+<references>
+<li>Wikipedia: <a href="https://en.wikipedia.org/wiki/Unreachable_code">Unreachable code</a></li>
+</references>
+</qhelp>

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

@@ -0,0 +1,75 @@
+/**
+ * @name Unreachable code
+ * @description Code that cannot be reached should be deleted.
+ * @kind problem
+ * @problem.severity recommendation
+ * @precision medium
+ * @id rust/dead-code
+ * @tags maintainability
+ */
+
+import rust
+import codeql.rust.controlflow.ControlFlowGraph
+import codeql.rust.controlflow.internal.ControlFlowGraphImpl as ControlFlowGraphImpl
+
+/**
+ * Holds if `n` is an AST node that's unreachable.
+ */
+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, _)
+  )
+}
+
+/**
+ * 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).
+ */
+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)
+    )
+  )
+}
+
+/**
+ * Gets a node we'd prefer not to report as unreachable.
+ */
+predicate skipNode(AstNode n) {
+  n instanceof ControlFlowGraphImpl::PostOrderTree or // location is counter-intuitive
+  not n instanceof ControlFlowGraphImpl::ControlFlowTree // not expected to be reachable
+}
+
+/**
+ * Gets the `ControlFlowTree` successor of a node we'd prefer not to report.
+ */
+AstNode skipSuccessor(AstNode n) {
+  skipNode(n) and
+  ControlFlowGraphImpl::succ(n, result, _)
+}
+
+/**
+ * Gets the node `n`, skipping past any nodes we'd prefer not to report.
+ */
+AstNode skipSuccessors(AstNode n) {
+  result = skipSuccessor*(n) and
+  not skipNode(result)
+}
+
+from AstNode first, AstNode report
+where
+  firstUnreachable(first) and
+  report = skipSuccessors(first) and
+  exists(report.getFile().getRelativePath()) // in source
+select report, "This code is never reached."

rust/ql/src/queries/unusedentities/UnreachableCodeBad.rs

@@ -0,0 +1,11 @@
+fn fib(input: u32) -> u32 {
+	if (input == 0) {
+		return 0;
+	} else if (input == 1) {
+		return 1;
+	} else {
+		return fib(input - 1) + fib(input - 2);
+	}
+
+	return input; // BAD: this code is never reached
+}

rust/ql/src/queries/unusedentities/UnreachableCodeGood.rs

@@ -0,0 +1,9 @@
+fn fib(input: u32) -> u32 {
+	if (input == 0) {
+		return 0;
+	} else if (input == 1) {
+		return 1;
+	} else {
+		return fib(input - 1) + fib(input - 2);
+	}
+}

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

@@ -0,0 +1,14 @@
+| unreachable.rs:12:3:12:17 | ExprStmt | This code is never reached. |
+| unreachable.rs:20:3:20:17 | ExprStmt | This code is never reached. |
+| unreachable.rs:32:3:32:17 | ExprStmt | This code is never reached. |
+| unreachable.rs:39:3:39:17 | ExprStmt | This code is never reached. |
+| unreachable.rs:60:2:60:16 | ExprStmt | This code is never reached. |
+| unreachable.rs:101:3:101:17 | ExprStmt | This code is never reached. |
+| unreachable.rs:109:3:109:17 | ExprStmt | This code is never reached. |
+| unreachable.rs:124:2:124:16 | ExprStmt | This code is never reached. |
+| unreachable.rs:134:2:134:16 | ExprStmt | This code is never reached. |
+| unreachable.rs:141:3:141:17 | ExprStmt | This code is never reached. |
+| unreachable.rs:150:4:150:18 | ExprStmt | This code is never reached. |
+| unreachable.rs:156:3:156:17 | ExprStmt | This code is never reached. |
+| unreachable.rs:162:4:162:18 | ExprStmt | This code is never reached. |
+| unreachable.rs:165:2:165:16 | ExprStmt | This code is never reached. |

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

@@ -0,0 +1 @@
+queries/unusedentities/UnreachableCode.ql

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

@@ -114,7 +114,7 @@ fn arrays() {
     for k // SPURIOUS: unused variable [macros not yet supported]
 	in ks
 	{
-        println!("lets use {}", k);
+        println!("lets use {}", k); // [unreachable FALSE POSITIVE]
     }
 }
 
@@ -322,15 +322,21 @@ fn shadowing() -> i32 {
     }
 }
 
+// --- main ---
 fn main() {
     locals_1();
     locals_2();
     structs();
     arrays();
     statics();
+	println!("lets use result {}", parameters(1, 2, 3));
     loops();
     if_lets_matches();
     shadowing();
 
-    println!("lets use result {}", parameters(1, 2, 3));
+	unreachable_if();
+	unreachable_panic();
+	unreachable_match();
+	unreachable_loop();
+	unreachable_paren();
 }

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

@@ -0,0 +1,172 @@
+
+//fn cond() -> bool;
+//fn get_a_number() -> i32;
+
+// --- unreachable code --
+
+fn do_something() {
+}
+
+fn unreachable_if() {
+	if false {
+		do_something(); // BAD: unreachable code
+	} else {
+		do_something();
+	}
+
+	if true {
+		do_something();
+	} else {
+		do_something(); // BAD: unreachable code
+	}
+
+	let v = get_a_number();
+	if v == 1 {
+		if v != 1 {
+			do_something(); // BAD: unreachable code [NOT DETECTED]
+		}
+	}
+
+	if cond() {
+		return;
+		do_something(); // BAD: unreachable code
+	}
+
+	if cond() {
+		do_something();
+	} else {
+		return;
+		do_something(); // BAD: unreachable code
+	}
+	do_something();
+
+	if cond() {
+		let x = cond();
+
+		if (x) {
+			do_something();
+			if (!x) {
+				do_something(); // BAD: unreachable code [NOT DETECTED]
+			}
+			do_something();
+		}
+	}
+
+	if cond() {
+		return;
+	} else {
+		return;
+	}
+	do_something(); // BAD: unreachable code
+}
+
+fn unreachable_panic() {
+	if cond() {
+		do_something();
+		panic!("Oh no!!!");
+		do_something(); // BAD: unreachable code [NOT DETECTED]
+	}
+
+	if cond() {
+		do_something();
+		unimplemented!();
+		do_something(); // BAD: unreachable code [NOT DETECTED]
+	}
+
+	if cond() {
+		do_something();
+		todo!();
+		do_something(); // BAD: unreachable code [NOT DETECTED]
+	}
+
+	if cond() {
+		let mut maybe;
+
+		maybe = Some("Thing");
+		_ = maybe.unwrap(); // (safe)
+		do_something();
+
+		maybe = if cond() { Some("Other") } else { None };
+		_ = maybe.unwrap(); // (might panic)
+		do_something();
+
+		maybe = None;
+		_ = maybe.unwrap(); // (always panics)
+		do_something(); // BAD: unreachable code [NOT DETECTED]
+	}
+
+	if cond() {
+		do_something();
+		_ = false && panic!(); // does not panic due to short-circuiting
+		do_something(); // SPURIOUS: unreachable
+		_ = false || panic!();
+		do_something(); // BAD: unreachable code [NOT DETECTED]
+	}
+
+	if cond() {
+		do_something();
+		_ = true || panic!(); // does not panic due to short-circuiting
+		do_something(); // SPURIOUS: unreachable
+		_ = true && panic!();
+		do_something(); // BAD: unreachable code [NOT DETECTED]
+	}
+}
+
+fn unreachable_match() {
+	match get_a_number() {
+		1=>{
+			return;
+		}
+		_=>{
+			do_something();
+		}
+	}
+	do_something(); // [unreachable FALSE POSITIVE]
+
+	match get_a_number() {
+		1=>{
+			return;
+		}
+		_=>{
+			return;
+		}
+	}
+	do_something(); // BAD: unreachable code
+}
+
+fn unreachable_loop() {
+	loop {
+		do_something();
+		break;
+		do_something(); // BAD: unreachable code
+	}
+
+	if cond() {
+		while cond() {
+			do_something();
+		}
+
+		while false {
+			do_something(); // BAD: unreachable code
+		}
+
+		while true {
+			do_something();
+		}
+		do_something(); // BAD: unreachable code
+	}
+
+	loop {
+		if cond() {
+			return;
+			do_something(); // BAD: unreachable code
+		}
+	}
+	do_something(); // BAD: unreachable code
+	do_something();
+	do_something();
+}
+
+fn unreachable_paren() {
+	let _ = (((1)));
+}