Merge pull request github#20443 from hvitved/rust/ssa-adjust-write-note

Rust: Adjust SSA write node for (compound) assignments

Author: hvitved

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

@@ -16,6 +16,36 @@ class ExitCfgNode = CfgImpl::ExitNode;
 
 class AnnotatedExitCfgNode = CfgImpl::AnnotatedExitNode;
 
+/** A variable access. */
+final class VariableAccessCfgNode extends PathExprBaseCfgNode {
+  private VariableAccess a;
+
+  VariableAccessCfgNode() { a = this.getAstNode() }
+
+  /** Gets the underlying `VariableAccess`. */
+  VariableAccess getAccess() { result = a }
+}
+
+/** A variable write. */
+final class VariableWriteAccessCfgNode extends VariableAccessCfgNode {
+  private VariableWriteAccess a;
+
+  VariableWriteAccessCfgNode() { a = this.getAstNode() }
+
+  /** Gets the underlying `VariableWriteAccess`. */
+  VariableWriteAccess getAccess() { result = a }
+}
+
+/** A variable read. */
+final class VariableReadAccessCfgNode extends VariableAccessCfgNode {
+  private VariableReadAccess a;
+
+  VariableReadAccessCfgNode() { a = this.getAstNode() }
+
+  /** Gets the underlying `VariableReadAccess`. */
+  VariableReadAccess getAccess() { result = a }
+}
+
 /**
  * An assignment expression, for example
  *
@@ -24,12 +54,42 @@ class AnnotatedExitCfgNode = CfgImpl::AnnotatedExitNode;
  * ```
  */
 final class AssignmentExprCfgNode extends BinaryExprCfgNode {
-  AssignmentExpr a;
+  AssignmentExprChildMapping a;
 
   AssignmentExprCfgNode() { a = this.getBinaryExpr() }
 
   /** Gets the underlying `AssignmentExpr`. */
   AssignmentExpr getAssignmentExpr() { result = a }
+
+  /**
+   * Gets a write access that occurs in the left-hand side of this assignment expression.
+   */
+  VariableWriteAccessCfgNode getAWriteAccess() {
+    a = result.getAccess().getAssignmentExpr() and
+    any(ChildMapping mapping).hasCfgChild(a, result.getAccess(), this, result)
+  }
+}
+
+/**
+ * A compound assignment expression, for example:
+ * ```rust
+ * x += y;
+ * ```
+ *
+ * Note that compound assignment expressions are syntatic sugar for
+ * trait invocations, i.e., the above actually means
+ *
+ * ```rust
+ * (&mut x).add_assign(y);
+ * ```
+ */
+final class CompoundAssignmentExprCfgNode extends BinaryExprCfgNode {
+  CompoundAssignmentExpr a;
+
+  CompoundAssignmentExprCfgNode() { a = this.getBinaryExpr() }
+
+  /** Gets the underlying `CompoundAssignmentExpr`. */
+  CompoundAssignmentExpr getCompoundAssignmentExpr() { result = a }
 }
 
 /**

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

@@ -81,6 +81,12 @@ class FormatArgsExprChildMapping extends ParentAstNode, CfgImpl::ExprTrees::Form
   override predicate relevantChild(AstNode child) { child = this.getChildNode(_) }
 }
 
+class AssignmentExprChildMapping extends ParentAstNode, AssignmentExpr {
+  override predicate relevantChild(AstNode child) {
+    child.(VariableWriteAccess).getAssignmentExpr() = this
+  }
+}
+
 private class ChildMappingImpl extends ChildMapping {
   /** Gets a CFG node for `child`, where `child` is a relevant child node of `parent`. */
   private CfgNode getRelevantChildCfgNode(AstNode parent, AstNode child) {

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

@@ -171,9 +171,20 @@ module Ssa {
     private CfgNode write;
 
     WriteDefinition() {
-      exists(BasicBlock bb, int i, Variable v |
+      exists(BasicBlock bb, int i, Variable v, CfgNode n |
         this.definesAt(v, bb, i) and
-        SsaImpl::variableWriteActual(bb, i, v, write)
+        SsaImpl::variableWriteActual(bb, i, v, n)
+      |
+        write.(VariableAccessCfgNode).getAccess().getVariable() = v and
+        (
+          write = n.(AssignmentExprCfgNode).getAWriteAccess()
+          or
+          write = n.(CompoundAssignmentExprCfgNode).getLhs()
+        )
+        or
+        not n instanceof AssignmentExprCfgNode and
+        not n instanceof CompoundAssignmentExprCfgNode and
+        write = n
       )
     }
 

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

@@ -263,7 +263,7 @@ module LocalFlow {
     or
     // An edge from a pattern/expression to its corresponding SSA definition.
     nodeFrom.(AstCfgFlowNode).getCfgNode() =
-      nodeTo.(SsaNode).asDefinition().(Ssa::WriteDefinition).getControlFlowNode()
+      nodeTo.(SsaNode).asDefinition().(Ssa::WriteDefinition).getWriteAccess()
     or
     nodeFrom.(SourceParameterNode).getParameter().(ParamCfgNode).getPat() = nodeTo.asPat()
     or

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

@@ -18,23 +18,22 @@ private predicate isInUninitializedLet(Name name) {
   )
 }
 
-/** Holds if `write` writes to variable `v`. */
-predicate variableWrite(AstNode write, Variable v) {
+/** Holds if `write` writes to variable `v` via `access`. */
+predicate variableWrite(AstNode write, AstNode access, Variable v) {
   exists(Name name |
     name = write and
+    access = write and
     name = v.getName() and
     not isInUninitializedLet(name)
   )
   or
-  exists(VariableAccess access |
-    access = write and
-    access.getVariable() = v
-  |
-    access instanceof VariableWriteAccess
+  v = access.(VariableAccess).getVariable() and
+  (
+    write = access.(VariableWriteAccess).getAssignmentExpr()
     or
     // Although compound assignments, like `x += y`, may in fact not update `x`,
     // it makes sense to treat them as such
-    access = any(CompoundAssignmentExpr cae).getLhs()
+    access = write.(CompoundAssignmentExpr).getLhs()
   )
 }
 
@@ -226,7 +225,7 @@ private module Cached {
   cached
   predicate variableWriteActual(BasicBlock bb, int i, Variable v, CfgNode write) {
     bb.getNode(i) = write and
-    variableWrite(write.getAstNode(), v)
+    variableWrite(write.getAstNode(), _, v)
   }
 
   cached

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

@@ -679,11 +679,11 @@ module Impl {
   }
 
   /** Holds if `e` occurs in the LHS of an assignment or compound assignment. */
-  private predicate assignmentExprDescendant(Expr e) {
-    e = any(AssignmentExpr ae).getLhs()
+  private predicate assignmentExprDescendant(AssignmentExpr ae, Expr e) {
+    e = ae.getLhs()
     or
     exists(Expr mid |
-      assignmentExprDescendant(mid) and
+      assignmentExprDescendant(ae, mid) and
       getImmediateParentAdj(e) = mid and
       not mid instanceof DerefExpr and
       not mid instanceof FieldExpr and
@@ -693,11 +693,16 @@ module Impl {
 
   /** A variable write. */
   class VariableWriteAccess extends VariableAccess {
+    private AssignmentExpr ae;
+
     cached
     VariableWriteAccess() {
       Cached::ref() and
-      assignmentExprDescendant(this)
+      assignmentExprDescendant(ae, this)
     }
+
+    /** Gets the assignment expression that has this write access in the left-hand side. */
+    AssignmentExpr getAssignmentExpr() { result = ae }
   }
 
   /** A variable read. */

rust/ql/src/queries/unusedentities/UnusedValue.ql

@@ -16,7 +16,7 @@ import UnusedVariable
 
 from AstNode write, Ssa::Variable v
 where
-  variableWrite(write, v) and
+  variableWrite(_, write, v) and
   not v instanceof DiscardVariable and
   not write.isInMacroExpansion() and
   not isAllowableUnused(v) and

rust/ql/test/library-tests/variables/CONSISTENCY/PathResolutionConsistency.expected

@@ -1,3 +1,3 @@
 multipleCallTargets
-| main.rs:89:19:89:40 | ...::from(...) |
-| main.rs:111:19:111:40 | ...::from(...) |
+| main.rs:91:19:91:40 | ...::from(...) |
+| main.rs:113:19:113:40 | ...::from(...) |