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

[bugzilla-daemon at webkit.org]

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

--- Comment #74 from Caitlin Potter (:caitp) <caitp at igalia.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

I'll have these addressed shortly, thanks for the look

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

Oops, good catch

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

AsyncArrowFunctionBodyMode never gets here, since it's just a normal function

>> Source/JavaScriptCore/parser/Parser.cpp:255
>> +    bool isArrowFunctionBodyExpression = parseMode == SourceParseMode::AsyncArrowFunctionBodyMode && !match(OPENBRACE);
> 
> Can we move this `parseMode == SourceParseMode::AsyncArrowFunctionBodyMode` to L263?

Unfortunately, this matters whether the function is being re-parsed or not --- this breaks if this isn't known here.

There might be a better place to move this to, but it can't be in the `isReparsingFunction()` conditional.

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

Right, thanks

>> Source/JavaScriptCore/parser/Parser.cpp:625
>> +                result = parseAsyncFunctionDeclaration(context);
> 
> Let's break here, call restoreSavePoint(savePoint); and fall through to the await / field path.

Alright

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

Sorry, can you clarify what you mean here?

I'm not sure if you're referring to this entire case statement, or the inside of `parseAsyncFunctionDeclaration` (which could share some code with parseFunctionDeclaration maybe), or what

>> Source/JavaScriptCore/parser/Parser.cpp:1797
>> +                }
> 
> Nit: breaking here may clean up the following code :)

Okay

>> Source/JavaScriptCore/parser/Parser.cpp:2268
>> +        }
> 
> 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.

Yeah, this stuff is gone in the rebase so far --- and seems to do the right thing still

>> 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(), "...")` ?

Re-reading the spec, it looks like you're right --- I was thinking ASI would matter here, but I suppose not.

Good catch!

>> Source/JavaScriptCore/parser/Parser.cpp:4014
>> +        }
> 
> Which tests need this?

Ah --- previously, I had rewritten the arrow function parsing stuff to pay attention to the error classifier, which potentially saves it a bit of time. However at this point, this is dead code, I thought I had removed this already. The goal was to help reparse arrow functions less frequently, but at this point it's unnecessary

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

We do parse async arrow functions in `parseAssignmentExpression`.

I can probably move the AsyncFunctionExpression parsing to `parsePrimaryExpression`, though.

>> Source/JavaScriptCore/runtime/CommonIdentifiers.h:36
>> +    macro(AsyncFunction) \
> 
> Let's remove this.

Okay

-- 
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/76e77990/attachment-0001.html>