Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement Top level await #6457

Merged
merged 1 commit into from
Dec 18, 2020
Merged

Conversation

rhuanjl
Copy link
Collaborator

@rhuanjl rhuanjl commented Jun 5, 2020

This PR implements the stage 3 proposal Top Level Await: https://github.com/tc39/proposal-top-level-await.

I wrote this code in December but it was stalled due to waiting for the generator/async refactor to land.

This makes the following changes:

  1. In the parser if await is found in a module it is marked as async module.
  2. A new reportModuleCompletionCallback is added - if set this is used for reporting the completion of all root modules, if not set, old behaviour is followed for modules without await completion of modules with await will not be reported.
  3. Module evaluation for modules containing await now uses Async function logic for execution, a counter of executing modules (paused by await) is used to trigger parent execution at the correct time.
  4. Some drive by cleanup in a few places.
  5. Feature is guarded by the flag ESTopLevelAwait but this is set to enabled by default.

TODO: investigate the odd case of a module that is both a root module/main entry point AND is dynamically imported - not sure if error handling works correctly for this - also need a test for that case.

Fix: #6262

@@ -600,30 +600,6 @@ JsValueRef WScriptJsrt::LoadScriptHelper(JsValueRef callee, bool isConstructCall
return returnValue;
}

JsErrorCode WScriptJsrt::InitializeModuleInfo(JsModuleRecord moduleRecord)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drive by clean up in the host with past changes to module logic there was no reason for this to be a separate function any more - it's a one off initialisation step.

@@ -2039,6 +2005,17 @@ HRESULT WScriptJsrt::ModuleMessage::Call(LPCSTR fileName)
return errorCode;
}

JsErrorCode WScriptJsrt::ReportModuleCompletionCallback(JsModuleRecord module, JsValueRef exception)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Callback used when a module completes - returns the exception if the completion was not a normal completion.

This had to be added to support asynchronous module completion - note see below if the callback is not set engine will fallback to direct reporting for modules without Top Level Await to avoid any compatability break.

Comment on lines +181 to +184
if (reportModuleCompletionCallback == nullptr)
{
return false;
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the host has not set the callback just return false - this is to avoid backwards compatibility breaks.

moduleHostInfo != JsModuleHostInfo_FetchImportedModuleFromScriptCallback &&
moduleHostInfo != JsModuleHostInfo_NotifyModuleReadyCallback &&
moduleHostInfo != JsModuleHostInfo_InitializeImportMetaCallback)
if (moduleHostInfo == JsModuleHostInfo_Exception ||
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Invert the condition as the number of callback types is now greater than the number of non-callbacks

Comment on lines +8907 to +8890
if (IsTopLevelModuleFunc())
{
MakeModuleAsync();
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Convert module into an async function when await is found in it.

else
{
if (childModuleRecord->ConfirmChildrenParsed())
if (childrenModuleSet != nullptr)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drive by improvement - previously in the case of no child modules an empty array was allocated here.

{
JavascriptExceptionOperators::DoThrowCheckClone(exception, scriptContext);
Arguments outArgs(CallInfo(CallFlags_Value, 0), nullptr);
this->generator = VarTo<JavascriptGenerator>(rootFunction->CallRootFunction(outArgs, scriptContext, true));
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This call executes the "header" of the module which hoists any function exports. It yields before the body of the module.

Comment on lines 1147 to 1179
else
{
SetWasEvaluated();
SetEvaluating(false);
gen->CallGenerator(scriptContext->GetLibrary()->GetUndefined(), ResumeYieldKind::Normal);
if (this->parentModuleList != nullptr && !shouldIncrementAwait)
{
parentModuleList->Map([=](uint i, SourceTextModuleRecord* parentModule)
{
if (parentModule->DecrementAwaited())
{
parentModule->FinishModuleEvaluation(false);
}
});
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Module without top level await - execute the whole thing.

Comment on lines +1132 to +1160
if (gen->IsAsyncModule())
{
JavascriptPromise* prom = JavascriptAsyncFunction::BeginAsyncFunctionExecution(this->generator);
if (this->parentModuleList != nullptr && shouldIncrementAwait)
{
parentModuleList->Map([=](uint i, SourceTextModuleRecord* parentModule)
{
parentModule->IncrementAwaited();
});
}
auto* fulfilled = scriptContext->GetLibrary()->CreateAsyncModuleCallbackFunction(EntryAsyncModuleFulfilled, this);
auto* rejected = scriptContext->GetLibrary()->CreateAsyncModuleCallbackFunction(EntryAsyncModuleRejected, this);
auto* unused = JavascriptPromise::UnusedPromiseCapability(scriptContext);
JavascriptPromise::PerformPromiseThen(prom, unused, fulfilled, rejected, scriptContext);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Module with top level await execute until the first await then PerformPromiseThen to queue the continuation.

Comment on lines +52 to +57
return BeginAsyncFunctionExecution(generator);
}

JavascriptPromise* JavascriptAsyncFunction::BeginAsyncFunctionExecution(JavascriptGenerator* generator)
{
auto* library = generator->GetLibrary();
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Async function implementation split in two - as second half which actually executes the function can be reused for modules with Top level Await, whilst first half is not needed for modules.

Comment on lines -1 to -5
NotifyModuleReadyCallback(exception) bug_OS12095746_mod0.js
NotifyModuleReadyCallback(exception) bug_OS12095746_mod1.js
mod0 catch:Syntax error
mod1 catch:Unexpected identifier after numeric literal
NotifyModuleReadyCallback(exception) bug_OS12095746_mod2.js
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This callback no longer fires for Dynamic module exceptions - there was no useful purpose to it firing before as Host ought to disregard it and leaving handling to within script - as promise is rejected.

@rhuanjl
Copy link
Collaborator Author

rhuanjl commented Jun 5, 2020

@ppenzin I'm aware that this isn't simple but would be great if you could take a look

@chicoxyzzy would be good if you could comment on the tests I've added for this - this wasn't the easiest spec to implement in light of both its complexity and the existing ChakraCore logic in these areas - so want to be sure the tests are hitting the key paths.

@rhuanjl
Copy link
Collaborator Author

rhuanjl commented Dec 5, 2020

I've gone over this again and rebased it - I think it's solid; aware it's not trivial but would be great if someone could take a look, @Fly-Style @ppenzin

@ppenzin
Copy link
Member

ppenzin commented Dec 8, 2020

I've started looking at this a couple of times, but got sidetracked by easier things. Will take another look.

@ppenzin ppenzin merged commit 9e2f198 into chakra-core:master Dec 18, 2020
@rhuanjl rhuanjl deleted the TopLevelAwait branch December 18, 2020 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement Top-Level Await (Stage 3 proposal)
2 participants