Fix computed class field yield evaluation#672
Open
frostney wants to merge 10 commits into
Open
Conversation
|
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 (4)
📝 WalkthroughWalkthroughAdds parser/AST metadata, compiler captured-key locals and opcodes, VM/runtime decorator and dynamic-define handling, evaluator replay/initialization for computed class field keys, GC/continuation helpers, and tests for computed public class-field semantics. ChangesComputed Public Class Fields
Sequence DiagramsequenceDiagram
participant Parser
participant Compiler
participant VM
participant Evaluator
Parser->>Compiler: emit element metadata (IsComputed, ComputedKeyExpression, ElementIndex)
Compiler->>Compiler: allocate deterministic computed-key locals and capture key expressions
Compiler->>VM: emit OP_TO_PROPERTY_KEY / OP_DEFINE_PROP_DYNAMIC and computed-key operand slots
Evaluator->>Evaluator: evaluate computed key (may SaveCompletedExpressionValue and suspend on yield)
Evaluator->>Evaluator: on resume TakeCompletedExpressionValue (reuse saved key)
VM->>VM: OP_DEFINE_PROP_DYNAMIC defines property using computed key (symbol/string)
Evaluator->>Evaluator: InitializeInstanceProperties uses resolved keys and runs decorator initializers
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 |
Contributor
Suite TimingTest Runner (interpreted: 9,766 passed; bytecode: 9,766 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: 🟢 54 improved · 🔴 201 regressed · 152 unchanged · avg -2.8% arraybuffer.js — Interp: 🟢 1, 🔴 6, 7 unch. · avg -1.8% · Bytecode: 🟢 7, 🔴 1, 6 unch. · avg +4.1%
arrays.js — Interp: 🔴 5, 14 unch. · avg -2.2% · Bytecode: 🔴 13, 6 unch. · avg -4.6%
async-await.js — Interp: 6 unch. · avg -0.2% · Bytecode: 6 unch. · avg -0.7%
async-generators.js — Interp: 🔴 1, 1 unch. · avg -1.5% · Bytecode: 2 unch. · avg -1.0%
base64.js — Interp: 🟢 7, 3 unch. · avg +6.1% · Bytecode: 🔴 6, 4 unch. · avg -1.9%
classes.js — Interp: 🟢 2, 🔴 13, 16 unch. · avg -2.6% · Bytecode: 🟢 2, 🔴 8, 21 unch. · avg -2.3%
closures.js — Interp: 🔴 9, 2 unch. · avg -4.4% · Bytecode: 🟢 1, 10 unch. · avg -0.3%
collections.js — Interp: 🟢 1, 🔴 3, 8 unch. · avg -0.2% · Bytecode: 12 unch. · avg -0.6%
csv.js — Interp: 🔴 10, 3 unch. · avg -5.4% · Bytecode: 🔴 8, 5 unch. · avg -2.0%
destructuring.js — Interp: 🔴 10, 12 unch. · avg -3.6% · Bytecode: 🟢 1, 🔴 2, 19 unch. · avg -1.0%
fibonacci.js — Interp: 🔴 3, 5 unch. · avg -2.0% · Bytecode: 🟢 2, 6 unch. · avg +3.9%
float16array.js — Interp: 🔴 21, 11 unch. · avg -6.3% · Bytecode: 🟢 4, 🔴 4, 24 unch. · avg +0.1%
for-of.js — Interp: 🔴 6, 1 unch. · avg -4.5% · Bytecode: 🔴 2, 5 unch. · avg -0.2%
generators.js — Interp: 🔴 3, 1 unch. · avg -2.9% · Bytecode: 4 unch. · avg -1.6%
iterators.js — Interp: 🟢 20, 🔴 3, 19 unch. · avg +2.3% · Bytecode: 🔴 28, 14 unch. · avg -5.5%
json.js — Interp: 🔴 17, 3 unch. · avg -6.8% · Bytecode: 🟢 7, 🔴 1, 12 unch. · avg +1.3%
jsx.jsx — Interp: 🔴 17, 4 unch. · avg -6.5% · Bytecode: 🔴 7, 14 unch. · avg -2.8%
modules.js — Interp: 🔴 1, 8 unch. · avg -1.2% · Bytecode: 🟢 5, 4 unch. · avg +2.5%
numbers.js — Interp: 🔴 8, 3 unch. · avg -4.9% · Bytecode: 🔴 3, 8 unch. · avg -2.0%
objects.js — Interp: 🔴 7 · avg -10.4% · Bytecode: 🔴 4, 3 unch. · avg -2.6%
promises.js — Interp: 🟢 1, 11 unch. · avg +1.6% · Bytecode: 🟢 3, 🔴 1, 8 unch. · avg +2.3%
regexp.js — Interp: 🟢 6, 🔴 5 · avg +2.0% · Bytecode: 🟢 1, 10 unch. · avg +1.4%
strings.js — Interp: 🔴 17, 2 unch. · avg -11.2% · Bytecode: 🟢 2, 🔴 7, 10 unch. · avg -2.3%
tsv.js — Interp: 🔴 9 · avg -8.7% · Bytecode: 🟢 2, 7 unch. · avg +0.6%
typed-arrays.js — Interp: 🟢 3, 🔴 12, 7 unch. · avg -13.7% · Bytecode: 🟢 7, 🔴 3, 12 unch. · avg +16.5%
uint8array-encoding.js — Interp: 🟢 7, 🔴 8, 3 unch. · avg +0.4% · Bytecode: 🟢 1, 🔴 14, 3 unch. · avg -26.4%
weak-collections.js — Interp: 🟢 6, 🔴 7, 2 unch. · avg +17.3% · Bytecode: 🟢 3, 🔴 5, 7 unch. · avg +7.3%
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 (+227 / -0)Newly passing (227):
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 |
frostney
force-pushed
the
issue-652-computed-field-yield
branch
3 times, most recently
from
6535227 to
5047e18
Compare
Resolve computed public class field names during class definition so yield resumes correctly for instance and static fields in both interpreted and bytecode execution.
frostney
force-pushed
the
issue-652-computed-field-yield
branch
from
5047e18 to
6d5fe06
Compare
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: 1
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 (2)
4413-4424:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMerge an existing symbol setter when emitting a computed getter.
The symbol-keyed getter path always overwrites the descriptor with a getter-only accessor. That means
set [s](...) {}followed byget [s]() {}loses the setter, even though the setter path below already does the symmetric merge in the opposite order.Suggested fix
cekGetter: begin GetterFunction := BuildClassGetter(Elem.GetterNode); if ComputedKey is TGocciaSymbolValue then begin - if Elem.IsStatic then - ClassValue.DefineSymbolProperty( - TGocciaSymbolValue(ComputedKey), - TGocciaPropertyDescriptorAccessor.Create(GetterFunction, nil, [pfConfigurable])) - else - ClassValue.Prototype.DefineSymbolProperty( - TGocciaSymbolValue(ComputedKey), - TGocciaPropertyDescriptorAccessor.Create(GetterFunction, nil, [pfConfigurable])); + if Elem.IsStatic then + ExistingDescriptor := ClassValue.GetOwnStaticSymbolDescriptor( + TGocciaSymbolValue(ComputedKey)) + else + ExistingDescriptor := ClassValue.Prototype.GetOwnSymbolPropertyDescriptor( + TGocciaSymbolValue(ComputedKey)); + + if (ExistingDescriptor is TGocciaPropertyDescriptorAccessor) and + Assigned(TGocciaPropertyDescriptorAccessor(ExistingDescriptor).Setter) then + begin + if Elem.IsStatic then + ClassValue.DefineSymbolProperty( + TGocciaSymbolValue(ComputedKey), + TGocciaPropertyDescriptorAccessor.Create( + GetterFunction, + TGocciaPropertyDescriptorAccessor(ExistingDescriptor).Setter, + [pfConfigurable])) + else + ClassValue.Prototype.DefineSymbolProperty( + TGocciaSymbolValue(ComputedKey), + TGocciaPropertyDescriptorAccessor.Create( + GetterFunction, + TGocciaPropertyDescriptorAccessor(ExistingDescriptor).Setter, + [pfConfigurable])); + end + else if Elem.IsStatic then + ClassValue.DefineSymbolProperty( + TGocciaSymbolValue(ComputedKey), + TGocciaPropertyDescriptorAccessor.Create( + GetterFunction, nil, [pfConfigurable])) + else + ClassValue.Prototype.DefineSymbolProperty( + TGocciaSymbolValue(ComputedKey), + TGocciaPropertyDescriptorAccessor.Create( + GetterFunction, nil, [pfConfigurable])); endAlso applies to: 4444-4469
🤖 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 4413 - 4424, When emitting a computed symbol getter in the GetterFunction path (built by BuildClassGetter) do not unconditionally overwrite existing symbol accessor descriptors; instead fetch the current descriptor from ClassValue (or ClassValue.Prototype when Elem.IsStatic is false) for the TGocciaSymbolValue(ComputedKey), detect if it's an existing TGocciaPropertyDescriptorAccessor with a setter, and construct the new TGocciaPropertyDescriptorAccessor reusing the existing setter and preserving flags (e.g., pfConfigurable) so the setter is not lost; replace the direct DefineSymbolProperty call in the GetterFunction branch with logic that merges the existing setter into the new getter accessor (mirroring the merge already done in the setter path).
3529-3559:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUnify the field-order path for all public instance fields.
Only the new computed branch materializes declaration-only fields as
undefinedand uses own-property definition. The fallback public branch still skipsx;when there is no initializer and still usesAssignProperty, so mixed classes likeclass C { [k]; x; }will dropx, andx = 1still behaves differently from the computed branch by going through assignment semantics.Suggested direction
- else - begin - if AClassValue.InstancePropertyDefs.TryGetValue(FOEntry.Name, Expr) and Assigned(Expr) then - begin - PropertyValue := EvaluateExpression(Expr, LocalContext); - AInstance.AssignProperty(FOEntry.Name, PropertyValue); - end; - end; + else + begin + if Assigned(FOEntry.Initializer) then + PropertyValue := EvaluateExpression(FOEntry.Initializer, LocalContext) + else + PropertyValue := TGocciaUndefinedLiteralValue.UndefinedValue; + AInstance.DefineProperty( + FOEntry.Name, + TGocciaPropertyDescriptorData.Create( + PropertyValue, [pfEnumerable, pfConfigurable, pfWritable])); + end;Apply the same shape in
InitializeObjectInstanceProperties, usingAContextthere.Also applies to: 3609-3640
🤖 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 3529 - 3559, The public-instance-field fallback path should mirror the computed-field branch: when handling FOEntry that is not computed and not private, materialize declaration-only fields as TGocciaUndefinedLiteralValue.UndefinedValue (instead of skipping them) and define them as own-properties with AInstance.DefineSymbolProperty or AInstance.DefineProperty (matching the computed-key handling) rather than calling AInstance.AssignProperty; update the logic in InitializeObjectInstanceProperties to use AContext and the same shape as the computed branch so that fields like `x;` are created as undefined and initializer-bearing fields still evaluate via EvaluateExpression(LocalContext/AContext) before defining the property. Ensure you reference FOEntry.ComputedKey / TGocciaSymbolValue, FOEntry.Name, AClassValue.InstancePropertyDefs, EvaluateExpression, DefineSymbolProperty, DefineProperty, AssignProperty (remove/replace), and SetPrivateProperty accordingly.
🧹 Nitpick comments (1)
source/units/Goccia.Compiler.Statements.pas (1)
3825-3844: 💤 Low valueConsider using
EncodeABCfor consistency with existingOP_LOAD_UNDEFINEDusage.Line 3830 uses
EncodeABx(OP_LOAD_UNDEFINED, ValReg, 0), but the existing codebase usesEncodeABC(OP_LOAD_UNDEFINED, ..., 0, 0)(e.g., lines 702, 1714). Both work functionally since the extra fields are zero, butEncodeABCwould be more consistent.Suggested fix for consistency
- EmitInstruction(ChildCtx, EncodeABx(OP_LOAD_UNDEFINED, ValReg, 0)); + EmitInstruction(ChildCtx, EncodeABC(OP_LOAD_UNDEFINED, ValReg, 0, 0));🤖 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.Statements.pas` around lines 3825 - 3844, Replace the inconsistent EncodeABx usage with EncodeABC for OP_LOAD_UNDEFINED: in the block inside the loop where OP_LOAD_UNDEFINED is emitted (use of EncodeABx(OP_LOAD_UNDEFINED, ValReg, 0)), change it to call EncodeABC(OP_LOAD_UNDEFINED, ValReg, 0, 0) so it matches other sites (e.g., lines that use EncodeABC(OP_LOAD_UNDEFINED,...)), keeping the same ValReg operand and zero immediates; this maintains consistency with existing EncodeABC patterns while preserving 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.Evaluator.pas`:
- Around line 4483-4500: The decorator/initializer paths must use the resolved
computed keys the same way the field-order code does: when plumbing metadata or
attaching initializers for AClassDef.FFieldOrder entries, check
FieldOrderEntries[I].IsComputed and AClassDef.FFieldOrder[I].ElementIndex and,
if in range, use
ResolvedComputedFieldKeys[AClassDef.FFieldOrder[I].ElementIndex] instead of
falling back to Elem.Name; propagate that resolved key into whatever code
records decorator metadata/initializer associations (the same places you call
ClassValue.SetFieldOrder and the decorator pass logic referenced at the other
spots), so decorated computed fields receive the actual key (including symbol
keys) when creating metadata or binding initializers.
---
Outside diff comments:
In `@source/units/Goccia.Evaluator.pas`:
- Around line 4413-4424: When emitting a computed symbol getter in the
GetterFunction path (built by BuildClassGetter) do not unconditionally overwrite
existing symbol accessor descriptors; instead fetch the current descriptor from
ClassValue (or ClassValue.Prototype when Elem.IsStatic is false) for the
TGocciaSymbolValue(ComputedKey), detect if it's an existing
TGocciaPropertyDescriptorAccessor with a setter, and construct the new
TGocciaPropertyDescriptorAccessor reusing the existing setter and preserving
flags (e.g., pfConfigurable) so the setter is not lost; replace the direct
DefineSymbolProperty call in the GetterFunction branch with logic that merges
the existing setter into the new getter accessor (mirroring the merge already
done in the setter path).
- Around line 3529-3559: The public-instance-field fallback path should mirror
the computed-field branch: when handling FOEntry that is not computed and not
private, materialize declaration-only fields as
TGocciaUndefinedLiteralValue.UndefinedValue (instead of skipping them) and
define them as own-properties with AInstance.DefineSymbolProperty or
AInstance.DefineProperty (matching the computed-key handling) rather than
calling AInstance.AssignProperty; update the logic in
InitializeObjectInstanceProperties to use AContext and the same shape as the
computed branch so that fields like `x;` are created as undefined and
initializer-bearing fields still evaluate via
EvaluateExpression(LocalContext/AContext) before defining the property. Ensure
you reference FOEntry.ComputedKey / TGocciaSymbolValue, FOEntry.Name,
AClassValue.InstancePropertyDefs, EvaluateExpression, DefineSymbolProperty,
DefineProperty, AssignProperty (remove/replace), and SetPrivateProperty
accordingly.
---
Nitpick comments:
In `@source/units/Goccia.Compiler.Statements.pas`:
- Around line 3825-3844: Replace the inconsistent EncodeABx usage with EncodeABC
for OP_LOAD_UNDEFINED: in the block inside the loop where OP_LOAD_UNDEFINED is
emitted (use of EncodeABx(OP_LOAD_UNDEFINED, ValReg, 0)), change it to call
EncodeABC(OP_LOAD_UNDEFINED, ValReg, 0, 0) so it matches other sites (e.g.,
lines that use EncodeABC(OP_LOAD_UNDEFINED,...)), keeping the same ValReg
operand and zero immediates; this maintains consistency with existing EncodeABC
patterns while preserving behavior.
🪄 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: 2f9e68d3-07cc-46f8-abad-a2722b8061dd
📒 Files selected for processing (11)
source/units/Goccia.AST.Statements.passource/units/Goccia.Bytecode.OpCodeNames.passource/units/Goccia.Bytecode.passource/units/Goccia.Compiler.Statements.passource/units/Goccia.Evaluator.passource/units/Goccia.Parser.passource/units/Goccia.VM.passource/units/Goccia.Values.ClassValue.pastests/language/classes/class-computed-field-yield-order.jstests/language/function-keyword/async-function.jstests/language/function-keyword/class-computed-field-yield.js
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 (2)
source/units/Goccia.VM.pas (1)
5981-6220:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse symbol-aware property access for decorated computed keys.
When
AComputedKeyis a symbol, this method records it inElementKeybut still reads and rewrites the element throughElementNamestring APIs. That makes decorated[@@sym]methods/getters/setters/accessors target a string-named property instead of the symbol slot, andcontext.accesspoints at the wrong member. Route those branches through the symbol-specific getters/setters/define paths wheneverElementKey is TGocciaSymbolValue.🤖 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.VM.pas` around lines 5981 - 6220, The code records symbol computed keys into ElementKey but continues to use ElementName/string-based APIs (e.g., calls to TGocciaClassValue.GetProperty, Prototype.GetOwnPropertyDescriptor, SetProperty, DefineProperty, AddStaticGetter/AddStaticSetter, AddGetter/AddSetter, AddFieldInitializerWithKey) which causes decorated symbol-keyed members to be read/written by name; update all branches that access or modify the element (the places that set ElementValue, call GetProperty/GetOwnPropertyDescriptor/DefineProperty, SetProperty, and the Add... methods when applying the decorator result) to check if ElementKey is a TGocciaSymbolValue and, when so, use the symbol-aware APIs (symbol overloads or the symbol-specific getters/setters/define methods on TGocciaClassValue/Prototype and AddFieldInitializerWithKey with ElementKey) instead of the string-based ElementName paths so the decorator targets the symbol slot and context.access points to the correct member.source/units/Goccia.Compiler.Statements.pas (1)
3720-3728:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake the computed-field key local unique per class.
Line 3723 derives the hidden local name from only
I. A second computed-field class in the same compiler scope will reuse names like#computed-field-key:0, and the laterResolveLocal/ResolveUpvaluecalls inCompileFieldInitializer, static field emission, and decorator orchestration can bind the wrong captured key.Illustrative fix
- ComputedKeyName := Format('`#computed-field-key`:%d', [I]); + ComputedKeyName := Format('`#computed-field-key`:%d:%d', + [CurrentCodePosition(ACtx), I]);🤖 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.Statements.pas` around lines 3720 - 3728, The computed-field hidden local name is only based on I (ComputedKeyName := Format('`#computed-field-key`:%d',[I])) so different classes in the same compiler scope can collide; change the name generation in the cekField branch to include a class-unique identifier (for example the current class symbol/name or scope/class id accessible from ACtx or the element owner) so ComputedKeyName becomes unique per class before calling ACtx.Scope.DeclareLocal; update any related logic that expects that name (CompileFieldInitializer, ResolveLocal/ResolveUpvalue, static field emission and decorator orchestration) to rely on the new class-scoped ComputedKeyName.
♻️ Duplicate comments (1)
source/units/Goccia.Evaluator.pas (1)
4625-4643:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftA related computed-key decorator gap is still open.
ResolvedComputedFieldKeysis field-only, but this path now consults it for every computed decorated element. That leavesElementKey = nilfor computed methods/getters/setters/accessors, socontext.nameand initializer bookkeeping still fall back toElem.Name. For symbol-keyed computed fields/accessors,context.accessalso stays string-based becauseElementNameis never updated from the resolved key. Please carry a per-element resolved key through this pass and use it consistently forcontext.name,context.access, andAddFieldInitializerWithKey(...).Also applies to: 4801-4801, 4830-4830
🤖 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 4625 - 4643, The code only reads ResolvedComputedFieldKeys (a field-only array) and sets ElementKey/ElementName for computed fields, leaving computed methods/getters/setters/accessors with nil ElementKey and string ElementName; fix by carrying a per-element resolved key (e.g., local PerElementResolvedKey) for every Elem when you detect Elem.IsComputed (not just field elements), populate it from ResolvedComputedFieldKeys when available or from the element's resolved symbol value when the element is a symbol-keyed computed accessor/method, and then use that PerElementResolvedKey everywhere you currently use ElementKey/ElementName — update ContextObject.AssignProperty(PROP_NAME,...), the context.access assignment, and the call to AddFieldInitializerWithKey(...) to consume the per-element resolved key instead of falling back to Elem.Name so symbol-keyed computed accessors/methods correctly carry symbol keys through.
🤖 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.Compiler.Statements.pas`:
- Around line 3720-3728: The computed-field hidden local name is only based on I
(ComputedKeyName := Format('`#computed-field-key`:%d',[I])) so different classes
in the same compiler scope can collide; change the name generation in the
cekField branch to include a class-unique identifier (for example the current
class symbol/name or scope/class id accessible from ACtx or the element owner)
so ComputedKeyName becomes unique per class before calling
ACtx.Scope.DeclareLocal; update any related logic that expects that name
(CompileFieldInitializer, ResolveLocal/ResolveUpvalue, static field emission and
decorator orchestration) to rely on the new class-scoped ComputedKeyName.
In `@source/units/Goccia.VM.pas`:
- Around line 5981-6220: The code records symbol computed keys into ElementKey
but continues to use ElementName/string-based APIs (e.g., calls to
TGocciaClassValue.GetProperty, Prototype.GetOwnPropertyDescriptor, SetProperty,
DefineProperty, AddStaticGetter/AddStaticSetter, AddGetter/AddSetter,
AddFieldInitializerWithKey) which causes decorated symbol-keyed members to be
read/written by name; update all branches that access or modify the element (the
places that set ElementValue, call
GetProperty/GetOwnPropertyDescriptor/DefineProperty, SetProperty, and the Add...
methods when applying the decorator result) to check if ElementKey is a
TGocciaSymbolValue and, when so, use the symbol-aware APIs (symbol overloads or
the symbol-specific getters/setters/define methods on
TGocciaClassValue/Prototype and AddFieldInitializerWithKey with ElementKey)
instead of the string-based ElementName paths so the decorator targets the
symbol slot and context.access points to the correct member.
---
Duplicate comments:
In `@source/units/Goccia.Evaluator.pas`:
- Around line 4625-4643: The code only reads ResolvedComputedFieldKeys (a
field-only array) and sets ElementKey/ElementName for computed fields, leaving
computed methods/getters/setters/accessors with nil ElementKey and string
ElementName; fix by carrying a per-element resolved key (e.g., local
PerElementResolvedKey) for every Elem when you detect Elem.IsComputed (not just
field elements), populate it from ResolvedComputedFieldKeys when available or
from the element's resolved symbol value when the element is a symbol-keyed
computed accessor/method, and then use that PerElementResolvedKey everywhere you
currently use ElementKey/ElementName — update
ContextObject.AssignProperty(PROP_NAME,...), the context.access assignment, and
the call to AddFieldInitializerWithKey(...) to consume the per-element resolved
key instead of falling back to Elem.Name so symbol-keyed computed
accessors/methods correctly carry symbol keys through.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7e51e346-6040-4025-9fc0-b750fcd224e9
📒 Files selected for processing (5)
source/units/Goccia.Compiler.Statements.passource/units/Goccia.Evaluator.passource/units/Goccia.VM.passource/units/Goccia.Values.ClassValue.pastests/language/decorators/computed-field-decorator.js
✅ Files skipped from review due to trivial changes (1)
- tests/language/decorators/computed-field-decorator.js
- Preserve computed property keys across fields, methods, getters, and setters - Support symbol keys in decorator access helpers - Add regression coverage for computed field decorators
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 (3)
source/units/Goccia.Compiler.Statements.pas (1)
3724-3767:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize decorated computed method/accessor keys before storing them.
Only fields go through
OP_TO_PROPERTY_KEYhere. For computed methods/getters/setters with decorators, the hidden local keeps the raw expression result, so decoratorcontext.namecan be1instead of"1"or even the original object instead of the resolved key. Coerce the key before preserving it whenever a later decorator pass reuses that local.Possible fix
ACtx.CompileExpression(Elem.ComputedKeyExpression, KeyReg); + if (Elem.Kind = cekField) or NeedsKeyLocal then + EmitInstruction(ACtx, EncodeABC(OP_TO_PROPERTY_KEY, KeyReg, KeyReg, 0)); case Elem.Kind of cekField: begin - // ES2026 §15.7.10 ClassFieldDefinitionEvaluation evaluates - // ClassElementName, including ToPropertyKey, during class definition. - EmitInstruction(ACtx, EncodeABC(OP_TO_PROPERTY_KEY, KeyReg, KeyReg, 0)); Continue; 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.Compiler.Statements.pas` around lines 3724 - 3767, Computed keys for decorated computed methods/getters/setters are not being coerced to property keys before being stored in the hidden local, so decorator context.name may see raw non-string values; ensure you run the same ToPropertyKey conversion used for fields (OP_TO_PROPERTY_KEY) on KeyReg before writing it to the preserved local when Elem.Decorators is non-empty. Modify the branch that allocates/declares KeyReg and the code path after ACtx.CompileExpression(Elem.ComputedKeyExpression, KeyReg) to emit EncodeABC(OP_TO_PROPERTY_KEY, KeyReg, KeyReg, 0) for any Elem.Kind with decorators (not just cekField), so the value stored in AComputedFieldKeyLocals (and used by CompileComputedGetterBody/CompileComputedSetterBody/CompileComputedMethodBody) is the normalized property key.source/units/Goccia.VM.pas (1)
8309-8347:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove
SetBytecodeHomeObjectcall fromOP_DEFINE_PROP_DYNAMIC—it should mirror the non-home-object data-property semantics.
OP_DEFINE_DATA_PROPintentionally callsDefineDataPropertyByKeyInternal(..., False)to define plain data properties without setting[[HomeObject]], butOP_DEFINE_PROP_DYNAMICexplicitly callsSetBytecodeHomeObjectbefore property definition. This creates an observable semantic difference where dynamically defined properties receive home-object treatment (changing function semantics) while statically defined data properties do not. Plain data properties should not attach[[HomeObject]]; that is reserved for concise methods viaDefineMethodPropertyByKey. Either remove theSetBytecodeHomeObjectcall or refactor to useDefineDataPropertyByKeyconsistently.🤖 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.VM.pas` around lines 8309 - 8347, In OP_DEFINE_PROP_DYNAMIC remove the call to SetBytecodeHomeObject so dynamic data-property definition matches OP_DEFINE_DATA_PROP (i.e., do not attach [[HomeObject]]); specifically, delete the SetBytecodeHomeObject(RightValue, TargetValue) invocation in the OP_DEFINE_PROP_DYNAMIC branch and ensure the subsequent property-definition paths use the same data-property APIs (DefineProperty / DefineSymbolProperty or the existing DefineDataPropertyByKeyInternal semantics) rather than treating the value as a method — keep DefineMethodPropertyByKey only for concise method cases.source/units/Goccia.Evaluator.pas (1)
4455-4472:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftInclude computed auto-accessors in the resolved-key path.
cekAccessoris skipped here, but the later accessor/decorator code readsResolvedComputedElementKeys[I]and otherwise falls back toElem.Name. That leavesaccessor [yield expr]outside the computed-name suspend/resume flow and wires computed accessors/decorators against the placeholder name instead of the actual property key. Please resolve/store accessor keys here and carry that key into the auto-accessor registration path too.Also applies to: 4592-4604
🤖 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 4455 - 4472, The code currently skips cekAccessor so computed auto-accessor names are never resolved/stored; update the kind check in the block using Elem.Kind to include cekAccessor (so it reads like including cekAccessor alongside cekMethod/getter/setter/field), then perform the same Continuation.TakeExpressionValue/EvaluateExpression/ToPropertyKey and Continuation.SaveExpressionValue flow to produce ComputedKey and assign ResolvedComputedElementKeys[I] for accessor elements as done for other kinds; also apply the same change to the similar block around the later location (the 4592-4604 area) so the auto-accessor registration path consumes ResolvedComputedElementKeys[I] instead of falling back to Elem.Name. Ensure you reference and preserve the existing symbols: Elem.ComputedKeyExpression, ComputedKey, ToPropertyKey, EvaluateExpression, Continuation.TakeExpressionValue, Continuation.SaveExpressionValue, and ResolvedComputedElementKeys.
🤖 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.Evaluator.Decorators.pas`:
- Around line 99-105: The current branches only handle TGocciaObjectValue for
symbol-keyed access (checking FPropertyKey is TGocciaSymbolValue) and return
undefined for non-object receivers; update the dispatch so that when
FPropertyKey is a TGocciaSymbolValue and the receiver (Target) is a
TGocciaClassValue you call TGocciaClassValue(GetSymbolProperty...) to obtain the
symbol property, otherwise preserve the existing TGocciaObjectValue call; apply
the same fix to the analogous get/set branch around the 149-155 region so static
(class) symbol properties are correctly read/written via
TGocciaClassValue.GetSymbolProperty rather than falling through to undefined or
skipping writes.
In `@source/units/Goccia.VM.pas`:
- Around line 5986-6053: DefineDecoratedDataProperty and
DefineDecoratedAccessorProperty currently duplicate computed-key property
installation and bypass the helpers that set bytecode home, causing decorated
methods/getters/setters to have different super resolution; update these
procedures to call the key-aware helpers (DefineMethodPropertyByKey,
DefineGetterPropertyByKey, DefineSetterPropertyByKey) instead of inlining
DefineSymbolProperty/DefineProperty logic, and ensure the helpers (or these call
sites) invoke SetBytecodeHomeObject for function/getter/setter values just like
OP_DEFINE_PROP_DYNAMIC does (and align OP_DEFINE_DATA_PROP semantics
accordingly).
---
Outside diff comments:
In `@source/units/Goccia.Compiler.Statements.pas`:
- Around line 3724-3767: Computed keys for decorated computed
methods/getters/setters are not being coerced to property keys before being
stored in the hidden local, so decorator context.name may see raw non-string
values; ensure you run the same ToPropertyKey conversion used for fields
(OP_TO_PROPERTY_KEY) on KeyReg before writing it to the preserved local when
Elem.Decorators is non-empty. Modify the branch that allocates/declares KeyReg
and the code path after ACtx.CompileExpression(Elem.ComputedKeyExpression,
KeyReg) to emit EncodeABC(OP_TO_PROPERTY_KEY, KeyReg, KeyReg, 0) for any
Elem.Kind with decorators (not just cekField), so the value stored in
AComputedFieldKeyLocals (and used by
CompileComputedGetterBody/CompileComputedSetterBody/CompileComputedMethodBody)
is the normalized property key.
In `@source/units/Goccia.Evaluator.pas`:
- Around line 4455-4472: The code currently skips cekAccessor so computed
auto-accessor names are never resolved/stored; update the kind check in the
block using Elem.Kind to include cekAccessor (so it reads like including
cekAccessor alongside cekMethod/getter/setter/field), then perform the same
Continuation.TakeExpressionValue/EvaluateExpression/ToPropertyKey and
Continuation.SaveExpressionValue flow to produce ComputedKey and assign
ResolvedComputedElementKeys[I] for accessor elements as done for other kinds;
also apply the same change to the similar block around the later location (the
4592-4604 area) so the auto-accessor registration path consumes
ResolvedComputedElementKeys[I] instead of falling back to Elem.Name. Ensure you
reference and preserve the existing symbols: Elem.ComputedKeyExpression,
ComputedKey, ToPropertyKey, EvaluateExpression,
Continuation.TakeExpressionValue, Continuation.SaveExpressionValue, and
ResolvedComputedElementKeys.
In `@source/units/Goccia.VM.pas`:
- Around line 8309-8347: In OP_DEFINE_PROP_DYNAMIC remove the call to
SetBytecodeHomeObject so dynamic data-property definition matches
OP_DEFINE_DATA_PROP (i.e., do not attach [[HomeObject]]); specifically, delete
the SetBytecodeHomeObject(RightValue, TargetValue) invocation in the
OP_DEFINE_PROP_DYNAMIC branch and ensure the subsequent property-definition
paths use the same data-property APIs (DefineProperty / DefineSymbolProperty or
the existing DefineDataPropertyByKeyInternal semantics) rather than treating the
value as a method — keep DefineMethodPropertyByKey only for concise method
cases.
🪄 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: 0bc6e272-b500-4052-90c0-04221e6e4470
📒 Files selected for processing (5)
source/units/Goccia.Compiler.Statements.passource/units/Goccia.Evaluator.Decorators.passource/units/Goccia.Evaluator.passource/units/Goccia.VM.pastests/language/decorators/computed-field-decorator.js
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)
4463-4470:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the completed-expression cache for resolved computed keys.
Line 4464/4470 stores a fully resolved computed key in the partial-expression channel, but that channel is also used for in-progress state on the same AST node. For a key like
[a + (yield 1)],EvaluateBinarycan resume with the saved left operand, and this code will incorrectly treat that partial value as the final property key. The replay and teardown here should use the completed-expression channel instead ofTakeExpressionValue/SaveExpressionValue/ClearExpressionValue.Based on learnings: GocciaScript keeps partial-expression and completed-expression continuation caches separate; conflating them breaks cases like
a + (yield b)on resume.Also applies to: 5106-5111
🤖 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 4463 - 4470, The code is saving/reading a fully resolved computed key into the partial-expression continuation channel (using Continuation.TakeExpressionValue / SaveExpressionValue / ClearExpressionValue), which conflates in-progress state with completed values; change these to use the completed-expression continuation cache APIs (use the corresponding TakeCompletedExpressionValue / SaveCompletedExpressionValue / ClearCompletedExpressionValue or whatever the completed-expression equivalents are) when handling Elem.ComputedKeyExpression so that ToPropertyKey(EvaluateExpression(...)) reads/writes the completed-expression slot; apply the same replacement for the other occurrence around lines 5106-5111.
🤖 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 4463-4470: The code is saving/reading a fully resolved computed
key into the partial-expression continuation channel (using
Continuation.TakeExpressionValue / SaveExpressionValue / ClearExpressionValue),
which conflates in-progress state with completed values; change these to use the
completed-expression continuation cache APIs (use the corresponding
TakeCompletedExpressionValue / SaveCompletedExpressionValue /
ClearCompletedExpressionValue or whatever the completed-expression equivalents
are) when handling Elem.ComputedKeyExpression so that
ToPropertyKey(EvaluateExpression(...)) reads/writes the completed-expression
slot; apply the same replacement for the other occurrence around lines
5106-5111.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4d33e1ff-ee86-46f6-8c8c-ec1ec3fddf7a
📒 Files selected for processing (11)
source/units/Goccia.Bytecode.OpCodeNames.passource/units/Goccia.Bytecode.passource/units/Goccia.Compiler.Statements.passource/units/Goccia.Evaluator.Decorators.passource/units/Goccia.Evaluator.passource/units/Goccia.Parser.passource/units/Goccia.VM.passource/units/Goccia.Values.ClassValue.pastests/language/decorators/auto-accessor-decorator.jstests/language/decorators/auto-accessor.jstests/language/decorators/computed-field-decorator.js
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 (2)
source/units/Goccia.Values.ClassValue.pas (1)
722-729:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftStatic decorator field initializers are enqueued but never executed.
Lines 851–852 show
RunDecoratorFieldInitializersexplicitly skips entries whereIsStatic=True. However, call sites inGoccia.Evaluator.pas(lines 4976, 5021) unconditionally passElem.IsStatictoAddFieldInitializerWithKey, meaning static decorated fields are being queued. The class provides no corresponding static execution method, so these initializers will never run.🤖 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.Values.ClassValue.pas` around lines 722 - 729, TGocciaClassValue currently queues decorator field initializers (AddFieldInitializerWithKey) even when AIsStatic=True but RunDecoratorFieldInitializers skips static entries, so static initializers never run; fix by either (A) filtering static entries out in AddFieldInitializerWithKey (return immediately when AIsStatic is true) so only instance initializers are queued, or (B) support static initializers properly by adding a dedicated storage (e.g., FStaticDecoratorFieldInitializers), a new runner method RunDecoratorStaticFieldInitializers, and ensure call sites (those in Goccia.Evaluator.pas that pass Elem.IsStatic) trigger that runner for static fields; update references to TGocciaClassValue.AddFieldInitializerWithKey and RunDecoratorFieldInitializers accordingly.source/units/Goccia.VM.pas (1)
6004-6066:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep decorated class elements non-enumerable.
These helpers now route decorated class methods/getters/setters through the generic
*ByKeydefiners, but those install[pfEnumerable]descriptors. That changesObject.keys/for...inbehavior for decorated class elements and diverges from the non-decorated class method path in this file. Preserve the computed-key/home-object logic here, but use class-element descriptor flags instead ([pfConfigurable, pfWritable]for methods and[pfConfigurable]for accessors).🤖 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.VM.pas` around lines 6004 - 6066, The decorated-element helpers currently call the generic "*ByKey" definers which set pfEnumerable; update DefineDecoratedMethodProperty, DefineDecoratedGetterProperty and DefineDecoratedSetterProperty so decorated class elements are non-enumerable by using the class-element descriptor flags: for methods set [pfConfigurable, pfWritable] and for accessors set [pfConfigurable]. Concretely, change the calls inside DefineDecoratedMethodProperty (currently calling DefineMethodPropertyByKey(TargetValue, ValueToRegister(KeyValue), AValue)) to the variant that accepts/sets descriptor flags (or add a flag parameter) so the created descriptor uses pfConfigurable+pfWritable; likewise change DefineDecoratedGetterProperty (currently calling DefineStaticGetterPropertyByKey/DefineGetterPropertyByKey) and DefineDecoratedSetterProperty (currently calling DefineStaticSetterPropertyByKey/DefineSetterPropertyByKey) to use the accessor descriptor flag pfConfigurable and ensure the key handling (ValueToRegister/creating TGocciaStringLiteralValue) and class vs prototype target logic remain unchanged.
🤖 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.Evaluator.Decorators.pas`:
- Around line 153-165: The branch that handles non-symbol FPropertyKey currently
calls TGocciaObjectValue.AssignProperty and thus bypasses class-specific logic;
change it so that when Target is TGocciaClassValue you call
TGocciaClassValue(Target).SetProperty(FPropertyName, NewValue) (or the
appropriate SetProperty signature) instead of TGocciaObjectValue.AssignProperty,
while keeping the existing AssignSymbolProperty and AssignProperty calls for the
other cases (symbols and plain objects); update the logic around Target,
FPropertyKey, FPropertyName, TGocciaClassValue and TGocciaObjectValue to ensure
class writes go through TGocciaClassValue.SetProperty.
---
Outside diff comments:
In `@source/units/Goccia.Values.ClassValue.pas`:
- Around line 722-729: TGocciaClassValue currently queues decorator field
initializers (AddFieldInitializerWithKey) even when AIsStatic=True but
RunDecoratorFieldInitializers skips static entries, so static initializers never
run; fix by either (A) filtering static entries out in
AddFieldInitializerWithKey (return immediately when AIsStatic is true) so only
instance initializers are queued, or (B) support static initializers properly by
adding a dedicated storage (e.g., FStaticDecoratorFieldInitializers), a new
runner method RunDecoratorStaticFieldInitializers, and ensure call sites (those
in Goccia.Evaluator.pas that pass Elem.IsStatic) trigger that runner for static
fields; update references to TGocciaClassValue.AddFieldInitializerWithKey and
RunDecoratorFieldInitializers accordingly.
In `@source/units/Goccia.VM.pas`:
- Around line 6004-6066: The decorated-element helpers currently call the
generic "*ByKey" definers which set pfEnumerable; update
DefineDecoratedMethodProperty, DefineDecoratedGetterProperty and
DefineDecoratedSetterProperty so decorated class elements are non-enumerable by
using the class-element descriptor flags: for methods set [pfConfigurable,
pfWritable] and for accessors set [pfConfigurable]. Concretely, change the calls
inside DefineDecoratedMethodProperty (currently calling
DefineMethodPropertyByKey(TargetValue, ValueToRegister(KeyValue), AValue)) to
the variant that accepts/sets descriptor flags (or add a flag parameter) so the
created descriptor uses pfConfigurable+pfWritable; likewise change
DefineDecoratedGetterProperty (currently calling
DefineStaticGetterPropertyByKey/DefineGetterPropertyByKey) and
DefineDecoratedSetterProperty (currently calling
DefineStaticSetterPropertyByKey/DefineSetterPropertyByKey) to use the accessor
descriptor flag pfConfigurable and ensure the key handling
(ValueToRegister/creating TGocciaStringLiteralValue) and class vs prototype
target logic remain unchanged.
🪄 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: 417ae3f2-912b-4dd2-99ee-5a9beae656de
📒 Files selected for processing (12)
source/units/Goccia.Bytecode.OpCodeNames.passource/units/Goccia.Bytecode.passource/units/Goccia.Compiler.Statements.passource/units/Goccia.Evaluator.Decorators.passource/units/Goccia.Evaluator.passource/units/Goccia.Generator.Continuation.passource/units/Goccia.Parser.passource/units/Goccia.VM.passource/units/Goccia.Values.ClassValue.pastests/language/decorators/auto-accessor-decorator.jstests/language/decorators/auto-accessor.jstests/language/decorators/computed-field-decorator.js
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)
5069-5074:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRun static field decorator initializers before a class decorator can swap out
ClassValue.By Line 5073,
ClassValuemay already have been replaced by a class decorator, but the static field/accessor decorator initializers were registered on the original class during element decoration. In cases where a class decorator returns a replacement constructor, this call will run those initializers on the wrong object or skip them entirely.💡 Suggested direction
- // Store initializer lists on the class for execution during instantiation + // Store initializer lists on the class for execution during instantiation InitializerResults := MethodCollector.GetInitializers; ClassValue.SetMethodInitializers(InitializerResults); InitializerResults := FieldCollector.GetInitializers; ClassValue.SetFieldInitializers(InitializerResults); - ClassValue.RunDecoratorStaticFieldInitializers; // Run class-level initializers after static fields InitializerResults := ClassCollector.GetInitializers;Capture the pre-class-decorator class value and run
RunDecoratorStaticFieldInitializerson that original class beforeClassValue := TGocciaClassValue(DecoratorResult)can redirect the reference, or explicitly transfer the pending static initializer state when a class decorator replaces the class.🤖 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 5069 - 5074, The static field/accessor decorator initializers must be executed on the original class object before a class decorator can replace ClassValue; capture the original class instance (the value currently referenced by ClassValue) prior to calling MethodCollector.GetInitializers / FieldCollector.GetInitializers and then invoke RunDecoratorStaticFieldInitializers on that captured instance (or transfer the pending initializer state to the replacement) before assigning ClassValue := TGocciaClassValue(DecoratorResult) or any code that applies the class decorator result, ensuring initializers registered during element decoration run on the original constructor.
♻️ Duplicate comments (1)
source/units/Goccia.VM.pas (1)
6007-6131:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve the class-value path for decorated static members.
Lines 6011, 6059, and 6101 downcast
ClassValtoTGocciaObjectValueand then inline the static definition logic. That means the subsequentSetBytecodeHomeObject(...)calls no longer see the static-class path, so decorated static methods/getters/setters can resolvesuperagainst the wrong chain. The inline accessor merge also bypassesGetOwnStaticSymbolDescriptor(...)for symbol-keyed static accessors. Please route the static cases back through the existing static key-aware helpers, or at least passClassValintoSetBytecodeHomeObjectand keep the class-specific static-symbol descriptor lookup. Based on learnings: static/computed definition sites insource/units/Goccia.VM.pasmust pass the class value intoSetBytecodeHomeObjectsosuperresolves on the correct prototype chain.🤖 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.VM.pas` around lines 6007 - 6131, Static decorated definitions cast ClassVal to TGocciaObjectValue which loses the class-value path causing SetBytecodeHomeObject to resolve super against the wrong prototype and also bypasses static-symbol descriptor helpers; update the static branches in the routines that define decorators (the block that defines properties, DefineDecoratedGetterProperty and DefineDecoratedSetterProperty) so that when AIsStatic is true you: pass the original ClassVal (as TGocciaClassValue(ClassVal)) into SetBytecodeHomeObject instead of the downcasted TGocciaObjectValue, and when reading existing descriptors for symbol keys call the class-specific static helper (GetOwnStaticSymbolDescriptor) rather than the instance symbol lookup; alternatively refactor to route static cases through the existing static key-aware helpers so static symbol/property lookups and SetBytecodeHomeObject preserve the class value path.
🤖 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 5069-5074: The static field/accessor decorator initializers must
be executed on the original class object before a class decorator can replace
ClassValue; capture the original class instance (the value currently referenced
by ClassValue) prior to calling MethodCollector.GetInitializers /
FieldCollector.GetInitializers and then invoke
RunDecoratorStaticFieldInitializers on that captured instance (or transfer the
pending initializer state to the replacement) before assigning ClassValue :=
TGocciaClassValue(DecoratorResult) or any code that applies the class decorator
result, ensuring initializers registered during element decoration run on the
original constructor.
---
Duplicate comments:
In `@source/units/Goccia.VM.pas`:
- Around line 6007-6131: Static decorated definitions cast ClassVal to
TGocciaObjectValue which loses the class-value path causing
SetBytecodeHomeObject to resolve super against the wrong prototype and also
bypasses static-symbol descriptor helpers; update the static branches in the
routines that define decorators (the block that defines properties,
DefineDecoratedGetterProperty and DefineDecoratedSetterProperty) so that when
AIsStatic is true you: pass the original ClassVal (as
TGocciaClassValue(ClassVal)) into SetBytecodeHomeObject instead of the
downcasted TGocciaObjectValue, and when reading existing descriptors for symbol
keys call the class-specific static helper (GetOwnStaticSymbolDescriptor) rather
than the instance symbol lookup; alternatively refactor to route static cases
through the existing static key-aware helpers so static symbol/property lookups
and SetBytecodeHomeObject preserve the class value path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aec1c221-3f18-4202-b3e1-e1b33791697f
📒 Files selected for processing (5)
source/units/Goccia.Evaluator.Decorators.passource/units/Goccia.Evaluator.passource/units/Goccia.VM.passource/units/Goccia.Values.ClassValue.pastests/language/decorators/computed-field-decorator.js
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
yieldresumes correctly for instance and static fields.async functionexpression source text anchored at theasynckeyword.Testing
./build/GocciaTestRunner tests/language/classes/class-computed-field-yield-order.js tests/language/function-keyword/class-computed-field-yield.js tests/language/objects/basic-object-creation.js tests/language/function-keyword/async-function.js --asi(19/19)./build/GocciaTestRunner tests/language/classes/class-computed-field-yield-order.js tests/language/function-keyword/class-computed-field-yield.js tests/language/objects/basic-object-creation.js tests/language/function-keyword/async-function.js --asi --mode=bytecode(19/19)./build/GocciaTestRunner tests --asi --unsafe-ffi(9743/9743)./build/GocciaTestRunner tests --asi --unsafe-ffi --mode=bytecode(9743/9743)bun scripts/run_test262_suite.ts --categories language --filter 'language/**/class/cpn-class-*-fields*computed-property-name-from-yield-expression.js' --mode interpreted --jobs 1 --verbose --timeout-ms 10000(4/4)bun scripts/run_test262_suite.ts --categories language --filter 'language/**/class/cpn-class-*-fields*computed-property-name-from-yield-expression.js' --mode bytecode --jobs 1 --verbose --timeout-ms 10000(4/4)./build.pas tests loaderbare bundler./format.pas --checkgit diff --cached --check./build.pas clean testrunner./build/GocciaBundler tests/language/objects/basic-object-creation.js --output=/tmp/goccia-object-basic.gbc