[Webkit-unassigned] [Bug 156147] [JSC] implement async functions proposal

[bugzilla-daemon at webkit.org]

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

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

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

Nice.

Seeing the code, it seems that AsyncArrowFunctionMode / AsyncArrowFunctionBodyMode cases are sometimes missing.
Can AsyncArrowFunctionBodyMode become an arrow function?

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:245
> +    bool containsArrowOrEvalButNotInArrowBlock = ((functionNode->usesArrowFunction() && functionNode->doAnyInnerArrowFunctionsUseAnyFeature()) || functionNode->usesEval()) && (!m_codeBlock->isArrowFunction() && !m_codeBlock->isAsyncArrowFunction());

Can we change this to the new `isArrowFunction()` including ArrowFunction / AsyncArrowFunction?
Or is there any obstacles?

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:624
> +        emitLabel(didNotThrow.get());

This phase is used when we encounter `async function (param = throwError()) { }`, right?
Looks correct.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:630
> +    if (SourceParseMode::ArrowFunctionMode == parseMode || SourceParseMode::AsyncArrowFunctionBodyMode == parseMode || SourceParseMode::AsyncArrowFunctionMode == parseMode || SourceParseMode::AsyncArrowFunctionMode == parseMode) {

AsyncArrowFunctionMode is duplicated.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:631
> +        if (functionNode->usesThis() || functionNode->usesSuperProperty() || isSuperUsedInInnerArrowFunction())

Nice.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2906
> +    ASSERT(func->metadata()->parseMode() == SourceParseMode::ArrowFunctionMode || func->metadata()->parseMode() == SourceParseMode::AsyncArrowFunctionMode);

Is it safe for AsyncArrowFunctionBodyMode with single line expression?

> Source/JavaScriptCore/parser/Parser.cpp:255
> +    bool isArrowFunctionBodyExpression = parseMode == SourceParseMode::AsyncArrowFunctionBodyMode && !match(OPENBRACE);

Can we move this `parseMode == SourceParseMode::AsyncArrowFunctionBodyMode` to L263?

> Source/JavaScriptCore/parser/Parser.cpp:552
> +        asyncFunctionBodyScope->setSourceParseMode(innerParseMode);

This part is changed in the latest trunk, so rebasing. :)

> Source/JavaScriptCore/parser/Parser.cpp:619
> +            // Eagerly parse as AsyncFUnctionDeclaration. This is the uncommon case,

Typo: AsyncFUnctionDeclaration => AsyncFunctionDeclaration

> Source/JavaScriptCore/parser/Parser.cpp:625
> +                result = parseAsyncFunctionDeclaration(context);

Let's break here, call restoreSavePoint(savePoint); and fall through to the await / field path.

> Source/JavaScriptCore/parser/Parser.cpp:1796
> +                    result = parseAsyncFunctionDeclaration(context);

These code is largely the same to the FUNCTION case. So extracting this to some function is recommended.

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

Nit: breaking here may clean up the following code :)

> 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();
> +        }

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.

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

Is it necessary to create a save point here? Can we just call `semanticFailIfFalse(match(FUNCTION) && !m_lexer->prevTerminator(), "...")` ?

> Source/JavaScriptCore/parser/Parser.cpp:3318
>      }

Handling async arrow function here seems natural :)

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

Is this safe for `async get value() { }`?

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

Which tests need this?

> Source/JavaScriptCore/parser/Parser.cpp:4221
> +                failIfFalse(base, "Failed to parse async function expression");

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).

https://tc39.github.io/ecmascript-asyncawait/#prod-UnaryExpression

> 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;

Could you extract this to static function?

> Source/JavaScriptCore/runtime/CommonIdentifiers.h:36
> +    macro(AsyncFunction) \

Let's remove this.

-- 
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/20160506/71ad9d4a/attachment-0001.html>