Expand Test262 support for classes, functions, and typed arrays#378
Merged
Expand Test262 support for classes, functions, and typed arrays#378
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)
📝 WalkthroughWalkthroughMultiple runtime and VM changes redefine how prototypes and constructor properties are installed (using explicit DefineProperty descriptors), introduce a non-global %TypedArray% intrinsic and wired shared typed-array prototype, extend call/apply/bind dispatch to support class constructors and callable proxies, and update the Test262 runner/filter with a --compat-var flag and BigInt typed-array mapping. Changes
Sequence Diagram(s)sequenceDiagram
participant Engine as Engine/Bootstrap
participant Intrinsic as %TypedArray%<br/>Intrinsic
participant Prototype as Shared<br/>TypedArray Prototype
participant Registry as Constructor<br/>Registry
participant Constructors as TypedArray<br/>Constructors
Engine->>Intrinsic: create %TypedArray% (name/length)
Engine->>Prototype: create shared typed-array prototype
Intrinsic->>Prototype: link intrinsic.prototype -> shared prototype
Engine->>Registry: register typed-array constructors using FTypedArrayIntrinsic
Registry->>Constructors: create each constructor with %TypedArray% intrinsic
Constructors->>Prototype: set constructor.prototype -> shared prototype
Engine->>Constructors: patch default prototypes for related constructors
sequenceDiagram
participant Caller as Caller Code
participant FnProto as Function.prototype.call/apply
participant Dispatcher as DispatchCall
participant FuncBase as TGocciaFunctionBase
participant ClassVal as TGocciaClassValue
participant ProxyVal as TGocciaProxyValue<br/>(callable)
Caller->>FnProto: invoke call/apply on callee
FnProto->>Dispatcher: DispatchCall(callee, thisArg, args)
alt callee is TGocciaFunctionBase
Dispatcher->>FuncBase: Call(...)
FuncBase-->>Dispatcher: result
else callee is TGocciaClassValue
Dispatcher->>ClassVal: Call(...)
ClassVal-->>Dispatcher: result
else callee is callable TGocciaProxyValue
Dispatcher->>ProxyVal: ApplyTrap(...)
ProxyVal-->>Dispatcher: result
end
Dispatcher-->>FnProto: result
FnProto-->>Caller: result
sequenceDiagram
participant VM as VM Executor
participant Opcode as OP_NEW_CLASS / OP_CLASS_ADD_METHOD_CONST
participant Proto as Class Prototype
participant Desc as TGocciaPropertyDescriptorData
VM->>Opcode: execute class creation opcode
Opcode->>Proto: create prototype object
VM->>Desc: construct descriptor with pfConfigurable, pfWritable
VM->>Proto: DefineProperty("constructor", Desc)
VM->>Proto: DefineProperty(method_name, Desc_non_enumerable)
Proto-->>VM: prototype and methods installed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
coderabbitai
Bot
added
new feature
Contributor
Suite Timing
Measured on ubuntu-latest x64. |
Contributor
Benchmark Results386 benchmarks Interpreted: 🟢 13 improved · 🔴 283 regressed · 90 unchanged · avg -5.8% arraybuffer.js — Interp: 🔴 5, 9 unch. · avg -2.6% · Bytecode: 🟢 13, 1 unch. · avg +25.9%
arrays.js — Interp: 🔴 11, 8 unch. · avg -6.7% · Bytecode: 🟢 19 · avg +27.3%
async-await.js — Interp: 🔴 4, 2 unch. · avg -8.3% · Bytecode: 🟢 6 · avg +28.0%
base64.js — Interp: 🔴 8, 2 unch. · avg -6.5% · Bytecode: 🟢 10 · avg +26.4%
classes.js — Interp: 🔴 20, 11 unch. · avg -7.3% · Bytecode: 🟢 31 · avg +15.5%
closures.js — Interp: 🔴 7, 4 unch. · avg -1.7% · Bytecode: 🟢 11 · avg +26.8%
collections.js — Interp: 🟢 1, 🔴 5, 6 unch. · avg -1.7% · Bytecode: 🟢 12 · avg +27.9%
csv.js — Interp: 🔴 9, 4 unch. · avg -5.7% · Bytecode: 🟢 13 · avg +27.9%
destructuring.js — Interp: 🔴 18, 4 unch. · avg -4.5% · Bytecode: 🟢 22 · avg +25.5%
fibonacci.js — Interp: 🔴 8 · avg -8.7% · Bytecode: 🟢 8 · avg +24.9%
float16array.js — Interp: 🟢 4, 🔴 26, 2 unch. · avg -3.7% · Bytecode: 🟢 32 · avg +27.3%
for-of.js — Interp: 🔴 1, 6 unch. · avg -1.5% · Bytecode: 🟢 7 · avg +27.1%
helpers/bench-module.js — Interp: 0 · Bytecode: 0
iterators.js — Interp: 🔴 39, 3 unch. · avg -9.9% · Bytecode: 🟢 42 · avg +21.7%
json.js — Interp: 🔴 20 · avg -9.3% · Bytecode: 🟢 20 · avg +29.5%
jsx.jsx — Interp: 🔴 11, 10 unch. · avg -2.7% · Bytecode: 🟢 21 · avg +21.6%
modules.js — Interp: 🔴 5, 4 unch. · avg -5.0% · Bytecode: 🟢 9 · avg +27.6%
numbers.js — Interp: 🟢 1, 🔴 9, 1 unch. · avg -5.5% · Bytecode: 🟢 11 · avg +21.7%
objects.js — Interp: 🔴 2, 5 unch. · avg -1.8% · Bytecode: 🟢 7 · avg +26.6%
promises.js — Interp: 🔴 11, 1 unch. · avg -9.2% · Bytecode: 🟢 12 · avg +23.5%
regexp.js — Interp: 🔴 11 · avg -5.8% · Bytecode: 🟢 11 · avg +24.6%
strings.js — Interp: 🔴 19 · avg -9.6% · Bytecode: 🟢 19 · avg +52.2%
tsv.js — Interp: 🔴 7, 2 unch. · avg -5.9% · Bytecode: 🟢 9 · avg +24.5%
typed-arrays.js — Interp: 🟢 5, 🔴 15, 2 unch. · avg -4.5% · Bytecode: 🟢 22 · avg +21.8%
uint8array-encoding.js — Interp: 🟢 2, 🔴 12, 4 unch. · avg -2.9% · Bytecode: 🟢 11, 🔴 5, 2 unch. · avg +10.7%
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. |
![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: 4
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.Values.ClassValue.pas (1)
468-512:⚠️ Potential issue | 🟡 MinorAccessor descriptors include
pfEnumerablebut class accessors should be non-enumerable.Per ES §14.3.7, class element definitions (including accessors) have
enumerable: false. TheAddMethodimplementation correctly omitspfEnumerable, butAddGetterandAddSetterinclude it.🔧 Proposed fix to remove pfEnumerable from accessor descriptors
- FClassPrototype.DefineProperty(AName, TGocciaPropertyDescriptorAccessor.Create(AGetter, ExistingSetter, [pfEnumerable, pfConfigurable, pfWritable])); + FClassPrototype.DefineProperty(AName, TGocciaPropertyDescriptorAccessor.Create(AGetter, ExistingSetter, [pfConfigurable, pfWritable]));Apply similar changes to lines 487, 505, and 510.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/units/Goccia.Values.ClassValue.pas` around lines 468 - 512, The accessor descriptors created in TGocciaClassValue.AddGetter and AddSetter wrongly include pfEnumerable; update the FClassPrototype.DefineProperty calls in both AddGetter and AddSetter (the TGocciaPropertyDescriptorAccessor.Create invocations) to omit pfEnumerable so class accessors are non-enumerable (i.e., use the same flags as AddMethod, removing pfEnumerable from the flags list).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@source/units/Goccia.Builtins.GlobalObject.pas`:
- Around line 355-364: Object.create's second-argument path passes PropsArg to
ObjectDefineProperties, but ObjectDefineProperties currently only iterates
PropertiesDescriptor.GetEnumerablePropertyEntries so symbol-keyed property
descriptors are ignored; update ObjectDefineProperties (the routine called from
the Object.create branch) to also collect/iterate symbol-keyed descriptor
entries (or use a descriptor API that returns both string and symbol entries)
instead of solely GetEnumerablePropertyEntries so descriptors for symbol keys
(e.g. PropertiesDescriptor.GetEnumerableSymbolEntries or equivalent) are
processed and applied to the new object.
In `@source/units/Goccia.Engine.pas`:
- Around line 1035-1047: The backfill calls to
TGocciaClassValue.PatchDefaultPrototype only patch the constructors listed (and
GenericConstructor only references the most recent registration), leaving
earlier-registered constructors like TextEncoder, TextDecoder, URL,
URLSearchParams, Headers, and Response unpatched and still detached from
Function.prototype; fix this by ensuring PatchDefaultPrototype is invoked for
every constructor registration (not just the latest GenericConstructor) — either
call TGocciaClassValue.PatchDefaultPrototype for each missing constructor
(TextEncoder, TextDecoder, URL, URLSearchParams, Headers, Response) in the
initialization block or update the registration code so that the
registry/registration routine iterates and calls PatchDefaultPrototype for all
entries when they are created, so all constructors end up with the correct
[[Prototype]] (ensure references to GenericConstructor, FunctionConstructor and
the named constructors are used to locate where to apply the changes).
In `@source/units/Goccia.Runtime.Bootstrap.pas`:
- Around line 505-517: PatchDefaultPrototype is only being applied to the
specific constructors listed and misses earlier-created constructors
(TextEncoder, TextDecoder, URL, URLSearchParams), so those globals won't inherit
from Function.prototype; update the bootstrap to call
TGocciaClassValue.PatchDefaultPrototype for TextEncoderConstructor,
TextDecoderConstructor, URLConstructor and URLSearchParamsConstructor (or
alternatively iterate over the registry of created constructors and call
TGocciaClassValue.PatchDefaultPrototype for each) instead of assuming
GenericConstructor covers earlier ones.
In `@source/units/Goccia.Values.FunctionBase.pas`:
- Around line 598-599: The bound-class call paths in TGocciaClassValue (where
FOriginalFunction is TGocciaClassValue and the call uses
TGocciaClassValue(...).Call with CombinedArgs and FBoundThis) execute correctly
but TGocciaBoundFunctionValue.GetFunctionName and GetFunctionLength still read
metadata only from TGocciaFunctionBase, causing bound constructors to report
fallback "bound " and length 0; update GetFunctionName and GetFunctionLength in
TGocciaBoundFunctionValue to detect when FOriginalFunction is a
TGocciaClassValue (or any class-derived callable) and return the original
class/constructor name and length by delegating to the underlying
TGocciaClassValue (or underlying callable) metadata rather than always reading
TGocciaFunctionBase, ensuring the bound wrapper preserves the original
constructor’s name and length for all call paths (also apply same fix for the
other similar branches around the 610–676 region).
---
Outside diff comments:
In `@source/units/Goccia.Values.ClassValue.pas`:
- Around line 468-512: The accessor descriptors created in
TGocciaClassValue.AddGetter and AddSetter wrongly include pfEnumerable; update
the FClassPrototype.DefineProperty calls in both AddGetter and AddSetter (the
TGocciaPropertyDescriptorAccessor.Create invocations) to omit pfEnumerable so
class accessors are non-enumerable (i.e., use the same flags as AddMethod,
removing pfEnumerable from the flags list).
🪄 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: 043e6da2-d4a8-4835-b905-e4a1520b1f96
📒 Files selected for processing (32)
scripts/run_test262_suite.pyscripts/test262_syntax_filter.pysource/units/Goccia.AST.Statements.passource/units/Goccia.Builtins.GlobalObject.passource/units/Goccia.Builtins.GlobalRegExp.passource/units/Goccia.Builtins.Globals.passource/units/Goccia.Engine.passource/units/Goccia.Evaluator.passource/units/Goccia.ObjectModel.Engine.passource/units/Goccia.Runtime.Bootstrap.passource/units/Goccia.SharedPrototype.passource/units/Goccia.Values.ArrayValue.passource/units/Goccia.Values.ClassValue.passource/units/Goccia.Values.FunctionBase.passource/units/Goccia.Values.TypedArrayValue.pastests/built-ins/Array/isArray.jstests/built-ins/Array/prototype/map.jstests/built-ins/Error/error-constructor.jstests/built-ins/Function/prototype/apply.jstests/built-ins/Function/prototype/bind.jstests/built-ins/Function/prototype/call.jstests/built-ins/Function/prototype/constructor.jstests/built-ins/Map/constructor.jstests/built-ins/Map/prototype/get.jstests/built-ins/Object/create.jstests/built-ins/Object/getPrototypeOf.jstests/built-ins/Object/keys.jstests/built-ins/Set/constructor.jstests/built-ins/Set/prototype/has.jstests/built-ins/TypedArray/constructors.jstests/language/classes/basic-class-declaration.jstests/language/classes/method-enumerable.js
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
scripts/test-cli-parser.ts (1)
15-25: Align the first test block with the rest of the file by usingBun.spawnSyncwith direct stdin piping instead of shellprintfand redirection.Lines 17–18 depend on shell piping and
2>&1redirection, while the JSON test blocks below useBun.spawnSyncwith explicit stdin encoding. For cross-platform consistency, refactor the first block to use the same stdin-piping pattern:Suggested change
console.log("Error display (SyntaxError with caret and suggestion)..."); { - const res = await $`printf '%s\n' 'const x = 1' 'const y = x +' | ${LOADER} 2>&1`.nothrow(); - const out = res.text(); - if (res.exitCode === 0) throw new Error("Expected syntax error exit code"); + const proc = Bun.spawnSync([LOADER], { + stdin: new TextEncoder().encode("const x = 1\nconst y = x +\n"), + stdout: "pipe", + stderr: "pipe", + }); + const out = proc.stdout.toString() + proc.stderr.toString(); + if (proc.exitCode === 0) throw new Error("Expected syntax error exit code"); if (!out.includes("SyntaxError")) throw new Error(`Expected SyntaxError, got: ${out}`); if (!out.includes("^")) throw new Error(`Expected caret in error display, got: ${out}`); const lower = out.toLowerCase();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/test-cli-parser.ts` around lines 15 - 25, Replace the shell-based invocation that uses printf and redirection with a Bun.spawnSync call that runs LOADER and supplies the test stdin directly; specifically, change the const res = await $`printf ... | ${LOADER} 2>&1`.nothrow() pattern to a synchronous Bun.spawnSync invocation of LOADER providing the input string "const x = 1\nconst y = x +" (or equivalent) and then build out from res.stdout and res.stderr (or combined output) into the same out variable used by the existing checks; keep the existing asserts that inspect res.exitCode, out.includes("SyntaxError"), out.includes("^"), and the suggestion hint checks unchanged but read output from the Bun.spawnSync result instead of res.text().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/test-cli-apps.ts`:
- Around line 279-284: The test currently only inspects stdinRes.text() for
errors when invoking the loader with --source-map; update the block that runs
${LOADER} (variables LOADER, stdinRes, stdinOut) to first assert
stdinRes.exitCode is non-zero (fail the test if exitCode === 0) before checking
the output text, so we reject successful exit with a warning; keep the existing
output assertions after the exitCode check.
In `@scripts/test-cli-config.ts`:
- Around line 403-480: The current test block (in scripts/test-cli-config.ts
using tmp, test.js, test-runner.js, bench.js and commands
LOADER/TESTRUNNER/BUNDLER/BENCHRUNNER) only verifies behavior under a single
goccia.json and thus would pass if compat-var were enabled globally; add a
sibling directory (e.g., tmpNoConfig or a subdir of tmp without a goccia.json)
and create a no-config file that uses var, then invoke the same runners (LOADER
and TESTRUNNER at minimum) against that file and assert they fail or do not
exhibit compat-var behavior (e.g., expect loader output not to be "Result: 10"
or test runner not to pass), ensuring the test asserts the feature is scoped
only to files covered by goccia.json.
In `@scripts/test-cli.ts`:
- Around line 103-132: Add a baseline "no-flag" assertion before the existing
--compat-var checks: run the same inputs against LOADER, BUNDLER and TESTRUNNER
without the --compat-var flag (use the same src "use-var.js" and test
"test-var.js") and assert the negative expectations (Loader output does NOT
include "Result: 10", Bundler does NOT emit use-var.gbc, TestRunner does NOT
report "Passed: 1" / exits with failure). Place these checks immediately before
the current flagged checks so regressions that accept var unconditionally are
caught; reference LOADER, BUNDLER, TESTRUNNER, src (use-var.js) and testSrc
(test-var.js) to locate where to insert them.
In `@source/units/Goccia.Compiler.Statements.pas`:
- Around line 560-563: The code currently assigns Info.Name into
TGocciaClassExpression(Info.Initializer).ClassDefinition.FName which converts an
anonymous class into a named class (affecting CompileClassExpression's
HasNameBinding behavior); instead remove the assignment to ClassDefinition.FName
and instead apply the inferred name to the resulting class value using the same
approach as the AST execution path (call SetInferredName on the produced class
value or equivalent helper) so the class definition remains anonymous while the
runtime value carries Info.Name; update the handling around Info.Initializer
(TGocciaClassExpression) to invoke SetInferredName( <produced class value>,
Info.Name ) at the point the value is created rather than setting
ClassDefinition.FName.
---
Nitpick comments:
In `@scripts/test-cli-parser.ts`:
- Around line 15-25: Replace the shell-based invocation that uses printf and
redirection with a Bun.spawnSync call that runs LOADER and supplies the test
stdin directly; specifically, change the const res = await $`printf ... |
${LOADER} 2>&1`.nothrow() pattern to a synchronous Bun.spawnSync invocation of
LOADER providing the input string "const x = 1\nconst y = x +" (or equivalent)
and then build out from res.stdout and res.stderr (or combined output) into the
same out variable used by the existing checks; keep the existing asserts that
inspect res.exitCode, out.includes("SyntaxError"), out.includes("^"), and the
suggestion hint checks unchanged but read output from the Bun.spawnSync result
instead of res.text().
🪄 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: 07f8dc79-aadc-4f3e-b674-f7bf0655855d
📒 Files selected for processing (8)
scripts/test-cli-apps.tsscripts/test-cli-config.tsscripts/test-cli-lexer.tsscripts/test-cli-parser.tsscripts/test-cli.tssource/units/Goccia.Compiler.Statements.passource/units/Goccia.VM.pastests/built-ins/Object/getPrototypeOf.js
✅ Files skipped from review due to trivial changes (1)
- tests/built-ins/Object/getPrototypeOf.js
Comment on lines
+279
to
+284
| console.log("Loader: stdin --source-map rejection..."); | ||
| const stdinRes = await $`echo 'const x = 1;' | ${LOADER} --source-map 2>&1`.nothrow(); | ||
| const stdinOut = stdinRes.text().toLowerCase(); | ||
| if (!stdinOut.includes("error") && !stdinOut.includes("cannot") && !stdinOut.includes("require")) { | ||
| throw new Error(`Stdin --source-map should produce an error, got: ${stdinRes.text()}`); | ||
| } |
Contributor
There was a problem hiding this comment.
Assert non-zero exit for the stdin --source-map rejection too.
Right now this only checks the text. If the loader starts ignoring --source-map on stdin and returns success with a warning, this block can still pass. Please assert the rejection via exitCode before inspecting the message.
Suggested change
console.log("Loader: stdin --source-map rejection...");
const stdinRes = await $`echo 'const x = 1;' | ${LOADER} --source-map 2>&1`.nothrow();
+ if (stdinRes.exitCode === 0) {
+ throw new Error("Stdin --source-map should exit non-zero");
+ }
const stdinOut = stdinRes.text().toLowerCase();
if (!stdinOut.includes("error") && !stdinOut.includes("cannot") && !stdinOut.includes("require")) {
throw new Error(`Stdin --source-map should produce an error, got: ${stdinRes.text()}`);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| console.log("Loader: stdin --source-map rejection..."); | |
| const stdinRes = await $`echo 'const x = 1;' | ${LOADER} --source-map 2>&1`.nothrow(); | |
| const stdinOut = stdinRes.text().toLowerCase(); | |
| if (!stdinOut.includes("error") && !stdinOut.includes("cannot") && !stdinOut.includes("require")) { | |
| throw new Error(`Stdin --source-map should produce an error, got: ${stdinRes.text()}`); | |
| } | |
| console.log("Loader: stdin --source-map rejection..."); | |
| const stdinRes = await $`echo 'const x = 1;' | ${LOADER} --source-map 2>&1`.nothrow(); | |
| if (stdinRes.exitCode === 0) { | |
| throw new Error("Stdin --source-map should exit non-zero"); | |
| } | |
| const stdinOut = stdinRes.text().toLowerCase(); | |
| if (!stdinOut.includes("error") && !stdinOut.includes("cannot") && !stdinOut.includes("require")) { | |
| throw new Error(`Stdin --source-map should produce an error, got: ${stdinRes.text()}`); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/test-cli-apps.ts` around lines 279 - 284, The test currently only
inspects stdinRes.text() for errors when invoking the loader with --source-map;
update the block that runs ${LOADER} (variables LOADER, stdinRes, stdinOut) to
first assert stdinRes.exitCode is non-zero (fail the test if exitCode === 0)
before checking the output text, so we reject successful exit with a warning;
keep the existing output assertions after the exitCode check.
Comment on lines
+103
to
+132
| console.log("--compat-var (Loader + Bundler + TestRunner)..."); | ||
| { | ||
| const tmp = mkdtempSync(join(tmpdir(), "goccia-var-")); | ||
| try { | ||
| const src = join(tmp, "use-var.js"); | ||
| writeFileSync(src, "var x = 10;\nx;\n"); | ||
|
|
||
| // Loader with --compat-var | ||
| const loaderOut = await $`${LOADER} ${src} --compat-var 2>&1`.text(); | ||
| if (!loaderOut.includes("Result: 10")) throw new Error(`Loader --compat-var expected Result: 10, got: ${loaderOut}`); | ||
|
|
||
| // Bundler with --compat-var | ||
| await $`${BUNDLER} ${src} --compat-var`.quiet(); | ||
| if (!existsSync(join(tmp, "use-var.gbc"))) throw new Error("Bundler --compat-var should produce .gbc"); | ||
|
|
||
| // TestRunner with --compat-var | ||
| const testSrc = join(tmp, "test-var.js"); | ||
| writeFileSync( | ||
| testSrc, | ||
| [ | ||
| "var y = 20;", | ||
| 'describe("var", () => {', | ||
| ' test("works", () => {', | ||
| " expect(y).toBe(20);", | ||
| " });", | ||
| "});", | ||
| ].join("\n") + "\n", | ||
| ); | ||
| const trOut = await $`${TESTRUNNER} ${testSrc} --compat-var --no-progress 2>&1`.text(); | ||
| if (!trOut.includes("Passed: 1")) throw new Error(`TestRunner --compat-var expected Passed: 1, got: ${trOut}`); |
Contributor
There was a problem hiding this comment.
Add one no-flag assertion for --compat-var.
This block only proves the positive path. If var becomes accepted unconditionally, all three checks still pass. A single baseline failure before enabling the flag would catch that regression.
Suggested change
try {
const src = join(tmp, "use-var.js");
writeFileSync(src, "var x = 10;\nx;\n");
+ const loaderNoCompat = await $`${LOADER} ${src} 2>&1`.nothrow();
+ if (loaderNoCompat.exitCode === 0) {
+ throw new Error("Loader should reject `var` without --compat-var");
+ }
+
// Loader with --compat-var
const loaderOut = await $`${LOADER} ${src} --compat-var 2>&1`.text();
if (!loaderOut.includes("Result: 10")) throw new Error(`Loader --compat-var expected Result: 10, got: ${loaderOut}`);Based on learnings: JavaScript tests should cover happy paths, edge cases, and error cases. Keep tests isolated and grouped by feature/filename.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/test-cli.ts` around lines 103 - 132, Add a baseline "no-flag"
assertion before the existing --compat-var checks: run the same inputs against
LOADER, BUNDLER and TESTRUNNER without the --compat-var flag (use the same src
"use-var.js" and test "test-var.js") and assert the negative expectations
(Loader output does NOT include "Result: 10", Bundler does NOT emit use-var.gbc,
TestRunner does NOT report "Passed: 1" / exits with failure). Place these checks
immediately before the current flagged checks so regressions that accept var
unconditionally are caught; reference LOADER, BUNDLER, TESTRUNNER, src
(use-var.js) and testSrc (test-var.js) to locate where to insert them.
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 `@source/units/Goccia.VM.pas`:
- Around line 4465-4475: Computed writes to class constructors are routed
through TGocciaClassValue.SetProperty (via
KeyToPropertyNameRegister/RegisterToValue) which creates missing static
properties without pfEnumerable; instead, treat computed writes like
symbol-keyed assignments and call TGocciaClassValue.AssignSymbolProperty so the
created static uses the same descriptor flags (including pfEnumerable) as normal
static fields. Locate the branch using KeyToPropertyNameRegister and
RegisterToValue and replace the SetProperty call with an AssignSymbolProperty
call that converts the computed key to a TGocciaSymbolValue (mirroring the
existing symbol branch) or otherwise ensure the property is created with
pfEnumerable when falling back from computed-key writes.
- Around line 4407-4427: OP_GET_INDEX must use the same class-first dispatch as
the OP_GET_PROPERTY code path: when handling index lookups check
FRegisters[B].Kind and whether FRegisters[B].ObjectValue is a TGocciaClassValue
before checking for TGocciaObjectValue so that
TGocciaClassValue.GetProperty/GetSymbolProperty (and its static getter/method
and superclass logic) run for computed reads; update the OP_GET_INDEX handler to
call SetRegister(A, TGocciaClassValue(...).GetSymbolProperty(...)) or
SetRegister(A,
TGocciaClassValue(...).GetProperty(KeyToPropertyNameRegister(...))) when B holds
a TGocciaClassValue, and only fall back to
TGocciaObjectValue.GetSymbolProperty/GetProperty afterwards.
🪄 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: 21789ee5-fe63-46aa-9d05-b869271de6ea
📒 Files selected for processing (1)
source/units/Goccia.VM.pas
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.Values.ClassValue.pas (1)
670-684:⚠️ Potential issue | 🟠 Major
AddAutoAccessorignores theAIsStaticparameter.The TC39 proposal supports
static accessor a = 1syntax, meaning auto-accessors can be either instance or static. For instance auto-accessors, the getter/setter should be on the prototype (FClassPrototype). For static auto-accessors, they should be on the class itself (Self).Currently, the method always defines on
FClassPrototyperegardless ofAIsStatic, which means static auto-accessors would incorrectly be placed on the prototype instead of the class.🐛 Proposed fix to handle static vs instance auto-accessors
procedure TGocciaClassValue.AddAutoAccessor(const AName, ABackingName: string; const AIsStatic: Boolean); var GetterHelper: TGocciaAutoAccessorGetter; SetterHelper: TGocciaAutoAccessorSetter; GetterFn, SetterFn: TGocciaNativeFunctionValue; + Target: TGocciaObjectValue; begin GetterHelper := TGocciaAutoAccessorGetter.Create(ABackingName); SetterHelper := TGocciaAutoAccessorSetter.Create(ABackingName); GetterFn := TGocciaNativeFunctionValue.CreateWithoutPrototype(GetterHelper.Get, 'get ' + AName, 0); SetterFn := TGocciaNativeFunctionValue.CreateWithoutPrototype(SetterHelper.SetValue, 'set ' + AName, 1); - FClassPrototype.DefineProperty(AName, TGocciaPropertyDescriptorAccessor.Create( + if AIsStatic then + Target := Self + else + Target := FClassPrototype; + + Target.DefineProperty(AName, TGocciaPropertyDescriptorAccessor.Create( GetterFn, SetterFn, [pfEnumerable, pfConfigurable, pfWritable])); end;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/units/Goccia.Values.ClassValue.pas` around lines 670 - 684, The AddAutoAccessor method (TGocciaClassValue.AddAutoAccessor) currently always defines the accessor on FClassPrototype and ignores the AIsStatic flag; update it to choose the target object based on AIsStatic (use Self for static accessors, FClassPrototype for instance accessors) and call DefineProperty on that chosen target with the created GetterFn and SetterFn (TGocciaPropertyDescriptorAccessor), preserving the existing flags and names so static accessors are attached to the class and instance accessors to the prototype.
🧹 Nitpick comments (1)
source/units/Goccia.Values.FunctionBase.pas (1)
695-701: Double property read inGetFunctionLength.
GetProperty(PROP_LENGTH)is called twice: once in the type check and again to extract the value. This is inefficient and could produce inconsistent results if the getter has side effects.♻️ Proposed fix to store the result in a local
function TGocciaBoundFunctionValue.GetFunctionLength: Integer; var OrigLength: Integer; + LengthVal: TGocciaValue; begin // ECMAScript: bound function length = max(0, originalLength - boundArgs.length) if FOriginalFunction is TGocciaFunctionBase then OrigLength := TGocciaFunctionBase(FOriginalFunction).GetFunctionLength else if FOriginalFunction is TGocciaClassValue then begin OrigLength := 0; - if TGocciaClassValue(FOriginalFunction).GetProperty(PROP_LENGTH) is TGocciaNumberLiteralValue then - OrigLength := Trunc(TGocciaNumberLiteralValue( - TGocciaClassValue(FOriginalFunction).GetProperty(PROP_LENGTH)).Value); + LengthVal := TGocciaClassValue(FOriginalFunction).GetProperty(PROP_LENGTH); + if LengthVal is TGocciaNumberLiteralValue then + OrigLength := Trunc(TGocciaNumberLiteralValue(LengthVal).Value); end else OrigLength := 0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/units/Goccia.Values.FunctionBase.pas` around lines 695 - 701, In GetFunctionLength, avoid calling TGocciaClassValue(FOriginalFunction).GetProperty(PROP_LENGTH) twice: fetch the result once into a local variable (e.g., LProp := TGocciaClassValue(FOriginalFunction).GetProperty(PROP_LENGTH)), check if LProp is a TGocciaNumberLiteralValue and only then extract and Trunc its .Value into OrigLength; ensure you handle LProp = nil safely. Update references in the TGocciaClassValue branch to use LProp instead of repeated GetProperty calls.
🤖 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 `@source/units/Goccia.Values.ClassValue.pas`:
- Around line 670-684: The AddAutoAccessor method
(TGocciaClassValue.AddAutoAccessor) currently always defines the accessor on
FClassPrototype and ignores the AIsStatic flag; update it to choose the target
object based on AIsStatic (use Self for static accessors, FClassPrototype for
instance accessors) and call DefineProperty on that chosen target with the
created GetterFn and SetterFn (TGocciaPropertyDescriptorAccessor), preserving
the existing flags and names so static accessors are attached to the class and
instance accessors to the prototype.
---
Nitpick comments:
In `@source/units/Goccia.Values.FunctionBase.pas`:
- Around line 695-701: In GetFunctionLength, avoid calling
TGocciaClassValue(FOriginalFunction).GetProperty(PROP_LENGTH) twice: fetch the
result once into a local variable (e.g., LProp :=
TGocciaClassValue(FOriginalFunction).GetProperty(PROP_LENGTH)), check if LProp
is a TGocciaNumberLiteralValue and only then extract and Trunc its .Value into
OrigLength; ensure you handle LProp = nil safely. Update references in the
TGocciaClassValue branch to use LProp instead of repeated GetProperty calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b8d75474-0304-4cac-b1d1-c4f18408379d
📒 Files selected for processing (5)
source/units/Goccia.Builtins.GlobalObject.passource/units/Goccia.Engine.passource/units/Goccia.Runtime.Bootstrap.passource/units/Goccia.Values.ClassValue.passource/units/Goccia.Values.FunctionBase.pas
🚧 Files skipped from review as they are similar to previous changes (2)
- source/units/Goccia.Builtins.GlobalObject.pas
- source/units/Goccia.Engine.pas
- Add compat-var and BigInt typed array handling to the Test262 runner - Fix constructor/prototype property descriptors and class name inference - Update built-in and language tests for the newly supported semantics
- cover loader, bundler, benchmark runner, and REPL app behaviors - add config discovery, per-file overrides, source maps, and coverage cases - fix statement and VM handling for Test262 object prototype checks
- Swap TGocciaClassValue/TGocciaObjectValue checks in VM property ops - Link derived class constructors to superclass via [[Prototype]]
…essor flags - Reorder TGocciaClassValue/TGocciaObjectValue type checks in VM ops (OP_ARRAY_GET, OP_ARRAY_SET) so the more specific class check runs first — fixes Symbol.species, static computed symbol accessors, and static property inheritance in bytecode mode - Set [[Prototype]] on derived class in OP_CLASS_SET_SUPER for static property inheritance via prototype chain - PatchDefaultPrototype for TextEncoder, TextDecoder, URL, and URLSearchParams constructors in both Engine and Bootstrap - ObjectDefineProperties: process symbol-keyed descriptor entries - BoundFunction name/length: delegate to TGocciaClassValue metadata when the original function is a class constructor - Class accessors (AddGetter/AddSetter): omit pfEnumerable per ES §14.3.8/§14.3.9 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
frostney
force-pushed
the
t3code/test262-feature-gaps
branch
from
ad398ac to
570fe7d
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
source/units/Goccia.VM.pas (1)
4510-4555:⚠️ Potential issue | 🟠 MajorBytecode class accessors are still created as enumerable.
Lines 4514-4555 fix
constructorand prototype methods, but Lines 5843-5886 still route class accessors throughDefineGetterProperty*/DefineSetterProperty*, and those helpers build descriptors withpfEnumerable. That means bytecode-compiledget/setclass elements still expose the wrong enumerability even after this method fix.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/units/Goccia.VM.pas` around lines 4510 - 4555, Bytecode-compiled class accessors are being created with pfEnumerable because the helpers DefineGetterProperty*/DefineSetterProperty* always add pfEnumerable; update those helpers (DefineGetterProperty, DefineSetterProperty) to either accept an explicit enumerability parameter or detect when the target object is a class/prototype (e.g., TGocciaVMClassValue.Prototype or TGocciaClassValue.Prototype) and create the property descriptor without pfEnumerable (use [pfConfigurable, pfWritable] like in OP_CLASS_ADD_METHOD_CONST for prototype methods/constructor), so get/set class elements are non-enumerable.source/units/Goccia.Values.ClassValue.pas (1)
918-927:⚠️ Potential issue | 🟠 MajorAdd 'prototype' as an own property in
TGocciaClassValue.
GetProperty('prototype')returnsFClassPrototype, butHasOwnPropertyandGetOwnPropertyDescriptordon't recognize it because 'prototype' is not in the property table. Either override these methods to handle 'prototype' specially, or callDefineProperty(PROP_PROTOTYPE, ...)during construction like native constructors do, to ensureObject.getOwnPropertyDescriptor(C, 'prototype')andC.hasOwnProperty('prototype')return correct values matching ordinary reads.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/units/Goccia.Values.ClassValue.pas` around lines 918 - 927, TGocciaClassValue currently returns FClassPrototype from GetProperty when AName = PROP_PROTOTYPE but does not expose it as an "own" property; update TGocciaClassValue to define 'prototype' as an own property during construction (or initialization) by calling DefineProperty(PROP_PROTOTYPE, FClassPrototype, <appropriate attributes>) so that HasOwnProperty and GetOwnPropertyDescriptor recognize it, or alternatively override HasOwnProperty and GetOwnPropertyDescriptor in TGocciaClassValue to special-case PROP_PROTOTYPE and return the descriptor/value tied to FClassPrototype; reference TGocciaClassValue.GetProperty, TGocciaClassValue.HasOwnProperty, TGocciaClassValue.GetOwnPropertyDescriptor, PROP_PROTOTYPE, FClassPrototype, and the class constructor/initializer when applying the change.
♻️ Duplicate comments (1)
source/units/Goccia.VM.pas (1)
4407-4427:⚠️ Potential issue | 🟠 Major
OP_GET_INDEXis still object-first, soC[key]can bypass class static lookup.This branch is fixed, but Line 4701 still checks
TGocciaObjectValuebeforeTGocciaClassValueinOP_GET_INDEX. Since class values are object-like here, bracket reads in bytecode mode can still missTGocciaClassValue.GetProperty/GetSymbolPropertyand skip superclass walking for static members. Based on learnings: InExecuteClosureRegistersInternal, property/index ops must dispatch "class-first" so static lookup and superclass walking happen.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/units/Goccia.VM.pas` around lines 4407 - 4427, The OP_GET_INDEX handling is dispatching object-first so class static lookups and superclass walking are bypassed; fix ExecuteClosureRegistersInternal by checking for TGocciaClassValue before TGocciaObjectValue when inspecting FRegisters[B] (and keeping the existing TGocciaSymbolValue check for FRegisters[C] inside that branch), i.e., move or duplicate the TGocciaClassValue branch so the code tests "(FRegisters[B].ObjectValue is TGocciaClassValue)" prior to the "(... is TGocciaObjectValue)" branch and call TGocciaClassValue.GetProperty/GetSymbolProperty there to ensure static member resolution and superclass traversal occur.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@source/units/Goccia.Runtime.Bootstrap.pas`:
- Around line 443-447: The property creation for the %TypedArray% intrinsic uses
SetProperty which makes writable/configurable properties; replace those
SetProperty calls for PROP_NAME and PROP_LENGTH on FTypedArrayIntrinsic with
DefineProperty calls that use TGocciaStringLiteralValue('TypedArray') and
TGocciaNumberLiteralValue(0) respectively and set the descriptor flags to make
them non-writable (and non-enumerable/non-configurable per built-in constructor
semantics). In other words, instead of FTypedArrayIntrinsic.SetProperty(...),
call FTypedArrayIntrinsic.DefineProperty(PROP_NAME, <string value>, writable:
false, enumerable: false, configurable: false) and similarly for PROP_LENGTH
with the number value.
In `@source/units/Goccia.Values.ClassValue.pas`:
- Around line 682-683: The class auto-accessor is being defined with
pfEnumerable which makes it enumerable; update the AddAutoAccessor path so
FClassPrototype.DefineProperty uses TGocciaPropertyDescriptorAccessor with flags
[pfConfigurable, pfWritable] (same as AddGetter/AddSetter) instead of including
pfEnumerable; locate the code that calls FClassPrototype.DefineProperty for
auto-accessors (the AddAutoAccessor implementation) and remove pfEnumerable from
the descriptor flags to make the auto-accessor non-enumerable per ECMAScript
class accessor semantics.
---
Outside diff comments:
In `@source/units/Goccia.Values.ClassValue.pas`:
- Around line 918-927: TGocciaClassValue currently returns FClassPrototype from
GetProperty when AName = PROP_PROTOTYPE but does not expose it as an "own"
property; update TGocciaClassValue to define 'prototype' as an own property
during construction (or initialization) by calling
DefineProperty(PROP_PROTOTYPE, FClassPrototype, <appropriate attributes>) so
that HasOwnProperty and GetOwnPropertyDescriptor recognize it, or alternatively
override HasOwnProperty and GetOwnPropertyDescriptor in TGocciaClassValue to
special-case PROP_PROTOTYPE and return the descriptor/value tied to
FClassPrototype; reference TGocciaClassValue.GetProperty,
TGocciaClassValue.HasOwnProperty, TGocciaClassValue.GetOwnPropertyDescriptor,
PROP_PROTOTYPE, FClassPrototype, and the class constructor/initializer when
applying the change.
In `@source/units/Goccia.VM.pas`:
- Around line 4510-4555: Bytecode-compiled class accessors are being created
with pfEnumerable because the helpers
DefineGetterProperty*/DefineSetterProperty* always add pfEnumerable; update
those helpers (DefineGetterProperty, DefineSetterProperty) to either accept an
explicit enumerability parameter or detect when the target object is a
class/prototype (e.g., TGocciaVMClassValue.Prototype or
TGocciaClassValue.Prototype) and create the property descriptor without
pfEnumerable (use [pfConfigurable, pfWritable] like in OP_CLASS_ADD_METHOD_CONST
for prototype methods/constructor), so get/set class elements are
non-enumerable.
---
Duplicate comments:
In `@source/units/Goccia.VM.pas`:
- Around line 4407-4427: The OP_GET_INDEX handling is dispatching object-first
so class static lookups and superclass walking are bypassed; fix
ExecuteClosureRegistersInternal by checking for TGocciaClassValue before
TGocciaObjectValue when inspecting FRegisters[B] (and keeping the existing
TGocciaSymbolValue check for FRegisters[C] inside that branch), i.e., move or
duplicate the TGocciaClassValue branch so the code tests
"(FRegisters[B].ObjectValue is TGocciaClassValue)" prior to the "(... is
TGocciaObjectValue)" branch and call
TGocciaClassValue.GetProperty/GetSymbolProperty there to ensure static member
resolution and superclass traversal occur.
🪄 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: 6e22cefe-a1d0-44fe-92df-eddb901738d5
📒 Files selected for processing (34)
scripts/run_test262_suite.pyscripts/test262_syntax_filter.pysource/units/Goccia.AST.Statements.passource/units/Goccia.Builtins.GlobalObject.passource/units/Goccia.Builtins.GlobalRegExp.passource/units/Goccia.Builtins.Globals.passource/units/Goccia.Compiler.Statements.passource/units/Goccia.Engine.passource/units/Goccia.Evaluator.passource/units/Goccia.ObjectModel.Engine.passource/units/Goccia.Runtime.Bootstrap.passource/units/Goccia.SharedPrototype.passource/units/Goccia.VM.passource/units/Goccia.Values.ArrayValue.passource/units/Goccia.Values.ClassValue.passource/units/Goccia.Values.FunctionBase.passource/units/Goccia.Values.TypedArrayValue.pastests/built-ins/Array/isArray.jstests/built-ins/Array/prototype/map.jstests/built-ins/Error/error-constructor.jstests/built-ins/Function/prototype/apply.jstests/built-ins/Function/prototype/bind.jstests/built-ins/Function/prototype/call.jstests/built-ins/Function/prototype/constructor.jstests/built-ins/Map/constructor.jstests/built-ins/Map/prototype/get.jstests/built-ins/Object/create.jstests/built-ins/Object/getPrototypeOf.jstests/built-ins/Object/keys.jstests/built-ins/Set/constructor.jstests/built-ins/Set/prototype/has.jstests/built-ins/TypedArray/constructors.jstests/language/classes/basic-class-declaration.jstests/language/classes/method-enumerable.js
✅ Files skipped from review due to trivial changes (14)
- tests/built-ins/Set/prototype/has.js
- tests/built-ins/Set/constructor.js
- tests/built-ins/Array/prototype/map.js
- tests/built-ins/Function/prototype/constructor.js
- tests/built-ins/Map/prototype/get.js
- tests/built-ins/Object/keys.js
- tests/built-ins/Array/isArray.js
- tests/language/classes/method-enumerable.js
- source/units/Goccia.Compiler.Statements.pas
- tests/built-ins/TypedArray/constructors.js
- tests/built-ins/Error/error-constructor.js
- tests/built-ins/Object/getPrototypeOf.js
- tests/built-ins/Function/prototype/bind.js
- tests/built-ins/Function/prototype/call.js
🚧 Files skipped from review as they are similar to previous changes (12)
- tests/built-ins/Function/prototype/apply.js
- source/units/Goccia.ObjectModel.Engine.pas
- source/units/Goccia.Builtins.GlobalRegExp.pas
- source/units/Goccia.SharedPrototype.pas
- tests/language/classes/basic-class-declaration.js
- tests/built-ins/Object/create.js
- source/units/Goccia.Values.TypedArrayValue.pas
- source/units/Goccia.Builtins.Globals.pas
- source/units/Goccia.AST.Statements.pas
- scripts/run_test262_suite.py
- source/units/Goccia.Builtins.GlobalObject.pas
- source/units/Goccia.Engine.pas
…lity - %TypedArray%.name and .length: use DefineProperty with [pfConfigurable] instead of SetProperty, making them non-writable per ES §17 - Auto-accessor (AddAutoAccessor): remove pfEnumerable to match class accessor semantics (AddGetter/AddSetter already fixed) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
source/units/Goccia.Compiler.Statements.pas (1)
560-563:⚠️ Potential issue | 🟠 MajorDo not turn anonymous class expressions into named class expressions for inferred names.
At Line 563, writing
ClassDefinition.FName := Info.Namechanges semantics:CompileClassExpressionwill treat it as named (HasNameBindingat Line 2558), creating inner class-name binding behavior instead of just inferring.name.Suggested direction
- if (Info.Initializer is TGocciaClassExpression) and - (TGocciaClassExpression(Info.Initializer).ClassDefinition.Name = '') then - TGocciaClassExpression(Info.Initializer).ClassDefinition.FName := Info.Name; + // Keep class definition anonymous here. + // Apply inferred class name on the produced class value instead.#!/bin/bash # Verify the mutation site and its semantic coupling to name-binding logic. rg -n -A6 -B6 'ClassDefinition\.FName := Info\.Name' source/units/Goccia.Compiler.Statements.pas rg -n -A8 -B6 'HasNameBinding := ClassDef\.Name <> ' source/units/Goccia.Compiler.Statements.pas🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/units/Goccia.Compiler.Statements.pas` around lines 560 - 563, The code assigns Info.Name to ClassDefinition.FName which turns an anonymous class into a named class (affecting CompileClassExpression's HasNameBinding logic); remove that assignment and instead record the inferred name without setting FName (e.g., introduce/use a separate property such as ClassDefinition.InferredName or a runtime-only name field) so the class keeps Name = '' for binding checks while still exposing the inferred .name; update TGocciaClassExpression/ClassDefinition to hold and use that inferred-name slot where needed, and ensure CompileClassExpression/HasNameBinding continue to rely on ClassDefinition.Name.source/units/Goccia.Engine.pas (1)
936-955:⚠️ Potential issue | 🟠 MajorBackfill
HeadersandResponsetoo.
PatchDefaultPrototypeonly fixes the exact constructor instance you pass in.HeadersandResponseare still registered beforeFunctionConstructor, but unlikeTextEncoder/URLthey are never saved and never patched here, so both globals stay detached fromFunction.prototype.Suggested fix
URLConstructor: TGocciaURLClassValue; URLSearchParamsConstructor: TGocciaURLSearchParamsClassValue; - TextEncoderConstructor, TextDecoderConstructor: TGocciaClassValue; + TextEncoderConstructor, TextDecoderConstructor: TGocciaClassValue; + HeadersConstructor, ResponseConstructor: TGocciaClassValue; TypeDef: TGocciaTypeDefinition; @@ TypeDef.AddSpeciesGetter := False; RegisterTypeDefinition(FInterpreter.GlobalScope, TypeDef, SpeciesGetter, GenericConstructor); + HeadersConstructor := GenericConstructor; @@ TypeDef.AddSpeciesGetter := False; RegisterTypeDefinition(FInterpreter.GlobalScope, TypeDef, SpeciesGetter, GenericConstructor); + ResponseConstructor := GenericConstructor; @@ TGocciaClassValue.PatchDefaultPrototype(TextEncoderConstructor); TGocciaClassValue.PatchDefaultPrototype(TextDecoderConstructor); TGocciaClassValue.PatchDefaultPrototype(URLConstructor); TGocciaClassValue.PatchDefaultPrototype(URLSearchParamsConstructor); + TGocciaClassValue.PatchDefaultPrototype(HeadersConstructor); + TGocciaClassValue.PatchDefaultPrototype(ResponseConstructor); TGocciaClassValue.PatchDefaultPrototype(FunctionConstructor);Also applies to: 1041-1057
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/units/Goccia.Engine.pas` around lines 936 - 955, TypeDef instances for CONSTRUCTOR_HEADERS and CONSTRUCTOR_RESPONSE are registered before FunctionConstructor and thus never patched by PatchDefaultPrototype; after FunctionConstructor is created and patched, locate where PatchDefaultPrototype is invoked for TextEncoder/URL and call the same for the Headers and Response constructors (the TypeDef entries with ConstructorName = CONSTRUCTOR_HEADERS and CONSTRUCTOR_RESPONSE or the concrete TGocciaHeadersClassValue/TGocciaResponseClassValue constructor objects) so their prototype chains are attached to Function.prototype; in short, ensure you backfill/patch the Headers and Response constructors after FunctionConstructor initialization by invoking PatchDefaultPrototype (or the equivalent patching routine) for those two constructors just like was done for TextEncoder/URL.
🧹 Nitpick comments (1)
tests/built-ins/Object/getPrototypeOf.js (1)
54-64: Test name says "interpreted" but describes cross-mode behavior.The test name includes "(interpreted)" but the comment explicitly states it's testing "cross-mode compatibility." Consider renaming for clarity.
✏️ Suggested rename
- test("returns superclass for a class with extends (interpreted)", () => { + test("returns superclass for a class with extends", () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/built-ins/Object/getPrototypeOf.js` around lines 54 - 64, Rename the test case whose title is "returns superclass for a class with extends (interpreted)" to reflect cross-mode behavior: update the test name string in the test containing classes A and B and the call Object.getPrototypeOf(B) to something like "returns superclass for a class with extends (cross-mode compatible)" or "returns superclass for a class with extends (cross-mode)"; keep the test body (A, B, Object.getPrototypeOf(B), and the prototype assertions) unchanged so the intent and assertions remain identical while the title matches the comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@source/units/Goccia.Values.ClassValue.pas`:
- Around line 302-306: The code currently skips creating the class .name own
property when FName = '' which breaks anonymous-class own-property semantics;
always call DefineProperty for PROP_NAME using TGocciaPropertyDescriptorData
with TGocciaStringLiteralValue.Create(FName) (allowing empty string) and the
same descriptor flags ([pfConfigurable]) instead of conditionally gating on
FName, so update the logic in the class constructor/initialization (references:
FName, PROP_NAME, DefineProperty, TGocciaPropertyDescriptorData,
TGocciaStringLiteralValue.Create) to unconditionally define the .name property.
In `@source/units/Goccia.Values.FunctionBase.pas`:
- Around line 314-325: DispatchCall currently only handles TGocciaFunctionBase
and TGocciaClassValue, so callable proxies (where ACallee.IsCallable = True)
fall through and raise 'not callable' instead of invoking their apply trap;
change DispatchCall to check the value's IsCallable property and, when true,
invoke the general call path on the value itself (use the value's Call method)
instead of only casting to TGocciaFunctionBase/TGocciaClassValue, e.g. first
test ACallee.IsCallable and call ACallee.Call(AArgs, AThisValue), otherwise keep
the existing checks for TGocciaFunctionBase/TGocciaClassValue and finally raise
the error.
---
Duplicate comments:
In `@source/units/Goccia.Compiler.Statements.pas`:
- Around line 560-563: The code assigns Info.Name to ClassDefinition.FName which
turns an anonymous class into a named class (affecting CompileClassExpression's
HasNameBinding logic); remove that assignment and instead record the inferred
name without setting FName (e.g., introduce/use a separate property such as
ClassDefinition.InferredName or a runtime-only name field) so the class keeps
Name = '' for binding checks while still exposing the inferred .name; update
TGocciaClassExpression/ClassDefinition to hold and use that inferred-name slot
where needed, and ensure CompileClassExpression/HasNameBinding continue to rely
on ClassDefinition.Name.
In `@source/units/Goccia.Engine.pas`:
- Around line 936-955: TypeDef instances for CONSTRUCTOR_HEADERS and
CONSTRUCTOR_RESPONSE are registered before FunctionConstructor and thus never
patched by PatchDefaultPrototype; after FunctionConstructor is created and
patched, locate where PatchDefaultPrototype is invoked for TextEncoder/URL and
call the same for the Headers and Response constructors (the TypeDef entries
with ConstructorName = CONSTRUCTOR_HEADERS and CONSTRUCTOR_RESPONSE or the
concrete TGocciaHeadersClassValue/TGocciaResponseClassValue constructor objects)
so their prototype chains are attached to Function.prototype; in short, ensure
you backfill/patch the Headers and Response constructors after
FunctionConstructor initialization by invoking PatchDefaultPrototype (or the
equivalent patching routine) for those two constructors just like was done for
TextEncoder/URL.
---
Nitpick comments:
In `@tests/built-ins/Object/getPrototypeOf.js`:
- Around line 54-64: Rename the test case whose title is "returns superclass for
a class with extends (interpreted)" to reflect cross-mode behavior: update the
test name string in the test containing classes A and B and the call
Object.getPrototypeOf(B) to something like "returns superclass for a class with
extends (cross-mode compatible)" or "returns superclass for a class with extends
(cross-mode)"; keep the test body (A, B, Object.getPrototypeOf(B), and the
prototype assertions) unchanged so the intent and assertions remain identical
while the title matches the comment.
🪄 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: f7a716f6-fad7-486c-8bdb-d84625adc0da
📒 Files selected for processing (34)
scripts/run_test262_suite.pyscripts/test262_syntax_filter.pysource/units/Goccia.AST.Statements.passource/units/Goccia.Builtins.GlobalObject.passource/units/Goccia.Builtins.GlobalRegExp.passource/units/Goccia.Builtins.Globals.passource/units/Goccia.Compiler.Statements.passource/units/Goccia.Engine.passource/units/Goccia.Evaluator.passource/units/Goccia.ObjectModel.Engine.passource/units/Goccia.Runtime.Bootstrap.passource/units/Goccia.SharedPrototype.passource/units/Goccia.VM.passource/units/Goccia.Values.ArrayValue.passource/units/Goccia.Values.ClassValue.passource/units/Goccia.Values.FunctionBase.passource/units/Goccia.Values.TypedArrayValue.pastests/built-ins/Array/isArray.jstests/built-ins/Array/prototype/map.jstests/built-ins/Error/error-constructor.jstests/built-ins/Function/prototype/apply.jstests/built-ins/Function/prototype/bind.jstests/built-ins/Function/prototype/call.jstests/built-ins/Function/prototype/constructor.jstests/built-ins/Map/constructor.jstests/built-ins/Map/prototype/get.jstests/built-ins/Object/create.jstests/built-ins/Object/getPrototypeOf.jstests/built-ins/Object/keys.jstests/built-ins/Set/constructor.jstests/built-ins/Set/prototype/has.jstests/built-ins/TypedArray/constructors.jstests/language/classes/basic-class-declaration.jstests/language/classes/method-enumerable.js
✅ Files skipped from review due to trivial changes (15)
- tests/built-ins/Map/prototype/get.js
- tests/built-ins/Array/prototype/map.js
- tests/built-ins/Set/constructor.js
- tests/built-ins/Object/keys.js
- tests/built-ins/Set/prototype/has.js
- tests/built-ins/Array/isArray.js
- tests/built-ins/Function/prototype/constructor.js
- tests/language/classes/basic-class-declaration.js
- tests/language/classes/method-enumerable.js
- tests/built-ins/Function/prototype/call.js
- source/units/Goccia.Builtins.GlobalObject.pas
- tests/built-ins/Function/prototype/bind.js
- tests/built-ins/Error/error-constructor.js
- tests/built-ins/Map/constructor.js
- source/units/Goccia.VM.pas
🚧 Files skipped from review as they are similar to previous changes (7)
- source/units/Goccia.Values.ArrayValue.pas
- tests/built-ins/Object/create.js
- source/units/Goccia.SharedPrototype.pas
- tests/built-ins/TypedArray/constructors.js
- tests/built-ins/Function/prototype/apply.js
- source/units/Goccia.ObjectModel.Engine.pas
- source/units/Goccia.Builtins.Globals.pas
- ClassValue constructor: unconditionally define .name own property (anonymous classes get empty string per ES spec) - Compiler: emit OP_ARRAY_SET for inferred class name instead of mutating AST FName, which broke HasNameBinding scope logic - DispatchCall: add comment clarifying callable proxy handling - Engine: save Headers/Response constructor refs and add PatchDefaultPrototype calls for both - Rename test case to drop "(interpreted)" qualifier Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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 (2)
669-683:⚠️ Potential issue | 🟠 MajorRespect
AIsStaticwhen installing auto-accessors.
AIsStaticis ignored here, sostatic accessor xis always defined onFClassPrototype. That puts the accessor on instances instead of the constructor and breaks static decorator output.Suggested fix
procedure TGocciaClassValue.AddAutoAccessor(const AName, ABackingName: string; const AIsStatic: Boolean); var GetterHelper: TGocciaAutoAccessorGetter; SetterHelper: TGocciaAutoAccessorSetter; GetterFn, SetterFn: TGocciaNativeFunctionValue; + Target: TGocciaObjectValue; begin GetterHelper := TGocciaAutoAccessorGetter.Create(ABackingName); SetterHelper := TGocciaAutoAccessorSetter.Create(ABackingName); GetterFn := TGocciaNativeFunctionValue.CreateWithoutPrototype(GetterHelper.Get, 'get ' + AName, 0); SetterFn := TGocciaNativeFunctionValue.CreateWithoutPrototype(SetterHelper.SetValue, 'set ' + AName, 1); - FClassPrototype.DefineProperty(AName, TGocciaPropertyDescriptorAccessor.Create( + if AIsStatic then + Target := Self + else + Target := FClassPrototype; + + Target.DefineProperty(AName, TGocciaPropertyDescriptorAccessor.Create( GetterFn, SetterFn, [pfConfigurable, pfWritable])); end;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/units/Goccia.Values.ClassValue.pas` around lines 669 - 683, The AddAutoAccessor method ignores AIsStatic and always installs accessors on FClassPrototype; update TGocciaClassValue.AddAutoAccessor to choose the target object based on AIsStatic (use FClassPrototype when AIsStatic = False and the class constructor object—e.g. FClassConstructor or whatever field holds the constructor—when AIsStatic = True) and call DefineProperty on that target with the existing GetterFn/SetterFn and TGocciaPropertyDescriptorAccessor.Create(...). Keep creation of GetterHelper/SetterHelper and GetterFn/SetterFn unchanged but branch on AIsStatic before the DefineProperty call so static accessors are attached to the constructor instead of the prototype.
954-979:⚠️ Potential issue | 🟠 MajorPreserve ordinary assignment semantics for runtime static writes.
After the setter lookup, this turns every
C.x = ...write into a non-enumerable define. That is correct for class-body statics and builtin slots, but not for ordinary userland assignment on constructor objects —Object.keys(C)should still see a property added viaC.x = 1.Suggested fix
procedure TGocciaClassValue.SetProperty(const AName: string; const AValue: TGocciaValue); var Setter: TGocciaFunctionBase; Args: TGocciaArgumentsCollection; Current: TGocciaClassValue; begin Current := Self; repeat if Current.FStaticSetters.TryGetValue(AName, Setter) then begin Args := TGocciaArgumentsCollection.CreateWithCapacity(1); try Args.Add(AValue); Setter.Call(Args, Self); finally Args.Free; end; Exit; end; Current := Current.FSuperClass; until not Assigned(Current); - inherited DefineProperty(AName, - TGocciaPropertyDescriptorData.Create(AValue, [pfConfigurable, pfWritable])); + inherited SetProperty(AName, AValue); end;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/units/Goccia.Values.ClassValue.pas` around lines 954 - 979, The fallback path in TGocciaClassValue.SetProperty currently calls inherited DefineProperty with a descriptor that omits pfEnumerable, making every runtime static write (e.g. C.x = 1) non-enumerable; update the fallback to define the property as enumerable for ordinary userland assignment by adding pfEnumerable to the TGocciaPropertyDescriptorData flags (i.e. use [pfConfigurable, pfWritable, pfEnumerable]) so constructor-object assignments become visible to Object.keys; keep this change localized to the fallback path in TGocciaClassValue.SetProperty where inherited DefineProperty is invoked.
🧹 Nitpick comments (1)
source/units/Goccia.Values.FunctionBase.pas (1)
696-702: Consider cachingGetPropertyresult to avoid double call.
GetProperty(PROP_LENGTH)is called twice—once for the type check and once for value extraction. Storing the result in a local variable improves clarity and avoids redundant work.♻️ Suggested refactor
else if FOriginalFunction is TGocciaClassValue then begin OrigLength := 0; - if TGocciaClassValue(FOriginalFunction).GetProperty(PROP_LENGTH) is TGocciaNumberLiteralValue then - OrigLength := Trunc(TGocciaNumberLiteralValue( - TGocciaClassValue(FOriginalFunction).GetProperty(PROP_LENGTH)).Value); + LengthVal := TGocciaClassValue(FOriginalFunction).GetProperty(PROP_LENGTH); + if LengthVal is TGocciaNumberLiteralValue then + OrigLength := Trunc(TGocciaNumberLiteralValue(LengthVal).Value); end(Declare
LengthVal: TGocciaValueat the top of the function.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/units/Goccia.Values.FunctionBase.pas` around lines 696 - 702, Cache the result of TGocciaClassValue(...).GetProperty(PROP_LENGTH) into a local variable (e.g., LengthVal: TGocciaValue) instead of calling GetProperty twice: first check "if LengthVal is TGocciaNumberLiteralValue" and then cast LengthVal to TGocciaNumberLiteralValue to set OrigLength (keep OrigLength assignment logic intact). Declare LengthVal at the top of the routine, replace both GetProperty(PROP_LENGTH) calls with LengthVal, and ensure the type cast uses LengthVal when extracting .Value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@source/units/Goccia.Values.ClassValue.pas`:
- Around line 809-812: GetProperty virtualizes the prototype but
ReplacePrototype (and the constructor Create code at ~918-927) only updates
FClassPrototype, leaving the PROP_PROTOTYPE descriptor out of sync with
own-property reads; modify TGocciaClassValue.ReplacePrototype and the
constructor initialization to also write a real own property entry into
FProperties under PROP_PROTOTYPE (create/update a property descriptor with the
new prototype value and appropriate attributes consistent with JS prototype
property semantics) so Object.getOwnPropertyDescriptor/hasOwnProperty reflect
the same value returned by GetProperty.
---
Outside diff comments:
In `@source/units/Goccia.Values.ClassValue.pas`:
- Around line 669-683: The AddAutoAccessor method ignores AIsStatic and always
installs accessors on FClassPrototype; update TGocciaClassValue.AddAutoAccessor
to choose the target object based on AIsStatic (use FClassPrototype when
AIsStatic = False and the class constructor object—e.g. FClassConstructor or
whatever field holds the constructor—when AIsStatic = True) and call
DefineProperty on that target with the existing GetterFn/SetterFn and
TGocciaPropertyDescriptorAccessor.Create(...). Keep creation of
GetterHelper/SetterHelper and GetterFn/SetterFn unchanged but branch on
AIsStatic before the DefineProperty call so static accessors are attached to the
constructor instead of the prototype.
- Around line 954-979: The fallback path in TGocciaClassValue.SetProperty
currently calls inherited DefineProperty with a descriptor that omits
pfEnumerable, making every runtime static write (e.g. C.x = 1) non-enumerable;
update the fallback to define the property as enumerable for ordinary userland
assignment by adding pfEnumerable to the TGocciaPropertyDescriptorData flags
(i.e. use [pfConfigurable, pfWritable, pfEnumerable]) so constructor-object
assignments become visible to Object.keys; keep this change localized to the
fallback path in TGocciaClassValue.SetProperty where inherited DefineProperty is
invoked.
---
Nitpick comments:
In `@source/units/Goccia.Values.FunctionBase.pas`:
- Around line 696-702: Cache the result of
TGocciaClassValue(...).GetProperty(PROP_LENGTH) into a local variable (e.g.,
LengthVal: TGocciaValue) instead of calling GetProperty twice: first check "if
LengthVal is TGocciaNumberLiteralValue" and then cast LengthVal to
TGocciaNumberLiteralValue to set OrigLength (keep OrigLength assignment logic
intact). Declare LengthVal at the top of the routine, replace both
GetProperty(PROP_LENGTH) calls with LengthVal, and ensure the type cast uses
LengthVal when extracting .Value.
🪄 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: 78c7dbd3-85ad-4c9a-8215-f71dbbec9573
📒 Files selected for processing (5)
source/units/Goccia.Compiler.Statements.passource/units/Goccia.Engine.passource/units/Goccia.Values.ClassValue.passource/units/Goccia.Values.FunctionBase.pastests/built-ins/Object/getPrototypeOf.js
🚧 Files skipped from review as they are similar to previous changes (1)
- source/units/Goccia.Compiler.Statements.pas
…lity - ClassValue: override GetOwnPropertyDescriptor/HasOwnProperty for .prototype so Object.getOwnPropertyDescriptor reflects FClassPrototype - ClassValue: AddAutoAccessor respects AIsStatic (static accessors go on the constructor, not prototype) - ClassValue: SetProperty fallback uses inherited SetProperty (creates enumerable properties for runtime assignment C.x = val) - ClassValue: .name property is writable for anonymous classes to allow bytecode name inference, non-writable for named classes - CopyStaticMembersToConstructor: use DefineProperty to preserve non-enumerable flags from source descriptors - FunctionBase: cache GetProperty(PROP_LENGTH) result in GetFunctionLength; add callable proxy support to DispatchCall and BoundFunction.Call via TGocciaProxyValue.ApplyTrap - Add tests for callable proxy with call/apply/bind Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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)
source/units/Goccia.Values.FunctionBase.pas (1)
696-723:⚠️ Potential issue | 🟠 MajorBound proxy metadata still collapses to
0/"bound ".Once
bind()accepts proxy targets,GetFunctionLengthandGetFunctionNamealso need to readlength/namefromFOriginalFunctionas a normal callable object. Right nowFunction.prototype.bind.call(new Proxy((a, b) => {}, {}), null, 1)will report fallback metadata instead of preserving the proxied callable’s values.Based on learnings, all values inherit from
TGocciaValue; prefer VMT methods likeGetProperty(Name)over type-gated branches for this kind of callable metadata lookup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/units/Goccia.Values.FunctionBase.pas` around lines 696 - 723, Update TGocciaBoundFunctionValue.GetFunctionLength and GetFunctionName to treat FOriginalFunction as a generic TGocciaValue when it is not a TGocciaFunctionBase or TGocciaClassValue: call FOriginalFunction.GetProperty(PROP_LENGTH) and, if that returns a TGocciaNumberLiteralValue, use its Value (truncate) to compute OrigLength; likewise call FOriginalFunction.GetProperty(PROP_NAME) and, if that returns a TGocciaStringLiteralValue, use its Value to build the bound name. Keep the existing special-case branches for TGocciaFunctionBase and TGocciaClassValue but add the fallback that queries properties via GetProperty on FOriginalFunction to preserve proxied callable metadata.
🧹 Nitpick comments (1)
source/units/Goccia.ObjectModel.Engine.pas (1)
67-73: Consider creating new descriptor objects for consistency with symbol handling.The change correctly preserves non-enumerable flags for built-in static methods. However, the same
Descriptorobject is reused (line 69) while symbol properties (lines 82-92) create fresh descriptor objects. This inconsistency is harmless for immutable built-in method values, but for uniformity and defensive coding, consider creating a new descriptor:♻️ Optional: Create fresh descriptor for consistency
// Copy with original descriptor flags (built-in static methods are non-enumerable) if Descriptor is TGocciaPropertyDescriptorData then - TGocciaObjectValue(AConstructor).DefineProperty(Key, Descriptor) + TGocciaObjectValue(AConstructor).DefineProperty(Key, + TGocciaPropertyDescriptorData.Create( + TGocciaPropertyDescriptorData(Descriptor).Value, + Descriptor.Flags)) else🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/units/Goccia.ObjectModel.Engine.pas` around lines 67 - 73, The code currently passes the existing Descriptor object directly into TGocciaObjectValue(AConstructor).DefineProperty for built-in static methods (using TGocciaPropertyDescriptorData), while symbol handling creates new descriptor instances; to make this consistent and defensive, construct a new TGocciaPropertyDescriptorData (or copy) that preserves Descriptor's attributes (value/getter/setter, configurable/writable/enumerable flags, etc.) and pass that new descriptor to DefineProperty for the AConstructor/Key case instead of reusing Descriptor.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@source/units/Goccia.Values.FunctionBase.pas`:
- Around line 615-620: The current bound-proxy call logic incorrectly rejects
callable targets via explicit type checks (FOriginalFunction is
TGocciaFunctionBase / TGocciaClassValue) causing fast-paths
(CallNoArgs/Call1/Call2) to throw; instead, use the VMT predicate IsCallable to
allow any callable TGocciaValue to proceed and route non-TGocciaFunctionBase
targets through the generic Call(...) path so ApplyTrap is reached. Replace the
type-check gates around FOriginalFunction in the BoundFunction.Call overrides
(and the similar blocks at the other two locations) with checks like "if not
FOriginalFunction.IsCallable then raise ..." and, when FOriginalFunction is
callable but not a TGocciaFunctionBase, call
Exit(Call(TGocciaArgumentsCollection.Create, AThisValue)) (or the appropriate
generic Call(...) invocation) rather than raising; keep special-casing only for
actual TGocciaFunctionBase instances so fast paths remain valid.
---
Outside diff comments:
In `@source/units/Goccia.Values.FunctionBase.pas`:
- Around line 696-723: Update TGocciaBoundFunctionValue.GetFunctionLength and
GetFunctionName to treat FOriginalFunction as a generic TGocciaValue when it is
not a TGocciaFunctionBase or TGocciaClassValue: call
FOriginalFunction.GetProperty(PROP_LENGTH) and, if that returns a
TGocciaNumberLiteralValue, use its Value (truncate) to compute OrigLength;
likewise call FOriginalFunction.GetProperty(PROP_NAME) and, if that returns a
TGocciaStringLiteralValue, use its Value to build the bound name. Keep the
existing special-case branches for TGocciaFunctionBase and TGocciaClassValue but
add the fallback that queries properties via GetProperty on FOriginalFunction to
preserve proxied callable metadata.
---
Nitpick comments:
In `@source/units/Goccia.ObjectModel.Engine.pas`:
- Around line 67-73: The code currently passes the existing Descriptor object
directly into TGocciaObjectValue(AConstructor).DefineProperty for built-in
static methods (using TGocciaPropertyDescriptorData), while symbol handling
creates new descriptor instances; to make this consistent and defensive,
construct a new TGocciaPropertyDescriptorData (or copy) that preserves
Descriptor's attributes (value/getter/setter, configurable/writable/enumerable
flags, etc.) and pass that new descriptor to DefineProperty for the
AConstructor/Key case instead of reusing Descriptor.
🪄 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: 1e3413e4-e0ac-464c-a908-b0b3dfd153f6
📒 Files selected for processing (4)
source/units/Goccia.ObjectModel.Engine.passource/units/Goccia.Values.ClassValue.passource/units/Goccia.Values.FunctionBase.pastests/built-ins/Proxy/apply.js
frostney
force-pushed
the
t3code/test262-feature-gaps
branch
from
84c9c82 to
f93b352
Compare
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 `@source/units/Goccia.Builtins.GlobalObject.pas`:
- Around line 653-664: The symbol-key loop is assigning descriptor objects
directly (via Obj.AssignSymbolProperty using
TGocciaPropertyDescriptorData.Value) and bypasses ToPropertyDescriptor, causing
wrong values and ignoring accessors; change the loop that iterates SymbolKeys
(from PropertiesDescriptor.GetOwnSymbols and GetOwnSymbolPropertyDescriptor) to
call ObjectDefineProperty(Obj, SymbolKeys[I], SymbolDesc) for each assigned
SymbolDesc (only when Assigned(SymbolDesc)), mirroring the string-key path so
descriptor objects are converted via ToPropertyDescriptor and accessors are
handled correctly instead of using Obj.AssignSymbolProperty or special-casing
TGocciaPropertyDescriptorData.
In `@source/units/Goccia.Runtime.Bootstrap.pas`:
- Around line 465-467: After replacing the prototype with the shared object in
the TypedArray block, the shared prototype’s own "constructor" property is not
set, leaving %TypedArray%.prototype.constructor missing; after calling
FTypedArrayIntrinsic.ReplacePrototype(TGocciaTypedArrayValue.GetSharedPrototypeObject)
set the shared prototype object's own "constructor" property to the TypedArray
constructor function (the FTypedArrayIntrinsic function object) — i.e. get
TGocciaTypedArrayValue.GetSharedPrototypeObject and define its "constructor"
property to reference the FTypedArrayIntrinsic function value with the same
attributes used in the Function block so %TypedArray%.prototype.constructor
exists on the shared prototype.
🪄 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: 47b60a63-f9e5-4efe-8c3c-300894d64a6c
📒 Files selected for processing (33)
scripts/run_test262_suite.pyscripts/test262_syntax_filter.pysource/units/Goccia.Builtins.GlobalObject.passource/units/Goccia.Builtins.GlobalRegExp.passource/units/Goccia.Builtins.Globals.passource/units/Goccia.Engine.passource/units/Goccia.Evaluator.passource/units/Goccia.ObjectModel.Engine.passource/units/Goccia.Runtime.Bootstrap.passource/units/Goccia.SharedPrototype.passource/units/Goccia.VM.passource/units/Goccia.Values.ArrayValue.passource/units/Goccia.Values.ClassValue.passource/units/Goccia.Values.FunctionBase.passource/units/Goccia.Values.TypedArrayValue.pastests/built-ins/Array/isArray.jstests/built-ins/Array/prototype/map.jstests/built-ins/Error/error-constructor.jstests/built-ins/Function/prototype/apply.jstests/built-ins/Function/prototype/bind.jstests/built-ins/Function/prototype/call.jstests/built-ins/Function/prototype/constructor.jstests/built-ins/Map/constructor.jstests/built-ins/Map/prototype/get.jstests/built-ins/Object/create.jstests/built-ins/Object/getPrototypeOf.jstests/built-ins/Object/keys.jstests/built-ins/Proxy/apply.jstests/built-ins/Set/constructor.jstests/built-ins/Set/prototype/has.jstests/built-ins/TypedArray/constructors.jstests/language/classes/basic-class-declaration.jstests/language/classes/method-enumerable.js
✅ Files skipped from review due to trivial changes (13)
- tests/built-ins/Set/constructor.js
- tests/built-ins/Map/constructor.js
- tests/built-ins/Function/prototype/constructor.js
- tests/built-ins/Set/prototype/has.js
- tests/built-ins/Object/keys.js
- tests/built-ins/Function/prototype/apply.js
- tests/built-ins/Map/prototype/get.js
- tests/language/classes/method-enumerable.js
- tests/built-ins/Function/prototype/bind.js
- tests/built-ins/Function/prototype/call.js
- tests/language/classes/basic-class-declaration.js
- tests/built-ins/Array/prototype/map.js
- source/units/Goccia.Values.ClassValue.pas
🚧 Files skipped from review as they are similar to previous changes (9)
- tests/built-ins/Proxy/apply.js
- tests/built-ins/Array/isArray.js
- source/units/Goccia.SharedPrototype.pas
- tests/built-ins/TypedArray/constructors.js
- tests/built-ins/Object/getPrototypeOf.js
- tests/built-ins/Error/error-constructor.js
- tests/built-ins/Object/create.js
- scripts/run_test262_suite.py
- source/units/Goccia.Engine.pas
…e-gaps # Conflicts: # source/units/Goccia.AST.Statements.pas
…lity - ClassValue: synthesize .name and .prototype descriptors in GetOwnPropertyDescriptor/HasOwnProperty overrides instead of storing in FProperties — avoids non-writable conflicts with static field overrides and bootstrap ReplacePrototype calls - ClassValue: SetProperty handles .name via DefineProperty to override the synthesized non-writable descriptor (configurable allows this) - ClassValue: SetInferredName simplified to just update FName - ClassValue: AddAutoAccessor respects AIsStatic parameter - CopyStaticMembersToConstructor: preserve descriptor flags from source - Remove redundant OP_ARRAY_SET for class name inference (main's CompileClassExpression handles it via InferredName parameter) - Fix duplicate SetInferredName declaration/implementation from rebase Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
BoundFunction.CallNoArgs/CallOneArg/CallTwoArgs used explicit type checks (TGocciaFunctionBase/TGocciaClassValue) that rejected callable proxies. Replace with IsCallable guard and route non-TGocciaFunctionBase targets through the generic Call path so ApplyTrap is reached. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
frostney
force-pushed
the
t3code/test262-feature-gaps
branch
from
860de19 to
2b43bf8
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 (3)
source/units/Goccia.Values.FunctionBase.pas (1)
693-723:⚠️ Potential issue | 🟡 MinorPreserve
bindmetadata for callable proxies too.
bindnow accepts proxy callables, butGetFunctionLengthandGetFunctionNamestill special-case onlyTGocciaFunctionBaseandTGocciaClassValue. A bound proxy therefore falls back to0and"bound "instead of reflecting the proxy-exposedlength/name.♻️ Possible fix
function TGocciaBoundFunctionValue.GetFunctionLength: Integer; var OrigLength: Integer; LengthVal: TGocciaValue; begin // ECMAScript: bound function length = max(0, originalLength - boundArgs.length) if FOriginalFunction is TGocciaFunctionBase then OrigLength := TGocciaFunctionBase(FOriginalFunction).GetFunctionLength - else if FOriginalFunction is TGocciaClassValue then + else if FOriginalFunction.IsCallable then begin OrigLength := 0; - LengthVal := TGocciaClassValue(FOriginalFunction).GetProperty(PROP_LENGTH); + LengthVal := FOriginalFunction.GetProperty(PROP_LENGTH); if LengthVal is TGocciaNumberLiteralValue then OrigLength := Trunc(TGocciaNumberLiteralValue(LengthVal).Value); end else OrigLength := 0; @@ -function TGocciaBoundFunctionValue.GetFunctionName: string; +function TGocciaBoundFunctionValue.GetFunctionName: string; +var + NameVal: TGocciaValue; begin // ECMAScript: bound function name = "bound " + originalName if FOriginalFunction is TGocciaFunctionBase then Result := 'bound ' + TGocciaFunctionBase(FOriginalFunction).GetFunctionName - else if FOriginalFunction is TGocciaClassValue then - Result := 'bound ' + TGocciaClassValue(FOriginalFunction).Name + else if FOriginalFunction.IsCallable then + begin + NameVal := FOriginalFunction.GetProperty(PROP_NAME); + if NameVal is TGocciaStringLiteralValue then + Result := 'bound ' + TGocciaStringLiteralValue(NameVal).Value + else + Result := 'bound '; + end else Result := 'bound '; end;source/units/Goccia.Values.ClassValue.pas (2)
513-520:⚠️ Potential issue | 🟠 MajorInstall static accessors as real own properties.
AddStaticGetterandAddStaticSetteronly populate the side maps, soC.x/C.x = ...work through the manual lookup path, but own-property reflection still cannot see the accessor.Object.getOwnPropertyDescriptor(C, 'x'),hasOwnProperty, and delete/redefine behavior will all be out of sync until the accessor descriptor is also defined on the constructor object itself. MirroringAddGetter/AddSetterwithDefinePropertyonSelfwould fix that.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/units/Goccia.Values.ClassValue.pas` around lines 513 - 520, AddStaticGetter and AddStaticSetter currently only add entries to FStaticGetters/FStaticSetters causing static accessors to be invisible to own-property reflection; mirror the pattern used by AddGetter/AddSetter by also calling DefineProperty on Self to create an accessor descriptor for the property name (using the same AGetter/ASetter and appropriate attributes) so Object.getOwnPropertyDescriptor(Self, AName), hasOwnProperty and delete/redefine behavior are correct; update TGocciaClassValue.AddStaticGetter and AddStaticSetter to both populate the side maps (FStaticGetters/FStaticSetters) and call DefineProperty(Self, AName, Descriptor) with the accessor descriptor constructed from AGetter/ASetter.
955-961:⚠️ Potential issue | 🟠 MajorThe synthesized
.namepath is now out of sync with real descriptor semantics.
GetPropertyreturnsFNamebefore consulting any real ownnamedescriptor,SetPropertyredefinesnameviaDefineProperty, andHasOwnPropertyhard-codesTrue. That means cases likestatic name = 123,Object.defineProperty(C, 'name', ...),delete C.name, and plainC.name = ...no longer follow the built-in non-writable-but-configurablenameproperty rules consistently. TheFNamefallback should only apply when no ownnamedescriptor exists, and ordinary assignment should stay on the normal write path.Also applies to: 1017-1025, 1032-1060
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/units/Goccia.Values.ClassValue.pas` around lines 955 - 961, GetProperty, SetProperty and HasOwnProperty are overriding real descriptor semantics by always using FName/forcing an own name descriptor; update GetProperty (check for an existing own property descriptor for PROP_NAME first, returning that descriptor's value/attributes if present, and only fall back to FName when no own descriptor exists), change SetProperty so it follows ordinary write semantics (do not unconditionally call DefineProperty to redefine name; instead perform normal assignment/update which respects non-writable/configurable flags of any existing descriptor), and make HasOwnProperty return whether an actual own descriptor for PROP_NAME exists (don’t hard-code True). Keep references to FName and PROP_NAME and ensure delete/define/assign paths now operate against real descriptors rather than always binding FName.
♻️ Duplicate comments (1)
source/units/Goccia.Runtime.Bootstrap.pas (1)
465-467:⚠️ Potential issue | 🟠 MajorRestore
%TypedArray%.prototype.constructorafter swapping in the shared prototype.After Line 467,
%TypedArray%.prototypepoints at the shared typed-array prototype, but this block never backfills that object's ownconstructorproperty. That leaves the shared%TypedArray%prototype incomplete even though theFunction.prototypeblock below does this wiring explicitly.🛠️ Minimal fix
// Wire %TypedArray%.prototype to the shared prototype (which has all methods) if Assigned(TGocciaTypedArrayValue.GetSharedPrototypeObject) then - FTypedArrayIntrinsic.ReplacePrototype(TGocciaTypedArrayValue.GetSharedPrototypeObject); + begin + FTypedArrayIntrinsic.ReplacePrototype(TGocciaTypedArrayValue.GetSharedPrototypeObject); + FTypedArrayIntrinsic.Prototype.DefineProperty(PROP_CONSTRUCTOR, + TGocciaPropertyDescriptorData.Create(FTypedArrayIntrinsic, [pfConfigurable, pfWritable])); + end;Based on learnings:
TGocciaClassValue.prototypeis backed byFClassPrototype, so this should be fixed by definingconstructoronFTypedArrayIntrinsic.Prototype, not by materializing a separate.prototypedescriptor.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/units/Goccia.Runtime.Bootstrap.pas` around lines 465 - 467, After swapping in the shared typed-array prototype via TGocciaTypedArrayValue.GetSharedPrototypeObject and FTypedArrayIntrinsic.ReplacePrototype, restore the constructor reference on the shared object by defining the "constructor" property on FTypedArrayIntrinsic.Prototype to point to the typed-array class function (the same value used by TGocciaClassValue.prototype / FClassPrototype wiring), rather than creating a separate .prototype descriptor; in short, after calling FTypedArrayIntrinsic.ReplacePrototype(...), set FTypedArrayIntrinsic.Prototype's "constructor" to the appropriate class function so the shared prototype has a correct constructor property.
🧹 Nitpick comments (2)
tests/built-ins/TypedArray/constructors.js (1)
306-319: Consider addingFloat16Arrayto the prototype identity test.The test at line 10 verifies
Float16Arrayis defined, but it's not included in the shared[[Prototype]]identity check. For completeness:test("all typed array constructors share the same [[Prototype]]", () => { const TA = Object.getPrototypeOf(Int8Array); expect(Object.getPrototypeOf(Uint8Array)).toBe(TA); expect(Object.getPrototypeOf(Int16Array)).toBe(TA); expect(Object.getPrototypeOf(Uint16Array)).toBe(TA); expect(Object.getPrototypeOf(Int32Array)).toBe(TA); expect(Object.getPrototypeOf(Uint32Array)).toBe(TA); + expect(Object.getPrototypeOf(Float16Array)).toBe(TA); expect(Object.getPrototypeOf(Float32Array)).toBe(TA); expect(Object.getPrototypeOf(Float64Array)).toBe(TA);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/built-ins/TypedArray/constructors.js` around lines 306 - 319, The prototype-identity test "all typed array constructors share the same [[Prototype]]" omits Float16Array; add an assertion to verify Float16Array shares the same [[Prototype]] as the others by calling Object.getPrototypeOf(Float16Array) and expecting it toBe(TA) (TA is the variable assigned from Object.getPrototypeOf(Int8Array)); insert this new expect alongside the other expect(...) lines in the same test block.tests/built-ins/Error/error-constructor.js (1)
107-115: Consider addingAggregateErrorto the NativeError descriptor test.The test covers
TypeError,RangeError,ReferenceError,SyntaxError, andURIError, but omitsAggregateError(andSuppressedErrorif supported). Since these also inherit fromErrorand have similar prototype descriptors, consider adding them for completeness.test("NativeError.prototype.name is non-enumerable", () => { - const types = [TypeError, RangeError, ReferenceError, SyntaxError, URIError]; + const types = [TypeError, RangeError, ReferenceError, SyntaxError, URIError, AggregateError]; types.forEach((ErrorType) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/built-ins/Error/error-constructor.js` around lines 107 - 115, The test "NativeError.prototype.name is non-enumerable" currently checks descriptor properties for TypeError, RangeError, ReferenceError, SyntaxError, and URIError; update the types array in that test to also include AggregateError (and optionally SuppressedError if the environment supports it) so the descriptor assertions (obtained via Object.getOwnPropertyDescriptor(ErrorType.prototype, "name")) run for those native error classes as well. Ensure you only add the class names to the existing types array used in the forEach and keep the same assertions (enumerable false, writable true, configurable true).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@source/units/Goccia.ObjectModel.Engine.pas`:
- Around line 67-69: The copy loop currently reuses the original Descriptor
instance when copying static string properties (the branch that calls
TGocciaObjectValue(AConstructor).DefineProperty(Key, Descriptor)), which can
couple state between ASource and the new object; instead, create/clone a new
TGocciaPropertyDescriptorData instance populated from Descriptor before calling
TGocciaObjectValue(AConstructor).DefineProperty so the target gets an
independent descriptor (mirror how the symbol branch clones descriptors),
ensuring ownership/lifetime safety for TGocciaPropertyDescriptorData.
---
Outside diff comments:
In `@source/units/Goccia.Values.ClassValue.pas`:
- Around line 513-520: AddStaticGetter and AddStaticSetter currently only add
entries to FStaticGetters/FStaticSetters causing static accessors to be
invisible to own-property reflection; mirror the pattern used by
AddGetter/AddSetter by also calling DefineProperty on Self to create an accessor
descriptor for the property name (using the same AGetter/ASetter and appropriate
attributes) so Object.getOwnPropertyDescriptor(Self, AName), hasOwnProperty and
delete/redefine behavior are correct; update TGocciaClassValue.AddStaticGetter
and AddStaticSetter to both populate the side maps
(FStaticGetters/FStaticSetters) and call DefineProperty(Self, AName, Descriptor)
with the accessor descriptor constructed from AGetter/ASetter.
- Around line 955-961: GetProperty, SetProperty and HasOwnProperty are
overriding real descriptor semantics by always using FName/forcing an own name
descriptor; update GetProperty (check for an existing own property descriptor
for PROP_NAME first, returning that descriptor's value/attributes if present,
and only fall back to FName when no own descriptor exists), change SetProperty
so it follows ordinary write semantics (do not unconditionally call
DefineProperty to redefine name; instead perform normal assignment/update which
respects non-writable/configurable flags of any existing descriptor), and make
HasOwnProperty return whether an actual own descriptor for PROP_NAME exists
(don’t hard-code True). Keep references to FName and PROP_NAME and ensure
delete/define/assign paths now operate against real descriptors rather than
always binding FName.
---
Duplicate comments:
In `@source/units/Goccia.Runtime.Bootstrap.pas`:
- Around line 465-467: After swapping in the shared typed-array prototype via
TGocciaTypedArrayValue.GetSharedPrototypeObject and
FTypedArrayIntrinsic.ReplacePrototype, restore the constructor reference on the
shared object by defining the "constructor" property on
FTypedArrayIntrinsic.Prototype to point to the typed-array class function (the
same value used by TGocciaClassValue.prototype / FClassPrototype wiring), rather
than creating a separate .prototype descriptor; in short, after calling
FTypedArrayIntrinsic.ReplacePrototype(...), set FTypedArrayIntrinsic.Prototype's
"constructor" to the appropriate class function so the shared prototype has a
correct constructor property.
---
Nitpick comments:
In `@tests/built-ins/Error/error-constructor.js`:
- Around line 107-115: The test "NativeError.prototype.name is non-enumerable"
currently checks descriptor properties for TypeError, RangeError,
ReferenceError, SyntaxError, and URIError; update the types array in that test
to also include AggregateError (and optionally SuppressedError if the
environment supports it) so the descriptor assertions (obtained via
Object.getOwnPropertyDescriptor(ErrorType.prototype, "name")) run for those
native error classes as well. Ensure you only add the class names to the
existing types array used in the forEach and keep the same assertions
(enumerable false, writable true, configurable true).
In `@tests/built-ins/TypedArray/constructors.js`:
- Around line 306-319: The prototype-identity test "all typed array constructors
share the same [[Prototype]]" omits Float16Array; add an assertion to verify
Float16Array shares the same [[Prototype]] as the others by calling
Object.getPrototypeOf(Float16Array) and expecting it toBe(TA) (TA is the
variable assigned from Object.getPrototypeOf(Int8Array)); insert this new expect
alongside the other expect(...) lines in the same test block.
🪄 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: a7c05717-304c-493d-ae66-ab00865ff544
📒 Files selected for processing (33)
scripts/run_test262_suite.pyscripts/test262_syntax_filter.pysource/units/Goccia.Builtins.GlobalObject.passource/units/Goccia.Builtins.GlobalRegExp.passource/units/Goccia.Builtins.Globals.passource/units/Goccia.Engine.passource/units/Goccia.Evaluator.passource/units/Goccia.ObjectModel.Engine.passource/units/Goccia.Runtime.Bootstrap.passource/units/Goccia.SharedPrototype.passource/units/Goccia.VM.passource/units/Goccia.Values.ArrayValue.passource/units/Goccia.Values.ClassValue.passource/units/Goccia.Values.FunctionBase.passource/units/Goccia.Values.TypedArrayValue.pastests/built-ins/Array/isArray.jstests/built-ins/Array/prototype/map.jstests/built-ins/Error/error-constructor.jstests/built-ins/Function/prototype/apply.jstests/built-ins/Function/prototype/bind.jstests/built-ins/Function/prototype/call.jstests/built-ins/Function/prototype/constructor.jstests/built-ins/Map/constructor.jstests/built-ins/Map/prototype/get.jstests/built-ins/Object/create.jstests/built-ins/Object/getPrototypeOf.jstests/built-ins/Object/keys.jstests/built-ins/Proxy/apply.jstests/built-ins/Set/constructor.jstests/built-ins/Set/prototype/has.jstests/built-ins/TypedArray/constructors.jstests/language/classes/basic-class-declaration.jstests/language/classes/method-enumerable.js
✅ Files skipped from review due to trivial changes (11)
- tests/built-ins/Map/prototype/get.js
- tests/built-ins/Set/constructor.js
- tests/built-ins/Set/prototype/has.js
- tests/built-ins/Array/isArray.js
- tests/built-ins/Object/keys.js
- tests/language/classes/method-enumerable.js
- tests/built-ins/Function/prototype/constructor.js
- tests/built-ins/Object/getPrototypeOf.js
- tests/built-ins/Function/prototype/call.js
- tests/built-ins/Function/prototype/bind.js
- source/units/Goccia.Values.TypedArrayValue.pas
🚧 Files skipped from review as they are similar to previous changes (7)
- tests/built-ins/Array/prototype/map.js
- tests/built-ins/Map/constructor.js
- tests/built-ins/Proxy/apply.js
- source/units/Goccia.Values.ArrayValue.pas
- tests/language/classes/basic-class-declaration.js
- tests/built-ins/Object/create.js
- scripts/run_test262_suite.py
CopyStaticMembersToConstructor was sharing TGocciaPropertyDescriptorData instances between the source builtin object and the target class constructor. When the GC swept the source during collection, its Destroy freed the shared descriptors, leaving dangling pointers in the constructor's FProperties. The next GC mark cycle hit use-after-free via MarkValues on the freed descriptors, causing EInvalidCast. Fix: clone descriptors with TGocciaPropertyDescriptorData.Create using the original value and flags, so each object owns its own descriptor instance. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- ObjectDefineProperties: route symbol-keyed descriptors through ObjectDefineProperty for proper ToPropertyDescriptor handling - Set %TypedArray%.prototype.constructor to %TypedArray% intrinsic per ES §23.2.3 (was missing, defaulted to Object) - Add test: Object.create with symbol-keyed property descriptor - Add test: instance auto-accessor on prototype not constructor - Add test: runtime class property assignment is enumerable Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
frostney
commented
Apr 23, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@source/units/Goccia.Builtins.GlobalObject.pas`:
- Around line 627-648: The loop currently skips symbol-keyed entries when the
descriptor in PropertiesDescriptor is an accessor (the code only handles
TGocciaPropertyDescriptorData), but per spec we must invoke the property's
getter (Get(props, key)) to obtain the descriptor object; modify the loop that
iterates SymbolKeys so that for non-data symbol descriptors you call
PropertiesDescriptor.Get(SymbolKeys[I]) (or the equivalent getter API) to
retrieve the descriptor value, then pass that value as the third argument to
ObjectDefineProperty (same as TGocciaPropertyDescriptorData.Value is passed
now), preserving the Enumerable check and exception behavior; reference Symbols:
PropertiesDescriptor.GetOwnSymbols,
PropertiesDescriptor.GetOwnSymbolPropertyDescriptor,
TGocciaPropertyDescriptorData, ObjectDefineProperty, and CallArgs. Ensure you
free CallArgs and handle nil/non-object returns consistently
(ObjectDefineProperty will perform ToPropertyDescriptor), and catch/propagate
any exceptions from the getter as before.
🪄 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: d280870e-4061-467f-bc06-ede4e1a0d063
📒 Files selected for processing (6)
source/units/Goccia.Builtins.GlobalObject.passource/units/Goccia.Engine.passource/units/Goccia.ObjectModel.Engine.passource/units/Goccia.Runtime.Bootstrap.pastests/built-ins/Object/create.jstests/language/classes/basic-class-declaration.js
✅ Files skipped from review due to trivial changes (1)
- tests/built-ins/Object/create.js
🚧 Files skipped from review as they are similar to previous changes (1)
- source/units/Goccia.Engine.pas
Use GetSymbolProperty (Get semantics) instead of reading TGocciaPropertyDescriptorData.Value directly, so accessor descriptors on the properties object invoke their getter to obtain the descriptor value before passing to ObjectDefineProperty. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
--compat-var, BigInt typed array helpers, and additional syntax filtering for unsupported feature paths.Function.prototype, and class method/static property descriptors match expected enumerability and inheritance behavior more closely.%TypedArray%intrinsic wiring and connected typed array constructors to the shared typed array prototype.Object.create,Object.getPrototypeOf, array helpers, maps/sets, typed arrays, and error constructors.