Rust: Address PR comments

Author: paldepind

rust/ql/lib/codeql/rust/dataflow/Ssa.qll

@@ -214,7 +214,7 @@ module Ssa {
      * variable.
      *
      * This is either the value in a direct assignment, `x = value`, or in a
-     * `let` statement, `let x = value`. Note that patterns on the rhs. are
+     * `let` statement, `let x = value`. Note that patterns on the lhs. are
      * currently not supported.
      */
     predicate assigns(ExprCfgNode value) {
@@ -337,37 +337,4 @@ module Ssa {
 
     override Location getLocation() { result = this.getBasicBlock().getLocation() }
   }
-
-  /**
-   * An SSA definition inserted at a call that may update the value of a captured
-   * variable. For example, in
-   *
-   * ```rust
-   * fn capture_mut() {
-   *   let mut y = 0;
-   *   (0..5).for_each(|| {
-   *     y += x
-   *   });
-   *   y
-   * }
-   * ```
-   *
-   * a definition for `y` is inserted at the call to `for_each`.
-   */
-  class CapturedCallDefinition extends Definition, SsaImpl::UncertainWriteDefinition {
-    CapturedCallDefinition() {
-      exists(Variable v, BasicBlock bb, int i |
-        this.definesAt(v, bb, i) and
-        SsaImpl::capturedCallWrite(_, bb, i, v)
-      )
-    }
-
-    /**
-     * Gets the immediately preceding definition. Since this update is uncertain,
-     * the value from the preceding definition might still be valid.
-     */
-    final Definition getPriorDefinition() { result = SsaImpl::uncertainWriteDefinitionInput(this) }
-
-    override string toString() { result = "<captured exit> " + this.getSourceVariable() }
-  }
 }

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

@@ -102,18 +102,18 @@ final class ParameterPosition extends TParameterPosition {
   predicate isSelf() { this = TSelfParameterPosition() }
 
   /**
-   * Holds if this position represents a reference to a lambda itself. Only
+   * Holds if this position represents a reference to a closure itself. Only
    * used for tracking flow through captured variables.
    */
-  predicate isLambdaSelf() { this = TLambdaSelfParameterPosition() }
+  predicate isClosureSelf() { this = TClosureSelfParameterPosition() }
 
   /** Gets a textual representation of this position. */
   string toString() {
     result = this.getPosition().toString()
     or
     result = "self" and this.isSelf()
     or
-    result = "lambda self" and this.isLambdaSelf()
+    result = "closure self" and this.isClosureSelf()
   }
 
   ParamBase getParameterIn(ParamList ps) {
@@ -276,15 +276,15 @@ module Node {
    * The run-time representation of a closure itself at function entry, viewed
    * as a node in a data flow graph.
    */
-  final class ClosureParameterNode extends ParameterNode, TLambdaSelfReferenceNode {
+  final class ClosureParameterNode extends ParameterNode, TClosureSelfReferenceNode {
     private CfgScope cfgScope;
 
-    ClosureParameterNode() { this = TLambdaSelfReferenceNode(cfgScope) }
+    ClosureParameterNode() { this = TClosureSelfReferenceNode(cfgScope) }
 
     final override CfgScope getCfgScope() { result = cfgScope }
 
     override predicate isParameterOf(DataFlowCallable c, ParameterPosition pos) {
-      cfgScope = c.asCfgScope() and pos.isLambdaSelf()
+      cfgScope = c.asCfgScope() and pos.isClosureSelf()
     }
 
     override Location getLocation() { result = cfgScope.getLocation() }
@@ -331,7 +331,7 @@ module Node {
 
     override predicate isArgumentOf(DataFlowCall call, RustDataFlow::ArgumentPosition pos) {
       call.asCallExprCfgNode() = call_ and
-      pos.isLambdaSelf()
+      pos.isClosureSelf()
     }
   }
 
@@ -403,6 +403,24 @@ module Node {
     override DataFlowCall getCall(ReturnKind kind) { result = call and kind = kind_ }
   }
 
+  /**
+   * A synthesized data flow node representing a closure object that tracks
+   * captured variables.
+   */
+  class CaptureNode extends Node, TCaptureNode {
+    private VariableCapture::Flow::SynthesizedCaptureNode cn;
+
+    CaptureNode() { this = TCaptureNode(cn) }
+
+    VariableCapture::Flow::SynthesizedCaptureNode getSynthesizedCaptureNode() { result = cn }
+
+    override CfgScope getCfgScope() { result = cn.getEnclosingCallable() }
+
+    override Location getLocation() { result = cn.getLocation() }
+
+    override string toString() { result = cn.toString() }
+  }
+
   /**
    * A node associated with an object after an operation that might have
    * changed its state.
@@ -458,24 +476,6 @@ module Node {
     final override string toString() { result = PostUpdateNode.super.toString() }
   }
 
-  /**
-   * A synthesized data flow node representing a closure object that tracks
-   * captured variables.
-   */
-  class CaptureNode extends Node, TCaptureNode {
-    private VariableCapture::Flow::SynthesizedCaptureNode cn;
-
-    CaptureNode() { this = TCaptureNode(cn) }
-
-    VariableCapture::Flow::SynthesizedCaptureNode getSynthesizedCaptureNode() { result = cn }
-
-    override CfgScope getCfgScope() { result = cn.getEnclosingCallable() }
-
-    override Location getLocation() { result = cn.getLocation() }
-
-    override string toString() { result = cn.toString() }
-  }
-
   final class CastNode = NaNode;
 }
 
@@ -847,8 +847,6 @@ module RustDataFlow implements InputSig<Location> {
     node instanceof Node::CaptureNode
     or
     node instanceof Node::ClosureParameterNode
-    or
-    node instanceof Node::ClosureArgumentNode
   }
 
   class DataFlowExpr = ExprCfgNode;
@@ -1383,7 +1381,7 @@ private module Cached {
     } or
     TSsaNode(SsaImpl::DataFlowIntegration::SsaNode node) or
     TFlowSummaryNode(FlowSummaryImpl::Private::SummaryNode sn) or
-    TLambdaSelfReferenceNode(CfgScope c) { lambdaCreationExpr(c, _) } or
+    TClosureSelfReferenceNode(CfgScope c) { lambdaCreationExpr(c, _) } or
     TCaptureNode(VariableCapture::Flow::SynthesizedCaptureNode cn)
 
   cached
@@ -1421,7 +1419,7 @@ private module Cached {
       or
       FlowSummaryImpl::ParsePositions::isParsedParameterPosition(_, i)
     } or
-    TLambdaSelfParameterPosition() or
+    TClosureSelfParameterPosition() or
     TSelfParameterPosition()
 
   cached

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

@@ -467,6 +467,39 @@ class PhiReadNode extends DefinitionExt, Impl::PhiReadNode {
   override Location getLocation() { result = Impl::PhiReadNode.super.getLocation() }
 }
 
+/**
+ * An SSA definition inserted at a call that may update the value of a captured
+ * variable. For example, in
+ *
+ * ```rust
+ * fn capture_mut() {
+ *   let mut y = 0;
+ *   (0..5).for_each(|x| {
+ *     y += x
+ *   });
+ *   y
+ * }
+ * ```
+ *
+ * a definition for `y` is inserted at the call to `for_each`.
+ */
+class CapturedCallDefinition extends Definition, Impl::UncertainWriteDefinition {
+  CapturedCallDefinition() {
+    exists(Variable v, BasicBlock bb, int i |
+      this.definesAt(v, bb, i) and
+      capturedCallWrite(_, bb, i, v)
+    )
+  }
+
+  /**
+   * Gets the immediately preceding definition. Since this update is uncertain,
+   * the value from the preceding definition might still be valid.
+   */
+  final Definition getPriorDefinition() { result = uncertainWriteDefinitionInput(this) }
+
+  override string toString() { result = "<captured exit> " + this.getSourceVariable() }
+}
+
 private module DataFlowIntegrationInput implements Impl::DataFlowIntegrationInputSig {
   class Expr extends CfgNodes::AstCfgNode {
     predicate hasCfgNode(SsaInput::BasicBlock bb, int i) { this = bb.getNode(i) }