Updates ExpressionDecomposer to accept compound declarations (LETs …

…with multiple variables) in FOR initializers, but only attempt to decompose the first declaration. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=226210609
google · Dec 21, 2018 · 9c174ce · 9c174ce
1 parent a1c7bc0
commit 9c174ce
Show file tree

Hide file tree

Showing 3 changed files with 125 additions and 91 deletions.
diff --git a/src/com/google/javascript/jscomp/ExpressionDecomposer.java b/src/com/google/javascript/jscomp/ExpressionDecomposer.java
@@ -699,16 +699,17 @@ private static boolean isConditionalOp(Node n) {
   }
 
   /**
-   * @return The statement containing the expression or null if the subExpression
+   * Finds the statement containing {@code subExpression}.
-   *     is not contain in a Node where inlining is known to be possible.
+   *
-   *     For example, a WHILE node condition expression.
+   * <p>If {@code subExpression} is not contained by a statement where inlining is known to be
+   * possible, {@code null} is returned. For example, the condition expression of a WHILE loop.
    */
   @Nullable
-  static Node findExpressionRoot(Node subExpression) {
+  private static Node findExpressionRoot(Node subExpression) {
     Node child = subExpression;
-    for (Node parent : child.getAncestors()) {
+    for (Node current : child.getAncestors()) {
-      Token parentType = parent.getToken();
+      Node parent = current.getParent();
-      switch (parentType) {
+      switch (current.getToken()) {
         // Supported expression roots:
         // SWITCH and IF can have multiple children, but the CASE, DEFAULT,
         // or BLOCK will be encountered first for any of the children other
@@ -718,37 +719,42 @@ static Node findExpressionRoot(Node subExpression) {
         case SWITCH:
         case RETURN:
         case THROW:
-          Preconditions.checkState(child == parent.getFirstChild());
+          Preconditions.checkState(child.isFirstChildOf(current));
-          return parent;
+          return current;
+
         case VAR:
-        case CONST:
+          // Normalization will remove LABELs from VARs.
         case LET:
-          Preconditions.checkState(child == parent.getFirstChild());
+        case CONST:
-          if (parent.getParent().isVanillaFor()
+          if (NodeUtil.isAnyFor(parent)) {
-              && parent == parent.getParent().getFirstChild()) {
+            break; // Name declarations may not be roots if they're for-loop initializers.
-            return parent.getParent();
-          } else {
-            return parent;
           }
+          return current;
+
         // Any of these indicate an unsupported expression:
         case FOR:
-          if (child == parent.getFirstChild()) {
+          if (child.isFirstChildOf(current)) {
-            return parent;
+            // Only the initializer of a for-loop could possibly be decomposed since the other
+            // statements need to execute each iteration.
+            return current;
           }
           // fall through
         case FOR_IN:
         case FOR_OF:
+        case DO:
+        case WHILE:
         case SCRIPT:
         case BLOCK:
         case LABEL:
         case CASE:
         case DEFAULT_CASE:
         case PARAM_LIST:
           return null;
+
         default:
           break;
       }
-      child = parent;
+      child = current;
     }
 
     throw new IllegalStateException("Unexpected AST structure.");
@@ -839,6 +845,15 @@ private DecompositionType isSubexpressionMovable(Node expressionRoot, Node subEx
 
     Node child = subExpression;
     for (Node parent : child.getAncestors()) {
+      if (NodeUtil.isNameDeclaration(parent) && !child.isFirstChildOf(parent)) {
+        // Case: `let x = 5, y = 2 * x;` where `child = y`.
+        // Compound declarations cannot generally be decomposed. Later declarations might reference
+        // earlier ones and if it were possible to separate them, `Normalize` would already have
+        // done so. Therefore, we only support decomposing the first declaration.
+        // TODO(b/121157467): FOR initializers are probably the only source of these cases.
+        return DecompositionType.UNDECOMPOSABLE;
+      }
+
       if (parent == expressionRoot) {
         // Done. The walk back to the root of the expression is complete, and
         // nothing was encountered that blocks the call from being moved.

diff --git a/src/com/google/javascript/jscomp/FunctionInjector.java b/src/com/google/javascript/jscomp/FunctionInjector.java
@@ -25,7 +25,6 @@
 import com.google.common.base.Supplier;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
-import com.google.javascript.jscomp.ExpressionDecomposer.DecompositionType;
 import com.google.javascript.rhino.Node;
 import com.google.javascript.rhino.Token;
 import com.google.javascript.rhino.jstype.JSType;
@@ -467,17 +466,14 @@ private CallSiteType classifyCallSite(Reference ref) {
       // left-hand-side of the assignments and handling them as EXPRESSION?
       return CallSiteType.VAR_DECL_SIMPLE_ASSIGNMENT;
     } else {
-      Node expressionRoot = ExpressionDecomposer.findExpressionRoot(callNode);
+      ExpressionDecomposer decomposer = getDecomposer(ref.scope);
-      if (expressionRoot != null) {
+      switch (decomposer.canExposeExpression(callNode)) {
-        ExpressionDecomposer decomposer = getDecomposer(ref.scope);
+        case MOVABLE:
-        DecompositionType type = decomposer.canExposeExpression(callNode);
-        if (type == DecompositionType.MOVABLE) {
           return CallSiteType.EXPRESSION;
-        } else if (type == DecompositionType.DECOMPOSABLE) {
+        case DECOMPOSABLE:
           return CallSiteType.DECOMPOSABLE_EXPRESSION;
-        } else {
+        case UNDECOMPOSABLE:
-          checkState(type == DecompositionType.UNDECOMPOSABLE);
+          break;
-        }
       }
     }
 

diff --git a/test/com/google/javascript/jscomp/ExpressionDecomposerTest.java b/test/com/google/javascript/jscomp/ExpressionDecomposerTest.java
@@ -16,13 +16,11 @@
 
 package com.google.javascript.jscomp;
 
-import static com.google.common.base.Preconditions.checkNotNull;
-import static com.google.common.base.Preconditions.checkState;
 import static com.google.common.truth.Truth.assertThat;
 import static com.google.common.truth.Truth.assertWithMessage;
 import static com.google.javascript.jscomp.CompilerTestCase.lines;
-import static com.google.javascript.rhino.testing.NodeSubject.assertNode;
 
+import com.google.common.collect.ImmutableList;
 import com.google.javascript.jscomp.CompilerOptions.LanguageMode;
 import com.google.javascript.jscomp.ExpressionDecomposer.DecompositionType;
 import com.google.javascript.jscomp.type.SemanticReverseAbstractInterpreter;
@@ -164,6 +162,31 @@ public void testCanExposeExpression3() {
         "function f(){ return goo() && foo();}", "foo");
   }
 
+  @Test
+  public void testCanExposeExpression_compoundDeclaration_inForInitializer_firstElement() {
+    // VAR will already be hoisted by `Normalize`.
+    helperCanExposeExpression(DecompositionType.MOVABLE, "for (var x = foo(), y = 5;;) {}", "foo");
+
+    helperCanExposeExpression(DecompositionType.MOVABLE, "for (let x = foo(), y = 5;;) {}", "foo");
+    helperCanExposeExpression(
+        DecompositionType.MOVABLE, "for (const x = foo(), y = 5;;) {}", "foo");
+  }
+
+  @Test
+  public void testCanExposeExpression_compoundDeclaration_inForInitializer_nthElement() {
+    // TODO(b/121157467) FOR introduces complex scoping that isn't currently `Normalize`d.
+    // Since in some cases we'd effectively end up having to `Normalize` these, decomposition just
+    // bails for now.
+
+    // VAR will already be hoisted by `Normalize`.
+    helperCanExposeExpression(DecompositionType.MOVABLE, "for (var x = 8, y = foo();;) {}", "foo");
+
+    helperCanExposeExpression(
+        DecompositionType.UNDECOMPOSABLE, "for (let x = 8, y = foo();;) {}", "foo");
+    helperCanExposeExpression(
+        DecompositionType.UNDECOMPOSABLE, "for (const x = 8, y = foo();;) {}", "foo");
+  }
+
   @Test
   public void testCannotExpose_expression4a() {
     // 'this' must be preserved in call.
@@ -624,6 +647,50 @@ public void testExposeExpression13() {
         "var temp_const$jscomp$0 = 1 + goo(); switch(temp_const$jscomp$0 + foo()){}");
   }
 
+  @Test
+  public void testExposeExpression_inVanillaForInitializer_simpleExpression() {
+    helperExposeExpression(
+        "for (x = goo() + foo();;) {}",
+        "foo",
+        lines(
+            "var temp_const$jscomp$0 = goo();", //
+            "for (x = temp_const$jscomp$0 + foo();;) {}"));
+  }
+
+  @Test
+  public void testExposeExpression_inVanillaForInitializer_usingLabel() {
+    helperExposeExpression(
+        "LABEL: for (x = goo() + foo();;) {}",
+        "foo",
+        lines(
+            "var temp_const$jscomp$0 = goo();", //
+            "LABEL: for (x = temp_const$jscomp$0 + foo();;) {}"));
+  }
+
+  @Test
+  public void testExposeExpression_inVanillaForInitializer_singleDeclaration_withLetOrConst() {
+    for (String dec : ImmutableList.of("let", "const")) {
+      helperExposeExpression(
+          "for (" + dec + " x = goo() + foo();;) {}",
+          "foo",
+          lines(
+              "var temp_const$jscomp$0 = goo();", //
+              "for (" + dec + " x = temp_const$jscomp$0 + foo();;) {}"));
+    }
+  }
+
+  @Test
+  public void testExposeExpression_inVanillaForInitializer_firstDeclaration_withLetOrConst() {
+    for (String dec : ImmutableList.of("let", "const")) {
+      helperExposeExpression(
+          "for (" + dec + " x = goo() + foo(), y = 5;;) {}",
+          "foo",
+          lines(
+              "var temp_const$jscomp$0 = goo();", //
+              "for (" + dec + " x = temp_const$jscomp$0 + foo(), y = 5;;) {}"));
+    }
+  }
+
   @Test
   public void testExposeExpression14() {
     helperExposeExpression(
@@ -932,31 +999,6 @@ public void testExposeObjectLit1() {
         "foo",
         "var result$jscomp$0=foo();var x = {set a(p) {}, b: result$jscomp$0};");
   }
-
-  @Test
-  public void testExposeExpressionInTemplateLibSub() {
-    helperExposeExpression(
-        "` ${ foo() }  ${ goo() } `;",
-        "goo",
-        "var temp_const$jscomp$0 = foo(); ` ${ temp_const$jscomp$0 }  ${ goo() } `;");
-  }
-
-  @Test
-  public void testExposeSubExpressionInTemplateLibSub() {
-    helperExposeExpression(
-        "` ${ foo() + goo() } `;",
-        "goo",
-        "var temp_const$jscomp$0 = foo(); ` ${ temp_const$jscomp$0 + goo() } `;");
-  }
-
-  @Test
-  public void testMoveExpressionInTemplateLibSub() {
-    helperMoveExpression(
-        "` ${ foo() }  ${ goo() } `;",
-        "foo",
-        "var result$jscomp$0 = foo(); ` ${ result$jscomp$0 }  ${ goo() } `;");
-  }
-
   @Test
   public void testMoveSpread_siblingOfCall_outOfArrayLiteral_usesTempArray() {
     shouldTestTypes = false; // TODO(nickreid): Enable this when tests support typed `AstFactory`.
@@ -998,50 +1040,31 @@ public void testMoveSpreadParent_siblingOfCall_outOfFunctionCall_usesNoTempArray
   }
 
   @Test
-  public void testFindExpressionRoot1() {
+  public void testExposeExpressionInTemplateLibSub() {
-    assertNode(findExpressionRoot("var x = f()", "f")).hasType(Token.VAR);
+    helperExposeExpression(
-  }
+        "` ${ foo() }  ${ goo() } `;",
-
+        "goo",
-  @Test
+        "var temp_const$jscomp$0 = foo(); ` ${ temp_const$jscomp$0 }  ${ goo() } `;");
-  public void testFindExpressionRoot2() {
-    assertNode(findExpressionRoot("foo(bar(f()));", "f")).hasType(Token.EXPR_RESULT);
-  }
-
-  @Test
-  public void testFindExpressionRoot3() {
-    assertThat(findExpressionRoot("for (let x of f()) {}", "f")).isNull();
   }
 
   @Test
-  public void testFindExpressionRoot4() {
+  public void testExposeSubExpressionInTemplateLibSub() {
-    assertThat(findExpressionRoot("for (let x in f()) {}", "f")).isNull();
+    helperExposeExpression(
+        "` ${ foo() + goo() } `;",
+        "goo",
+        "var temp_const$jscomp$0 = foo(); ` ${ temp_const$jscomp$0 + goo() } `;");
   }
 
   @Test
-  public void testFindExpressionRoot5() {
+  public void testMoveExpressionInTemplateLibSub() {
-    assertNode(findExpressionRoot("for (let x = f();;) {}", "f")).hasType(Token.FOR);
+    helperMoveExpression(
+        "` ${ foo() }  ${ goo() } `;",
+        "foo",
+        "var result$jscomp$0 = foo(); ` ${ result$jscomp$0 }  ${ goo() } `;");
   }
 
   /** Test case helpers. */
 
-  /**
-   * @return The result of calling {@link ExpressionDecomposer#findExpressionRoot} on the CALL
-   *     node in {@code js} whose callee is a NAME matching {@code name}.
-   */
-  @Nullable
-  private Node findExpressionRoot(String js, String name) {
-    Compiler compiler = getCompiler();
-    Node tree = parse(compiler, js);
-    Node call = findCall(tree, name);
-    checkNotNull(call);
-
-    Node root = ExpressionDecomposer.findExpressionRoot(call);
-    if (root != null) {
-      checkState(NodeUtil.isStatement(root), root);
-    }
-    return root;
-  }
-
   private void helperCanExposeFunctionExpression(
       DecompositionType expectedResult, String code, int call) {
     Compiler compiler = getCompiler();