Consolidate engine semantics and test262 roadmap#494
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughConsolidates arithmetic/bitwise/relational semantics into a new Goccia.Arithmetic unit and wires it across VM, evaluator, values, and compiler; centralizes destructuring binding-name collection; adds a Test262 compatibility roadmap, runner clustering, and CI comment rendering; integrates profiler support into the benchmark runner; plus docs, harness, BigInt parsing, and tests. ChangesArithmetic & Bitwise Consolidation
Test262 Compatibility Roadmap & Runner
Binding Patterns & Compiler Refactor
Benchmark Profiling Integration
Miscellaneous docs, harness, BigInt & tests
Sequence DiagramsequenceDiagram
participant Parser as Parser/Compiler
participant BindingUtil as BindingPatterns (unit)
participant Evaluator as Evaluator
participant VM as VM (bytecode)
participant Arithmetic as Goccia.Arithmetic
participant Runner as test262 runner
Parser->>BindingUtil: CollectPatternBindingNames / CollectVarBindingNames...
BindingUtil-->>Parser: binding name list
Evaluator->>Arithmetic: CompoundOperations(current, new, operator)
Arithmetic-->>Evaluator: TGocciaValue result
VM->>Arithmetic: EvaluateAddition/Division/Bitwise/Relational(...)
Arithmetic-->>VM: TGocciaValue (handles NaN/Infinity/-0)
Evaluator->>Arithmetic: IsStrictEqual / LessThan / GreaterThan
Arithmetic-->>Evaluator: Boolean (call sites wrap via FromBoolean)
Runner->>Runner: build clusters (path_bucket, message_fingerprint)
Runner-->>Runner: summary["clusters"]
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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. Review rate limit: 2/5 reviews remaining, refill in 26 minutes and 17 seconds. Comment |
coderabbitai
Bot
added
new feature
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
source/units/Goccia.Evaluator.pas (2)
1398-1407:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the block-scoped assignment path for
vardestructuring in loop heads.
AssignVariablePattern(...)bypasses the newAssignBinding/FVarBindingspath and evaluates computed keys/defaults againstAContext, so nested patterns infor/for awaitcan see the wrong scope. Switch these branches toAssignPattern(..., False)instead.Proposed fix
- if AForOfStatement.BindingPattern <> nil then - AssignVariablePattern(AForOfStatement.BindingPattern, CurrentValue, AContext) + if AForOfStatement.BindingPattern <> nil then + AssignPattern(AForOfStatement.BindingPattern, CurrentValue, IterContext, False, DeclarationType)Apply the same change in
EvaluateForAwaitOf.Also applies to: 1618-1626
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/units/Goccia.Evaluator.pas` around lines 1398 - 1407, The var-destructuring branch in the for/of loop uses AssignVariablePattern(AForOfStatement.BindingPattern, ...) which bypasses the block-scoped AssignBinding/FVarBindings path and evaluates computed keys/defaults against AContext; replace that call with AssignPattern(AForOfStatement.BindingPattern, CurrentValue, IterContext, False, DeclarationType) so the destructuring uses the block-scoped assignment path (do the same replacement in EvaluateForAwaitOf and the other similar branch around the 1618-1626 region).
4467-4499:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTemp-root
RestElementsbefore recursing into nested destructuring.The array/string fast paths create
RestElementsand immediately recurse intoAssignVariablePattern(...)without rooting it, so a default expression or computed binding can let GC reclaim the array mid-destructuring. The iterable fallback already does the right thing; these branches need the same protection.Proposed fix
- RestElements := TGocciaArrayValue.Create; - for J := I to ArrayValue.Elements.Count - 1 do - RestElements.Elements.Add(ArrayValue.Elements[J]); - AssignVariablePattern( - TGocciaRestDestructuringPattern(ArrPat.Elements[I]).Argument, - RestElements, AContext); + RestElements := TGocciaArrayValue.Create; + TGarbageCollector.Instance.AddTempRoot(RestElements); + try + for J := I to ArrayValue.Elements.Count - 1 do + RestElements.Elements.Add(ArrayValue.Elements[J]); + AssignVariablePattern( + TGocciaRestDestructuringPattern(ArrPat.Elements[I]).Argument, + RestElements, AContext); + finally + TGarbageCollector.Instance.RemoveTempRoot(RestElements); + end;Mirror the same rooting in the string branch as well.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/units/Goccia.Evaluator.pas` around lines 4467 - 4499, The RestElements local is allocated in the array and string fast-paths and then immediately passed into AssignVariablePattern, which can recurse and let the GC collect the temporary; root the RestElements just like the iterable fallback does before recursing. Concretely, in the branches that create RestElements (the block that uses TGocciaArrayValue.Create and the block that builds RestElements from TGocciaStringLiteralValue), ensure you root/push RestElements to the GC (or use the same rooting helper/mechanism used by the iterable fallback) for the lifetime of the AssignVariablePattern call, and then unroot/pop it afterward; reference TGocciaArrayValue, TGocciaStringLiteralValue, RestElements, TGocciaRestDestructuringPattern and AssignVariablePattern when applying the fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/pr.yml:
- Around line 969-972: The fingerprint escaping only handles backticks, so any
'|' in clusters.failureFingerprints items will break the Markdown table; update
the sanitization for the variable fingerprint (where fingerprint is derived from
String(item.fingerprint).replaceAll('`', '\\`')) to also escape pipe characters
(replace '|' with '\\|') and any other Markdown-sensitive characters as needed
before composing the table row, keeping the sanitized variable name
(fingerprint) used in the body concatenation.
In `@scripts/test262_compatibility_roadmap.json`:
- Around line 68-71: The mapping for "ArrayBuffer" currently points to the wrong
target "arrays"; update the "ArrayBuffer" entry in
scripts/test262_compatibility_roadmap.json so its "target" value is
"arraybuffer-typedarray" (the target defined as owning ArrayBuffer) to ensure
correct cluster attribution; locate the "ArrayBuffer" JSON object and replace
"arrays" with "arraybuffer-typedarray".
In `@source/units/Goccia.Arithmetic.pas`:
- Around line 689-725: Call ToPrimitive on both operands at the start of
CompareRelationalValues and use those primitive results for all type dispatch
and final numeric conversion: create local variables (e.g., LPrim :=
ALeft.ToPrimitive; RPrim := ARight.ToPrimitive) and replace all occurrences of
ALeft/ARight in the relational type checks (the TGocciaUndefinedLiteralValue,
TGocciaNullLiteralValue, TGocciaStringLiteralValue, TGocciaBigIntValue and the
BigInt-vs-Number branches) with LPrim/RPrim, and finally call
CompareNumberValues(LPrim.ToNumberLiteral, RPrim.ToNumberLiteral) instead of
using the original ALeft/ARight so string-vs-string and BigInt comparisons are
handled correctly before numeric coercion.
- Around line 297-326: In EvaluateModulo, preserve negative zero for exact-zero
remainders: after computing the remainder (currently Result :=
NumberValue(LeftNum.Value - RightNum.Value * Trunc(LeftNum.Value /
RightNum.Value))), store the numeric remainder in a local Double (e.g., rem :=
LeftNum.Value - RightNum.Value * Trunc(LeftNum.Value / RightNum.Value)); if rem
= 0.0 then detect a negative dividend by checking LeftNum.Value < 0.0 or
(LeftNum.Value = 0.0 and (1.0 / LeftNum.Value) < 0.0) and return
NumberValue(-0.0) in that case, otherwise return NumberValue(0.0); otherwise
return NumberValue(rem). Reference: function EvaluateModulo, variables LeftNum,
RightNum, and the NumberValue constructor.
In `@source/units/Goccia.AST.BindingPatterns.pas`:
- Around line 70-79: The centralized collector (CollectVarBindingNamesFromNode)
currently handles TGocciaForOfStatement but omits TGocciaForAwaitOfStatement,
causing names from "for await (var ...)" to be skipped; update
CollectVarBindingNamesFromNode to treat TGocciaForAwaitOfStatement the same as
TGocciaForOfStatement (visit its Left/Variable/Declaration nodes and collect
identifiers into the same VarDecl/DestructDecl/ExportVarDecl paths), and make
the analogous addition in the other mirrored visitor block (the one around the
114-125 region) so both switch/case branches include TGocciaForAwaitOfStatement
handling. Ensure you reference TGocciaForAwaitOfStatement and reuse the existing
handling logic for TGocciaForOfStatement to avoid duplication.
---
Outside diff comments:
In `@source/units/Goccia.Evaluator.pas`:
- Around line 1398-1407: The var-destructuring branch in the for/of loop uses
AssignVariablePattern(AForOfStatement.BindingPattern, ...) which bypasses the
block-scoped AssignBinding/FVarBindings path and evaluates computed
keys/defaults against AContext; replace that call with
AssignPattern(AForOfStatement.BindingPattern, CurrentValue, IterContext, False,
DeclarationType) so the destructuring uses the block-scoped assignment path (do
the same replacement in EvaluateForAwaitOf and the other similar branch around
the 1618-1626 region).
- Around line 4467-4499: The RestElements local is allocated in the array and
string fast-paths and then immediately passed into AssignVariablePattern, which
can recurse and let the GC collect the temporary; root the RestElements just
like the iterable fallback does before recursing. Concretely, in the branches
that create RestElements (the block that uses TGocciaArrayValue.Create and the
block that builds RestElements from TGocciaStringLiteralValue), ensure you
root/push RestElements to the GC (or use the same rooting helper/mechanism used
by the iterable fallback) for the lifetime of the AssignVariablePattern call,
and then unroot/pop it afterward; reference TGocciaArrayValue,
TGocciaStringLiteralValue, RestElements, TGocciaRestDestructuringPattern and
AssignVariablePattern when applying the fix.
🪄 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: e2d548c1-da3d-47b3-bdce-9420b0de40fd
📒 Files selected for processing (33)
.github/workflows/pr.ymldocs/adding-built-in-types.mddocs/benchmarks.mddocs/core-patterns.mddocs/value-system.mdscripts/check-doc-symbols.tsscripts/run_test262_suite.pyscripts/test262_compatibility_roadmap.jsonscripts/test262_syntax_filter.pysource/app/GocciaBenchmarkRunner.dprsource/units/Goccia.AST.BindingPatterns.passource/units/Goccia.AST.Expressions.passource/units/Goccia.Arithmetic.passource/units/Goccia.Builtins.GlobalObject.passource/units/Goccia.Builtins.TestingLibrary.passource/units/Goccia.Compiler.Expressions.passource/units/Goccia.Compiler.Statements.passource/units/Goccia.Evaluator.Arithmetic.passource/units/Goccia.Evaluator.Assignment.passource/units/Goccia.Evaluator.Bitwise.passource/units/Goccia.Evaluator.Comparison.passource/units/Goccia.Evaluator.PatternMatching.passource/units/Goccia.Evaluator.passource/units/Goccia.PatternMatching.passource/units/Goccia.VM.passource/units/Goccia.Values.ArrayValue.passource/units/Goccia.Values.ClassHelper.passource/units/Goccia.Values.MapValue.passource/units/Goccia.Values.ObjectValue.passource/units/Goccia.Values.Primitives.passource/units/Goccia.Values.ProxyValue.passource/units/Goccia.Values.SetValue.pastests/built-ins/BigInt/boxed-bigint-operators.js
💤 Files with no reviewable changes (2)
- source/units/Goccia.Evaluator.Arithmetic.pas
- source/units/Goccia.Evaluator.Bitwise.pas
Contributor
Suite TimingTest Runner (interpreted: 8,434 passed; bytecode: 8,434 passed)
MemoryGC rows aggregate the main thread plus all worker thread-local GCs. Test runner worker shutdown frees thread-local heaps in bulk; that shutdown reclamation is not counted as GC collections or collected objects.
Benchmarks (interpreted: 407; bytecode: 407)
MemoryGC rows aggregate the main thread plus all worker thread-local GCs. Benchmark runner performs explicit between-file collections, so collection and collected-object counts can be much higher than the test runner.
Measured on ubuntu-latest x64. |
Contributor
Benchmark Results407 benchmarks Interpreted: 🟢 18 improved · 🔴 246 regressed · 143 unchanged · avg -2.8% arraybuffer.js — Interp: 🔴 10, 4 unch. · avg -3.2% · Bytecode: 🔴 11, 3 unch. · avg -9.2%
arrays.js — Interp: 🔴 16, 3 unch. · avg -4.3% · Bytecode: 🔴 17, 2 unch. · avg -12.2%
async-await.js — Interp: 🔴 4, 2 unch. · avg -4.1% · Bytecode: 🔴 6 · avg -16.2%
async-generators.js — Interp: 2 unch. · avg -3.3% · Bytecode: 🔴 1, 1 unch. · avg -8.4%
base64.js — Interp: 🔴 8, 2 unch. · avg -4.4% · Bytecode: 🔴 10 · avg -15.9%
classes.js — Interp: 🔴 21, 10 unch. · avg -3.5% · Bytecode: 🔴 21, 10 unch. · avg -7.7%
closures.js — Interp: 🔴 6, 5 unch. · avg -3.5% · Bytecode: 🔴 11 · avg -12.4%
collections.js — Interp: 🔴 7, 5 unch. · avg -3.7% · Bytecode: 🔴 12 · avg -14.0%
csv.js — Interp: 🟢 1, 🔴 5, 7 unch. · avg -2.1% · Bytecode: 🔴 13 · avg -10.1%
destructuring.js — Interp: 🔴 14, 8 unch. · avg -4.0% · Bytecode: 🔴 22 · avg -11.6%
fibonacci.js — Interp: 🔴 6, 2 unch. · avg -4.0% · Bytecode: 🔴 8 · avg -14.2%
float16array.js — Interp: 🟢 4, 🔴 19, 9 unch. · avg -2.2% · Bytecode: 🟢 4, 🔴 27, 1 unch. · avg -7.9%
for-of.js — Interp: 🔴 6, 1 unch. · avg -3.6% · Bytecode: 🔴 7 · avg -14.0%
generators.js — Interp: 🔴 1, 3 unch. · avg -1.2% · Bytecode: 🔴 4 · avg -6.2%
iterators.js — Interp: 🔴 35, 7 unch. · avg -4.7% · Bytecode: 🔴 42 · avg -11.3%
json.js — Interp: 🟢 1, 🔴 5, 14 unch. · avg -1.2% · Bytecode: 🔴 19, 1 unch. · avg -11.1%
jsx.jsx — Interp: 🔴 14, 7 unch. · avg -3.1% · Bytecode: 🔴 9, 12 unch. · avg -3.5%
modules.js — Interp: 🔴 5, 4 unch. · avg -4.7% · Bytecode: 🔴 9 · avg -13.9%
numbers.js — Interp: 🔴 5, 6 unch. · avg -3.4% · Bytecode: 🔴 11 · avg -11.2%
objects.js — Interp: 🔴 6, 1 unch. · avg -6.1% · Bytecode: 🔴 7 · avg -9.5%
promises.js — Interp: 🔴 9, 3 unch. · avg -3.7% · Bytecode: 🔴 9, 3 unch. · avg -5.8%
regexp.js — Interp: 🔴 5, 6 unch. · avg -1.8% · Bytecode: 🔴 11 · avg -10.4%
strings.js — Interp: 🔴 10, 9 unch. · avg -3.7% · Bytecode: 🔴 19 · avg -10.3%
tsv.js — Interp: 9 unch. · avg -0.2% · Bytecode: 🔴 6, 3 unch. · avg -6.0%
typed-arrays.js — Interp: 🟢 9, 🔴 7, 6 unch. · avg +19.6% · Bytecode: 🔴 21, 1 unch. · avg -15.8%
uint8array-encoding.js — Interp: 🟢 2, 🔴 11, 5 unch. · avg -21.2% · Bytecode: 🟢 7, 🔴 5, 6 unch. · avg +11.3%
weak-collections.js — Interp: 🟢 1, 🔴 11, 3 unch. · avg +0.4% · Bytecode: 🔴 15 · avg -18.8%
Measured on ubuntu-latest x64. Benchmark ranges compare cached main-branch min/max ops/sec with the PR run; overlapping ranges are treated as unchanged noise. Percentage deltas are secondary context. |
Contributor
test262 Conformance
Areas closest to 100%
Compatibility roadmap skips
Non-blocking. Measured on ubuntu-latest x64, bytecode mode. Areas grouped by the first two test262 path components; minimum 25 attempted tests, areas already at 100% excluded. Δ vs main compares against the most recent cached |
coderabbitai
Bot
added
documentation
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/workflows/pr.yml (1)
959-961:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEscape
item.targetbefore composing Markdown table rows.On Line [960],
item.targetis inserted raw. If a target contains|, backticks, or newlines, the PR-comment table can render incorrectly.Proposed fix
+ const escapeMdCell = (value) => String(value) + .replaceAll('\\', '\\\\') + .replaceAll('|', '\\|') + .replaceAll('`', '\\`') + .replace(/\r?\n/g, ' '); ... for (const item of clusters.skipsByTarget.slice(0, 5)) { - body += `| \`${item.target}\` | ${fmt(item.count)} |\n`; + const target = escapeMdCell(item.target); + body += `| \`${target}\` | ${fmt(item.count)} |\n`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/pr.yml around lines 959 - 961, Sanitize item.target before composing the Markdown table row: create an escapedTarget (e.g., in the loop over clusters.skipsByTarget) by replacing backticks, pipe characters and newlines (for example .replace(/`/g, '\\`').replace(/\|/g, '\\|').replace(/\r?\n/g, ' ')) and then use the escapedTarget in the template (replace `${item.target}` with `${escapedTarget}`) so the table row generation in the loop over clusters.skipsByTarget remains well-formed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/workflows/pr.yml:
- Around line 959-961: Sanitize item.target before composing the Markdown table
row: create an escapedTarget (e.g., in the loop over clusters.skipsByTarget) by
replacing backticks, pipe characters and newlines (for example .replace(/`/g,
'\\`').replace(/\|/g, '\\|').replace(/\r?\n/g, ' ')) and then use the
escapedTarget in the template (replace `${item.target}` with `${escapedTarget}`)
so the table row generation in the loop over clusters.skipsByTarget remains
well-formed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 56596b13-8010-4971-b81f-e47f090a2c88
📒 Files selected for processing (4)
.github/workflows/pr.ymlscripts/run_test262_suite.pyscripts/test262_compatibility_roadmap.jsonscripts/test262_syntax_filter.py
✅ Files skipped from review due to trivial changes (1)
- scripts/test262_compatibility_roadmap.json
🚧 Files skipped from review as they are similar to previous changes (2)
- scripts/test262_syntax_filter.py
- scripts/run_test262_suite.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/test262_harness/wellKnownIntrinsicObjects.js (1)
70-76: ⚡ Quick winConvert these helpers to arrow functions.
resolveWellKnownIntrinsicandgetWellKnownIntrinsicObjectshould follow the repo’s JS style forscripts/test262_harness/and be written as arrow functions instead offunctiondeclarations. As per coding guidelines, in this repo’s GocciaScript-executed JavaScript underscripts/test262_harness/, use arrow functions only.♻️ Suggested rewrite
-function resolveWellKnownIntrinsic(source) { +const resolveWellKnownIntrinsic = (source) => { try { return Function("return " + source)(); } catch (exception) { return undefined; } -} +}; const WellKnownIntrinsicObjects = WellKnownIntrinsicSources.map(([name, source]) => ({ name, source, value: resolveWellKnownIntrinsic(source), })); -function getWellKnownIntrinsicObject(key) { +const getWellKnownIntrinsicObject = (key) => { for (const intrinsic of WellKnownIntrinsicObjects) { if (intrinsic.name === key) { if (intrinsic.value !== undefined) { return intrinsic.value; } throw new Test262Error("this implementation could not obtain " + key); } } throw new Test262Error("unknown well-known intrinsic " + key); -} +};Also applies to: 84-95
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/test262_harness/wellKnownIntrinsicObjects.js` around lines 70 - 76, Convert the two function declarations into arrow-function const bindings: replace the function declaration for resolveWellKnownIntrinsic with a const resolveWellKnownIntrinsic = (source) => { ... } preserving the try/catch and return behavior, and likewise replace getWellKnownIntrinsicObject with const getWellKnownIntrinsicObject = (source) => { ... } keeping its existing logic and return values; ensure you keep the original function names as const identifiers so callers still resolve correctly and that no hoisting-dependent code is affected.
🤖 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/test262_syntax_filter.py`:
- Around line 41-54: The code assumes COMPATIBILITY_ROADMAP["unknownFeature"]
exists which can raise KeyError; update _roadmap_entry and _entry_is_eligible to
use a safe fallback (e.g., COMPATIBILITY_ROADMAP.get("unknownFeature", {}) or a
module-level UNKNOWN_FEATURE = {}) so lookups never index a missing key, and
ensure _entry_is_eligible treats that safe fallback consistently when entry is
None or falsy (use entry.get("status","") and entry.get("eligible") safely).
Reference: functions _roadmap_entry and _entry_is_eligible and the
COMPATIBILITY_ROADMAP lookup for "unknownFeature".
---
Nitpick comments:
In `@scripts/test262_harness/wellKnownIntrinsicObjects.js`:
- Around line 70-76: Convert the two function declarations into arrow-function
const bindings: replace the function declaration for resolveWellKnownIntrinsic
with a const resolveWellKnownIntrinsic = (source) => { ... } preserving the
try/catch and return behavior, and likewise replace getWellKnownIntrinsicObject
with const getWellKnownIntrinsicObject = (source) => { ... } keeping its
existing logic and return values; ensure you keep the original function names as
const identifiers so callers still resolve correctly and that no
hoisting-dependent code is affected.
🪄 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: 12fd88c2-e0aa-453a-83e9-da9de26bfd68
📒 Files selected for processing (6)
scripts/run_test262_suite.pyscripts/test262_compatibility_roadmap.jsonscripts/test262_harness/assertRelativeDateMs.jsscripts/test262_harness/wellKnownIntrinsicObjects.jsscripts/test262_harness_map.pyscripts/test262_syntax_filter.py
✅ Files skipped from review due to trivial changes (1)
- scripts/test262_harness_map.py
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/test262_compatibility_roadmap.json
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/test262_syntax_filter.py`:
- Around line 400-401: The current logic short-circuits syntax/negative
classification by extending reasons only when reasons is empty, causing
missing_harness:* to block later syntax checks; update the flow so
check_includes(includes) is always run after syntax/negative classification and
its results merged into reasons regardless of whether reasons is already
populated — modify the block around the reasons variable (the conditional using
"if not reasons") to always call check_includes(includes) and extend or merge
its returned list into reasons so harness-include failures don't preempt
language-semantic classification.
In `@source/units/Goccia.Compiler.ConstantFolding.pas`:
- Around line 42-50: The fast integer path in FoldedModuloValue (function
FoldedModuloValue) uses Trunc(...) mod Trunc(...), which narrows to Int64 and
can overflow; remove that conditional fast path so the function always computes
the remainder using the floating-point routine FMod(ALeft, ARight) (i.e., call
FMod unconditionally) and preserve the subsequent negative-zero adjustment (the
if that sets Result := NegativeZeroFloat when Result = 0.0 and ALeft is negative
or negative-zero). This makes FoldedModuloValue consistent with FMod and the VM
implementation and avoids Int64 narrowing/overflow.
In `@source/units/Goccia.Values.BigIntValue.pas`:
- Line 73: StringToBigInt uses Trim(AValue) which only strips ASCII whitespace
and misses ECMAScript whitespace like U+00A0 and U+FEFF; replace the Trim call
in StringToBigInt so the input is normalized with an ECMA-262 compliant
whitespace trimmer (e.g., call or add a utility like
StrTrimEcma/TrimEcmaWhitespace that removes all StrWhiteSpace code points
including U+00A0 and U+FEFF) instead of Trim(AValue), or augment the trimming
logic to explicitly strip #$00A0 and #$FEFF before parsing; update the
assignment to Text (and any other callers) to use that ECMA-aware trimmer so
BigInt(string) parsing matches the spec.
🪄 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: 50497a0f-2b87-40ee-91ec-a45282f1a2e0
📒 Files selected for processing (9)
scripts/test262_syntax_filter.pysource/units/Goccia.Arithmetic.passource/units/Goccia.Builtins.GlobalBigInt.passource/units/Goccia.Compiler.ConstantFolding.passource/units/Goccia.VM.passource/units/Goccia.Values.BigIntValue.pastests/language/expressions/bigint/bigint-comparison.jstests/language/expressions/modulo/modulo.jstests/language/expressions/type-coercion/comparison-coercion.js
🚧 Files skipped from review as they are similar to previous changes (1)
- source/units/Goccia.Arithmetic.pas
…-analysis # Conflicts: # source/app/GocciaBenchmarkRunner.dpr
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 `@scripts/test262_syntax_filter.py`:
- Around line 355-358: The current logic checks AVAILABLE_INCLUDES then consults
_roadmap_entry/_entry_is_eligible which can mark an unmapped include eligible;
change the branch so any include not in AVAILABLE_INCLUDES is unconditionally
treated as missing_harness by immediately appending
reasons.append(f"missing_harness:{inc}") (and skipping the roadmap eligibility
check), while preserving the roadmap lookup only inside classify_skip_reason()
for reporting; update the block around the check involving AVAILABLE_INCLUDES,
_roadmap_entry and _entry_is_eligible accordingly.
🪄 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: df550950-7d61-44d6-ba55-1defa756bd3c
📒 Files selected for processing (11)
docs/adding-built-in-types.mddocs/benchmarks.mddocs/core-patterns.mddocs/value-system.mdscripts/test262_syntax_filter.pysource/app/GocciaBenchmarkRunner.dprsource/units/Goccia.Compiler.ConstantFolding.passource/units/Goccia.VM.passource/units/Goccia.Values.BigIntValue.pastests/built-ins/BigInt/constructor.jstests/language/expressions/bigint/bigint-comparison.js
✅ Files skipped from review due to trivial changes (2)
- docs/core-patterns.md
- source/units/Goccia.VM.pas
🚧 Files skipped from review as they are similar to previous changes (6)
- docs/value-system.md
- docs/adding-built-in-types.md
- docs/benchmarks.md
- tests/built-ins/BigInt/constructor.js
- tests/language/expressions/bigint/bigint-comparison.js
- source/app/GocciaBenchmarkRunner.dpr
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 `@scripts/test262_syntax_filter.py`:
- Around line 39-42: The constant UNKNOWN_ROADMAP_ENTRY currently hardcodes
"target": "unassigned" and causes _roadmap_entry() to ignore the roadmap's
unknownFeature policy; update the logic so that when creating fallback entries
(the UNKNOWN_ROADMAP_ENTRY usage) you read the roadmap's unknownFeature (or
equivalent field available to _roadmap_entry) and set the entry's "target" to
that value instead of the hardcoded "unassigned" (also apply the same change to
the other fallback at the second occurrence noted). Ensure you still set
"status": "unsupported" but derive "target" from the roadmap input before
returning the fallback entry.
🪄 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: 8d5cfc4f-982d-4f0d-a6c2-d9e352c38cf6
📒 Files selected for processing (1)
scripts/test262_syntax_filter.py
This was referenced May 2, 2026
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
Goccia.Arithmeticfor evaluator and VM fallback paths.Verification
./build.pas clean testrunner loader./build.pas --prod testrunner loader./build/GocciaTestRunner tests --asi --unsafe-ffi --no-progress./build/GocciaTestRunner tests --asi --unsafe-ffi --mode=bytecode --no-progress./format.pas --checkgit diff --checkNote: local pre-commit doc hooks scanned the untracked
tmp/test262-suitecheckout and failed on upstream test262 markdown files.tmp/is not part of this commit.