Implement non-strict arguments and with support#645
Merged
Conversation
- add unmapped arguments objects for interpreter and bytecode functions - implement with statement object environments across interpreter and VM - add regression coverage for arguments, with, and repeated NaN bundling
|
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)
📝 WalkthroughWalkthroughAdds --compat-non-strict-mode across CLI/engine; enables optional unmapped arguments and with-statement parsing/execution; extends compiler and bytecode to emit with/arguments/loose ops; updates VM call-frame arguments, evaluator/scope non-strict semantics, typed-array behavior, test262 runner, tests, and docs. ChangesCompatibility non-strict mode & runtime/compiler/VM updates
Estimated code review effort Possibly related PRs
Sequence Diagram(s) sequenceDiagram
participant CLI
participant Engine
participant Parser
participant Compiler
participant VM
CLI->>Engine: set NonStrictModeEnabled
Engine->>Parser: NonStrictModeEnabled
Parser->>Compiler: emit with/arguments/loose ops
Compiler->>VM: bytecode (OP_CREATE_ARGUMENTS / OP_HAS_WITH_BINDING / loose ops)
VM->>Runtime: execute with/assign/delete semantics
|
Contributor
Benchmark Results407 benchmarks Interpreted: 🟢 36 improved · 🔴 108 regressed · 263 unchanged · avg -3.4% arraybuffer.js — Interp: 🔴 5, 9 unch. · avg -2.3% · Bytecode: 🟢 1, 🔴 2, 11 unch. · avg -6.0%
arrays.js — Interp: 🔴 3, 16 unch. · avg -0.8% · Bytecode: 🔴 16, 3 unch. · avg -13.8%
async-await.js — Interp: 🔴 1, 5 unch. · avg -3.7% · Bytecode: 🔴 2, 4 unch. · avg -10.4%
async-generators.js — Interp: 2 unch. · avg -1.2% · Bytecode: 🔴 1, 1 unch. · avg -36.1%
base64.js — Interp: 🔴 6, 4 unch. · avg -3.1% · Bytecode: 🔴 4, 6 unch. · avg -4.4%
classes.js — Interp: 🟢 5, 🔴 1, 25 unch. · avg +1.1% · Bytecode: 🟢 1, 🔴 6, 24 unch. · avg -0.8%
closures.js — Interp: 🟢 1, 🔴 2, 8 unch. · avg -0.9% · Bytecode: 🔴 11 · avg -10.1%
collections.js — Interp: 🔴 4, 8 unch. · avg -2.3% · Bytecode: 🔴 10, 2 unch. · avg -5.5%
csv.js — Interp: 🟢 1, 12 unch. · avg +0.2% · Bytecode: 🟢 3, 🔴 1, 9 unch. · avg +0.5%
destructuring.js — Interp: 🔴 2, 20 unch. · avg -0.9% · Bytecode: 🟢 3, 🔴 8, 11 unch. · avg -1.8%
fibonacci.js — Interp: 🟢 2, 6 unch. · avg -0.1% · Bytecode: 🔴 4, 4 unch. · avg -10.7%
float16array.js — Interp: 🔴 25, 7 unch. · avg -20.8% · Bytecode: 🟢 1, 🔴 24, 7 unch. · avg -25.0%
for-of.js — Interp: 🔴 1, 6 unch. · avg -1.5% · Bytecode: 7 unch. · avg +1.3%
generators.js — Interp: 🟢 1, 3 unch. · avg +2.5% · Bytecode: 🔴 4 · avg -5.8%
iterators.js — Interp: 🟢 7, 🔴 6, 29 unch. · avg +0.4% · Bytecode: 🟢 4, 🔴 12, 26 unch. · avg -1.4%
json.js — Interp: 🟢 3, 🔴 2, 15 unch. · avg +1.0% · Bytecode: 🔴 5, 15 unch. · avg -0.9%
jsx.jsx — Interp: 🔴 2, 19 unch. · avg -0.9% · Bytecode: 🔴 14, 7 unch. · avg -3.9%
modules.js — Interp: 🔴 1, 8 unch. · avg +0.3% · Bytecode: 🔴 9 · avg -16.5%
numbers.js — Interp: 🔴 2, 9 unch. · avg -1.7% · Bytecode: 🔴 5, 6 unch. · avg -3.2%
objects.js — Interp: 🟢 3, 4 unch. · avg +1.3% · Bytecode: 🟢 1, 🔴 4, 2 unch. · avg -1.5%
promises.js — Interp: 🟢 1, 🔴 2, 9 unch. · avg +0.1% · Bytecode: 🔴 6, 6 unch. · avg -2.2%
regexp.js — Interp: 🔴 4, 7 unch. · avg -4.5% · Bytecode: 🔴 6, 5 unch. · avg -3.7%
strings.js — Interp: 🔴 7, 12 unch. · avg -2.1% · Bytecode: 🟢 1, 🔴 12, 6 unch. · avg -3.5%
tsv.js — Interp: 🟢 2, 7 unch. · avg +2.1% · Bytecode: 🟢 3, 🔴 1, 5 unch. · avg +2.4%
typed-arrays.js — Interp: 🟢 2, 🔴 16, 4 unch. · avg -19.6% · Bytecode: 🟢 5, 🔴 13, 4 unch. · avg -14.3%
uint8array-encoding.js — Interp: 🟢 6, 🔴 8, 4 unch. · avg +8.4% · Bytecode: 🟢 5, 🔴 8, 5 unch. · avg -0.1%
weak-collections.js — Interp: 🟢 2, 🔴 8, 5 unch. · avg -18.2% · Bytecode: 🟢 1, 🔴 10, 4 unch. · avg -9.2%
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
Suite TimingTest Runner (interpreted: 9,467 passed; bytecode: 9,467 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
test262 Conformance
Areas closest to 100%
Per-test deltas (+1266 / -63)Newly failing (63):
Newly passing (1266):
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 |
- expand language docs for arguments shadowing and with receiver behavior - add interpreter and bytecode implementation notes - link the decision log entry to PR #645
- clarify arguments object coverage and arrow lexical behavior - document with statement lookup, unscopables, closures, and receiver behavior - note generator arguments object support in the quick-reference table
- Inject strict directives for onlyStrict test262 script tests while keeping non-strict compat parsing enabled. - Keep bytecode arguments-object creation tied to compat availability instead of effective sloppy mode. - Make unmapped arguments.callee a throwing accessor and keep module with strict in all frontends.
frostney
changed the title
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: 18
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.Compiler.Expressions.pas (1)
1243-1308:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDestructuring assignment identifiers never target
withbindings.
EmitDestructuringsends identifier patterns toEmitBindingAssignmentFromRegister, but that helper only checks locals, upvalues, and globals. In assignment mode it never consultsShouldTryWithBinding/EmitWithAssignmentOrFallback, sowith (o) ({x} = src)will write the fallback binding instead ofo.x.Also applies to: 1326-1331
🤖 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.Compiler.Expressions.pas` around lines 1243 - 1308, The helper EmitBindingAssignmentFromRegister fails to target "with" bindings for identifier patterns during assignments; before resolving locals/upvalues, add a branch that when AAssignmentMode is true and ShouldTryWithBinding(ACtx, AName) (or the existing predicate used elsewhere) returns true, call EmitWithAssignmentOrFallback(ACtx, AName, AValueReg) (or the existing helper signature) and Exit so the assignment writes the with-bound property; make the same change at the other similar site referenced by EmitDestructuring to ensure destructuring identifiers use with-bindings in assignment mode.source/units/Goccia.Parser.pas (1)
3299-3328:⚠️ Potential issue | 🟠 Major | ⚡ Quick winScope the
letnewline fallback to thewithambiguity only.This rewrites every non-strict
letfollowed by a later-line{into an identifier expression statement. This will incorrectly parse valid sloppy lexical declarations likelet\n{a} = obj;as two statements instead of one declaration, since ECMAScript's grammar forbids line terminators betweenletand the binding pattern. Gate this logic behind the specificwith-body disambiguation instead of applying it to all statement contexts.🤖 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.Parser.pas` around lines 3299 - 3328, The current branch that treats a non-strict `let` followed by a later-line `{` as an identifier statement (the Check(gttLet) + FNonStrictModeEnabled + FAutomaticSemicolonInsertion + (FTokens[FCurrent + 1].TokenType = gttLeftBrace) + (Peek.Line < FTokens[FCurrent + 1].Line) block) is too broad and breaks valid sloppy `let` lexical declarations; restrict this behavior so it runs only when resolving the specific `with`-body ambiguity. Replace the broad condition with one that also checks a dedicated disambiguation predicate (e.g. call or add IsWithBodyAmbiguity/ShouldTreatLetAsWithBody at the same spot) and only create the TGocciaExpressionStatement/TGocciaIdentifierExpression when that predicate is true, leaving normal `let` declaration parsing untouched otherwise.
🧹 Nitpick comments (1)
tests/language/non-strict-mode/assignment.js (1)
83-96: ⚡ Quick winMake global state cleanup exception-safe.
If an assertion fails before cleanup,
globalThis.__gocciaNonStrictFixedAssignmentcan leak into subsequent tests. Wrap setup/assertions intry/finallyso cleanup always runs.Proposed patch
test("global object identifier writes silently fail for non-writable properties", () => { - Object.defineProperty(globalThis, "__gocciaNonStrictFixedAssignment", { - value: 1, - writable: false, - configurable: true, - }); - - const result = (__gocciaNonStrictFixedAssignment = 2); - - expect(result).toBe(2); - expect(globalThis.__gocciaNonStrictFixedAssignment).toBe(1); - - delete globalThis.__gocciaNonStrictFixedAssignment; + Object.defineProperty(globalThis, "__gocciaNonStrictFixedAssignment", { + value: 1, + writable: false, + configurable: true, + }); + try { + const result = (__gocciaNonStrictFixedAssignment = 2); + + expect(result).toBe(2); + expect(globalThis.__gocciaNonStrictFixedAssignment).toBe(1); + } finally { + delete globalThis.__gocciaNonStrictFixedAssignment; + } });🤖 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 `@tests/language/non-strict-mode/assignment.js` around lines 83 - 96, The test creates a non-writable global property __gocciaNonStrictFixedAssignment on globalThis but leaves deletion to run only on the success path; make cleanup exception-safe by wrapping the setup and assertions in a try/finally block and perform delete globalThis.__gocciaNonStrictFixedAssignment in the finally clause so the global is removed regardless of test failures; locate the test function (the test(...) block that defines __gocciaNonStrictFixedAssignment) and modify it to ensure the delete happens in finally.
🤖 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 `@docs/value-system.md`:
- Around line 543-544: The sentence that reads "Standalone calls to any function
type receive `undefined` as `this`" is misleading about arrow functions; update
the wording around that sentence (the paragraph containing "Standalone calls to
any function type" and the nearby reference to "`--compat-non-strict-mode`") to
state that standalone calls to ordinary (non-arrow) functions receive
`undefined` as `this` in strict mode, while arrow functions use lexical `this`
and do not get a call-site `this`; keep the existing note that script-source
`--compat-non-strict-mode` coerces nullish `this` for ordinary functions and
that module source remains strict.
In `@scripts/test-cli.ts`:
- Around line 327-328: The test currently only checks bundleNoFlag.exitCode but
not that the produced artifact exists; after the bundling call that sets
bundleNoFlag (using BUNDLER, src and tmp producing "no-flag.gbc"), add an
explicit file-existence assertion (e.g., fs.existsSync or fs.promises.access)
for join(tmp, "no-flag.gbc") and throw a clear Error if the output file is
missing or not readable so the test fails on a silent no-op compile; keep the
existing exitCode check as well.
In `@source/app/GocciaBundler.dpr`:
- Around line 223-224: Compute the effective source-type first and use it when
resolving EffectiveNonStrictMode so bundler matches the bytecode parse path for
modules; call ResolveFlagOption to get an EffectiveSourceType (e.g.
EffectiveSourceType := ResolveFlagOption(EngineOptions.SourceType, FileConfig,
'source-type')) before computing EffectiveNonStrictMode, then incorporate that
value into the non-strict decision (for example, treat source-type='module' as
forcing strict semantics or skip compat non-strict when
EffectiveSourceType='module') instead of basing EffectiveNonStrictMode solely on
ResolveFlagOption(EngineOptions.CompatNonStrictMode, FileConfig,
'compat-non-strict-mode'); apply the same change to the other non-strict
computations referenced by EffectiveNonStrictMode usages at the other locations.
In `@source/units/Goccia.AST.BindingPatterns.pas`:
- Around line 80-84: The name lookup for destructured parameters is using
TStringList.IndexOf which defaults to case-insensitive comparison; update the
logic around Names (the TStringList created when calling
CollectPatternBindingNames on AParameters[I].Pattern) to use case-sensitive
comparisons (e.g., set Names.CaseSensitive := True after creation) so that the
IndexOf check against AName is exact-case like the other string equality usages;
ensure this change is applied where Names is created/used in the routine that
iterates AParameters and checks Names.IndexOf(AName) (referencing Names,
CollectPatternBindingNames, AParameters[I].Pattern, and AName).
In `@source/units/Goccia.Compiler.Expressions.pas`:
- Around line 1044-1048: The fast path currently compiles AExpr.Value before
resolving the with-binding, which can change behavior if the RHS mutates the
with object; change the order so the with-assignment target is resolved first.
Introduce or call a resolver that performs the same lookup logic used by
EmitWithAssignmentOrFallback (e.g. ResolveWithAssignmentTarget or inline the
lookup used there) using ACtx and AExpr.Name to determine and store the
assignment target/binding before calling ACtx.CompileExpression(AExpr.Value,
ADest); then call EmitWithAssignmentOrFallback (or the assignment emitter) to
perform the write using the previously resolved target and the compiled ADest.
- Around line 417-438: ShouldTryWithBinding currently only avoids 'with' lookup
when a same-scope local hides the name, but it must also avoid probing the
with-object when the identifier is a captured outer binding (an upvalue) so
nested functions prefer the captured variable over with.<Name>. Modify
ShouldTryWithBinding to first detect whether AName is captured as an upvalue for
this scope (or any enclosing function scope represented by AScope) and if so
return False before probing the with binding; keep the existing checks
(AScope.WithBindingCount, KEYWORD_THIS, HiddenWithBindingName) and the
local-resolution logic (ResolveLocal/GetLocal/Local.Depth/GetWithBindingDepth)
but add an early check for captured/upvalue status and exit False when true.
In `@source/units/Goccia.Compiler.pas`:
- Around line 637-638: The assignment to FCurrentTemplate.StrictCode currently
allows FNonStrictMode to clear strictness; change it so top-level <module>
entrypoints remain strict regardless of the compatibility flag: when setting
FCurrentTemplate.StrictCode use a conditional that forces True for the module
entrypoint (i.e. if this template is the top-level module entrypoint — use the
existing marker/flag your codebase uses to detect that) otherwise evaluate (not
FNonStrictMode) or HasUseStrictDirective(AProgram); keep the references to
FCurrentTemplate.StrictCode, FNonStrictMode and HasUseStrictDirective(AProgram)
when implementing this conditional.
- Around line 241-243: TGocciaWithStatement bodies are not traversed by
HoistVarLocals, so var declarations inside a with body aren't hoisted before
earlier statements are compiled; update HoistVarLocals to walk the
TGocciaWithStatement.Body (or add a specific branch handling
TGocciaWithStatement in the traversal) so that any var/local bindings declared
inside the with body are hoisted the same way as other compound statements;
ensure the traversal/visitor used by HoistVarLocals also handles nodes produced
by CompileWithStatement and references TGocciaWithStatement and its Body so
semantics match other statement types.
In `@source/units/Goccia.Compiler.Statements.pas`:
- Around line 1182-1183: The current TGocciaWithStatement branch only inspects
the Body; update it so StatementNeedsIteratorClose also considers the with's
ObjectExpression (and any OP_TO_OBJECT usage) because evaluating the expression
or OP_TO_OBJECT can throw after iterator.next() has yielded a value; i.e., in
the branch handling TGocciaWithStatement call StatementNeedsIteratorClose on
both TGocciaWithStatement(AStmt).Body and
TGocciaWithStatement(AStmt).ObjectExpression (or otherwise treat the with node
as needing iterator-close if its object expression or OP_TO_OBJECT may throw) so
iterator-close analysis remains conservative.
In `@source/units/Goccia.Evaluator.pas`:
- Around line 1033-1041: EvaluateTaggedTemplate currently evaluates a bare
identifier tag with EvaluateExpression and forces ThisValue to
TGocciaUndefinedLiteralValue, losing the receiver in sloppy/with scopes; change
the tag resolution to mirror the call-site logic: if the tag expression is
TGocciaIdentifierExpression, call AContext.Scope.ResolveIdentifierReference with
the identifier name to obtain both the Callee and ThisValue, otherwise fall back
to EvaluateExpression and set ThisValue to undefined (reuse the same
TGocciaIdentifierExpression, ACallExpression.Callee, ThisValue,
EvaluateTaggedTemplate and AContext.Scope.ResolveIdentifierReference symbols to
locate and update the code).
In `@source/units/Goccia.Scope.pas`:
- Around line 698-710: ResolveIdentifierReference currently always leaves
AThisValue undefined except for KEYWORD_THIS; change it to preserve the receiver
when the resolved binding comes from an object (including the global object).
Call GetBinding once into a local binding variable (use the same symbol returned
by GetBinding in ResolveIdentifierReference), assign AValue from binding.Value,
and then set AThisValue from the binding's receiver/owner (or the global object)
instead of leaving it as TGocciaUndefinedLiteralValue; keep the existing
special-case for KEYWORD_THIS that returns FThisValue.
In `@source/units/Goccia.Values.ArgumentsObjectValue.pas`:
- Around line 55-61: ArrayPrototypeValuesFunction currently does a live
prototype lookup by creating a TGocciaArrayValue and calling
GetProperty(PROP_VALUES), which allows later mutations to Array.prototype.values
to affect newly created arguments iterators; instead capture the realm intrinsic
at arguments object creation. Update CreateUnmappedArgumentsObject to fetch and
store the realm's %Array.prototype.values% intrinsic (do not call
ArrayPrototypeValuesFunction/GetProperty(PROP_VALUES) at creation time) so the
arguments object's Symbol.iterator points to the realm-captured function;
reference the realm/intrinsics accessor used elsewhere in the project to obtain
the array prototype values intrinsic and use that stored value when wiring up
the unmapped arguments object's iterator rather than performing a dynamic
GetProperty on TGocciaArrayValue.
In `@source/units/Goccia.Values.ArrayValue.pas`:
- Around line 487-500: The helper DeleteSparseArrayIndexesAtOrAbove currently
discards the Boolean return from AArray.DeleteProperty which hides failures for
non-configurable sparse indexes; change DeleteSparseArrayIndexesAtOrAbove to
return Boolean and, while iterating Keys and invoking AArray.DeleteProperty,
check its return and immediately return False on failure so truncation aborts
atomically; then update the callers (the places that call DefineProperty and
TryDefineProperty which perform length truncation) to check the helper's Boolean
result and propagate/return failure so DefineProperty/TryDefineProperty halt
truncation when DeleteSparseArrayIndexesAtOrAbove fails. Ensure references to
TGocciaArrayValue, FElements behavior, and DeleteProperty semantics are
preserved.
In `@source/units/Goccia.Values.FunctionValue.pas`:
- Around line 205-209: Currently the guard only creates the unmapped arguments
object when the function is not strict, which omits the required arguments
binding for strict-directive functions under CompatibilityNonStrictMode; change
the logic so that when CompatibilityNonStrictMode and CreatesArgumentsObject are
true and neither ParameterListBindsName(FParameters, IDENTIFIER_ARGUMENTS) nor
ACallScope.ContainsOwnLexicalBinding(IDENTIFIER_ARGUMENTS) is true you always
call ACallScope.DefineVariableBinding(IDENTIFIER_ARGUMENTS,
CreateUnmappedArgumentsObject(AArguments), True) to create the unmapped
arguments object, and move any sloppy-only parameter-aliasing behavior behind a
separate condition checking not FStrictCode so FStrictCode no longer prevents
the unmapped binding creation.
In `@source/units/Goccia.Values.TypedArrayValue.pas`:
- Around line 237-259: Several methods still read the internal field FLength
directly causing stale-length bugs for length-tracking arrays; update algorithms
that loop or compute bounds (e.g., those referenced around lines 1094, 1130,
1298) to take a stable snapshot Len := GetLength after calling SyncBufferData
(or ensure SyncBufferData has run), then use Len instead of FLength for bounds
checks, loop limits and any math that also reads FBufferData/FByteOffset; also
ensure any direct reads of FBufferData occur only after SyncBufferData so the
snapshot and buffer view are consistent (use BytesPerElement(FKind) and
HasValidBackingRange only via GetLength or with Len).
In `@source/units/Goccia.VM.CallFrame.pas`:
- Line 21: The initialization of TGocciaVMCallFrame using FillChar(AFrame,
SizeOf(AFrame), 0) is unsafe now that TGocciaVMCallFrame contains a managed
field (Arguments: TGocciaRegisterArray); replace the FillChar usage with a
structured initialization such as AFrame := Default(TGocciaVMCallFrame) or
explicitly set each field to its zero/default value so that dynamic-array
finalization/initialization is handled correctly and avoids memory corruption.
In `@source/units/Goccia.VM.pas`:
- Around line 9575-9582: The OP_DELETE_GLOBAL handler currently returns false
when the global binding is absent; change it so that if the binding is absent
but the engine is in sloppy (non-strict) mode it returns true (i.e., set
FRegisters[A] := RegisterBoolean(True)), while still returning false only for
existing non-deletable bindings; locate the strictness flag used elsewhere in
this VM (the same flag/field the runtime uses for strict mode checks) and
consult it in OP_DELETE_GLOBAL before deciding the result from
FGlobalScope.DeleteBinding(GlobalName).
- Around line 9553-9564: The OP_SET_GLOBAL_LOOSE path currently throws via
ThrowReferenceError when FGlobalScope.Contains(GlobalName) is false; change this
so that in sloppy (non-strict) mode you create/update the global property
instead of throwing: when FGlobalScope is assigned and Contains(GlobalName) is
false, call FGlobalScope.AssignBinding(GlobalName,
RegisterToValue(FRegisters[A]), 0, 0, True) (the same form used when the name
exists) rather than ThrowReferenceError; keep the ThrowReferenceError for strict
mode (guard with your VM's strict-mode flag used elsewhere) and continue to
obtain the name via
Template.GetConstantUnchecked(DecodeBx(Instruction)).StringValue and the value
via RegisterToValue(FRegisters[A]).
---
Outside diff comments:
In `@source/units/Goccia.Compiler.Expressions.pas`:
- Around line 1243-1308: The helper EmitBindingAssignmentFromRegister fails to
target "with" bindings for identifier patterns during assignments; before
resolving locals/upvalues, add a branch that when AAssignmentMode is true and
ShouldTryWithBinding(ACtx, AName) (or the existing predicate used elsewhere)
returns true, call EmitWithAssignmentOrFallback(ACtx, AName, AValueReg) (or the
existing helper signature) and Exit so the assignment writes the with-bound
property; make the same change at the other similar site referenced by
EmitDestructuring to ensure destructuring identifiers use with-bindings in
assignment mode.
In `@source/units/Goccia.Parser.pas`:
- Around line 3299-3328: The current branch that treats a non-strict `let`
followed by a later-line `{` as an identifier statement (the Check(gttLet) +
FNonStrictModeEnabled + FAutomaticSemicolonInsertion + (FTokens[FCurrent +
1].TokenType = gttLeftBrace) + (Peek.Line < FTokens[FCurrent + 1].Line) block)
is too broad and breaks valid sloppy `let` lexical declarations; restrict this
behavior so it runs only when resolving the specific `with`-body ambiguity.
Replace the broad condition with one that also checks a dedicated disambiguation
predicate (e.g. call or add IsWithBodyAmbiguity/ShouldTreatLetAsWithBody at the
same spot) and only create the
TGocciaExpressionStatement/TGocciaIdentifierExpression when that predicate is
true, leaving normal `let` declaration parsing untouched otherwise.
---
Nitpick comments:
In `@tests/language/non-strict-mode/assignment.js`:
- Around line 83-96: The test creates a non-writable global property
__gocciaNonStrictFixedAssignment on globalThis but leaves deletion to run only
on the success path; make cleanup exception-safe by wrapping the setup and
assertions in a try/finally block and perform delete
globalThis.__gocciaNonStrictFixedAssignment in the finally clause so the global
is removed regardless of test failures; locate the test function (the test(...)
block that defines __gocciaNonStrictFixedAssignment) and modify it to ensure the
delete happens in finally.
🪄 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: ea5f7a95-b293-43cc-8120-a6ad5fb84512
📒 Files selected for processing (81)
docs/build-system.mddocs/bytecode-vm.mddocs/decision-log.mddocs/embedding.mddocs/errors.mddocs/goals.mddocs/interpreter.mddocs/language-tables.mddocs/language.mddocs/test262.mddocs/testing.mddocs/tutorial.mddocs/value-system.mdscripts/run_test262_suite.tsscripts/test-cli-apps.tsscripts/test-cli-config.tsscripts/test-cli.tssource/app/Goccia.CLI.Application.passource/app/GocciaBenchmarkRunner.dprsource/app/GocciaBundler.dprsource/app/GocciaREPL.dprsource/app/GocciaScriptLoader.dprsource/app/GocciaScriptLoaderBare.dprsource/app/GocciaTestRunner.dprsource/shared/CLI.Options.passource/units/Goccia.AST.BindingPatterns.passource/units/Goccia.AST.Expressions.passource/units/Goccia.AST.Statements.passource/units/Goccia.Bytecode.Binary.passource/units/Goccia.Bytecode.Chunk.passource/units/Goccia.Bytecode.OpCodeNames.passource/units/Goccia.Bytecode.passource/units/Goccia.Compiler.ConstantFolding.passource/units/Goccia.Compiler.Context.passource/units/Goccia.Compiler.Expressions.passource/units/Goccia.Compiler.Scope.passource/units/Goccia.Compiler.Statements.passource/units/Goccia.Compiler.passource/units/Goccia.Constants.PropertyNames.passource/units/Goccia.Constants.passource/units/Goccia.Engine.Backend.passource/units/Goccia.Engine.passource/units/Goccia.Evaluator.Assignment.passource/units/Goccia.Evaluator.Context.passource/units/Goccia.Evaluator.passource/units/Goccia.Interpreter.passource/units/Goccia.Modules.Loader.passource/units/Goccia.Parser.passource/units/Goccia.Scope.passource/units/Goccia.VM.CallFrame.passource/units/Goccia.VM.passource/units/Goccia.Values.ArgumentsObjectValue.passource/units/Goccia.Values.ArrayValue.passource/units/Goccia.Values.FunctionBase.passource/units/Goccia.Values.FunctionValue.passource/units/Goccia.Values.GeneratorValue.passource/units/Goccia.Values.ObjectValue.passource/units/Goccia.Values.ProxyValue.passource/units/Goccia.Values.TypedArrayValue.pastests/built-ins/Array/array-modification.jstests/built-ins/Proxy/set.jstests/built-ins/TypedArray/element-access.jstests/built-ins/TypedArray/non-strict/element-access.jstests/built-ins/TypedArray/non-strict/goccia.jsontests/language/arguments/arguments-object.jstests/language/arguments/goccia.jsontests/language/arguments/strict-directive-arguments.jstests/language/non-strict-mode-defaults/arguments-disabled.jstests/language/non-strict-mode-defaults/assignment-disabled.jstests/language/non-strict-mode-defaults/goccia.jsontests/language/non-strict-mode-modules/goccia.jsontests/language/non-strict-mode-modules/this-binding.jstests/language/non-strict-mode/array-length.jstests/language/non-strict-mode/assignment.jstests/language/non-strict-mode/delete.jstests/language/non-strict-mode/goccia.jsontests/language/non-strict-mode/this-binding.jstests/language/non-strict-mode/use-strict-directive.jstests/language/statements/unsupported-features.jstests/language/with/goccia.jsontests/language/with/with-statement.js
💤 Files with no reviewable changes (1)
- docs/errors.md
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
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.Evaluator.pas (1)
5592-5617:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve
thisfor private-member tag expressions.Line 5609 mirrors the identifier fix, but tagged templates can also be driven by
TGocciaPrivateMemberExpression. That path still falls through toEvaluateExpression(...)and forcesThisValue := undefined, soobj.#tag\x`won’t receiveobjas the receiver even thoughobj.#tag()` now does.Suggested fix
function EvaluateTaggedTemplate(const ATaggedTemplateExpression: TGocciaTaggedTemplateExpression; const AContext: TGocciaEvaluationContext): TGocciaValue; var Callee, ThisValue, ExprValue, TemplateObject: TGocciaValue; MemberExpr: TGocciaMemberExpression; + PrivateMemberExpr: TGocciaPrivateMemberExpression; CookedArray, RawArray: TGocciaArrayValue; Arguments: TGocciaArgumentsCollection; I: Integer; CalleeName: string; begin @@ if ATaggedTemplateExpression.Tag is TGocciaMemberExpression then begin MemberExpr := TGocciaMemberExpression(ATaggedTemplateExpression.Tag); Callee := EvaluateMember(MemberExpr, AContext, ThisValue); end + else if ATaggedTemplateExpression.Tag is TGocciaPrivateMemberExpression then + begin + PrivateMemberExpr := TGocciaPrivateMemberExpression( + ATaggedTemplateExpression.Tag); + Callee := EvaluatePrivateMember(PrivateMemberExpr, AContext, ThisValue); + end else if ATaggedTemplateExpression.Tag is TGocciaIdentifierExpression then AContext.Scope.ResolveIdentifierReference( TGocciaIdentifierExpression(ATaggedTemplateExpression.Tag).Name, Callee, ThisValue)🤖 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 5592 - 5617, The tagged-template tag evaluation currently treats private-member tags like other expressions and falls through to EvaluateExpression which sets ThisValue to undefined; update the tag handling so that when ATaggedTemplateExpression.Tag is a TGocciaPrivateMemberExpression you evaluate it similarly to TGocciaMemberExpression: cast to TGocciaPrivateMemberExpression, call the appropriate member evaluation (same codepath used by EvaluateMember) to obtain Callee and preserve ThisValue (do not overwrite ThisValue with Undefined), ensuring obj.#tag`...` receives obj as the receiver.source/units/Goccia.Compiler.Expressions.pas (1)
2798-2846:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftFallback path incorrectly uses method call semantics.
When the tag is an identifier that might be in a with-binding,
AIsMethodCallis set toTrueunconditionally at line 2802. If no with-object actually contains the binding, the fallback path (line 2830) loads the identifier butAObjRegremainsundefined. The caller then emitsOP_CALL_METHODwithundefinedas receiver, which is incorrect—the non-method fallback should useOP_CALLinstead.Compare with
TryCompileWithIdentifierCallwhich correctly callsEmitBranchCall(True, ...)for with-binding hits andEmitBranchCall(False, ...)for the fallback.Suggested approach
Restructure to emit the call within
CompileTagCalleeRegisters(or a new helper), usingOP_CALL_METHODfor with-binding paths andOP_CALLfor the fallback, similar toTryCompileWithIdentifierCall. Alternatively, return a runtime flag indicating whether a with-binding was found and let the caller branch on it—but that would require additional bytecode.🤖 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.Compiler.Expressions.pas` around lines 2798 - 2846, The code sets AIsMethodCall := True for identifier tags before actually finding a with-binding, so the fallback path leaves AObjReg undefined and causes callers to emit OP_CALL_METHOD incorrectly; change the logic in the TGoccia.Compiler.Expressions block that handles TGocciaIdentifierExpression so that you only mark or emit a method call when a with-binding is actually found (i.e., when EndCount > 0), and for the fallback path emit/indicate a normal call (OP_CALL) instead of a method call (OP_CALL_METHOD). Concretely: either move call emission into this block like TryCompileWithIdentifierCall does (use EmitBranchCall/EmitCall variants to emit OP_CALL_METHOD for the with-binding branches and OP_CALL for the fallback), or keep the current API but set AIsMethodCall := (EndCount > 0) and ensure AObjReg is only allocated/used when EndCount > 0 so the caller receives the correct call semantics; reference IdentExpr, AObjReg, ABaseReg, EndCount, CompileIdentifierAccessNoWith and TryCompileWithIdentifierCall to locate and adjust the behavior.
🤖 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.Compiler.Expressions.pas`:
- Around line 1245-1255: DeclareArgumentsObjectLocal currently only checks the
global ACtx.CompatibilityNonStrictMode and therefore can incorrectly create an
implicit arguments binding for functions that are themselves strict; modify
DeclareArgumentsObjectLocal to accept an extra boolean parameter (e.g.,
AChildStrict) and only allow creating the implicit IDENTIFIER_ARGUMENTS local
when ACtx.CompatibilityNonStrictMode is true AND AChildStrict is false. Update
every call site to pass the child function's strictness (use
ChildTemplate.StrictCode at call sites where the child template is available)
and reference the existing symbols DeclareArgumentsObjectLocal,
ACtx.CompatibilityNonStrictMode, IDENTIFIER_ARGUMENTS and
ChildTemplate.StrictCode to locate places needing change.
In `@source/units/Goccia.Values.ArrayValue.pas`:
- Around line 2861-2865: The code currently frees ADescriptor when
DeleteSparseArrayIndexesAtOrAbove(Self, NewLen) returns false, which violates
DefineProperty's ownership contract and can cause a double-free; remove the
ADescriptor.Free call from that error path so the descriptor remains owned by
the caller, leaving the subsequent
ThrowTypeError(Format(SErrorCannotRedefineNonConfigurable, [AName]),
SSuggestCannotDeleteNonConfigurable) unchanged; ensure no other early-return
paths free ADescriptor in this block so ownership semantics for ADescriptor (and
callers that free it in their except handlers) remain consistent.
---
Outside diff comments:
In `@source/units/Goccia.Compiler.Expressions.pas`:
- Around line 2798-2846: The code sets AIsMethodCall := True for identifier tags
before actually finding a with-binding, so the fallback path leaves AObjReg
undefined and causes callers to emit OP_CALL_METHOD incorrectly; change the
logic in the TGoccia.Compiler.Expressions block that handles
TGocciaIdentifierExpression so that you only mark or emit a method call when a
with-binding is actually found (i.e., when EndCount > 0), and for the fallback
path emit/indicate a normal call (OP_CALL) instead of a method call
(OP_CALL_METHOD). Concretely: either move call emission into this block like
TryCompileWithIdentifierCall does (use EmitBranchCall/EmitCall variants to emit
OP_CALL_METHOD for the with-binding branches and OP_CALL for the fallback), or
keep the current API but set AIsMethodCall := (EndCount > 0) and ensure AObjReg
is only allocated/used when EndCount > 0 so the caller receives the correct call
semantics; reference IdentExpr, AObjReg, ABaseReg, EndCount,
CompileIdentifierAccessNoWith and TryCompileWithIdentifierCall to locate and
adjust the behavior.
In `@source/units/Goccia.Evaluator.pas`:
- Around line 5592-5617: The tagged-template tag evaluation currently treats
private-member tags like other expressions and falls through to
EvaluateExpression which sets ThisValue to undefined; update the tag handling so
that when ATaggedTemplateExpression.Tag is a TGocciaPrivateMemberExpression you
evaluate it similarly to TGocciaMemberExpression: cast to
TGocciaPrivateMemberExpression, call the appropriate member evaluation (same
codepath used by EvaluateMember) to obtain Callee and preserve ThisValue (do not
overwrite ThisValue with Undefined), ensuring obj.#tag`...` receives obj as the
receiver.
🪄 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: 2e58c2c6-8b6b-4045-b392-8dcb12092217
📒 Files selected for processing (12)
docs/value-system.mdscripts/test-cli.tssource/app/Goccia.CLI.Application.passource/app/GocciaBundler.dprsource/units/Goccia.AST.BindingPatterns.passource/units/Goccia.Compiler.Expressions.passource/units/Goccia.Compiler.Statements.passource/units/Goccia.Compiler.passource/units/Goccia.Evaluator.passource/units/Goccia.Scope.passource/units/Goccia.VM.passource/units/Goccia.Values.ArrayValue.pas
✅ Files skipped from review due to trivial changes (1)
- docs/value-system.md
🚧 Files skipped from review as they are similar to previous changes (7)
- source/units/Goccia.Compiler.pas
- source/units/Goccia.AST.BindingPatterns.pas
- source/app/Goccia.CLI.Application.pas
- source/app/GocciaBundler.dpr
- source/units/Goccia.Scope.pas
- source/units/Goccia.Compiler.Statements.pas
- source/units/Goccia.VM.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
argumentsobjects andwithobject environments behind--compat-non-strict-modein interpreter and bytecode mode, while keeping modules and strict directive code on strict semantics.arguments.calleeis a throwing accessor,onlyStricttest262 cases receive a strict directive while retaining compat parser support, and modulewithsyntax now fails consistently in interpreter and bytecode paths.withSymbol.unscopables, closures, assignment/update behavior, method-call receivers, and ASI aroundwithfollowed bylet.[[Set]], non-strict assignment/delete/this semantics, and the bundler repeated-NaNconstant crash.Testing
Verification run:
./format.pas --checkgit diff --check./build.pas clean testrunner loader loaderbare bundler benchmarkrunner repl./build.pas tests(native Pascal test target builds successfully)bun scripts/test-cli.ts./build/GocciaTestRunner tests/language/arguments --mode=bytecode --no-progress./build/GocciaTestRunner tests/language/non-strict-mode --no-progress./build/GocciaTestRunner tests/language/non-strict-mode --mode=bytecode --no-progress./build/GocciaTestRunner tests/language/non-strict-mode-modules --no-progress./build/GocciaTestRunner tests/language/non-strict-mode-modules --mode=bytecode --no-progressbun scripts/run_test262_suite.ts --suite-dir /tmp/goccia-test262-suite --filter "language/arguments-object/10.6-14-c-4-s.js" --mode=interpreted --jobs=1 --output=/tmp/t262-args-callee-interpreted.jsonbun scripts/run_test262_suite.ts --suite-dir /tmp/goccia-test262-suite --filter "language/arguments-object/10.6-14-c-4-s.js" --mode=bytecode --jobs=1 --output=/tmp/t262-args-callee-bytecode.jsonFull
tests --asi --unsafe-ffiand bytecode full-suite runs before the latest reviewer fixes both reached 9331/9336 passing; the remaining 5 failures were the existing local FFI fixture load issue for./fixtures/ffi/libfixture.dylib.