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

VERIFIED FIXED in mozilla1.2alpha

Status

()

Core
JavaScript Engine
--
major
VERIFIED FIXED
15 years ago
13 years ago

People

(Reporter: George Vanous, Assigned: brendan)

Tracking

({js1.5})

Trunk
mozilla1.2alpha
x86
Windows 2000
js1.5
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Have filed bug 169830 against Rhino on same issue], URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

15 years ago
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

15 years ago
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 () {
}
Assignee: rogerl → khanson
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 2

15 years ago
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 -
Whiteboard: [Have filed bug 169830 against Rhino on same issue]

Updated

15 years ago
Summary: Array.concat(function) adds "undefined" instead of the function to the array. → Array.concat(function) doesn't add function to the array

Comment 3

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

Comment 4

15 years ago
Created attachment 99984 [details] [diff] [review]
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
(Assignee)

Updated

15 years ago
Assignee: khanson → brendan
Keywords: js1.5, mozilla1.2
Summary: Array.concat(function) doesn't add function to the array → Array.concat(function) doesn't add function (or anything with .length but no elements) to the array
Target Milestone: --- → mozilla1.2alpha
(Assignee)

Comment 5

15 years ago
Got the patch, taking the bug.

/be

Comment 6

15 years ago
alength isn't getting set any more, so the inner for-loop is not so good?
(Assignee)

Comment 7

15 years ago
Created attachment 99990 [details] [diff] [review]
remember to caffeinate before patching in the a.m.

Better this time.

/be
Attachment #99984 - Attachment is obsolete: true
(Assignee)

Comment 8

15 years ago
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

15 years ago
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

15 years ago
+                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

15 years ago
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 ''
(Assignee)

Comment 12

15 years ago
Created attachment 99999 [details] [diff] [review]
gurgle, groan

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

/be
Attachment #99990 - Attachment is obsolete: true
(Assignee)

Comment 13

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

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

Checking in.

/be
Attachment #99999 - Flags: review+
(Assignee)

Comment 14

15 years ago
Fixed.

/be
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Comment 15

15 years ago
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.
Status: RESOLVED → VERIFIED

Updated

13 years ago
Flags: testcase+
You need to log in before you can comment on or make changes to this bug.