Thread optional receiver through Reflect.get and Reflect.set#243
Merged
Conversation
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 (1)
📝 WalkthroughWalkthroughThreads the optional receiver through Reflect.get and Reflect.set and the object property pipeline: Reflect defaults receiver to target when omitted, calls context-aware getters/setters with receiver as this, and applies data writes to the receiver per ECMAScript semantics. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant ReflectGet as Reflect.get
participant Target as Target
participant Proto as Prototype
participant Getter as Accessor Getter
Caller->>ReflectGet: Reflect.get(target, prop, receiver?)
ReflectGet->>ReflectGet: Receiver := receiver ?? target
ReflectGet->>Target: GetPropertyWithContext(prop, Receiver)
alt Own data property
Target-->>ReflectGet: value
else Own accessor
Target->>Getter: Call getter with Receiver as this
Getter-->>Target: value
Target-->>ReflectGet: value
else Not own → Proto
Target->>Proto: GetPropertyWithContext(prop, Receiver)
alt Accessor found
Proto->>Getter: Call getter with Receiver as this
Getter-->>Proto: value
Proto-->>Target: value
Target-->>ReflectGet: value
else Data found
Proto-->>Target: value
Target-->>ReflectGet: value
end
end
ReflectGet-->>Caller: value
sequenceDiagram
participant Caller as Caller
participant ReflectSet as Reflect.set
participant Target as Target
participant Proto as Prototype
participant Setter as Accessor Setter
participant ReceiverObj as Receiver
Caller->>ReflectSet: Reflect.set(target, prop, value, receiver?)
ReflectSet->>ReflectSet: Receiver := receiver ?? target
ReflectSet->>Target: AssignPropertyWithReceiver(prop, value, Receiver)
alt Own data descriptor on Target
alt writable
Target->>ReceiverObj: Define/Update data property on Receiver
ReceiverObj-->>Target: success (true)
Target-->>ReflectSet: true
else not writable
Target-->>ReflectSet: false
end
else Not own → find on Proto
alt Accessor with setter found
Proto->>Setter: Call setter(value) with Receiver as this
Setter-->>Proto: success
Proto-->>Target: true
Target-->>ReflectSet: true
else Not found and Receiver extensible
Target->>ReceiverObj: Define new data property on Receiver
ReceiverObj-->>Target: success
Target-->>ReflectSet: true
else Not found and Receiver non-extensible
Target-->>ReflectSet: false
end
end
ReflectSet-->>Caller: boolean
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 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 |
![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)
units/Goccia.Values.ObjectValue.pas (1)
53-77:⚠️ Potential issue | 🔴 CriticalMake the receiver-aware property hooks virtual before routing Reflect through them.
Reflect.get/Reflect.setnow depend onGetPropertyWithContext,GetSymbolPropertyWithReceiver,AssignPropertyWithReceiver, andAssignSymbolPropertyWithReceiver, but this API still lives as concreteTGocciaObjectValuemethods. That means subtype-specific property behavior cannot participate in the receiver-aware path, so cases likeReflect.get(arr, "length", receiver)orReflect.set(arr, "length", 1)can silently fall back to plain-object semantics instead of the subclass’s[[Get]]/[[Set]].Suggested API direction
- function AssignPropertyWithReceiver(const AName: string; const AValue: TGocciaValue; const AReceiver: TGocciaValue): Boolean; + function AssignPropertyWithReceiver(const AName: string; const AValue: TGocciaValue; const AReceiver: TGocciaValue): Boolean; virtual; - function GetPropertyWithContext(const AName: string; const AThisContext: TGocciaValue): TGocciaValue; + function GetPropertyWithContext(const AName: string; const AThisContext: TGocciaValue): TGocciaValue; virtual; - function AssignSymbolPropertyWithReceiver(const ASymbol: TGocciaSymbolValue; const AValue: TGocciaValue; const AReceiver: TGocciaValue): Boolean; + function AssignSymbolPropertyWithReceiver(const ASymbol: TGocciaSymbolValue; const AValue: TGocciaValue; const AReceiver: TGocciaValue): Boolean; virtual; - function GetSymbolPropertyWithReceiver(const ASymbol: TGocciaSymbolValue; const AReceiver: TGocciaValue): TGocciaValue; + function GetSymbolPropertyWithReceiver(const ASymbol: TGocciaSymbolValue; const AReceiver: TGocciaValue): TGocciaValue; virtual;Subclasses that already customize property access would then need receiver-aware overrides too.
As per coding guidelines, "Use virtual methods on
TGocciaValuefor polymorphic operations:GetProperty(Name),SetProperty(Name, Value)for property access".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Values.ObjectValue.pas` around lines 53 - 77, The receiver-aware hooks (AssignPropertyWithReceiver, AssignSymbolPropertyWithReceiver, GetPropertyWithContext, GetSymbolPropertyWithReceiver) are currently concrete on TGocciaObjectValue so subclasses cannot customize receiver-aware semantics; change their declarations to be virtual (ideally declared on TGocciaValue or at least marked virtual in TGocciaObjectValue) and keep/adjust existing overrides in subclasses so Reflect.get/Reflect.set routing uses polymorphic overrides; update method signatures for AssignPropertyWithReceiver, AssignSymbolPropertyWithReceiver, GetPropertyWithContext, GetSymbolPropertyWithReceiver to virtual and ensure any subclass implementations use override.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@units/Goccia.Values.ObjectValue.pas`:
- Around line 53-77: The receiver-aware hooks (AssignPropertyWithReceiver,
AssignSymbolPropertyWithReceiver, GetPropertyWithContext,
GetSymbolPropertyWithReceiver) are currently concrete on TGocciaObjectValue so
subclasses cannot customize receiver-aware semantics; change their declarations
to be virtual (ideally declared on TGocciaValue or at least marked virtual in
TGocciaObjectValue) and keep/adjust existing overrides in subclasses so
Reflect.get/Reflect.set routing uses polymorphic overrides; update method
signatures for AssignPropertyWithReceiver, AssignSymbolPropertyWithReceiver,
GetPropertyWithContext, GetSymbolPropertyWithReceiver to virtual and ensure any
subclass implementations use override.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4d973150-75ad-4ce0-a20f-a8cd1092d1c3
📒 Files selected for processing (4)
tests/built-ins/Reflect/get.jstests/built-ins/Reflect/set.jsunits/Goccia.Builtins.GlobalReflect.pasunits/Goccia.Values.ObjectValue.pas
Contributor
Benchmark Results274 benchmarks Interpreted: 🟢 229 improved · 🔴 9 regressed · 36 unchanged · avg +7.5% arraybuffer.js — Interp: 🟢 13, 🔴 1 · avg +10.2% · Bytecode: 🟢 5, 🔴 5, 4 unch. · avg -0.6%
arrays.js — Interp: 🟢 19 · avg +11.1% · Bytecode: 🟢 4, 🔴 4, 11 unch. · avg -0.5%
async-await.js — Interp: 🟢 6 · avg +15.3% · Bytecode: 🔴 6 · avg -5.3%
classes.js — Interp: 🟢 27, 4 unch. · avg +8.6% · Bytecode: 🟢 2, 🔴 2, 27 unch. · avg +0.9%
closures.js — Interp: 🟢 10, 1 unch. · avg +10.2% · Bytecode: 🟢 7, 🔴 2, 2 unch. · avg +2.7%
collections.js — Interp: 🟢 10, 2 unch. · avg +13.7% · Bytecode: 🟢 3, 🔴 4, 5 unch. · avg -0.5%
destructuring.js — Interp: 🟢 14, 🔴 1, 7 unch. · avg +2.8% · Bytecode: 🟢 3, 🔴 14, 5 unch. · avg -1.8%
fibonacci.js — Interp: 🟢 6, 2 unch. · avg +6.4% · Bytecode: 🟢 1, 🔴 6, 1 unch. · avg -2.2%
for-of.js — Interp: 🟢 1, 🔴 1, 5 unch. · avg -0.4% · Bytecode: 🟢 3, 🔴 2, 2 unch. · avg +0.5%
helpers/bench-module.js — Interp: 0 · Bytecode: 0
iterators.js — Interp: 🟢 19, 1 unch. · avg +5.3% · Bytecode: 🟢 6, 🔴 8, 6 unch. · avg -0.2%
json.js — Interp: 🟢 19, 1 unch. · avg +8.3% · Bytecode: 🟢 2, 🔴 4, 14 unch. · avg -1.2%
jsx.jsx — Interp: 🟢 21 · avg +7.3% · Bytecode: 🟢 5, 🔴 8, 8 unch. · avg -0.3%
modules.js — Interp: 🟢 8, 1 unch. · avg +9.4% · Bytecode: 🔴 8, 1 unch. · avg -2.9%
numbers.js — Interp: 🟢 11 · avg +12.9% · Bytecode: 🟢 8, 3 unch. · avg +2.9%
objects.js — Interp: 🟢 5, 2 unch. · avg +6.1% · Bytecode: 🟢 2, 5 unch. · avg +0.2%
promises.js — Interp: 🟢 11, 1 unch. · avg +7.8% · Bytecode: 🟢 1, 🔴 3, 8 unch. · avg -1.0%
regexp.js — Interp: 🟢 8, 3 unch. · avg +4.0% · Bytecode: 🟢 2, 9 unch. · avg +1.2%
strings.js — Interp: 🟢 10, 1 unch. · avg +6.9% · Bytecode: 🟢 2, 🔴 7, 2 unch. · avg -0.6%
typed-arrays.js — Interp: 🟢 11, 🔴 6, 5 unch. · avg +2.5% · Bytecode: 🟢 6, 🔴 3, 13 unch. · avg +0.6%
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 Timing
Measured on ubuntu-latest x64. |
Reflect.get now passes the receiver to getter calls via GetPropertyWithContext (string keys) and GetSymbolPropertyWithReceiver (symbol keys). Reflect.set uses new AssignPropertyWithReceiver / AssignSymbolPropertyWithReceiver methods that implement the ES2026 §10.1.9.2 OrdinarySetWithOwnDescriptor algorithm: accessor setters receive the receiver as this, and data property writes land on the receiver rather than the target. Fixes #230 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Mark GetPropertyWithContext, GetSymbolPropertyWithReceiver, AssignPropertyWithReceiver, and AssignSymbolPropertyWithReceiver as virtual on TGocciaObjectValue so subclasses can customize receiver-aware semantics. Add GetPropertyWithContext overrides to all 14 subclasses that previously overrided GetProperty: each override moves the custom property logic (length, size, byteLength, indices, accessors, etc.) into GetPropertyWithContext and simplifies GetProperty to delegate via GetPropertyWithContext(AName, Self). This ensures Reflect.get routes through polymorphic dispatch for both default and explicit receiver cases. Remove the args-length workaround in ReflectGet — now always uses GetPropertyWithContext since virtual dispatch handles subclasses. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
frostney
force-pushed
the
fix/reflect-get-set-receiver
branch
from
955414d to
e571e40
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
units/Goccia.Values.ProxyValue.pas (1)
198-229:⚠️ Potential issue | 🔴 CriticalReceiver is still dropped in proxy
[[Get]]path.In
GetPropertyWithContext, Line 202 passesSelfto the get trap, and Line 228 falls back toFTarget.GetProperty(AName). Both should useAThisContext; otherwiseReflect.get(proxy, key, receiver)observes the wrongthis.Suggested fix
@@ - Args.Add(Self); + Args.Add(AThisContext); @@ - else - Result := FTarget.GetProperty(AName); + else + Result := FTarget.GetPropertyWithContext(AName, AThisContext);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Values.ProxyValue.pas` around lines 198 - 229, GetPropertyWithContext drops the receiver: it passes Self to InvokeTrap and falls back to FTarget.GetProperty(AName) instead of using the provided receiver; update the calls in GetPropertyWithContext so InvokeTrap is given AThisContext (not Self) as the third arg and the fallback uses FTarget.GetProperty(AName, AThisContext) (or the equivalent overload) so Reflect.get(proxy, key, receiver) and Trap invocation observe the correct receiver; locate these changes around the InvokeTrap call and the FTarget.GetProperty fallback in GetPropertyWithContext.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@units/Goccia.Values.ObjectValue.pas`:
- Around line 599-612: When OwnDesc = nil in AssignPropertyWithReceiver (and
similarly in AssignSymbolPropertyWithReceiver) you must first consult the
receiver's own property before creating a new one: if AReceiver is a
TGocciaObjectValue, call its GetOwnProperty/GetOwnPropertyDescriptor (or
equivalent) to obtain ReceiverDesc; if ReceiverDesc is nil then proceed to
DefineProperty only when ReceiverObj.Extensible is true, but if ReceiverDesc
exists then obey its attributes—update value if it's a writable data property,
return False if it's a non-writable data property, and return False for accessor
properties without a setter—do not unconditionally call DefineProperty; ensure
you reference FPrototype and existing logic for prototype fallthrough and keep
use of TGocciaPropertyDescriptorData and DefineProperty/DefineSymbolProperty
unchanged except guarded by the receiver-own-property checks.
---
Outside diff comments:
In `@units/Goccia.Values.ProxyValue.pas`:
- Around line 198-229: GetPropertyWithContext drops the receiver: it passes Self
to InvokeTrap and falls back to FTarget.GetProperty(AName) instead of using the
provided receiver; update the calls in GetPropertyWithContext so InvokeTrap is
given AThisContext (not Self) as the third arg and the fallback uses
FTarget.GetProperty(AName, AThisContext) (or the equivalent overload) so
Reflect.get(proxy, key, receiver) and Trap invocation observe the correct
receiver; locate these changes around the InvokeTrap call and the
FTarget.GetProperty fallback in GetPropertyWithContext.
🪄 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: 97733d74-d789-4921-92d8-2780ec51fb3e
📒 Files selected for processing (18)
tests/built-ins/Reflect/get.jstests/built-ins/Reflect/set.jsunits/Goccia.Builtins.GlobalReflect.pasunits/Goccia.Values.ArrayBufferValue.pasunits/Goccia.Values.ArrayValue.pasunits/Goccia.Values.BooleanObjectValue.pasunits/Goccia.Values.ClassValue.pasunits/Goccia.Values.FFILibrary.pasunits/Goccia.Values.FFIPointer.pasunits/Goccia.Values.FunctionBase.pasunits/Goccia.Values.MapValue.pasunits/Goccia.Values.NumberObjectValue.pasunits/Goccia.Values.ObjectValue.pasunits/Goccia.Values.PromiseValue.pasunits/Goccia.Values.ProxyValue.pasunits/Goccia.Values.SetValue.pasunits/Goccia.Values.SharedArrayBufferValue.pasunits/Goccia.Values.StringObjectValue.pas
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/built-ins/Reflect/get.js
Proxy GetPropertyWithContext: pass AThisContext (not Self) as the receiver argument to the get trap, and thread the receiver through the no-trap fallback via target.GetPropertyWithContext. This ensures Reflect.get(proxy, key, receiver) observes the correct this in traps. AssignPropertyWithReceiver / AssignSymbolPropertyWithReceiver: when the prototype chain ends (OwnDesc = nil, no prototype), consult Receiver.[[GetOwnProperty]](P) before creating a new property per ES2026 §10.1.9.2 step 1c→2. This correctly returns false when the receiver already has a non-writable data property or an accessor for the same key, and updates an existing writable property in place. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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 `@units/Goccia.Values.ObjectValue.pas`:
- Around line 609-623: The receiver update must go through the boolean
[[DefineOwnProperty]] path instead of mutating descriptors or directly calling
DefineProperty; replace in-place writes (the check for Assigned(ReceiverDesc),
the Writable test, and the assignment TGocciaPropertyDescriptorData.Value :=
AValue) with logic that constructs a new property descriptor reflecting the
desired change and calls ReceiverObj.DefineOwnProperty(AName, NewDesc) (or the
symbol variant) returning its boolean result; likewise, when creating a new
property, check ReceiverObj.Extensible and then call
ReceiverObj.DefineOwnProperty(AName, NewDataDesc) instead of
DefineProperty/DefineSymbolProperty so the proxy
DefineOwnProperty/defineProperty trap behavior and false return/throw semantics
are preserved (apply same fix to the other affected blocks around the
identifiers ReceiverObj.GetOwnPropertyDescriptor, TGocciaPropertyDescriptorData,
ReceiverObj.DefineProperty/DefineSymbolProperty mentioned in the comment).
In `@units/Goccia.Values.ProxyValue.pas`:
- Around line 30-31: TGocciaProxyValue currently overrides
GetPropertyWithContext but misses overrides for the receiver-aware virtuals, so
symbol-key reads and receiver-based writes bypass proxy traps; update
TGocciaProxyValue to also override GetSymbolPropertyWithReceiver,
AssignPropertyWithReceiver, and AssignSymbolPropertyWithReceiver on the
TGocciaValue hierarchy, delegating to the proxy trap logic (or invoking the same
proxy-handling code used by GetPropertyWithContext) and preserving invariant
checks for symbol keys and receiver-aware assignments; ensure each new override
calls the proxy target/trap handling paths rather than the default TGocciaValue
implementations so Reflect.get/set semantics remain correct.
🪄 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: 71c2abcb-dd03-4577-9631-d629383edb26
📒 Files selected for processing (2)
units/Goccia.Values.ObjectValue.pasunits/Goccia.Values.ProxyValue.pas
TGocciaProxyValue now overrides GetSymbolPropertyWithReceiver, AssignPropertyWithReceiver, and AssignSymbolPropertyWithReceiver so that Reflect.get/set with symbol keys and explicit receivers route through proxy traps instead of bypassing them via the base TGocciaObjectValue implementation. AssignPropertyWithReceiver returns false (not throw) when the set trap returns a falsy value, matching the boolean OrdinarySet contract. All three overrides include the same invariant validation as the existing string-key counterparts. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This was referenced Apr 11, 2026
This was referenced Apr 30, 2026
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
Reflect.get: passes the optionalreceiverargument to getter calls viaGetPropertyWithContext(string keys) andGetSymbolPropertyWithReceiver(symbol keys), so accessor getters observe the correctthis(ES2026 §28.1.5)Reflect.set: addsAssignPropertyWithReceiver/AssignSymbolPropertyWithReceiverimplementing the fullOrdinarySetWithOwnDescriptoralgorithm (ES2026 §10.1.9.2) — accessor setters receivereceiverasthis, and data property writes land onreceiverrather thantargetCloses #230
Test plan
🤖 Generated with Claude Code