Skip to content

Commit 90b2ecf

Browse files
sosukesuzukisosukesuzuki
committed
[JSC] hostResolveImportedModule should respect the module request's import attribute type
https://bugs.webkit.org/show_bug.cgi?id=320341 Reviewed by Yusuke Suzuki. Since 317844@main, hostResolveImportedModule looks up [[LoadedModules]] by specifier alone, probing each ScriptFetchParameters::Type in a fixed order (JavaScript first). When one specifier is loaded under two types, JSON/WebAssembly-typed bindings resolve against the JavaScript module: // a.js contains `42`, run with --useImportDefer=1 import jsonValue from "./a.js" with { type: "json" }; // SyntaxError: no default export in the JavaScript module import defer * as ns from "./a.js"; This patch: 1. Records the request's ScriptFetchParameters::Type in ImportEntry, ExportEntry, and star export entries, and makes hostResolveImportedModule an exact (specifier, type) lookup. The enum fits existing struct padding, so entry sizes are unchanged. 2. Dedups ModuleAnalyzer's requested modules by (specifier, type) instead of specifier alone, so two same-specifier requests with different types are both loaded. 3. Keys m_starExportEntries by (module name, type) instead of module name alone (still an OrderedHashSet, so occurrence order is preserved), which keeps `export * from "./a"` and `export * from "./a" with { type: "json" }` distinct. Tests: JSTests/modules/import-attributes-same-specifier-different-type.js JSTests/modules/import-attributes-same-specifier-same-phase.js * JSTests/modules/import-attributes-same-specifier-different-type.js: Added. * JSTests/modules/import-attributes-same-specifier-same-phase.js: Added. * JSTests/modules/resources/dual-type-reexport.js: Added. * JSTests/modules/resources/dual-type.js: Added. * Source/JavaScriptCore/parser/ModuleAnalyzer.cpp: (JSC::ModuleAnalyzer::appendRequestedModule): (JSC::ModuleAnalyzer::exportVariable): * Source/JavaScriptCore/parser/ModuleAnalyzer.h: * Source/JavaScriptCore/parser/NodesAnalyzeModule.cpp: (JSC::ImportDeclarationNode::analyzeModule): (JSC::ExportAllDeclarationNode::analyzeModule): (JSC::ExportNamedDeclarationNode::analyzeModule): * Source/JavaScriptCore/runtime/AbstractModuleRecord.cpp: (JSC::AbstractModuleRecord::addStarExportEntry): (JSC::AbstractModuleRecord::ExportEntry::createLocal): (JSC::AbstractModuleRecord::ExportEntry::createIndirect): (JSC::AbstractModuleRecord::ExportEntry::createNamespace): (JSC::AbstractModuleRecord::hostResolveImportedModule): (JSC::AbstractModuleRecord::resolveImport): (JSC::AbstractModuleRecord::resolveExportImpl): (JSC::AbstractModuleRecord::resolveExport): (JSC::AbstractModuleRecord::getModuleNamespace): (JSC::AbstractModuleRecord::dump): * Source/JavaScriptCore/runtime/AbstractModuleRecord.h: * Source/JavaScriptCore/runtime/CyclicModuleRecord.cpp: (JSC::CyclicModuleRecord::initializeEnvironment): * Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp: (JSC::JSWebAssemblyInstance::tryCreate): * Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp: (JSC::WebAssemblyModuleRecord::initializeImports): Canonical link: https://commits.webkit.org/318292@main
1 parent 8a83b30 commit 90b2ecf

12 files changed

Lines changed: 101 additions & 39 deletions

‎JSTests/modules/import-attributes-same-specifier-different-type.js‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
//@ requireOptions("--useImportDefer=1")
2+
import { shouldBe } from "./resources/assert.js";
3+
4+
// The same specifier is requested both as a JSON module (evaluation phase) and as a
5+
// JavaScript module (defer phase), so [[LoadedModules]] holds it under both types.
6+
// Each binding must resolve against the module record matching its request's type.
7+
import jsonValue from "./resources/dual-type.js" with { type: "json" };
8+
import * as jsonNs from "./resources/dual-type.js" with { type: "json" };
9+
import defer * as deferredNs from "./resources/dual-type.js";
10+
import { reExportedJson, reExportedJsonNs, deferredTag } from "./resources/dual-type-reexport.js";
11+
12+
shouldBe(jsonValue, 42);
13+
shouldBe(jsonNs.default, 42);
14+
shouldBe(Object.prototype.toString.call(deferredNs), "[object Deferred Module]");
15+
shouldBe(reExportedJson, 42);
16+
shouldBe(reExportedJsonNs.default, 42);
17+
shouldBe(deferredTag, "[object Deferred Module]");

‎JSTests/modules/import-attributes-same-specifier-same-phase.js‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
import { shouldBe } from "./resources/assert.js";
2+
3+
// A JSON-typed request and an attribute-less (JavaScript) request for the same
4+
// specifier are distinct module requests: both must be loaded, and each binding
5+
// must resolve against the module record matching its request's type.
6+
import jsonValue from "./resources/dual-type.js" with { type: "json" };
7+
import * as jsNs from "./resources/dual-type.js";
8+
9+
shouldBe(jsonValue, 42);
10+
shouldBe(Object.prototype.toString.call(jsNs), "[object Module]");
11+
shouldBe("default" in jsNs, false);

‎JSTests/modules/resources/dual-type-reexport.js‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
export { default as reExportedJson } from "./dual-type.js" with { type: "json" };
2+
export * as reExportedJsonNs from "./dual-type.js" with { type: "json" };
3+
// Requests the same specifier as a JavaScript module in the defer phase, so this
4+
// module's [[LoadedModules]] holds "./dual-type.js" under both types.
5+
import defer * as deferredNs from "./dual-type.js";
6+
export const deferredTag = Object.prototype.toString.call(deferredNs);

‎JSTests/modules/resources/dual-type.js‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
42

‎Source/JavaScriptCore/parser/ModuleAnalyzer.cpp‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,8 @@ ModuleAnalyzer::ModuleAnalyzer(JSGlobalObject* globalObject, const Identifier& m
4343

4444
void ModuleAnalyzer::appendRequestedModule(const Identifier& specifier, RefPtr<ScriptFetchParameters>&& attributes, AbstractModuleRecord::ModulePhase phase)
4545
{
46-
if (m_requestedModules[phase].add(specifier.impl()).isNewEntry)
46+
ModuleMapKey key { specifier.impl(), attributes ? attributes->type() : ScriptFetchParameters::Type::JavaScript };
47+
if (m_requestedModules[phase].add(key).isNewEntry)
4748
moduleRecord()->appendRequestedModule(specifier, WTF::move(attributes), phase);
4849
}
4950

@@ -87,7 +88,7 @@ void ModuleAnalyzer::exportVariable(ModuleProgramNode& moduleProgramNode, const
8788
if (importEntry.phase == AbstractModuleRecord::ModulePhase::Defer)
8889
moduleRecord()->addExportEntry(JSModuleRecord::ExportEntry::createLocal(Identifier::fromUid(m_vm, exportName.get()), Identifier::fromUid(m_vm, localName.get())));
8990
else
90-
moduleRecord()->addExportEntry(JSModuleRecord::ExportEntry::createNamespace(Identifier::fromUid(m_vm, exportName.get()), importEntry.moduleRequest));
91+
moduleRecord()->addExportEntry(JSModuleRecord::ExportEntry::createNamespace(Identifier::fromUid(m_vm, exportName.get()), importEntry.moduleRequest, importEntry.moduleRequestType));
9192
}
9293
return;
9394
}
@@ -96,7 +97,7 @@ void ModuleAnalyzer::exportVariable(ModuleProgramNode& moduleProgramNode, const
9697
// import a from "mod"
9798
// export { a }
9899
for (auto& exportName : moduleProgramNode.moduleScopeData().exportedBindings().get(localName.get()))
99-
moduleRecord()->addExportEntry(JSModuleRecord::ExportEntry::createIndirect(Identifier::fromUid(m_vm, exportName.get()), importEntry.importName, importEntry.moduleRequest));
100+
moduleRecord()->addExportEntry(JSModuleRecord::ExportEntry::createIndirect(Identifier::fromUid(m_vm, exportName.get()), importEntry.importName, importEntry.moduleRequest, importEntry.moduleRequestType));
100101
}
101102

102103

‎Source/JavaScriptCore/parser/ModuleAnalyzer.h‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727

2828
#include "ErrorType.h"
2929
#include "JSModuleRecord.h"
30+
#include "ModuleMap.h"
3031
#include "Nodes.h"
3132
#include <wtf/EnumeratedArray.h>
3233

@@ -57,7 +58,7 @@ class ModuleAnalyzer {
5758

5859
VM& m_vm;
5960
JSModuleRecord* m_moduleRecord;
60-
EnumeratedArray<AbstractModuleRecord::ModulePhase, IdentifierSet, AbstractModuleRecord::ModulePhase::Defer> m_requestedModules;
61+
EnumeratedArray<AbstractModuleRecord::ModulePhase, UncheckedKeyHashSet<ModuleMapKey, ModuleMapHash>, AbstractModuleRecord::ModulePhase::Defer> m_requestedModules;
6162
std::tuple<ErrorType, String> m_errorMessage;
6263
};
6364

‎Source/JavaScriptCore/parser/NodesAnalyzeModule.cpp‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -85,12 +85,14 @@ bool ImportDeclarationNode::analyzeModule(ModuleAnalyzer& analyzer)
8585
}
8686

8787
auto phase = m_type == ImportType::Deferred ? AbstractModuleRecord::ModulePhase::Defer : AbstractModuleRecord::ModulePhase::Evaluation;
88+
auto moduleRequestType = result.value() ? result.value()->type() : ScriptFetchParameters::Type::JavaScript;
8889
analyzer.appendRequestedModule(m_moduleName->moduleName(), WTF::move(result.value()), phase);
8990
for (auto* specifier : m_specifierList->specifiers()) {
9091
analyzer.moduleRecord()->addImportEntry(JSModuleRecord::ImportEntry {
9192
specifier->importedName() == analyzer.vm().propertyNames->starNamespacePrivateName
9293
? JSModuleRecord::ImportEntryType::Namespace : JSModuleRecord::ImportEntryType::Single,
9394
phase,
95+
moduleRequestType,
9496
m_moduleName->moduleName(),
9597
specifier->importedName(),
9698
specifier->localName(),
@@ -107,8 +109,9 @@ bool ExportAllDeclarationNode::analyzeModule(ModuleAnalyzer& analyzer)
107109
return false;
108110
}
109111

112+
auto moduleRequestType = result.value() ? result.value()->type() : ScriptFetchParameters::Type::JavaScript;
110113
analyzer.appendRequestedModule(m_moduleName->moduleName(), WTF::move(result.value()));
111-
analyzer.moduleRecord()->addStarExportEntry(m_moduleName->moduleName());
114+
analyzer.moduleRecord()->addStarExportEntry(m_moduleName->moduleName(), moduleRequestType);
112115
return true;
113116
}
114117

@@ -124,13 +127,16 @@ bool ExportLocalDeclarationNode::analyzeModule(ModuleAnalyzer&)
124127

125128
bool ExportNamedDeclarationNode::analyzeModule(ModuleAnalyzer& analyzer)
126129
{
130+
auto moduleRequestType = ScriptFetchParameters::Type::JavaScript;
127131
if (m_moduleName) {
128132
auto result = tryCreateAttributes(analyzer.vm(), attributesList());
129133
if (!result) {
130134
analyzer.fail(WTF::move(result.error()));
131135
return false;
132136
}
133137

138+
if (result.value())
139+
moduleRequestType = result.value()->type();
134140
analyzer.appendRequestedModule(m_moduleName->moduleName(), WTF::move(result.value()));
135141
}
136142

@@ -145,9 +151,9 @@ bool ExportNamedDeclarationNode::analyzeModule(ModuleAnalyzer& analyzer)
145151
//
146152
// If it is namespace export, we should use createNamespace.
147153
if (specifier->localName() == analyzer.vm().propertyNames->starNamespacePrivateName)
148-
analyzer.moduleRecord()->addExportEntry(JSModuleRecord::ExportEntry::createNamespace(specifier->exportedName(), m_moduleName->moduleName()));
154+
analyzer.moduleRecord()->addExportEntry(JSModuleRecord::ExportEntry::createNamespace(specifier->exportedName(), m_moduleName->moduleName(), moduleRequestType));
149155
else
150-
analyzer.moduleRecord()->addExportEntry(JSModuleRecord::ExportEntry::createIndirect(specifier->exportedName(), specifier->localName(), m_moduleName->moduleName()));
156+
analyzer.moduleRecord()->addExportEntry(JSModuleRecord::ExportEntry::createIndirect(specifier->exportedName(), specifier->localName(), m_moduleName->moduleName(), moduleRequestType));
151157
}
152158
}
153159
return true;

‎Source/JavaScriptCore/runtime/AbstractModuleRecord.cpp‎

Lines changed: 21 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -138,9 +138,9 @@ void AbstractModuleRecord::appendRequestedModule(const Identifier& moduleName, R
138138
m_requestedModules.append({ moduleName, WTF::move(attributes), phase });
139139
}
140140

141-
void AbstractModuleRecord::addStarExportEntry(const Identifier& moduleName)
141+
void AbstractModuleRecord::addStarExportEntry(const Identifier& moduleName, ScriptFetchParameters::Type moduleRequestType)
142142
{
143-
m_starExportEntries.add(moduleName.impl());
143+
m_starExportEntries.add({ moduleName.impl(), moduleRequestType });
144144
}
145145

146146
void AbstractModuleRecord::addImportEntry(const ImportEntry& entry)
@@ -173,17 +173,17 @@ auto AbstractModuleRecord::tryGetExportEntry(UniquedStringImpl* exportName) -> s
173173

174174
auto AbstractModuleRecord::ExportEntry::createLocal(const Identifier& exportName, const Identifier& localName) -> ExportEntry
175175
{
176-
return ExportEntry { Type::Local, exportName, Identifier(), Identifier(), localName };
176+
return ExportEntry { Type::Local, ScriptFetchParameters::Type::JavaScript, exportName, Identifier(), Identifier(), localName };
177177
}
178178

179-
auto AbstractModuleRecord::ExportEntry::createIndirect(const Identifier& exportName, const Identifier& importName, const Identifier& moduleName) -> ExportEntry
179+
auto AbstractModuleRecord::ExportEntry::createIndirect(const Identifier& exportName, const Identifier& importName, const Identifier& moduleName, ScriptFetchParameters::Type moduleRequestType) -> ExportEntry
180180
{
181-
return ExportEntry { Type::Indirect, exportName, moduleName, importName, Identifier() };
181+
return ExportEntry { Type::Indirect, moduleRequestType, exportName, moduleName, importName, Identifier() };
182182
}
183183

184-
auto AbstractModuleRecord::ExportEntry::createNamespace(const Identifier& exportName, const Identifier& moduleName) -> ExportEntry
184+
auto AbstractModuleRecord::ExportEntry::createNamespace(const Identifier& exportName, const Identifier& moduleName, ScriptFetchParameters::Type moduleRequestType) -> ExportEntry
185185
{
186-
return ExportEntry { Type::Namespace, exportName, moduleName, Identifier(), Identifier() };
186+
return ExportEntry { Type::Namespace, moduleRequestType, exportName, moduleName, Identifier(), Identifier() };
187187
}
188188

189189
auto AbstractModuleRecord::Resolution::notFound() -> Resolution
@@ -201,12 +201,10 @@ auto AbstractModuleRecord::Resolution::ambiguous() -> Resolution
201201
return Resolution { Type::Ambiguous, nullptr, Identifier() };
202202
}
203203

204-
AbstractModuleRecord* AbstractModuleRecord::hostResolveImportedModule(JSGlobalObject*, const Identifier& moduleName)
204+
AbstractModuleRecord* AbstractModuleRecord::hostResolveImportedModule(JSGlobalObject*, const Identifier& moduleName, ScriptFetchParameters::Type moduleRequestType)
205205
{
206-
for (auto type : { ScriptFetchParameters::Type::JavaScript, ScriptFetchParameters::Type::JSON, ScriptFetchParameters::Type::WebAssembly, ScriptFetchParameters::Type::None }) {
207-
if (auto iter = m_loadedModules.find(ModuleMapKey { moduleName.impl(), type }); iter != m_loadedModules.end())
208-
return iter->value.m_module.get();
209-
}
206+
if (auto iter = m_loadedModules.find(ModuleMapKey { moduleName.impl(), moduleRequestType }); iter != m_loadedModules.end())
207+
return iter->value.m_module.get();
210208
return nullptr;
211209
}
212210

@@ -223,7 +221,7 @@ auto AbstractModuleRecord::resolveImport(JSGlobalObject* globalObject, const Ide
223221
if (importEntry.type == AbstractModuleRecord::ImportEntryType::Namespace)
224222
return Resolution::notFound();
225223

226-
AbstractModuleRecord* importedModule = hostResolveImportedModule(globalObject, importEntry.moduleRequest);
224+
AbstractModuleRecord* importedModule = hostResolveImportedModule(globalObject, importEntry.moduleRequest, importEntry.moduleRequestType);
227225
RETURN_IF_EXCEPTION(scope, Resolution::error());
228226

229227
RELEASE_AND_RETURN(scope, importedModule->resolveExport(globalObject, importEntry.importName));
@@ -600,8 +598,8 @@ auto AbstractModuleRecord::resolveExportImpl(JSGlobalObject* globalObject, const
600598

601599
// Enqueue the tasks in reverse order.
602600
for (auto iterator = query.moduleRecord->starExportEntries().rbegin(), end = query.moduleRecord->starExportEntries().rend(); iterator != end; ++iterator) {
603-
const RefPtr<UniquedStringImpl>& starModuleName = *iterator;
604-
AbstractModuleRecord* importedModuleRecord = query.moduleRecord->hostResolveImportedModule(globalObject, Identifier::fromUid(vm, starModuleName.get()));
601+
const auto& [starModuleName, starModuleRequestType] = *iterator;
602+
AbstractModuleRecord* importedModuleRecord = query.moduleRecord->hostResolveImportedModule(globalObject, Identifier::fromUid(vm, starModuleName.get()), starModuleRequestType);
605603
RETURN_IF_EXCEPTION(scope, false);
606604
pendingTasks.append(Task { ResolveQuery(importedModuleRecord, query.exportName.get()), Type::Query });
607605
}
@@ -690,7 +688,7 @@ auto AbstractModuleRecord::resolveExportImpl(JSGlobalObject* globalObject, const
690688
}
691689
}
692690

693-
AbstractModuleRecord* importedModuleRecord = moduleRecord->hostResolveImportedModule(globalObject, exportEntry.moduleName);
691+
AbstractModuleRecord* importedModuleRecord = moduleRecord->hostResolveImportedModule(globalObject, exportEntry.moduleName, exportEntry.moduleRequestType);
694692
RETURN_IF_EXCEPTION(scope, Resolution::error());
695693

696694
// When the imported module does not produce any resolved binding, we need to look into the stars in the *current*
@@ -703,7 +701,7 @@ auto AbstractModuleRecord::resolveExportImpl(JSGlobalObject* globalObject, const
703701
}
704702

705703
case ExportEntry::Type::Namespace: {
706-
AbstractModuleRecord* importedModuleRecord = moduleRecord->hostResolveImportedModule(globalObject, exportEntry.moduleName);
704+
AbstractModuleRecord* importedModuleRecord = moduleRecord->hostResolveImportedModule(globalObject, exportEntry.moduleName, exportEntry.moduleRequestType);
707705
RETURN_IF_EXCEPTION(scope, Resolution::error());
708706
Resolution resolution { Resolution::Type::Resolved, importedModuleRecord, vm.propertyNames->starNamespacePrivateName };
709707
if (!mergeToCurrentTop(resolution))
@@ -774,7 +772,7 @@ auto AbstractModuleRecord::resolveExport(JSGlobalObject* globalObject, const Ide
774772
ASSERT(!entry.localName.isNull());
775773
return Resolution { Resolution::Type::Resolved, this, entry.localName };
776774
case ExportEntry::Type::Namespace: {
777-
AbstractModuleRecord* importedModuleRecord = hostResolveImportedModule(globalObject, entry.moduleName);
775+
AbstractModuleRecord* importedModuleRecord = hostResolveImportedModule(globalObject, entry.moduleName, entry.moduleRequestType);
778776
RETURN_IF_EXCEPTION(scope, Resolution::error());
779777
return Resolution { Resolution::Type::Resolved, importedModuleRecord, vm.propertyNames->starNamespacePrivateName };
780778
}
@@ -856,7 +854,7 @@ JSModuleNamespaceObject* AbstractModuleRecord::getModuleNamespace(JSGlobalObject
856854
candidate = { Resolution::Type::Resolved, moduleRecord, exportEntry.localName };
857855
break;
858856
case ExportEntry::Type::Namespace: {
859-
AbstractModuleRecord* importedModuleRecord = moduleRecord->hostResolveImportedModule(globalObject, exportEntry.moduleName);
857+
AbstractModuleRecord* importedModuleRecord = moduleRecord->hostResolveImportedModule(globalObject, exportEntry.moduleName, exportEntry.moduleRequestType);
860858
RETURN_IF_EXCEPTION(scope, nullptr);
861859
candidate = { Resolution::Type::Resolved, importedModuleRecord, vm.propertyNames->starNamespacePrivateName };
862860
break;
@@ -883,8 +881,8 @@ JSModuleNamespaceObject* AbstractModuleRecord::getModuleNamespace(JSGlobalObject
883881
}
884882
}
885883

886-
for (const auto& starModuleName : moduleRecord->starExportEntries()) {
887-
AbstractModuleRecord* requestedModuleRecord = moduleRecord->hostResolveImportedModule(globalObject, Identifier::fromUid(vm, starModuleName.get()));
884+
for (const auto& [starModuleName, starModuleRequestType] : moduleRecord->starExportEntries()) {
885+
AbstractModuleRecord* requestedModuleRecord = moduleRecord->hostResolveImportedModule(globalObject, Identifier::fromUid(vm, starModuleName.get()), starModuleRequestType);
888886
RETURN_IF_EXCEPTION(scope, nullptr);
889887
pendingModules.append(requestedModuleRecord);
890888
}
@@ -1474,8 +1472,8 @@ void AbstractModuleRecord::dump()
14741472
break;
14751473
}
14761474
}
1477-
for (const auto& moduleName : m_starExportEntries)
1478-
dataLog(" [Star] module(", printableName(moduleName.get()), ")\n");
1475+
for (const auto& starExportEntry : m_starExportEntries)
1476+
dataLog(" [Star] module(", printableName(starExportEntry.first), ")\n");
14791477
}
14801478

14811479
} // namespace JSC

0 commit comments

Comments
 (0)