fix(engine): map ShadowRealm eval instantiation errors to SyntaxError#816
Merged
Conversation
ShadowRealm.prototype.evaluate mapped every error from EvaluateEvalProgram to
a caller-realm TypeError, so an EvalDeclarationInstantiation conflict such as
`r.evaluate("let x; let x;")` surfaced as TypeError instead of the SyntaxError
required by proposal-shadowrealm 3.1.3: static early errors are SyntaxErrors,
only runtime abrupt completions are wrapped as TypeError.
Split EvaluateEvalProgram into PrepareEvalProgram (the static early-error
passes plus EvalDeclarationInstantiation) and RunEvalProgramBody (body
evaluation). ShadowRealm.evaluate now runs the two phases separately and maps
a phase-1 TGocciaSyntaxError to a caller-realm SyntaxError, while still
wrapping phase-2 runtime abrupt completions (including a nested-eval runtime
SyntaxError) as TypeError; a CanDeclareGlobal* TypeError from instantiation
stays a TypeError. EvaluateEvalProgram chains the two phases, so the VM
direct-eval and $262.evalScript callers are unchanged.
built-ins/ShadowRealm test262 stays 64/64 in interpreter and bytecode modes;
the full repo JS suite stays green in both modes.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
📝 WalkthroughWalkthroughShadowRealm evaluate now separates preparation from body execution, with new control-flow validation and updated handling for early syntax errors versus runtime abrupt completions. Tests add coverage for duplicate lexical bindings and illegal top-level control flow. ChangesShadowRealm eval flow split
Sequence Diagram(s)sequenceDiagram
participant TGocciaShadowRealmHost.Evaluate
participant PrepareEvalProgram
participant RunEvalProgramBody
participant Caller realm
TGocciaShadowRealmHost.Evaluate->>PrepareEvalProgram: prepare eval program
alt TGocciaSyntaxError
TGocciaShadowRealmHost.Evaluate->>Caller realm: ThrowSyntaxError(EarlyErrorMessage)
else abrupt completion
TGocciaShadowRealmHost.Evaluate->>Caller realm: ThrowTypeError('ShadowRealm evaluate threw')
else prepared successfully
TGocciaShadowRealmHost.Evaluate->>RunEvalProgramBody: run prepared body
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
Contributor
Suite TimingTest Runner (interpreted: 10,946 passed; bytecode: 10,946 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: 430; bytecode: 430)
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.
Measured on ubuntu-latest x64. |
Contributor
Benchmark Results430 benchmarks Interpreted: 🟢 1 improved · 🔴 208 regressed · 221 unchanged · avg -9.4% arraybuffer.js — Interp: 🔴 8, 6 unch. · avg -10.1% · Bytecode: 🟢 7, 7 unch. · avg +8.4%
arrays.js — Interp: 🔴 7, 12 unch. · avg -8.8% · Bytecode: 🟢 11, 8 unch. · avg +9.3%
async-await.js — Interp: 🔴 4, 2 unch. · avg -11.7% · Bytecode: 🟢 4, 2 unch. · avg +13.9%
async-generators.js — Interp: 2 unch. · avg -12.3% · Bytecode: 2 unch. · avg +10.8%
atomics.js — Interp: 🔴 4, 2 unch. · avg -12.7% · Bytecode: 🟢 2, 4 unch. · avg +7.8%
base64.js — Interp: 🔴 7, 3 unch. · avg -10.5% · Bytecode: 🟢 9, 1 unch. · avg +11.8%
classes.js — Interp: 🔴 18, 13 unch. · avg -7.9% · Bytecode: 🟢 21, 10 unch. · avg +9.2%
closures.js — Interp: 🔴 7, 4 unch. · avg -10.2% · Bytecode: 🟢 7, 4 unch. · avg +11.6%
collections.js — Interp: 🔴 7, 5 unch. · avg -11.1% · Bytecode: 🟢 7, 5 unch. · avg +13.1%
csv.js — Interp: 🔴 9, 4 unch. · avg -14.3% · Bytecode: 🟢 8, 5 unch. · avg +10.3%
destructuring.js — Interp: 🔴 14, 8 unch. · avg -11.5% · Bytecode: 🟢 13, 9 unch. · avg +7.5%
fibonacci.js — Interp: 🔴 5, 3 unch. · avg -12.5% · Bytecode: 🟢 8 · avg +16.4%
float16array.js — Interp: 🔴 22, 10 unch. · avg -9.5% · Bytecode: 🟢 21, 11 unch. · avg +10.4%
for-of.js — Interp: 🔴 3, 4 unch. · avg -4.3% · Bytecode: 🟢 5, 2 unch. · avg +16.7%
generators.js — Interp: 🟢 1, 3 unch. · avg -0.6% · Bytecode: 🟢 3, 1 unch. · avg +8.4%
intl.js — Interp: 🔴 5, 1 unch. · avg -11.7% · Bytecode: 🟢 6 · avg +16.9%
iterators.js — Interp: 🔴 10, 32 unch. · avg -8.3% · Bytecode: 🟢 11, 🔴 3, 28 unch. · avg +4.3%
json.js — Interp: 🔴 12, 8 unch. · avg -10.2% · Bytecode: 🟢 17, 3 unch. · avg +12.4%
jsx.jsx — Interp: 🔴 8, 13 unch. · avg -10.5% · Bytecode: 🟢 10, 11 unch. · avg +11.5%
modules.js — Interp: 🔴 2, 7 unch. · avg -7.8% · Bytecode: 🔴 1, 8 unch. · avg -2.9%
numbers.js — Interp: 🔴 3, 8 unch. · avg -8.9% · Bytecode: 🟢 8, 3 unch. · avg +14.0%
objects.js — Interp: 🔴 1, 6 unch. · avg -8.3% · Bytecode: 🟢 5, 2 unch. · avg +11.4%
promises.js — Interp: 🔴 3, 9 unch. · avg -10.2% · Bytecode: 🟢 3, 9 unch. · avg +14.5%
property-access.js — Interp: 5 unch. · avg +0.5% · Bytecode: 🟢 2, 3 unch. · avg +11.0%
regexp.js — Interp: 🔴 2, 9 unch. · avg -7.8% · Bytecode: 🟢 4, 7 unch. · avg +9.1%
strings.js — Interp: 🔴 4, 15 unch. · avg -5.8% · Bytecode: 🟢 12, 7 unch. · avg +14.3%
temporal.js — Interp: 🔴 2, 4 unch. · avg -13.6% · Bytecode: 🟢 2, 4 unch. · avg +17.1%
tsv.js — Interp: 🔴 6, 3 unch. · avg -10.6% · Bytecode: 🟢 6, 3 unch. · avg +10.2%
typed-arrays.js — Interp: 🔴 12, 10 unch. · avg -3.9% · Bytecode: 🟢 13, 9 unch. · avg +17.8%
uint8array-encoding.js — Interp: 🔴 11, 7 unch. · avg -8.4% · Bytecode: 🟢 7, 🔴 1, 10 unch. · avg +5.2%
weak-collections.js — Interp: 🔴 12, 3 unch. · avg -19.1% · Bytecode: 🟢 9, 🔴 1, 5 unch. · avg +6.0%
Deterministic profile diffDeterministic profile diff: no significant changes. 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. |
Contributor
test262 Conformance
Areas closest to 100%
Per-test deltas (+1 / -0)Newly passing (1):
Steady-state failures are non-blocking; regressions vs the cached main baseline (lower total pass count, or any PASS → non-PASS transition) 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 |
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.Evaluator.pas (1)
2942-2951: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftMove illegal top-level control-flow errors into the prepare phase.
RunEvalProgramBodystill raisesTGocciaSyntaxErrorfor top-levelreturn/break/continue. Since ShadowRealm now treats all phase-2TGocciaErrors as runtime abrupt completions, these syntax errors become caller-realmTypeErrors instead ofSyntaxErrors. Add/pre-run this validation inPrepareEvalProgramso these cases stay in phase 1.🤖 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.Evaluator.pas` around lines 2942 - 2951, Move the illegal top-level control-flow validation out of RunEvalProgramBody into the phase-1 preparation path so these errors remain SyntaxError/parse-time failures. Update PrepareEvalProgram to detect cfkReturn, cfkBreak, and cfkContinue before execution starts, using the existing TGocciaSyntaxError/ThrowSyntaxError paths, and keep RunEvalProgramBody limited to executing already-validated control flow cases.
🤖 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.Evaluator.pas`:
- Around line 2942-2951: Move the illegal top-level control-flow validation out
of RunEvalProgramBody into the phase-1 preparation path so these errors remain
SyntaxError/parse-time failures. Update PrepareEvalProgram to detect cfkReturn,
cfkBreak, and cfkContinue before execution starts, using the existing
TGocciaSyntaxError/ThrowSyntaxError paths, and keep RunEvalProgramBody limited
to executing already-validated control flow cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e5341ce5-dc97-418f-aade-9ce15296c1e2
📒 Files selected for processing (3)
source/units/Goccia.Builtins.GlobalShadowRealm.passource/units/Goccia.Evaluator.pastests/built-ins/ShadowRealm/evaluate.js
Top-level `return`, and unlabeled `break`/`continue` with no enclosing iteration or switch, are Script early errors (SyntaxErrors) — eval code is never a function body, loop, or switch. The eval-phase split surfaced these only at runtime, via control-flow propagation in RunEvalProgramBody, which ShadowRealm.prototype.evaluate wraps as a caller-realm TypeError. Add ValidateEvalControlFlow to PrepareEvalProgram (the pre-execution phase), raising a TGocciaSyntaxError directly (as the other eval early-error passes do) so the error is classified as a static SyntaxError, not a runtime abrupt completion. The walk visits only statements, so it stops at function/class boundaries and never flags a return/break/continue inside a nested function; labeled break/continue are left to the parser, which validates their targets. RunEvalProgramBody's control-flow arms remain as a defensive backstop. built-ins/ShadowRealm test262 stays 64/64 in both modes; language/eval-code is unchanged (the validator now runs for direct eval too); the full repo JS suite stays green in both modes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ror' into claude/shadowrealm-eval-syntaxerror
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
ShadowRealm.prototype.evaluatemapped every error fromEvaluateEvalProgramto a caller-realm TypeError, so anEvalDeclarationInstantiationconflict such asr.evaluate("let x; let x;")surfaced as a TypeError instead of the SyntaxError required by proposal-shadowrealm §3.1.3 — static early errors are SyntaxErrors (step 2); only runtime abrupt completions are wrapped as TypeError (step 15).EvaluateEvalProgramintoPrepareEvalProgram(the static early-error passes plusEvalDeclarationInstantiation, returning the prepared eval context) andRunEvalProgramBody(body evaluation).EvaluateEvalProgramnow just chains the two, so the VM direct-eval and$262.evalScriptcallers are unchanged.ShadowRealm.evaluateruns the two phases separately and distinguishes their errors: a phase-1TGocciaSyntaxError→ caller-realm SyntaxError; everything else — a phase-1CanDeclareGlobal*TypeError, plus all phase-2 runtime abrupt completions (including a nested-eval runtime SyntaxError) → caller-realm TypeError.return, or an unlabeledbreak/continuewith no enclosing iteration/switch — as a caller-realm SyntaxError. A staticValidateEvalControlFlowpass runs in the pre-execution phase (PrepareEvalProgram); previously these were detected only at runtime (RunEvalProgramBody) and wrapped as TypeError. The walk visits only statements, so it never flags areturn/break/continueinside a nested function, and labeled break/continue are left to the parser.let y; var y;) is a separate detection gap and is not addressed here.Testing
built-ins/ShadowRealmtest262 — 64/64 in interpreter and bytecode modes (unchanged from the feat(engine): complete built-ins/ShadowRealm test262 conformance (64/64) #814 baseline)tests/built-ins/ShadowRealm/evaluate.js: duplicatelet/const/classandlet z; const z→SyntaxError(fresh re-eval still succeeds); illegal top-levelreturn/break/continue→SyntaxError, with no false positives for control flow inside a nested function or a loop/switchlanguage/eval-code(346/347 interp, 347/347 bytecode) andbuilt-ins/eval(10/10) are identical between baseline and fix — confirming both the sharedEvaluateEvalProgramrefactor and theValidateEvalControlFlowpass are behavior-preserving for the direct-eval /$262.evalScriptcallers (the oneeval-codefailure is the pre-existing, unrelateddirect/super-prop-method.js)