Merged
Conversation
- Add BigInteger arithmetic and conversions - Wire BigInt literals, builtins, and VM handling - Add parser, evaluator, and test coverage for BigInt
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)
📝 WalkthroughWalkthroughAdds ECMAScript BigInt: new limb-backed TBigInteger, BigInt primitive/object wrappers, lexer/parser/bytecode/compiler/VM/evaluator/operator wiring, global BigInt builtin with asIntN/asUintN, error/suggestion strings, tests, docs, and spec updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Source
participant Lexer
participant Parser
participant Compiler
participant Bytecode
Source->>Lexer: "42n"
Note right of Lexer: scan number, ensure no decimal/exponent, consume 'n', strip separators
Lexer->>Parser: Token(gttBigInt, "42")
Parser->>Parser: TBigInteger.FromDecimalString("42")
Parser->>Compiler: TGocciaLiteralExpression(BigInt)
Compiler->>Compiler: AddConstantBigInt("42")
Compiler->>Bytecode: Emit OP_LOAD_CONST (bckBigInt)
sequenceDiagram
participant Bytecode
participant VM
participant ConstPool
participant Evaluator
participant Stack
Bytecode->>VM: OP_LOAD_CONST (index)
VM->>ConstPool: Fetch constant (bckBigInt -> "42")
ConstPool->>VM: Return "42"
VM->>VM: TBigInteger.FromDecimalString("42")
VM->>Stack: Push TGocciaBigIntValue(42n)
Note right of Stack: Later OP_ADD (42n + 1n)
Bytecode->>VM: OP_ADD
VM->>Evaluator: Both operands BigInt?
Evaluator->>Evaluator: Call TBigInteger.Add()
Evaluator->>Stack: Push result BigInt
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: 🟢 41 improved · 🔴 95 regressed · 250 unchanged · avg -0.1% arraybuffer.js — Interp: 🟢 1, 🔴 2, 11 unch. · avg -0.2% · Bytecode: 🔴 7, 7 unch. · avg -1.9%
arrays.js — Interp: 🔴 12, 7 unch. · avg -3.5% · Bytecode: 🔴 7, 12 unch. · avg -1.9%
async-await.js — Interp: 🔴 1, 5 unch. · avg -1.1% · Bytecode: 🔴 1, 5 unch. · avg -0.3%
base64.js — Interp: 🟢 2, 8 unch. · avg +0.9% · Bytecode: 10 unch. · avg -0.2%
classes.js — Interp: 🟢 3, 🔴 6, 22 unch. · avg -0.5% · Bytecode: 🔴 3, 28 unch. · avg -0.3%
closures.js — Interp: 🟢 1, 🔴 2, 8 unch. · avg -1.5% · Bytecode: 🔴 10, 1 unch. · avg -4.0%
collections.js — Interp: 🟢 2, 10 unch. · avg +0.2% · Bytecode: 🔴 3, 9 unch. · avg -1.0%
csv.js — Interp: 🟢 2, 11 unch. · avg +1.6% · Bytecode: 🔴 2, 11 unch. · avg +0.0%
destructuring.js — Interp: 🔴 4, 18 unch. · avg -1.0% · Bytecode: 🔴 5, 17 unch. · avg -0.8%
fibonacci.js — Interp: 🔴 4, 4 unch. · avg -3.5% · Bytecode: 🔴 4, 4 unch. · avg -1.4%
float16array.js — Interp: 🟢 4, 🔴 9, 19 unch. · avg -0.6% · Bytecode: 🟢 4, 🔴 12, 16 unch. · avg -2.3%
for-of.js — Interp: 🔴 2, 5 unch. · avg -1.9% · Bytecode: 🔴 1, 6 unch. · avg -0.8%
helpers/bench-module.js — Interp: 0 · Bytecode: 0
iterators.js — Interp: 🟢 4, 🔴 14, 24 unch. · avg -1.3% · Bytecode: 🟢 2, 🔴 19, 21 unch. · avg -1.8%
json.js — Interp: 20 unch. · avg +0.1% · Bytecode: 🟢 3, 🔴 5, 12 unch. · avg -0.6%
jsx.jsx — Interp: 🟢 2, 🔴 4, 15 unch. · avg -1.4% · Bytecode: 🔴 10, 11 unch. · avg -2.8%
modules.js — Interp: 🔴 1, 8 unch. · avg -0.0% · Bytecode: 🔴 9 · avg -9.0%
numbers.js — Interp: 🟢 1, 🔴 2, 8 unch. · avg -0.6% · Bytecode: 🔴 10, 1 unch. · avg -5.9%
objects.js — Interp: 🔴 2, 5 unch. · avg -2.1% · Bytecode: 🟢 1, 6 unch. · avg +1.4%
promises.js — Interp: 🟢 1, 🔴 2, 9 unch. · avg -0.9% · Bytecode: 🟢 2, 🔴 1, 9 unch. · avg -0.0%
regexp.js — Interp: 🔴 3, 8 unch. · avg -0.5% · Bytecode: 🟢 1, 🔴 3, 7 unch. · avg -0.8%
strings.js — Interp: 🟢 12, 🔴 1, 6 unch. · avg +22.0% · Bytecode: 🟢 8, 🔴 9, 2 unch. · avg +21.9%
tsv.js — Interp: 🔴 5, 4 unch. · avg -2.4% · Bytecode: 🟢 2, 🔴 2, 5 unch. · avg +0.1%
typed-arrays.js — Interp: 🟢 3, 🔴 14, 5 unch. · avg -0.5% · Bytecode: 🟢 4, 🔴 9, 9 unch. · avg -0.8%
uint8array-encoding.js — Interp: 🟢 3, 🔴 5, 10 unch. · avg -6.9% · Bytecode: 🟢 6, 🔴 3, 9 unch. · avg +2.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. |
![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: 14
🧹 Nitpick comments (1)
source/units/Goccia.VM.pas (1)
467-468: VMAddValues correctly calls ToPrimitive; other arithmetic operations don't.VMAddValues (lines 754-772) properly calls
ToPrimitive(ALeft)andToPrimitive(ARight)before checking for BigInt mixed-type, so the addition example is correct per spec. However, VMSubtractValues, VMMultiplyValues, VMDivideValues, VMModuloValues, and VMPowerValues skip this step and check raw types directly via VMToNumericPair, creating an inconsistency. If object wrapping (ES6 Object(1n)) is later implemented, subtraction and the other operations will fail to coerce boxed BigInts correctly.🤖 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 467 - 468, VMSubtractValues, VMMultiplyValues, VMDivideValues, VMModuloValues and VMPowerValues currently operate on raw operands and skip ToPrimitive, causing incorrect handling of boxed BigInts; update each of these functions to call ToPrimitive(ALeft) and ToPrimitive(ARight) (as VMAddValues does) before performing the BigInt mixed-type check and before calling VMToNumericPair, and then use the same XOR check (if (ALeft is TGocciaBigIntValue) xor (ARight is TGocciaBigIntValue) then ThrowTypeError(...)) so boxed BigInt objects are coerced correctly prior to numeric conversion.
🤖 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.GlobalBigInt.pas`:
- Around line 157-167: Replace the raw Trunc(ToNumberLiteral.Value) coercion for
the bits parameter with the standard ToIndex logic: call the existing ToIndex
abstract-op implementation (from Goccia.Values.ArrayBufferValue.pas) on
AArgs.GetElement(0) to produce Bits so undefined/NaN → 0 and
Infinity/negative/too-large values throw RangeError; keep the existing
RequireAtLeast(AArgs, 2, 'BigInt.asIntN', ThrowError) and BigInt type-check
(BigIntArg is TGocciaBigIntValue) and then pass the resulting Bits to
TGocciaBigIntValue(...).Value.AsIntN(Bits); apply the same replacement in
BigInt.asUintN where Trunc(ToNumberLiteral.Value) is used.
- Around line 117-124: Replace the incorrect ThrowTypeError calls with
ThrowRangeError in the Number-to-BigInt conversion branch: where the code checks
TGocciaNumberLiteralValue(Arg).IsNaN or .IsInfinite and where it checks
Frac(NumVal) <> 0, call ThrowRangeError (passing the same formatted
SErrorBigIntNotInteger and SSuggestBigIntNoImplicitConversion arguments) instead
of ThrowTypeError so the BigInt(number) path follows the ECMAScript
NumberToBigInt semantics.
- Around line 129-140: The string-to-BigInt branch must accept non-decimal
integer literal prefixes and throw SyntaxError on parse failure: trim whitespace
from TGocciaStringLiteralValue(Arg).Value, detect prefix 0x/0X → use
TBigInteger.FromHexString, 0o/0O → FromOctalString, 0b/0B → FromBinaryString,
otherwise use TBigInteger.FromDecimalString; on any parsing exception replace
the current ThrowTypeError call with the SyntaxError variant (e.g.,
ThrowSyntaxError) and include the same formatted message/arguments, and only
create TGocciaBigIntValue on successful parse.
In `@source/units/Goccia.Engine.pas`:
- Line 704: TGocciaEngine currently allocates FBuiltinGlobalBigInt with
TGocciaGlobalBigInt.Create but never frees it; update TGocciaEngine.Destroy to
call FBuiltinGlobalBigInt.Free (or FreeAndNil) and ensure it runs
before/alongside other cleanup to avoid use-after-free—locate
FBuiltinGlobalBigInt allocation at the constructor and add the matching free in
the TGocciaEngine.Destroy method to properly release the TGocciaGlobalBigInt
instance.
In `@source/units/Goccia.Evaluator.Arithmetic.pas`:
- Around line 49-55: The arithmetic routines are missing a ToPrimitive(number)
step before BigInt/type checks; update ToNumericPair and the operator handlers
EvaluateSubtraction, EvaluateMultiplication, EvaluateDivision, EvaluateModulo
and EvaluateExponentiation to call ToPrimitive on both operands (with hint
number) first, then perform the BigInt mixed-type checks (CheckBigIntMixedTypes)
and only after that call ALeft.ToNumberLiteral / ARight.ToNumberLiteral so boxed
BigInt objects (e.g. Object(1n)) are unboxed via valueOf() and handled
consistently like EvaluateAddition already does.
- Around line 199-206: Replace the incorrect ThrowTypeError calls that handle
BigInt division-by-zero, modulo-by-zero, and negative-exponent cases with
ThrowRangeError and remove the SSuggestBigIntNoMixedArithmetic suggestion;
specifically locate the branches handling TGocciaBigIntValue (e.g., the division
branch using SErrorBigIntDivisionByZero, the modulo branch, and the
exponentiation branch that detect negative exponents) and change
ThrowTypeError(SError..., SSuggestBigIntNoMixedArithmetic) to
ThrowRangeError(SError...) so the error type matches the ECMAScript spec.
In `@source/units/Goccia.Evaluator.Comparison.pas`:
- Around line 634-642: The NaN guard currently skips both operands whenever any
operand is a TGocciaBigIntValue, allowing unordered comparisons
(CompareBigIntAndNumber returning code 2) to flow through and make >=/<= return
true; change the logic to individually check NaN only on non-BigInt operands
instead of skipping both: for ALeft and ARight separately, if the operand is not
TGocciaBigIntValue and its ToNumberLiteral.IsNaN then set Result := False and
Exit. Update the block around ALeft/ARight checks (the code that now uses if not
((ALeft is TGocciaBigIntValue) or (ARight is TGocciaBigIntValue)) ...) so it
performs two independent guards, one per operand, so CompareBigIntAndNumber /
LessThan / GreaterThan handle unordered cases correctly.
- Around line 540-567: CompareBigIntAndNumber currently converts the BigInt to
Double (via ABigInt.Value.ToDouble) which loses precision; instead implement the
ES2026 exact mathematical comparison: for finite ANumber, compute ANumber's
integer part (trunc toward zero) and whether ANumber has a non‑zero fractional
part, convert that integer part to a TGocciaBigIntValue (or otherwise construct
a BigInt representation) and use the BigInt comparison routine on ABigInt.Value
vs that integer-part BigInt; if the BigInts differ return that result, and if
they are equal then decide by the sign of the fractional part (if fractional
part > 0 then BigInt < Number, if fractional part < 0 then BigInt > Number, else
equal). Update CompareBigIntAndNumber to remove the ToDouble path and use this
integer-part BigInt comparison and fractional check while keeping the existing
NaN/Infinity handling.
In `@source/units/Goccia.JSON.pas`:
- Around line 841-844: The current throw for TGocciaBigIntValue bypasses any
BigInt toJSON hook because ApplyToJSON only probes toJSON for objects via
AValue.Box/MethodTarget; remove the is TGocciaObjectValue type-check and instead
call GetProperty directly on the TGocciaValue (i.e., use
AValue.GetProperty('toJSON')/Method lookup without boxing), since
TGocciaValue.GetProperty is virtual and TGocciaBigIntValue.GetProperty delegates
to its prototype; this ensures GetV(value,'toJSON') runs for BigInt before the
TypeError throw and fixes the bypass described (no changes to ClassHelper.pas
boxing behavior required).
In `@source/units/Goccia.Lexer.pas`:
- Around line 1429-1437: The lexer currently emits gttBigInt lexemes (via Peek,
Advance, HasDecimalOrExponent, HasSeparator and AddToken) preserving non-decimal
prefixes (0x, 0b, 0o) while the parser constructs BigInts using
TBigInteger.FromDecimalString(Token.Lexeme), causing mis-parsing; fix by
normalizing the lexeme to a decimal string before emitting gttBigInt (remove
prefix and convert the hex/bin/octal value to its decimal string after stripping
separators) OR update the parser to detect prefixed lexemes and call a
radix-aware constructor instead (replace
TBigInteger.FromDecimalString(Token.Lexeme) with a branch that parses based on
0x/0b/0o prefixes), ensuring the same decision (normalization or radix-aware
parse) is applied for both code paths mentioned (and the nearby block at lines
~1440-1444).
In `@source/units/Goccia.Values.BigIntValue.pas`:
- Around line 209-212: The code is using Trunc(RadixValue.ToNumberLiteral.Value)
and ThrowTypeError for BigInt.prototype.toString radix checks; replace the raw
truncation with the proper ToIntegerOrInfinity coercion on RadixValue and change
the thrown error to a RangeError. Locate the radix handling around
RadixValue/ToNumberLiteral and replace the Trunc(...) call with
RadixValue.ToIntegerOrInfinity (or the equivalent helper that implements
ToIntegerOrInfinity), then change ThrowTypeError(...) to ThrowRangeError(...)
(keeping a clear message like "toString() radix must be between 2 and 36") so
out-of-range radices produce a RangeError per spec.
In `@source/units/Goccia.VM.pas`:
- Around line 1802-1804: bckBigInt now produces TGocciaBigIntValue which exposes
BigInt operands to bytecode paths, but VM helpers (VMBitwiseAndValues,
VMBitwiseOrValues, VMBitwiseXorValues, VMShiftLeftValues, VMShiftRightValues,
VMUnsignedShiftRightValues, VMBitwiseNotValues and comparison helpers
VMLessThan/VMLessThanOrEqual/VMLargerThan/VMLargerThanOrEqual) still call
ToNumberLiteral and will TypeError for BigInt; update those helpers to first
check for TGocciaBigIntValue (or the concrete BigInt value type) and perform the
correct BigInt operation (use the BigInt value methods like
BitwiseAnd/BitwiseOr/BitwiseXor/ShiftLeft/ShiftRight/UnsignedShiftRight/BitwiseNot
and Compare) or delegate to the existing evaluator routines (EvaluateBitwiseAnd,
EvaluateGreaterThan, etc.) when BigInt operands are present, falling back to the
existing numeric path only when operands are not BigInt.
In `@tests/language/expressions/bigint/bigint-comparison.js`:
- Around line 7-9: Replace literal self-comparisons (1n === 1n, 0n === 0n, 1n
!== 1n, 1n <= 1n, 1n >= 1n) with comparisons between distinct variable bindings
to avoid the Biome noSelfCompare lint error: introduce const bindings like a =
1n, b = 1n (and similarly c = 0n for the zero checks) and update the
expectations to compare a === b, c === cB (or c === d if you create two zero
bindings), a !== b2 (use a different var set where needed), a <= b and a >= b as
appropriate so semantics are preserved while eliminating literal
self-comparisons.
---
Nitpick comments:
In `@source/units/Goccia.VM.pas`:
- Around line 467-468: VMSubtractValues, VMMultiplyValues, VMDivideValues,
VMModuloValues and VMPowerValues currently operate on raw operands and skip
ToPrimitive, causing incorrect handling of boxed BigInts; update each of these
functions to call ToPrimitive(ALeft) and ToPrimitive(ARight) (as VMAddValues
does) before performing the BigInt mixed-type check and before calling
VMToNumericPair, and then use the same XOR check (if (ALeft is
TGocciaBigIntValue) xor (ARight is TGocciaBigIntValue) then ThrowTypeError(...))
so boxed BigInt objects are coerced correctly prior to numeric conversion.
🪄 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: 3d28b19b-66e3-4f89-8e0d-11e5b807e38b
📒 Files selected for processing (28)
source/shared/BigInteger.passource/units/Goccia.Builtins.GlobalBigInt.passource/units/Goccia.Bytecode.Chunk.passource/units/Goccia.Compiler.Expressions.passource/units/Goccia.Constants.ConstructorNames.passource/units/Goccia.Constants.TypeNames.passource/units/Goccia.Engine.passource/units/Goccia.Error.Messages.passource/units/Goccia.Error.Suggestions.passource/units/Goccia.Evaluator.Arithmetic.passource/units/Goccia.Evaluator.Bitwise.passource/units/Goccia.Evaluator.Comparison.passource/units/Goccia.Evaluator.passource/units/Goccia.JSON.passource/units/Goccia.Lexer.passource/units/Goccia.Parser.passource/units/Goccia.Token.passource/units/Goccia.VM.passource/units/Goccia.Values.BigIntValue.passource/units/Goccia.Values.Primitives.pastests/built-ins/BigInt/coercion.jstests/built-ins/BigInt/constructor.jstests/built-ins/BigInt/prototype-methods.jstests/language/expressions/bigint/bigint-arithmetic.jstests/language/expressions/bigint/bigint-bitwise.jstests/language/expressions/bigint/bigint-comparison.jstests/language/expressions/bigint/bigint-literals.jstests/language/expressions/bigint/bigint-type-errors.js
Fix bytecode VM operations that were missing BigInt dispatch (bitwise, comparison, negation, equality), causing TypeError for all BigInt operations in bytecode mode. Fix spec compliance issues from review: use RangeError for division-by-zero/negative-exponent/non-integral conversion, SyntaxError for invalid string conversion, support non-decimal string formats in BigInt() constructor, fix cross-type BigInt/NaN comparison returning true for >= and <=, and free FBuiltinGlobalBigInt in engine destructor. Update Goccia.spec array, language tables, built-ins docs, and decision log. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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.Evaluator.Arithmetic.pas`:
- Around line 311-315: The code correctly throws ThrowRangeError when
TGocciaBigIntValue(ARight).Value.IsNegative, but the
SErrorBigIntNegativeExponent message text says "Exponent must be positive" which
is misleading because zero is allowed; update the SErrorBigIntNegativeExponent
message to a non‑negative wording such as "Exponent must be non-negative" (or
"Exponent must be >= 0") in the error messages unit so the thrown error
accurately reflects the check in TGocciaBigIntValue exponentiation.
In `@source/units/Goccia.Values.ClassHelper.pas`:
- Around line 36-40: Add unit tests for BigInt strict equality covering positive
cases, negative cases and edge values: create or update a test in
units/*.Test.pas (e.g., a test unit exercising the BigInt value wrapper used in
Goccia.Values.BigIntValue) that constructs TBigInteger-backed values and asserts
strict equality when values are identical (including very large integers) and
asserts inequality when they differ; specifically exercise the equality path
that relies on TBigInteger.Equal() (the code around the BigInt equality
implementation referenced in Goccia.Values.ClassHelper.pas) and include tests
that compare equal magnitude but different sign and very large magnitude values
to ensure correct behavior; finally run the formatter check ./format.pas --check
locally before committing.
🪄 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: 46c9fe8d-9d74-4c04-8a7e-b2c720f5222c
📒 Files selected for processing (13)
docs/built-ins-binary-data.mddocs/built-ins.mddocs/decision-log.mddocs/language-tables.mdsource/units/Goccia.Builtins.GlobalBigInt.passource/units/Goccia.Engine.passource/units/Goccia.Evaluator.Arithmetic.passource/units/Goccia.Evaluator.Comparison.passource/units/Goccia.Spec.passource/units/Goccia.VM.passource/units/Goccia.Values.BigIntValue.passource/units/Goccia.Values.ClassHelper.pastests/language/expressions/bigint/bigint-comparison.js
✅ Files skipped from review due to trivial changes (7)
- docs/language-tables.md
- docs/decision-log.md
- docs/built-ins.md
- tests/language/expressions/bigint/bigint-comparison.js
- docs/built-ins-binary-data.md
- source/units/Goccia.Spec.pas
- source/units/Goccia.Values.BigIntValue.pas
🚧 Files skipped from review as they are similar to previous changes (1)
- source/units/Goccia.Evaluator.Comparison.pas
Implement BigInt.asIntN and BigInt.asUintN with correct ToIndex coercion for the bits parameter. Replace ToDouble-based cross-type BigInt/Number comparison with exact mathematical-value comparison using TBigInteger.FromDouble, eliminating precision loss beyond 2^53. Fix BigInt/NaN comparisons for <= and >= returning true instead of false (stale .ppu cache masked the fix in prior commit). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Change "Exponent must be positive" to "Exponent must be non-negative" since 0n is a valid exponent (2n ** 0n === 1n). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/built-ins/BigInt/constructor.js (2)
6-10: Add a no-argument constructor test.Consider adding
BigInt()(no args) coverage to lock in theundefinedconversion failure path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/built-ins/BigInt/constructor.js` around lines 6 - 10, Add a new test case that asserts calling the BigInt constructor with no arguments throws the undefined conversion error: create a test like test("BigInt no-args", () => { expect(() => BigInt()).toThrow(TypeError); }) so the undefined-to-BigInt failure path is covered alongside the existing BigInt(number) assertions (referencing BigInt and the existing test names in constructor.js).
26-40: Assert specific error classes for negative paths.These tests currently verify “throws” but not the error type. Tightening to
RangeError/SyntaxErrorassertions will better protect the spec-compliance behavior this PR adds.Also applies to: 74-78
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/built-ins/BigInt/constructor.js` around lines 26 - 40, Tests currently use generic expect(...).toThrow(); tighten assertions to specific error classes: for BigInt(1.5), BigInt(NaN), and BigInt(Infinity) assert they throw RangeError; for BigInt("abc") assert it throws SyntaxError. Update the test cases that call BigInt in this file (e.g., the test blocks with BigInt(1.5), BigInt(NaN), BigInt(Infinity), BigInt("abc")) and the analogous tests around lines 74-78 to use .toThrow(RangeError) or .toThrow(SyntaxError) as appropriate.source/units/Goccia.Builtins.GlobalBigInt.pas (1)
193-201: Prefer centralized error message constants for index failures.
'Invalid index'is duplicated inBigIntToIndex. Consider routing this through a shared error-message constant (and optional suggestion) for consistency with the rest of runtime diagnostics.🤖 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 193 - 201, Replace the duplicated literal 'Invalid index' in BigIntToIndex with a centralized error-message constant (e.g. ERR_INVALID_INDEX) defined in this unit (or a shared runtime/errors unit) and update both ThrowRangeError calls to use that constant (optionally append the existing suggestion text where other runtime diagnostics do). Ensure the new constant name is unique and referenced from the same function (BigIntToIndex) so future index failures use the single shared message.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@source/units/Goccia.Builtins.GlobalBigInt.pas`:
- Around line 193-201: Replace the duplicated literal 'Invalid index' in
BigIntToIndex with a centralized error-message constant (e.g. ERR_INVALID_INDEX)
defined in this unit (or a shared runtime/errors unit) and update both
ThrowRangeError calls to use that constant (optionally append the existing
suggestion text where other runtime diagnostics do). Ensure the new constant
name is unique and referenced from the same function (BigIntToIndex) so future
index failures use the single shared message.
In `@tests/built-ins/BigInt/constructor.js`:
- Around line 6-10: Add a new test case that asserts calling the BigInt
constructor with no arguments throws the undefined conversion error: create a
test like test("BigInt no-args", () => { expect(() =>
BigInt()).toThrow(TypeError); }) so the undefined-to-BigInt failure path is
covered alongside the existing BigInt(number) assertions (referencing BigInt and
the existing test names in constructor.js).
- Around line 26-40: Tests currently use generic expect(...).toThrow(); tighten
assertions to specific error classes: for BigInt(1.5), BigInt(NaN), and
BigInt(Infinity) assert they throw RangeError; for BigInt("abc") assert it
throws SyntaxError. Update the test cases that call BigInt in this file (e.g.,
the test blocks with BigInt(1.5), BigInt(NaN), BigInt(Infinity), BigInt("abc"))
and the analogous tests around lines 74-78 to use .toThrow(RangeError) or
.toThrow(SyntaxError) as appropriate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 25a9ae2a-3e2c-4811-9279-93391fcacbf2
📒 Files selected for processing (8)
docs/built-ins.mddocs/decision-log.mddocs/language-tables.mdsource/units/Goccia.Builtins.GlobalBigInt.passource/units/Goccia.Evaluator.Comparison.passource/units/Goccia.VM.pastests/built-ins/BigInt/constructor.jstests/language/expressions/bigint/bigint-comparison.js
✅ Files skipped from review due to trivial changes (3)
- docs/language-tables.md
- docs/built-ins.md
- docs/decision-log.md
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/language/expressions/bigint/bigint-comparison.js
- source/units/Goccia.VM.pas
- source/units/Goccia.Evaluator.Comparison.pas
Create TGocciaBigIntObjectValue for boxing BigInt primitives via Object(). Fix Object() to call ToObject/Box for primitive arguments per ES2026 §20.1.1.1 instead of Instantiate. Add ToPrimitive calls in subtraction, multiplication, division, modulo, and exponentiation so boxed BigInts (Object(1n) - 1n) coerce correctly via valueOf(). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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.Values.BigIntObjectValue.pas`:
- Around line 108-116: The BigInt prototype methods must validate their receiver
types and handle the optional radix: update
TGocciaBigIntObjectValue.BigIntObjectValueOf to throw a TypeError when
AThisValue is neither TGocciaBigIntObjectValue nor TGocciaBigIntValue (instead
of returning arbitrary values), and implement TGocciaBigIntObjectValue.ToString
(or the existing BigInt toString method) to read radix from AArgs[0],
coerce/convert it the same way as Number.prototype.toString (see
Goccia.Values.NumberObjectValue.pas), defaulting to 10, validate radix is an
integer in [2,36] and raise RangeError if out of range, then produce the
radix-based string representation of the BigInt (e.g. 16 => "ff", 2 => "1010");
use the same error throwing pattern and helper utilities used elsewhere for
TypeError/RangeError to keep behavior consistent with ES2026.
🪄 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: 75aa32d6-a891-4743-9358-e301ff25a8b6
📒 Files selected for processing (5)
source/units/Goccia.Builtins.GlobalObject.passource/units/Goccia.Evaluator.Arithmetic.passource/units/Goccia.Values.BigIntObjectValue.passource/units/Goccia.Values.ClassHelper.passource/units/Goccia.Values.ClassValue.pas
✅ Files skipped from review due to trivial changes (1)
- source/units/Goccia.Builtins.GlobalObject.pas
🚧 Files skipped from review as they are similar to previous changes (1)
- source/units/Goccia.Evaluator.Arithmetic.pas
Move all hardcoded BigInt error strings to Goccia.Error.Messages and add corresponding suggestions to Goccia.Error.Suggestions. Add ToPrimitive calls to VM arithmetic functions so boxed BigInt works in bytecode mode. Add JS tests for String(), Number(), and Object() primitive conversions including boxed BigInt arithmetic. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
BigIntObjectValue.valueOf now throws TypeError for non-BigInt receivers instead of returning the arbitrary this value. BigIntObjectValue.toString validates the receiver, parses the radix argument with range validation (2-36, RangeError), and produces radix-based string output. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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.Values.BigIntValue.pas`:
- Around line 199-201: The BigInt.prototype methods in
Goccia.Values.BigIntValue.pas currently only accept TGocciaBigIntValue
primitives; update the receiver checks in the implementations of
BigInt.prototype.toString, valueOf and toLocaleString so they accept boxed
BigInt objects too by detecting TGocciaBigIntObjectValue and extracting its
.Primitive into a TGocciaBigIntValue (else use the primitive directly),
otherwise call ThrowTypeError as before; reference the pattern already used in
TGocciaBigIntObjectValue.BigIntObjectValueOf and BigIntObjectToString to guide
the exact conditional and extraction logic.
🪄 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: a6ffa2e1-6594-4fda-a4ec-60acc1ac6f9d
📒 Files selected for processing (11)
source/units/Goccia.Builtins.GlobalBigInt.passource/units/Goccia.Builtins.GlobalObject.passource/units/Goccia.Error.Messages.passource/units/Goccia.Error.Suggestions.passource/units/Goccia.Evaluator.Arithmetic.passource/units/Goccia.VM.passource/units/Goccia.Values.BigIntObjectValue.passource/units/Goccia.Values.BigIntValue.passource/units/Goccia.Values.ClassHelper.passource/units/Goccia.Values.ClassValue.pastests/built-ins/BigInt/coercion.js
✅ Files skipped from review due to trivial changes (3)
- source/units/Goccia.Builtins.GlobalObject.pas
- tests/built-ins/BigInt/coercion.js
- source/units/Goccia.Error.Suggestions.pas
🚧 Files skipped from review as they are similar to previous changes (6)
- source/units/Goccia.Values.ClassHelper.pas
- source/units/Goccia.Values.ClassValue.pas
- source/units/Goccia.Evaluator.Arithmetic.pas
- source/units/Goccia.Error.Messages.pas
- source/units/Goccia.VM.pas
- source/units/Goccia.Builtins.GlobalBigInt.pas
This was referenced Apr 18, 2026
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
asIntN/asUintNhelpers.