169795 - Array.concat(function) doesn't add function (or anything with .length but no elements) to the array

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[George Vanous]

Executing this code demonstrates how Mozilla adds "undefined" instead of the
function using Array.concat(f):

    var f = function() { alert(1); };
    var a = [].concat(f);
    alert(typeof a[0]); // Mozilla outputs `undefined`; IE6 correctly outputs
`function`

Comment 1

[Phil Schwartau]

Confirming bug in the current JS shell. Function objects are not
getting pushed onto the output array of Array.prototype.concat():

js> x = 'Hi';
Hi
js> [].concat(x);
Hi

js> x = 1;
1
js> [].concat(x);
1

js> x = {};
[object Object]
js> [].concat(x);
[object Object]


js> x = function() {}

function () {
}

js> [].concat(x);
                  <<<------------------- x WAS NOT PUSHED ONTO THE ARRAY !!!
js> [].concat(x).length;
0


By contrast, if we wrap the function object in an array,
Array.prototype.concat() will successfully push it:


js> x = function() {}

function () {
}

js> [].concat([x])

function () {
}

Comment 2

[Phil Schwartau]

Testcase added to JS testsuite:

      mozilla/js/tests/ecma_3/Array/15.4.4.4-001.js

Currently failing in both SpiderMonkey and in Rhino.
Have filed bug 169830 against Rhino for this issue -

Comment 3

[Igor Bukanov]

It seems both SM and Rhino failed to perform check for Array object as required
by Ecma262v3, 15.4.4.4:

When the concat method is called with zero or more arguments item1, item2, etc.,
it returns an array
containing the array elements of the object followed by the array elements of
each argument in order.
The following steps are taken:
1.  Let A be a new array created as if by the expression new Array().
2.  Let n be 0.
3.  Let E be this object.
4.  If E is not an Array object, go to step 16.
5.  Let k be 0.
6.  Call the [[Get]] method of E with argument "length".
7.  If k equals Result(6) go to step 19.
8.  Call ToString(k).
9.  If E has a property named by Result(8), go to step 10, but if E has no
property named by
Result(8), go to step 13.
10.  Call ToString(n).
11.  Call the [[Get]] method of E with argument Result(8).
12.  Call the [[Put]] method of A with arguments Result(10) and Result(11).
13.  Increase n by 1.
14.  Increase k by 1.
15.  Go to step 7.
16.  Call ToString(n).
17.  Call the [[Put]] method of A with arguments Result(16) and E.
18.  Increase n by 1.
19.  Get the next argument in the argument list; if there are no more arguments,
go to step 22.
20.  Let E be Result(19).
21.  Go to step 4.
22.  Call the [[Put]] method of A with arguments "length" and n.


Effectively the step 4 is always passed and as function has the length property,
the code will try to add into resulting array not the function but its
properties with indexes 0,1..length.

Here is another 2 tests to show the problem with the same behavior in SM and Rhino:


js> var x={length: 0}
js> [].concat(x).length
0
js> var y={length: 2, 0:0, 1:1}
js> [].concat(y).length         
2
js> [].concat(y)                
0,1
js> 

Comment 4

[Brendan Eich [:brendan]]

Attachment: proposed fix

There was an XXXmccabe telltale suggesting a further improvement beyond the fix
to this bug: roll the unrolled obj case up into the loop over argv, by noticing
that argv[-1] == OBJECT_TO_JSVAL(obj).

Quick r= and I'll check this in?

/be

Comment 5

[Brendan Eich [:brendan]]

Got the patch, taking the bug.

/be

Comment 6

[rogerl (gone)]

alength isn't getting set any more, so the inner for-loop is not so good?

Comment 7

[Brendan Eich [:brendan]]

Attachment: remember to caffeinate before patching in the a.m.

Better this time.

/be

Comment 8

[Brendan Eich [:brendan]]

Tabs still haunt jsarray.c; I'll expand all tabs in one fell swoop befor 1.5RC5,
or some such, so don't mind any peeking out from the patch.

/be

Comment 9

[rogerl (gone)]

Running test suite with patch, fails "tests/js1_2/Array/general2.js":

Array.push,pop,shift,unshift,slice,splice = false FAILED! expected: true

Comment 10

[rogerl (gone)]

+                if (!OBJ_GET_PROPERTY(cx, obj,
+                                      (jsid)cx->runtime->atomState.lengthAtom,
+                                      &v)) {

should be

+                if (!OBJ_GET_PROPERTY(cx, aobj,
+                                      (jsid)cx->runtime->atomState.lengthAtom,
+                                      &v)) {

Comment 11

[Phil Schwartau]

I have added Igor's two examples to the testcase for this bug:

      mozilla/js/tests/ecma_3/Array/15.4.4.4-001.js

The latest patch fixes Igor's examples, and the original problem
(Section 5 of test). However, Sections 6 and 7 of the test now fail:


-------------------------  BEFORE PATCH  -------------------------
*-* Testcase ecma_3/Array/15.4.4.4-001.js failed:
Failure messages were:

FAILED!: Section 5 of test -
FAILED!: Expected value '
FAILED!: function () {
FAILED!:     return "Hello";
FAILED!: }
FAILED!: '
FAILED!: Actual value ''

FAILED!: Section 9 of test -
FAILED!: Expected value '[object Object]', Actual value ''

FAILED!: Section 10 of test -
FAILED!: Expected value '[object Object]', Actual value '0,1'


-------------------------  AFTER PATCH  -------------------------
*-* Testcase ecma_3/Array/15.4.4.4-001.js failed:
Failure messages were:

FAILED!: Section 6 of test -
FAILED!: Expected value '
FAILED!: function () {
FAILED!:     return "Hello";
FAILED!: }
FAILED!: '
FAILED!: Actual value ''

FAILED!: Section 7 of test -
FAILED!: Expected value '1,2,3,4,5,6', Actual value ''

Comment 12

[Brendan Eich [:brendan]]

Attachment: gurgle, groan

Two cups of tea and I'm ready for anything.

/be

Comment 13

[Brendan Eich [:brendan]]

Comment on attachment 99999 [details] [diff] [review]
gurgle, groan

Imputing r=rogerl from his willingness to test and fix this!

Checking in.

/be

Comment 14

[Brendan Eich [:brendan]]

Fixed.

/be

Comment 15

[Phil Schwartau]

Verified FIXED.

I tested the latest patch, and it passed all sections of the 
above testcase. In addition, it passed the JS testsuite on WinNT
in both the debug and optimized JS shell.