Don't replace substr() with charAt() when the start index is negative

Eg. "foo".substr(-1, 1) is not equivalent to "foo".charAt(-1). Apparently I misread the specs or something. Fixes #2268 on GitHub. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=146431008
google · Feb 4, 2017 · e5437d7 · e5437d7
1 parent bdfdaed
commit e5437d7
Show file tree

Hide file tree

Showing 2 changed files with 26 additions and 46 deletions.
diff --git a/src/com/google/javascript/jscomp/PeepholeReplaceKnownMethods.java b/src/com/google/javascript/jscomp/PeepholeReplaceKnownMethods.java
@@ -127,12 +127,28 @@ private Node tryFoldKnownStringMethods(Node subtree, Node callTarget) {
     }
     if (useTypes && firstArg != null && (isStringLiteral
         || (stringNode.getJSType() != null && stringNode.getJSType().isStringValueType()))) {
-      switch (functionNameString) {
-        case "substr":
-          return tryReplaceSubstrWithCharAt(subtree, callTarget, firstArg);
-        case "substring":
-        case "slice":
-          return tryReplaceSubstringOrSliceWithCharAt(subtree, callTarget, firstArg);
+      if (subtree.hasXChildren(3)) {
+        Double maybeStart = NodeUtil.getNumberValue(firstArg, useTypes);
+        if (maybeStart != null) {
+          int start = maybeStart.intValue();
+          Double maybeLengthOrEnd = NodeUtil.getNumberValue(firstArg.getNext(), useTypes);
+          if (maybeLengthOrEnd != null) {
+            switch (functionNameString) {
+              case "substr":
+                int length = maybeLengthOrEnd.intValue();
+                if (start >= 0 && length == 1) {
+                  return replaceWithCharAt(subtree, callTarget, firstArg);
+                }
+                break;
+              case "substring":
+              case "slice":
+                int end = maybeLengthOrEnd.intValue();
+                if (end - start == 1) {
+                  return replaceWithCharAt(subtree, callTarget, firstArg);
+                }
+            }
+          }
+        }
       }
     }
     return subtree;
@@ -616,43 +632,6 @@ private Node tryFoldStringSubstringOrSlice(Node n, Node stringNode, Node arg1) {
     return resultNode;
   }
 
-  private Node tryReplaceSubstrWithCharAt(Node n, Node callTarget, Node firstArg) {
-    if (n.hasXChildren(3)) {
-      Double maybeLength = NodeUtil.getNumberValue(firstArg.getNext(), useTypes);
-      if (maybeLength != null) {
-        int length = maybeLength.intValue();
-        if (length == 1) {
-          return replaceWithCharAt(n, callTarget, firstArg);
-        }
-      }
-    }
-    return n;
-  }
-
-  private Node tryReplaceSubstringOrSliceWithCharAt(Node n, Node callTarget, Node firstArg) {
-    if (!n.hasXChildren(3)) {
-      return n;
-    }
-    Double maybeStart = NodeUtil.getNumberValue(firstArg, useTypes);
-    if (maybeStart == null) {
-      return n;
-    }
-    Node secondArg = firstArg.getNext();
-    Double maybeEnd = NodeUtil.getNumberValue(secondArg, useTypes);
-    if (maybeEnd == null) {
-      return n;
-    }
-    int start = maybeStart.intValue();
-    int end = maybeEnd.intValue();
-    if (start >= end) {
-      return n; // Bail out for simplicity
-    }
-    if (end - start == 1) {
-      return replaceWithCharAt(n, callTarget, firstArg);
-    }
-    return n;
-  }
-
   private Node replaceWithCharAt(Node n, Node callTarget, Node firstArg) {
     // TODO(moz): Maybe correct the arity of the function type here.
     callTarget.getLastChild().setString("charAt");

diff --git a/test/com/google/javascript/jscomp/PeepholeReplaceKnownMethodsTest.java b/test/com/google/javascript/jscomp/PeepholeReplaceKnownMethodsTest.java
@@ -328,6 +328,7 @@ public void testFoldParseOctalNumbers() {
   public void testReplaceWithCharAt() {
     enableTypeCheck();
     foldStringTyped("a.substring(0, 1)", "a.charAt(0)");
+    foldStringTyped("a.substring(-4, -3)", "a.charAt(-4)");
     foldSameStringTyped("a.substring(i, j + 1)");
     foldSameStringTyped("a.substring(i, i + 1)");
     foldSameStringTyped("a.substring(1, 2, 3)");
@@ -351,9 +352,9 @@ public void testReplaceWithCharAt() {
 
     foldStringTyped("a.substr(0, 1)", "a.charAt(0)");
     foldStringTyped("a.substr(2, 1)", "a.charAt(2)");
-    foldStringTyped("a.substr(-2, 1)", "a.charAt(-2)");
-    foldStringTyped("a.substr(bar(), 1)", "a.charAt(bar())");
-    foldStringTyped("''.substr(bar(), 1)", "''.charAt(bar())");
+    foldSameStringTyped("a.substr(-2, 1)");
+    foldSameStringTyped("a.substr(bar(), 1)");
+    foldSameStringTyped("''.substr(bar(), 1)");
     foldSameStringTyped("a.substr(2, 1, 3)");
     foldSameStringTyped("a.substr(1, 2, 3)");
     foldSameStringTyped("a.substr()");