Fix problems with destructuring in FlowSensitiveInlineVariables

Correctly handles destructuring patterns in MaybeReachingVariableUse. This still doesn't try to inline definitions in destructuring patterns, though. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=185450434
google · Feb 13, 2018 · c45d5e6 · c45d5e6
1 parent cbd51f1
commit c45d5e6
Show file tree

Hide file tree

Showing 4 changed files with 134 additions and 24 deletions.
diff --git a/src/com/google/javascript/jscomp/FlowSensitiveInlineVariables.java b/src/com/google/javascript/jscomp/FlowSensitiveInlineVariables.java
@@ -258,8 +258,12 @@ public void visit(NodeTraversal t, Node n, Node parent) {
 
         // Make sure that the name node is purely a read.
         if ((NodeUtil.isAssignmentOp(parent) && parent.getFirstChild() == n)
-            || parent.isVar() || parent.isInc() || parent.isDec()
-            || parent.isParamList() || parent.isCatch()) {
+            || parent.isVar()
+            || parent.isInc()
+            || parent.isDec()
+            || parent.isParamList()
+            || parent.isCatch()
+            || NodeUtil.isLhsByDestructuring(n)) {
           return;
         }
 
@@ -354,17 +358,17 @@ private boolean canInline(final Scope scope) {
         return false;
       }
 
-      // The right of the definition has side effect:
+      // A subexpression evaluated after the variable has a side effect.
       // Example, for x:
       // x = readProp(b), modifyProp(b); print(x);
-      if (checkRightOf(def, getDefCfgNode(), SIDE_EFFECT_PREDICATE)) {
+      if (checkPostExpressions(def, getDefCfgNode(), SIDE_EFFECT_PREDICATE)) {
         return false;
       }
 
       // Similar check as the above but this time, all the sub-expressions
-      // left of the use of the variable.
+      // evaluated before the variable.
       // x = readProp(b); modifyProp(b), print(x);
-      if (checkLeftOf(use, useCfgNode, SIDE_EFFECT_PREDICATE)) {
+      if (checkPreExpressions(use, useCfgNode, SIDE_EFFECT_PREDICATE)) {
         return false;
       }
 
@@ -574,14 +578,18 @@ private boolean isAssignChain(Node child, Node ancestor) {
   }
 
   /**
-   * Given an expression by its root and sub-expression n, return true if there
-   * the predicate is true for some expression on the right of n.
+   * Given an expression by its root and sub-expression n, return true if the predicate is true for
+   * some expression evaluated after n.
    *
-   * Example:
+   * <p>NOTE: this doesn't correctly check destructuring patterns, because their order of evaluation
+   * is different from AST traversal order,  but currently this is ok because
+   * FlowSensitiveInlineVariables never inlines variable assignments inside destructuring.
    *
-   * NotChecked(), NotChecked(), n, Checked(), Checked();
+   * <p>Example:
+   *
+   * <p>NotChecked(), NotChecked(), n, Checked(), Checked();
    */
-  private static boolean checkRightOf(
+  private static boolean checkPostExpressions(
       Node n, Node expressionRoot, Predicate<Node> predicate) {
     for (Node p = n; p != expressionRoot; p = p.getParent()) {
       for (Node cur = p.getNext(); cur != null; cur = cur.getNext()) {
@@ -594,18 +602,30 @@ private static boolean checkRightOf(
   }
 
   /**
-   * Given an expression by its root and sub-expression n, return true if there
-   * the predicate is true for some expression on the left of n.
+   * Given an expression by its root and sub-expression n, return true if the predicate is true for
+   * some expression evaluated before n.
+   *
+   * <p>In most cases evaluation order follows left-to-right AST order. Destructuring pattern
+   * evaluation is an exception.
    *
-   * Example:
+   * <p>Example:
    *
-   * Checked(), Checked(), n, NotChecked(), NotChecked();
+   * <p>Checked(), Checked(), n, NotChecked(), NotChecked();
    */
-  private static boolean checkLeftOf(
+  private static boolean checkPreExpressions(
       Node n, Node expressionRoot, Predicate<Node> predicate) {
     for (Node p = n; p != expressionRoot; p = p.getParent()) {
-      for (Node cur = p.getParent().getFirstChild(); cur != p;
-          cur = cur.getNext()) {
+      Node oldestSibling = p.getParent().getFirstChild();
+      // Evaluate a destructuring assignment right-to-left.
+      if (oldestSibling.isDestructuringPattern()) {
+        if (p.isDestructuringPattern()) {
+          if (p.getNext() != null && predicate.apply(p.getNext())) {
+            return true;
+          }
+        }
+        continue;
+      }
+      for (Node cur = oldestSibling; cur != p; cur = cur.getNext()) {
         if (predicate.apply(cur)) {
           return true;
         }

diff --git a/src/com/google/javascript/jscomp/MaybeReachingVariableUse.java b/src/com/google/javascript/jscomp/MaybeReachingVariableUse.java
@@ -17,8 +17,8 @@
 package com.google.javascript.jscomp;
 
 import static com.google.common.base.Preconditions.checkNotNull;
+import static com.google.common.base.Preconditions.checkState;
 
-import com.google.common.base.Preconditions;
 import com.google.common.collect.HashMultimap;
 import com.google.common.collect.Multimap;
 import com.google.common.collect.MultimapBuilder;
@@ -186,7 +186,13 @@ private void computeMayUse(
         return;
 
       case NAME:
-        addToUseIfLocal(n.getString(), cfgNode, output);
+        if (NodeUtil.isLhsByDestructuring(n)) {
+          if (!conditional) {
+            removeFromUseIfLocal(n.getString(), output);
+          }
+        } else {
+          addToUseIfLocal(n.getString(), cfgNode, output);
+        }
         return;
 
       case WHILE:
@@ -226,15 +232,23 @@ private void computeMayUse(
         return;
 
       case VAR:
+        // TODO(b/73123594): this should also handle LET/CONST
         Node varName = n.getFirstChild();
-        Preconditions.checkState(n.hasChildren(), "AST should be normalized", n);
+        checkState(n.hasChildren(), "AST should be normalized", n);
 
-        if (varName.hasChildren()) {
+        if (varName.isDestructuringLhs()) {
+          // Note: we never inline variables used twice in the same CFG node, so the order of
+          // traversal here isn't important. If that changes, though, MaybeReachingVariableUse
+          // must be updated to correctly handle destructuring assignment evaluation order.
+          computeMayUse(varName.getFirstChild(), cfgNode, output, conditional);
+          computeMayUse(varName.getSecondChild(), cfgNode, output, conditional);
+
+        } else if (varName.hasChildren()) {
           computeMayUse(varName.getFirstChild(), cfgNode, output, conditional);
           if (!conditional) {
             removeFromUseIfLocal(varName.getString(), output);
           }
-        }
+        } // else var name declaration with no initial value
         return;
 
       default:

diff --git a/test/com/google/javascript/jscomp/FlowSensitiveInlineVariablesTest.java b/test/com/google/javascript/jscomp/FlowSensitiveInlineVariablesTest.java
@@ -740,8 +740,77 @@ public void testTemplateStrings() {
         "var age; `Age: ${3}`");
   }
 
-  public void testDestructuring() {
+  public void testArrayDestructuring() {
     noInline("var [a, b, c] = [1, 2, 3]; print(a + b + c);");
+    noInline("var arr = [1, 2, 3, 4]; var [a, b, ,d] = arr;");
+    noInline("var x = 3; [x] = 4; print(x);");
+    inline("var [x] = []; x = 3; print(x);", "var [x] = []; print(3);");
+  }
+
+  public void testObjectDestructuring() {
+    noInline("var {a, b} = {a: 3, b: 4}; print(a + b);");
+    noInline("var obj = {a: 3, b: 4}; var {a, b} = obj;");
+  }
+
+  public void testDestructuringDefaultValue() {
+    inline("var x = 1; var [y = x] = [];", "var x; var [y = 1] = [];");
+    inline("var x = 1; var {y = x} = {};", "var x; var {y = 1} = {};");
+    inline("var x = 1; var {[3]: y = x} = {};", "var x; var {[3]: y = 1} = {};");
+    noInline("var x = 1; var {[x]: y = x} = {};");
+    noInline("var x = 1; var [y = x] = []; print(x);");
+
+    // don't inline because x is only conditionally reassigned to 2.
+    noInline("var x = 1; var [y = (x = 2, 4)] = []; print(x);");
+  }
+
+  public void testDestructuringComputedProperty() {
+    inline("var x = 1; var {[x]: y} = {};", "var x; var {[1]: y} = {};");
+    noInline("var x = 1; var {[x]: y} = {}; print(x);");
+  }
+
+  public void testDeadAssignments() {
+    inline(
+        "let a = 3; if (3 < 4) { a = 8; } else { print(a); }",
+        "let a; if (3 < 4) { a = 8 } else { print(3); }");
+
+    inline(
+        "let a = 3; if (3 < 4) { [a] = 8; } else { print(a); }",
+        "let a; if (3 < 4) { [a] = 8 } else { print(3); }");
+  }
+
+  public void testDestructuringEvaluationOrder() {
+    // Should not inline "x = 2" in these cases because x is changed beforehand
+    noInline("var x = 2; var {x, y = x} = {x: 3};");
+    noInline("var x = 2; var {y = (x = 3), z = x} = {};");
+
+    // These examples are safe to inline, but FlowSensitiveInlineVariables never inlines variables
+    // used twice in the same CFG node even when safe to do so.
+    noInline("var x = 2; var {a: y = (x = 3)} = {a: x};");
+    noInline("var x = 1; var {a = x} = {a: (x = 2, 3)};");
+    noInline("var x = 2; var {a: x = 3} = {a: x};");
+  }
+
+  public void testDestructuringWithSideEffects() {
+    noInline("function f() { x++; }  var x = 2; var {y = x} = {key: f()}");
+
+    noInline("function f() { x++; } var x = 2; var y = x; var {z = y} = {a: f()}");
+    noInline("function f() { x++; } var x = 2; var y = x; var {a = f(), b = y} = {}");
+    noInline("function f() { x++; } var x = 2; var y = x; var {[f()]: z = y} = {}");
+    noInline("function f() { x++; } var x = 2; var y = x; var {a: {b: z = y} = f()} = {};");
+    noInline("function f() { x++; } var x = 2; var y; var {z = (y = x, 3)} = {a: f()}; print(y);");
+
+    inline(
+        "function f() { x++; }  var x = 2; var y = x; var {z = f()} = {a: y}",
+        "function f() { x++; }  var x = 2; var y    ; var {z = f()} = {a: x}");
+    inline(
+        "function f() { x++; }  var x = 2; var y = x; var {a = y, b = f()} = {}",
+        "function f() { x++; }  var x = 2; var y    ; var {a = x, b = f()} = {}");
+    inline(
+        "function f() { x++; }  var x = 2; var y = x; var {[y]: z = f()} = {}",
+        "function f() { x++; }  var x = 2; var y    ; var {[x]: z = f()} = {}");
+    inline(
+        "function f() { x++; } var x = 2; var y = x; var {a: {b: z = f()} = y} = {};",
+        "function f() { x++; } var x = 2; var y    ; var {a: {b: z = f()} = x} = {};");
   }
 
   private void noInline(String input) {

diff --git a/test/com/google/javascript/jscomp/MaybeReachingVariableUseTest.java b/test/com/google/javascript/jscomp/MaybeReachingVariableUseTest.java
@@ -119,6 +119,13 @@ public void testTryCatch() {
         + "U: var z = x;");
   }
 
+  public void testDestructuring() {
+    assertMatch("D: var x = 1; U: var [y = x] = [];");
+    assertMatch("D: var x = 1; var y; U: [y = x] = [];");
+    assertMatch("D: var [x] = []; U: x;");
+    assertMatch("var x; x = 3; D: [x] = 5; U: x;");
+  }
+
   /**
    * The def of x at D: may be used by the read of x at U:.
    */