diff --git a/rust/ql/src/queries/unusedentities/UnreachableCode.qhelp b/rust/ql/src/queries/unusedentities/UnreachableCode.qhelp new file mode 100644 index 000000000000..fef1aa71f6ba --- /dev/null +++ b/rust/ql/src/queries/unusedentities/UnreachableCode.qhelp @@ -0,0 +1,24 @@ + + + + +

This rule finds code that is never reached. Unused code should be removed to increase readability and avoid confusion.

+ + + +

Remove any unreachable code.

+ + + +

In the following example, the final return statement can never be reached:

+ +

The problem can be fixed simply by removing the unreachable code:

+ + + + +
  • Wikipedia: Unreachable code
    • +     +     diff --git a/rust/ql/src/queries/unusedentities/UnreachableCode.ql b/rust/ql/src/queries/unusedentities/UnreachableCode.ql new file mode 100644 index 000000000000..f07ab0f982a2 --- /dev/null +++ b/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." diff --git a/rust/ql/src/queries/unusedentities/UnreachableCodeBad.rs b/rust/ql/src/queries/unusedentities/UnreachableCodeBad.rs new file mode 100644 index 000000000000..a27bc0cab164 --- /dev/null +++ b/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 +} diff --git a/rust/ql/src/queries/unusedentities/UnreachableCodeGood.rs b/rust/ql/src/queries/unusedentities/UnreachableCodeGood.rs new file mode 100644 index 000000000000..0b2bde66fe95 --- /dev/null +++ b/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); + } +} diff --git a/rust/ql/test/query-tests/unusedentities/UnreachableCode.expected b/rust/ql/test/query-tests/unusedentities/UnreachableCode.expected new file mode 100644 index 000000000000..02c2998f1de2 --- /dev/null +++ b/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. | diff --git a/rust/ql/test/query-tests/unusedentities/UnreachableCode.qlref b/rust/ql/test/query-tests/unusedentities/UnreachableCode.qlref new file mode 100644 index 000000000000..f65928931a13 --- /dev/null +++ b/rust/ql/test/query-tests/unusedentities/UnreachableCode.qlref @@ -0,0 +1 @@ +queries/unusedentities/UnreachableCode.ql \ No newline at end of file diff --git a/rust/ql/test/query-tests/unusedentities/main.rs b/rust/ql/test/query-tests/unusedentities/main.rs index d7c08f05b62a..35622f7926c6 100644 --- a/rust/ql/test/query-tests/unusedentities/main.rs +++ b/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(); } diff --git a/rust/ql/test/query-tests/unusedentities/unreachable.rs b/rust/ql/test/query-tests/unusedentities/unreachable.rs new file mode 100644 index 000000000000..8a8fbcf70bdb --- /dev/null +++ b/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))); +}