Implement ES6 array and generator comprehensions

RESOLVED FIXED in mozilla30

Status

()

RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: wingo, Assigned: wingo)

Tracking

(Blocks: 1 bug, {dev-doc-complete, feature, relnote})

unspecified
mozilla30
dev-doc-complete, feature, relnote
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(relnote-firefox 30+)

Details

(Whiteboard: [js:p1][DocArea=JS])

Attachments

(5 attachments)

(Assignee)

Description

5 years ago
Patches to be attached implement ES6 array and generator comprehensions.
(Assignee)

Updated

5 years ago
Assignee: nobody → wingo
(Assignee)

Comment 5

5 years ago
(Assignee)

Updated

5 years ago
Attachment #8386091 - Flags: review?(jorendorff)
(Assignee)

Updated

5 years ago
Attachment #8386092 - Flags: review?(jorendorff)
(Assignee)

Updated

5 years ago
Attachment #8386093 - Flags: review?(jorendorff)
(Assignee)

Updated

5 years ago
Attachment #8386094 - Flags: review?(jorendorff)
(Assignee)

Updated

5 years ago
Attachment #8386095 - Flags: review?(jorendorff)
(Assignee)

Comment 6

5 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.
(Assignee)

Comment 7

5 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.
(Assignee)

Comment 9

5 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.
Keywords: feature
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [js:p1]
Keywords: dev-doc-needed
Whiteboard: [js:p1] → [js:p1][DocArea=JS]
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)
Attachment #8386091 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 11

5 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
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
Attachment #8386092 - Flags: review?(jorendorff) → review+
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"
Attachment #8386093 - Flags: review?(jorendorff) → review+
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.
Attachment #8386094 - Flags: review?(jorendorff) → review+
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...
Attachment #8386095 - Flags: review?(jorendorff) → review+
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.
(Assignee)

Updated

5 years ago
Duplicate of this bug: 883468

Updated

5 years ago
relnote-firefox: --- → ?
Keywords: relnote

Comment 20

5 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/...).
Flags: needinfo?(wingo)
(Assignee)

Comment 21

5 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.
Flags: needinfo?(wingo)
(Assignee)

Comment 22

5 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.
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).
Flags: needinfo?(wingo)
(Assignee)

Comment 24

5 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.
Flags: needinfo?(wingo)
relnote-firefox: ? → 30+

Updated

5 years ago
QA Whiteboard: [qa-]
(Assignee)

Comment 25

5 years ago
The release notes appear to be unchanged.  Needinfo'ing teoli for a response on comment 24.
Flags: needinfo?(jypenator)
Don't have time to changed this. My next window about it is likely right before Beta. Maybe earlier, but days are too short.
Flags: needinfo?(jypenator)
Oh, and it is a wiki, so you can help if you want.
(Assignee)

Comment 28

5 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...
Depends on: 1015578
Keywords: dev-doc-needed → dev-doc-complete
Depends on: 1122334
You need to log in before you can comment on or make changes to this bug.