<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - [ES6] Implement ES6 arrow function syntax. Parser of arrow function with execution as common function"
href="https://bugs.webkit.org/show_bug.cgi?id=144955#c81">Comment # 81</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - [ES6] Implement ES6 arrow function syntax. Parser of arrow function with execution as common function"
href="https://bugs.webkit.org/show_bug.cgi?id=144955">bug 144955</a>
from <span class="vcard"><a class="email" href="mailto:utatane.tea@gmail.com" title="Yusuke Suzuki <utatane.tea@gmail.com>"> <span class="fn">Yusuke Suzuki</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=255506&action=diff" name="attach_255506" title="Patch">attachment 255506</a> <a href="attachment.cgi?id=255506&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=255506&action=review">https://bugs.webkit.org/attachment.cgi?id=255506&action=review</a>
Great. I think the current token modification code has several issues. I suggest one solution. Please point it if there's an issue.
<span class="quote">> Source/JavaScriptCore/parser/Parser.cpp:358
> + TreeStatement arrowfunctionStatement = parseArrowFunctionSingleExpressionBody(context, functionParseType);</span >
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.
<span class="quote">> Source/JavaScriptCore/parser/Parser.cpp:365
> + if ((functionParseType == ArrowFunctionParseType && isEndOfArrowFunction()) || !arrowfunctionStatement) {</span >
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.
<span class="quote">> Source/JavaScriptCore/parser/Parser.cpp:368
> + }</span >
Is here's fall through correct?
<span class="quote">> Source/JavaScriptCore/parser/Parser.cpp:610
> + failDueToUnexpectedToken();</span >
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.
<span class="quote">> Source/JavaScriptCore/parser/Parser.cpp:631
> + if (parseType == StandardFunctionParseType)</span >
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?
<span class="quote">> Source/JavaScriptCore/parser/Parser.cpp:1567
> + }</span >
Drop them.
<span class="quote">> Source/JavaScriptCore/parser/Parser.cpp:1574
> + next();</span >
Drop this.
<span class="quote">> Source/JavaScriptCore/parser/Parser.cpp:1590
> + info.startFunctionOffset = m_token.m_data.offset;</span >
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.
<span class="quote">> Source/JavaScriptCore/parser/Parser.cpp:1625
> + info.endFunctionOffset = location.endOffset + 1;</span >
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.
<span class="quote">> Source/JavaScriptCore/parser/Parser.cpp:1629
> + info.isEndByTerminator = m_lexer->prevTerminator();</span >
Drop isEndByTerminator information.
<span class="quote">> Source/JavaScriptCore/parser/Parser.cpp:1647
> + parameters.endFunctionEndOffset = endFunctionEndOffset;</span >
In `m_lexer->prevTerminator()` case, `info.endFunctionOffset` = `location.endOffset + 1`. And `endFunctionEndOffset` is `location.endOffset`.
So now, `parameters.endFunctionStartOffset > parameters.endFunctionEndOffset`. Is it indended?
<span class="quote">> Source/JavaScriptCore/parser/Parser.cpp:1663
> + if ((parseType == ArrowFunctionParseType && info.functionBodyType == ArrowFunctionBodyBlock) || parseType != ArrowFunctionParseType) {</span >
Use `parseType == StandardFunctionParseType` instead of `parseType != ArrowFunctionParseType`.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>