Rust: Handle chained let expressions

Author: hvitved

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

@@ -7,8 +7,7 @@ private import codeql.rust.controlflow.CfgNodes
 private import codeql.rust.internal.CachedStages
 
 private predicate isPostOrder(AstNode n) {
-  n instanceof Expr and
-  not n instanceof LetExpr
+  n instanceof Expr
   or
   n instanceof OrPat
   or

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

@@ -200,8 +200,7 @@ class TypeReprTree extends LeafTree instanceof TypeRepr { }
 /**
  * Provides `ControlFlowTree`s for expressions.
  *
- * Since expressions construct values, they are modeled in post-order, except for
- * `LetExpr`s.
+ * Since expressions construct values, they are modeled in post-order.
  */
 module ExprTrees {
   class ArrayExprTree extends StandardPostOrderTree, ArrayExpr {
@@ -341,21 +340,15 @@ module ExprTrees {
       child = [super.getCondition(), super.getABranch()]
     }
 
-    private ConditionalCompletion conditionCompletion(Completion c) {
-      if super.getCondition() instanceof LetExpr
-      then result = c.(MatchCompletion)
-      else result = c.(BooleanCompletion)
-    }
-
     override predicate succ(AstNode pred, AstNode succ, Completion c) {
       // Edges from the condition to the branches
       last(super.getCondition(), pred, c) and
       (
-        first(super.getThen(), succ) and this.conditionCompletion(c).succeeded()
+        first(super.getThen(), succ) and c.(ConditionalCompletion).succeeded()
         or
-        first(super.getElse(), succ) and this.conditionCompletion(c).failed()
+        first(super.getElse(), succ) and c.(ConditionalCompletion).failed()
         or
-        not super.hasElse() and succ = this and this.conditionCompletion(c).failed()
+        not super.hasElse() and succ = this and c.(ConditionalCompletion).failed()
       )
       or
       // An edge from the then branch to the last node
@@ -401,10 +394,7 @@ module ExprTrees {
     }
   }
 
-  // `LetExpr` is a pre-order tree such that the pattern itself ends up
-  // dominating successors in the graph in the same way that patterns do in
-  // `match` expressions.
-  class LetExprTree extends StandardPreOrderTree, LetExpr {
+  class LetExprTree extends StandardPostOrderTree, LetExpr {
     override AstNode getChildNode(int i) {
       i = 0 and
       result = this.getScrutinee()
@@ -456,21 +446,15 @@ module ExprTrees {
 
     override predicate first(AstNode node) { first(super.getCondition(), node) }
 
-    private ConditionalCompletion conditionCompletion(Completion c) {
-      if super.getCondition() instanceof LetExpr
-      then result = c.(MatchCompletion)
-      else result = c.(BooleanCompletion)
-    }
-
     override predicate succ(AstNode pred, AstNode succ, Completion c) {
       super.succ(pred, succ, c)
       or
       last(super.getCondition(), pred, c) and
-      this.conditionCompletion(c).succeeded() and
+      c.(ConditionalCompletion).succeeded() and
       first(this.getLoopBody(), succ)
       or
       last(super.getCondition(), pred, c) and
-      this.conditionCompletion(c).failed() and
+      c.(ConditionalCompletion).failed() and
       succ = this
     }
   }

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

@@ -71,13 +71,7 @@ module ConditionalCompletionSplitting {
       child = parent.(LogicalNotExpr).getExpr() and
       childCompletion.getDual() = parentCompletion
       or
-      (
-        childCompletion = parentCompletion
-        or
-        // needed for `let` expressions
-        childCompletion.(MatchCompletion).getValue() =
-          parentCompletion.(BooleanCompletion).getValue()
-      ) and
+      childCompletion = parentCompletion and
       (
         child = parent.(BinaryLogicalOperation).getAnOperand()
         or
@@ -92,6 +86,9 @@ module ConditionalCompletionSplitting {
         or
         child = parent.(PatternTrees::PostOrderPatTree).getPat(_)
       )
+      or
+      child = parent.(LetExpr).getPat() and
+      childCompletion.(MatchCompletion).getValue() = parentCompletion.(BooleanCompletion).getValue()
     }
   }
 

rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll

@@ -241,6 +241,12 @@ module LocalFlow {
       nodeTo.getCfgNode() = s.getPat()
     )
     or
+    // An edge from the right-hand side of a let expression to the left-hand side.
+    exists(LetExprCfgNode e |
+      nodeFrom.getCfgNode() = e.getScrutinee() and
+      nodeTo.getCfgNode() = e.getPat()
+    )
+    or
     exists(IdentPatCfgNode p |
       not p.isRef() and
       nodeFrom.getCfgNode() = p and
@@ -379,6 +385,8 @@ module RustDataFlow implements InputSig<Location> {
   predicate neverSkipInPathGraph(Node node) {
     node.(Node::Node).getCfgNode() = any(LetStmtCfgNode s).getPat()
     or
+    node.(Node::Node).getCfgNode() = any(LetExprCfgNode e).getPat()
+    or
     node.(Node::Node).getCfgNode() = any(AssignmentExprCfgNode a).getLhs()
     or
     exists(MatchExprCfgNode match |
@@ -899,6 +907,12 @@ module VariableCapture {
           v.getPat() = ls.getPat().getPat() and
           ls.getInitializer() = source
         )
+        or
+        exists(LetExprCfgNode le |
+          this = le and
+          v.getPat() = le.getPat().getPat() and
+          le.getScrutinee() = source
+        )
       }
 
       CapturedVariable getVariable() { result = v }

rust/ql/lib/codeql/rust/elements/internal/VariableImpl.qll

@@ -152,8 +152,14 @@ module Impl {
     /** Gets the `let` statement that introduces this variable, if any. */
     LetStmt getLetStmt() { this.getPat() = result.getPat() }
 
+    /** Gets the `let` expression that introduces this variable, if any. */
+    LetExpr getLetExpr() { this.getPat() = result.getPat() }
+
     /** Gets the initial value of this variable, if any. */
-    Expr getInitializer() { result = this.getLetStmt().getInitializer() }
+    Expr getInitializer() {
+      result = this.getLetStmt().getInitializer() or
+      result = this.getLetExpr().getScrutinee()
+    }
 
     /** Holds if this variable is captured. */
     predicate isCaptured() { this.getAnAccess().isCapture() }
@@ -198,6 +204,7 @@ module Impl {
       n instanceof Pat or
       n instanceof VariableAccessCand or
       n instanceof LetStmt or
+      n instanceof LetExpr or
       n instanceof VariableScope
     ) and
     exists(AstNode n0 |
@@ -243,6 +250,7 @@ module Impl {
       this instanceof VariableScope or
       this instanceof VariableAccessCand or
       this instanceof LetStmt or
+      this instanceof LetExpr or
       getImmediateChild(this, _) instanceof RelevantElement
     }
 
@@ -354,6 +362,14 @@ module Impl {
           ord = getLastPreOrderNumbering(scope, let) + 1
         )
         or
+        exists(LetExpr let |
+          let.getPat() = pat and
+          scope = getEnclosingScope(let) and
+          // for `let` expressions, variables are bound _after_ the statement, i.e.
+          // not in the RHS
+          ord = getLastPreOrderNumbering(scope, let) + 1
+        )
+        or
         exists(IfExpr ie, LetExpr let |
           let.getPat() = pat and
           ie.getCondition() = let and

rust/ql/test/library-tests/controlflow-unstable/Cfg.expected

@@ -13,13 +13,14 @@ edges
 | test.rs:5:67:11:5 | { ... } | test.rs:5:5:11:5 | exit fn test_and_if_let (normal) |  |
 | test.rs:6:9:10:9 | if ... {...} else {...} | test.rs:5:67:11:5 | { ... } |  |
 | test.rs:6:12:6:12 | a | test.rs:6:12:6:31 | [boolean(false)] ... && ... | false |
-| test.rs:6:12:6:12 | a | test.rs:6:17:6:31 | let ... = b | true |
+| test.rs:6:12:6:12 | a | test.rs:6:31:6:31 | b | true |
 | test.rs:6:12:6:31 | [boolean(false)] ... && ... | test.rs:9:13:9:17 | false | false |
 | test.rs:6:12:6:31 | [boolean(true)] ... && ... | test.rs:7:13:7:13 | d | true |
-| test.rs:6:17:6:31 | let ... = b | test.rs:6:31:6:31 | b |  |
-| test.rs:6:21:6:27 | Some(...) | test.rs:6:12:6:31 | [boolean(false)] ... && ... | no-match |
+| test.rs:6:17:6:31 | [boolean(false)] let ... = b | test.rs:6:12:6:31 | [boolean(false)] ... && ... | false |
+| test.rs:6:17:6:31 | [boolean(true)] let ... = b | test.rs:6:12:6:31 | [boolean(true)] ... && ... | true |
+| test.rs:6:21:6:27 | Some(...) | test.rs:6:17:6:31 | [boolean(false)] let ... = b | no-match |
 | test.rs:6:21:6:27 | Some(...) | test.rs:6:26:6:26 | d | match |
-| test.rs:6:26:6:26 | d | test.rs:6:12:6:31 | [boolean(true)] ... && ... | match |
+| test.rs:6:26:6:26 | d | test.rs:6:17:6:31 | [boolean(true)] let ... = b | match |
 | test.rs:6:26:6:26 | d | test.rs:6:26:6:26 | d |  |
 | test.rs:6:31:6:31 | b | test.rs:6:21:6:27 | Some(...) |  |
 | test.rs:6:33:8:9 | { ... } | test.rs:6:9:10:9 | if ... {...} else {...} |  |
@@ -40,13 +41,13 @@ edges
 | test.rs:13:59:21:5 | { ... } | test.rs:13:5:21:5 | exit fn test_and_if_let2 (normal) |  |
 | test.rs:14:9:20:9 | if ... {...} else {...} | test.rs:13:59:21:5 | { ... } |  |
 | test.rs:14:12:14:12 | a | test.rs:14:12:14:25 | [boolean(false)] ... && ... | false |
-| test.rs:14:12:14:12 | a | test.rs:14:17:14:25 | let ... = b | true |
+| test.rs:14:12:14:12 | a | test.rs:14:25:14:25 | b | true |
 | test.rs:14:12:14:25 | [boolean(false)] ... && ... | test.rs:14:12:15:16 | [boolean(false)] ... && ... | false |
 | test.rs:14:12:14:25 | [boolean(true)] ... && ... | test.rs:15:16:15:16 | c | true |
 | test.rs:14:12:15:16 | [boolean(false)] ... && ... | test.rs:19:13:19:17 | false | false |
 | test.rs:14:12:15:16 | [boolean(true)] ... && ... | test.rs:17:13:17:13 | d | true |
-| test.rs:14:17:14:25 | let ... = b | test.rs:14:25:14:25 | b |  |
-| test.rs:14:21:14:21 | d | test.rs:14:12:14:25 | [boolean(true)] ... && ... | match |
+| test.rs:14:17:14:25 | [boolean(true)] let ... = b | test.rs:14:12:14:25 | [boolean(true)] ... && ... | true |
+| test.rs:14:21:14:21 | d | test.rs:14:17:14:25 | [boolean(true)] let ... = b | match |
 | test.rs:14:21:14:21 | d | test.rs:14:21:14:21 | d |  |
 | test.rs:14:25:14:25 | b | test.rs:14:21:14:21 | d |  |
 | test.rs:15:16:15:16 | c | test.rs:14:12:15:16 | [boolean(false)] ... && ... | false |