Merged
Conversation
- align BigInt, Reflect.construct, and global property semantics with Test262 expectations - add non-constructable native function support and BigInt conversion fixes - expand harness helpers for constructor and subclassing cases
Contributor
📝 WalkthroughWalkthroughThis PR enhances ES2026 spec compliance by introducing a Changes
Sequence DiagramsequenceDiagram
participant Caller
participant ReflectConstruct
participant IsConstructorCheck
participant ConstructLogic
participant TargetType
participant NewTargetType
participant Instance
participant Prototype
Caller->>ReflectConstruct: Reflect.construct(target, args, newTarget?)
ReflectConstruct->>IsConstructorCheck: Validate target is constructor
alt target not constructor
ReflectConstruct-->>Caller: TypeError
end
alt newTarget provided
ReflectConstruct->>IsConstructorCheck: Validate newTarget is constructor
alt newTarget not constructor
ReflectConstruct-->>Caller: TypeError
end
end
ReflectConstruct->>TargetType: Determine target type
alt target is class
TargetType->>ConstructLogic: ClassPath
alt newTarget is class (different from target)
ConstructLogic->>Instance: Instantiate with newTarget class
else newTarget is native function
ConstructLogic->>Instance: Instantiate with target class
ConstructLogic->>NewTargetType: Get newTarget.[[prototype]]
alt prototype is object
ConstructLogic->>Prototype: Set instance prototype from newTarget
end
else no newTarget
ConstructLogic->>Instance: Instantiate with target class
end
else target is native function
TargetType->>ConstructLogic: NativeFunctionPath
ConstructLogic->>Instance: Invoke target as constructor via Call
end
ConstructLogic->>Caller: Return instance
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
coderabbitai
Bot
added
bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
source/units/Goccia.Builtins.GlobalBigInt.pas (1)
230-290: Well-implementedToBigIntValuefor ES2026 §7.1.13 ToBigInt.The implementation correctly:
- Applies
ToPrimitive(argument, number)for objects- Returns BigInt primitives directly
- Converts Boolean and String primitives appropriately
- Throws TypeError for Number/undefined/null/symbol (as ToBigInt differs from
BigInt()function)The forward declaration pattern (line 231) works but is unnecessary in Pascal when the function appears before its only caller. Consider removing the forward declaration for clarity.
♻️ Optional: Remove redundant forward declaration
-// ES2026 §7.1.13 ToBigInt(argument) — forward declaration -function ToBigIntValue(const AValue: TGocciaValue): TGocciaBigIntValue; forward; - // ES2026 §7.1.13 ToBigInt — apply ToPrimitive then convert primitive to BigInt function ToBigIntValue(const AValue: TGocciaValue): TGocciaBigIntValue;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/units/Goccia.Builtins.GlobalBigInt.pas` around lines 230 - 290, The file contains an unnecessary forward declaration for function ToBigIntValue (function ToBigIntValue(const AValue: TGocciaValue): TGocciaBigIntValue; forward;) even though the actual implementation appears before any other callers; remove that forward declaration line and any matching forward-only prototype so the single implementation of ToBigIntValue remains, ensuring no other references rely on the forward prototype.
🤖 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_harness/temporalHelpers.js`:
- Around line 399-407: The helper checkSubclassSpeciesThrows is missing a call
to resultAssertions(result), so after creating the instance, forcing
constructor[Symbol.species] to throw, invoking the method, and asserting the
result prototype with assert.sameValue(Object.getPrototypeOf(result),
construct.prototype), add a call to resultAssertions(result) to run the same
result validations other checkSubclass* helpers perform; update the function
checkSubclassSpeciesThrows (parameters construct, constructArgs, method,
methodArgs, resultAssertions) to invoke resultAssertions(result) immediately
after the prototype assertion.
---
Nitpick comments:
In `@source/units/Goccia.Builtins.GlobalBigInt.pas`:
- Around line 230-290: The file contains an unnecessary forward declaration for
function ToBigIntValue (function ToBigIntValue(const AValue: TGocciaValue):
TGocciaBigIntValue; forward;) even though the actual implementation appears
before any other callers; remove that forward declaration line and any matching
forward-only prototype so the single implementation of ToBigIntValue remains,
ensuring no other references rely on the forward 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: 95e22d8d-92c4-4439-8178-6d694938de99
📒 Files selected for processing (15)
scripts/test262_harness/isConstructor.jsscripts/test262_harness/propertyHelper.jsscripts/test262_harness/temporalHelpers.jsscripts/test262_harness/testTypedArray.jssource/shared/BigInteger.passource/units/Goccia.Builtins.GlobalBigInt.passource/units/Goccia.Builtins.GlobalReflect.passource/units/Goccia.Engine.passource/units/Goccia.Evaluator.passource/units/Goccia.ObjectModel.Types.passource/units/Goccia.ObjectModel.passource/units/Goccia.Runtime.Bootstrap.passource/units/Goccia.VM.passource/units/Goccia.Values.BigIntValue.passource/units/Goccia.Values.NativeFunction.pas
Contributor
Benchmark Results386 benchmarks Interpreted: 🟢 35 improved · 🔴 102 regressed · 249 unchanged · avg +0.1% arraybuffer.js — Interp: 🔴 2, 12 unch. · avg +0.1% · Bytecode: 🔴 9, 5 unch. · avg -5.2%
arrays.js — Interp: 🟢 1, 🔴 5, 13 unch. · avg -1.2% · Bytecode: 🔴 19 · avg -8.9%
async-await.js — Interp: 🔴 3, 3 unch. · avg +0.6% · Bytecode: 🔴 5, 1 unch. · avg -9.8%
base64.js — Interp: 10 unch. · avg -0.3% · Bytecode: 🔴 10 · avg -7.9%
classes.js — Interp: 🟢 1, 🔴 10, 20 unch. · avg -1.2% · Bytecode: 🔴 13, 18 unch. · avg -4.7%
closures.js — Interp: 🔴 2, 9 unch. · avg -0.4% · Bytecode: 🔴 11 · avg -8.6%
collections.js — Interp: 🟢 2, 🔴 1, 9 unch. · avg +0.1% · Bytecode: 🔴 12 · avg -11.4%
csv.js — Interp: 🔴 3, 10 unch. · avg -0.7% · Bytecode: 🔴 13 · avg -8.7%
destructuring.js — Interp: 🔴 7, 15 unch. · avg -1.3% · Bytecode: 🔴 22 · avg -7.7%
fibonacci.js — Interp: 🟢 1, 7 unch. · avg -0.7% · Bytecode: 🔴 8 · avg -7.0%
float16array.js — Interp: 🟢 3, 🔴 5, 24 unch. · avg +0.0% · Bytecode: 🟢 4, 🔴 26, 2 unch. · avg -4.9%
for-of.js — Interp: 🔴 2, 5 unch. · avg -1.2% · Bytecode: 🔴 7 · avg -10.0%
helpers/bench-module.js — Interp: 0 · Bytecode: 0
iterators.js — Interp: 🟢 7, 🔴 8, 27 unch. · avg -0.1% · Bytecode: 🔴 40, 2 unch. · avg -8.5%
json.js — Interp: 🔴 7, 13 unch. · avg -2.2% · Bytecode: 🔴 19, 1 unch. · avg -10.2%
jsx.jsx — Interp: 🟢 3, 🔴 5, 13 unch. · avg -0.9% · Bytecode: 🔴 18, 3 unch. · avg -7.2%
modules.js — Interp: 🔴 5, 4 unch. · avg -1.9% · Bytecode: 🔴 9 · avg -13.0%
numbers.js — Interp: 🔴 3, 8 unch. · avg -2.2% · Bytecode: 🟢 2, 🔴 9 · avg -3.1%
objects.js — Interp: 🔴 4, 3 unch. · avg -1.9% · Bytecode: 🔴 7 · avg -9.1%
promises.js — Interp: 🔴 10, 2 unch. · avg -4.8% · Bytecode: 🔴 12 · avg -7.7%
regexp.js — Interp: 🟢 1, 🔴 2, 8 unch. · avg -1.6% · Bytecode: 🔴 8, 3 unch. · avg -4.7%
strings.js — Interp: 🟢 6, 🔴 1, 12 unch. · avg +0.8% · Bytecode: 🟢 3, 🔴 13, 3 unch. · avg +4.5%
tsv.js — Interp: 🟢 1, 🔴 2, 6 unch. · avg -0.5% · Bytecode: 🔴 8, 1 unch. · avg -7.8%
typed-arrays.js — Interp: 🟢 2, 🔴 10, 10 unch. · avg -0.7% · Bytecode: 🟢 9, 🔴 11, 2 unch. · avg -1.9%
uint8array-encoding.js — Interp: 🟢 7, 🔴 5, 6 unch. · avg +19.3% · Bytecode: 🟢 1, 🔴 11, 6 unch. · avg -10.6%
Measured on ubuntu-latest x64. Benchmark ranges compare cached main-branch min/max ops/sec with the PR run; overlapping ranges are treated as unchanged noise. Percentage deltas are secondary context. |
Contributor
Suite Timing
Measured on ubuntu-latest x64. |
This was referenced Apr 21, 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
BigInt.prototypemetadata andBigInt.asIntN/asUintNcoercion behavior.new BigInt()and other non-constructable natives now throw consistentTypeErrors in both interpreter and VM paths.Reflect.constructhandling, including constructor detection andnewTargetprototype behavior for native and class constructors.