Add dynamic import support to the runtime#282
Merged
Conversation
- Parse and compile `import()` expressions - Plumb module loading through scopes and closures - Add bytecode VM support, docs, and tests
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds ECMAScript dynamic Changes
Sequence DiagramsequenceDiagram
participant Parser
participant Compiler
participant VM
participant ModuleLoader
participant Promise
Parser->>Compiler: emit TGocciaImportCallExpression
Compiler->>Compiler: compile specifier -> register B
Compiler->>VM: emit OP_DYNAMIC_IMPORT (dest=A, spec=B)
VM->>VM: create TGocciaPromiseValue
VM->>ModuleLoader: LoadModule(specifier, referrer)
ModuleLoader-->>VM: Module object / throws error
VM->>Promise: Resolve with Module.GetNamespaceObject / Reject with ErrorObject
VM-->>Caller: return Promise in register A
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
coderabbitai
Bot
added
documentation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/language/modules/dynamic-import.js`:
- Around line 3-71: Add a new test in the "dynamic import()" suite that verifies
the cross-module exported-function regression: create a helper module (e.g.,
helpers/caller.js) that exports a function callAndImport which does return
import("./dep.js") (where dep.js exports a known value), then in the test
require/import helpers/caller.js and invoke callAndImport() from this test file
and assert the resolved module's exported value; this proves resolution is done
relative to the caller module rather than the importer. Ensure the new helper
dep.js exports a distinctive value (e.g., exported constant) and the test name
describes the regression (e.g., "resolves relative to exporting module when
import() is inside exported function").
In `@units/Goccia.VM.pas`:
- Around line 5219-5220: The import resolution currently calls
DynImportPromise.Resolve(ImportModuleValue(RegisterToValue(FRegisters[B]).ToStringLiteral.Value))
which uses FCurrentModuleSourcePath and bypasses VM coercion; change to build
the request with VMRegisterToECMAStringFast(FRegisters[B]).Value and call an
ImportModuleValue overload that accepts a referrer path (use
Template.DebugInfo.SourceFile when present, otherwise fall back to
FCurrentModuleSourcePath), then pass that result into DynImportPromise.Resolve
so imports resolve against the closure’s source module and use the VM’s normal
ToString coercion path; add the new ImportModuleValue(referrerPath) overload to
accept and use the referrer.
- Around line 5221-5228: The catch-all Exception branch in the dyn import
handler is converting specialized exceptions into a generic Error when calling
DynImportPromise.Reject; update the except block in the dynamic import handling
so it mirrors the outer opcode loop’s typed mappings: detect TGocciaTypeError,
TGocciaReferenceError, TGocciaSyntaxError (and any other TGoccia* error classes
used elsewhere) and call DynImportPromise.Reject with the corresponding
specialized error object instead of CreateErrorObject(ERROR_NAME, E.Message),
only falling back to the generic CreateErrorObject for unknown Exception types;
reference the existing patterns used for EGocciaBytecodeThrow, TGocciaThrowValue
and CreateErrorObject to implement the same typed mapping.
🪄 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
Run ID: 55baa236-f909-49e3-8ad9-848501cf67d8
📒 Files selected for processing (16)
AGENTS.mddocs/bytecode-vm.mddocs/design-decisions.mddocs/language-restrictions.mdtests/language/modules/dynamic-import.jsunits/Goccia.AST.Expressions.pasunits/Goccia.Bytecode.OpCodeNames.pasunits/Goccia.Bytecode.pasunits/Goccia.Compiler.Expressions.pasunits/Goccia.Compiler.pasunits/Goccia.Error.Suggestions.pasunits/Goccia.Interpreter.pasunits/Goccia.Parser.pasunits/Goccia.Scope.pasunits/Goccia.VM.pasunits/Goccia.Values.FunctionValue.pas
…-plan # Conflicts: # AGENTS.md
…test coverage - Use Template.DebugInfo.SourceFile as referrer in OP_DYNAMIC_IMPORT so dynamic imports inside exported functions resolve relative to the defining module, not the calling module - Use VMRegisterToECMAStringFast for specifier coercion in the VM path - Preserve typed error classes (SyntaxError, TypeError, ReferenceError) in rejection instead of collapsing to plain Error - Add cross-module regression test with helper modules that prove resolution stays relative to the exporting module Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
Suite Timing
Measured on ubuntu-latest x64. |
Contributor
Benchmark Results364 benchmarks Interpreted: 🟢 196 improved · 🔴 117 regressed · 51 unchanged · avg +2.2% arraybuffer.js — Interp: 🟢 13, 🔴 1 · avg +10.4% · Bytecode: 🟢 10, 🔴 1, 3 unch. · avg +4.5%
arrays.js — Interp: 🟢 19 · avg +6.8% · Bytecode: 🟢 19 · avg +8.4%
async-await.js — Interp: 🟢 6 · avg +8.1% · Bytecode: 🟢 6 · avg +8.0%
base64.js — Interp: 🟢 8, 2 unch. · avg +8.8% · Bytecode: 🟢 9, 🔴 1 · avg +4.9%
classes.js — Interp: 🟢 31 · avg +7.6% · Bytecode: 🟢 7, 🔴 11, 13 unch. · avg -2.9%
closures.js — Interp: 🟢 11 · avg +4.9% · Bytecode: 🟢 2, 🔴 4, 5 unch. · avg -1.3%
collections.js — Interp: 🟢 11, 1 unch. · avg +3.9% · Bytecode: 🟢 8, 🔴 2, 2 unch. · avg +2.7%
destructuring.js — Interp: 🟢 19, 🔴 1, 2 unch. · avg +4.2% · Bytecode: 🟢 1, 🔴 17, 4 unch. · avg -4.9%
fibonacci.js — Interp: 🟢 5, 🔴 1, 2 unch. · avg +2.5% · Bytecode: 🟢 5, 🔴 2, 1 unch. · avg +2.7%
float16array.js — Interp: 🟢 16, 🔴 6, 10 unch. · avg +6.8% · Bytecode: 🟢 8, 🔴 18, 6 unch. · avg -2.1%
for-of.js — Interp: 🔴 7 · avg -4.9% · Bytecode: 🔴 4, 3 unch. · avg -2.7%
helpers/bench-module.js — Interp: 0 · Bytecode: 0
iterators.js — Interp: 🟢 1, 🔴 39, 2 unch. · avg -6.2% · Bytecode: 🟢 12, 🔴 19, 11 unch. · avg -0.8%
json.js — Interp: 🟢 9, 🔴 6, 5 unch. · avg +0.7% · Bytecode: 🟢 7, 🔴 1, 12 unch. · avg +1.6%
jsx.jsx — Interp: 🟢 15, 6 unch. · avg +2.0% · Bytecode: 🔴 12, 9 unch. · avg -1.7%
modules.js — Interp: 🟢 1, 🔴 7, 1 unch. · avg -1.8% · Bytecode: 🟢 8, 1 unch. · avg +10.5%
numbers.js — Interp: 🟢 7, 🔴 1, 3 unch. · avg +3.8% · Bytecode: 🟢 7, 🔴 2, 2 unch. · avg +2.5%
objects.js — Interp: 🟢 1, 🔴 5, 1 unch. · avg -3.1% · Bytecode: 🟢 2, 🔴 2, 3 unch. · avg +0.7%
promises.js — Interp: 🔴 11, 1 unch. · avg -4.6% · Bytecode: 🟢 8, 4 unch. · avg +2.9%
regexp.js — Interp: 🟢 5, 🔴 3, 3 unch. · avg -0.0% · Bytecode: 🟢 2, 🔴 2, 7 unch. · avg -0.9%
strings.js — Interp: 🟢 4, 🔴 7, 8 unch. · avg -0.7% · Bytecode: 🟢 8, 🔴 2, 9 unch. · avg +1.4%
typed-arrays.js — Interp: 🟢 9, 🔴 11, 2 unch. · avg +1.3% · Bytecode: 🟢 7, 🔴 8, 7 unch. · avg -0.3%
uint8array-encoding.js — Interp: 🟢 5, 🔴 11, 2 unch. · avg -0.3% · Bytecode: 🟢 2, 🔴 8, 8 unch. · avg -3.5%
Measured on ubuntu-latest x64. Benchmark ranges compare cached main-branch min/max ops/sec with the PR run; overlapping ranges are treated as unchanged noise. Percentage deltas are secondary context. |
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
import(specifier)support so dynamic imports now resolve to a Promise and can be used inside functions, conditionals, and callbacks.Testing
./build.pas testrunner && ./build/TestRunner teststests/language/modules/dynamic-import.jscase through./build/TestRunner tests/language/modules/