Don't try to fold Array.join() when the array has a SPREAD node

------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=171068301
google · Oct 5, 2017 · b0763d2 · b0763d2
1 parent bc9cf44
commit b0763d2
Show file tree

Hide file tree

Showing 2 changed files with 27 additions and 10 deletions.
diff --git a/src/com/google/javascript/jscomp/PeepholeReplaceKnownMethods.java b/src/com/google/javascript/jscomp/PeepholeReplaceKnownMethods.java
@@ -18,6 +18,7 @@
 
 import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkNotNull;
+import static com.google.common.base.Preconditions.checkState;
 import static com.google.common.base.Strings.isNullOrEmpty;
 
 import com.google.javascript.rhino.IR;
@@ -106,6 +107,7 @@ private Node tryFoldKnownStringMethods(Node subtree, Node callTarget) {
             return tryFoldStringToLowerCase(subtree, stringNode);
           case "toUpperCase":
             return tryFoldStringToUpperCase(subtree, stringNode);
+          default: // fall out
         }
       } else {
         if (NodeUtil.isImmutableValue(firstArg)) {
@@ -122,6 +124,7 @@ private Node tryFoldKnownStringMethods(Node subtree, Node callTarget) {
               return tryFoldStringCharAt(subtree, stringNode, firstArg);
             case "charCodeAt":
               return tryFoldStringCharCodeAt(subtree, stringNode, firstArg);
+            default: // fall out
           }
         }
       }
@@ -150,6 +153,8 @@ private Node tryFoldKnownStringMethods(Node subtree, Node callTarget) {
                 if (end - start == 1) {
                   return replaceWithCharAt(subtree, callTarget, firstArg);
                 }
+                break;
+              default: // fall out
             }
           }
         }
@@ -407,6 +412,7 @@ private Node tryFoldStringIndexOf(
    * Try to fold an array join: ['a', 'b', 'c'].join('') -> 'abc';
    */
   private Node tryFoldArrayJoin(Node n) {
+    checkState(n.isCall(), n);
     Node callTarget = n.getFirstChild();
 
     if (callTarget == null || !callTarget.isGetProp()) {
@@ -423,13 +429,11 @@ private Node tryFoldArrayJoin(Node n) {
     Node arrayNode = callTarget.getFirstChild();
     Node functionName = arrayNode.getNext();
 
-    if (!arrayNode.isArrayLit() ||
-        !functionName.getString().equals("join")) {
+    if (!arrayNode.isArrayLit() || !functionName.getString().equals("join")) {
       return n;
     }
 
-    if (right != null && right.isString()
-        && ",".equals(right.getString())) {
+    if (right != null && right.isString() && ",".equals(right.getString())) {
       // "," is the default, it doesn't need to be explicit
       n.removeChild(right);
       compiler.reportChangeToEnclosingScope(n);
@@ -485,7 +489,9 @@ private Node tryFoldArrayJoin(Node n) {
         return emptyStringNode;
       case 1:
         Node foldedStringNode = arrayFoldedChildren.remove(0);
-        if (foldedSize > originalSize) {
+        // The spread isn't valid outside any array literal (or would change meaning)
+        // so don't try to fold it.
+        if (foldedStringNode.isSpread() || foldedSize > originalSize) {
           return n;
         }
         arrayNode.detachChildren();

diff --git a/test/com/google/javascript/jscomp/PeepholeReplaceKnownMethodsTest.java b/test/com/google/javascript/jscomp/PeepholeReplaceKnownMethodsTest.java
@@ -304,13 +304,24 @@ public void testJoinBug() {
             "  ].join()"));
   }
 
-  // Fails with:
-  // java.lang.IllegalStateException: SPREAD node should not be the child of a ADD node.
-  // TODO(b/67381773): Fix and enable this test.
-  public void disabled_testJoinSpread() {
-    foldSame("var x = [...foo].join(',');");
+  public void testJoinSpread1() {
     foldSame("var x = [...foo].join('');");
     foldSame("var x = [...someMap.keys()].join('');");
+    foldSame("var x = [foo, ...bar].join('');");
+    foldSame("var x = [...foo, bar].join('');");
+    foldSame("var x = [...foo, 'bar'].join('');");
+    foldSame("var x = ['1', ...'2', '3'].join('');");
+    foldSame("var x = ['1', ...['2'], '3'].join('');");
+  }
+
+  public void testJoinSpread2() {
+    fold("var x = [...foo].join(',');", "var x = [...foo].join();");
+    fold("var x = [...someMap.keys()].join(',');", "var x = [...someMap.keys()].join();");
+    fold("var x = [foo, ...bar].join(',');", "var x = [foo, ...bar].join();");
+    fold("var x = [...foo, bar].join(',');", "var x = [...foo, bar].join();");
+    fold("var x = [...foo, 'bar'].join(',');", "var x = [...foo, 'bar'].join();");
+    fold("var x = ['1', ...'2', '3'].join(',');", "var x = ['1', ...'2', '3'].join();");
+    fold("var x = ['1', ...['2'], '3'].join(',');", "var x = ['1', ...['2'], '3'].join();");
   }
 
   public void testToUpper() {