[MERGE #5803 @rhuanjl] Various module import fixes

Merge pull request #5803 from rhuanjl:moduleFixes

This PR fixes a collection of module related issues - some of these are very small so I thought better to combine them into one PR. I hope this isn't too much at once.

1. Restore the use of relative paths for imports within modules in ch - this was implemented a long time ago and then broken - the functionality was mostly still there just broken in a few places - originally this was issue 3257, it also has a newer issue I opened 5237. As well as restoring the functionality I restored the test case from 3257 which was no longer testing what it had been designed for after a previous edit.

2. Don't print messages about failing to load modules or syntax errors in modules when processing dynamic imports - this was purely a ch issue but was problematic for several test262 cases per issue 5796.

3. Reject the promise from import() if a child module of the dynamically imported module throws a runtime error - there was a bug where this promise was never being resolved - this is also part of 5796.

4. `new import(anything)` should be a syntax error - 5797

5. import() should return a different promise each time even when using the same specifier - fix this by using JavascriptPromise::CreatePassThroughPromise 5795

cc @rwaldron @boingoing

fix #5796
fix #5795
fix #5797
fix #5237
re-fix #3257

Author: boingoing

bin/ch/Helpers.cpp

@@ -155,7 +155,7 @@ uint ConcatPath(LPCSTR filenameLeft, uint posPathSep, LPCSTR filenameRight, char
     return totalLength;
 }
 
-HRESULT Helpers::LoadScriptFromFile(LPCSTR filenameToLoad, LPCSTR& contents, UINT* lengthBytesOut /*= nullptr*/)
+HRESULT Helpers::LoadScriptFromFile(LPCSTR filenameToLoad, LPCSTR& contents, UINT* lengthBytesOut /*= nullptr*/, std::string* fullPath /*= nullptr*/, bool shouldMute /*=false */)
 {
     static char sHostApplicationPathBuffer[MAX_URI_LENGTH];
     static uint sHostApplicationPathBufferLength = (uint) -1;
@@ -169,7 +169,7 @@ HRESULT Helpers::LoadScriptFromFile(LPCSTR filenameToLoad, LPCSTR& contents, UIN
     FILE * file = NULL;
     size_t bufferLength = 0;
 
-    LPCSTR filename = filenameToLoad;
+    LPCSTR filename = fullPath == nullptr ? filenameToLoad : LPCSTR(fullPath->c_str());
     if (sHostApplicationPathBufferLength == (uint)-1)
     {
         // consider incoming filename as the host app and base its' path for others
@@ -188,7 +188,7 @@ HRESULT Helpers::LoadScriptFromFile(LPCSTR filenameToLoad, LPCSTR& contents, UIN
         }
         sHostApplicationPathBuffer[sHostApplicationPathBufferLength] = char(0);
     }
-    else if (filename[0] != '/' && filename[0] != '\\') // make sure it's not a full path
+    else if (filename[0] != '/' && filename[0] != '\\' && fullPath == nullptr) // make sure it's not a full path
     {
         // concat host path and filename
         uint len = ConcatPath(sHostApplicationPathBuffer, sHostApplicationPathBufferLength,
@@ -216,7 +216,7 @@ HRESULT Helpers::LoadScriptFromFile(LPCSTR filenameToLoad, LPCSTR& contents, UIN
         // etc.
         if (fopen_s(&file, filename, "rb") != 0)
         {
-            if (!HostConfigFlags::flags.MuteHostErrorMsgIsEnabled)
+            if (!HostConfigFlags::flags.MuteHostErrorMsgIsEnabled && !shouldMute)
             {
 #ifdef _WIN32
                 DWORD lastError = GetLastError();

bin/ch/Helpers.h

@@ -7,7 +7,7 @@
 class Helpers
 {
 public :
-    static HRESULT LoadScriptFromFile(LPCSTR filename, LPCSTR& contents, UINT* lengthBytesOut = nullptr);
+    static HRESULT LoadScriptFromFile(LPCSTR filename, LPCSTR& contents, UINT* lengthBytesOut = nullptr, std::string* fullPath = nullptr, bool shouldMute = false);
     static LPCWSTR JsErrorCodeToString(JsErrorCode jsErrorCode);
     static void LogError(__in __nullterminated const char16 *msg, ...);
     static HRESULT LoadBinaryFile(LPCSTR filename, LPCSTR& contents, UINT& lengthBytes, bool printFileOpenError = true);

bin/ch/WScriptJsrt.cpp

@@ -47,6 +47,7 @@
 MessageQueue* WScriptJsrt::messageQueue = nullptr;
 std::map<std::string, JsModuleRecord>  WScriptJsrt::moduleRecordMap;
 std::map<JsModuleRecord, std::string>  WScriptJsrt::moduleDirMap;
+std::map<JsModuleRecord, ModuleState>  WScriptJsrt::moduleErrMap;
 std::map<DWORD_PTR, std::string> WScriptJsrt::scriptDirMap;
 DWORD_PTR WScriptJsrt::sourceContext = 0;
 
@@ -223,7 +224,6 @@ JsValueRef WScriptJsrt::LoadScriptFileHelper(JsValueRef callee, JsValueRef *argu
             hr = Helpers::LoadScriptFromFile(*fileName, fileContent);
             if (FAILED(hr))
             {
-                // check if have it registered
                 fprintf(stderr, "Couldn't load file '%s'\n", fileName.GetString());
                 IfJsrtErrorSetGo(ChakraRTInterface::JsGetUndefinedValue(&returnValue));
                 return returnValue;
@@ -482,6 +482,7 @@ JsErrorCode WScriptJsrt::LoadModuleFromString(LPCSTR fileName, LPCSTR fileConten
             }
 
             moduleRecordMap[std::string(moduleRecordKey)] = requestModule;
+            moduleErrMap[requestModule] = RootModule;
         }
     }
     else
@@ -504,9 +505,10 @@ JsErrorCode WScriptJsrt::LoadModuleFromString(LPCSTR fileName, LPCSTR fileConten
 
     errorCode = ChakraRTInterface::JsParseModuleSource(requestModule, dwSourceCookie, (LPBYTE)fileContent,
         fileContentLength, JsParseModuleSourceFlags_DataIsUTF8, &errorObject);
-    if ((errorCode != JsNoError) && errorObject != JS_INVALID_REFERENCE && fileContent != nullptr && !HostConfigFlags::flags.IgnoreScriptErrorCode)
+    if ((errorCode != JsNoError) && errorObject != JS_INVALID_REFERENCE && fileContent != nullptr && !HostConfigFlags::flags.IgnoreScriptErrorCode && moduleErrMap[requestModule] == RootModule)
     {
         ChakraRTInterface::JsSetException(errorObject);
+        moduleErrMap[requestModule] = ErroredModule;
         return errorCode;
     }
     return JsNoError;
@@ -1145,6 +1147,7 @@ bool WScriptJsrt::Uninitialize()
     // to avoid worrying about global destructor order.
     moduleRecordMap.clear();
     moduleDirMap.clear();
+    moduleErrMap.clear();
     scriptDirMap.clear();
 
     auto& threadData = GetRuntimeThreadLocalData().threadData;
@@ -1232,7 +1235,6 @@ JsValueRef __stdcall WScriptJsrt::LoadTextFileCallback(JsValueRef callee, bool i
 
             if (FAILED(hr))
             {
-                // check if have it registered
                 fprintf(stderr, "Couldn't load file '%s'\n", fileName.GetString());
                 IfJsrtErrorSetGo(ChakraRTInterface::JsGetUndefinedValue(&returnValue));
                 return returnValue;
@@ -1396,7 +1398,6 @@ JsValueRef __stdcall WScriptJsrt::LoadBinaryFileCallback(JsValueRef callee,
 
             if (FAILED(hr))
             {
-                // check if have it registered
                 fprintf(stderr, "Couldn't load file '%s'\n", fileName.GetString());
                 IfJsrtErrorSetGoLabel(ChakraRTInterface::JsGetUndefinedValue(&returnValue), Error);
                 return returnValue;
@@ -1845,12 +1846,14 @@ HRESULT WScriptJsrt::CallbackMessage::CallFunction(LPCSTR fileName)
     return hr;
 }
 
-WScriptJsrt::ModuleMessage::ModuleMessage(JsModuleRecord module, JsValueRef specifier)
+WScriptJsrt::ModuleMessage::ModuleMessage(JsModuleRecord module, JsValueRef specifier, std::string* fullPathPtr)
     : MessageBase(0), moduleRecord(module), specifier(specifier)
 {
+    fullPath = nullptr;
     ChakraRTInterface::JsAddRef(module, nullptr);
     if (specifier != nullptr)
     {
+        fullPath = new std::string (*fullPathPtr);
         // nullptr specifier means a Promise to execute; non-nullptr means a "fetch" operation.
         ChakraRTInterface::JsAddRef(specifier, nullptr);
     }
@@ -1861,21 +1864,39 @@ WScriptJsrt::ModuleMessage::~ModuleMessage()
     ChakraRTInterface::JsRelease(moduleRecord, nullptr);
     if (specifier != nullptr)
     {
+        delete fullPath;
         ChakraRTInterface::JsRelease(specifier, nullptr);
     }
 }
 
 HRESULT WScriptJsrt::ModuleMessage::Call(LPCSTR fileName)
 {
-    JsErrorCode errorCode;
+    JsErrorCode errorCode = JsNoError;
     JsValueRef result = JS_INVALID_REFERENCE;
     HRESULT hr;
     if (specifier == nullptr)
     {
-        errorCode = ChakraRTInterface::JsModuleEvaluation(moduleRecord, &result);
-        if (errorCode != JsNoError)
+        if (moduleErrMap[moduleRecord] != ErroredModule)
         {
-            PrintException(fileName, errorCode);
+            errorCode = ChakraRTInterface::JsModuleEvaluation(moduleRecord, &result);
+            if (errorCode != JsNoError)
+            {
+                if (moduleErrMap[moduleRecord] == RootModule)
+                {
+                    PrintException(fileName, errorCode);
+                }
+                else
+                {
+                    bool hasException = false;
+                    ChakraRTInterface::JsHasException(&hasException);
+                    if (hasException)
+                    {
+                        JsValueRef exception;
+                        ChakraRTInterface::JsGetAndClearException(&exception);
+                        exception; //unusued
+                    }
+                }
+            }
         }
     }
     else
@@ -1885,19 +1906,22 @@ HRESULT WScriptJsrt::ModuleMessage::Call(LPCSTR fileName)
         errorCode = specifierStr.GetError();
         if (errorCode == JsNoError)
         {
-            hr = Helpers::LoadScriptFromFile(*specifierStr, fileContent);
+            hr = Helpers::LoadScriptFromFile(*specifierStr, fileContent, nullptr, fullPath, true);
 
             if (FAILED(hr))
             {
-                // check if have it registered
                 if (!HostConfigFlags::flags.MuteHostErrorMsgIsEnabled)
                 {
-                    fprintf(stderr, "Couldn't load file '%s'\n", specifierStr.GetString());
+                    auto actualModuleRecord = moduleRecordMap.find(*fullPath);
+                    if (actualModuleRecord == moduleRecordMap.end() || moduleErrMap[actualModuleRecord->second] == RootModule)
+                    {
+                        fprintf(stderr, "Couldn't load file '%s'\n", specifierStr.GetString());
+                    }
                 }
-                LoadScript(nullptr, *specifierStr, nullptr, "module", true, WScriptJsrt::FinalizeFree, false);
+                LoadScript(nullptr, fullPath == nullptr ? *specifierStr : fullPath->c_str(), nullptr, "module", true, WScriptJsrt::FinalizeFree, false);
                 goto Error;
             }
-            LoadScript(nullptr, *specifierStr, fileContent, "module", true, WScriptJsrt::FinalizeFree, true);
+            LoadScript(nullptr, fullPath == nullptr ? *specifierStr : fullPath->c_str(), fileContent, "module", true, WScriptJsrt::FinalizeFree, true);
         }
     }
 Error:
@@ -1936,9 +1960,10 @@ JsErrorCode WScriptJsrt::FetchImportedModuleHelper(JsModuleRecord referencingMod
     {
         GetDir(fullPath, &moduleDirMap[moduleRecord]);
         InitializeModuleInfo(specifier, moduleRecord);
-        moduleRecordMap[std::string(fullPath)] = moduleRecord;
-        ModuleMessage* moduleMessage =
-            WScriptJsrt::ModuleMessage::Create(referencingModule, specifier);
+        std::string pathKey = std::string(fullPath);
+        moduleRecordMap[pathKey] = moduleRecord;
+        moduleErrMap[moduleRecord] = ImportedModule;
+        ModuleMessage* moduleMessage = WScriptJsrt::ModuleMessage::Create(referencingModule, specifier, &pathKey);
         if (moduleMessage == nullptr)
         {
             return JsErrorOutOfMemory;
@@ -1973,14 +1998,6 @@ JsErrorCode WScriptJsrt::FetchImportedModule(_In_ JsModuleRecord referencingModu
 JsErrorCode WScriptJsrt::FetchImportedModuleFromScript(_In_ JsSourceContext dwReferencingSourceContext,
     _In_ JsValueRef specifier, _Outptr_result_maybenull_ JsModuleRecord* dependentModuleRecord)
 {
-    // ch.exe assumes all imported source files are located at .
-    auto scriptDirEntry = scriptDirMap.find(dwReferencingSourceContext);
-    if (scriptDirEntry != scriptDirMap.end())
-    {
-        std::string dir = scriptDirEntry->second;
-        return FetchImportedModuleHelper(nullptr, specifier, dependentModuleRecord, dir.c_str());
-    }
-
     return FetchImportedModuleHelper(nullptr, specifier, dependentModuleRecord);
 }
 
@@ -2008,7 +2025,8 @@ JsErrorCode WScriptJsrt::NotifyModuleReadyCallback(_In_opt_ JsModuleRecord refer
         ChakraRTInterface::JsGetAndClearException(&exception);
         exception; // unused
     }
-    else
+    
+    if (exceptionVar != nullptr || moduleErrMap[referencingModule] != ErroredModule)
     {
         WScriptJsrt::ModuleMessage* moduleMessage =
             WScriptJsrt::ModuleMessage::Create(referencingModule, nullptr);

bin/ch/WScriptJsrt.h

@@ -5,6 +5,13 @@
 #pragma once
 #include <list>
 
+enum ModuleState
+{
+    RootModule,
+    ImportedModule,
+    ErroredModule
+};
+
 class WScriptJsrt
 {
 public:
@@ -36,17 +43,18 @@ class WScriptJsrt
     private:
         JsModuleRecord moduleRecord;
         JsValueRef specifier;
+        std::string* fullPath;
 
-        ModuleMessage(JsModuleRecord module, JsValueRef specifier);
+        ModuleMessage(JsModuleRecord module, JsValueRef specifier, std::string* fullPathPtr);
 
     public:
         ~ModuleMessage();
 
         virtual HRESULT Call(LPCSTR fileName) override;
 
-        static ModuleMessage* Create(JsModuleRecord module, JsValueRef specifier)
+        static ModuleMessage* Create(JsModuleRecord module, JsValueRef specifier, std::string* fullPath = nullptr)
         {
-            return new ModuleMessage(module, specifier);
+            return new ModuleMessage(module, specifier, fullPath);
         }
 
     };
@@ -140,5 +148,6 @@ class WScriptJsrt
     static DWORD_PTR sourceContext;
     static std::map<std::string, JsModuleRecord> moduleRecordMap;
     static std::map<JsModuleRecord, std::string> moduleDirMap;
+    static std::map<JsModuleRecord, ModuleState> moduleErrMap;
     static std::map<DWORD_PTR, std::string> scriptDirMap;
 };

lib/Parser/Parse.cpp

@@ -3624,6 +3624,10 @@ ParseNodePtr Parser::ParseTerm(BOOL fAllowCall,
     case tkIMPORT:
         if (m_scriptContext->GetConfig()->IsES6ModuleEnabled() && m_scriptContext->GetConfig()->IsESDynamicImportEnabled())
         {
+            if (!fAllowCall)
+            {
+                Error(ERRTokenAfter, _u("import"), _u("new"));
+            }
             this->GetScanner()->Scan();
             ChkCurTokNoScan(tkLParen, ERRnoLparen);
             pnode = ParseImportCall<buildAST>();