630232 - Reflect.parse("[,,,]") should make an ArrayExpression with .elements.length 3

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P5)

Description

[Jason Orendorff [:jorendorff]]

Reflect.parse("[]") and Reflect.parse("[,,,]") return identical ASTs, even though those are two different programs.

  js> eval("[]").length
  0
  js> eval("[,,,]").length
  3
  js> assertEq(Reflect.parse("[,,,]").body[0].expression.elements.length, 3)
  typein:11: Error: Assertion failed: got 0, expected 3

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: v1

Taking, since I have a patch lying around.

This bug would have been detected by the tests, but the testing mechanism had a minor bug. Fixing that bug caused the tests to detect bug 638577 (i.e. fail because of it)! So I had to comment out a few lines of tests.

Comment 2

[Dave Herman [:dherman]]

Comment on attachment 516703 [details] [diff] [review]
v1

>-    JSObject *array = NewDenseEmptyArray(cx);
>+    const size_t len = elts.length();
>+    JSObject *array = NewDenseAllocatedArray(cx, uint(len));

Neat -- didn't know about NewDenseAllocatedArray.

>     if (!array)
>         return false;
> 
>-    const size_t len = elts.length();
>     for (size_t i = 0; i < len; i++) {
>         Value val = elts[i];
> 
>         JS_ASSERT_IF(val.isMagic(), val.whyMagic() == JS_SERIALIZE_NO_NODE);
> 
>         /* Represent "no node" as an array hole by not adding the value. */
>         if (val.isMagic(JS_SERIALIZE_NO_NODE))
>             continue;

Aaaaargh, fencepost bug! I haz a sad.

>-assertExpr("[,,,1,2,3,]", arrExpr([,,,lit(1),lit(2),lit(3)]));
>-assertExpr("[,,,1,2,3,,]", arrExpr([,,,lit(1),lit(2),lit(3),]));
>-assertExpr("[,,,1,2,3,,,]", arrExpr([,,,lit(1),lit(2),lit(3),,]));
>-assertExpr("[,,,,,]", arrExpr([,,,,]));
>+assertExpr("[,,,1,2,3,]", arrExpr([,,,lit(1),lit(2),lit(3),]));
>+assertExpr("[,,,1,2,3,,]", arrExpr([,,,lit(1),lit(2),lit(3),,]));
>+assertExpr("[,,,1,2,3,,,]", arrExpr([,,,lit(1),lit(2),lit(3),,,]));
>+assertExpr("[,,,,,]", arrExpr([,,,,,]));

Bogus test case is bogus. Mea culpa.

Looks good! r=me

Comment 3

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/21f05e099a36

Comment 4

[Marco Bonardo [:mak] (Away July 4-13)]

https://hg.mozilla.org/mozilla-central/rev/21f05e099a36