Fix Error.prototype.toString to be ES5 conform.

Fix Error.prototype.toString to be ES5 conform.

R=lrn@chromium.org
TEST=test262/15.11.4.4-8-1,mjsunit/error-tostring

Committed: http://code.google.com/p/v8/source/detail?r=9790

[Michael Starzinger, 2011-10-25 16:27:36]

The updated test expectations in cyclic-error-to-string.js are in line with the
current Firefox stable. Unfortunately Safari still shows the old non-ES5
behavior. However I think it is OK for us to move to ES5 in that regard.

[Erik Corry, 2011-10-25 19:22:29]

Is there a bug filed against JSC?

[Lasse Reichstein, 2011-10-26 07:40:24]

LGTM

http://codereview.chromium.org/8341021/diff/1/src/messages.js
File src/messages.js (right):

http://codereview.chromium.org/8341021/diff/1/src/messages.js#newcode1157
src/messages.js:1157: var type = error.type;
The spec doesn't say anything about reading .type. If that's something we use
internally, we should first check (somehow) whether it is an internal error
(i.e., we control .type) before reading or using it, so we don't trigger a
user-visible action (e.g., a getter on "type").

http://codereview.chromium.org/8341021/diff/1/src/messages.js#newcode1158
src/messages.js:1158: var name = error.name
According to the spec, we should do ToString on name before reading message.

http://codereview.chromium.org/8341021/diff/1/src/messages.js#newcode1180
src/messages.js:1180: // the builtins object do not work inside of a catch
clause.
We should check whether this is really (still) the case.

http://codereview.chromium.org/8341021/diff/1/test/mjsunit/cyclic-error-to-st...
File test/mjsunit/cyclic-error-to-string.js (right):

http://codereview.chromium.org/8341021/diff/1/test/mjsunit/cyclic-error-to-st...
test/mjsunit/cyclic-error-to-string.js:32: assertEquals('Error', e + '');
I'd use 
 String(e)
to convert an object to a string. More readable.
In this case, we are testing the toString function, so e.toString() is probably
even better here.

http://codereview.chromium.org/8341021/diff/1/test/mjsunit/cyclic-error-to-st...
test/mjsunit/cyclic-error-to-string.js:38: e.arguments = e;
Why .stack and .arguments? They don't seem to affect (and should not be expected
to affect) the toString function.
It would be better if they actually contained string values that should not
occur in the result. I.e. change them to:
  e.stack = "Does not occur in output";
  e.arguments = "Does not occur in output";
  e.type = "Does not occur in output";
(adding .type just to be sure).

http://codereview.chromium.org/8341021/diff/1/test/mjsunit/cyclic-error-to-st...
test/mjsunit/cyclic-error-to-string.js:39: assertEquals('', e + '');
Seems a little odd that we can get ": " when neither name nor message is the
empty string. I can see that we need to do something, but I'd prefer putting
something else than the empty string in place of recurring objects in the
toString call graph. E.g. "#<error>". But we match Safari, so let's keep it for
now.

http://codereview.chromium.org/8341021/diff/1/test/mjsunit/cyclic-error-to-st...
test/mjsunit/cyclic-error-to-string.js:46: assertEquals('', e + '');
Add a test to check the sequence of visible operations of toString, e.g.,

var seq;
function testSeq(name, message) {
 var e = {
   get name() { 
     seq.push(1); 
     return (name === undefined) ? name : {
         toString: function() { seq.push(2); return name; }; 
     };
   }, 
   get message() {
     seq.push(3);
     return message === undefined ? message : {
       toString: function() { seq.push(4); return message; }
     };
   }
 };
 return Error.prototype.toString.call(e);
}

seq = [];
assertEquals("Error", testSeq(undefined, undefined));
assertEquals([1,3], seq);
seq = [];
assertEquals("e1", testSeq("e1", undefined));
assertEquals([1,2,3], seq);

seq = [];
assertEquals("e2", testSeq(undefined, "e2"));
assertEquals([1,3,4], seq);

seq = [];
assertEquals("e1: e2", testSeq("e1", "e2"));
assertEquals([1,2,3,4], seq);

(not tested).

[Lasse Reichstein, 2011-10-26 08:50:44]

http://codereview.chromium.org/8341021/diff/1/test/mjsunit/cyclic-error-to-st...
File test/mjsunit/cyclic-error-to-string.js (right):

http://codereview.chromium.org/8341021/diff/1/test/mjsunit/cyclic-error-to-st...
test/mjsunit/cyclic-error-to-string.js:30: 
So we have a test of Error.toString that tests that it does what it should in
the non-cyclic case?
I.e., that it does the correct conversion using ToString if not undefined for
name and message (and that, e.g., null isn't treated the same as undefined).

[Michael Starzinger, 2011-10-26 09:07:40]

On 2011/10/25 19:22:29, Erik Corry wrote:
> Is there a bug filed against JSC?

Now there is: https://bugs.webkit.org/show_bug.cgi?id=70889

[Michael Starzinger, 2011-10-26 09:30:20]

Added new patch set. PTAQL.

http://codereview.chromium.org/8341021/diff/1/src/messages.js
File src/messages.js (right):

http://codereview.chromium.org/8341021/diff/1/src/messages.js#newcode1157
src/messages.js:1157: var type = error.type;
On 2011/10/26 07:40:24, Lasse Reichstein wrote:
> The spec doesn't say anything about reading .type. If that's something we use
> internally, we should first check (somehow) whether it is an internal error
> (i.e., we control .type) before reading or using it, so we don't trigger a
> user-visible action (e.g., a getter on "type").

I agree, the spec says nothing about any "type" or "arguments" property. I think
the only correct solution would be to hide those two properties completely from
the JavaScript world. But that is outside the scope of this CL.

http://codereview.chromium.org/8341021/diff/1/src/messages.js#newcode1158
src/messages.js:1158: var name = error.name
On 2011/10/26 07:40:24, Lasse Reichstein wrote:
> According to the spec, we should do ToString on name before reading message.

Done.

http://codereview.chromium.org/8341021/diff/1/src/messages.js#newcode1180
src/messages.js:1180: // the builtins object do not work inside of a catch
clause.
On 2011/10/26 07:40:24, Lasse Reichstein wrote:
> We should check whether this is really (still) the case.

Done (no longer needed, removed).

http://codereview.chromium.org/8341021/diff/1/test/mjsunit/cyclic-error-to-st...
File test/mjsunit/cyclic-error-to-string.js (right):

http://codereview.chromium.org/8341021/diff/1/test/mjsunit/cyclic-error-to-st...
test/mjsunit/cyclic-error-to-string.js:30: 
On 2011/10/26 08:50:44, Lasse Reichstein wrote:
> So we have a test of Error.toString that tests that it does what it should in
> the non-cyclic case?
> I.e., that it does the correct conversion using ToString if not undefined for
> name and message (and that, e.g., null isn't treated the same as undefined).

Done. I have renamed this test case to error-tostring.js, because it now does
way more than just testing the cyclic case.

http://codereview.chromium.org/8341021/diff/1/test/mjsunit/cyclic-error-to-st...
test/mjsunit/cyclic-error-to-string.js:32: assertEquals('Error', e + '');
On 2011/10/26 07:40:24, Lasse Reichstein wrote:
> I'd use 
>  String(e)
> to convert an object to a string. More readable.
> In this case, we are testing the toString function, so e.toString() is
probably
> even better here.

Done.

http://codereview.chromium.org/8341021/diff/1/test/mjsunit/cyclic-error-to-st...
test/mjsunit/cyclic-error-to-string.js:38: e.arguments = e;
On 2011/10/26 07:40:24, Lasse Reichstein wrote:
> Why .stack and .arguments? They don't seem to affect (and should not be
expected
> to affect) the toString function.
> It would be better if they actually contained string values that should not
> occur in the result. I.e. change them to:
>   e.stack = "Does not occur in output";
>   e.arguments = "Does not occur in output";
>   e.type = "Does not occur in output";
> (adding .type just to be sure).

Done.

http://codereview.chromium.org/8341021/diff/1/test/mjsunit/cyclic-error-to-st...
test/mjsunit/cyclic-error-to-string.js:39: assertEquals('', e + '');
On 2011/10/26 07:40:24, Lasse Reichstein wrote:
> Seems a little odd that we can get ": " when neither name nor message is the
> empty string. I can see that we need to do something, but I'd prefer putting
> something else than the empty string in place of recurring objects in the
> toString call graph. E.g. "#<error>". But we match Safari, so let's keep it
for
> now.

I agree, having a more sensible string representation for the cyclic case would
be good. But since it is not specified how to treat cycles, I think staying in
sync with Firefox (and probably Safari once they fix the ES5 issue here) is a
good thing.

http://codereview.chromium.org/8341021/diff/1/test/mjsunit/cyclic-error-to-st...
test/mjsunit/cyclic-error-to-string.js:46: assertEquals('', e + '');
On 2011/10/26 07:40:24, Lasse Reichstein wrote:
> Add a test to check the sequence of visible operations of toString, e.g.,
> 
> var seq;
> function testSeq(name, message) {
>  var e = {
>    get name() { 
>      seq.push(1); 
>      return (name === undefined) ? name : {
>          toString: function() { seq.push(2); return name; }; 
>      };
>    }, 
>    get message() {
>      seq.push(3);
>      return message === undefined ? message : {
>        toString: function() { seq.push(4); return message; }
>      };
>    }
>  };
>  return Error.prototype.toString.call(e);
> }
> 
> seq = [];
> assertEquals("Error", testSeq(undefined, undefined));
> assertEquals([1,3], seq);
> seq = [];
> assertEquals("e1", testSeq("e1", undefined));
> assertEquals([1,2,3], seq);
> 
> seq = [];
> assertEquals("e2", testSeq(undefined, "e2"));
> assertEquals([1,3,4], seq);
> 
> seq = [];
> assertEquals("e1: e2", testSeq("e1", "e2"));
> assertEquals([1,2,3,4], seq);
> 
> (not tested).

Done (added with slight modifications).

[Lasse Reichstein, 2011-10-26 10:28:36]

LGTM!

http://codereview.chromium.org/8341021/diff/2002/src/messages.js
File src/messages.js (right):

http://codereview.chromium.org/8341021/diff/2002/src/messages.js#newcode1165
src/messages.js:1165: message = IS_UNDEFINED(message) ? "" :
TO_STRING_INLINE(message);
Nano-optimization (i.e., feel free to ignore), but if message is undefined, you
can return name directly.
It's probably better to just follow the spec algorithm layout.

http://codereview.chromium.org/8341021/diff/2002/test/mjsunit/error-tostring.js
File test/mjsunit/error-tostring.js (right):

http://codereview.chromium.org/8341021/diff/2002/test/mjsunit/error-tostring....
test/mjsunit/error-tostring.js:29: // Test default String representation of an
Error object.
Lower case "string", otherwise it seems like it's an object.

http://codereview.chromium.org/8341021/diff/2002/test/mjsunit/error-tostring....
test/mjsunit/error-tostring.js:56: // on a given Error object.  Verify the
produced String representation.
Lower case "string" here too.

http://codereview.chromium.org/8341021/diff/2002/test/mjsunit/error-tostring....
test/mjsunit/error-tostring.js:85: assertEquals(["e1: e2",[1,2,3,4]],
testErrorToString("e1", "e2"));
Good stuff!

[Michael Starzinger, 2011-10-26 10:42:50]

Added new patch set. Landed.

http://codereview.chromium.org/8341021/diff/2002/test/mjsunit/error-tostring.js
File test/mjsunit/error-tostring.js (right):

http://codereview.chromium.org/8341021/diff/2002/test/mjsunit/error-tostring....
test/mjsunit/error-tostring.js:29: // Test default String representation of an
Error object.
On 2011/10/26 10:28:36, Lasse Reichstein wrote:
> Lower case "string", otherwise it seems like it's an object.

Done.

http://codereview.chromium.org/8341021/diff/2002/test/mjsunit/error-tostring....
test/mjsunit/error-tostring.js:56: // on a given Error object.  Verify the
produced String representation.
On 2011/10/26 10:28:36, Lasse Reichstein wrote:
> Lower case "string" here too.

Done.