Rust: Various fixes to the CFG construction

rust/ql/lib/codeql/rust/controlflow/internal/Completion.qll

@@ -36,13 +36,7 @@ class SimpleCompletion extends NormalCompletion, TSimpleCompletion {
 
   // `SimpleCompletion` is the "default" completion type, thus it is valid for
   // any node where there isn't another more specific completion type.
-  override predicate isValidFor(AstNode e) {
-    not any(Completion c).isValidForSpecific(e)
-    or
-    // A `?` expression can both proceed normally or cause an early return, so
-    // we explicitly allow the former here.
-    e instanceof TryExpr
-  }
+  override predicate isValidFor(AstNode e) { not any(Completion c).isValidForSpecific(e) }
 
   override string toString() { result = "simple" }
 }
@@ -177,6 +171,8 @@ class MatchCompletion extends TMatchCompletion, ConditionalCompletion {
   override predicate isValidForSpecific(AstNode e) {
     e instanceof Pat and
     if isExhaustiveMatch(e) then value = true else any()
+    or
+    e instanceof TryExpr and value = true
   }
 
   override MatchSuccessor getAMatchingSuccessorType() { result.getValue() = value }

rust/ql/lib/codeql/rust/controlflow/internal/ControlFlowGraphImpl.qll

@@ -147,23 +147,15 @@ class BlockExprTree extends StandardPostOrderTree, BlockExpr {
   override predicate propagatesAbnormal(AstNode child) { child = this.getChildNode(_) }
 }
 
-class BreakExprTree extends PostOrderTree, BreakExpr {
-  override predicate propagatesAbnormal(AstNode child) { child = this.getExpr() }
-
-  override predicate first(AstNode node) {
-    first(this.getExpr(), node)
-    or
-    not this.hasExpr() and node = this
-  }
+class BreakExprTree extends StandardPostOrderTree, BreakExpr {
+  override AstNode getChildNode(int i) { i = 0 and result = this.getExpr() }
 
   override predicate last(AstNode last, Completion c) { none() }
 
   override predicate succ(AstNode pred, AstNode succ, Completion c) {
-    last(super.getExpr(), pred, c) and succ = this
+    super.succ(pred, succ, c)
     or
-    pred = this and
-    c.isValidFor(pred) and
-    succ = this.getTarget()
+    pred = this and c.isValidFor(pred) and succ = this.getTarget()
   }
 }
 
@@ -325,7 +317,7 @@ class LetStmtTree extends PreOrderTree, LetStmt {
     not this.hasInitializer()
     or
     // Edge from end of initializer to pattern.
-    last(this.getInitializer(), pred, c) and first(this.getPat(), succ)
+    last(this.getInitializer(), pred, c) and first(this.getPat(), succ) and completionIsNormal(c)
     or
     // Edge from failed pattern to `else` branch.
     last(this.getPat(), pred, c) and
@@ -507,9 +499,12 @@ class MatchExprTree extends PostOrderTree instanceof MatchExpr {
     first(super.getArm(0).getPat(), succ) and
     completionIsNormal(c)
     or
-    // Edge from a failed match/guard in one arm to the beginning of the next arm.
+    // Edge from a failed pattern or guard in one arm to the beginning of the next arm.
     exists(int i |
-      last(super.getArm(i), pred, c) and
+      (
+        last(super.getArm(i).getPat(), pred, c) or
+        last(super.getArm(i).getGuard().getCondition(), pred, c)
+      ) and
       first(super.getArm(i + 1), succ) and
       c.(ConditionalCompletion).failed()
     )
@@ -521,10 +516,7 @@ class MatchExprTree extends PostOrderTree instanceof MatchExpr {
 
 class MethodCallExprTree extends StandardPostOrderTree, MethodCallExpr {
   override AstNode getChildNode(int i) {
-    i = 0 and
-    result = this.getReceiver()
-    or
-    result = this.getArgList().getArg(i + 1)
+    if i = 0 then result = this.getReceiver() else result = this.getArgList().getArg(i - 1)
   }
 }
 
@@ -605,89 +597,48 @@ class YeetExprTree extends StandardPostOrderTree instanceof YeetExpr {
  * `OrPat`s and `IdentPat`s.
  */
 module PatternTrees {
-  abstract class PreOrderPatTree extends PreOrderTree {
+  abstract class StandardPatTree extends StandardTree {
     abstract Pat getPat(int i);
 
-    private Pat getPatRanked(int i) {
+    Pat getPatRanked(int i) {
       result = rank[i + 1](Pat pat, int j | pat = this.getPat(j) | pat order by j)
     }
 
-    override predicate propagatesAbnormal(AstNode child) { child = this.getPatRanked(_) }
+    override AstNode getChildNode(int i) { result = this.getPat(i) }
+  }
 
+  abstract class PreOrderPatTree extends StandardPatTree, StandardPreOrderTree {
     override predicate succ(AstNode pred, AstNode succ, Completion c) {
-      pred = this and
-      completionIsValidFor(c, this) and
       c.(MatchCompletion).succeeded() and
-      first(this.getPatRanked(0), succ)
-      or
-      exists(int i | last(this.getPatRanked(i), pred, c) |
-        // Edge from successful pattern to the next
-        c.(MatchCompletion).succeeded() and
-        first(this.getPatRanked(i + 1), succ)
+      (
+        StandardPatTree.super.succ(pred, succ, c)
+        or
+        pred = this and first(this.getFirstChildNode(), succ) and completionIsValidFor(c, this)
       )
     }
 
     override predicate last(AstNode node, Completion c) {
-      node = this and
-      completionIsValidFor(c, this) and
-      c.(MatchCompletion).failed()
+      super.last(node, c)
       or
-      exists(int i | last(this.getPatRanked(i), node, c) |
-        c.(MatchCompletion).failed()
-        or
-        not exists(this.getPatRanked(i + 1)) and
-        completionIsNormal(c)
-      )
+      c.(MatchCompletion).failed() and
+      completionIsValidFor(c, this) and
+      (node = this or last(this.getPatRanked(_), node, c))
     }
   }
 
-  abstract class PostOrderPatTree extends PostOrderTree {
-    abstract Pat getPat(int i);
-
-    private Pat getPatRanked(int i) {
-      result = rank[i + 1](Pat pat, int j | pat = this.getPat(j) | pat order by j)
-    }
-
-    override predicate propagatesAbnormal(AstNode child) { child = this.getPatRanked(_) }
-
-    override predicate first(AstNode node) {
-      first(this.getPat(0), node)
-      or
-      not exists(this.getPat(_)) and
-      node = this
-    }
-
-    override predicate succ(AstNode pred, AstNode succ, Completion c) {
-      exists(int i | last(this.getPat(i), pred, c) |
-        // Edge from unsuccessful pattern to the next
-        c.(MatchCompletion).failed() and
-        first(this.getPat(i + 1), succ)
-        or
-        // Edge from successful pattern to this
-        c.(MatchCompletion).succeeded() and
-        succ = this
-        or
-        // Edge from last pattern to this
-        not exists(this.getPat(i + 1)) and
-        succ = this and
-        completionIsNormal(c)
-      )
-    }
-  }
+  abstract class PostOrderPatTree extends StandardPatTree, StandardPostOrderTree { }
 
   class IdentPatTree extends PostOrderPatTree, IdentPat {
     override Pat getPat(int i) { i = 0 and result = this.getPat() }
 
     override predicate last(AstNode node, Completion c) {
       super.last(node, c)
       or
-      last(this.getPat(), node, c) and
-      c.(MatchCompletion).failed()
+      last(this.getPat(), node, c) and c.(MatchCompletion).failed()
     }
 
     override predicate succ(AstNode pred, AstNode succ, Completion c) {
-      super.succ(pred, succ, c) and
-      not (succ = this and c.(MatchCompletion).failed())
+      super.succ(pred, succ, c) and c.(MatchCompletion).succeeded()
     }
   }
 
@@ -705,6 +656,14 @@ module PatternTrees {
 
   class OrPatTree extends PostOrderPatTree instanceof OrPat {
     override Pat getPat(int i) { result = OrPat.super.getPat(i) }
+
+    override predicate succ(AstNode pred, AstNode succ, Completion c) {
+      // Failed patterns advance normally between children
+      c.(MatchCompletion).failed() and super.succ(pred, succ, c)
+      or
+      // Successful pattern step to this
+      c.(MatchCompletion).succeeded() and succ = this and last(this.getPat(_), pred, c)
+    }
   }
 
   class ParenPatTree extends ControlFlowTree, ParenPat {

rust/ql/lib/codeql/rust/controlflow/internal/Scope.qll

@@ -12,6 +12,12 @@ abstract class CfgScope extends AstNode {
 }
 
 final class FunctionScope extends CfgScope, Function {
+  FunctionScope() {
+    // A function without a body corresponds to a trait method signature and
+    // should not have a CFG scope.
+    this.hasBody()
+  }
+
   override predicate scopeFirst(AstNode node) {
     first(this.(FunctionTree).getFirstChildNode(), node)
   }