Last Comment Bug 562446 - ES5: array functions don't work on non-arrays
: ES5: array functions don't work on non-arrays
Status: RESOLVED FIXED
fixed-in-tracemonkey
: dev-doc-complete
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla2.0b3
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on: 635389
Blocks: es5
  Show dependency treegraph
 
Reported: 2010-04-28 13:22 PDT by Andreas Gal :gal
Modified: 2011-03-08 12:38 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch and tests (7.82 KB, patch)
2010-07-14 16:27 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
igor: review+
Details | Diff | Splinter Review

Description Andreas Gal :gal 2010-04-28 13:22:08 PDT
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 2010-04-28 13:25:59 PDT
This is an ES5 violation.
Comment 2 Jeff Walden [:Waldo] (remove +bmo to email) 2010-04-28 14:18:31 PDT
...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] (remove +bmo to email) 2010-07-14 16:27:09 PDT
Created attachment 457436 [details] [diff] [review]
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 2010-07-15 03:34:06 PDT
Comment on attachment 457436 [details] [diff] [review]
Patch and tests

># HG changeset patch
># Parent f48dbb0de1a89c0b62add248aaba002d6de22136
># User Jeff Walden <jwalden@mit.edu>
>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 <jwalden+code@mit.edu>
>+ */
>+
>+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] (remove +bmo to email) 2010-07-16 13:08:07 PDT
http://hg.mozilla.org/tracemonkey/rev/a3946d490610
Comment 6 Jeff Walden [:Waldo] (remove +bmo to email) 2010-08-04 16:43:37 PDT
http://hg.mozilla.org/mozilla-central/rev/a3946d490610
Comment 8 Eric Shepherd [:sheppy] 2011-03-08 09:46:43 PST
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] (remove +bmo to email) 2011-03-08 11:16:41 PST
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] 2011-03-08 12:38:18 PST
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

Note You need to log in before you can comment on or make changes to this bug.