The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla2.0b3

Status

()

Core
JavaScript Engine
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: gal, Assigned: Waldo)

Tracking

({dev-doc-complete})

Trunk
mozilla2.0b3
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment)

(Reporter)

Description

7 years ago
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
(Reporter)

Comment 1

7 years ago
This is an ES5 violation.
...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.
Blocks: 445494
Summary: TM: array functions don't work on non-arrays → ES5: array functions don't work on non-arrays
Keywords: dev-doc-needed
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.
Assignee: general → jwalden+bmo
Status: NEW → ASSIGNED
Attachment #457436 - Flags: review?(igor)
OS: Mac OS X → All
Hardware: x86 → All

Comment 4

7 years ago
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.
Attachment #457436 - Flags: review?(igor) → review+
http://hg.mozilla.org/tracemonkey/rev/a3946d490610
Whiteboard: fixed-in-tracemonkey
Target Milestone: --- → mozilla2.0b2
http://hg.mozilla.org/mozilla-central/rev/a3946d490610
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: mozilla2.0b2 → mozilla2.0b3

Comment 7

7 years ago
http://hg.mozilla.org/mozilla-central/rev/a3946d490610

Updated

6 years ago
Depends on: 635389
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.
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.)
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
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.