Merge pull request #363 from xiemaisi/js/destructuring-assignment-cfg

JavaScript: Improve handling of destructuring assignments.

Author: Max Schaefer

javascript/ql/src/semmle/javascript/BasicBlocks.qll

@@ -67,7 +67,27 @@ private cached module Internal {
   }
 
   cached predicate defAt(BasicBlock bb, int i, Variable v, VarDef d) {
-    v = d.getAVariable() and bbIndex(bb, d, i)
+    exists (VarRef lhs |
+      lhs = d.getTarget().(BindingPattern).getABindingVarRef() and
+      v = lhs.getVariable() |
+      lhs = d.getTarget() and
+      bbIndex(bb, d, i)
+      or
+      exists (PropertyPattern pp |
+        lhs = pp.getValuePattern() and
+        bbIndex(bb, pp, i)
+      )
+      or
+      exists (ObjectPattern op |
+        lhs = op.getRest() and
+        bbIndex(bb, lhs, i)
+      )
+      or
+      exists (ArrayPattern ap |
+        lhs = ap.getAnElement() and
+        bbIndex(bb, lhs, i)
+      )
+    )
   }
 
   cached predicate reachableBB(BasicBlock bb) {

javascript/ql/src/semmle/javascript/DefUse.qll

@@ -98,7 +98,8 @@ private predicate lvalAux(Expr l, ControlFlowNode def) {
   exists (ArrayPattern ap | lvalAux(ap, def) | l = ap.getAnElement().stripParens())
   or
   exists (ObjectPattern op | lvalAux(op, def) |
-    l = op.getAPropertyPattern().getValuePattern().stripParens()
+    l = op.getAPropertyPattern().getValuePattern().stripParens() or
+    l = op.getRest().stripParens()
   )
 }
 

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

@@ -122,17 +122,25 @@ private cached module Internal {
        }
     or TPhi(ReachableJoinBlock bb, SsaSourceVariable v) {
          liveAtEntry(bb, v) and
-         exists (ReachableBasicBlock defbb, SsaDefinition def |
-          def.definesAt(defbb, _, v) and
-          bb.inDominanceFrontierOf(defbb)
-         )
+         inDefDominanceFrontier(bb, v)
        }
     or TRefinement(ReachableBasicBlock bb, int i, GuardControlFlowNode guard, SsaSourceVariable v) {
          bb.getNode(i) = guard and
          guard.getTest().(Refinement).getRefinedVar() = v and
          liveAtEntry(bb, v)
        }
 
+  /**
+   * Holds if `bb` is in the dominance frontier of a block containing a definition of `v`.
+   */
+  pragma[noinline]
+  private predicate inDefDominanceFrontier(ReachableJoinBlock bb, SsaSourceVariable v) {
+    exists (ReachableBasicBlock defbb, SsaDefinition def |
+      def.definesAt(defbb, _, v) and
+      bb.inDominanceFrontierOf(defbb)
+    )
+  }
+
   /**
    * Holds if `v` is a captured variable which is declared in `declContainer` and read in
    * `useContainer`.
@@ -216,6 +224,13 @@ private cached module Internal {
     ref(bb, i, v, tp)
   }
 
+  /**
+   * Gets the maximum rank among all references to `v` in basic block `bb`.
+   */
+  private int maxRefRank(ReachableBasicBlock bb, SsaSourceVariable v) {
+    result = max(refRank(bb, _, v, _))
+  }
+
   /**
    * Holds if variable `v` is live after the `i`th node of basic block `bb`, where
    * `i` is the index of a node that may assign or capture `v`.
@@ -230,8 +245,8 @@ private cached module Internal {
       or
       // this is the last reference to `v` inside `bb`, but `v` is live at entry
       // to a successor basic block of `bb`
-      r = max(refRank(bb, _, v, _)) and
-      liveAtEntry(bb.getASuccessor(), v)
+      r = maxRefRank(bb, v) and
+      liveAtSuccEntry(bb, v)
     )
   }
 
@@ -248,6 +263,13 @@ private cached module Internal {
     // there is no reference to `v` inside `bb`, but `v` is live at entry
     // to a successor basic block of `bb`
     not exists(refRank(bb, _, v, _)) and
+    liveAtSuccEntry(bb, v)
+  }
+
+  /**
+   * Holds if `v` is live at the beginning of any successor of basic block `bb`.
+   */
+  private predicate liveAtSuccEntry(ReachableBasicBlock bb, SsaSourceVariable v) {
     liveAtEntry(bb.getASuccessor(), v)
   }
 
@@ -311,25 +333,32 @@ private cached module Internal {
     )
   }
 
+  /**
+   * Gets an SSA definition of `v` that reaches the end of the immediate dominator of `bb`.
+   */
+  pragma[noinline]
+  private SsaDefinition getDefReachingEndOfImmediateDominator(ReachableBasicBlock bb, SsaSourceVariable v) {
+    result = getDefReachingEndOf(bb.getImmediateDominator(), v)
+  }
+
   /**
    * Gets an SSA definition of `v` that reaches the end of basic block `bb`.
    */
   cached SsaDefinition getDefReachingEndOf(ReachableBasicBlock bb, SsaSourceVariable v) {
-    bb.getASuccessor().localIsLiveAtEntry(v) and
-    (
-      exists (int lastRef | lastRef = max(int i | ssaRef(bb, i, v, _)) |
-        result = getLocalDefinition(bb, lastRef, v)
-        or
-        result.definesAt(bb, lastRef, v)
-      )
+    exists (int lastRef | lastRef = max(int i | ssaRef(bb, i, v, _)) |
+      result = getLocalDefinition(bb, lastRef, v)
       or
-      /* In SSA form, the (unique) reaching definition of a use is the closest
-       * definition that dominates the use. If two definitions dominate a node
-       * then one must dominate the other, so we can find the reaching definition
-       * by following the idominance relation backwards. */
-      result = getDefReachingEndOf(bb.getImmediateDominator(), v) and
-      not exists (SsaDefinition ssa | ssa.definesAt(bb, _, v))
+      result.definesAt(bb, lastRef, v) and
+      liveAtSuccEntry(bb, v)
     )
+    or
+    /* In SSA form, the (unique) reaching definition of a use is the closest
+     * definition that dominates the use. If two definitions dominate a node
+     * then one must dominate the other, so we can find the reaching definition
+     * by following the idominance relation backwards. */
+    result = getDefReachingEndOfImmediateDominator(bb, v) and
+    not exists (SsaDefinition ssa | ssa.definesAt(bb, _, v)) and
+    liveAtSuccEntry(bb, v)
   }
 
   /**

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

@@ -579,20 +579,20 @@
 | destructuring | 3 | o | 4 | p |
 | destructuring | 4 | p | 5 | {\\n    v ...  = p;\\n} |
 | destructuring | 5 | {\\n    v ...  = p;\\n} | 6 | var\\n    ...  } = o; |
-| destructuring | 6 | var\\n    ...  } = o; | 7 | {\\n      ... \\n     } |
+| destructuring | 6 | var\\n    ...  } = o; | 10 | o |
 | destructuring | 7 | {\\n      ...   } = o | 11 | var\\n    ...  ] = p; |
 | destructuring | 7 | {\\n      ... \\n     } | 8 | x |
 | destructuring | 8 | x | 8 | x |
 | destructuring | 8 | x | 9 | y |
+| destructuring | 9 | y | 7 | {\\n      ...   } = o |
 | destructuring | 9 | y | 9 | y |
-| destructuring | 9 | y | 10 | o |
-| destructuring | 10 | o | 7 | {\\n      ...   } = o |
-| destructuring | 11 | var\\n    ...  ] = p; | 12 | [\\n      ... \\n     ] |
+| destructuring | 10 | o | 7 | {\\n      ... \\n     } |
+| destructuring | 11 | var\\n    ...  ] = p; | 16 | p |
 | destructuring | 12 | [\\n      ...   ] = p | 17 | exit node of functio ...  = p;\\n} |
 | destructuring | 12 | [\\n      ... \\n     ] | 13 | a |
 | destructuring | 13 | a | 15 | c |
-| destructuring | 15 | c | 16 | p |
-| destructuring | 16 | p | 12 | [\\n      ...   ] = p |
+| destructuring | 15 | c | 12 | [\\n      ...   ] = p |
+| destructuring | 16 | p | 12 | [\\n      ... \\n     ] |
 | enum | 1 | 'value' | 1 | a = 'value' |
 | enum | 1 | E | 1 | a |
 | enum | 1 | a | 1 | 'value' |

javascript/ql/test/library-tests/Flow/AbstractValues.expected

@@ -59,6 +59,8 @@
 | d.js:1:1:4:0 | exports object of module d |
 | d.js:1:1:4:0 | module object of module d |
 | d.js:1:18:3:1 | object literal |
+| destructuring.js:1:1:4:1 | function f |
+| destructuring.js:1:1:4:1 | instance of function f |
 | e.js:1:1:6:0 | exports object of module e |
 | e.js:1:1:6:0 | module object of module e |
 | es2015.js:1:1:50:0 | exports object of module es2015 |

javascript/ql/test/library-tests/Flow/abseval.expected

@@ -68,6 +68,8 @@
 | classAccessors.js:11:9:11:11 | myY | classAccessors.js:11:15:11:20 | this.y | file://:0:0:0:0 | indefinite value (heap) |
 | classAccessors.js:12:9:12:11 | myZ | classAccessors.js:12:15:12:20 | this.z | file://:0:0:0:0 | indefinite value (call) |
 | classAccessors.js:12:9:12:11 | myZ | classAccessors.js:12:15:12:20 | this.z | file://:0:0:0:0 | indefinite value (heap) |
+| destructuring.js:2:7:2:24 | { x, y = (z = x) } | destructuring.js:2:28:2:28 | o | file://:0:0:0:0 | indefinite value (call) |
+| destructuring.js:3:7:3:8 | z1 | destructuring.js:3:12:3:12 | z | file://:0:0:0:0 | indefinite value (heap) |
 | es2015.js:1:5:1:7 | Sup | es2015.js:1:11:6:1 | class { ... ;\\n  }\\n} | es2015.js:1:11:6:1 | class Sup |
 | es2015.js:4:9:4:12 | ctor | es2015.js:4:16:4:25 | new.target | file://:0:0:0:0 | indefinite value (call) |
 | es2015.js:19:7:19:11 | _args | es2015.js:19:15:19:18 | args | file://:0:0:0:0 | object |

javascript/ql/test/library-tests/Flow/destructuring.js

@@ -0,0 +1,4 @@
+function f(o) {
+  var { x, y = (z = x) } = o, z;
+  var z1 = z;
+}

javascript/ql/test/library-tests/Flow/types.expected

@@ -38,6 +38,8 @@
 | classAccessors.js:10:9:10:11 | myX | classAccessors.js:10:15:10:20 | this.x | boolean, class, date, function, null, number, object, regular expression,string or undefined |
 | classAccessors.js:11:9:11:11 | myY | classAccessors.js:11:15:11:20 | this.y | boolean, class, date, function, null, number, object, regular expression,string or undefined |
 | classAccessors.js:12:9:12:11 | myZ | classAccessors.js:12:15:12:20 | this.z | boolean, class, date, function, null, number, object, regular expression,string or undefined |
+| destructuring.js:2:7:2:24 | { x, y = (z = x) } | destructuring.js:2:28:2:28 | o | boolean, class, date, function, null, number, object, regular expression,string or undefined |
+| destructuring.js:3:7:3:8 | z1 | destructuring.js:3:12:3:12 | z | boolean, class, date, function, null, number, object, regular expression,string or undefined |
 | es2015.js:1:5:1:7 | Sup | es2015.js:1:11:6:1 | class { ... ;\\n  }\\n} | class |
 | es2015.js:4:9:4:12 | ctor | es2015.js:4:16:4:25 | new.target | boolean, class, date, function, null, number, object, regular expression,string or undefined |
 | es2015.js:19:7:19:11 | _args | es2015.js:19:15:19:18 | args | object |

javascript/ql/test/query-tests/Declarations/MissingVarDecl/MissingVarDecl.expected

@@ -4,3 +4,5 @@
 | test.js:54:10:54:10 | z | Variable z is used like a local variable, but is missing a declaration. |
 | test.js:60:6:60:6 | y | Variable y is used like a local variable, but is missing a declaration. |
 | test.js:66:2:66:2 | z | Variable z is used like a local variable, but is missing a declaration. |
+| tst3.js:7:10:7:10 | x | Variable x is used like a local variable, but is missing a declaration. |
+| tst3.js:7:16:7:19 | rest | Variable rest is used like a local variable, but is missing a declaration. |

javascript/ql/test/query-tests/Declarations/MissingVarDecl/tst3.js

@@ -2,3 +2,8 @@ function sc_alert(i) {
    for(;i;) ;
    foo;
 }
+
+function f(o) {
+    for({x, ...rest} of o)
+        console.log(x in rest);
+}