Allow var/function declarations to shadow built-in globals in script mode#577
Merged
Conversation
…mode (#521) Per ES2026 §16.1.7, top-level var and function declarations may shadow built-in globals (Array, NaN, Infinity, undefined, etc.) in script mode. Previously all such redeclarations were rejected with SyntaxError. Add a BuiltIn flag to TLexicalBinding so the engine can distinguish engine-registered bindings from user declarations. CheckTopLevelRedeclarations now skips var/function declarations when the existing binding is built-in, and DefineVariableBinding removes the built-in lexical binding at runtime so the var binding becomes visible through GetBinding. Closes #521 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
📝 WalkthroughWalkthroughThis PR implements built-in global shadowing support by adding a BuiltIn flag to lexical bindings, extending TGocciaScope.DefineLexicalBinding to accept ABuiltIn and exposing IsBuiltInBinding, updating variable binding and redeclaration checks to allow script-mode var/function shadowing of built-ins, marking builtin registrations accordingly, and adding tests verifying shadowing rules. ChangesBuilt-in Global Shadowing Support
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Contributor
Suite TimingTest Runner (interpreted: 8,940 passed; bytecode: 8,940 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: 407; bytecode: 407)
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 Results407 benchmarks Interpreted: 🟢 401 improved · 🔴 4 regressed · 2 unchanged · avg +35.2% arraybuffer.js — Interp: 🟢 14 · avg +34.1% · Bytecode: 🔴 2, 12 unch. · avg -1.0%
arrays.js — Interp: 🟢 19 · avg +31.4% · Bytecode: 🟢 6, 13 unch. · avg +2.3%
async-await.js — Interp: 🟢 6 · avg +34.3% · Bytecode: 🟢 4, 2 unch. · avg -0.1%
async-generators.js — Interp: 🟢 2 · avg +29.1% · Bytecode: 2 unch. · avg -1.3%
base64.js — Interp: 🟢 10 · avg +35.5% · Bytecode: 🔴 2, 8 unch. · avg -1.0%
classes.js — Interp: 🟢 31 · avg +28.0% · Bytecode: 🟢 1, 🔴 3, 27 unch. · avg -0.7%
closures.js — Interp: 🟢 11 · avg +36.0% · Bytecode: 🔴 1, 10 unch. · avg -0.0%
collections.js — Interp: 🟢 12 · avg +32.7% · Bytecode: 12 unch. · avg -0.4%
csv.js — Interp: 🟢 13 · avg +41.3% · Bytecode: 🔴 3, 10 unch. · avg -2.2%
destructuring.js — Interp: 🟢 22 · avg +32.1% · Bytecode: 🟢 2, 🔴 1, 19 unch. · avg -0.3%
fibonacci.js — Interp: 🟢 8 · avg +36.0% · Bytecode: 🟢 2, 6 unch. · avg -0.3%
float16array.js — Interp: 🟢 30, 🔴 1, 1 unch. · avg +29.8% · Bytecode: 🟢 7, 🔴 4, 21 unch. · avg +1.0%
for-of.js — Interp: 🟢 7 · avg +36.4% · Bytecode: 🔴 1, 6 unch. · avg -1.6%
generators.js — Interp: 🟢 4 · avg +31.6% · Bytecode: 🟢 2, 2 unch. · avg +2.0%
iterators.js — Interp: 🟢 42 · avg +29.7% · Bytecode: 🟢 1, 🔴 26, 15 unch. · avg -2.8%
json.js — Interp: 🟢 20 · avg +30.9% · Bytecode: 🟢 7, 13 unch. · avg +1.1%
jsx.jsx — Interp: 🟢 21 · avg +45.7% · Bytecode: 🔴 2, 19 unch. · avg -0.3%
modules.js — Interp: 🟢 9 · avg +33.4% · Bytecode: 🟢 1, 8 unch. · avg +0.6%
numbers.js — Interp: 🟢 11 · avg +39.6% · Bytecode: 🟢 4, 7 unch. · avg +2.0%
objects.js — Interp: 🟢 7 · avg +38.9% · Bytecode: 7 unch. · avg +0.2%
promises.js — Interp: 🟢 12 · avg +25.6% · Bytecode: 🟢 3, 9 unch. · avg +2.0%
regexp.js — Interp: 🟢 11 · avg +35.9% · Bytecode: 🟢 1, 🔴 1, 9 unch. · avg +1.0%
strings.js — Interp: 🟢 19 · avg +42.0% · Bytecode: 🟢 3, 🔴 1, 15 unch. · avg +1.1%
tsv.js — Interp: 🟢 9 · avg +44.1% · Bytecode: 🟢 2, 7 unch. · avg +0.0%
typed-arrays.js — Interp: 🟢 22 · avg +43.3% · Bytecode: 🟢 9, 🔴 3, 10 unch. · avg +1.8%
uint8array-encoding.js — Interp: 🟢 14, 🔴 3, 1 unch. · avg +17.4% · Bytecode: 🟢 5, 🔴 2, 11 unch. · avg +2.5%
weak-collections.js — Interp: 🟢 15 · avg +75.7% · Bytecode: 🟢 6, 🔴 2, 7 unch. · avg +2.5%
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. |
- Initialize BuiltIn := False in PredeclareLexicalBinding to avoid undefined record field values. - Restructure CheckTopLevelRedeclarations so var/function declarations still error against user-declared lexical bindings (let/const from a prior script evaluation) — only built-in bindings are exempt. - Move function shadowing tests to file level so they exercise the CheckTopLevelRedeclarations path. - Remove eval-based let/const tests (eval is undefined in GocciaScript); replace with valid block-scope shadowing tests. - Use bare `var NaN;` (not `var NaN = 42`) since NaN on globalThis is non-writable per §19.1 and strict mode correctly throws TypeError on assignment to non-writable properties. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
test262 Conformance
Areas closest to 100%
Per-test deltas (+9 / -0)Newly passing (9):
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 |
Bare `var NaN;` at the top level tests the §16.1.7 declaration path.
Function-scoped `var NaN = 42` verifies a local binding is created
with the initializer value (42), independent of the global NaN property.
Top-level `var NaN = 42` correctly throws TypeError because globalThis.NaN
is {writable: false} per §19.1 and strict mode rejects the assignment, but
testing that path via toThrow is blocked by a pre-existing segfault in the
test library when the callback does not throw.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Built-in bindings only exist on the global scope. Skip the TryGetValue probe in DefineVariableBinding for function/module-scoped vars, which is the common case during execution. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
source/units/Goccia.Scope.pas (2)
353-368:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPropagate
ABuiltInwhen materializing a predeclared binding.The new parameter is ignored on the
not ExistingLexicalBinding.Initializedpath, so this branch can only ever produce a non-built-in binding even if the caller explicitly passesABuiltIn=True.Suggested fix
if not ExistingLexicalBinding.Initialized then begin ExistingLexicalBinding.Value := AValue; ExistingLexicalBinding.DeclarationType := ADeclarationType; ExistingLexicalBinding.Initialized := True; + ExistingLexicalBinding.BuiltIn := ABuiltIn; FLexicalBindings.AddOrSetValue(AName, ExistingLexicalBinding); Exit; 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.Scope.pas` around lines 353 - 368, The predeclared-materialization branch in TGocciaScope.DefineLexicalBinding ignores the ABuiltIn flag so materialized bindings are never marked built-in; update the branch that handles not ExistingLexicalBinding.Initialized to propagate the ABuiltIn parameter onto the ExistingLexicalBinding (e.g., set ExistingLexicalBinding.BuiltIn := ABuiltIn) before re-adding it to FLexicalBindings so the new binding preserves the caller's built-in status; ensure you reference the ExistingLexicalBinding/TLexicalBinding fields when making this change.
423-477:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd
Binding.BuiltIn := False;to first-declaration path in DefineVariableBinding.The first-declaration branch (lines 470–478) creates a new
TLexicalBindingrecord without initializing theBuiltInfield. In FPC, uninitialized record fields contain garbage values, so later reads can nondeterministically misclassify uservarbindings. All other binding creation paths (e.g., lines 345–350, 383–392) explicitly initializeBuiltIn.Suggested fix
else begin // First declaration: create the binding Binding.Value := EffectiveValue; Binding.DeclarationType := dtVar; Binding.Initialized := True; + Binding.BuiltIn := False; Binding.TypeHint := sltUntyped; TargetScope.FVarBindings.AddOrSetValue(AName, Binding); 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.Scope.pas` around lines 423 - 477, DefineVariableBinding creates a TLexicalBinding on the "first declaration" path but never sets its BuiltIn flag, leaving it with garbage under FPC; fix by explicitly setting Binding.BuiltIn := False before adding the binding (i.e., in TGocciaScope.DefineVariableBinding’s first-declaration branch prior to TargetScope.FVarBindings.AddOrSetValue), mirroring other binding creation sites that initialize BuiltIn.
🤖 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.Engine.pas`:
- Around line 445-446: The injected-global registration currently calls
FInterpreter.GlobalScope.DefineLexicalBinding(AName, AValue, dtConst, True)
marking them as BuiltIn, which causes top-level redeclarations to treat injected
names as engine intrinsics; change the call so injected names are defined
without the BuiltIn flag (pass False or use the overload that does not mark
built-ins) and keep tracking the name with FInjectedGlobals.Add(AName) as before
so injected globals are recorded but not treated as built-ins.
In `@source/units/Goccia.Scope.Redeclaration.pas`:
- Around line 83-88: The top-level built-in-shadow exemption for var
declarations isn't applied to TGocciaDestructuringDeclaration because
CheckTopLevelRedeclarations calls CheckPatternRedeclarations without the IsVar
context; update the call site in CheckTopLevelRedeclarations to pass the
declaration's IsVar flag into CheckPatternRedeclarations (or alternatively
perform the AScope.IsBuiltInBinding(DeclName) && IsVar check in
CheckTopLevelRedeclarations before delegating) so that
CheckPatternRedeclarations can skip built-in bindings when the pattern comes
from a var destructuring (ensure you reference
TGocciaDestructuringDeclaration.IsVar, CheckTopLevelRedeclarations and
CheckPatternRedeclarations to locate the change).
---
Outside diff comments:
In `@source/units/Goccia.Scope.pas`:
- Around line 353-368: The predeclared-materialization branch in
TGocciaScope.DefineLexicalBinding ignores the ABuiltIn flag so materialized
bindings are never marked built-in; update the branch that handles not
ExistingLexicalBinding.Initialized to propagate the ABuiltIn parameter onto the
ExistingLexicalBinding (e.g., set ExistingLexicalBinding.BuiltIn := ABuiltIn)
before re-adding it to FLexicalBindings so the new binding preserves the
caller's built-in status; ensure you reference the
ExistingLexicalBinding/TLexicalBinding fields when making this change.
- Around line 423-477: DefineVariableBinding creates a TLexicalBinding on the
"first declaration" path but never sets its BuiltIn flag, leaving it with
garbage under FPC; fix by explicitly setting Binding.BuiltIn := False before
adding the binding (i.e., in TGocciaScope.DefineVariableBinding’s
first-declaration branch prior to TargetScope.FVarBindings.AddOrSetValue),
mirroring other binding creation sites that initialize BuiltIn.
🪄 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: 7047f63b-4a71-47ca-8bbd-aa95a95ac967
📒 Files selected for processing (33)
source/units/Goccia.Builtins.Benchmark.passource/units/Goccia.Builtins.CSV.passource/units/Goccia.Builtins.Console.passource/units/Goccia.Builtins.DisposableStack.passource/units/Goccia.Builtins.GlobalBigInt.passource/units/Goccia.Builtins.GlobalFFI.passource/units/Goccia.Builtins.GlobalFetch.passource/units/Goccia.Builtins.GlobalPromise.passource/units/Goccia.Builtins.GlobalProxy.passource/units/Goccia.Builtins.GlobalReflect.passource/units/Goccia.Builtins.GlobalRegExp.passource/units/Goccia.Builtins.GlobalSymbol.passource/units/Goccia.Builtins.Globals.passource/units/Goccia.Builtins.JSON.passource/units/Goccia.Builtins.JSON5.passource/units/Goccia.Builtins.JSONL.passource/units/Goccia.Builtins.Math.passource/units/Goccia.Builtins.Performance.passource/units/Goccia.Builtins.TOML.passource/units/Goccia.Builtins.TSV.passource/units/Goccia.Builtins.Temporal.passource/units/Goccia.Builtins.TestingLibrary.passource/units/Goccia.Builtins.YAML.passource/units/Goccia.Engine.passource/units/Goccia.ObjectModel.Engine.passource/units/Goccia.Runtime.passource/units/Goccia.Scope.BindingMap.passource/units/Goccia.Scope.Redeclaration.passource/units/Goccia.Scope.pastests/language/declarations/const/cannot-shadow-builtin-globals.jstests/language/declarations/let/cannot-shadow-builtin-globals.jstests/language/function-keyword/shadow-builtin-globals.jstests/language/var/shadow-builtin-globals.js
- Injected globals (RegisterGlobal) no longer marked BuiltIn — they are
user-provided, not engine intrinsics, and should not be shadowable.
- CheckPatternRedeclarations now receives the IsVar flag so var
destructuring (e.g. `var { NaN } = obj;`) gets the same built-in
exemption as simple var declarations. Closes #580.
- DefineLexicalBinding's predeclared-materialization branch now
propagates ABuiltIn onto the existing binding instead of silently
dropping it.
- DefineVariableBinding's first-declaration branch explicitly sets
Binding.BuiltIn := False to avoid uninitialized record fields.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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
var,function, andvar-destructuring declarations to shadow built-in globals (Array,NaN,Infinity,undefined, etc.) in script mode, per ES2026 §16.1.7 GlobalDeclarationInstantiation.let/const/classredeclarations of built-in globals are blocked.BuiltInflag toTLexicalBindingso the engine can distinguish engine-registered bindings from user declarations. All built-in registration sites passTrueat the call site. User-injected globals (RegisterGlobal) are intentionally not marked built-in.CheckTopLevelRedeclarationsskipsvar/functiondeclarations when the existing binding is built-in;CheckPatternRedeclarationsreceives theIsVarflag sovar-destructuring patterns get the same exemption.DefineVariableBindingremoves the built-in lexical binding at runtime (preserving the original value for barevar NaN;-style declarations without initializers) so the var binding becomes visible throughGetBinding. The built-in check is guarded byskGlobalto skip the probe for function/module-scoped vars.TLexicalBindingcreation sites (PredeclareLexicalBinding,DefineLexicalBindingmaterialization branch,DefineVariableBindingfirst-declaration branch) explicitly initialize theBuiltInfield.Testing
varshadowing (NaN,Infinity,undefined,Array),functionshadowing (Array,Object,Error), function-scopedvar NaN = 42, andlet/constblock-scope shadowing