Add --source-type CLI flag and config to load entry as a module#480
Merged
Conversation
Threads a new TGocciaSourceType (stScript, stModule) enum through the engine and CLI options layer. When source-type=module, Engine.Execute runs the entry program in a fresh skModule scope with this=undefined, matching the semantics imported modules already get from the module loader. EvaluateModuleBody changes from procedure to function so the test runner can still observe the runTests(...) result, and the bytecode executor exposes RunModuleInScope so script loader, test runner, and benchmark runner bytecode paths honour the flag too. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds SourceType to the embedding guide's "Preprocessors and Compatibility" table alongside Preprocessors/Compatibility/StrictTypes and includes "source-type" in the goccia.json example so the option appears next to the other engine flags it sits beside in CLI.Options. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a new engine source mode ( Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI Parser
participant Resolver as ResolveSourceTypeOption
participant Engine as TGocciaEngine
participant Executor as TGocciaExecutor
participant Scope as TGocciaScope
CLI->>Resolver: parse --source-type / per-file config / root config
Resolver->>Resolver: validate (module|script) or warn + fallback
Resolver->>Engine: set Engine.SourceType
alt Script mode
Engine->>Executor: ExecuteProgram in GlobalScope
Executor->>Executor: run top-level statements (script semantics)
Executor-->>Engine: completion
else Module mode
Engine->>Scope: create child scope (skModule)
Scope->>Scope: set ThisValue := undefined
Engine->>Executor: EvaluateModuleBody with module scope
Executor->>Executor: evaluate statements, capture return value
Executor-->>Engine: return TGocciaValue
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Review rate limit: 2/5 reviews remaining, refill in 31 minutes and 28 seconds. Comment |
Contributor
Suite Timing
Measured on ubuntu-latest x64. |
Contributor
Benchmark Results407 benchmarks Interpreted: 🟢 12 improved · 🔴 323 regressed · 72 unchanged · avg -5.6% arraybuffer.js — Interp: 🟢 1, 🔴 11, 2 unch. · avg -5.1% · Bytecode: 🟢 5, 🔴 1, 8 unch. · avg +1.9%
arrays.js — Interp: 🔴 19 · avg -7.5% · Bytecode: 🟢 7, 12 unch. · avg +3.0%
async-await.js — Interp: 🔴 5, 1 unch. · avg -9.2% · Bytecode: 🟢 3, 3 unch. · avg +0.1%
async-generators.js — Interp: 🔴 1, 1 unch. · avg -8.5% · Bytecode: 2 unch. · avg +2.7%
base64.js — Interp: 🔴 10 · avg -8.5% · Bytecode: 🟢 2, 8 unch. · avg +2.1%
classes.js — Interp: 🔴 20, 11 unch. · avg -4.6% · Bytecode: 🟢 7, 🔴 2, 22 unch. · avg +0.9%
closures.js — Interp: 🔴 11 · avg -7.0% · Bytecode: 11 unch. · avg +1.4%
collections.js — Interp: 🔴 11, 1 unch. · avg -8.7% · Bytecode: 🟢 2, 🔴 1, 9 unch. · avg +1.1%
csv.js — Interp: 🔴 12, 1 unch. · avg -7.9% · Bytecode: 🟢 3, 🔴 3, 7 unch. · avg -0.3%
destructuring.js — Interp: 🔴 18, 4 unch. · avg -6.9% · Bytecode: 🟢 4, 18 unch. · avg +1.6%
fibonacci.js — Interp: 🔴 8 · avg -7.6% · Bytecode: 🔴 1, 7 unch. · avg -0.6%
float16array.js — Interp: 🟢 5, 🔴 21, 6 unch. · avg -2.1% · Bytecode: 🟢 4, 🔴 8, 20 unch. · avg -1.1%
for-of.js — Interp: 🔴 1, 6 unch. · avg -1.5% · Bytecode: 🟢 1, 🔴 2, 4 unch. · avg -0.5%
generators.js — Interp: 🔴 4 · avg -6.1% · Bytecode: 4 unch. · avg -1.5%
iterators.js — Interp: 🔴 34, 8 unch. · avg -5.7% · Bytecode: 🟢 2, 🔴 13, 27 unch. · avg -1.9%
json.js — Interp: 🔴 20 · avg -10.5% · Bytecode: 🟢 13, 7 unch. · avg +4.3%
jsx.jsx — Interp: 🔴 12, 9 unch. · avg -3.7% · Bytecode: 🔴 1, 20 unch. · avg -0.7%
modules.js — Interp: 🔴 8, 1 unch. · avg -6.0% · Bytecode: 🟢 1, 🔴 5, 3 unch. · avg -1.4%
numbers.js — Interp: 🔴 11 · avg -8.1% · Bytecode: 🟢 3, 🔴 6, 2 unch. · avg -1.0%
objects.js — Interp: 🔴 3, 4 unch. · avg -2.6% · Bytecode: 7 unch. · avg +0.6%
promises.js — Interp: 🔴 10, 2 unch. · avg -4.5% · Bytecode: 🟢 4, 8 unch. · avg +1.6%
regexp.js — Interp: 🔴 11 · avg -7.7% · Bytecode: 🟢 1, 10 unch. · avg +0.2%
strings.js — Interp: 🔴 14, 5 unch. · avg -5.2% · Bytecode: 🟢 3, 🔴 1, 15 unch. · avg +0.4%
tsv.js — Interp: 🔴 8, 1 unch. · avg -8.9% · Bytecode: 🟢 5, 4 unch. · avg +3.0%
typed-arrays.js — Interp: 🔴 18, 4 unch. · avg -10.0% · Bytecode: 🟢 8, 🔴 3, 11 unch. · avg +10.0%
uint8array-encoding.js — Interp: 🔴 14, 4 unch. · avg -6.2% · Bytecode: 🟢 9, 🔴 4, 5 unch. · avg +21.3%
weak-collections.js — Interp: 🟢 6, 🔴 8, 1 unch. · avg +12.6% · Bytecode: 🟢 4, 🔴 8, 3 unch. · avg -14.2%
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. |
coderabbitai
Bot
added
new feature
![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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/embedding.md`:
- Around line 436-447: Update the docs to explicitly state that when
TGocciaEngine.SourceType is set to stModule, each call to TGocciaEngine.Execute
runs in a fresh module scope (skModule) so top-level bindings are not preserved
across repeated Execute calls; reference the earlier "long-lived engine" example
and clarify that behavior differs from default script mode where the engine
reuses the top-level scope, and suggest using script mode (stScript) or an
explicit persistence mechanism if callers expect bindings to persist between
Execute invocations.
In `@source/app/Goccia.CLI.Application.pas`:
- Around line 443-448: The current block that uses FindConfigEntry to parse
'source-type' silently treats any non-'module' value as 'script'; change it to
explicitly validate ValueStr (case-insensitive) against allowed enums ('module'
and 'script') and reject unknown values instead of defaulting to script—use the
same enum-parsing/validation behavior as the CLI/root-config code: if ValueStr =
'module' then return Goccia.Engine.stModule, if ValueStr = 'script' then return
Goccia.Engine.stScript, otherwise raise an error or propagate a configuration
parse failure (with a clear message) so invalid per-file source-type entries are
not silently accepted.
🪄 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: aa8220d7-5fa5-4e72-b2fb-888e29965c0d
📒 Files selected for processing (16)
AGENTS.mddocs/build-system.mddocs/embedding.mdsource/app/Goccia.CLI.Application.passource/app/GocciaBenchmarkRunner.dprsource/app/GocciaScriptLoader.dprsource/app/GocciaTestRunner.dprsource/shared/CLI.Options.passource/units/Goccia.Engine.Backend.passource/units/Goccia.Engine.passource/units/Goccia.Executor.passource/units/Goccia.Interpreter.passource/units/Goccia.Modules.Loader.pastests/language/source-type/goccia.jsontests/language/source-type/import-meta.jstests/language/source-type/this-binding.js
Two fixes: * docs/embedding.md: spell out that stModule allocates a fresh skModule scope per Execute call, so top-level let/const/class declarations do not persist across Execute invocations the way they do in default stScript mode (the long-lived engine pattern). Suggest staying on stScript or going through the global scope (RegisterGlobal / DefineLexicalBinding) when persistence is wanted. * source/app/Goccia.CLI.Application.pas: per-file goccia.json values for "source-type" are now validated case-insensitively. Unknown values emit a stderr warning and fall back to stScript instead of silently flipping to script. CLI flag and root-config values were already validated by TGocciaEnumOption.Apply, so this just plugs the per-file gap without changing the existing fail-fast behavior on the other layers. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
coderabbitai
Bot
added
the
documentation
4 tasks
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
--source-type=script|module(and matchinggoccia.json"source-type") on every CLI that callsAddEngineOptions:GocciaScriptLoader,GocciaTestRunner,GocciaBenchmarkRunner,GocciaBundler,GocciaREPL. Default isscript, preserving existing behavior.TGocciaSourceType = (stScript, stModule)onTGocciaEngine. When set tostModule,Executeruns the entry program in a freshskModulescope withthis = undefined, mirroring the semantics imported modules already get fromTGocciaModuleLoader(ES2026 §16.2.1.6.4).import.meta.urlresolves and top-levelthisno longer aliasesglobalThis.EvaluateModuleBodychanges from procedure to function returningTGocciaValueso the test runner can still observe therunTests(...)completion value when the entry runs as a module. Existing module loader call sites discard the return.TGocciaBytecodeExecutor.RunModuleInScopelets non-Engine.Executecallers (the script loader's bytecode-from-source/file paths plus the test and benchmark runners) honor the flag without losing their per-phase timing optimizations.Test plan
./build.pasclean dev build of all five binaries./build/GocciaTestRunner tests --asi --unsafe-ffi --no-progress(interpreted) — 8233/8279 (5 pre-existing FFI fixture failures, unrelated)./build/GocciaTestRunner tests --asi --unsafe-ffi --mode=bytecode --no-progress— 8274/8279 (same 5 pre-existing failures)build/Goccia.*.Test)./format.pas --checkpassestests/language/source-type/directory with a per-directorygoccia.jsonand assertions on top-levelthis+import.meta— 6/6 tests pass in both interpreted and bytecode mode--helplisting all five binaries🤖 Generated with Claude Code