Rust: Address PR review comments

github · Sep 19, 2024 · 2511986 · 2511986
1 parent bbf5902
commit 2511986
Show file tree

Hide file tree

Showing 6 changed files with 78 additions and 63 deletions.
diff --git a/rust/ql/lib/codeql/rust/controlflow/internal/Completion.qll b/rust/ql/lib/codeql/rust/controlflow/internal/Completion.qll
@@ -1,4 +1,3 @@
-private import codeql.util.Option
 private import codeql.util.Boolean
 private import codeql.rust.controlflow.ControlFlowGraph
 private import rust
@@ -8,9 +7,14 @@ private newtype TCompletion =
   TSimpleCompletion() or
   TBooleanCompletion(Boolean b) or
   TMatchCompletion(Boolean isMatch) or
-  TLoopCompletion(TLoopJumpType kind, TLabelType label) or
-  TReturnCompletion() or
-  TDivergeCompletion() // A completion that never reaches the successor (e.g. by panicking or spinning)
+  TLoopCompletion(TLoopJumpType kind, TLabelType label) {
+    label = TNoLabel()
+    or
+    kind = TBreakJump() and label = TLabel(any(BreakExpr b).getLabel().getName())
+    or
+    kind = TContinueJump() and label = TLabel(any(ContinueExpr b).getLabel().getName())
+  } or
+  TReturnCompletion()
 
 /** A completion of a statement or an expression. */
 abstract class Completion extends TCompletion {

diff --git a/rust/ql/lib/codeql/rust/controlflow/internal/ControlFlowGraphImpl.qll b/rust/ql/lib/codeql/rust/controlflow/internal/ControlFlowGraphImpl.qll
@@ -55,11 +55,11 @@ module CfgInput implements InputSig<Location> {
   predicate scopeLast(CfgScope scope, AstNode last, Completion c) { scope.scopeLast(last, c) }
 }
 
-module CfgImpl = Make<Location, CfgInput>;
+private module CfgImpl = Make<Location, CfgInput>;
 
 import CfgImpl
 
-/** Holds for a trivial pattern that is always guaranteed to match. */
+/** Holds if `p` is a trivial pattern that is always guaranteed to match. */
 predicate trivialPat(Pat p) { p instanceof WildcardPat or p instanceof IdentPat }
 
 class AsmExprTree extends LeafTree instanceof AsmExpr { }
@@ -90,7 +90,6 @@ class LogicalOrBinaryOpExprTree extends PreOrderTree instanceof BinaryExpr {
     child = [super.getRhs(), super.getLhs()]
   }
 
-  // override predicate first(AstNode node) { first(super.getLhs(), node) }
   override predicate succ(AstNode pred, AstNode succ, Completion c) {
     // Edge to the first node in the lhs
     pred = this and
@@ -109,7 +108,7 @@ class LogicalOrBinaryOpExprTree extends PreOrderTree instanceof BinaryExpr {
     c.(BooleanCompletion).succeeded()
     or
     // Rhs. as the last node
-    last(super.getRhs(), node, c) // and
+    last(super.getRhs(), node, c)
   }
 }
 
@@ -120,7 +119,6 @@ class LogicalAndBinaryOpExprTree extends PreOrderTree instanceof BinaryExpr {
     child = [super.getRhs(), super.getLhs()]
   }
 
-  // override predicate first(AstNode node) { first(super.getLhs(), node) }
   override predicate succ(AstNode pred, AstNode succ, Completion c) {
     // Edge to the first node in the lhs
     pred = this and
@@ -283,15 +281,15 @@ class LetStmtTree extends PreOrderTree instanceof LetStmt {
     pred = this and first(super.getInitializer(), succ) and completionIsSimple(c)
     or
     // Edge from end of initializer to pattern.
-    last(super.getInitializer(), pred, c) and succ = super.getPat()
+    last(super.getInitializer(), pred, c) and first(super.getPat(), succ)
     or
     // Edge from failed pattern to `else` branch.
-    pred = super.getPat() and first(super.getElse(), succ) and c.(MatchCompletion).failed()
+    last(super.getPat(), pred, c) and first(super.getElse(), succ) and c.(MatchCompletion).failed()
   }
 
   override predicate last(AstNode node, Completion c) {
     // Edge out of a successfully matched pattern.
-    node = super.getPat() and c.(MatchCompletion).succeeded()
+    last(super.getPat(), node, c) and c.(MatchCompletion).succeeded()
     // NOTE: No edge out of the `else` branch as that is guaranteed to diverge.
   }
 }
@@ -304,7 +302,7 @@ class LoopExprTree extends PostOrderTree instanceof LoopExpr {
   override predicate first(AstNode node) { first(super.getBody(), node) }
 
   /** Whether this `LoopExpr` captures the `c` completion. */
-  predicate capturesLoopJumpCompletion(LoopJumpCompletion c) {
+  private predicate capturesLoopJumpCompletion(LoopJumpCompletion c) {
     not c.hasLabel()
     or
     c.getLabelName() = super.getLabel().getName()
@@ -346,7 +344,11 @@ class MatchArmTree extends ControlFlowTree instanceof MatchArm {
     // Edge from pattern to guard/arm if match succeeds.
     pred = super.getPat() and
     c.(MatchCompletion).succeeded() and
-    (if super.hasGuard() then first(super.getGuard(), succ) else first(super.getExpr(), succ))
+    (
+      first(super.getGuard(), succ)
+      or
+      not super.hasGuard() and first(super.getExpr(), succ)
+    )
     or
     // Edge from guard to arm if the guard succeeds.
     last(super.getGuard(), pred, c) and
@@ -364,7 +366,9 @@ class MatchArmTree extends ControlFlowTree instanceof MatchArm {
 }
 
 class MatchExprTree extends PostOrderTree instanceof MatchExpr {
-  override predicate propagatesAbnormal(AstNode child) { child = super.getABranch().getExpr() }
+  override predicate propagatesAbnormal(AstNode child) {
+    child = [super.getExpr(), super.getABranch().getExpr()]
+  }
 
   override predicate first(AstNode node) { first(super.getExpr(), node) }
 
@@ -380,7 +384,7 @@ class MatchExprTree extends PostOrderTree instanceof MatchExpr {
     )
     or
     // Edge from the end of each arm to the match expression.
-    last(super.getBranch(_), pred, c) and succ = this and completionIsSimple(c)
+    last(super.getBranch(_), pred, c) and succ = this and completionIsNormal(c)
   }
 }
 
@@ -436,7 +440,7 @@ class ReturnExprTree extends PostOrderTree instanceof ReturnExpr {
   }
 
   override predicate succ(AstNode pred, AstNode succ, Completion c) {
-    last(super.getExpr(), pred, c) and succ = this
+    last(super.getExpr(), pred, c) and succ = this and completionIsNormal(c)
   }
 }
 

diff --git a/rust/ql/lib/codeql/rust/controlflow/internal/Scope.qll b/rust/ql/lib/codeql/rust/controlflow/internal/Scope.qll
@@ -17,7 +17,7 @@ final class FunctionScope extends CfgScope, Function {
   override predicate scopeLast(AstNode node, Completion c) { last(this.getBody(), node, c) }
 }
 
-final class LambdaScope extends CfgScope, ClosureExpr {
+final class ClosureScope extends CfgScope, ClosureExpr {
   override predicate scopeFirst(AstNode node) { first(this.getBody(), node) }
 
   override predicate scopeLast(AstNode node, Completion c) { last(this.getBody(), node, c) }

diff --git a/rust/ql/lib/codeql/rust/controlflow/internal/SuccessorType.qll b/rust/ql/lib/codeql/rust/controlflow/internal/SuccessorType.qll
@@ -17,7 +17,6 @@ newtype TSuccessorType =
   TLoopSuccessor(TLoopJumpType kind, TLabelType label) or
   TReturnSuccessor()
 
-// class TBreakSuccessor = TUnlabeledBreakSuccessor or TLabeledBreakSuccessor;
 /** The type of a control flow successor. */
 abstract private class SuccessorTypeImpl extends TSuccessorType {
   /** Gets a textual representation of successor type. */

diff --git a/rust/ql/test/library-tests/controlflow/Cfg.expected b/rust/ql/test/library-tests/controlflow/Cfg.expected
@@ -192,17 +192,17 @@ nodes
 | test.rs:106:44:112:1 | BlockExpr | semmle.order | 191 |
 | test.rs:107:5:111:5 | MatchExpr | semmle.order | 192 |
 | test.rs:107:11:107:21 | PathExpr | semmle.order | 193 |
-| test.rs:108:9:108:15 | TupleStructPat | semmle.order | 194 |
-| test.rs:108:20:108:20 | PathExpr | semmle.order | 195 |
-| test.rs:108:20:108:25 | BinaryExpr | semmle.order | 196 |
-| test.rs:108:24:108:25 | LiteralExpr | semmle.order | 197 |
-| test.rs:108:30:108:30 | PathExpr | semmle.order | 198 |
-| test.rs:108:30:108:34 | BinaryExpr | semmle.order | 199 |
-| test.rs:108:34:108:34 | LiteralExpr | semmle.order | 200 |
-| test.rs:109:9:109:15 | TupleStructPat | semmle.order | 201 |
-| test.rs:109:20:109:20 | PathExpr | semmle.order | 202 |
-| test.rs:110:9:110:12 | IdentPat | semmle.order | 203 |
-| test.rs:110:17:110:17 | LiteralExpr | semmle.order | 204 |
+| test.rs:108:9:108:23 | TupleStructPat | semmle.order | 194 |
+| test.rs:108:28:108:28 | PathExpr | semmle.order | 195 |
+| test.rs:108:28:108:33 | BinaryExpr | semmle.order | 196 |
+| test.rs:108:32:108:33 | LiteralExpr | semmle.order | 197 |
+| test.rs:108:38:108:38 | PathExpr | semmle.order | 198 |
+| test.rs:108:38:108:42 | BinaryExpr | semmle.order | 199 |
+| test.rs:108:42:108:42 | LiteralExpr | semmle.order | 200 |
+| test.rs:109:9:109:23 | TupleStructPat | semmle.order | 201 |
+| test.rs:109:28:109:28 | PathExpr | semmle.order | 202 |
+| test.rs:110:9:110:20 | PathPat | semmle.order | 203 |
+| test.rs:110:25:110:25 | LiteralExpr | semmle.order | 204 |
 | test.rs:115:5:120:5 | enter test_infinite_loop | semmle.order | 205 |
 | test.rs:116:9:118:9 | ExprStmt | semmle.order | 206 |
 | test.rs:116:14:118:9 | BlockExpr | semmle.order | 207 |
@@ -618,36 +618,44 @@ edges
 | test.rs:106:44:112:1 | BlockExpr | test.rs:106:1:112:1 | exit test_match (normal) | semmle.order | 1 |
 | test.rs:107:5:111:5 | MatchExpr | test.rs:106:44:112:1 | BlockExpr | semmle.label |  |
 | test.rs:107:5:111:5 | MatchExpr | test.rs:106:44:112:1 | BlockExpr | semmle.order | 1 |
-| test.rs:107:11:107:21 | PathExpr | test.rs:108:9:108:15 | TupleStructPat | semmle.label |  |
-| test.rs:107:11:107:21 | PathExpr | test.rs:108:9:108:15 | TupleStructPat | semmle.order | 1 |
-| test.rs:108:9:108:15 | TupleStructPat | test.rs:108:20:108:20 | PathExpr | semmle.label | match |
-| test.rs:108:9:108:15 | TupleStructPat | test.rs:108:20:108:20 | PathExpr | semmle.order | 1 |
-| test.rs:108:9:108:15 | TupleStructPat | test.rs:109:9:109:15 | TupleStructPat | semmle.label | no-match |
-| test.rs:108:9:108:15 | TupleStructPat | test.rs:109:9:109:15 | TupleStructPat | semmle.order | 2 |
-| test.rs:108:20:108:20 | PathExpr | test.rs:108:24:108:25 | LiteralExpr | semmle.label |  |
-| test.rs:108:20:108:20 | PathExpr | test.rs:108:24:108:25 | LiteralExpr | semmle.order | 1 |
-| test.rs:108:20:108:25 | BinaryExpr | test.rs:108:30:108:30 | PathExpr | semmle.label | true |
-| test.rs:108:20:108:25 | BinaryExpr | test.rs:108:30:108:30 | PathExpr | semmle.order | 1 |
-| test.rs:108:20:108:25 | BinaryExpr | test.rs:109:9:109:15 | TupleStructPat | semmle.label | false |
-| test.rs:108:20:108:25 | BinaryExpr | test.rs:109:9:109:15 | TupleStructPat | semmle.order | 2 |
-| test.rs:108:24:108:25 | LiteralExpr | test.rs:108:20:108:25 | BinaryExpr | semmle.label |  |
-| test.rs:108:24:108:25 | LiteralExpr | test.rs:108:20:108:25 | BinaryExpr | semmle.order | 1 |
-| test.rs:108:30:108:30 | PathExpr | test.rs:108:34:108:34 | LiteralExpr | semmle.label |  |
-| test.rs:108:30:108:30 | PathExpr | test.rs:108:34:108:34 | LiteralExpr | semmle.order | 1 |
-| test.rs:108:30:108:34 | BinaryExpr | test.rs:107:5:111:5 | MatchExpr | semmle.label |  |
-| test.rs:108:30:108:34 | BinaryExpr | test.rs:107:5:111:5 | MatchExpr | semmle.order | 1 |
-| test.rs:108:34:108:34 | LiteralExpr | test.rs:108:30:108:34 | BinaryExpr | semmle.label |  |
-| test.rs:108:34:108:34 | LiteralExpr | test.rs:108:30:108:34 | BinaryExpr | semmle.order | 1 |
-| test.rs:109:9:109:15 | TupleStructPat | test.rs:109:20:109:20 | PathExpr | semmle.label | match |
-| test.rs:109:9:109:15 | TupleStructPat | test.rs:109:20:109:20 | PathExpr | semmle.order | 1 |
-| test.rs:109:9:109:15 | TupleStructPat | test.rs:110:9:110:12 | IdentPat | semmle.label | no-match |
-| test.rs:109:9:109:15 | TupleStructPat | test.rs:110:9:110:12 | IdentPat | semmle.order | 2 |
-| test.rs:109:20:109:20 | PathExpr | test.rs:107:5:111:5 | MatchExpr | semmle.label |  |
-| test.rs:109:20:109:20 | PathExpr | test.rs:107:5:111:5 | MatchExpr | semmle.order | 1 |
-| test.rs:110:9:110:12 | IdentPat | test.rs:110:17:110:17 | LiteralExpr | semmle.label | match |
-| test.rs:110:9:110:12 | IdentPat | test.rs:110:17:110:17 | LiteralExpr | semmle.order | 1 |
-| test.rs:110:17:110:17 | LiteralExpr | test.rs:107:5:111:5 | MatchExpr | semmle.label |  |
-| test.rs:110:17:110:17 | LiteralExpr | test.rs:107:5:111:5 | MatchExpr | semmle.order | 1 |
+| test.rs:107:11:107:21 | PathExpr | test.rs:108:9:108:23 | TupleStructPat | semmle.label |  |
+| test.rs:107:11:107:21 | PathExpr | test.rs:108:9:108:23 | TupleStructPat | semmle.order | 1 |
+| test.rs:108:9:108:23 | TupleStructPat | test.rs:107:5:111:5 | MatchExpr | semmle.label | no-match |
+| test.rs:108:9:108:23 | TupleStructPat | test.rs:107:5:111:5 | MatchExpr | semmle.order | 1 |
+| test.rs:108:9:108:23 | TupleStructPat | test.rs:108:28:108:28 | PathExpr | semmle.label | match |
+| test.rs:108:9:108:23 | TupleStructPat | test.rs:108:28:108:28 | PathExpr | semmle.order | 2 |
+| test.rs:108:9:108:23 | TupleStructPat | test.rs:109:9:109:23 | TupleStructPat | semmle.label | no-match |
+| test.rs:108:9:108:23 | TupleStructPat | test.rs:109:9:109:23 | TupleStructPat | semmle.order | 3 |
+| test.rs:108:28:108:28 | PathExpr | test.rs:108:32:108:33 | LiteralExpr | semmle.label |  |
+| test.rs:108:28:108:28 | PathExpr | test.rs:108:32:108:33 | LiteralExpr | semmle.order | 1 |
+| test.rs:108:28:108:33 | BinaryExpr | test.rs:107:5:111:5 | MatchExpr | semmle.label | false |
+| test.rs:108:28:108:33 | BinaryExpr | test.rs:107:5:111:5 | MatchExpr | semmle.order | 1 |
+| test.rs:108:28:108:33 | BinaryExpr | test.rs:108:38:108:38 | PathExpr | semmle.label | true |
+| test.rs:108:28:108:33 | BinaryExpr | test.rs:108:38:108:38 | PathExpr | semmle.order | 2 |
+| test.rs:108:28:108:33 | BinaryExpr | test.rs:109:9:109:23 | TupleStructPat | semmle.label | false |
+| test.rs:108:28:108:33 | BinaryExpr | test.rs:109:9:109:23 | TupleStructPat | semmle.order | 3 |
+| test.rs:108:32:108:33 | LiteralExpr | test.rs:108:28:108:33 | BinaryExpr | semmle.label |  |
+| test.rs:108:32:108:33 | LiteralExpr | test.rs:108:28:108:33 | BinaryExpr | semmle.order | 1 |
+| test.rs:108:38:108:38 | PathExpr | test.rs:108:42:108:42 | LiteralExpr | semmle.label |  |
+| test.rs:108:38:108:38 | PathExpr | test.rs:108:42:108:42 | LiteralExpr | semmle.order | 1 |
+| test.rs:108:38:108:42 | BinaryExpr | test.rs:107:5:111:5 | MatchExpr | semmle.label |  |
+| test.rs:108:38:108:42 | BinaryExpr | test.rs:107:5:111:5 | MatchExpr | semmle.order | 1 |
+| test.rs:108:42:108:42 | LiteralExpr | test.rs:108:38:108:42 | BinaryExpr | semmle.label |  |
+| test.rs:108:42:108:42 | LiteralExpr | test.rs:108:38:108:42 | BinaryExpr | semmle.order | 1 |
+| test.rs:109:9:109:23 | TupleStructPat | test.rs:107:5:111:5 | MatchExpr | semmle.label | no-match |
+| test.rs:109:9:109:23 | TupleStructPat | test.rs:107:5:111:5 | MatchExpr | semmle.order | 1 |
+| test.rs:109:9:109:23 | TupleStructPat | test.rs:109:28:109:28 | PathExpr | semmle.label | match |
+| test.rs:109:9:109:23 | TupleStructPat | test.rs:109:28:109:28 | PathExpr | semmle.order | 2 |
+| test.rs:109:9:109:23 | TupleStructPat | test.rs:110:9:110:20 | PathPat | semmle.label | no-match |
+| test.rs:109:9:109:23 | TupleStructPat | test.rs:110:9:110:20 | PathPat | semmle.order | 3 |
+| test.rs:109:28:109:28 | PathExpr | test.rs:107:5:111:5 | MatchExpr | semmle.label |  |
+| test.rs:109:28:109:28 | PathExpr | test.rs:107:5:111:5 | MatchExpr | semmle.order | 1 |
+| test.rs:110:9:110:20 | PathPat | test.rs:107:5:111:5 | MatchExpr | semmle.label | no-match |
+| test.rs:110:9:110:20 | PathPat | test.rs:107:5:111:5 | MatchExpr | semmle.order | 1 |
+| test.rs:110:9:110:20 | PathPat | test.rs:110:25:110:25 | LiteralExpr | semmle.label | match |
+| test.rs:110:9:110:20 | PathPat | test.rs:110:25:110:25 | LiteralExpr | semmle.order | 2 |
+| test.rs:110:25:110:25 | LiteralExpr | test.rs:107:5:111:5 | MatchExpr | semmle.label |  |
+| test.rs:110:25:110:25 | LiteralExpr | test.rs:107:5:111:5 | MatchExpr | semmle.order | 1 |
 | test.rs:115:5:120:5 | enter test_infinite_loop | test.rs:116:9:118:9 | ExprStmt | semmle.label |  |
 | test.rs:115:5:120:5 | enter test_infinite_loop | test.rs:116:9:118:9 | ExprStmt | semmle.order | 1 |
 | test.rs:116:9:118:9 | ExprStmt | test.rs:117:13:117:13 | LiteralExpr | semmle.label |  |

diff --git a/rust/ql/test/library-tests/controlflow/test.rs b/rust/ql/test/library-tests/controlflow/test.rs
@@ -105,9 +105,9 @@ mod logical_operators {
 
 fn test_match(maybe_digit: Option<i64>) -> {
     match maybe_digit {
-        Some(x) if x < 10 => x + 5,
-        Some(x) => x,
-        None => 5,
+        Option::Some(x) if x < 10 => x + 5,
+        Option::Some(x) => x,
+        Option::None => 5,
     }
 }