Dynamic Module Import #2913

suwc · 2017-05-04T17:04:17Z

This PR adds support for import() dynamic module import sematic.

Per https://github.com/tc39/proposal-dynamic-import:

"A call to import(specifier) returns a promise for the module namespace object of the requested module, which is created after fetching, instantiating, and evaluating all of the module's dependencies, as well as the module itself.

"Here specifier will be interpreted the same way as in an import declaration (i.e., the same strings will work in both places). However, while specifier is a string it is not necessarily a string literal; thus code like import(./language-packs/${navigator.language}.js) will work—something impossible to accomplish with the usual import declarations.

"import() is proposed to work in both scripts and modules. This gives script code an easy asynchronous entry point into the module world, allowing it to start running module code."

This PR includes following changes:
- Update parser and bytecode generator to support import() sematic in module and in script
- Add new bytecode ImportCall
- Add runtime function for import() that:
○ Uses caller from stack to look up module record or source context that are associated with the module or script from which import() is called
○ Requests host to load target module source file (gets module record in return)
○ Creates promise unless the module record has one
○ Resolves/rejects promise if appropriates
○ Returns promise
- Add new host callback (FetchImportedModuleFromScript) for fetching imported module from script (accepts source context)
- Add promiseCapability field to module record class
- Update SourceTextModuleRecord's methods to accept callback from host and to handle dynamically imported module and its promise capability
- Update exception checks and assertions to cover new usage scenario of importing and evaluating module code with active script
Add unit tests for dynamic import functionality

suwc · 2017-05-04T17:08:21Z

@Microsoft/chakra-es @Microsoft/chakra-developers, please help code review. Thanks!

agarwal-sandeep · 2017-05-04T17:12:55Z

lib/Jsrt/ChakraCore.h

@@ -70,6 +71,21 @@ typedef JsErrorCode(CHAKRA_CALLBACK * FetchImportedModuleCallBack)(_In_ JsModule
 /// <returns>
 ///     true if the operation succeeded, false otherwise.
 /// </returns>
+typedef JsErrorCode(CHAKRA_CALLBACK * FetchImportedModuleFromScriptCallBack)(_In_ DWORD_PTR dwReferencingSourceContext, _In_ JsValueRef specifier, _Outptr_result_maybenull_ JsModuleRecord* dependentModuleRecord);


DWORD_PTR [](start = 82, length = 9)

DWORD_PTR [](start = 82, length = 9)

Use JsSourceContext instead of DWORD_PTR #Resolved

Updated in 2nd iteration.

In reply to: 114836637 [](ancestors = 114836637)

agarwal-sandeep · 2017-05-04T17:28:24Z

test/es6/dynamic-module-functionality.js

+                `testDynamicImport(
+                    ()=>{
+                        var getName = ()=>{ return 'ModuleSimpleExport'; };
+                        return import( getName() + '.js');


import [](start = 31, length = 6)

What if a non-string is passed to import? like import() or import({}) etc. Can we add test cases covering error scenarios? #Resolved

Added new test dynamic-module-import-specifier.js.
More tests to come.

In reply to: 114840008 [](ancestors = 114840008)

agarwal-sandeep · 2017-05-04T17:49:39Z

lib/Runtime/Language/JavascriptOperators.cpp

+            {
+                Js::JavascriptError * error = scriptContext->GetLibrary()->CreateURIError();
+                Var value = JavascriptString::NewCopySz(moduleName, scriptContext);
+                JavascriptOperators::OP_SetProperty(error, PropertyIds::message, value, scriptContext);


OP_SetProperty [](start = 37, length = 14)

Should you use JavascriptError::SetErrorMessageProperties instead? #Resolved

Or JavascriptError::SetErrorMessage #Resolved

Updated in 2nd iteration.

In reply to: 114844985 [](ancestors = 114844985)

agarwal-sandeep · 2017-05-04T17:51:21Z

lib/Runtime/Language/JavascriptOperators.cpp

+        }
+        else
+        {
+            Assert(false);


Assert [](start = 12, length = 6)

nit: Using AssertMsg with a message would be more useful #WontFix

Good point. This line is removed along with stack walker call though.

In reply to: 114845379 [](ancestors = 114845379)

tcare · 2017-05-04T23:59:08Z

lib/Runtime/Language/i386/StackFrame.cpp

-    this->codeAddr = this->GetReturnAddress();
-    this->frame = (void **)this->frame[0];
+    if (frame != nullptr)
+    {


Which situation needs this and why only x86? #Resolved

Only with nested enter_script, which I have removed.

In reply to: 114913205 [](ancestors = 114913205)

boingoing

Looks generally good. Can you look into storing the promise object itself in the module record instead of the promise capability? I think this makes sense to do and is simpler and more efficient.

boingoing · 2017-05-04T22:20:28Z

lib/Common/Exceptions/ExceptionCheck.cpp

@@ -58,7 +58,8 @@ void ExceptionCheck::SetHandledExceptionType(ExceptionType e)
 #if DBG
    if(!(e == ExceptionType_None ||
         e == ExceptionType_DisableCheck ||
-         !JsUtil::ExternalApi::IsScriptActiveOnCurrentThreadContext() ||
+         (e & ExceptionType_OutOfMemory) == ExceptionType_OutOfMemory ||  // InitializeModuleRecord handles OOM during dynamic import
+        !JsUtil::ExternalApi::IsScriptActiveOnCurrentThreadContext() ||


Nitpick: one space missing here. #Resolved

Fixed in 2nd iteration

In reply to: 114900876 [](ancestors = 114900876)

boingoing · 2017-05-04T22:27:00Z

lib/Parser/Parse.cpp

 {
    Assert(m_scriptContext->GetConfig()->IsES6ModuleEnabled());
    Assert(m_token.tk == tkIMPORT);

+    m_pscan->Scan();


Shouldn't we peek at the next token instead of scanning here? We don't know if this is a dynamic import or a static import at this point. If we scan and find that this is a static import at an invalid place (inside a function or outside of module code etc), won't we error on the wrong token? #Resolved

Yes, updated in 2nd iteration.

In reply to: 114901862 [](ancestors = 114901862)

boingoing · 2017-05-04T22:48:42Z

lib/Runtime/Language/JavascriptOperators.cpp

+            {
+                Js::JavascriptError * error = scriptContext->GetLibrary()->CreateURIError();
+                Var value = JavascriptString::NewCopySz(moduleName, scriptContext);
+                JavascriptOperators::OP_SetProperty(error, PropertyIds::message, value, scriptContext);


Or JavascriptError::SetErrorMessage #Resolved

boingoing · 2017-05-04T22:55:03Z

lib/Runtime/Language/SourceTextModuleRecord.cpp

@@ -61,7 +61,7 @@ namespace Js
        // in practice the modulerecord lifetime should be the same as the scriptcontext so it could be retrieved for the same
        // site. Host might hold a reference to the module as well after initializing the module.
        // In our implementation, we'll use the moduleId in bytecode to identify the module.
-        childModuleRecord->moduleId = scriptContext->GetLibrary()->EnsureModuleRecordList()->Add(childModuleRecord);
+        childModuleRecord->moduleId = scriptContext->GetLibrary()->EnsureModuleRecordList()->Add(childModuleRecord) + 1; // 0 == kmodGlobal


Can you move the comment about kmodGlobal into the comment above the call? Maybe expand it a little to explain that module record moduleId should be > 0. #Resolved

Found a better way to check module code so this shift is no longer needed.

In reply to: 114905856 [](ancestors = 114905856)

boingoing · 2017-05-04T22:57:42Z

lib/Runtime/Language/SourceTextModuleRecord.cpp

@@ -230,15 +259,15 @@ namespace Js
            OUTPUT_TRACE_DEBUGONLY(Js::ModulePhase, _u("\t>NotifyParentsAsNeeded\n"));
            NotifyParentsAsNeeded();

-            if (!WasDeclarationInitialized() && isRootModule)
+            if (this->promiseCapability != nullptr || (!WasDeclarationInitialized() && isRootModule))
            {
                // TODO: move this as a promise call? if parser is called from a different thread


Does this PR address the TODO? #Resolved

This PR doesn't enable the case where parser is called from a different thread. Therefore I leave this TODO here for future enhancement.

In reply to: 114906209 [](ancestors = 114906209)

boingoing · 2017-05-04T23:02:11Z

lib/Runtime/Language/SourceTextModuleRecord.cpp

+            promiseCapability = JavascriptPromise::NewPromiseCapability(scriptContext->GetLibrary()->GetPromiseConstructor(), scriptContext);
+        }
+
+        Var resolveOrRejectVar = toResolve ? promiseCapability->GetResolve() : promiseCapability->GetReject();


So toResolve indicates the promise should be resolved instead of rejected? Nitpicky, but I think I would prefer this to be named isResolve, isReject, success, or something along those lines. #Resolved

+1, I also found this hard to read.

In reply to: 114906734 [](ancestors = 114906734)

Changed to 'isResolve'

In reply to: 114906734 [](ancestors = 114906734)

boingoing · 2017-05-04T23:52:51Z

lib/Runtime/Language/JavascriptOperators.cpp

+                if (promiseCapability == nullptr)
+                {
+                    BEGIN_TRANSLATE_TO_HRESULT(static_cast<ExceptionType>(ExceptionType_OutOfMemory | ExceptionType_StackOverflow));
+                    promiseCapability = JavascriptPromise::NewPromiseCapability(scriptContext->GetLibrary()->GetPromiseConstructor(), scriptContext);


Does the spec call-out that we should be storing the PromiseCapabilityRecord here or is this more of an implementation detail? I would suspect it should be simpler to store a JavascriptPromise* here and call JavascriptPromise::Resolve() or JavascriptPromise::Reject() on it when it's time to resolve/reject it.

Looks like we're storing promiseCapability here to avoid casting the promise into a JavascriptPromise* because JavascriptPromiseCapability::promise is a Var? I think that cast should be safe to do since we pass %Promise% to JavascriptPromise::NewPromiseCapability which is guaranteed to return something that's a JavascriptPromise* because the return object is constructed via JavascriptLibrary::CreatePromise and we aren't doing a super call. Actually might be even simpler than that since we don't even need to call an executor function for this promise, right? So we could just call library->CreatePromise() and pass the result to JavascriptPromise::InitializePromise.

Might make sense to add a helper to JavascriptPromise itself

JavascriptPromise* JavascriptPromise::CreateEnginePromise(ScriptContext* scriptContext) { // Unused JavascriptPromiseResolveOrRejectFunction* resolve; JavascriptPromiseResolveOrRejectFunction* reject; JavascriptPromise* promise = library->CreatePromise(); InitializePromise(promise, &resolve, &reject, scriptContext); return promise; }

I think it would be simpler to resolve the promise at least if we stored the promise itself instead of the capability record. We would also avoid constructing a bunch of objects and making a bunch of calls by constructing the promise as the above helper. #Resolved

Good idea. Updated in 2nd iteration.

In reply to: 114912538 [](ancestors = 114912538)

tcare · 2017-05-05T00:16:09Z

lib/Parser/Parse.cpp

+    if (m_token.tk == tkLParen)
+    {
+        ParseNodePtr specifier = ParseTerm<buildAST>();
+        return CreateCallNode(knopCall, CreateNodeWithScanner<knopImport>(), specifier);


CreateCallNode [](start = 15, length = 14)

Since we parse this like a call, I would like to make sure we have sufficient coverage for parsing cases where import() could be mistaken for a function call, e.g. with multiple parameters, spread, etc. #Resolved

This is a good point #Resolved

Added dynamic-module-import-specifier.js
More to come.

In reply to: 114915006 [](ancestors = 114915006)

tcare · 2017-05-05T00:19:40Z

lib/Runtime/ByteCode/ByteCodeEmitter.cpp

+        else if (pnode->sxCall.pnodeTarget->nop == knopImport)
+        {
+            ParseNodePtr args = pnode->sxCall.pnodeArgs;
+            Emit(args, byteCodeGenerator, funcInfo, false);


args [](start = 17, length = 4)

Since we're piggybacking off a call node, perhaps we want to assert some restrictions e.g. single parameter #Resolved

Added assertion in 2nd iteration.

In reply to: 114915361 [](ancestors = 114915361)

curtisman · 2017-05-05T04:04:32Z

lib/Common/Exceptions/ExceptionCheck.cpp

@@ -58,7 +58,8 @@ void ExceptionCheck::SetHandledExceptionType(ExceptionType e)
 #if DBG
    if(!(e == ExceptionType_None ||
         e == ExceptionType_DisableCheck ||
-         !JsUtil::ExternalApi::IsScriptActiveOnCurrentThreadContext() ||
+         (e & ExceptionType_OutOfMemory) == ExceptionType_OutOfMemory ||  // InitializeModuleRecord handles OOM during dynamic import


InitializeModuleRecord [](start = 77, length = 22)

Why add this? The point here is that if you are in script, you should be throwing the JS exception object instead of OutOfMemory. #Resolved

Removed along with nested EnterScript.

In reply to: 114929334 [](ancestors = 114929334)

curtisman · 2017-05-05T04:20:43Z

lib/Runtime/Language/SourceTextModuleRecord.cpp

+        if (nullptr == this->childrenModuleSet)
+        {
+            ArenaAllocator* allocator = scriptContext->GeneralAllocator();
+            this->childrenModuleSet = (ChildModuleRecordSet*)AllocatorNew(ArenaAllocator, allocator, ChildModuleRecordSet, allocator);


childrenModuleSet [](start = 18, length = 17)

Use Anew? #Resolved

Updated in 2nd iteration.

In reply to: 114930171 [](ancestors = 114930171)

curtisman · 2017-05-05T04:21:55Z

    childrenModuleSet = nullptr;

childrenModuleSet is leaked? #Resolved

Refers to: lib/Runtime/Language/SourceTextModuleRecord.cpp:77 in ea66233. [](commit_id = ea66233, deletion_comment = False)

curtisman · 2017-05-05T04:24:47Z

lib/Runtime/Language/JavascriptOperators.cpp

+        SourceTextModuleRecord *moduleRecord = nullptr;
+
+        JavascriptFunction* pfuncCaller;
+        if (JavascriptStackWalker::GetCaller(&pfuncCaller, scriptContext) && pfuncCaller && pfuncCaller->IsScriptFunction())


Why do we need to use the stack walker here? Didn't the interpreter just called this OP_ImportCall and know what the function is already? Can't the interpreter pass it in? #Resolved

Updated interpreter and lowerer code to pass in function and removed stack walker call.

In reply to: 114930380 [](ancestors = 114930380)

curtisman · 2017-05-05T04:35:41Z

            srcInfo->moduleID = moduleId;

moduleID in SRCINFO is unrelated to javascript modules. It is probably wrong to store this here? #Resolved

Refers to: lib/Runtime/Language/SourceTextModuleRecord.cpp:122 in ea66233. [](commit_id = ea66233, deletion_comment = False)

curtisman · 2017-05-05T04:53:00Z

lib/Runtime/Base/LeaveScriptObject.cpp

@@ -63,7 +63,7 @@ namespace Js
            // not run and we might be in an inconsistent state

            // Put any code that may raise an exception in OnScriptStart
-            scriptContext->GetThreadContext()->EnterScriptStart(entryExitRecord, doCleanup);
+            scriptContext->GetThreadContext()->EnterScriptStart(entryExitRecord, doCleanup, &this->isScriptActive);


are we trying to allow nested EnterScript without LeaveScript? Why? #Resolved

Removed nested EnterScript in 2nd iteration

In reply to: 114931889 [](ancestors = 114931889)

suwc · 2017-05-08T07:30:30Z

            srcInfo->moduleID = moduleId;

Switched to using fscrIsModuleCode to detect ES6 module in ParseableFuncitonInfo::IsES6ModuleCode().
Removed shifting of moduleId by 1 and going back to starting from 0.
Will follow up SRCINFO->moduleID usage separately in:

#2931

In reply to: 299373242 [](ancestors = 299373242)

Refers to: lib/Runtime/Language/SourceTextModuleRecord.cpp:122 in ea66233. [](commit_id = ea66233, deletion_comment = False)

suwc · 2017-05-08T07:35:44Z

    childrenModuleSet = nullptr;

Added Adelete in 2nd iteration.

In reply to: 299368705 [](ancestors = 299368705)

Refers to: lib/Runtime/Language/SourceTextModuleRecord.cpp:77 in ea66233. [](commit_id = ea66233, deletion_comment = False)

suwc · 2017-05-08T17:20:38Z

@dotnet-bot
test Windows arm_release
test Windows x64_release
test Windows x86_release

akroshg · 2017-05-08T18:53:17Z

lib/Parser/Parse.cpp

+    ParseNodePtr specifier = ParseExpr<buildAST>(koplCma, nullptr, /* fAllowIn */FALSE, /* fAllowEllipsis */FALSE);
+    if (m_token.tk != tkRParen)
+    {
+        Error(ERRsyntax);


nit: proper error would be useful. #Resolved

Changing to 'ERRnoRparen'.

In reply to: 115324907 [](ancestors = 115324907)

akroshg · 2017-05-08T19:03:32Z

bin/ch/WScriptJsrt.cpp

+// we are not doing any translation, just treat the specifier as fileName.
+// While this call will come back directly from runtime script or module code, the additional
+// task can be scheduled asynchronously that executed later.
+JsErrorCode WScriptJsrt::FetchImportedModuleFromScript(_In_ JsSourceContext dwReferencingSourceContext,


FetchImportedModuleFromScript [](start = 25, length = 29)

wondering if this function can be refactored with above function. It seems most of the code is shared. #Resolved

updated in latest iteration

In reply to: 115327127 [](ancestors = 115327127)

akroshg · 2017-05-08T19:07:54Z

lib/Jsrt/Core/JsrtContextCore.cpp

+
+    HRESULT hr = NOERROR;
+    Js::JavascriptString* specifierVar = nullptr;
+    specifierVar = Js::JavascriptString::NewCopySz(specifier, GetScriptContext());


nit: combine these two lines? #WontFix

NA in latest iteration (file refactored)

In reply to: 115328093 [](ancestors = 115328093)

akroshg · 2017-05-08T19:08:32Z

lib/Jsrt/Core/JsrtContextCore.cpp

+    Js::JavascriptString* specifierVar = nullptr;
+    specifierVar = Js::JavascriptString::NewCopySz(specifier, GetScriptContext());
+
+    if (FAILED(hr))


is this redundant check? where the hr is changed in above code? #Resolved

NA in latest iteration (function refactored).

In reply to: 115328224 [](ancestors = 115328224)

akroshg · 2017-05-08T19:10:24Z

lib/Jsrt/Core/JsrtContextCore.cpp

+        return hr;
+    }
+
+    JsModuleRecord dependentRecord = JS_INVALID_REFERENCE;


there is not much difference between this function and above function (FetchImportedModule). Did you consider creating a lambda and just stub relevant part in that? #Resolved

Refactored in latest iteration.

In reply to: 115328606 [](ancestors = 115328606)

akroshg · 2017-05-08T22:01:29Z

test/es6/dynamic-module-import-syntax.js

+        body: function () {
+                let functionBody =
+                    `
+                    assert.doesNotThrow(function () { eval("import(undefined)"); }, "undefined");


aren't these tests repeated from import-specifier.js file? #WontFix

They are not exactly the same. Import-specifier.js tests ToString operation applied on specifier. This one tests parser.

In reply to: 115364519 [](ancestors = 115364519)

I don't see this file is added to the rlexe.xml, am I missing something?

In reply to: 115372781 [](ancestors = 115372781,115364519)

akroshg · 2017-05-08T22:03:21Z

test/es6/dynamic-module-import-specifier.js

+                            return import(${specifier});
+                        },
+                        (v)=>{
+                            assert.isTrue(false, 'Expected: promise rejected; actual: promise resolved: ' + '${message}');


nit: change this to assert.fail() . #Resolved

Done.

In reply to: 115364860 [](ancestors = 115364860)

akroshg · 2017-05-08T22:17:13Z

test/es6/dynamic-module-functionality.js

+    },
+];
+
+testRunner.runTests(tests, { verbose: WScript.Arguments[0] != "summary" });


Is there a test case which covers the nested dynamic imports ? #Resolved

Nest import syntax is test in dynamic-module-import-specifier.js

In reply to: 115367185 [](ancestors = 115367185)

tcare · 2017-05-08T22:41:58Z

lib/Parser/Parse.cpp

 {
    Assert(m_scriptContext->GetConfig()->IsES6ModuleEnabled());
    Assert(m_token.tk == tkIMPORT);

+    RestorePoint parsedImport;
+    m_pscan->Capture(&parsedImport);
+    m_pscan->Scan();


Do we need to do a rewind? We are just going to scan again a few lines down anyway. #WontFix

This is just to make sure col# reported by error msg is not off due to the premature scan.

In reply to: 115371147 [](ancestors = 115371147)

suwc · 2017-05-08T23:26:43Z

    case JsModuleHostInfo_NotifyModuleReadyCallback:

Done.

In reply to: 299962864 [](ancestors = 299962864)

Refers to: lib/Jsrt/Core/JsrtCore.cpp:209 in db8d7fd. [](commit_id = db8d7fd, deletion_comment = False)

suwc · 2017-05-08T23:26:52Z

    if ((this->GetGrfscr() & fscrIsModuleCode) == fscrIsModuleCode || this->GetModuleID() == kmodGlobal)

Done.

In reply to: 299963481 [](ancestors = 299963481)

Refers to: lib/Runtime/Base/FunctionBody.cpp:3166 in db8d7fd. [](commit_id = db8d7fd, deletion_comment = False)

akroshg · 2017-05-08T23:40:41Z

lib/Jsrt/Core/JsrtContextCore.cpp

@@ -110,7 +129,7 @@ HRESULT ChakraCoreHostScriptContext::FetchImportedModule(Js::ModuleRecordBase* r
    JsModuleRecord dependentRecord = JS_INVALID_REFERENCE;
    {
        AUTO_NO_EXCEPTION_REGION;
-        JsErrorCode errorCode = fetchImportedModuleCallback(referencingModule, specifierVar, &dependentRecord);
+        JsErrorCode errorCode = fetch(specifierVar, &dependentRecord);//fetchImportedModuleCallback(referencingModule, specifierVar, &dependentRecord);


nitpick : comments can be removed #Resolved

akroshg · 2017-05-08T23:43:38Z

lib/Runtime/Language/JavascriptOperators.cpp

+                return SourceTextModuleRecord::ResolveOrRejectDynamicImportPromise(false, errorObject, scriptContext);
+            }
+
+            Throw::InternalError();


what happens if
import( { toString : function() {
throw 1;
}});
#Resolved

I think the reasonable thing to expect is to reject the promise with 1.

Here spec says:
IfAbruptRejectPromise(specifierString, promiseCapability).

which is questionable because specifierString will be nullptr.
I will seek clarification separately.

In reply to: 115379478 [](ancestors = 115379478)

akroshg · 2017-05-08T23:53:39Z

lib/Runtime/Language/JavascriptOperators.cpp

+
+        // Walk the call stack if caller function is neither module code nor having host source context
+        DWORD_PTR dwReferencingSourceContext = dwReferencingSourceContext = parentFuncBody->GetFunctionInfo()->GetFunctionProxy()->GetSourceContextInfo()->dwHostSourceContext;
+        if (!parentFuncBody->IsES6ModuleCode() && dwReferencingSourceContext == Js::Constants::NoHostSourceContext)


if dwReferencingSourceContext == Js::Constants::NoHostSourceContext
could you not use callerUtf8SourceInfo (defined in the utf8sourceinfo)? Confirm with @agarwal.sandeep@microsoft.com that this is populated in the non-debug scenario as well. #WontFix

callerUtf8SourceInfo is populated in non-debug scenario as well. You will still need to walk the chain to get to first non evaled parent. #ByDesign

Yes it works for a single eval case, but fails for nested eval.

In reply to: 115380766 [](ancestors = 115380766)

akroshg · 2017-05-08T23:56:06Z

lib/Runtime/Language/JavascriptOperators.cpp

+                    JavascriptError::SetErrorMessageProperties(error, hr, moduleName, scriptContext);
+                    return SourceTextModuleRecord::ResolveOrRejectDynamicImportPromise(false, error, scriptContext);
+                }
+            }


would be nice if this code moved to SoureTextModuleRecord. I don't think javascriptoperator.cpp needs to know any of this. #Resolved

Good point. Moved and renamed the function.

In reply to: 115381049 [](ancestors = 115381049)

akroshg · 2017-05-09T00:04:28Z

lib/Runtime/Language/SourceTextModuleRecord.cpp

+            if (this->promise != nullptr)
+            {
+                SourceTextModuleRecord::ResolveOrRejectDynamicImportPromise(false, this->GetErrorObject(), this->GetScriptContext(), this);
+                this->SetPromise(nullptr);


SetPromise [](start = 22, length = 10)

would it be wise to reset the promise to null first and do the ResolveOrReject.. ? #Resolved

In fact ResolveOrRejectDynamicImportPromise() takes care of this already. Removing this line and in similar cases

In reply to: 115382160 [](ancestors = 115382160)

akroshg · 2017-05-09T00:04:59Z

lib/Runtime/Language/SourceTextModuleRecord.cpp

@@ -175,6 +175,12 @@ namespace Js
            }

            OUTPUT_TRACE_DEBUGONLY(Js::ModulePhase, _u("\t>NotifyParentAsNeeded\n"));
+            if (this->promise != nullptr)
+            {
+                SourceTextModuleRecord::ResolveOrRejectDynamicImportPromise(false, this->GetErrorObject(), this->GetScriptContext(), this);


this->GetScriptContext() [](start = 107, length = 24)

nitpick : use the local scriptContext (also you can use the errorObject as well) #Resolved

akroshg · 2017-05-09T00:08:21Z

lib/Runtime/Language/SourceTextModuleRecord.cpp

+
+            if (this->errorObject != nullptr)
+            {
+                SourceTextModuleRecord::ResolveOrRejectDynamicImportPromise(false, this->GetErrorObject(), scriptContext, this);


scriptContext [](start = 107, length = 13)

nitpick : could you not use the this->scriptContext? I am not sure why we need to have local variable for a scriptcontext. #Resolved

akroshg · 2017-05-09T00:13:47Z

lib/Runtime/Language/SourceTextModuleRecord.cpp

+            AssertMsg(errorObject != nullptr && Js::JavascriptError::Is(errorObject), "ModuleEvaluation: Unhandled exception thrown by calling root function");
+            if (errorObject != nullptr && Js::JavascriptError::Is(errorObject))
+            {
+                ResolveOrRejectDynamicImportPromise(false, errorObject, scriptContext);


setPromise to null? #Resolved

Adding 'this' as last param and SetPromise(nullptr) will be taken care of.

In reply to: 115383242 [](ancestors = 115383242)

akroshg · 2017-05-09T00:18:41Z

lib/Runtime/Language/SourceTextModuleRecord.cpp

+            Js::JavascriptError * error = scriptContext->GetLibrary()->CreateError();
+            JavascriptError::SetErrorMessageProperties(error, E_OUTOFMEMORY, this->GetSpecifierSz(), scriptContext);
+            ResolveOrRejectDynamicImportPromise(false, error, scriptContext);
+            return scriptContext->GetLibrary()->GetUndefined();


we are returning undefined in the OOM case? why not re-thorw? #Resolved

Only in the dynamic import case, we reject the promise with error object and return undefined (not supposed to reject and throw)
In the static case, we should throw the error object. Making the change.

In reply to: 115383826 [](ancestors = 115383826)

akroshg · 2017-05-09T18:09:30Z

lib/Runtime/Language/JavascriptOperators.cpp

+                    if (walker.GetCaller(&caller) && caller != nullptr && caller->IsScriptFunction())
+                    {
+                        parentFuncBody = caller->GetFunctionBody();
+                        dwReferencingSourceContext = dwReferencingSourceContext = parentFuncBody->GetFunctionInfo()->GetFunctionProxy()->GetSourceContextInfo()->dwHostSourceContext;


nit: function body has function GetHostSourceContext(), which will give you the dwHostSOurceContext. #Resolved

suwc · 2017-05-09T18:10:12Z

@dotnet-bot
test Windows x64_release

akroshg · 2017-05-09T18:10:52Z

lib/Runtime/Language/JavascriptOperators.cpp

+
+                do
+                {
+                    if (walker.GetCaller(&caller) && caller != nullptr && caller->IsScriptFunction())


interesting to see what happens when the import is called in the setTimeout. Will you add a test case just to see that we don't get the fatal error. #WontFix

I couldn't find a scenario which involves the nested imports also dynamic->static->dynamic. sort of?

In reply to: 115561112 [](ancestors = 115561112)

I am still working on these test cases. They will come in later.

In reply to: 115577016 [](ancestors = 115577016,115561112)

akroshg · 2017-05-09T18:36:09Z

lib/Runtime/Language/SourceTextModuleRecord.cpp

+                }
+            }
+
+            throw(err);


throw outside of catch handler. (look at other example in the code base, where they use different function for re-throw). #Resolved

akroshg · 2017-05-09T18:37:30Z

lib/Runtime/Language/SourceTextModuleRecord.cpp

+            if (this->promise != nullptr)
+            {
+                Var errorObject = err.GetAndClear()->GetThrownObject(scriptContext);
+                AssertMsg(errorObject != nullptr, "ModuleEvaluation: null error object thrown from root function");


AssertMsg [](start = 16, length = 9)

nit : convert to AssertOrFailFast so that you don't have to have if/else blocks #Resolved

akroshg · 2017-05-09T18:38:34Z

lib/Runtime/Language/SourceTextModuleRecord.cpp

        }
+
+        Assert(this->errorObject == nullptr);
        Assert(!WasEvaluated());


nit: SetWasEvaluated has this assert covered. #Resolved

akroshg · 2017-05-09T19:11:58Z

Is import() configurable through config flags or it is enabled by default on? #Resolved

suwc · 2017-05-09T19:14:02Z

We are enabling it by default.

In reply to: 300271647 [](ancestors = 300271647)

This PR adds support for import() dynamic module import sematic. Per https://github.com/tc39/proposal-dynamic-import: "A call to import(specifier) returns a promise for the module namespace object of the requested module, which is created after fetching, instantiating, and evaluating all of the module's dependencies, as well as the module itself. "Here specifier will be interpreted the same way as in an import declaration (i.e., the same strings will work in both places). However, while specifier is a string it is not necessarily a string literal; thus code like import(`./language-packs/${navigator.language}.js`) will work—something impossible to accomplish with the usual import declarations. "import() is proposed to work in both scripts and modules. This gives script code an easy asynchronous entry point into the module world, allowing it to start running module code." This PR includes following changes: - Update parser and bytecode generator to support import() sematic in module and in script - Add new bytecode 'ImportCall' - Add runtime function for import() that: ○ Uses caller from stack to look up module record or source context that are associated with the module or script from which 'import()' is called ○ Requests host to load target module source file (gets module record in return) ○ Creates promise unless the module record has one ○ Resolves/rejects promise if appropriates ○ Returns promise - Add new host callback ('FetchImportedModuleFromScript') for fetching imported module from script (accepts source context) - Add 'promiseCapability' field to module record class - Update SourceTextModuleRecord's methods to accept callback from host and to handle dynamically imported module and its promise capability - Update exception checks and assertions to cover new usage scenario of importing and evaluating module code with active script Add unit tests for dynamic import functionality

Merge pull request #2913 from suwc:build/suwc/buddy This PR adds support for `import()` dynamic module import sematic. Per https://github.com/tc39/proposal-dynamic-import: "A call to `import(specifier)` returns a promise for the module namespace object of the requested module, which is created after fetching, instantiating, and evaluating all of the module's dependencies, as well as the module itself. "Here specifier will be interpreted the same way as in an import declaration (i.e., the same strings will work in both places). However, while specifier is a string it is not necessarily a string literal; thus code like import(`./language-packs/${navigator.language}.js`) will work—something impossible to accomplish with the usual import declarations. "`import()` is proposed to work in both scripts and modules. This gives script code an easy asynchronous entry point into the module world, allowing it to start running module code." This PR includes following changes: - Update parser and bytecode generator to support `import()` sematic in module and in script - Add new bytecode `ImportCall` - Add runtime function for `import()` that: ○ Uses caller from stack to look up module record or source context that are associated with the module or script from which `import()` is called ○ Requests host to load target module source file (gets module record in return) ○ Creates promise unless the module record has one ○ Resolves/rejects promise if appropriates ○ Returns promise - Add new host callback (`FetchImportedModuleFromScript`) for fetching imported module from script (accepts source context) - Add `promiseCapability` field to module record class - Update `SourceTextModuleRecord`'s methods to accept callback from host and to handle dynamically imported module and its promise capability - Update exception checks and assertions to cover new usage scenario of importing and evaluating module code with active script Add unit tests for dynamic import functionality

suwc · 2017-05-11T23:39:22Z

Thanks for the code review.

msftclas added the cla-not-required label May 4, 2017

suwc requested review from boingoing, tcare and pleath May 4, 2017 17:06

suwc self-assigned this May 4, 2017

agarwal-sandeep reviewed May 4, 2017

View reviewed changes

suwc force-pushed the build/suwc/buddy branch from 2d5807f to ea66233 Compare May 4, 2017 17:40

agarwal-sandeep reviewed May 4, 2017

View reviewed changes

tcare reviewed May 4, 2017

View reviewed changes

boingoing reviewed May 5, 2017

View reviewed changes

tcare reviewed May 5, 2017

View reviewed changes

curtisman reviewed May 5, 2017

View reviewed changes

suwc force-pushed the build/suwc/buddy branch from ea66233 to 34d935f Compare May 8, 2017 08:00

akroshg reviewed May 8, 2017

View reviewed changes

tcare reviewed May 8, 2017

View reviewed changes

akroshg reviewed May 8, 2017

View reviewed changes

akroshg reviewed May 9, 2017

View reviewed changes

suwc force-pushed the build/suwc/buddy branch from d78cc11 to bae3e30 Compare May 9, 2017 17:51

akroshg reviewed May 9, 2017

View reviewed changes

suwc force-pushed the build/suwc/buddy branch from bae3e30 to 2277186 Compare May 9, 2017 19:30

chakrabot merged commit 2277186 into chakra-core:master May 9, 2017

suwc deleted the build/suwc/buddy branch May 11, 2017 23:39

Dynamic Module Import #2913

Dynamic Module Import #2913

Conversation

suwc commented May 4, 2017 • edited Loading

suwc commented May 4, 2017

agarwal-sandeep May 4, 2017 • edited by suwc Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agarwal-sandeep May 4, 2017 • edited by suwc Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agarwal-sandeep May 4, 2017 • edited by suwc Loading

Choose a reason for hiding this comment

boingoing May 4, 2017 • edited by suwc Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agarwal-sandeep May 4, 2017 • edited by suwc Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcare May 4, 2017 • edited by suwc Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boingoing left a comment

Choose a reason for hiding this comment

boingoing May 4, 2017 • edited by suwc Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boingoing May 4, 2017 • edited by suwc Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boingoing May 4, 2017 • edited by suwc Loading

Choose a reason for hiding this comment

boingoing May 4, 2017 • edited by suwc Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boingoing May 4, 2017 • edited by suwc Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boingoing May 4, 2017 • edited by suwc Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boingoing May 4, 2017 • edited by suwc Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcare May 5, 2017 • edited by suwc Loading

Choose a reason for hiding this comment

boingoing May 5, 2017 • edited by suwc Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcare May 5, 2017 • edited by suwc Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

curtisman May 5, 2017 • edited by suwc Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

curtisman May 5, 2017 • edited by suwc Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

curtisman commented May 5, 2017 • edited by suwc Loading

curtisman May 5, 2017 • edited by suwc Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

curtisman commented May 5, 2017 • edited by suwc Loading

curtisman May 5, 2017 • edited by suwc Loading

Choose a reason for hiding this comment

suwc May 5, 2017 • edited Loading

Choose a reason for hiding this comment

suwc commented May 8, 2017 • edited Loading

suwc commented May 8, 2017

suwc commented May 8, 2017

akroshg May 8, 2017 • edited by suwc Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akroshg May 8, 2017 • edited by suwc Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akroshg May 8, 2017 • edited by suwc Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suwc commented May 4, 2017 •

edited

Loading

agarwal-sandeep May 4, 2017 •

edited by suwc

Loading

agarwal-sandeep May 4, 2017 •

edited by suwc

Loading

agarwal-sandeep May 4, 2017 •

edited by suwc

Loading

boingoing May 4, 2017 •

edited by suwc

Loading

agarwal-sandeep May 4, 2017 •

edited by suwc

Loading

tcare May 4, 2017 •

edited by suwc

Loading

boingoing May 4, 2017 •

edited by suwc

Loading

boingoing May 4, 2017 •

edited by suwc

Loading

boingoing May 4, 2017 •

edited by suwc

Loading

boingoing May 4, 2017 •

edited by suwc

Loading

boingoing May 4, 2017 •

edited by suwc

Loading

boingoing May 4, 2017 •

edited by suwc

Loading

boingoing May 4, 2017 •

edited by suwc

Loading

tcare May 5, 2017 •

edited by suwc

Loading

boingoing May 5, 2017 •

edited by suwc

Loading

tcare May 5, 2017 •

edited by suwc

Loading

curtisman May 5, 2017 •

edited by suwc

Loading

curtisman May 5, 2017 •

edited by suwc

Loading

curtisman commented May 5, 2017 •

edited by suwc

Loading

curtisman May 5, 2017 •

edited by suwc

Loading

curtisman commented May 5, 2017 •

edited by suwc

Loading

curtisman May 5, 2017 •

edited by suwc

Loading

suwc May 5, 2017 •

edited

Loading

suwc commented May 8, 2017 •

edited

Loading

akroshg May 8, 2017 •

edited by suwc

Loading

akroshg May 8, 2017 •

edited by suwc

Loading

akroshg May 8, 2017 •

edited by suwc

Loading

akroshg May 8, 2017 •

edited by suwc

Loading

akroshg May 8, 2017 •

edited by suwc

Loading

akroshg May 8, 2017 •

edited by suwc

Loading

akroshg May 8, 2017 •

edited by suwc

Loading

akroshg May 8, 2017 •

edited by suwc

Loading

tcare May 8, 2017 •

edited by suwc

Loading

akroshg May 8, 2017 •

edited by suwc

Loading

akroshg May 8, 2017 •

edited by suwc

Loading

suwc May 9, 2017 •

edited

Loading

akroshg May 8, 2017 •

edited by suwc

Loading

agarwal-sandeep May 9, 2017 •

edited by suwc

Loading

akroshg May 8, 2017 •

edited by suwc

Loading

akroshg May 9, 2017 •

edited by suwc

Loading