Fix int.parse bug (dart2js version)

sdk/lib/_internal/compiler/js_lib/js_helper.dart

Index: sdk/lib/_internal/compiler/js_lib/js_helper.dart
diff --git a/sdk/lib/_internal/compiler/js_lib/js_helper.dart b/sdk/lib/_internal/compiler/js_lib/js_helper.dart
index 89766d12b75c77824bc30d72c8a15331ff6f9f2d..6563192f335b05b7490c4d059f36699e8eb8b360 100644
--- a/sdk/lib/_internal/compiler/js_lib/js_helper.dart
+++ b/sdk/lib/_internal/compiler/js_lib/js_helper.dart
@@ -586,82 +586,81 @@ class Primitives {
     return JS('int', '#', hash);
   }
 
-  static _throwFormatException(String string) {
-    throw new FormatException(string);
+  @NoInline()
+  static int _parseError(String source, int handleError(String source)) {
+    if (handleError == null) throw new FormatException(source);
+    return handleError(source);
   }
 
   static int parseInt(String source,
                       int radix,
                       int handleError(String source)) {
-    if (handleError == null) handleError = _throwFormatException;
-
     checkString(source);
-    var match = JS('JSExtendableArray|Null',
-        r'/^\s*[+-]?((0x[a-f0-9]+)|(\d+)|([a-z0-9]+))\s*$/i.exec(#)',
-        source);
+    var re = JS('', r'/^\s*[+-]?((0x[a-f0-9]+)|(\d+)|([a-z0-9]+))\s*$/i');
+    var match = JS('JSExtendableArray|Null', '#.exec(#)', re, source);
     int digitsIndex = 1;
     int hexIndex = 2;
     int decimalIndex = 3;
     int nonDecimalHexIndex = 4;
+    if (match == null) {
+      // TODO(sra): It might be that the match failed due to unrecognized U+0085
+      // spaces.  We could replace them with U+0020 spaces and try matching
+      // again.

[Lasse Reichstein Nielsen, 2015/03/06 13:06:51]

If \s doesn't always include U+0085, maybe change \s to [\s\x85] in the regexp?

[sra1, 2015/03/06 21:38:38]

In that case, JavaScript's parseInt fails too.

+      return _parseError(source, handleError);
+    }
     if (radix == null) {
-      radix = 10;
-      if (match != null) {
-        if (match[hexIndex] != null) {
-          // Cannot fail because we know that the digits are all hex.
-          return JS('num', r'parseInt(#, 16)', source);
-        }
-        if (match[decimalIndex] != null) {
-          // Cannot fail because we know that the digits are all decimal.
-          return JS('num', r'parseInt(#, 10)', source);
-        }
-        return handleError(source);
+      if (match[decimalIndex] != null) {
+        // Cannot fail because we know that the digits are all decimal.
+        return JS('int', r'parseInt(#, 10)', source);
       }
-    } else {
-      if (radix is! int) throw new ArgumentError("Radix is not an integer");
-      if (radix < 2 || radix > 36) {
-        throw new RangeError("Radix $radix not in range 2..36");
+      if (match[hexIndex] != null) {
+        // Cannot fail because we know that the digits are all hex.
+        return JS('int', r'parseInt(#, 16)', source);
       }
-      if (match != null) {
-        if (radix == 10 && match[decimalIndex] != null) {
-          // Cannot fail because we know that the digits are all decimal.
-          return JS('num', r'parseInt(#, 10)', source);
-        }
-        if (radix < 10 || match[decimalIndex] == null) {
-          // We know that the characters must be ASCII as otherwise the
-          // regexp wouldn't have matched. Lowercasing by doing `| 0x20` is thus
-          // guaranteed to be a safe operation, since it preserves digits
-          // and lower-cases ASCII letters.
-          int maxCharCode;
-          if (radix <= 10) {
-            // Allow all digits less than the radix. For example 0, 1, 2 for
-            // radix 3.
-            // "0".codeUnitAt(0) + radix - 1;
-            maxCharCode = 0x30 + radix - 1;
-          } else {
-            // Letters are located after the digits in ASCII. Therefore we
-            // only check for the character code. The regexp above made already
-            // sure that the string does not contain anything but digits or
-            // letters.
-            // "a".codeUnitAt(0) + (radix - 10) - 1;
-            maxCharCode = 0x61 + radix - 10 - 1;
-          }
-          String digitsPart = match[digitsIndex];
-          for (int i = 0; i < digitsPart.length; i++) {
-            int characterCode = digitsPart.codeUnitAt(0) | 0x20;
-            if (digitsPart.codeUnitAt(i) > maxCharCode) {
-              return handleError(source);
-            }
-          }
+      return _parseError(source, handleError);
+    }
+
+    if (radix is! int) throw new ArgumentError("Radix is not an integer");
+    if (radix < 2 || radix > 36) {
+      throw new RangeError("Radix $radix not in range 2..36");

[Lasse Reichstein Nielsen, 2015/03/06 13:06:51]

You can change this one to:
  throw new RangeError.range(radix, 2, 36, "radix");

[sra1, 2015/03/06 21:38:39]

Done.

+    }
+    if (radix == 10 && match[decimalIndex] != null) {
+      // Cannot fail because we know that the digits are all decimal.
+      return JS('int', r'parseInt(#, 10)', source);
+    }
+    if (radix < 10 || match[decimalIndex] == null) {
+      // We know that the characters must be ASCII as otherwise the
+      // regexp wouldn't have matched. Lowercasing by doing `| 0x20` is thus
+      // guaranteed to be a safe operation, since it preserves digits
+      // and lower-cases ASCII letters.

[Lasse Reichstein Nielsen, 2015/03/06 13:06:51]

How about doing parseInt first, and only scanning the string if it seems to have
not accounted for all digits?

Maybe:

  var s = match[digitsIndex];
  // count digits
  int digits = s.length;
  // skip leading zeros.  
  // - or maybe just let anything with leading zeros go slow-case.
  for (int i = 0; i < s.length; i++) {
    if (s.codeUnitAt(i) == 0x30) {
      digits--;
    } else {
      break;
    }
  }
  var result = JS("num", "parseInt(#, #)", s, radix);
  if (result.isNaN) return _parseError(source, handleError);
  // The result is correct for a valid prefix of s. If that prefix is
  // all of s, then the result can be returned - that is, if all digits 
  // are used.
  // Math.pow should be logarithmic on integers.
  // (Could there be rounding issues?).
  if (result >= Math.pow(radix, digits)) return result;
  // Otherwise check digits.
  // Fallback in case of leading zeros (if not counted above) and 
  // maybe rounding errors?
  int digitLimit = radix < 10 ? 0x30 + radix : 0x61 - 10 + radix;
  for (int i = 0; i < s.length; i++) {
    if ((s.codeUnitAt(i) | 0x20) >= digitLimit) {
      return _parsError(source, handleError)
    }
  }
  return result;

Alternatively just do the digit check and then use parseInt "safely"?

[sra1, 2015/03/06 21:38:38]

Another idea is to have a table of per-radix regexps.  They will scan faster
than anything we can write by hand.

The downside of these ideas is that it is more code.  Everything in this method
is a tax over JavaScript.

+      int maxCharCode;
+      if (radix <= 10) {
+        // Allow all digits less than the radix. For example 0, 1, 2 for
+        // radix 3.
+        // "0".codeUnitAt(0) + radix - 1;
+        maxCharCode = 0x30 + radix - 1;

[Lasse Reichstein Nielsen, 2015/03/06 13:06:51]

Maybe: 0x30 - 1 + radix
to constant fold (0x30-1) into 0x2f.
If the output is already 0x2f+radix, bonus points to the compiler.

[sra1, 2015/03/06 21:38:38]

Done.

+      } else {
+        // Letters are located after the digits in ASCII. Therefore we
+        // only check for the character code. The regexp above made already
+        // sure that the string does not contain anything but digits or
+        // letters.
+        // "a".codeUnitAt(0) + (radix - 10) - 1;
+        maxCharCode = 0x61 + radix - 10 - 1;

[Lasse Reichstein Nielsen, 2015/03/06 13:06:51]

Similar constant folding possible.

[sra1, 2015/03/06 21:38:38]

Done.

+      }
+      assert(match[digitsIndex] is String);
+      String digitsPart = JS('String', '#[#]', match, digitsIndex);
+      for (int i = 0; i < digitsPart.length; i++) {
+        int characterCode = digitsPart.codeUnitAt(i) | 0x20;
+        if (characterCode > maxCharCode) {
+          return _parseError(source, handleError);
         }
       }
     }
-    if (match == null) return handleError(source);
-    return JS('num', r'parseInt(#, #)', source, radix);
+    return JS('int', r'parseInt(#, #)', source, radix);

[Lasse Reichstein Nielsen, 2015/03/06 13:06:51]

Make a comment that the checks above guarantee that all the digits are valid and
there are at least one of them, so parseInt can't return NaN.

[sra1, 2015/03/06 21:38:39]

Done.

   }
 
   static double parseDouble(String source, double handleError(String source)) {
     checkString(source);
-    if (handleError == null) handleError = _throwFormatException;
     // Notice that JS parseFloat accepts garbage at the end of the string.
     // Accept only:
     // - [+/-]NaN
@@ -672,7 +671,7 @@ class Primitives {
             r'/^\s*[+-]?(?:Infinity|NaN|'
                 r'(?:\.\d+|\d+(?:\.\d*)?)(?:[eE][+-]?\d+)?)\s*$/.test(#)',
             source)) {
-      return handleError(source);
+      return _parseError(source, handleError);
     }
     var result = JS('num', r'parseFloat(#)', source);
     if (result.isNaN) {
@@ -680,7 +679,7 @@ class Primitives {
       if (trimmed == 'NaN' || trimmed == '+NaN' || trimmed == '-NaN') {
         return result;
       }
-      return handleError(source);
+      return _parseError(source, handleError);
     }
     return result;
   }