[Webkit-unassigned] [Bug 144955] [ES6] Implement ES6 arrow function syntax. Parser of arrow function with execution as common function

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=144955

--- Comment #81 from Yusuke Suzuki <utatane.tea at gmail.com> ---
Comment on attachment 255506
  --> https://bugs.webkit.org/attachment.cgi?id=255506
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=255506&action=review

Great. I think the current token modification code has several issues. I suggest one solution. Please point it if there's an issue.

> Source/JavaScriptCore/parser/Parser.cpp:358
> +        TreeStatement arrowfunctionStatement = parseArrowFunctionSingleExpressionBody(context, functionParseType);

OK, I understand.
This inner branch is intended to be used for ArrowFunctionExpression + ExpressionBody case because in body case ((x) => { } case), ARROWFUNCTION token is already consumed.
However, in reparsing phase, functionParseType becomes StandardFunctionParseType (I think it should be fixed in the separated patch).

The problematic case is that,

{
    => x;
}

This is avoided because,
1. If the current phase is not reparsing phase, functionParseType is restricted to arrow function parse type.
2. If the current phase is reparsing phase, these code is once parsed in non reparsing phase. In reparsing phase, there's no syntax error. So above case is already avoided in the non reparsing phase.

> Source/JavaScriptCore/parser/Parser.cpp:365
> +        if ((functionParseType == ArrowFunctionParseType && isEndOfArrowFunction()) || !arrowfunctionStatement) {

Why this branch is needed?
Is it possible to write just the following here?

        propagateError();
        return sourceElements;

Since this branch is only used for body = expr case. So if we parse one assignment expression in parseArrowFunctionSingleExpressionBody, no further parsing should not be allowed.

> Source/JavaScriptCore/parser/Parser.cpp:368
> +        }

Is here's fall through correct?

> Source/JavaScriptCore/parser/Parser.cpp:610
> +        failDueToUnexpectedToken();

OK, please insert comment about reparsing phase and parseType relation like the following.

// When reparsing phase, parseType becomes StandardFunctionParseType even if the function is arrow function.
// This condition considers the following situations.
// (1): If we are in the reparsing phase, this arrow function is already parsed once, so there is no syntax error.
// (2): But if we are not in the reparsing phase, we should check this function is called in the context of the arrow function.

> Source/JavaScriptCore/parser/Parser.cpp:631
> +    if (parseType == StandardFunctionParseType)

Change it to `m_lexer->isReparsing()`. `parseType == StandardFunctionParseType` is not essential.

BTW, this code path is a little bit complicated. I recommend to insert comment here.

// Only when reparsing phase, our parsing target contains the EndOfArrowFunction token at the end of the code.
// So consuming the last token here is needed.

Correct?

> Source/JavaScriptCore/parser/Parser.cpp:1567
> +        }

Drop them.

> Source/JavaScriptCore/parser/Parser.cpp:1574
> +            next();

Drop this.

> Source/JavaScriptCore/parser/Parser.cpp:1590
> +            info.startFunctionOffset = m_token.m_data.offset;

This line is not needed since it's already set.
I think info.startFunctionOffset should be set before looking up the cache and should not be touched after that.

> Source/JavaScriptCore/parser/Parser.cpp:1625
> +            info.endFunctionOffset = location.endOffset + 1;

Modifying offset is dangerous I think. It easily leads heap overflow.
And as described below, it leads `parameters.endFunctionStartOffset > parameters.endFunctionEndOffset`.

I suggest always storing locationBeforeLastToken() in bodyexpression case. And drop prevTerminator information.
It ensures the following invariant.

1. the stored token is always the last token in the function's body. if it using braces ({ ... }), closed brace is stored. And if it takes expr style ((x) => x), the last token `x` is stored.

And when recovering from it, always consume the token.

> Source/JavaScriptCore/parser/Parser.cpp:1629
> +        info.isEndByTerminator = m_lexer->prevTerminator();

Drop isEndByTerminator information.

> Source/JavaScriptCore/parser/Parser.cpp:1647
> +        parameters.endFunctionEndOffset = endFunctionEndOffset;

In `m_lexer->prevTerminator()` case, `info.endFunctionOffset` = `location.endOffset + 1`. And `endFunctionEndOffset` is `location.endOffset`.
So now, `parameters.endFunctionStartOffset > parameters.endFunctionEndOffset`. Is it indended?

> Source/JavaScriptCore/parser/Parser.cpp:1663
> +    if ((parseType == ArrowFunctionParseType && info.functionBodyType == ArrowFunctionBodyBlock) || parseType != ArrowFunctionParseType) {

Use `parseType == StandardFunctionParseType` instead of `parseType != ArrowFunctionParseType`.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20150625/77c39647/attachment.html>