802155 - Reflect.parse should handle for-of comprehension correctly

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Yusuke Suzuki]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_8_2) AppleWebKit/537.4 (KHTML, like Gecko) Chrome/22.0.1229.94 Safari/537.4

Steps to reproduce:

Compared Reflect.parse('[x for (x in [])]') and Reflect.parse('[x for (x of [])]')


Actual results:

They are the same.


Expected results:

Reflect.parse should handle for-of comprehension correctly.

Comment 1

[Yusuke Suzuki]

I suggest adding of flag to ComprehensionBlock.

Comment 2

[Yusuke Suzuki]

Attachment: rev1 patch

Attached rev1 patch.
This patch changes builder.comprehensionBlock & ComprehensionBlock interface.

Comment 3

[Yusuke Suzuki]

I've opened the same issue to Esprima.
http://code.google.com/p/esprima/issues/detail?id=359

Comment 4

[Jason Orendorff [:jorendorff]]

Comment on attachment 671862 [details] [diff] [review]
rev1 patch

Review of attachment 671862 [details] [diff] [review]:
-----------------------------------------------------------------

The only thing missing here is a test. See js/src/tests/js1_8_5/extensions/reflect-parse.js and search for comprehensions.

Clearing the review flag. Please set it r?me again if you post a new patch.

Sorry for the very slow review here. I've been out a bit.

Comment 5

[Yusuke Suzuki]

Attachment: rev2 patch

(In reply to Jason Orendorff [:jorendorff] from comment #4)
Thanks for your review.
I've just attached revised patch that contains test cases.

Comment 6

[Jason Orendorff [:jorendorff]]

Comment on attachment 677284 [details] [diff] [review]
rev2 patch

Review of attachment 677284 [details] [diff] [review]:
-----------------------------------------------------------------

I'll push this Monday.

::: js/src/tests/js1_8_5/extensions/reflect-parse.js
@@ +836,5 @@
> +           genExpr(arrExpr([ident("x"), ident("y")]), [compOfBlock(ident("x"), ident("foo")), compOfBlock(ident("y"), ident("bar"))], ident("p")));
> +assertExpr("( [x,y,z] for (x of foo) for (y of bar) for (z of baz) if (p) )",
> +           genExpr(arrExpr([ident("x"), ident("y"), ident("z")]),
> +                   [compOfBlock(ident("x"), ident("foo")), compOfBlock(ident("y"), ident("bar")), compOfBlock(ident("z"), ident("baz"))],
> +                   ident("p")));

Thanks.

Comment 7

[Jason Orendorff [:jorendorff]]

Completely forgot about this. Found it in my queue and pushed it today.

https://hg.mozilla.org/integration/mozilla-inbound/rev/b7fd23ced4ee

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b7fd23ced4ee

Comment 9

[Yusuke Suzuki]

Thanks!