846406 - Implement arrow functions

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

As of now this is in sec. 13-2 of the draft spec:
  http://people.mozilla.org/~jorendorff/es6-draft.html#sec-13.2

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: WIP 1 - many broken tests

Comment 2

[Jason Orendorff [:jorendorff]]

Work to do that I can think of offhand:
- `() => x` arrow functions with no arguments don't work yet
- `(...rest) => x` arrow functions with rest arguments don't work yet
- 'this' should be lexical, unlike other functions
- arrow functions should have no 'arguments' binding, unlike other functions
- FunctionToString is moderately broken

Comment 3

[Jason Orendorff [:jorendorff]]

Attachment: WIP 2 - implement arrow functions with block bodies

needs tests.

Comment 4

[Jason Orendorff [:jorendorff]]

More TODO items:
- tests for arrow-block functions
- Reflect.parse support
- ban yield in arrow functions

Comment 5

[Jason Orendorff [:jorendorff]]

fiveop, a volunteer, wants to take on the "() => body" syntax.

Just did Reflect.parse; I'll work on rest arguments next...

Comment 6

[Jason Orendorff [:jorendorff]]

Attachment: WIP 3 - adding rest parameters and Reflect.parse support

Comment 7

[Jason Orendorff [:jorendorff]]

Attachment: WIP 4 - tweaks for rest params, more tests, ban 'yield' in arrow functions

Comment 8

[Jason Orendorff [:jorendorff]]

Attachment: v5

OK, I'm punting lexical 'this' to follow-up bug 848062 so that this can land.

Comment 9

[Brian Hackett [Laid off!]]

Comment on attachment 721380 [details] [diff] [review]
v5

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

::: js/src/frontend/Parser.cpp
@@ +1483,4 @@
>  {
> +    FunctionBox *funbox = pc->sc->asFunctionBox();
> +
> +    bool parenFree = false;

Maybe name this 'parenFreeArrow' to be more descriptive.

@@ +1603,5 @@
>                  if (!defineArg(funcpn, name, disallowDuplicateArgs, &duplicatedArg))
>                      return false;
>  
>                  if (tokenStream.matchToken(TOK_ASSIGN)) {
> +                    JS_ASSERT(!parenFree);

What guarantees this assert?

@@ +4803,5 @@
>  #endif
>  
> +    // Save two source locations, both just in case we turn out to be in an
> +    // arrow function. 'offset' is for FunctionToString. 'start' is for
> +    // rewinding and reparsing arguments.

This seems unfortunate, can you describe a bit more in the comment why this is necessary?

@@ +4807,5 @@
> +    // rewinding and reparsing arguments.
> +    if (tokenStream.getToken() == TOK_ERROR)
> +        return null();
> +    size_t offset = tokenStream.offsetOfToken(tokenStream.currentToken());
> +    tokenStream.ungetToken();

It seems like you could just compute offset in the TOK_ARROW case of the switch, since you will be seeking back to the start of the ::assignExpr anyways.

Comment 10

[Jason Orendorff [:jorendorff]]

(In reply to Brian Hackett (:bhackett) from comment #9)
> ::: js/src/frontend/Parser.cpp
> >                  if (tokenStream.matchToken(TOK_ASSIGN)) {
> > +                    JS_ASSERT(!parenFree);
> 
> What guarantees this assert?

I added a comment:

                // A default argument without parentheses would look like:
                // a = expr => body, but both operators are right-associative, so
                // that would have been parsed as a = (expr => body) instead.
                // Therefore it's impossible to get here with parenFreeArrow.
                JS_ASSERT(!parenFreeArrow);

> > +    // rewinding and reparsing arguments.
> > +    if (tokenStream.getToken() == TOK_ERROR)
> > +        return null();
> > +    size_t offset = tokenStream.offsetOfToken(tokenStream.currentToken());
> > +    tokenStream.ungetToken();
> 
> It seems like you could just compute offset in the TOK_ARROW case of the
> switch, since you will be seeking back to the start of the ::assignExpr
> anyways.

Yup. Done.

Comment 11

[Nicholas Nethercote [inactive]]

This appears to have landed.  Please add the URL for the patch.

Comment 12

[Nicholas Nethercote [inactive]]

Comment on attachment 721380 [details] [diff] [review]
v5

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

::: js/src/frontend/TokenStream.h
@@ +558,5 @@
>      }
>  
>      TokenKind peekToken() {
>          if (lookahead != 0) {
> +            JS_ASSERT(lookahead <= maxLookahead);

Given the following line, shouldn't this be |1 <= maxLookahead|?

@@ +573,5 @@
>      }
>  
>      TokenKind peekTokenSameLine(unsigned withFlags = 0) {
> +        if (lookahead != 0) {
> +            JS_ASSERT(lookahead <= maxLookahead);

Ditto.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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

Comment 14

[Philipp Matthias Schäfer [:fiveop]]

@Comment 12: I think both asserts are unnecessary. We (should) make sure that |lookahead| stays within bounds in the ungetToken() function. And as soon as |lookahead > 0| holds, we know that in |tokens[cursor + 1]| is the token after current Token, so executing the 'then' clause is fine.

Comment 15

[Nicholas Nethercote [inactive]]

> @Comment 12: I think both asserts are unnecessary.

I agree :)  Feel free to make that change in bug 846933.