Andy Wingo [:wingo] Assignee
	Description • 10 years ago

Patches to be attached implement ES6 array and generator comprehensions.

	Andy Wingo [:wingo] Assignee
	Comment 6 • 10 years ago

Comment on attachment 8386093 [details] [diff] [review]
Part 3: Implement ES6 array comprehensions

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

::: js/src/frontend/Parser.cpp
@@ +6859,5 @@
>           * Mark empty arrays as non-constant, since we cannot easily
>           * determine their type.
>           */
>          handler.setListFlag(literal, PNX_NONCONST);
> +    } else if (tokenStream.matchToken(TOK_FOR)) {

Probably I need to set TokenStream::Operand here to allow regexps through; though since the operand is already in the pushback buffer it has no effect now, still it seems a good idea.

	Andy Wingo [:wingo] Assignee
	Comment 7 • 10 years ago

Differences between ES6 comprehensions and JS 1.8 comprehensions:

  * ES6 comprehensions create one scope per "for" node -- not one for the comprehension as a whole.

  * ES6 comprehensions start with the "for" instead of the assignmentexpr.

  * ES6 comprehensions are valid in web-mode code.  They disallow "let" as an identifier.

  * I currently do not implement destructuring in ES6 comprehensions, as it is exposed to the web, and I don't know if SM destructuring is ES6-compatible.

  * ES6 comprehensions can have multiple "comprehensionIf" components, whose tail is a comprehensionTail (i.e. can be a comprehensionFor/comprehensionIf/etc.)

  * ES6 comprehensions should make a fresh binding on each iteration of a for; see bug 449811.  The situation is the same with standard for-of, so when bug 449811 is fixed we will see the benefit here as well.

That's all that I could see from the spec.

	Andy Wingo [:wingo] Assignee
	Comment 8 • 10 years ago

Try build: https://tbpl.mozilla.org/?tree=Try&rev=b81e6b62fb8e

	Andy Wingo [:wingo] Assignee
	Comment 9 • 10 years ago

A couple more differences.

  * ES6 comprehensions only do for-of iteration, not for-in iteration.

  * There are some cases in which generator comprehensions don't need to be parenthesized, in JS 1.8.  ES6 generator comprehensions always need parentheses, though.  See the note at the end of Parser.cpp.

  * ES6 generator comprehensions are ES6 generators, not legacy generators.

	Jason Orendorff [:jorendorff]
	Comment 10 • 10 years ago

Comment on attachment 8386091 [details] [diff] [review]
Part 1: Refactor comprehension parsing

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

::: js/src/frontend/Parser.cpp
@@ +6286,5 @@
> +                                        nullptr, ComprehensionHeadBlockScopeDepth(pc));
> +    if (!comp)
> +        return null();
> +
> +    handler.setEndPosition(comp, pos().end);

Thank you for adding this setEndPosition() call. But it can be moved into comprehensionTail, right?

Node positions are part of parsing; it's bogus for a parser method to return a node with its position unset.

(Ideally the node for a production is created immediately when we finish parsing it, and the position baked into it at that point, never to change again. Relying on follow-up mutation, as we have here, makes bugs of omission possible, which code reviews tend to miss.)

Assuming you make this change, please eliminate any redundant set-end-position code after the other comprehensionTail() call site.

@@ +6292,5 @@
> +    array->append(comp);
> +
> +    MUST_MATCH_TOKEN(TOK_RB, JSMSG_BRACKET_AFTER_ARRAY_COMPREHENSION);
> +
> +    return array;

I think you have to setEndPosition(array, pos().end) here to pick up the closing ']'.

(Or if you prefer:
    TokenPos p = handler.getPosition(array);
    p.end = pos().end;
    return handler.newArrayComprehension(comp, arrayPush, p);
and add the new handler method.)

@@ +6333,5 @@
>          return null();
> +    yieldExpr->setInParens(true);
> +
> +    // A statement to wrap the yield expression.
> +    ParseNode *yieldStmt = handler.newUnary(PNK_SEMI, JSOP_NOP, kid->pn_pos.begin, yieldExpr);

= handler.newExprStatement(yieldExpr, kid->pn_pos.end)

	Andy Wingo [:wingo] Assignee
	Comment 11 • 10 years ago

Comment on attachment 8386093 [details] [diff] [review]
Part 3: Implement ES6 array comprehensions

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

::: js/src/frontend/Parser.cpp
@@ +6515,5 @@
> +template <typename ParseHandler>
> +typename ParseHandler::Node
> +Parser<ParseHandler>::comprehensionTail(GeneratorKind comprehensionKind)
> +{
> +    if (tokenStream.matchToken(TOK_FOR))

Probably need a TokenStream::Operand here too

	Jason Orendorff [:jorendorff]
	Comment 12 • 10 years ago

Comment on attachment 8386092 [details] [diff] [review]
Part 2: Internally rename JS1.8 comprehensions as "legacy"

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

::: js/src/frontend/Parser.cpp
@@ +5780,5 @@
>   *
>   * NB: This is not a general tree transplanter -- it knows in particular that
>   * the one or more bindings induced by V have not yet been created.
>   */
> +class LegacyCompExprTransplanter

<3 <3 <3

@@ +6675,5 @@
>  {
>      JS_ASSERT(tokenStream.isCurrentTokenType(TOK_LB));
>  
> +    uint32_t begin = pos().begin;
> +    Node literal = handler.newArrayLiteral(begin, pc->blockidGen);

Does this belong in a different patch?

::: js/src/js.msg
@@ +262,5 @@
>  MSG_DEF(JSMSG_BAD_ANON_GENERATOR_RETURN, 209, 0, JSEXN_TYPEERR, "anonymous generator function returns a value")
>  MSG_DEF(JSMSG_PROTO_SETTING_SLOW,     210, 0, JSEXN_TYPEERR, "mutating the [[Prototype]] of an object will cause your code to run very slowly; instead create the object with the correct initial [[Prototype]] value using Object.create")
>  MSG_DEF(JSMSG_IN_AFTER_FOR_NAME,      211, 0, JSEXN_SYNTAXERR, "missing 'in' or 'of' after for")
>  MSG_DEF(JSMSG_BAD_TRAP_RETURN_VALUE,  212, 2, JSEXN_TYPEERR,"trap {1} for {0} returned a primitive value")
> +MSG_DEF(JSMSG_OF_AFTER_FOR_NAME,      213, 0, JSEXN_SYNTAXERR, "missing 'of' after for")

Same here

	Jason Orendorff [:jorendorff]
	Comment 13 • 10 years ago

Comment on attachment 8386093 [details] [diff] [review]
Part 3: Implement ES6 array comprehensions

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

::: js/src/frontend/Parser.cpp
@@ +6431,5 @@
> +    MUST_MATCH_TOKEN(TOK_LP, JSMSG_PAREN_AFTER_FOR);
> +
> +    // FIXME: Here we could allow destructuring, as in legacyComprehensionTail.
> +    // First we need to verify that SM's destructuring actually matches the
> +    // specification, and change it as needed.

Please file a follow-up bug for this, blocking bug 694100, and cite it here. (No need for the comment to spell out what needs to be done; the open bug will should a better job.)

@@ +6470,5 @@
> +        return null();
> +    Node letScope = pushLetScope(blockObj, &stmtInfo);
> +    if (!letScope)
> +        return null();
> +    handler.setLexicalScopeBody(letScope, decls);

Phew. All this looks good.

@@ +6514,5 @@
> +
> +template <typename ParseHandler>
> +typename ParseHandler::Node
> +Parser<ParseHandler>::comprehensionTail(GeneratorKind comprehensionKind)
> +{

Please add a JS_CHECK_RECURSION call here.

@@ +6540,5 @@
> +    Node yieldStmt = handler.newUnary(PNK_SEMI, JSOP_NOP, begin, yieldExpr);
> +    if (!yieldStmt)
> +        return null();
> +
> +    return yieldStmt;

No 'yieldStmt' local variable is necessary; you can just return the result of the handler call directly. But also, use handler.newExprStatement().

@@ +6545,5 @@
> +}
> +
> +/*
> + * Starting from a |for| keyword after the first array initialiser element or
> + * an expression in an open parenthesis, parse the tail of the comprehension

This comment is wrong -- here the 'for' keyword appears right after the '[' or '(' token.

@@ +6549,5 @@
> + * an expression in an open parenthesis, parse the tail of the comprehension
> + * or generator expression signified by this |for| keyword in context.
> + *
> + * Return null on failure, else return the top-most parse node for the array
> + * comprehension or generator expression, with a unary node as the body of the

"a unary node" -> "a PNK_ARRAYPUSH or PNK_YIELD node"

	Jason Orendorff [:jorendorff]
	Comment 14 • 10 years ago

Comment on attachment 8386094 [details] [diff] [review]
Part 4: Implement ES6 generator comprehensions

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

::: js/src/frontend/Parser.cpp
@@ +2054,5 @@
> +        pn->pn_body->append(body);
> +        pn->pn_body->pn_pos = body->pn_pos;
> +    } else {
> +        // This case happens when parsing ES6 generator comprehensions.
> +        pn->pn_body = body;

Hmm. Right now, I think the invariant is that all functions that have a PNK_ARGSBODY go through this method, and generator-expression functions do not.

Please either strengthen the invariant to "all functions go through this method", or keep generator-comprehension functions out of it as well --- and the latter might be the right call; it looks like it'll save a few lines of code.

@@ +6610,5 @@
> +
> +    // We have no problem parsing generator comprehensions inside lazy
> +    // functions, but the bytecode emitter currently can't handle them that way,
> +    // because when it goes to emit the code for the inner generator function,
> +    // it expects outer functions to have non-lazy scripts.

(Oh. I wondered, when you originally asked, why the parser doesn't bail out anyway due to the let-scoping. Now I see.)

@@ +6621,5 @@
> +        return null();
> +    handler.setOp(genfn, JSOP_LAMBDA);
> +
> +    {
> +        ParseContext<ParseHandler> *outerpc = pc;

Please factor common code out of this block (at least) and legacyGeneratorExpr(). We may be stuck with the legacy generators for years; in the meantime, let's not have two copies of all the bugs...

@@ +6627,5 @@
> +        RootedFunction fun(context, newFunction(outerpc, /* atom = */ NullPtr(), Expression));
> +        if (!fun)
> +            return null();
> +
> +        /* Create box for fun->object early to protect against last-ditch GC. */

"to protect against last-ditch GC" -> "to root it" (also in the original, if you don't mind)

@@ +6659,5 @@
> +        if (!body)
> +            return null();
> +
> +        if (!tokenStream.matchToken(TOK_RP)) {
> +            report(ParseError, false, null(), JSMSG_BAD_GENEXP_BODY, js_yield_str);

I don't think this is the right error message here.

Perhaps: MUST_MATCH_TOKEN(TOK_RP, JSMSG_PAREN_IN_PAREN);

@@ -7228,5 @@
> -            return null();
> -        pn = handler.setInParens(pn);
> -
> -        if (!genexp)
> -            MUST_MATCH_TOKEN(TOK_RP, JSMSG_PAREN_IN_PAREN);

Thank you for deleting this horribleness!

@@ +7450,5 @@
> +//   if (_)
> +//   while (_) {}
> +//   do {} while (_)
> +//   switch (_) {}
> +//   with (_) {}

Wow, in hindsight all these are *crazy*.

@@ +7451,5 @@
> +//   while (_) {}
> +//   do {} while (_)
> +//   switch (_) {}
> +//   with (_) {}
> +//   foo(_) // must be first and only argument

This last form is handy, though, in Python:

    sum(x.mass for x in samples)

Oh well.

	Jason Orendorff [:jorendorff]
	Comment 15 • 10 years ago

Comment on attachment 8386095 [details] [diff] [review]
Part 5: Add tests

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

::: js/src/tests/ecma_6/Comprehensions/arguments.js
@@ +1,4 @@
> +
> +
> +function values(g) {
> +  return [for (x of g) x];

This one seems a little messed up. values() is never called?

::: js/src/tests/ecma_6/Comprehensions/generator-semantics.js
@@ +2,5 @@
> +function a1() {
> +  var a = 10;
> +  var g = (for (y of [0]) eval('var a=42;'));
> +  g.next();
> +  return a;

Buhhhhhh, ok!

::: js/src/tests/ecma_6/Comprehensions/nested-for-if.js
@@ +9,5 @@
> +
> +function primesBetween6And25() {
> +  return [for (n of range(6, 25)) if (n % 2) if (n % 3) if (n % 5) n];
> +}
> +assertDeepEq([7,11,13,17,19,23], primesBetween6And25());

Contrary to reportCompare, the order of arguments to assertEq and assertDeepEq is (actual, expected), so:

assertDeepEq([for (n of range(6, 25)) if (n % 2) if (n % 3) if (n % 5) n],
             [7, 11, 13, 17, 19, 23]);

This affects all these tests... don't bother changing it if you find you don't care. It only affects the error message in case a test should fail...

	André Bargull [:anba]
	Comment 16 • 10 years ago

Comment on attachment 8386093 [details] [diff] [review]
Part 3: Implement ES6 array comprehensions

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

Two minor comments from me:

::: js/src/frontend/Parser.cpp
@@ +6444,5 @@
> +        report(ParseError, false, null(), JSMSG_OF_AFTER_FOR_NAME);
> +        return null();
> +    }
> +
> +    Node rhs = expr();

assignExpr() instead of expr(), also see bug 901987 and https://bugs.ecmascript.org/show_bug.cgi?id=1163.

@@ +6500,5 @@
> +    JS_ASSERT(tokenStream.isCurrentTokenType(TOK_IF));
> +
> +    uint32_t begin = pos().begin;
> +
> +    Node cond = condition();

condition() accepts any parenthesised expression, whereas ComprehensionIf is restricted to AssignmentExpression.

	Andy Wingo [:wingo] Assignee
	Comment 17 • 10 years ago

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/aab169fa66be
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f0fc695a395d
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f4216dc177e4
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1cb9462ced39
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/653d9da6c5a2

Thanks to both of you for the review; I think I covered everything in those patches.  Cheers :)

	Phil Ringnalda (:philor)
	Comment 18 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/aab169fa66be
https://hg.mozilla.org/mozilla-central/rev/f0fc695a395d
https://hg.mozilla.org/mozilla-central/rev/f4216dc177e4
https://hg.mozilla.org/mozilla-central/rev/1cb9462ced39
https://hg.mozilla.org/mozilla-central/rev/653d9da6c5a2

	Ioana (away)
	Comment 20 • 10 years ago

Is there any need for QA here? I see the feature is already covered by some automated tests (
js/src/tests/ecma_6/Comprehensions/...).

	Andy Wingo [:wingo] Assignee
	Comment 21 • 10 years ago

I think the existing automated tests are sufficient.  It's not an entirely new mechanism, as it reuses comprehension machinery from JS1.8, so there's not that much new code; QA would only be for semantics, and I think we have that covered.

	Andy Wingo [:wingo] Assignee
	Comment 22 • 10 years ago

FWIW I wrote an article on this feature here: http://wingolog.org/archives/2014/03/07/es6-generator-and-array-comprehensions-in-spidermonkey

That might help the people writing the release notes.

	Jean-Yves Perrier [:teoli]
	Comment 23 • 10 years ago

Andy: I mainly used comment 7 and 9 for https://developer.mozilla.org/en-US/Firefox/Releases/30
Can you double check? (I will add a link to your blog post too).

	Andy Wingo [:wingo] Assignee
	Comment 24 • 10 years ago

(In reply to Jean-Yves Perrier [:teoli] from comment #23)
> Andy: I mainly used comment 7 and 9 for
> https://developer.mozilla.org/en-US/Firefox/Releases/30
> Can you double check? (I will add a link to your blog post too).

Thanks for writing that up; I think there are some things to fix though.  Note that these are both array and generator comprehensions (two different things).

The list above (and in the release notes, and in my blog post) are basically for people that already know about JS1.8 comprehensions and are interested in the differences.  (Note that the existing JS1.8 comprehensions are still there in js1.8 mode; the text seemed to imply they went away.)

I don't think starting with a list of differences is that helpful to the general reader; instead we should describe the feature as if it were new, and then later provide a list of differences to JS1.8 comprehensions if the reader is familiar with them.

How about this draft text:

"Firefox now implements ES6 array and generator comprehensions.  Array comprehensions allow you to easily iterate over a sequence and collect the results into a fresh array.  For example:

  [for (item of iterable) process(item)]

Generator comprehensions are similar, except they produce their results on demand instead of all at once.  Generator expressions are written with round parentheses instead of square brackets:

  (for (item of iterable) process(item))

For more details, including the differences between ES6 comprehensions and the older JS1.7/JS1.8 comprehensions, see XXX.

	Andy Wingo [:wingo] Assignee
	Comment 25 • 10 years ago

The release notes appear to be unchanged.  Needinfo'ing teoli for a response on comment 24.

	Jean-Yves Perrier [:teoli]
	Comment 26 • 10 years ago

Don't have time to changed this. My next window about it is likely right before Beta. Maybe earlier, but days are too short.

	Jean-Yves Perrier [:teoli]
	Comment 27 • 10 years ago

Oh, and it is a wiki, so you can help if you want.

	Andy Wingo [:wingo] Assignee
	Comment 28 • 10 years ago

I spent time writing new draft text for you and you don't have time to paste it into the document?  I don't have an account on the wiki.  Whatever though...

	Tooru Fujisawa [:arai]
	Comment 29 • 10 years ago

Updated following documents:
  https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Generator_comprehensions

Following documents are already updated:
  https://developer.mozilla.org/en-US/Firefox/Releases/30
  https://developer.mozilla.org/en-US/docs/Web/JavaScript/New_in_JavaScript/ECMAScript_7_support_in_Mozilla
  https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Array_comprehensions