562446 - ES5: array functions don't work on non-arrays

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andreas Gal :gal]

static JSBool
array_toString(JSContext *cx, uintN argc, jsval *vp)
{
    JSObject *obj;

    obj = JS_THIS_OBJECT(cx, vp);
    if (!obj ||
        (obj->getClass() != &js_SlowArrayClass &&
         !JS_InstanceOf(cx, obj, &js_ArrayClass, vp + 2))) {
        return JS_FALSE;
    }

also

toSource, toLocaleString

Comment 1

[Andreas Gal :gal]

This is an ES5 violation.

Comment 2

[Jeff Walden [:Waldo]]

...but not an ES3 violation, for anyone who (like me) remembered the old rule.

While the to*String methods certainly should be changed, I think toSource should be left as is, without adaptation to handle non-array this values.

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Patch and tests

I tend to think the new definition for Array.prototype.toString is...unfortunate, and save for the genericity not really an improvement over ES3, but the spec is what it is.  I continue to maintain that Array.prototype.toSource need not, and should not, be made generic.

Comment 4

[Igor Bukanov]

Comment on attachment 457436 [details] [diff] [review]
Patch and tests

># HG changeset patch
># Parent f48dbb0de1a89c0b62add248aaba002d6de22136
># User Jeff Walden <[e-mail]>
>Bug 562446 - ES5: Array.prototype.toString and Array.prototype.toLocaleString should be generic.  NOT REVIEWED YET
>
>diff --git a/js/src/jsarray.cpp b/js/src/jsarray.cpp
>--- a/js/src/jsarray.cpp
>+++ b/js/src/jsarray.cpp
>@@ -1403,29 +1403,45 @@ array_toString_sub(JSContext *cx, JSObje
> static JSBool
> array_toString(JSContext *cx, uintN argc, jsval *vp)
> {

Add a comment that this follows 15.4.4.2 from ES5.

>-    JSObject *obj;
>-
>-    obj = JS_THIS_OBJECT(cx, vp);
>-    if (!obj ||
>-        (obj->getClass() != &js_SlowArrayClass &&
>-         !JS_InstanceOf(cx, obj, &js_ArrayClass, vp + 2))) {
>+    JSObject *obj = JS_THIS_OBJECT(cx, vp);
>+    if (!obj)
>         return JS_FALSE;

Nit: use false/true in this function.

>+    AutoObjectRooter thisObj(cx, obj);

Even without the conservative GC this autorooter is not necessary. obj comes from JS_THIS_OBJECT and should be rooted as such. 

>+
>+    AutoValueRooter join(cx);
>+    if (!obj->getProperty(cx, ATOM_TO_JSID(cx->runtime->atomState.joinAtom), join.addr()))
>+        return JS_FALSE;
>+
>+    if (!js_IsCallable(join.value())) {
>+        JSString *str = js::obj_toStringHelper(cx, obj);

Nit: no need for js:: prefix.

>diff --git a/js/src/tests/ecma_3/Array/regress-387501.js b/js/src/tests/ecma_3/Array/regress-387501.js
>@@ -56,7 +58,7 @@ function test()
>  
>   try
>   {
>-    expect = 'TypeError: Array.prototype.toString called on incompatible String';
>+    expect = '[object String]';
>     actual = Array.prototype.toString.call((new String('foo')));
>   }
>   catch(ex)

Nit: no need for the catch block here as no exception is expected. Also use assertEq.


>@@ -67,7 +69,7 @@ function test()
> 
>   try
>   {
>-    expect = 'TypeError: Array.prototype.toLocaleString called on incompatible String';
>+    expect = 'f,o,o';
>     actual = Array.prototype.toLocaleString.call((new String('foo')));

Nit: again, replace this with assertEq.

>diff --git a/js/src/tests/ecma_5/Array/toString-01.js b/js/src/tests/ecma_5/Array/toString-01.js
>new file mode 100644
>--- /dev/null
>+++ b/js/src/tests/ecma_5/Array/toString-01.js
>@@ -0,0 +1,34 @@
>+/*
>+ * Any copyright is dedicated to the Public Domain.
>+ * http://creativecommons.org/licenses/publicdomain/
>+ * Contributor:
>+ *   Jeff Walden <[e-mail]>
>+ */
>+
>+var gTestfile = 'toString-01.js';
>+//-----------------------------------------------------------------------------
>+var BUGNUMBER = 562446;
>+var summary = 'ES5: Array.prototype.toString';
>+
>+print(BUGNUMBER + ": " + summary);
>+
>+/**************
>+ * BEGIN TEST *
>+ **************/
>+
>+var o;
>+
>+o = { join: function() { return "ohai"; } };

Add to the function a check that arguments.length is 0.

>+assertEq(Array.prototype.toString.call(o), "ohai");
>+
>+o = {};
>+assertEq(Array.prototype.toString.call(o), "[object Object]");
>+
>+Array.prototype.join = function() { return "kthxbai"; };
>+assertEq(Array.prototype.toString.call([]), "kthxbai");
>+

We need at least two more tests. One should test a join returning non-callable like 0. Another should check that if a join is defined using a getter and the getter throws, than the exception is propagated.

r+ with this fixed.

Comment 5

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/tracemonkey/rev/a3946d490610

Comment 6

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/mozilla-central/rev/a3946d490610

Comment 7

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/a3946d490610

Comment 8

[Eric Shepherd [:sheppy]]

While the descriptions here may be good for people intimately familiar with ES5 specification, that's not me. :)

What's changed here? I'm looking at this and the syntax in the tests is cryptic enough to my non-expert eyes that I'm not sure how to document this change.

Comment 9

[Jeff Walden [:Waldo]]

Basically [].toString, if called with |this| not an Array, will call a join method on the |this| object (if there is one) or Object.prototype.toString otherwise, and return the value that method returns.  The [].toString MDN page already documents this, although I just tweaked the wording a little.  Does that help?  (I assume it still needs to be mentioned on some sort of "what's new" page, so leaving d-d-n as-is.)

Comment 10

[Eric Shepherd [:sheppy]]

Oh-ho. There ya go then. Added a mention to:

https://developer.mozilla.org/en/JavaScript/New_in_JavaScript/1.8.5#New_ECMAScript5_features