Skip to content

Commit cc673d7

Browse files
ConstellationConstellation
committed
[JSC] Update import-defer implementation based on the proposal update
https://bugs.webkit.org/show_bug.cgi?id=321369 rdar://184423667 Reviewed by Sosuke Suzuki. After 313139@main implemented import-defer, the proposal fixed several bugs. This patch applies these fixes. 1. tc39/proposal-defer-import-eval#85 Consider SCC status when deciding to short circuit on ReadyForSyncExecution and GatherAsynchronousTransitiveDependencies 2. tc39/proposal-defer-import-eval#87 (1) introduced an assertion inconsistency, and the PR fixes it. Tests: JSTests/modules/import-defer-async-cycle-sync-access.js JSTests/modules/import-defer/async-cycle-deferred.js JSTests/modules/import-defer/async-cycle-member.js JSTests/modules/import-defer/async-cycle-setup.js JSTests/modules/import-defer/async-cycle-tla.js JSTests/modules/import-defer/async-cycle-toucher.js * JSTests/modules/import-defer-async-cycle-sync-access.js: Added. * JSTests/modules/import-defer/async-cycle-deferred.js: Added. * JSTests/modules/import-defer/async-cycle-member.js: Added. * JSTests/modules/import-defer/async-cycle-setup.js: Added. * JSTests/modules/import-defer/async-cycle-tla.js: Added. * JSTests/modules/import-defer/async-cycle-toucher.js: Added. * JSTests/test262/expectations.yaml: * Source/JavaScriptCore/runtime/AbstractModuleRecord.cpp: (JSC::AbstractModuleRecord::gatherAsynchronousTransitiveDependencies): (JSC::AbstractModuleRecord::readyForSyncExecution): * Source/JavaScriptCore/runtime/CyclicModuleRecord.h: (JSC::CyclicModuleRecord::isSCCEvaluated const): Canonical link: https://commits.webkit.org/318875@main
1 parent 40d37f3 commit cc673d7

9 files changed

Lines changed: 76 additions & 8 deletions

File tree

‎JSTests/modules/import-defer-async-cycle-sync-access.js‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
//@ requireOptions("--useImportDefer=1")
2+
import { shouldBe, shouldThrow } from "./resources/assert.js";
3+
import { blocker, aStarted } from "./import-defer/async-cycle-setup.js";
4+
5+
// SCC {tla, member}: tla has TLA and is the cycle root, member is the non-root member.
6+
// While tla is suspended on the blocker, member has already reached EVALUATED even though the
7+
// cycle has not finished. The deferred module behind ns depends on member, so it still cannot be
8+
// evaluated synchronously: ReadyForSyncExecution must consult IsModuleSCCEvaluated(member), which
9+
// follows [[CycleRoot]], rather than member's own [[Status]].
10+
11+
const evaluations = globalThis.asyncCycleEvaluations;
12+
13+
const pTLA = import("./import-defer/async-cycle-tla.js");
14+
await aStarted.promise;
15+
16+
shouldBe(JSON.stringify(evaluations), JSON.stringify(["toucher", "member", "A-before-await"]));
17+
18+
shouldThrow(() => globalThis.asyncCycleTouch(), "TypeError: Unable to synchronously evaluate deferred module");
19+
shouldBe(JSON.stringify(evaluations), JSON.stringify(["toucher", "member", "A-before-await"]));
20+
21+
blocker.resolve();
22+
await pTLA;
23+
24+
// The cycle root is EVALUATED now, so the same access succeeds and evaluates only the deferred module.
25+
shouldBe(globalThis.asyncCycleTouch(), 1);
26+
shouldBe(JSON.stringify(evaluations), JSON.stringify(["toucher", "member", "A-before-await", "A-after-await", "deferred"]));

‎JSTests/modules/import-defer/async-cycle-deferred.js‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
import "./async-cycle-member.js";
2+
3+
globalThis.asyncCycleEvaluations.push("deferred");
4+
export const value = 1;

‎JSTests/modules/import-defer/async-cycle-member.js‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
import "./async-cycle-tla.js";
2+
import "./async-cycle-toucher.js";
3+
4+
globalThis.asyncCycleEvaluations.push("member");

‎JSTests/modules/import-defer/async-cycle-setup.js‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
globalThis.asyncCycleEvaluations = [];
2+
3+
export const blocker = Promise.withResolvers();
4+
export const aStarted = Promise.withResolvers();

‎JSTests/modules/import-defer/async-cycle-tla.js‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
import { blocker, aStarted } from "./async-cycle-setup.js";
2+
import "./async-cycle-member.js";
3+
4+
globalThis.asyncCycleEvaluations.push("A-before-await");
5+
aStarted.resolve();
6+
await blocker.promise;
7+
globalThis.asyncCycleEvaluations.push("A-after-await");

‎JSTests/modules/import-defer/async-cycle-toucher.js‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
import defer * as ns from "./async-cycle-deferred.js";
2+
3+
// This module is evaluated while the cycle it belongs to is still on the evaluation stack, so
4+
// GatherAsynchronousTransitiveDependencies finds nothing to await and the deferred graph stays
5+
// unevaluated. Handing the access out lets the test poke it at a chosen point.
6+
globalThis.asyncCycleTouch = () => ns.value;
7+
globalThis.asyncCycleEvaluations.push("toucher");

‎JSTests/test262/expectations.yaml‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -144,8 +144,6 @@ test/language/import/import-attributes/text-string.js:
144144
module: 3
145145
test/language/import/import-attributes/text-via-namespace.js:
146146
module: 3
147-
test/language/import/import-defer/evaluation-top-level-await/async-cycle-dependency-of-deferred-module/main.js:
148-
module: 3
149147
test/language/statements/class/elements/private-class-field-on-nonextensible-objects.js:
150148
strict mode: 3
151149
test/language/statements/class/subclass/private-class-field-on-nonextensible-return-override.js:

‎Source/JavaScriptCore/runtime/AbstractModuleRecord.cpp‎

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -956,8 +956,8 @@ void AbstractModuleRecord::gatherAsynchronousTransitiveDependencies(OrderedHashS
956956
auto* cyclic = dynamicDowncast<CyclicModuleRecord>(module);
957957
if (!cyclic)
958958
continue;
959-
// 6. If module.[[Status]] is either EVALUATING or EVALUATED, return result.
960-
if (cyclic->status() == CyclicModuleRecord::Status::Evaluating || cyclic->status() == CyclicModuleRecord::Status::Evaluated)
959+
// 6. If module.[[Status]] is either EVALUATING or IsModuleSCCEvaluated(module), return result.
960+
if (cyclic->status() == CyclicModuleRecord::Status::Evaluating || cyclic->isSCCEvaluated())
961961
continue;
962962
// 7. If module.[[HasTLA]] is true, then
963963
if (cyclic->hasTLA()) {
@@ -994,14 +994,17 @@ bool AbstractModuleRecord::readyForSyncExecution()
994994
// 4. Append module to seen.
995995
if (!seen.add(module).isNewEntry)
996996
continue;
997-
// 5. If module.[[Status]] is EVALUATED, return true.
998-
if (cyclic->status() == CyclicModuleRecord::Status::Evaluated)
997+
// 5. If IsModuleSCCEvaluated(module), return true.
998+
if (cyclic->isSCCEvaluated())
999999
continue;
10001000
// 6. If module.[[Status]] is either EVALUATING or EVALUATING-ASYNC, return false.
10011001
if (cyclic->status() == CyclicModuleRecord::Status::Evaluating || cyclic->status() == CyclicModuleRecord::Status::EvaluatingAsync)
10021002
return false;
1003-
// 7. Assert: module.[[Status]] is LINKED.
1004-
ASSERT(cyclic->status() == CyclicModuleRecord::Status::Linked);
1003+
// 7. Assert: module.[[Status]] is LINKED or EVALUATED.
1004+
// EVALUATED is reachable for a module whose own body has run inside a cycle that is still
1005+
// awaiting; the walk below then reaches its EVALUATING-ASYNC cycle root and returns false.
1006+
// https://github.com/tc39/proposal-defer-import-eval/issues/86
1007+
ASSERT(cyclic->status() == CyclicModuleRecord::Status::Linked || cyclic->status() == CyclicModuleRecord::Status::Evaluated);
10051008
// 8. If module.[[HasTLA]] is true, return false.
10061009
if (cyclic->hasTLA())
10071010
return false;

‎Source/JavaScriptCore/runtime/CyclicModuleRecord.h‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,21 @@ class CyclicModuleRecord : public AbstractModuleRecord {
6767
JSValue evaluationError() const { return m_evaluationError.get(); }
6868
unsigned dfsAncestorIndex() const { return m_dfsAncestorIndex; }
6969

70+
// https://tc39.es/proposal-defer-import-eval/#sec-IsModuleSCCEvaluated
71+
// A module in an import cycle reaches EVALUATED once its own body has run, so only its cycle
72+
// root reaching EVALUATED tells you the whole cycle is done.
73+
bool isSCCEvaluated() const
74+
{
75+
// 1. If module.[[CycleRoot]] is not EMPTY, then
76+
// 1.a. If module.[[CycleRoot]].[[Status]] is EVALUATED, return true.
77+
// 1.b. Return false.
78+
if (CyclicModuleRecord* root = cycleRoot())
79+
return root->status() == Status::Evaluated;
80+
// 2. If module.[[Status]] is EVALUATED, return true.
81+
// 3. Return false.
82+
return status() == Status::Evaluated;
83+
}
84+
7085
void setStatus(Status newStatus) { m_status = newStatus; }
7186
void setEvaluationError(VM& vm, JSValue error) { m_evaluationError.set(vm, this, error); }
7287
void setDFSAncestorIndex(unsigned newIndex) { m_dfsAncestorIndex = newIndex; }

0 commit comments

Comments
 (0)