JS: Introduce DataFlow::lvalueNode

javascript/ql/src/semmle/javascript/SSA.qll

@@ -527,7 +527,7 @@ class SsaExplicitDefinition extends SsaDefinition, TExplicitDef {
    * if any.
    */
   DataFlow::Node getRhsNode() {
-    result = DataFlow::defSourceNode(getDef(), getSourceVariable())
+    result = DataFlow::ssaDefinitionNode(this).getImmediatePredecessor()
   }
 }
 

javascript/ql/src/semmle/javascript/StringConcatenation.qll

@@ -9,7 +9,7 @@ module StringConcatenation {
   private DataFlow::Node getAssignAddResult(AssignAddExpr expr) {
     result = expr.flow()
     or
-    result = DataFlow::ssaDefinitionNode(SSA::definition(expr))
+    result = DataFlow::lvalueNode(expr.getTarget())
   }
 
   /** Gets the `n`th operand to the string concatenation defining `node`. */

javascript/ql/src/semmle/javascript/dataflow/DataFlow.qll

@@ -166,33 +166,27 @@ module DataFlow {
      * Gets the immediate predecessor of this node, if any.
      *
      * A node with an immediate predecessor can usually only have the value that flows
-     * into its from its immediate predecessor, currently with two exceptions:
+     * into its from its immediate predecessor, currently with one exception:
      *
      * - An immediately-invoked function expression with a single return expression `e`
      *   has `e` as its immediate predecessor, even if the function can fall over the
      *   end and return `undefined`.
-     *
-     * - A destructuring property pattern or element pattern with a default value has
-     *   both the `PropRead` and its default value as immediate predecessors.
      */
     cached
     DataFlow::Node getImmediatePredecessor() {
+      lvalueFlowStep(result, this) and
+      not lvalueDefaultFlowStep(_, this)
+      or
       // Use of variable -> definition of variable
       exists(SsaVariable var |
-        this = DataFlow::valueNode(var.getAUse()) and
-        result.(DataFlow::SsaDefinitionNode).getSsaVariable() = var
+        this = valueNode(var.getAUse()) and
+        result = TSsaDefNode(var)
       )
       or
       // Refinement of variable -> original definition of variable
       exists(SsaRefinementNode refinement |
-        this.(DataFlow::SsaDefinitionNode).getSsaVariable() = refinement.getVariable() and
-        result.(DataFlow::SsaDefinitionNode).getSsaVariable() = refinement.getAnInput()
-      )
-      or
-      // Definition of variable -> RHS of definition
-      exists(SsaExplicitDefinition def |
-        this = TSsaDefNode(def) and
-        result = def.getRhsNode()
+        this = TSsaDefNode(refinement) and
+        result = TSsaDefNode(refinement.getAnInput())
       )
       or
       // IIFE call -> return value of IIFE
@@ -202,24 +196,6 @@ module DataFlow {
         result = fun.getAReturnedExpr().flow() and
         strictcount(fun.getAReturnedExpr()) = 1
       )
-      or
-      // IIFE parameter -> IIFE call
-      exists(Parameter param |
-        this = DataFlow::parameterNode(param) and
-        localArgumentPassing(result.asExpr(), param)
-      )
-      or
-      // `{ x } -> e` in `let { x } = e`
-      exists(DestructuringPattern pattern |
-        this = TDestructuringPatternNode(pattern)
-      |
-        exists(VarDef def |
-          pattern = def.getTarget() and
-          result = DataFlow::valueNode(def.getDestructuringSource())
-        )
-        or
-        result = patternPropRead(pattern)
-      )
     }
   }
 
@@ -1106,11 +1082,7 @@ module DataFlow {
    * INTERNAL: Use `parameterNode(Parameter)` instead.
    */
   predicate parameterNode(DataFlow::Node nd, Parameter p) {
-    nd = ssaDefinitionNode(SSA::definition((SimpleParameter)p))
-    or
-    nd = TDestructuringPatternNode(p)
-    or
-    nd = TUnusedParameterNode(p)
+    nd = lvalueNode(p)
   }
 
   /**
@@ -1150,24 +1122,21 @@ module DataFlow {
   }
 
   /**
-   * INTERNAL. DO NOT USE.
+   * Gets the data flow node corresponding the given l-value expression.
    *
-   * Gets the `PropRead` node corresponding to the value stored in the given
-   * binding pattern due to destructuring.
-   *
-   * For example, in `let { p: value } = f()`, the `value` pattern maps to a `PropRead`
-   * extracting the `p` property.
+   * This differs from `DataFlow::valueNode()`, which represents the value
+   * _before_ the l-value is assigned to, whereas `DataFlow::lvalueNode()`
+   * represents the value _after_ the assignment.
    */
-  private DataFlow::PropRead patternPropRead(BindingPattern value) {
-    exists(PropertyPattern prop |
-      value = prop.getValuePattern() and
-      result = TPropNode(prop)
+  Node lvalueNode(BindingPattern lvalue) {
+    exists(SsaExplicitDefinition ssa |
+      ssa.defines(lvalue.(LValue).getDefNode(), lvalue.(VarRef).getVariable()) and
+      result = TSsaDefNode(ssa)
     )
     or
-    exists(ArrayPattern array |
-      value = array.getAnElement() and
-      result = TElementPatternNode(array, value)
-    )
+    result = TDestructuringPatternNode(lvalue)
+    or
+    result = TUnusedParameterNode(lvalue)
   }
 
   /**
@@ -1212,18 +1181,60 @@ module DataFlow {
     any(ImmediatelyInvokedFunctionExpr iife).argumentPassing(parm, arg)
   }
 
+  /**
+   * Holds if there is a step from `pred -> succ` due to an assignment
+   * to an expression in l-value position.
+   */
+  private predicate lvalueFlowStep(Node pred, Node succ) {
+    exists(VarDef def |
+      pred = valueNode(defSourceNode(def)) and
+      succ = lvalueNode(def.getTarget())
+    )
+    or
+    exists(PropertyPattern pattern |
+      pred = TPropNode(pattern) and
+      succ = lvalueNode(pattern.getValuePattern())
+    )
+    or
+    exists(Expr element |
+      pred = TElementPatternNode(_, element) and
+      succ = lvalueNode(element)
+    )
+  }
+
+  /**
+   * Holds if there is a step from `pred -> succ` from the default
+   * value of a destructuring pattern or parameter.
+   */
+  private predicate lvalueDefaultFlowStep(Node pred, Node succ) {
+    exists(PropertyPattern pattern |
+      pred = valueNode(pattern.getDefault()) and
+      succ = lvalueNode(pattern.getValuePattern())
+    )
+    or
+    exists(ArrayPattern array, int i |
+      pred = valueNode(array.getDefault(i)) and
+      succ = lvalueNode(array.getElement(i))
+    )
+    or
+    exists(Parameter param |
+      pred = valueNode(param.getDefault()) and
+      succ = parameterNode(param)
+    )
+  }
+
   /**
    * Holds if data can flow from `pred` to `succ` in one local step.
    */
   cached
   predicate localFlowStep(Node pred, Node succ) {
-    // flow into local variables
-    exists(SsaDefinition ssa | succ = TSsaDefNode(ssa) |
-      // from the rhs of an explicit definition into the variable
-      exists(SsaExplicitDefinition def | def = ssa |
-        pred = defSourceNode(def.getDef(), def.getSourceVariable())
-      )
-      or
+    // flow from RHS into LHS
+    lvalueFlowStep(pred, succ)
+    or
+    lvalueDefaultFlowStep(pred, succ)
+    or
+    // Flow through implicit SSA nodes
+    exists(SsaImplicitDefinition ssa | succ = TSsaDefNode(ssa) |
       // from any explicit definition or implicit init of a captured variable into
       // the capturing definition
       exists(SsaSourceVariable v, SsaDefinition predDef |
@@ -1270,29 +1281,6 @@ module DataFlow {
       )
     )
     or
-    exists(VarDef def |
-      // from `e` to `{ p: x }` in `{ p: x } = e`
-      pred = valueNode(defSourceNode(def)) and
-      succ = TDestructuringPatternNode(def.getTarget())
-    )
-    or
-    // flow from the value read from a property pattern to the value being
-    // destructured in the child pattern. For example, for
-    //
-    //   let { p: { q: x } } = obj
-    //
-    // add edge from the 'p:' pattern to '{ q:x }'.
-    exists(PropertyPattern pattern |
-      pred = TPropNode(pattern) and
-      succ = TDestructuringPatternNode(pattern.getValuePattern())
-    )
-    or
-    // Like the step above, but for array destructuring patterns.
-    exists(Expr elm |
-      pred = TElementPatternNode(_, elm) and
-      succ = TDestructuringPatternNode(elm)
-    )
-    or
     // flow from 'this' parameter into 'this' expressions
     exists(ThisExpr thiz |
       pred = TThisNode(thiz.getBindingContainer()) and
@@ -1323,31 +1311,6 @@ module DataFlow {
     localArgumentPassing(result, def)
   }
 
-  /**
-   * INTERNAL. DO NOT USE.
-   *
-   * Gets the data flow node representing the source of the definition of `v` at `def`,
-   * if any.
-   */
-  Node defSourceNode(VarDef def, SsaSourceVariable v) {
-    exists(BindingPattern lhs, VarRef r |
-      lhs = def.getTarget() and r = lhs.getABindingVarRef() and r.getVariable() = v
-    |
-      // follow one step of the def-use chain if the lhs is a simple variable reference
-      lhs = r and
-      result = TValueNode(defSourceNode(def))
-      or
-      // handle destructuring assignments
-      exists(PropertyPattern pp | r = pp.getValuePattern() |
-        result = TPropNode(pp) or result = pp.getDefault().flow()
-      )
-      or
-      result = TElementPatternNode(_, r)
-      or
-      exists(ArrayPattern ap, int i | ap.getElement(i) = r and result = ap.getDefault(i).flow())
-    )
-  }
-
   /**
    * Holds if the flow information for this node is incomplete.
    *

javascript/ql/test/library-tests/GlobalAccessPaths/GlobalAccessPaths.expected

@@ -51,11 +51,12 @@ test_fromReference
 | test.js:24:9:24:16 | NS \|\| {} | NS |
 | test.js:26:1:26:8 | Conflict | Conflict |
 | test.js:33:7:33:18 | { bar = {} } | foo |
-| test.js:33:7:33:24 | bar | foo.bar |
 | test.js:33:9:33:16 | bar = {} | foo.bar |
 | test.js:33:22:33:24 | foo | foo |
-| test.js:34:11:34:13 | bar | foo.bar |
-| test.js:34:11:34:17 | bar.baz | foo.bar.baz |
+| test.js:39:3:39:20 | lazyInit | foo.bar |
+| test.js:39:14:39:16 | foo | foo |
+| test.js:39:14:39:20 | foo.bar | foo.bar |
+| test.js:40:3:40:10 | lazyInit | foo.bar |
 test_fromRhs
 | other_ns.js:4:9:4:16 | NS \|\| {} | NS |
 | other_ns.js:6:12:6:13 | {} | Conflict |
@@ -65,8 +66,10 @@ test_fromRhs
 | test.js:28:1:28:20 | class GlobalClass {} | GlobalClass |
 | test.js:30:1:30:28 | functio ... on() {} | globalFunction |
 | test.js:32:1:35:1 | functio ... .baz'\\n} | destruct |
+| test.js:37:1:41:1 | functio ... Init;\\n} | lazy |
 test_assignedUnique
 | GlobalClass |
 | destruct |
 | f |
 | globalFunction |
+| lazy |

javascript/ql/test/library-tests/GlobalAccessPaths/test.js

@@ -33,3 +33,9 @@ function destruct() {
   let { bar = {} } = foo;
   let v = bar.baz; // 'foo.bar.baz'
 }
+
+function lazy() {
+  var lazyInit;
+  lazyInit = foo.bar; // 'foo.bar'
+  lazyInit;
+}