fix(modules): preserve live import bindings across module graphs#999
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
📝 WalkthroughWalkthroughModule imports now use indirect live bindings resolved against target exports at read time. Module linking creates these bindings and namespace bindings, while scope read paths and GC tracking support them. Tests cover transitive imports, re-exports, namespace reads, hoisted functions, and circular-import behavior. ChangesLive import bindings
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ImportDeclaration
participant ModuleLoader
participant ImportingScope
participant TargetModule
ImportDeclaration->>ModuleLoader: Request module and import specifiers
ModuleLoader->>TargetModule: Instantiate and link requested module
ModuleLoader->>ImportingScope: CreateImportBinding(localName, targetModule, exportName)
ModuleLoader->>TargetModule: Evaluate linked dependencies and module body
ImportingScope->>TargetModule: Resolve exported value on each read
TargetModule-->>ImportingScope: Current value or TDZ/unresolved state
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
Contributor
Web Tooling Benchmark
18 pinned Web Tooling workloads; 18 workloads produced at least one Goccia sample. Raw results from 1 sample per workload; full stdout/stderr for failures and min/max/CV stay in the |
Contributor
Benchmark Results439 benchmarks · PR vs same-runner Interpreted: 🟢 21 improved · 🔴 60 regressed · 358 unchanged · avg -1.0% Typical per-run noise (median variance): interpreted ±2.4%, bytecode ±1.6%. Deltas within noise overlap and read as unchanged. arraybuffer.js — Interp: 🔴 2, 12 unch. · avg -1.0% · Bytecode: 🔴 1, 13 unch. · avg -0.3%
arrays.js — Interp: 🔴 1, 18 unch. · avg -2.0% · Bytecode: 🔴 3, 16 unch. · avg -0.0%
async-await.js — Interp: 6 unch. · avg -0.0% · Bytecode: 🔴 1, 5 unch. · avg -3.1%
async-generators.js — Interp: 2 unch. · avg -2.5% · Bytecode: 2 unch. · avg -0.4%
atomics.js — Interp: 🔴 1, 5 unch. · avg +2.6% · Bytecode: 6 unch. · avg +0.6%
base64.js — Interp: 🔴 3, 7 unch. · avg -1.7% · Bytecode: 10 unch. · avg -0.7%
classes.js — Interp: 🟢 1, 🔴 4, 26 unch. · avg -0.7% · Bytecode: 🟢 2, 🔴 1, 28 unch. · avg +1.1%
closures.js — Interp: 🔴 1, 10 unch. · avg -2.2% · Bytecode: 🔴 1, 10 unch. · avg -0.1%
collections.js — Interp: 🟢 1, 🔴 1, 10 unch. · avg -0.1% · Bytecode: 🟢 1, 🔴 1, 10 unch. · avg -1.6%
csv.js — Interp: 🔴 4, 9 unch. · avg -2.7% · Bytecode: 🔴 1, 12 unch. · avg -2.1%
destructuring.js — Interp: 🔴 5, 17 unch. · avg -1.7% · Bytecode: 🔴 3, 19 unch. · avg -2.2%
fibonacci.js — Interp: 8 unch. · avg -1.6% · Bytecode: 8 unch. · avg -0.1%
float16array.js — Interp: 🟢 1, 🔴 5, 26 unch. · avg -1.6% · Bytecode: 🟢 3, 🔴 3, 26 unch. · avg -0.1%
for-in/for-in.js — Interp: 3 unch. · avg +2.6% · Bytecode: 3 unch. · avg +5.9%
for-of.js — Interp: 🟢 1, 6 unch. · avg -0.2% · Bytecode: 7 unch. · avg +0.4%
generators.js — Interp: 🔴 2, 2 unch. · avg -3.5% · Bytecode: 🔴 2, 2 unch. · avg -2.0%
intl.js — Interp: 🔴 1, 5 unch. · avg +1.8% · Bytecode: 6 unch. · avg -0.1%
iterators.js — Interp: 🟢 1, 41 unch. · avg -0.3% · Bytecode: 🟢 3, 🔴 11, 28 unch. · avg -2.3%
json.js — Interp: 🔴 1, 22 unch. · avg -2.3% · Bytecode: 🟢 2, 🔴 3, 18 unch. · avg -0.2%
jsx.jsx — Interp: 🟢 1, 🔴 1, 19 unch. · avg +0.3% · Bytecode: 🟢 4, 17 unch. · avg +0.4%
modules.js — Interp: 🔴 8, 1 unch. · avg -27.4% · Bytecode: 🔴 1, 8 unch. · avg -1.6%
numbers.js — Interp: 🔴 1, 11 unch. · avg -2.3% · Bytecode: 🔴 3, 9 unch. · avg +1.2%
objects.js — Interp: 7 unch. · avg -1.7% · Bytecode: 🔴 1, 6 unch. · avg -0.1%
promises.js — Interp: 12 unch. · avg -0.5% · Bytecode: 🟢 2, 🔴 2, 8 unch. · avg +0.9%
property-access.js — Interp: 5 unch. · avg +0.2% · Bytecode: 5 unch. · avg +3.5%
regexp.js — Interp: 🔴 1, 12 unch. · avg -1.3% · Bytecode: 🔴 1, 12 unch. · avg -0.9%
strings.js — Interp: 🔴 3, 16 unch. · avg -2.8% · Bytecode: 🔴 6, 13 unch. · avg -2.2%
temporal.js — Interp: 6 unch. · avg -1.1% · Bytecode: 6 unch. · avg -1.7%
tsv.js — Interp: 🔴 2, 7 unch. · avg -2.9% · Bytecode: 9 unch. · avg -0.3%
typed-arrays.js — Interp: 🟢 9, 🔴 2, 11 unch. · avg +11.1% · Bytecode: 🟢 2, 🔴 8, 12 unch. · avg -8.5%
uint8array-encoding.js — Interp: 🟢 6, 12 unch. · avg +22.4% · Bytecode: 🟢 7, 🔴 3, 8 unch. · avg +21.6%
weak-collections.js — Interp: 🔴 11, 4 unch. · avg -25.5% · Bytecode: 🟢 1, 🔴 3, 11 unch. · avg -8.0%
Deterministic profile diffDeterministic profile diff: no significant changes. Measured on ubuntu-latest x64. Each PR run also builds the |
Contributor
Suite TimingTest Runner (interpreted: 11,551 passed; bytecode: 11,551 passed)
MemoryGC rows aggregate the main thread plus all worker thread-local GCs. Test runner worker shutdown frees thread-local heaps in bulk; that shutdown reclamation is not counted as GC collections or collected objects.
Benchmarks (interpreted: 439; bytecode: 439)
MemoryGC rows aggregate the main thread plus all worker thread-local GCs. Benchmark runner performs explicit between-file collections, so collection and collected-object counts can be much higher than the test runner.
Boot
Empty-script ( Measured on ubuntu-latest x64. |
Contributor
JetStream 3 Performance Barometer
Geomean reference ratio: QuickJS 26.21×; Node.js 298.60×. 1.00× means aligned; values above 1.00× mean Goccia was proportionally slower after normalizing JetStream’s higher-is-better score. This is a directional barometer across runtimes with different goals, not a product ranking. Raw samples and failure details remain in the |
Contributor
AWFY Results
Geomean Ratios
14 pinned AWFY benchmarks. Medians from 5 interleaved samples per engine; raw JSON includes min/max/CV and is attached as the |
Contributor
test262 Conformance
Areas closest to 100%
Per-test deltas (+0 / -0 / timeout +0 / -1)Resolved timeouts (1):
Steady-state failures and timeouts are non-blocking; PASS → non-timeout failure transitions fail the conformance gate. Measured on ubuntu-latest x64, bytecode mode. Areas grouped by the first two test262 path components; minimum 25 attempted tests, areas already at 100% excluded. Δ vs main compares against the most recent cached |
frostney
marked this pull request as ready for review
July 22, 2026 19:08
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
coderabbitai
Bot
added
bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
source/units/Goccia.Modules.Loader.pas (1)
1193-1231: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftLink parent imports before evaluating cyclic dependencies.
LoadModule(...)evaluatesRequestedModulebefore lines 1218–1231 install this module’s imports. In a cycle, that dependency can call a hoisted function exported by this module; reads of this module’s named or namespace imports then see the predeclared TDZ/undefined placeholder rather than the target’s live binding. Split graph instantiation/linking from evaluation so all imports in the cycle are installed before any module body executes.Also applies to: 1713-1713
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@source/units/Goccia.Modules.Loader.pas` around lines 1193 - 1231, Update LinkAndEvaluateRequestedModulesInSourceOrder and the corresponding module-loading path so LoadModule does not evaluate RequestedModule before this module’s named and namespace import bindings are installed. Separate dependency graph instantiation/linking from module-body evaluation, recursively link the cycle’s parent imports first, then evaluate modules while preserving live bindings and existing source-order semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@source/units/Goccia.Modules.Loader.pas`:
- Around line 1193-1231: Update LinkAndEvaluateRequestedModulesInSourceOrder and
the corresponding module-loading path so LoadModule does not evaluate
RequestedModule before this module’s named and namespace import bindings are
installed. Separate dependency graph instantiation/linking from module-body
evaluation, recursively link the cycle’s parent imports first, then evaluate
modules while preserving live bindings and existing source-order semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f96fd107-0158-460f-8402-24ea01a429f4
📒 Files selected for processing (16)
CONTEXT.mddocs/architecture.mddocs/core-patterns.mddocs/interpreter.mdsource/units/Goccia.AST.Statements.passource/units/Goccia.Modules.Loader.passource/units/Goccia.Modules.passource/units/Goccia.Scope.pastests/language/modules/hoisted-function-import-capture/helpers/live-bindings-cycle-a.jstests/language/modules/hoisted-function-import-capture/helpers/live-bindings-cycle-b.jstests/language/modules/hoisted-function-import-capture/helpers/live-bindings-leaf.jstests/language/modules/hoisted-function-import-capture/helpers/live-bindings-middle.jstests/language/modules/hoisted-function-import-capture/helpers/live-bindings-outer.jstests/language/modules/hoisted-function-import-capture/helpers/live-bindings-re-export.jstests/language/modules/hoisted-function-import-capture/helpers/live-bindings-star-re-export.jstests/language/modules/hoisted-function-import-capture/transitive-live-bindings.js
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@source/units/Goccia.Modules.Loader.pas`:
- Around line 1267-1276: Update the failure cleanup in the module-loading flow
around LoadState and AModule so failed modules remain alive until all dependent
TGocciaModuleImportBinding references are detached. Ensure TryGetValue and
MarkReferences cannot observe a freed AModule, then dispose the failed module
only after detachment or defer disposal while preserving existing
successful-load cleanup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f49531f2-1f32-4bf5-9cb0-cdecb6c9f963
📒 Files selected for processing (4)
fixtures/modules/live-bindings-link-child.jsfixtures/modules/live-bindings-link-parent.jssource/units/Goccia.Modules.Loader.pastests/language/modules/hoisted-function-import-capture/transitive-live-bindings.js
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
source/units/Goccia.Modules.ContentProvider.Test.pas (2)
531-539: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winException check is too broad to prove the "failed cycle" throw actually fired.
on E: Exception do RaisedExpected := True;matches any exception, so an unrelated failure (e.g. resolution error, missing global) would make the test pass without exercising the intended cyclic-failure path. Other tests in this file (e.g.TestEngineReportsJSONLModuleLineNumbers, lines 617-628) assert on message content for this reason.♻️ Proposed tightening
try ModuleLoader.LoadModule(FAILED_PATH, ENTRY_PATH); except on E: Exception do - RaisedExpected := True; + begin + RaisedExpected := Pos('failed cycle', E.Message) > 0; + if not RaisedExpected then + Fail('Unexpected exception: ' + E.Message); + end; end;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@source/units/Goccia.Modules.ContentProvider.Test.pas` around lines 531 - 539, Update the exception handling in the cyclic parent module test around ModuleLoader.LoadModule to assert that the caught exception contains the expected cyclic-evaluation failure message, rather than treating every Exception as success. Preserve the failure assertion when no matching exception is raised, and follow the message-content assertion pattern used by TestEngineReportsJSONLModuleLineNumbers.
541-545: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInvoke
readImportedValue()and assert'still live'
The export is only checked for existence and type here. Call it withTGocciaFunctionBase(ExportValue).CallNoArgs(TGocciaUndefinedLiteralValue.UndefinedValue)and assert the returned string so this case exercises the live-binding path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@source/units/Goccia.Modules.ContentProvider.Test.pas` around lines 541 - 545, Extend the test around readImportedValue in the SurvivorModule flow: after confirming ExportValue is a TGocciaFunctionBase, invoke it via CallNoArgs with TGocciaUndefinedLiteralValue.UndefinedValue and assert that the returned value is the string “still live”. Keep the existing export existence, type, and MarkExportReferences checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@source/units/Goccia.Modules.ContentProvider.Test.pas`:
- Around line 531-539: Update the exception handling in the cyclic parent module
test around ModuleLoader.LoadModule to assert that the caught exception contains
the expected cyclic-evaluation failure message, rather than treating every
Exception as success. Preserve the failure assertion when no matching exception
is raised, and follow the message-content assertion pattern used by
TestEngineReportsJSONLModuleLineNumbers.
- Around line 541-545: Extend the test around readImportedValue in the
SurvivorModule flow: after confirming ExportValue is a TGocciaFunctionBase,
invoke it via CallNoArgs with TGocciaUndefinedLiteralValue.UndefinedValue and
assert that the returned value is the string “still live”. Keep the existing
export existence, type, and MarkExportReferences checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5e9d697a-2f9a-4776-8e50-5a4e326177d2
📒 Files selected for processing (2)
source/units/Goccia.Modules.ContentProvider.Test.passource/units/Goccia.Modules.Loader.pas
🚧 Files skipped from review as they are similar to previous changes (1)
- source/units/Goccia.Modules.Loader.pas
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Testing
Validation performed: