<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - [JSC] implement async functions proposal"
href="https://bugs.webkit.org/show_bug.cgi?id=156147#c73">Comment # 73</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - [JSC] implement async functions proposal"
href="https://bugs.webkit.org/show_bug.cgi?id=156147">bug 156147</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=277938&action=diff" name="attach_277938" title="Patch">attachment 277938</a> <a href="attachment.cgi?id=277938&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=277938&action=review">https://bugs.webkit.org/attachment.cgi?id=277938&action=review</a>
Nice.
Seeing the code, it seems that AsyncArrowFunctionMode / AsyncArrowFunctionBodyMode cases are sometimes missing.
Can AsyncArrowFunctionBodyMode become an arrow function?
<span class="quote">> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:245
> + bool containsArrowOrEvalButNotInArrowBlock = ((functionNode->usesArrowFunction() && functionNode->doAnyInnerArrowFunctionsUseAnyFeature()) || functionNode->usesEval()) && (!m_codeBlock->isArrowFunction() && !m_codeBlock->isAsyncArrowFunction());</span >
Can we change this to the new `isArrowFunction()` including ArrowFunction / AsyncArrowFunction?
Or is there any obstacles?
<span class="quote">> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:624
> + emitLabel(didNotThrow.get());</span >
This phase is used when we encounter `async function (param = throwError()) { }`, right?
Looks correct.
<span class="quote">> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:630
> + if (SourceParseMode::ArrowFunctionMode == parseMode || SourceParseMode::AsyncArrowFunctionBodyMode == parseMode || SourceParseMode::AsyncArrowFunctionMode == parseMode || SourceParseMode::AsyncArrowFunctionMode == parseMode) {</span >
AsyncArrowFunctionMode is duplicated.
<span class="quote">> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:631
> + if (functionNode->usesThis() || functionNode->usesSuperProperty() || isSuperUsedInInnerArrowFunction())</span >
Nice.
<span class="quote">> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2906
> + ASSERT(func->metadata()->parseMode() == SourceParseMode::ArrowFunctionMode || func->metadata()->parseMode() == SourceParseMode::AsyncArrowFunctionMode);</span >
Is it safe for AsyncArrowFunctionBodyMode with single line expression?
<span class="quote">> Source/JavaScriptCore/parser/Parser.cpp:255
> + bool isArrowFunctionBodyExpression = parseMode == SourceParseMode::AsyncArrowFunctionBodyMode && !match(OPENBRACE);</span >
Can we move this `parseMode == SourceParseMode::AsyncArrowFunctionBodyMode` to L263?
<span class="quote">> Source/JavaScriptCore/parser/Parser.cpp:552
> + asyncFunctionBodyScope->setSourceParseMode(innerParseMode);</span >
This part is changed in the latest trunk, so rebasing. :)
<span class="quote">> Source/JavaScriptCore/parser/Parser.cpp:619
> + // Eagerly parse as AsyncFUnctionDeclaration. This is the uncommon case,</span >
Typo: AsyncFUnctionDeclaration => AsyncFunctionDeclaration
<span class="quote">> Source/JavaScriptCore/parser/Parser.cpp:625
> + result = parseAsyncFunctionDeclaration(context);</span >
Let's break here, call restoreSavePoint(savePoint); and fall through to the await / field path.
<span class="quote">> Source/JavaScriptCore/parser/Parser.cpp:1796
> + result = parseAsyncFunctionDeclaration(context);</span >
These code is largely the same to the FUNCTION case. So extracting this to some function is recommended.
<span class="quote">> Source/JavaScriptCore/parser/Parser.cpp:1797
> + }</span >
Nit: breaking here may clean up the following code :)
<span class="quote">> Source/JavaScriptCore/parser/Parser.cpp:2268
> + if (mode != SourceParseMode::AsyncArrowFunctionMode) {
> + semanticFailIfTrue(generatorBodyScope->hasDirectSuper(), "super is not valid in this context");
> + if (generatorBodyScope->needsSuperBinding())
> + semanticFailIfTrue(expectedSuperBinding == SuperBinding::NotNeeded, "super can only be used in a method of a derived class");
> + } else {
> + if (generatorBodyScope->hasDirectSuper())
> + functionScope->setHasDirectSuper();
> + if (generatorBodyScope->needsSuperBinding())
> + functionScope->setNeedsSuperBinding();
> + }</span >
These checks are now done in parsing location. Let's remove this.
When removing this, maybe, we need to recheck that closestParentOrdinaryFunctionNonLexicalScope works correctly with async arrow wrapper / body functions.
<span class="quote">> Source/JavaScriptCore/parser/Parser.cpp:3197
> + next();</span >
Is it necessary to create a save point here? Can we just call `semanticFailIfFalse(match(FUNCTION) && !m_lexer->prevTerminator(), "...")` ?
<span class="quote">> Source/JavaScriptCore/parser/Parser.cpp:3318
> }</span >
Handling async arrow function here seems natural :)
<span class="quote">> Source/JavaScriptCore/parser/Parser.cpp:3602
> + }</span >
Is this safe for `async get value() { }`?
<span class="quote">> Source/JavaScriptCore/parser/Parser.cpp:4014
> + }</span >
Which tests need this?
<span class="quote">> Source/JavaScriptCore/parser/Parser.cpp:4221
> + failIfFalse(base, "Failed to parse async function expression");</span >
Why is this handled here? Can we handle `async function` case in parsePrimaryExpression?
And can we parse async arrow function in parseAssignmentExpression? (At that case, we check `async IDENT =>` in parsePrimaryExpression similar to the current arrow function parsing).
<a href="https://tc39.github.io/ecmascript-asyncawait/#prod-UnaryExpression">https://tc39.github.io/ecmascript-asyncawait/#prod-UnaryExpression</a>
<span class="quote">> Source/JavaScriptCore/runtime/CodeCache.cpp:226
> + ConstructAbility constructAbility;
> + switch (metadata->parseMode()) {
> + case SourceParseMode::NormalFunctionMode:
> + // let allocKind be "constructor".
> + constructAbility = ConstructAbility::CanConstruct;
> + break;
> +
> + case SourceParseMode::GeneratorBodyMode:
> + case SourceParseMode::GeneratorWrapperFunctionMode:
> + case SourceParseMode::GetterMode:
> + case SourceParseMode::SetterMode:
> + case SourceParseMode::MethodMode:
> + case SourceParseMode::ArrowFunctionMode:
> + case SourceParseMode::AsyncFunctionBodyMode:
> + case SourceParseMode::AsyncArrowFunctionBodyMode:
> + case SourceParseMode::AsyncFunctionMode:
> + case SourceParseMode::AsyncMethodMode:
> + case SourceParseMode::AsyncArrowFunctionMode:
> + // let allocKind be "non-constructor".
> + constructAbility = ConstructAbility::CannotConstruct;
> + break;
> +
> + case SourceParseMode::ProgramMode:
> + case SourceParseMode::ModuleAnalyzeMode:
> + case SourceParseMode::ModuleEvaluateMode:
> + RELEASE_ASSERT_NOT_REACHED();
> + break;</span >
Could you extract this to static function?
<span class="quote">> Source/JavaScriptCore/runtime/CommonIdentifiers.h:36
> + macro(AsyncFunction) \</span >
Let's remove this.</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>