Skip to content

Commit 5983ef6

Browse files
obastemurobastemur
committed
[1.8>1.9] [MERGE #4551 @obastemur] module: support for circular import
Merge pull request #4551 from obastemur:module_circular Alternative to #4550 , fixes #4482 Saw the test case brought by @rhuanjl to #4482 and the proposed fix at #4550 . After looking at the code, looks like couple of tiny changes are needed to module support. I don't have much context on modules though. This PR is just a weekend after breakfast hacking. /cc @akroshg @boingoing @rhuanjl #### how it works Separate `ModuleDeclarationInstantiation` into `ModuleDeclarationInstantiation` and `GenerateRootFunction`. This way, in case `childrenModuleSet` has circular dependents, we may instantiate all the modules prior to triggering the rest
2 parents 3444116 + 6785a49 commit 5983ef6

File tree

7 files changed

+106
-25
lines changed

7 files changed

+106
-25
lines changed

‎lib/Runtime/Language/ModuleRecordBase.h‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,8 @@ namespace Js
1515
ModuleNameRecord(const ModuleNameRecord& other)
1616
:module(other.module), bindingName(other.bindingName)
1717
{}
18-
ModuleNameRecord(ModuleRecordBase* module, PropertyId bindingName)
19-
:module(module), bindingName(bindingName)
18+
ModuleNameRecord(ModuleRecordBase* module, PropertyId bindingName)
19+
:module(module), bindingName(bindingName)
2020
{}
2121
ModuleNameRecord() {}
2222
Field(ModuleRecordBase*) module;
@@ -42,7 +42,8 @@ namespace Js
4242
// return false when "ambiguous".
4343
// otherwise nullptr means "null" where we have circular reference/cannot resolve.
4444
virtual bool ResolveExport(PropertyId exportName, ResolveSet* resolveSet, ModuleNameRecord** exportRecord) = 0;
45-
virtual void ModuleDeclarationInstantiation() = 0;
45+
virtual bool ModuleDeclarationInstantiation() = 0;
46+
virtual void GenerateRootFunction() = 0;
4647
virtual Var ModuleEvaluation() = 0;
4748
virtual bool IsSourceTextModuleRecord() { return false; }
4849

‎lib/Runtime/Language/SourceTextModuleRecord.cpp‎

Lines changed: 32 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -277,7 +277,10 @@ namespace Js
277277
HRESULT hr = NOERROR;
278278
if (!WasDeclarationInitialized())
279279
{
280-
ModuleDeclarationInstantiation();
280+
if (ModuleDeclarationInstantiation())
281+
{
282+
GenerateRootFunction();
283+
}
281284

282285
if (this->errorObject != nullptr)
283286
{
@@ -336,7 +339,10 @@ namespace Js
336339
bool isScriptActive = scriptContext->GetThreadContext()->IsScriptActive();
337340
Assert(!isScriptActive || this->promise != nullptr);
338341

339-
ModuleDeclarationInstantiation();
342+
if (ModuleDeclarationInstantiation())
343+
{
344+
GenerateRootFunction();
345+
}
340346
if (!hadNotifyHostReady && !WasEvaluated())
341347
{
342348
OUTPUT_TRACE_DEBUGONLY(Js::ModulePhase, _u("\t>NotifyHostAboutModuleReady %s (PrepareForModuleDeclarationInitialization)\n"), this->GetSpecifierSz());
@@ -800,14 +806,14 @@ namespace Js
800806
// helper information for now.
801807
}
802808

803-
void SourceTextModuleRecord::ModuleDeclarationInstantiation()
809+
bool SourceTextModuleRecord::ModuleDeclarationInstantiation()
804810
{
805811
OUTPUT_TRACE_DEBUGONLY(Js::ModulePhase, _u("ModuleDeclarationInstantiation(%s)\n"), this->GetSpecifierSz());
806812
ScriptContext* scriptContext = GetScriptContext();
807813

808814
if (this->WasDeclarationInitialized())
809815
{
810-
return;
816+
return false;
811817
}
812818

813819
try
@@ -825,7 +831,17 @@ namespace Js
825831
childrenModuleSet->Map([](LPCOLESTR specifier, SourceTextModuleRecord* moduleRecord)
826832
{
827833
Assert(moduleRecord->WasParsed());
828-
moduleRecord->ModuleDeclarationInstantiation();
834+
moduleRecord->shouldGenerateRootFunction =
835+
moduleRecord->ModuleDeclarationInstantiation();
836+
});
837+
838+
childrenModuleSet->Map([](LPCOLESTR specifier, SourceTextModuleRecord* moduleRecord)
839+
{
840+
if (moduleRecord->shouldGenerateRootFunction)
841+
{
842+
moduleRecord->shouldGenerateRootFunction = false;
843+
moduleRecord->GenerateRootFunction();
844+
}
829845
});
830846
}
831847

@@ -843,12 +859,21 @@ namespace Js
843859
{
844860
OUTPUT_TRACE_DEBUGONLY(Js::ModulePhase, _u("\t>NotifyParentsAsNeeded (errorObject)\n"));
845861
NotifyParentsAsNeeded();
846-
return;
862+
return false;
847863
}
848864

865+
return true;
866+
}
867+
868+
void SourceTextModuleRecord::GenerateRootFunction()
869+
{
870+
ScriptContext* scriptContext = GetScriptContext();
849871
Js::AutoDynamicCodeReference dynamicFunctionReference(scriptContext);
850-
Assert(this == scriptContext->GetLibrary()->GetModuleRecord(this->pSourceInfo->GetSrcInfo()->moduleID));
851872
CompileScriptException se;
873+
874+
Assert(this->WasDeclarationInitialized());
875+
Assert(this == scriptContext->GetLibrary()->GetModuleRecord(this->pSourceInfo->GetSrcInfo()->moduleID));
876+
852877
this->rootFunction = scriptContext->GenerateRootFunction(parseTree, sourceIndex, this->parser, this->pSourceInfo->GetParseFlags(), &se, _u("module"));
853878
if (rootFunction == nullptr)
854879
{

‎lib/Runtime/Language/SourceTextModuleRecord.h‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -27,11 +27,12 @@ namespace Js
2727
virtual ExportedNames* GetExportedNames(ExportModuleRecordList* exportStarSet) override;
2828
virtual bool IsSourceTextModuleRecord() override { return true; } // we don't really have other kind of modulerecord at this time.
2929

30-
// return false when "ambiguous".
30+
// return false when "ambiguous".
3131
// otherwise nullptr means "null" where we have circular reference/cannot resolve.
3232
bool ResolveExport(PropertyId exportName, ResolveSet* resolveSet, ModuleNameRecord** exportRecord) override;
3333
bool ResolveImport(PropertyId localName, ModuleNameRecord** importRecord);
34-
void ModuleDeclarationInstantiation() override;
34+
bool ModuleDeclarationInstantiation() override;
35+
void GenerateRootFunction();
3536
Var ModuleEvaluation() override;
3637
virtual ModuleNamespace* GetNamespace();
3738
virtual void SetNamespace(ModuleNamespace* moduleNamespace);
@@ -114,8 +115,9 @@ namespace Js
114115
const static uint InvalidSlotCount = 0xffffffff;
115116
const static uint InvalidSlotIndex = 0xffffffff;
116117
// TODO: move non-GC fields out to avoid false reference?
117-
// This is the parsed tree resulted from compilation.
118+
// This is the parsed tree resulted from compilation.
118119
Field(bool) wasParsed;
120+
Field(bool) shouldGenerateRootFunction;
119121
Field(bool) wasDeclarationInitialized;
120122
Field(bool) parentsNotified;
121123
Field(bool) isRootModule;

‎test/es6/module-bugfixes.js‎

Lines changed: 19 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -42,18 +42,25 @@ var tests = [
4242
testRunner.LoadModule(functionBody, 'samethread');
4343
}
4444
},
45-
{
46-
name: "Memory leak test on syntax error",
47-
body: function() {
48-
try {
49-
WScript.LoadModule('');
50-
WScript.LoadModule('1');
51-
WScript.LoadModule('const a = () -> {};');
52-
} catch(e) {
53-
// no-op
54-
}
55-
}
56-
},
45+
{
46+
name: "Memory leak test on syntax error",
47+
body: function() {
48+
try {
49+
WScript.LoadModule('');
50+
WScript.LoadModule('1');
51+
WScript.LoadModule('const a = () -> {};');
52+
} catch(e) {
53+
// no-op
54+
}
55+
}
56+
},
57+
{
58+
name: "Issue 4482: Indirect circular module dependencies",
59+
body: function() {
60+
let functionBody = "import 'module_4482_dep1.js';"
61+
testRunner.LoadModule(functionBody);
62+
}
63+
},
5764
];
5865

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

‎test/es6/module_4482_dep1.js‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
//-------------------------------------------------------------------------------------------------------
2+
// Copyright (C) Microsoft. All rights reserved.
3+
// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
4+
//-------------------------------------------------------------------------------------------------------
5+
6+
//this test case's circular dependencies would segfault
7+
//with CC's orriginal module implementation if JIT was enabled
8+
//issue was that:
9+
//i) dep 1 requires dep2 and dep3
10+
//ii) MDI would therefore be triggerred for dep2 (with dep3 queued)
11+
//iii) MDI for dep2 would not explicitly require dep3 as the import is via dep1
12+
//iv) second half of MDI for dep2 would attempt to reference the function Thing2 defined in dep 3
13+
//v) Thing2 would not yet exist = segfault
14+
15+
16+
import Thing1 from './module_4482_dep2.js';
17+
import Thing2 from './module_4482_dep3.js';
18+
19+
export { Thing1, Thing2 };
20+
21+
export default
22+
function main()
23+
{}

‎test/es6/module_4482_dep2.js‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
//-------------------------------------------------------------------------------------------------------
2+
// Copyright (C) Microsoft. All rights reserved.
3+
// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
4+
//-------------------------------------------------------------------------------------------------------
5+
6+
//thing1.js
7+
import { Thing2 } from './module_4482_dep1.js';
8+
9+
export default
10+
function Thing1()
11+
{
12+
Thing2();
13+
}

‎test/es6/module_4482_dep3.js‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
//-------------------------------------------------------------------------------------------------------
2+
// Copyright (C) Microsoft. All rights reserved.
3+
// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
4+
//-------------------------------------------------------------------------------------------------------
5+
6+
7+
import { Thing1 } from './module_4482_dep1.js';
8+
9+
export default
10+
function Thing2(){}

0 commit comments

Comments
 (0)