Last Comment Bug 846406 - Implement arrow functions
: Implement arrow functions
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: All All
: -- normal (vote)
: mozilla22
Assigned To: Jason Orendorff [:jorendorff]
: general
:
Mentors:
Depends on: 848062 848197 852761 852762 882877 885067 885219 1101265
Blocks: es6
  Show dependency treegraph
 
Reported: 2013-02-28 11:00 PST by Jason Orendorff [:jorendorff]
Modified: 2014-11-25 10:51 PST (History)
18 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP 1 - many broken tests (37.76 KB, patch)
2013-02-28 11:15 PST, Jason Orendorff [:jorendorff]
no flags Details | Diff | Splinter Review
WIP 2 - implement arrow functions with block bodies (43.78 KB, patch)
2013-02-28 12:38 PST, Jason Orendorff [:jorendorff]
no flags Details | Diff | Splinter Review
WIP 3 - adding rest parameters and Reflect.parse support (56.17 KB, patch)
2013-02-28 16:42 PST, Jason Orendorff [:jorendorff]
no flags Details | Diff | Splinter Review
WIP 4 - tweaks for rest params, more tests, ban 'yield' in arrow functions (63.02 KB, patch)
2013-03-01 13:09 PST, Jason Orendorff [:jorendorff]
no flags Details | Diff | Splinter Review
v5 (61.89 KB, patch)
2013-03-05 11:41 PST, Jason Orendorff [:jorendorff]
bhackett1024: review+
Details | Diff | Splinter Review

Description Jason Orendorff [:jorendorff] 2013-02-28 11:00:20 PST
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] 2013-02-28 11:15:59 PST
Created attachment 719598 [details] [diff] [review]
WIP 1 - many broken tests
Comment 2 Jason Orendorff [:jorendorff] 2013-02-28 11:18:15 PST
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] 2013-02-28 12:38:17 PST
Created attachment 719637 [details] [diff] [review]
WIP 2 - implement arrow functions with block bodies

needs tests.
Comment 4 Jason Orendorff [:jorendorff] 2013-02-28 12:43:31 PST
More TODO items:
- tests for arrow-block functions
- Reflect.parse support
- ban yield in arrow functions
Comment 5 Jason Orendorff [:jorendorff] 2013-02-28 13:26:06 PST
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] 2013-02-28 16:42:24 PST
Created attachment 719756 [details] [diff] [review]
WIP 3 - adding rest parameters and Reflect.parse support
Comment 7 Jason Orendorff [:jorendorff] 2013-03-01 13:09:51 PST
Created attachment 720109 [details] [diff] [review]
WIP 4 - tweaks for rest params, more tests, ban 'yield' in arrow functions
Comment 8 Jason Orendorff [:jorendorff] 2013-03-05 11:41:42 PST
Created attachment 721380 [details] [diff] [review]
v5

OK, I'm punting lexical 'this' to follow-up bug 848062 so that this can land.
Comment 9 Brian Hackett (:bhackett) 2013-03-07 03:25:15 PST
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] 2013-03-15 15:27:21 PDT
(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 [:njn] 2013-03-18 21:22:29 PDT
This appears to have landed.  Please add the URL for the patch.
Comment 12 Nicholas Nethercote [:njn] 2013-03-18 21:35:21 PDT
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] 2013-03-19 07:26:23 PDT
https://hg.mozilla.org/mozilla-central/rev/bf3ce88c6ea3
Comment 14 Philipp Matthias Schäfer [:fiveop] 2013-03-19 12:51:14 PDT
@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 [:njn] 2013-03-19 13:46:17 PDT
> @Comment 12: I think both asserts are unnecessary.

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

Note You need to log in before you can comment on or make changes to this bug.