Skip to content

JSModuleRecord::evaluate: resume suspended TLA body when a cycle sibling throws #281

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
robobun wants to merge 1 commit into
base: main
Choose a base branch
from
Open

JSModuleRecord::evaluate: resume suspended TLA body when a cycle sibling throws #281

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions Source/JavaScriptCore/runtime/JSModuleRecord.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,9 +112,15 @@
VM& vm = globalObject->vm();
auto scope = DECLARE_THROW_SCOPE(vm);

if (JSValue error = evaluationError()) {
scope.throwException(globalObject, error);
return { };
// A TLA body suspended at an await (State != Init) must resume even if a
// sibling in the same cycle set our graph-level evaluationError meanwhile;
// asyncExecution{Fulfilled,Rejected} already no-op when status == Evaluated.
JSValue state = internalField(Field::State).get();
if (state.isNumber() && state.asInt32AsAnyInt() == std::to_underlying(State::Init)) {

Check warning on line 119 in Source/JavaScriptCore/runtime/JSModuleRecord.cpp

View check run for this annotation

Claude / Claude Code Review

New 'state' local shadowed by existing if-init 'state' below 

nit: The new function-scope `state` local (line 118) is shadowed by the pre-existing if-init `JSValue state = ...` on line 130. Both reads are correct — line 130 intentionally re-reads after `executeModuleProgram` may mutate the field — but renaming this one to something like `initialState` (or scoping it in its own if-init) would make it clearer that it's the pre-execution snapshot.
Comment on lines +118 to +119

@claude claude Bot Jul 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 nit: The new function-scope state local (line 118) is shadowed by the pre-existing if-init JSValue state = ... on line 130. Both reads are correct — line 130 intentionally re-reads after executeModuleProgram may mutate the field — but renaming this one to something like initialState (or scoping it in its own if-init) would make it clearer that it's the pre-execution snapshot.

Extended reasoning...

What

The PR introduces JSValue state = internalField(Field::State).get(); at function scope on line 118 of JSModuleRecord::evaluate. A few lines below, the pre-existing code on line 130 has:

if (JSValue state = internalField(Field::State).get(); !state.isNumber() || ...)

which declares a second state in the C++17 if-init scope, shadowing the outer one added by this PR.

Why it's only a nit

Both reads are semantically correct and intentionally distinct:

  • Line 118 reads the state before executeModuleProgram runs, to decide whether the evaluationError() early-return applies (only when the body is still in State::Init).
  • Line 130 re-reads the state after executeModuleProgram returns, because the interpreter mutates Field::State during execution (e.g. to Executing on completion, or a suspend index on await). Reusing the outer state here would be wrong — it holds a stale pre-execution value — so the re-read is required regardless of naming.

There is no build impact: JavaScriptCore does not compile with -Wshadow (that flag only appears under Source/ThirdParty for gtest/opus/abseil/libwebrtc, and OptionsIOS.cmake explicitly passes -Wno-shadow). So this cannot cause a warning or failure on any supported configuration.

Step-by-step

  1. evaluate() is entered; line 118 binds outer state to, say, jsNumber(0) (Init) or a suspend index.
  2. The Init check on line 119 uses outer state — correct.
  3. executeModuleProgram runs and writes a new value into internalField(Field::State).
  4. Line 130's if-init declares a new state bound to the fresh field value and uses it in the condition. The outer state is shadowed but never referenced again, so behavior is unchanged.

Addressing the refutation

One verifier argued this shouldn't be reported at all because scoped if-init reuse is idiomatic in this file (e.g. execute() uses the same pattern) and JSC doesn't build with -Wshadow. That's fair — this is purely a readability suggestion, not a defect, and it should not block the PR. It's flagged only because the two reads are deliberately different snapshots of the same field within ~12 lines, and a distinct name makes that intent self-documenting.

Suggested fix

Rename the new variable to initialState (or move it into its own if-init so both declarations are equally scoped):

if (JSValue initialState = internalField(Field::State).get();
    initialState.isNumber() && initialState.asInt32AsAnyInt() == std::to_underlying(State::Init)) {
    if (JSValue error = evaluationError()) { ... }
}

Entirely optional.

if (JSValue error = evaluationError()) {
scope.throwException(globalObject, error);
return { };
}
}

ModuleProgramExecutable* executable = m_moduleProgramExecutable.get();
Expand Down
Loading