Fix bytecode object literal define semantics#669
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds OP_DEFINE_DATA_PROP and OP_DEFINE_METHOD_PROP to the bytecode format (version 34), propagates method metadata from the parser, changes the compiler to emit define opcodes for object-literal properties, implements VM define-path helpers and opcode handling, updates evaluator skipping of superseded entries, and adds tests and documentation clarifying define vs set semantics. ChangesDefine Data Property Opcode for Object Literals
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
Suite TimingTest Runner (interpreted: 9,729 passed; bytecode: 9,729 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. |
Benchmark Results407 benchmarks Interpreted: 🟢 51 improved · 🔴 58 regressed · 298 unchanged · avg +0.8% arraybuffer.js — Interp: 🟢 4, 🔴 1, 9 unch. · avg +0.7% · Bytecode: 🟢 11, 🔴 1, 2 unch. · avg +8.7%
arrays.js — Interp: 🟢 1, 🔴 3, 15 unch. · avg -0.8% · Bytecode: 🟢 18, 1 unch. · avg +10.9%
async-await.js — Interp: 6 unch. · avg +0.8% · Bytecode: 🟢 5, 1 unch. · avg +6.4%
async-generators.js — Interp: 2 unch. · avg -1.6% · Bytecode: 🟢 2 · avg +7.8%
base64.js — Interp: 10 unch. · avg +0.1% · Bytecode: 🟢 3, 🔴 7 · avg -2.3%
classes.js — Interp: 🟢 2, 🔴 3, 26 unch. · avg -0.3% · Bytecode: 🟢 24, 🔴 1, 6 unch. · avg +6.4%
closures.js — Interp: 🟢 3, 8 unch. · avg +2.7% · Bytecode: 🟢 9, 🔴 1, 1 unch. · avg +7.3%
collections.js — Interp: 🟢 4, 8 unch. · avg +1.2% · Bytecode: 🟢 12 · avg +9.2%
csv.js — Interp: 🟢 1, 12 unch. · avg +1.2% · Bytecode: 🟢 13 · avg +12.0%
destructuring.js — Interp: 🟢 3, 🔴 1, 18 unch. · avg +0.6% · Bytecode: 🟢 15, 🔴 5, 2 unch. · avg +2.3%
fibonacci.js — Interp: 🔴 1, 7 unch. · avg -0.7% · Bytecode: 🟢 5, 🔴 2, 1 unch. · avg +6.0%
float16array.js — Interp: 🟢 3, 🔴 6, 23 unch. · avg -0.1% · Bytecode: 🟢 26, 🔴 1, 5 unch. · avg +6.4%
for-of.js — Interp: 7 unch. · avg -0.0% · Bytecode: 🟢 7 · avg +9.8%
generators.js — Interp: 4 unch. · avg +0.1% · Bytecode: 🟢 4 · avg +6.1%
iterators.js — Interp: 🟢 9, 🔴 1, 32 unch. · avg +1.0% · Bytecode: 🟢 11, 🔴 17, 14 unch. · avg -1.0%
json.js — Interp: 🟢 3, 🔴 2, 15 unch. · avg +0.5% · Bytecode: 🟢 18, 2 unch. · avg +9.5%
jsx.jsx — Interp: 🔴 16, 5 unch. · avg -3.9% · Bytecode: 🔴 20, 1 unch. · avg -9.5%
modules.js — Interp: 9 unch. · avg -1.2% · Bytecode: 🟢 9 · avg +16.3%
numbers.js — Interp: 🟢 1, 10 unch. · avg +1.2% · Bytecode: 🟢 10, 1 unch. · avg +10.1%
objects.js — Interp: 🔴 3, 4 unch. · avg -3.9% · Bytecode: 🟢 3, 🔴 4 · avg -3.5%
promises.js — Interp: 12 unch. · avg +1.2% · Bytecode: 🟢 7, 5 unch. · avg +4.6%
regexp.js — Interp: 🟢 1, 10 unch. · avg +0.3% · Bytecode: 🟢 5, 🔴 5, 1 unch. · avg +2.7%
strings.js — Interp: 🔴 7, 12 unch. · avg -2.7% · Bytecode: 🟢 18, 1 unch. · avg +10.7%
tsv.js — Interp: 9 unch. · avg +0.0% · Bytecode: 🟢 9 · avg +9.0%
typed-arrays.js — Interp: 🟢 10, 🔴 1, 11 unch. · avg +24.4% · Bytecode: 🟢 11, 🔴 2, 9 unch. · avg -0.1%
uint8array-encoding.js — Interp: 🟢 3, 🔴 9, 6 unch. · avg -13.8% · Bytecode: 🟢 2, 🔴 8, 8 unch. · avg -20.6%
weak-collections.js — Interp: 🟢 3, 🔴 4, 8 unch. · avg +4.2% · Bytecode: 🟢 11, 🔴 2, 2 unch. · avg +33.0%
Deterministic profile diffarraybuffer
arrays
classes
closures
collections
csv
destructuring
fibonacci
float16array
generators
iterators
json
jsx
objects
regexp
tsv
typed-arrays
uint8array-encoding
weak-collections
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. |
test262 Conformance
Areas closest to 100%
Per-test deltas (+9 / -0)Newly passing (9):
Steady-state failures are non-blocking; regressions vs the cached main baseline (lower total pass count, or any PASS → non-PASS transition) fail the conformance gate. Measured on ubuntu-latest x64, bytecode mode. Areas grouped by the first two test262 path components; minimum 25 attempted tests, areas already at 100% excluded. Δ vs main compares against the most recent cached |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.VM.pas`:
- Around line 5087-5089: OP_DEFINE_DATA_PROP is assigning a data property but
currently calls SetBytecodeHomeObject(AValue, TargetObject) after TargetObject
:= TGocciaObjectValue(ATarget), which incorrectly assigns a HomeObject and
changes super resolution for bytecode function values; remove the
SetBytecodeHomeObject call from the data-property path (leave other code paths
that legitimately set a home intact) so AValue is created/overwritten as a plain
own data property without modifying its HomeObject.
🪄 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: 24ac2af6-d858-4518-80c1-e631a73e6a71
📒 Files selected for processing (6)
docs/bytecode-vm.mdsource/units/Goccia.Bytecode.OpCodeNames.passource/units/Goccia.Bytecode.passource/units/Goccia.Compiler.Expressions.passource/units/Goccia.VM.pastests/language/objects/basic-object-creation.js
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)
source/units/Goccia.Parser.pas (1)
2920-2988:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftMultiple conflicting define instructions generated for duplicate static keys
When duplicate keys appear like
{ a() {}, a: 1 }, the parser stores each occurrence inPropertySourceOrderwith its ownIsMethodflag, but thePropertiesdictionary only retains the final value (line 2975). The compiler loop (lines 2613–2650) processes ALLPropertySourceOrderentries without deduplication, generating separateOP_DEFINE_METHOD_PROPandOP_DEFINE_DATA_PROPinstructions for the same key with mismatched metadata. This causes incorrect[[HomeObject]]setup when methods withsuperreferences are shadowed by later non-method definitions.Fix: Either deduplicate
PropertySourceOrderentries for duplicate keys or skip entries whose keys have already been processed in the loop.🤖 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 2920 - 2988, The parser is recording every property occurrence in PropertySourceOrder even when Properties (the Properties dictionary) is later overwritten, causing the compiler loop to emit conflicting OP_DEFINE_METHOD_PROP/OP_DEFINE_DATA_PROP for the same static key; update the parser (in the object literal handling around PropertySourceOrder/Properties/PropertyOrder and the IsMethod logic) to avoid adding duplicate static-key entries to PropertySourceOrder (or mark them as skipped) when Properties already contains the key (i.e., when Properties.ContainsKey(Key) is true you should not append another pstStatic entry), so the compiler that iterates PropertySourceOrder will only see the final effective definition for each static key (alternatively add a de-dup pass in the compiler loop before emitting OP_DEFINE_* instructions to skip already-processed StaticKey values).
🤖 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 2457-2461: The code trusts AIsMethod only when choosing DefineOp,
causing synthetic or unordered literals where the value is a
TGocciaMethodExpression to be emitted as OP_DEFINE_DATA_PROP; update the
decision before calling EmitDefineDataPropertyByName to set DefineOp to
OP_DEFINE_METHOD_PROP if AIsMethod is true OR the value expression is a
TGocciaMethodExpression (inspect the value node represented by ValReg / the AST
node for the value), otherwise use OP_DEFINE_DATA_PROP; adjust the logic in the
method containing AIsMethod/DefineOp (the block that calls
EmitDefineDataPropertyByName with ACtx, ADest, AKey, ValReg) so method semantics
are derived from the value expression as well.
---
Outside diff comments:
In `@source/units/Goccia.Parser.pas`:
- Around line 2920-2988: The parser is recording every property occurrence in
PropertySourceOrder even when Properties (the Properties dictionary) is later
overwritten, causing the compiler loop to emit conflicting
OP_DEFINE_METHOD_PROP/OP_DEFINE_DATA_PROP for the same static key; update the
parser (in the object literal handling around
PropertySourceOrder/Properties/PropertyOrder and the IsMethod logic) to avoid
adding duplicate static-key entries to PropertySourceOrder (or mark them as
skipped) when Properties already contains the key (i.e., when
Properties.ContainsKey(Key) is true you should not append another pstStatic
entry), so the compiler that iterates PropertySourceOrder will only see the
final effective definition for each static key (alternatively add a de-dup pass
in the compiler loop before emitting OP_DEFINE_* instructions to skip
already-processed StaticKey values).
🪄 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: d123423a-9cca-4e28-aff1-372393080fa0
📒 Files selected for processing (8)
docs/bytecode-vm.mdsource/units/Goccia.AST.Expressions.passource/units/Goccia.Bytecode.OpCodeNames.passource/units/Goccia.Bytecode.passource/units/Goccia.Compiler.Expressions.passource/units/Goccia.Parser.passource/units/Goccia.VM.pastests/language/classes/scope-walk-function-boundary.js
✅ Files skipped from review due to trivial changes (1)
- docs/bytecode-vm.md
🚧 Files skipped from review as they are similar to previous changes (2)
- source/units/Goccia.Bytecode.OpCodeNames.pas
- source/units/Goccia.VM.pas
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/language/objects/basic-object-creation.js (1)
88-104: ⚡ Quick winSuppress the Biome duplicate-key warning.
The duplicate
valuekey at lines 96-99 is intentional test behavior to verify object literal define semantics handle duplicate keys correctly (final definition wins). The static analysis warning is a false positive.🔕 Proposed suppression comment
Add a suppression comment before the object literal:
}; + // biome-ignore lint/suspicious/noDuplicateObjectKeys: intentionally testing duplicate key behavior const obj = { value() {🤖 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/objects/basic-object-creation.js` around lines 88 - 104, This test intentionally uses duplicate static object keys in the test "duplicate static object keys evaluate final definition once" (see the obj literal with duplicate key value and the helper function mark), so suppress the Biome duplicate-key warning by adding the appropriate suppression comment immediately before the object literal; keep the comment scoped to that literal so the duplicate behavior is preserved for the test and no other warnings are globally disabled.
🤖 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.
Nitpick comments:
In `@tests/language/objects/basic-object-creation.js`:
- Around line 88-104: This test intentionally uses duplicate static object keys
in the test "duplicate static object keys evaluate final definition once" (see
the obj literal with duplicate key value and the helper function mark), so
suppress the Biome duplicate-key warning by adding the appropriate suppression
comment immediately before the object literal; keep the comment scoped to that
literal so the duplicate behavior is preserved for the test and no other
warnings are globally disabled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c6c379b2-cf8d-40cf-a559-74cebee7d482
📒 Files selected for processing (5)
source/units/Goccia.AST.Expressions.passource/units/Goccia.Compiler.Expressions.passource/units/Goccia.Evaluator.passource/units/Goccia.Parser.pastests/language/objects/basic-object-creation.js
🚧 Files skipped from review as they are similar to previous changes (2)
- source/units/Goccia.AST.Expressions.pas
- source/units/Goccia.Parser.pas
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 (3)
source/units/Goccia.Evaluator.pas (1)
1451-1452:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon't skip overwritten data-property initializers.
Line 1451 now bypasses evaluation entirely, but Lines 1458-1461 rehydrate static properties from
AObjectExpression.Properties, which is already deduplicated by key. For{ a: sideEffect(1), a: sideEffect(2) }, the earlier initializer never runs, so its side effects disappear instead of just being overwritten. Object literals still need to evaluate each property definition in source order; only the final descriptor should win. This likely needs the source-order entry to carry its own expression node instead of looking it up through the deduplicated map.Also applies to: 1458-1461
🤖 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 1451 - 1452, The loop over AObjectExpression.PropertySourceOrder must still evaluate every source-order property definition (to preserve side effects) even when the property was deduplicated; stop using "if ... Skip then Continue" to bypass evaluation. Instead, evaluate the expression node carried by the source-order entry (add or use a field like PropertySourceOrder[].Expression) for each entry in source order, then, separately, use AObjectExpression.Properties (the deduplicated map) only to pick the final property descriptor to assign to the resulting object; update the rehydration logic around the current use of AObjectExpression.Properties so it only applies descriptors but does not replace evaluation of skipped entries.source/units/Goccia.Parser.pas (2)
2013-2028:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve the async-function start position before consuming the optional name.
Tokengets reused for the identifier, so namedasync function foo(){}/async function* foo(){}expressions end up callingParseFunctionBodyExpressionandExtractSourceRangefromfooinstead of fromasync. That truncatesSourceTextand shifts the function node’s location metadata.Suggested fix
- Name := ''; + Line := Token.Line; + Column := Token.Column; + Name := ''; if Check(gttIdentifier) then begin Token := Advance; ValidateIdentifierBinding(Token); Name := Token.Lexeme; end; CollectGenericParameters; - Result := ParseFunctionBodyExpression(Token.Line, Token.Column, True, IsGenerator); - TGocciaFunctionExpression(Result).SourceText := ExtractSourceRange(Token.Line, Token.Column); + Result := ParseFunctionBodyExpression(Line, Column, True, IsGenerator); + TGocciaFunctionExpression(Result).SourceText := ExtractSourceRange(Line, Column);🤖 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 2013 - 2028, The async-function start position is lost when you Advance into the optional identifier (Token is reused), so capture the async start Token (or its Line/Column) before calling Check/Advance for the name: when entering the async function handling code, save the current Token (or Token.Line/Token.Column) into a local variable (e.g. AsyncStartTokenLine/AsyncStartTokenColumn or StartToken) before calling Advance/ValidateIdentifierBinding/CollectGenericParameters; then pass those saved start coordinates into ParseFunctionBodyExpression and use them with ExtractSourceRange and when assigning TGocciaFunctionExpression(SourceText)/location fields so the function node reflects the async keyword position rather than the identifier position.
2975-2997:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftDon’t discard earlier duplicate property initializers.
This path overwrites
Properties[Key]and then marks the earlier source-order entry as skipped, so duplicate data properties lose the earlier value expression entirely. For{ a: f(), a: g() }, the AST no longer has any way to evaluatef(), even though ECMAScript still evaluates both property definitions in order and only lets the final descriptor win. The source-order representation needs to retain each static property’s originalValuenode, not just the deduplicated key.🤖 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 2975 - 2997, The current logic overwrites Properties[Key] and marks earlier PropertySourceOrder entries as Skip, losing earlier static property Value nodes; instead ensure every static property still appends a PropertySourceOrder entry that preserves its Value expression so earlier initializers can be evaluated in source order while Properties[Key] reflects the final value. Concretely: stop setting PropertySourceOrder[OrderIndex].Skip for earlier static entries and do not discard their Value nodes — store the Value in the PropertySourceOrder record for pstStatic (add a field if needed) and always Inc(SourceOrderCount)/SetLength/assign PropertySourceOrder[SourceOrderCount-1] with PropertyType = pstStatic, StaticKey = Key and the Value node; maintain Properties[Key] as the last seen value and keep PropertyOrder as-is for insertion tracking.
🤖 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 1451-1452: The loop over AObjectExpression.PropertySourceOrder
must still evaluate every source-order property definition (to preserve side
effects) even when the property was deduplicated; stop using "if ... Skip then
Continue" to bypass evaluation. Instead, evaluate the expression node carried by
the source-order entry (add or use a field like
PropertySourceOrder[].Expression) for each entry in source order, then,
separately, use AObjectExpression.Properties (the deduplicated map) only to pick
the final property descriptor to assign to the resulting object; update the
rehydration logic around the current use of AObjectExpression.Properties so it
only applies descriptors but does not replace evaluation of skipped entries.
In `@source/units/Goccia.Parser.pas`:
- Around line 2013-2028: The async-function start position is lost when you
Advance into the optional identifier (Token is reused), so capture the async
start Token (or its Line/Column) before calling Check/Advance for the name: when
entering the async function handling code, save the current Token (or
Token.Line/Token.Column) into a local variable (e.g.
AsyncStartTokenLine/AsyncStartTokenColumn or StartToken) before calling
Advance/ValidateIdentifierBinding/CollectGenericParameters; then pass those
saved start coordinates into ParseFunctionBodyExpression and use them with
ExtractSourceRange and when assigning
TGocciaFunctionExpression(SourceText)/location fields so the function node
reflects the async keyword position rather than the identifier position.
- Around line 2975-2997: The current logic overwrites Properties[Key] and marks
earlier PropertySourceOrder entries as Skip, losing earlier static property
Value nodes; instead ensure every static property still appends a
PropertySourceOrder entry that preserves its Value expression so earlier
initializers can be evaluated in source order while Properties[Key] reflects the
final value. Concretely: stop setting PropertySourceOrder[OrderIndex].Skip for
earlier static entries and do not discard their Value nodes — store the Value in
the PropertySourceOrder record for pstStatic (add a field if needed) and always
Inc(SourceOrderCount)/SetLength/assign PropertySourceOrder[SourceOrderCount-1]
with PropertyType = pstStatic, StaticKey = Key and the Value node; maintain
Properties[Key] as the last seen value and keep PropertyOrder as-is for
insertion tracking.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5e97d53b-82f9-4f6b-9ec5-0f4e88af8a32
📒 Files selected for processing (11)
docs/decision-log.mddocs/language.mddocs/value-system.mdsource/units/Goccia.AST.Expressions.passource/units/Goccia.AST.Statements.passource/units/Goccia.Compiler.Expressions.passource/units/Goccia.Compiler.Statements.passource/units/Goccia.Compiler.passource/units/Goccia.Evaluator.passource/units/Goccia.Parser.passource/units/Goccia.Types.Enforcement.pas
✅ Files skipped from review due to trivial changes (2)
- docs/value-system.md
- docs/decision-log.md
Summary
OP_DEFINE_DATA_PROPand bump the bytecode format to v33 so bytecode object literal data properties use define semantics instead of ordinary[[Set]]assignment.OP_SET_*path.Testing
./build/GocciaTestRunner tests/language/objects/basic-object-creation.js --asi./build/GocciaTestRunner tests/language/objects/basic-object-creation.js --mode=bytecode --asibun scripts/run_test262_suite.ts --suite-dir /tmp/goccia-test262-suite --filter "language/expressions/object/11.1.5_3-3-1.js" --mode=bytecode --jobs=1bun scripts/run_test262_suite.ts --suite-dir /tmp/goccia-test262-suite --filter "built-ins/Array/prototype/indexOf/15.4.4.14-9-b-i-6.js" --mode=bytecode --jobs=1./build/GocciaTestRunner tests --asi --unsafe-ffi./format.pas --checkgit diff --checkNote: a sequential full bytecode run with
--jobs=1exposes an existingGoccia.gc()runner/runtime fatal intests/built-ins/global-properties/goccia.js,tests/built-ins/WeakMap/gc.js, andtests/built-ins/WeakSet/gc.js; the same files fail individually in interpreted mode too, so this is separate from the bytecode object literal change.