Fix Temporal BigInt duration overflow#431
Merged
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 (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughTemporal internals switched to exact BigInteger math: added a DurationMath unit, migrated Duration components to TBigInteger, changed Instant/ZonedDateTime diffs/rounding to epoch-nanoseconds BigInteger arithmetic, added epoch-ns validation and new range error messages, and updated tests for big-range/formatting behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as rgba(52,152,219,0.5)
participant Instant as rgba(46,204,113,0.5)
participant DurationMath as rgba(155,89,182,0.5)
participant DurationFactory as rgba(241,196,15,0.5)
Client->>Instant: until(other, options)
Instant->>DurationMath: EpochNanosecondsFromParts(epochMs, subMs)
DurationMath-->>Instant: epochNs (TBigInteger)
Instant->>DurationMath: TimeDurationFromEpochNanosecondsDifference(epochNs1, epochNs2)
DurationMath-->>Instant: diffNs (TBigInteger)
Instant->>DurationMath: RoundTimeDurationToIncrement(diffNs, increment, mode)
DurationMath-->>Instant: roundedDiffNs
Instant->>DurationMath: BalanceTimeDurationToFields(roundedDiffNs, largestUnit)
DurationMath-->>Instant: h,m,s,ms,us,ns,days (TBigInteger fields)
Instant->>DurationFactory: CreateFromBigIntegers(..., h,m,s,ms,us,ns)
DurationFactory-->>Instant: TGocciaTemporalDurationValue
Instant-->>Client: Duration result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
coderabbitai
Bot
added
bug
Contributor
Suite Timing
Measured on ubuntu-latest x64. |
Contributor
Benchmark Results386 benchmarks Interpreted: 🟢 157 improved · 🔴 8 regressed · 221 unchanged · avg +3.4% arraybuffer.js — Interp: 🟢 7, 7 unch. · avg +2.6% · Bytecode: 🟢 1, 🔴 11, 2 unch. · avg -6.7%
arrays.js — Interp: 🟢 6, 13 unch. · avg +1.7% · Bytecode: 🔴 17, 2 unch. · avg -9.5%
async-await.js — Interp: 🟢 1, 5 unch. · avg +2.9% · Bytecode: 🔴 4, 2 unch. · avg -8.4%
base64.js — Interp: 🟢 4, 6 unch. · avg +2.1% · Bytecode: 🔴 9, 1 unch. · avg -10.4%
classes.js — Interp: 🟢 9, 22 unch. · avg +1.5% · Bytecode: 🔴 21, 10 unch. · avg -5.3%
closures.js — Interp: 🟢 7, 4 unch. · avg +2.2% · Bytecode: 🔴 11 · avg -8.6%
collections.js — Interp: 🟢 8, 4 unch. · avg +2.9% · Bytecode: 🔴 12 · avg -9.2%
csv.js — Interp: 🟢 4, 9 unch. · avg +2.0% · Bytecode: 🔴 13 · avg -8.4%
destructuring.js — Interp: 🟢 7, 15 unch. · avg +2.3% · Bytecode: 🔴 19, 3 unch. · avg -8.1%
fibonacci.js — Interp: 🟢 1, 7 unch. · avg +1.7% · Bytecode: 🔴 8 · avg -12.2%
float16array.js — Interp: 🟢 12, 20 unch. · avg +2.5% · Bytecode: 🟢 7, 🔴 18, 7 unch. · avg -3.1%
for-of.js — Interp: 🟢 3, 4 unch. · avg +2.0% · Bytecode: 🔴 6, 1 unch. · avg -7.8%
helpers/bench-module.js — Interp: 0 · Bytecode: 0
iterators.js — Interp: 🟢 20, 🔴 3, 19 unch. · avg +2.4% · Bytecode: 🔴 38, 4 unch. · avg -7.1%
json.js — Interp: 🟢 10, 10 unch. · avg +3.5% · Bytecode: 🔴 19, 1 unch. · avg -7.5%
jsx.jsx — Interp: 🟢 13, 8 unch. · avg +2.5% · Bytecode: 🔴 20, 1 unch. · avg -7.2%
modules.js — Interp: 9 unch. · avg +1.1% · Bytecode: 🔴 9 · avg -9.4%
numbers.js — Interp: 🟢 5, 6 unch. · avg +3.3% · Bytecode: 🔴 10, 1 unch. · avg -5.6%
objects.js — Interp: 🟢 1, 6 unch. · avg +0.9% · Bytecode: 🔴 7 · avg -7.1%
promises.js — Interp: 🟢 8, 4 unch. · avg +4.7% · Bytecode: 🔴 11, 1 unch. · avg -6.6%
regexp.js — Interp: 🟢 1, 🔴 2, 8 unch. · avg +0.5% · Bytecode: 🔴 11 · avg -10.1%
strings.js — Interp: 🟢 4, 🔴 2, 13 unch. · avg -2.5% · Bytecode: 🟢 1, 🔴 17, 1 unch. · avg -9.5%
tsv.js — Interp: 🟢 6, 3 unch. · avg +3.4% · Bytecode: 🔴 9 · avg -8.6%
typed-arrays.js — Interp: 🟢 7, 🔴 1, 14 unch. · avg +1.0% · Bytecode: 🟢 10, 🔴 9, 3 unch. · avg +2.3%
uint8array-encoding.js — Interp: 🟢 13, 5 unch. · avg +31.6% · Bytecode: 🔴 15, 3 unch. · avg -11.2%
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: 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.Values.TemporalDuration.pas`:
- Around line 175-240: BigIntToLegacyInt64 currently returns 0 for out-of-range
TBigInteger which silently corrupts the legacy Int64 mirrors
(FYears..FNanoseconds) while the bigint fields (FYearsBig..FNanosecondsBig) keep
the real values; change the behavior to fail fast: modify BigIntToLegacyInt64 to
raise an exception (e.g. ERangeError) when AValue is outside Int64 bounds, and
update callers (SetDurationFields, TGocciaTemporalDurationValue.Create/
CreateFromBigIntegers) to allow that exception to propagate so code doesn’t
silently zero legacy mirrors—this ensures duration arithmetic uses consistent
bigint data or stops on overflow instead of mismatched state.
In `@tests/built-ins/Temporal/ZonedDateTime/prototype/until.js`:
- Around line 41-47: The test "until() with largestUnit microseconds does not
overflow" currently asserts dur.microseconds against a numeric literal that
exceeds Number.MAX_SAFE_INTEGER; change the assertion to compare a safe
representation such as the string form or a decomposition: call dur.toString()
and assert it equals the expected string, or split dur into safe integer
components (e.g., seconds and remaining microseconds) and assert those instead;
update the assertion referencing z1, z2, and dur (result of z1.until(z2, {
largestUnit: "microseconds" })) so you no longer compare dur.microseconds to an
unsafe JS number.
🪄 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: 2a98467b-9767-4e04-a902-695e36f81e02
📒 Files selected for processing (13)
source/units/Goccia.Builtins.Temporal.passource/units/Goccia.Error.Messages.passource/units/Goccia.Error.Suggestions.passource/units/Goccia.Temporal.DurationMath.passource/units/Goccia.Values.TemporalDuration.passource/units/Goccia.Values.TemporalInstant.passource/units/Goccia.Values.TemporalZonedDateTime.pastests/built-ins/Temporal/Duration/prototype/toString.jstests/built-ins/Temporal/Instant/fromEpochNanoseconds.jstests/built-ins/Temporal/Instant/prototype/since.jstests/built-ins/Temporal/Instant/prototype/until.jstests/built-ins/Temporal/ZonedDateTime/prototype/since.jstests/built-ins/Temporal/ZonedDateTime/prototype/until.js
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
source/units/Goccia.Values.TemporalDuration.pas (2)
1188-1193:⚠️ Potential issue | 🟠 Major
Duration.prototype.total()still demotes bigint-backed durations through checked Int64 accessors.
SubDayNanoseconds,ComputeTotalInUnits, and theResolvedDaysbranch all readD.*via the checkedInt64properties, sototal()becomes unusable on the same large nanosecond/microsecond durations this PR now produces. The accumulation should stay inTBigInteger/DurationMathuntil the finalNumberconversion at the API boundary. Based on learnings: the earlier microsecond/nanosecond overflow was intentionally deferred untilTGocciaTemporalDurationValuebecame BigInt-backed.May an implementation of `Temporal.Duration.prototype.total()` reject a valid duration only because one internal time component exceeds 64-bit integer range, or is the computation expected to use arbitrary-precision duration math until the final Number result?Also applies to: 1217-1217, 1339-1355
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/units/Goccia.Values.TemporalDuration.pas` around lines 1188 - 1193, Duration.prototype.total() currently reads D.* via checked Int64 accessors (e.g., SubDayNanoseconds, ComputeTotalInUnits, ResolvedDays and code paths using TGocciaTemporalDurationValue), which demotes BigInt-backed durations and can overflow; change these code paths to use the BigInteger/DurationMath representations (keep accumulation in TBigInteger or DurationMath) and only convert to Number at the API boundary (the final return), avoiding any use of the checked Int64 accessors for intermediate reads so large nanosecond/microsecond durations remain precise.
737-746:⚠️ Potential issue | 🟠 Major
Duration.prototype.round()is still Int64-bound.Lines 737-746 and 776-997 still route the new BigInt-backed fields through
D.Years/D.NanosecondsandInt64accumulators likeTotalNs. A valid duration produced byInstant.until(..., { largestUnit: "microsecond" | "nanosecond" })can now exceedHigh(Int64), soround()raises before it ever rounds. This path needs to stay onF*Big/TBigInteger, and the result should be built withCreateFromBigIntegerswhenever the rounded component can still be out ofInt64range. Based on learnings: the earlier microsecond/nanosecond overflow was intentionally deferred untilTGocciaTemporalDurationValuebecame BigInt-backed.Does the TC39 Temporal specification require `Temporal.Duration.prototype.round()` to operate on valid large durations returned from `Temporal.Instant.prototype.until(..., { largestUnit: "nanosecond" })`, even when the internal nanosecond count exceeds 64-bit integer range?Also applies to: 755-757, 776-787, 990-997
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/units/Goccia.Values.TemporalDuration.pas` around lines 737 - 746, Duration.prototype.round currently uses Int64-bound accessors (D.Years, D.Nanoseconds) and accumulators (TotalNs) which overflow for BigInt-backed durations; change the rounding path to operate on the BigInt fields (F*Big / TBigInteger) throughout instead of D.*/Int64 accumulators, and when constructing the result use TGocciaTemporalDurationValue.CreateFromBigIntegers so the returned Duration can contain components outside Int64 range; update all affected branches referenced in the diff (the D.Years..D.Microseconds checks, the TotalNs calculations, and the final construction paths around lines noted) to use the F*Big/TBigInteger variants and CreateFromBigIntegers where the component or total may exceed High(Int64).
🤖 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.TemporalDuration.pas`:
- Around line 428-449: The fractional subsecond trimming in ToISOString
currently conditions how many digits to keep based on FNanosecondsBig/FM
icrosecondsBig/FM illisecondsBig; instead, after computing SubSecondsPart and
building Fraction via Format('%.9d', [SubSecondsPart.ToInt64]) (around
SecondsDuration/AbsSecondsDuration/SecondsPart/SubSecondsPart/Fraction), always
strip trailing '0' characters from Fraction (loop while Length(Fraction)>0 and
last char='0' then delete) so the recombined nanoseconds are reduced to the
minimal representation (e.g., "200000000" -> "2"); remove the conditional
branches that truncate to 3 or 6 digits and ensure if Fraction becomes empty you
handle it the same way as the current "no fractional part" case.
---
Duplicate comments:
In `@source/units/Goccia.Values.TemporalDuration.pas`:
- Around line 1188-1193: Duration.prototype.total() currently reads D.* via
checked Int64 accessors (e.g., SubDayNanoseconds, ComputeTotalInUnits,
ResolvedDays and code paths using TGocciaTemporalDurationValue), which demotes
BigInt-backed durations and can overflow; change these code paths to use the
BigInteger/DurationMath representations (keep accumulation in TBigInteger or
DurationMath) and only convert to Number at the API boundary (the final return),
avoiding any use of the checked Int64 accessors for intermediate reads so large
nanosecond/microsecond durations remain precise.
- Around line 737-746: Duration.prototype.round currently uses Int64-bound
accessors (D.Years, D.Nanoseconds) and accumulators (TotalNs) which overflow for
BigInt-backed durations; change the rounding path to operate on the BigInt
fields (F*Big / TBigInteger) throughout instead of D.*/Int64 accumulators, and
when constructing the result use
TGocciaTemporalDurationValue.CreateFromBigIntegers so the returned Duration can
contain components outside Int64 range; update all affected branches referenced
in the diff (the D.Years..D.Microseconds checks, the TotalNs calculations, and
the final construction paths around lines noted) to use the F*Big/TBigInteger
variants and CreateFromBigIntegers where the component or total may exceed
High(Int64).
🪄 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: a8b215ee-713f-4591-b36a-4d60af8e94f6
📒 Files selected for processing (5)
source/units/Goccia.Values.TemporalDuration.pastests/built-ins/Temporal/Instant/prototype/since.jstests/built-ins/Temporal/Instant/prototype/until.jstests/built-ins/Temporal/ZonedDateTime/prototype/since.jstests/built-ins/Temporal/ZonedDateTime/prototype/until.js
✅ Files skipped from review due to trivial changes (1)
- tests/built-ins/Temporal/Instant/prototype/until.js
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/built-ins/Temporal/ZonedDateTime/prototype/until.js
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.TemporalDuration.pas (1)
853-863:⚠️ Potential issue | 🟠 MajorUse the shared negative bucket formula here as well.
Both calendar-unit branches still align negative values with
WholeUnits - 1. That is the old off-by-one bucket selection and it can over-round negativeyear/monthresults, especially withroundingIncrement = 1.Suggested fix
- if Sign < 0 then - ScaledValue := WholeUnits - 1 - else - ScaledValue := WholeUnits; - if (ScaledValue >= 0) or (ScaledValue mod Increment = 0) then - ScaledValue := (ScaledValue div Increment) * Increment - else - ScaledValue := ((ScaledValue div Increment) - 1) * Increment; + if Sign < 0 then + ScaledValue := -(((-WholeUnits) div Increment) * Increment) + else if (WholeUnits >= 0) or (WholeUnits mod Increment = 0) then + ScaledValue := (WholeUnits div Increment) * Increment + else + ScaledValue := ((WholeUnits div Increment) - 1) * Increment;Based on learnings: In
source/units/Goccia.Temporal.Options.pas(RoundDiffDuration, calendar-unit rounding path fortuYear/tuMonth), the correct negative alignment isScaledValue := -(((-WholeUnits) div AIncrement) * AIncrement);WholeUnits - 1was the prior bug.Also applies to: 903-911
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/units/Goccia.Values.TemporalDuration.pas` around lines 853 - 863, The negative-alignment branch that currently sets ScaledValue := WholeUnits - 1 should be replaced with the shared negative-bucket formula used in RoundDiffDuration: compute ScaledValue for negative values as -(((-WholeUnits) div Increment) * Increment) instead of WholeUnits - 1; update the branch that checks Sign < 0 (and the equivalent mirror at the other occurrence around lines 903-911) so the negative alignment and subsequent bucketing use WholeUnits via that formula while keeping the existing positive-path logic and the later mod/div adjustments unchanged.
♻️ Duplicate comments (1)
source/units/Goccia.Values.TemporalDuration.pas (1)
34-43:⚠️ Potential issue | 🟠 MajorKeep the Pascal accessors BigInt-safe too.
The class can now hold spec-valid duration components that exceed
High(Int64)in a single field, but these public properties still expose them only through checkedInt64getters. That meansTGocciaTemporalDurationValuecan represent a value that its Pascal API immediately rejects, which is a fragile surface for any remaining internal callers.Also applies to: 56-65, 293-341
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/units/Goccia.Values.TemporalDuration.pas` around lines 34 - 43, The Int64 getters (GetYearsInt64, GetMonthsInt64, GetWeeksInt64, GetDaysInt64, GetHoursInt64, GetMinutesInt64, GetSecondsInt64, GetMillisecondsInt64, GetMicrosecondsInt64, GetNanosecondsInt64) expose values with checked Int64 semantics while the TGocciaTemporalDurationValue can hold components larger than High(Int64); update these accessors to be BigInt-safe by returning the unlimited-precision integer type used in the module (or by adding corresponding BigInt getters/properties) and ensure any existing callers use the new BigInt-returning methods (or properly handle overflows) so the public Pascal API can represent spec-valid large components without truncation or exceptions.
🤖 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.TemporalDuration.pas`:
- Around line 666-675: GetFieldOr is truncating via Trunc(...) and
TBigInteger.FromDouble which forces values through Int64/double and overflows
large object-supplied integers; replace that conversion so we build a
TBigInteger directly from the value text or native bigint representation instead
of truncating to Int64. Specifically, in GetFieldOr use the object's
bigint-aware accessor (or parse the numeric literal string) to produce a
TBigInteger (e.g. construct from Val.ToString/ToStringLiteral or a ToBigInteger
method if available) instead of
TBigInteger.FromDouble(Trunc(Val.ToNumberLiteral.Value)) so large integers are
preserved.
---
Outside diff comments:
In `@source/units/Goccia.Values.TemporalDuration.pas`:
- Around line 853-863: The negative-alignment branch that currently sets
ScaledValue := WholeUnits - 1 should be replaced with the shared negative-bucket
formula used in RoundDiffDuration: compute ScaledValue for negative values as
-(((-WholeUnits) div Increment) * Increment) instead of WholeUnits - 1; update
the branch that checks Sign < 0 (and the equivalent mirror at the other
occurrence around lines 903-911) so the negative alignment and subsequent
bucketing use WholeUnits via that formula while keeping the existing
positive-path logic and the later mod/div adjustments unchanged.
---
Duplicate comments:
In `@source/units/Goccia.Values.TemporalDuration.pas`:
- Around line 34-43: The Int64 getters (GetYearsInt64, GetMonthsInt64,
GetWeeksInt64, GetDaysInt64, GetHoursInt64, GetMinutesInt64, GetSecondsInt64,
GetMillisecondsInt64, GetMicrosecondsInt64, GetNanosecondsInt64) expose values
with checked Int64 semantics while the TGocciaTemporalDurationValue can hold
components larger than High(Int64); update these accessors to be BigInt-safe by
returning the unlimited-precision integer type used in the module (or by adding
corresponding BigInt getters/properties) and ensure any existing callers use the
new BigInt-returning methods (or properly handle overflows) so the public Pascal
API can represent spec-valid large components without truncation or exceptions.
🪄 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: 9acc65dd-90ec-483d-93ca-c4a0c826ee22
📒 Files selected for processing (4)
source/units/Goccia.Values.TemporalDuration.pastests/built-ins/Temporal/Duration/prototype/round.jstests/built-ins/Temporal/Duration/prototype/toString.jstests/built-ins/Temporal/Duration/prototype/total.js
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/built-ins/Temporal/Duration/prototype/toString.js
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.TemporalDuration.pas`:
- Around line 201-219: Replace the plain Pascal Exception raises in
NumberToBigInteger with the proper Temporal/JS error classes: when IsNaN(AValue)
or IsInfinite(AValue) raise the JS RangeError (e.g. ERangeError or the project’s
Temporal RangeError wrapper) with a clear message, and when Frac(AValue) <> 0
raise the JS TypeError/RangeError as appropriate for your Temporal
implementation (use the project’s EJSTypeError or ERangeError class) instead of
Exception.Create; keep the rest of NumberToBigInteger and ValueToBigInteger
logic unchanged and reference the function name NumberToBigInteger when making
the changes.
🪄 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: 93798ea6-8f2a-4036-8821-e35923523ef8
📒 Files selected for processing (5)
source/units/Goccia.Builtins.Temporal.passource/units/Goccia.Values.TemporalDuration.pastests/built-ins/Temporal/Duration/compare.jstests/built-ins/Temporal/Duration/from.jstests/built-ins/Temporal/Duration/prototype/with.js
🚧 Files skipped from review as they are similar to previous changes (1)
- source/units/Goccia.Builtins.Temporal.pas
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.TemporalDuration.pas (1)
1120-1121:⚠️ Potential issue | 🟠 MajorReject
ZonedDateTimehere until the DST-awarerelativeTopath exists.Collapsing
relativeToto a plain date loses the zone/instant context needed for variable-length days, soDuration.prototype.total()will now silently treat a DST-crossingZonedDateTimeas a fixed 24-hour day. It’s safer to keep throwing for this input until the timezone-aware path is implemented. Based on learnings: support forTGocciaTemporalZonedDateTimeValueinParseRelativeToforDuration.prototype.total()was intentionally deferred because DST-aware day-length handling is still missing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/units/Goccia.Values.TemporalDuration.pas` around lines 1120 - 1121, In the branch that currently accepts TGocciaTemporalZonedDateTimeValue (the else if AValue is TGocciaTemporalZonedDateTimeValue then Result := ZonedDateTimeToDateRecord(...)) reject this type instead: update the ParseRelativeTo logic (or the enclosing method containing that branch) to raise/return an error for TGocciaTemporalZonedDateTimeValue rather than converting it to a plain date, and add a comment referencing that DST-aware handling is required (keep the ZonedDateTimeToDateRecord call removed/disabled until TGocciaTemporalZonedDateTimeValue is supported by the timezone-aware relativeTo path).
🤖 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.TemporalDuration.pas`:
- Around line 188-193: BigIntToCheckedInt64 currently raises a raw ERangeError
when the BigInt is out of Int64 range; create a specific exception (e.g.,
ETemporalOverflow or ETemporalRangeError inheriting from Exception) and have
BigIntToCheckedInt64 raise that instead (with the same formatted message) so the
JS-facing layers can catch/translate it instead of letting a raw Pascal
ERangeError escape; update the function BigIntToCheckedInt64 to raise the new
exception type and ensure any callers of the Years..Nanoseconds accessors will
be able to catch and convert this new exception for proper JS error handling.
---
Outside diff comments:
In `@source/units/Goccia.Values.TemporalDuration.pas`:
- Around line 1120-1121: In the branch that currently accepts
TGocciaTemporalZonedDateTimeValue (the else if AValue is
TGocciaTemporalZonedDateTimeValue then Result := ZonedDateTimeToDateRecord(...))
reject this type instead: update the ParseRelativeTo logic (or the enclosing
method containing that branch) to raise/return an error for
TGocciaTemporalZonedDateTimeValue rather than converting it to a plain date, and
add a comment referencing that DST-aware handling is required (keep the
ZonedDateTimeToDateRecord call removed/disabled until
TGocciaTemporalZonedDateTimeValue is supported by the timezone-aware relativeTo
path).
🪄 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: b953a421-fc1a-49dd-9960-329d86b10989
📒 Files selected for processing (2)
source/units/Goccia.Values.TemporalDuration.pastests/built-ins/Temporal/Duration/prototype/with.js
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/built-ins/Temporal/Duration/prototype/with.js
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.TemporalInstant.pas (1)
259-266:⚠️ Potential issue | 🔴 CriticalUse
Int64to accumulate sub-millisecond nanoseconds before normalization.When
Dur.Microsecondsexceeds ~2.1 million, the expressionInteger(Dur.Microseconds * 1000 + Dur.Nanoseconds)overflows the 32-bit integer range and wraps silently. The exception handler only catchesETemporalDurationInt64Overflowfrom property access, not the Integer overflow itself, so the wrapped value corrupts the result.Accumulate in
Int64and normalize in-place:SubNsTotal := Int64(Inst.FSubMillisecondNanoseconds) + (Dur.Microseconds * 1000 + Dur.Nanoseconds); if SubNsTotal >= 0 then begin Inc(NewMs, SubNsTotal div 1000000); NewSubMs := Integer(SubNsTotal mod 1000000); end else begin CarryMs := (SubNsTotal - 999999) div 1000000; Inc(NewMs, CarryMs); NewSubMs := Integer(SubNsTotal - CarryMs * 1000000); end;Apply the same fix to
InstantSubtract(line 307–314).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/units/Goccia.Values.TemporalInstant.pas` around lines 259 - 266, The code currently computes NewSubMs using Integer(Dur.Microseconds * 1000 + Dur.Nanoseconds) which can overflow 32-bit; change the accumulation to use an Int64 temporary (e.g., SubNsTotal := Int64(Inst.FSubMillisecondNanoseconds) + Int64(Dur.Microseconds) * 1000 + Int64(Dur.Nanoseconds)), then normalize by carrying whole milliseconds into NewMs and leaving the remainder in NewSubMs (handling both positive and negative totals correctly), replacing the direct Integer cast; make the identical change in the corresponding InstantSubtract logic so both InstantAdd/InstantSubtract use Int64 accumulation and proper normalization of sub-millisecond nanoseconds.
🤖 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.TemporalInstant.pas`:
- Around line 259-266: The code currently computes NewSubMs using
Integer(Dur.Microseconds * 1000 + Dur.Nanoseconds) which can overflow 32-bit;
change the accumulation to use an Int64 temporary (e.g., SubNsTotal :=
Int64(Inst.FSubMillisecondNanoseconds) + Int64(Dur.Microseconds) * 1000 +
Int64(Dur.Nanoseconds)), then normalize by carrying whole milliseconds into
NewMs and leaving the remainder in NewSubMs (handling both positive and negative
totals correctly), replacing the direct Integer cast; make the identical change
in the corresponding InstantSubtract logic so both InstantAdd/InstantSubtract
use Int64 accumulation and proper normalization of sub-millisecond nanoseconds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1674536b-1b30-41b0-9507-0de85de274f6
📒 Files selected for processing (10)
source/units/Goccia.Values.TemporalDuration.passource/units/Goccia.Values.TemporalInstant.passource/units/Goccia.Values.TemporalPlainDate.passource/units/Goccia.Values.TemporalPlainDateTime.passource/units/Goccia.Values.TemporalPlainTime.passource/units/Goccia.Values.TemporalPlainYearMonth.passource/units/Goccia.Values.TemporalZonedDateTime.pastests/built-ins/Temporal/Duration/prototype/round.jstests/built-ins/Temporal/Duration/prototype/total.jstests/built-ins/Temporal/Duration/prototype/with.js
✅ Files skipped from review due to trivial changes (1)
- tests/built-ins/Temporal/Duration/prototype/with.js
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 (3)
source/units/Goccia.Values.TemporalInstant.pas (3)
437-489:⚠️ Potential issue | 🟠 MajorBuild the rounding divisor in BigInteger space.
UnitToNanoseconds(SmallestUnit) * Incrementstill multiplies in Int64 before conversion, so a legal largeroundingIncrementcan overflow here, especially when rounding by hours. Convert each operand toTBigIntegerfirst, then multiply.Proposed fix
- BigDivisor := TBigInteger.FromInt64(UnitToNanoseconds(SmallestUnit) * Increment); + BigDivisor := TBigInteger.FromInt64(UnitToNanoseconds(SmallestUnit)) + .Multiply(TBigInteger.FromInt64(Increment));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/units/Goccia.Values.TemporalInstant.pas` around lines 437 - 489, In InstantRound, BigDivisor is currently built by multiplying UnitToNanoseconds(SmallestUnit) * Increment in Int64 then converting to TBigInteger, which can overflow; change construction so both operands are converted to TBigInteger first (e.g. BigDivisor := TBigInteger.FromInt64(UnitToNanoseconds(SmallestUnit)).Multiply(TBigInteger.FromInt32(Increment))) so the multiplication happens in big-int space before passing to RoundBigIntWithMode; update references in InstantRound (BigDivisor, BigTotal, BigMillion, RoundBigIntWithMode usage) accordingly.
295-343:⚠️ Potential issue | 🟠 MajorSame overflow gap in
subtract().This path has the same problem as
add(): the new exception handler does not protect the Int64 arithmetic used to scale the duration components, so large valid inputs can still produce the wrong instant. Please switch the whole subtraction path to BigInteger math before normalizing the sub-millisecond remainder.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/units/Goccia.Values.TemporalInstant.pas` around lines 295 - 343, The subtraction path in TGocciaTemporalInstantValue.InstantSubtract uses Int64 math for scaling duration components (variables NewMs and SubNsTotal) and can overflow; change the computation to use BigInteger (or an arbitrary-precision integer type) for the whole subtraction sequence (subtract days/hours/minutes/seconds/milliseconds and micro/nanosecond parts) and only convert the final results back to Int64/Integer when calling NormalizeInstantSubMillisecondNanoseconds and creating the result; keep the existing ETemporalDurationInt64Overflow handler around the conversion so large-but-valid durations are computed safely before normalizing sub-millisecond remainder and returning TGocciaTemporalInstantValue.Create(NewMs, NewSubMs).
243-292:⚠️ Potential issue | 🟠 MajorMove the add-path delta math off Int64.
The new
try/exceptonly catchesETemporalDurationInt64Overflow; theDur.Days..Dur.Millisecondsscaling and sub-millisecond accumulation still use Int64 math and can wrap before normalization for large but valid durations. Please accumulate the full delta inTBigInteger, then normalize back to milliseconds/nanoseconds.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/units/Goccia.Values.TemporalInstant.pas` around lines 243 - 292, InstantAdd currently multiplies Dur fields into Int64 (NewMs/SubNsTotal) and can overflow before the try/except; change the delta accumulation to use TBigInteger: compute totalMilliseconds = TBigInteger(Dur.Days)*86400000 + TBigInteger(Dur.Hours)*3600000 + ... + TBigInteger(Dur.Milliseconds) and totalNanoseconds = TBigInteger(Inst.FSubMillisecondNanoseconds) + TBigInteger(Dur.Microseconds)*NANOSECONDS_PER_MICROSECOND + TBigInteger(Dur.Nanoseconds), then convert the combined big-integer values into Int64 NewMs and Integer NewSubMs only after calling NormalizeInstantSubMillisecondNanoseconds (or an adapted normalization that accepts big integers), and keep the existing except on ETemporalDurationInt64Overflow to ThrowRangeError; update references to NewMs/SubNsTotal to use the big-integer temporaries and ensure any conversion detects overflow and triggers the same range error.
🤖 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.TemporalInstant.pas`:
- Around line 437-489: In InstantRound, BigDivisor is currently built by
multiplying UnitToNanoseconds(SmallestUnit) * Increment in Int64 then converting
to TBigInteger, which can overflow; change construction so both operands are
converted to TBigInteger first (e.g. BigDivisor :=
TBigInteger.FromInt64(UnitToNanoseconds(SmallestUnit)).Multiply(TBigInteger.FromInt32(Increment)))
so the multiplication happens in big-int space before passing to
RoundBigIntWithMode; update references in InstantRound (BigDivisor, BigTotal,
BigMillion, RoundBigIntWithMode usage) accordingly.
- Around line 295-343: The subtraction path in
TGocciaTemporalInstantValue.InstantSubtract uses Int64 math for scaling duration
components (variables NewMs and SubNsTotal) and can overflow; change the
computation to use BigInteger (or an arbitrary-precision integer type) for the
whole subtraction sequence (subtract days/hours/minutes/seconds/milliseconds and
micro/nanosecond parts) and only convert the final results back to Int64/Integer
when calling NormalizeInstantSubMillisecondNanoseconds and creating the result;
keep the existing ETemporalDurationInt64Overflow handler around the conversion
so large-but-valid durations are computed safely before normalizing
sub-millisecond remainder and returning
TGocciaTemporalInstantValue.Create(NewMs, NewSubMs).
- Around line 243-292: InstantAdd currently multiplies Dur fields into Int64
(NewMs/SubNsTotal) and can overflow before the try/except; change the delta
accumulation to use TBigInteger: compute totalMilliseconds =
TBigInteger(Dur.Days)*86400000 + TBigInteger(Dur.Hours)*3600000 + ... +
TBigInteger(Dur.Milliseconds) and totalNanoseconds =
TBigInteger(Inst.FSubMillisecondNanoseconds) +
TBigInteger(Dur.Microseconds)*NANOSECONDS_PER_MICROSECOND +
TBigInteger(Dur.Nanoseconds), then convert the combined big-integer values into
Int64 NewMs and Integer NewSubMs only after calling
NormalizeInstantSubMillisecondNanoseconds (or an adapted normalization that
accepts big integers), and keep the existing except on
ETemporalDurationInt64Overflow to ThrowRangeError; update references to
NewMs/SubNsTotal to use the big-integer temporaries and ensure any conversion
detects overflow and triggers the same range error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: eab049dc-aeb9-4b47-a18d-94b61624d671
📒 Files selected for processing (3)
source/units/Goccia.Values.TemporalInstant.pastests/built-ins/Temporal/Instant/prototype/add.jstests/built-ins/Temporal/Instant/prototype/subtract.js
✅ Files skipped from review due to trivial changes (1)
- tests/built-ins/Temporal/Instant/prototype/add.js
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
Fixes #420