Improve decoding of JS TypeError.

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

Index: dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart
diff --git a/dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart b/dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart
index 1b78b32d343cd25eb64cb1492c4c62de784d16d8..ceec5c42d5c0bf5eba82064aaf4a8e6730db176c 100644
--- a/dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart
+++ b/dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart
@@ -694,7 +694,6 @@ class Primitives {
                        List positionalArguments,
                        Map<String, dynamic> namedArguments) {
     int argumentCount = 0;
-    StringBuffer buffer = new StringBuffer();
     List arguments = [];
 
     if (positionalArguments != null) {
@@ -702,6 +701,8 @@ class Primitives {
       arguments.addAll(positionalArguments);
     }
 
+    String selectorName = 'call\$$argumentCount';
+
     // Sort the named arguments to get the right selector name and
     // arguments order.
     if (namedArguments != null && !namedArguments.isEmpty) {
@@ -714,12 +715,11 @@ class Primitives {
       // sort implementation, we use the JavaScript one instead.
       JS('void', '#.sort()', listOfNamedArguments);
       listOfNamedArguments.forEach((String name) {
-        buffer.add('\$$name');
+        selectorName = '$selectorName\$$name';
         arguments.add(namedArguments[name]);
       });
     }
 
-    String selectorName = 'call\$$argumentCount$buffer';
     var jsFunction = JS('var', '#[#]', function, selectorName);
     if (jsFunction == null) {
       throw new NoSuchMethodError(function, selectorName, arguments, {});
@@ -962,6 +962,98 @@ throwAbstractClassInstantiationError(className) {
   throw new AbstractClassInstantiationError(className);
 }
 
+class TypeErrorDecoder {
+  static final noSuchMethodPattern =

[kasperl, 2013/01/21 13:54:09]

I find that this method is really hard to read and grok. Maybe there's a way to
improve that through formatting and helper methods or named constant strings?

[sra1, 2013/01/29 20:54:16]

In addition to Kaspers suggestions, don't forget that documentation can also
make this more comprehensible.

It took me a long time to understand that this code forces an error with known
components in order to generate a pattern for matching errors.  A comment to
that effect would have saved me 30 minutes in trying to understand what is going
on here.

[ahe, 2013/01/29 22:06:46]

Stephen, I'm sorry you spent so much time reviewing this version. I guess I
uploaded this version while you had an older version open. I hope you find the
newer version easier to understand.

+      extractPattern(JS('',
+                        r'{ toString: function() { return "$receiver$"; } }'));
+  static final nullErrorPattern = extractPattern(JS('', 'null'));
+  static final undefinedErrorPattern = extractPattern(JS('', 'void 0'));
+
+  static extractPattern(expression) {
+    var function = JS('', r"""function($expr$) {

[sra1, 2013/01/29 20:54:16]

Why are these names $name$?
If they are not substituted, I'd rather they be 'normal'.

I'm confused. See comment above.

[ahe, 2013/01/29 22:06:46]

I haven't documented why I use "$name$", not "name". I will do so.

The reason is that I'm trying to inject something known into something unknown.
The unknown entity is the error message that the browser provides with a
TypeError. It is a human readable message, possibly localized in a language I
don't understand. I'm guessing that $name$ would never naturally occur in a
human readable error message, yet it is easy to decode.

For example, if I evaluate this in V8 version 3.13.7.6:

var $expr$ = null; $expr$.$method$()

The VM throws an instance of TypeError whose message property contains "Cannot
call method '$method$' of null". I'm reasonable sure that if the string contains
$method$, that's where the method name will be in general.

So I turn it into this regular expression:

"Cannot call method '(.*)' of null"

Similarly, if I evaluate:

var $expr$ = {toString: function() { return '$receiver$'; }}; $expr$.$method$()

I get this message: "Object $receiver$ has no method '$method$'"

Which I turn into this regular expression:

"Object (.*) has no method '(.*)'"

Firefox/jsshell is slightly different, it tries to include the source code that
caused the exception, so I get this message: "$expr$.$method$ is not a function"
which I turn into this regular expression:

"(.*)\\.(.*) is not a function"

+  var $argumentsExpr$ = '$arguments$'
+  try {
+    $expr$.$method$($argumentsExpr$);
+  } catch (e) {
+    return (e.message"""
+// Subtle bug/feature in V8: it no longer calls toString.

[sra1, 2013/01/29 20:54:16]

it? Can you be more specific about what no longer calls toString? V8 or a
certian operation?

[ahe, 2013/01/29 22:06:46]

I looked at my new comment. I can still clarify it a bit. Later versions of V8
no longer calls toString on the receiver, so instead of this message:

"Object $receiver$ has no method '$method$'"

I get:

"Object [object Object] has no method '$method$'"

This is actually quite weird, because if I had not provided a toString method,
it uses a different logic that gives a much better message. For example:

var $expr$ = {}; $expr$.$method$()

Gives:

"Object #<Object> has no method '$method$'"

And 

function Foo() {}; var $expr$ = new Foo(); $expr$.$method$()

Gives:

"Object #<Foo> has no method '$method$'"

+r"""
+        .replace(String({}), '$receiver$')

[sra1, 2013/01/29 20:54:16]

Surely there are other receivers?
Try with some DOM elements, e.g. obfuscate(window).frazzle();

where obfuscate() is a dynamic-returning function that is actually the identity
function but too confusing to the compiler to be inlined or inferenced to return
the input.

[ahe, 2013/01/29 22:06:46]

I don't understand what you're saying.

+        .replace(new RegExp(#, 'g'), '\\$&'));
+  }
+}""", ESCAPE_REGEXP);
+    String message = JS('String', '(#)(#)', function, expression);
+    List<String> match =
+        JS('=List', r"#.match(/\\\$[a-zA-Z]+\\\$/g)", message);
+    int arguments = JS('int', '#.indexOf(#)', match, r'\$arguments\$');
+    int argumentsExpr = JS('int', '#.indexOf(#)', match, r'\$argumentsExpr\$');
+    int expr = JS('int', '#.indexOf(#)', match, r'\$expr\$');
+    int method = JS('int', '#.indexOf(#)', match, r'\$method\$');
+    int receiver = JS('int', '#.indexOf(#)', match, r'\$receiver\$');
+
+    String pattern = JS('String',
+                        r"#.replace('\\$arguments\\$', '(.*)')"

[sra1, 2013/01/29 20:54:16]

It will be slightly better if emitted JavaScript strings have double quotes.

[ahe, 2013/01/29 22:06:46]

Why is that?

+                        r".replace('\\$argumentsExpr\\$',  '(.*)')"
+                        r".replace('\\$expr\\$',  '(.*)')"
+                        r".replace('\\$method\\$',  '(.*)')"
+                        r".replace('\\$receiver\\$',  '(.*)')",
+                        message);
+
+    return JS('',
+              r'{ arguments: #, argumentsExpr: #, expr: #, method: #, '
+              r'receiver: #, pattern: #, '
+              r"""match: function(msg) {
+var match = new RegExp(this.pattern).exec(msg);
+if (!match) return void 0;
+var result = {};
+if (this.arguments != -1) result.arguments = match[this.arguments + 1];
+if (this.argumentsExpr != -1) result.argumentsExpr = match[this.argumentsExpr + 1];
+if (this.expr != -1) result.expr = match[this.expr + 1];
+if (this.method != -1) result.method = match[this.method + 1];
+if (this.receiver != -1) result.receiver = match[this.receiver + 1];
+return result;
+}}""",
+              arguments,
+              argumentsExpr,
+              expr,
+              method,
+              receiver,
+              pattern);
+  }
+}
+
+class NullError implements NoSuchMethodError {
+  final String _message;
+  final String _method;
+
+  NullError(this._message, match)
+    : this._method = JS('', '#.method', match);
+
+  String toString() {
+    if (_method == null) return 'NullError: $_message';
+    return 'NullError: Cannot call "$_method" on null';
+  }
+}
+
+class JsNoSuchMethodError implements NoSuchMethodError {
+  final String _message;
+  final String _method;
+  final String _receiver;
+
+  JsNoSuchMethodError(this._message, match)
+    : this._method = JS('', '#.method', match),
+      this._receiver = JS('', '#.receiver', match);
+
+  String toString() {
+    if (_method == null) return 'NoSuchMethodError: $_message';
+    if (_receiver == null) {
+      return 'NoSuchMethodError: Cannot call "$_method" ($_message)';
+    }
+    return 'NoSuchMethodError: Cannot call "$_method" on "$_receiver" '
+        '($_message)';
+  }
+}
+
 /**
  * Called from catch blocks in generated code to extract the Dart
  * exception from the thrown value. The thrown value may have been
@@ -982,44 +1074,23 @@ unwrapException(ex) {
   var message = JS('var', r'#.message', ex);
 
   if (JS('bool', r'# instanceof TypeError', ex)) {
-    // The type and arguments fields are Chrome specific but they
-    // allow us to get very detailed information about what kind of
-    // exception occurred.
-    var type = JS('var', r'#.type', ex);
-    var name = JS('var', r'#.arguments ? #.arguments[0] : ""', ex, ex);

[ahe, 2013/01/17 01:08:05]

These fields have been removed from V8 (and Chrome).

The current version of V8 in Chrome dev is 3.16.4. The one in the Dart repo is
3.13.7.5 which still has support for type and argument, it is gone in 3.15.11.8
which I just tested locally. There is no stable release of 3.16 yet.

-    if (contains(message, 'JSNull') ||
-        type == 'property_not_function' ||
-        type == 'called_non_callable' ||
-        type == 'non_object_property_call' ||
-        type == 'non_object_property_load') {
-      return new NoSuchMethodError(null, name, [], {});
-    } else if (type == 'undefined_method') {
-      return new NoSuchMethodError('', name, [], {});
+    var match;
+    var nsme = TypeErrorDecoder.noSuchMethodPattern;
+    var nul = TypeErrorDecoder.nullErrorPattern;
+    var undef = TypeErrorDecoder.undefinedErrorPattern;
+    if ((match = JS('', '#.match(#)', nsme, message)) != null) {
+      return new JsNoSuchMethodError(message, match);
+    } else if ((match = JS('', '#.match(#)', nul, message)) != null ||
+               (match = JS('', '#.match(#)', undef, message)) != null) {
+      return new NullError(message, match);
     }
 
     var ieErrorCode = JS('int', '#.number & 0xffff', ex);
     var ieFacilityNumber = JS('int', '#.number>>16 & 0x1FFF', ex);
-    // If we cannot use [type] to determine what kind of exception
-    // we're dealing with we fall back on looking at the exception
-    // message if it is available and a string.
-    if (message is String) {
-      if (message.endsWith('is null') ||
-          message.endsWith('is undefined') ||
-          message.endsWith('is null or undefined') ||
-          message.endsWith('of undefined') ||
-          message.endsWith('of null')) {
-        return new NoSuchMethodError(null, '<unknown>', [], {});
-      } else if (contains(message, ' has no method ') ||
-                 contains(message, ' is not a function') ||
-                 (ieErrorCode == 438 && ieFacilityNumber == 10)) {
-        // Examples:
-        //  x.foo is not a function
-        //  'undefined' is not a function (evaluating 'x.foo(1,2,3)')
-        // Object doesn't support property or method 'foo' which sets the error
-        // code 438 in IE.
-        // TODO(kasperl): Compute the right name if possible.
-        return new NoSuchMethodError('', '<unknown>', [], {});
-      }
+    if (ieErrorCode == 438 && ieFacilityNumber == 10) {
+      // Object doesn't support property or method 'foo' which sets the error
+      // code 438 in IE.
+      return new NoSuchMethodError('', '<unknown>', [], {});
     }
 
     // If we cannot determine what kind of error this is, we fall back
@@ -1655,17 +1726,17 @@ String runtimeTypeToString(type) {
 
 String joinArguments(var types, int startIndex) {
   bool firstArgument = true;
-  StringBuffer buffer = new StringBuffer();
+  String result = '';
   for (int index = startIndex; index < types.length; index++) {
     if (firstArgument) {
       firstArgument = false;
     } else {
-      buffer. add(', ');
+      result = '$result, ';
     }
     var argument = types[index];
-    buffer.add(runtimeTypeToString(argument));
+    result = '$result${runtimeTypeToString(argument)}';
   }
-  return buffer.toString();
+  return result;
 }
 
 String getRuntimeTypeString(var object) {