Improve Temporal conformance and enable BigInt in test262#354
Merged
Conversation
- Accept BigInt syntax in Test262 filtering - Coerce Temporal string inputs with offsets and annotations - Allow offset-based time zone IDs and improve Temporal constructor properties
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)
📝 WalkthroughWalkthroughCentralizes Temporal ISO coercion and annotation handling, adds option-driven toString/toLocaleString rounding and calendar annotations, accepts offset-form timezone IDs, registers @@toStringTag and toLocaleString on Temporal prototypes, and enables BigInt literals in the test syntax filter. Changes
Sequence Diagram(s)sequenceDiagram
participant Input as rgba(200,200,255,0.5) String/Input/Args
participant Utils as rgba(200,255,200,0.5) TemporalUtils
participant Coerce as rgba(255,220,200,0.5) CoercionLayer
participant TZ as rgba(255,200,200,0.5) TimeZoneResolver
participant Value as rgba(200,255,255,0.5) TemporalValue
Input->>Utils: StripAnnotations / StripOffsetAndAnnotations
Utils-->>Coerce: Cleaned string
Coerce->>TZ: Parse offset-form ID if present
TZ-->>Coerce: Offset seconds (or none)
Coerce->>Value: Return Date/Time/Instant/YearMonth/MonthDay records
Value-->>Value: Construct/compare using object or coerced record
sequenceDiagram
participant Value as rgba(200,200,255,0.5) TemporalValue
participant Options as rgba(200,255,200,0.5) ResolveTemporalToStringOptions
participant Rounder as rgba(255,220,200,0.5) RoundTimeForToString
participant Formatter as rgba(255,200,200,0.5) Formatter/Annotation
Value->>Options: Resolve options (fractionalDigits, roundingMode, calendarDisplay)
Options-->>Value: FracDigits, RoundingMode, CalendarDisplay
Value->>Rounder: Round time fields (may produce ExtraDays)
Rounder-->>Value: Rounded fields + ExtraDays
Value->>Formatter: Format time (precision or HH:MM) and append calendar annotation
Formatter-->>Value: Formatted string
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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: 🟢 265 improved · 🔴 19 regressed · 102 unchanged · avg +6.1% arraybuffer.js — Interp: 🟢 10, 🔴 1, 3 unch. · avg +5.6% · Bytecode: 🟢 14 · avg +35.7%
arrays.js — Interp: 🟢 16, 3 unch. · avg +9.0% · Bytecode: 🟢 19 · avg +39.0%
async-await.js — Interp: 🟢 5, 1 unch. · avg +5.7% · Bytecode: 🟢 6 · avg +32.8%
base64.js — Interp: 🟢 9, 1 unch. · avg +7.9% · Bytecode: 🟢 10 · avg +39.4%
classes.js — Interp: 🟢 14, 🔴 1, 16 unch. · avg +2.3% · Bytecode: 🟢 31 · avg +28.6%
closures.js — Interp: 🟢 10, 1 unch. · avg +8.1% · Bytecode: 🟢 11 · avg +39.1%
collections.js — Interp: 🟢 12 · avg +12.1% · Bytecode: 🟢 12 · avg +42.9%
csv.js — Interp: 🟢 12, 1 unch. · avg +8.4% · Bytecode: 🟢 13 · avg +37.9%
destructuring.js — Interp: 🟢 20, 2 unch. · avg +6.8% · Bytecode: 🟢 22 · avg +36.4%
fibonacci.js — Interp: 🟢 8 · avg +9.2% · Bytecode: 🟢 8 · avg +43.8%
float16array.js — Interp: 🟢 20, 🔴 4, 8 unch. · avg +2.9% · Bytecode: 🟢 31, 1 unch. · avg +34.8%
for-of.js — Interp: 🟢 2, 🔴 1, 4 unch. · avg +1.0% · Bytecode: 🟢 7 · avg +45.6%
helpers/bench-module.js — Interp: 0 · Bytecode: 0
iterators.js — Interp: 🟢 26, 16 unch. · avg +5.4% · Bytecode: 🟢 42 · avg +29.8%
json.js — Interp: 🟢 19, 1 unch. · avg +9.4% · Bytecode: 🟢 20 · avg +42.5%
jsx.jsx — Interp: 🟢 4, 17 unch. · avg +1.5% · Bytecode: 🟢 21 · avg +34.5%
modules.js — Interp: 🟢 7, 2 unch. · avg +9.2% · Bytecode: 🟢 9 · avg +51.3%
numbers.js — Interp: 🟢 8, 3 unch. · avg +6.6% · Bytecode: 🟢 11 · avg +37.5%
objects.js — Interp: 🟢 7 · avg +8.1% · Bytecode: 🟢 7 · avg +38.9%
promises.js — Interp: 🟢 10, 2 unch. · avg +6.4% · Bytecode: 🟢 12 · avg +37.4%
regexp.js — Interp: 🟢 9, 2 unch. · avg +5.1% · Bytecode: 🟢 11 · avg +34.6%
strings.js — Interp: 🟢 3, 🔴 7, 9 unch. · avg -10.1% · Bytecode: 🟢 19 · avg +38.0%
tsv.js — Interp: 🟢 7, 2 unch. · avg +7.8% · Bytecode: 🟢 9 · avg +38.7%
typed-arrays.js — Interp: 🟢 15, 🔴 4, 3 unch. · avg +4.2% · Bytecode: 🟢 22 · avg +27.1%
uint8array-encoding.js — Interp: 🟢 12, 🔴 1, 5 unch. · avg +27.6% · Bytecode: 🟢 18 · avg +87.4%
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: 7
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.TemporalZonedDateTime.pas (1)
1111-1151:⚠️ Potential issue | 🟠 MajorRecompute the offset after rounding.
OffsetSecondsis captured from the original instant beforeRoundTimeForToStringmutates the displayed wall-clock fields. If rounding rolls into a different offset boundary, this can emit a rounded local date/time paired with the old offset, i.e. a string that never actually occurred in that zone.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/units/Goccia.Values.TemporalZonedDateTime.pas` around lines 1111 - 1151, Recompute and use the zone offset after rounding: currently OffsetSeconds is obtained before calling RoundTimeForToString, so if rounding changes the wall-clock instant across an offset boundary the formatted string will show a stale offset. Move or repeat the call to GetUtcOffsetSeconds after RoundTimeForToString (and after adjusting DateRec when ExtraDays != 0) using Zdt.FTimeZone and the possibly adjusted epoch/instant (derived from the rounded local fields or by computing the shifted epoch milliseconds corresponding to DateRec and LHour/LMinute/etc.), then use that recomputed OffsetSeconds in FormatOffsetString; ensure the new OffsetSeconds replaces the original variable used when building S.source/units/Goccia.Builtins.Temporal.pas (2)
884-898:⚠️ Potential issue | 🟠 MajorMissing required-field validation for object input.
Unlike
PlainDateFrom(lines 554-574) which throwsTypeErrorwhenyear,month, ordayare missing/undefined,PlainDateTimeFromsilently defaults them to0. This creates invalid dates (month 0, day 0). Per TC39 Temporal spec,PlainDateTime.from(propertyBag)requires year/month/day for the ISO calendar.🐛 Proposed fix to validate required fields
else if Arg is TGocciaObjectValue then begin Obj := TGocciaObjectValue(Arg); - Y := 0; Mo := 0; D := 0; H := 0; Mi := 0; S := 0; Ms := 0; Us := 0; Ns := 0; - V := Obj.GetProperty(PROP_YEAR); if Assigned(V) and not (V is TGocciaUndefinedLiteralValue) then Y := Trunc(V.ToNumberLiteral.Value); - V := Obj.GetProperty(PROP_MONTH); if Assigned(V) and not (V is TGocciaUndefinedLiteralValue) then Mo := Trunc(V.ToNumberLiteral.Value); - V := Obj.GetProperty(PROP_DAY); if Assigned(V) and not (V is TGocciaUndefinedLiteralValue) then D := Trunc(V.ToNumberLiteral.Value); + V := Obj.GetProperty(PROP_YEAR); + if (V = nil) or (V is TGocciaUndefinedLiteralValue) then + ThrowTypeError(SErrorTemporalPlainDateTimeFromProps, SSuggestTemporalFromArg); + Y := Trunc(V.ToNumberLiteral.Value); + V := Obj.GetProperty(PROP_MONTH); + if (V = nil) or (V is TGocciaUndefinedLiteralValue) then + ThrowTypeError(SErrorTemporalPlainDateTimeFromProps, SSuggestTemporalFromArg); + Mo := Trunc(V.ToNumberLiteral.Value); + V := Obj.GetProperty(PROP_DAY); + if (V = nil) or (V is TGocciaUndefinedLiteralValue) then + ThrowTypeError(SErrorTemporalPlainDateTimeFromProps, SSuggestTemporalFromArg); + D := Trunc(V.ToNumberLiteral.Value); + H := 0; Mi := 0; S := 0; Ms := 0; Us := 0; Ns := 0; V := Obj.GetProperty(PROP_HOUR); if Assigned(V) and not (V is TGocciaUndefinedLiteralValue) then H := Trunc(V.ToNumberLiteral.Value);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/units/Goccia.Builtins.Temporal.pas` around lines 884 - 898, The PlainDateTimeFrom branch is missing required-field validation: when Arg is a TGocciaObjectValue you must ensure PROP_YEAR, PROP_MONTH and PROP_DAY are present and not TGocciaUndefinedLiteralValue and raise a TypeError if any are missing (same behavior as PlainDateFrom), before extracting and Trunc()-ing values and creating TGocciaTemporalPlainDateTimeValue; update the code that reads V := Obj.GetProperty(PROP_YEAR)/PROP_MONTH/PROP_DAY to check for Assigned(V) AND not (V is TGocciaUndefinedLiteralValue) and if that check fails for year, month or day call the runtime TypeError throw path used elsewhere.
310-316:⚠️ Potential issue | 🔴 Critical
Duration.compareviolates TC39 Temporal spec for calendar units.Per TC39 Temporal specification,
Duration.compare(one, two)must throwRangeErrorif either duration has nonzeroyearsormonthscomponents without arelativeTooption. The current implementation (1) accepts only 2 arguments and lacks support for arelativeToparameter, (2) silently ignoresyearsandmonthsfields in the nanoseconds calculation, and (3) produces incorrect comparison results for durations with calendar units.The function must be updated to:
- Accept an optional third argument for
{ relativeTo }options- Validate and throw
RangeErrorif either duration has nonzeroyearsormonthsandrelativeTois absent or invalid- For ISO calendar (the only supported calendar),
weeksdo not requirerelativeTo(fixed 7 days), but years/months do🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/units/Goccia.Builtins.Temporal.pas` around lines 310 - 316, The Duration.compare implementation must be updated: change the signature of Duration.compare to accept an optional third parameter (options) carrying relativeTo, validate options.relativeTo (or its absence), and if either input duration (D1 or D2) has nonzero years or months and no valid relativeTo is provided, raise a RangeError; do not silently ignore years/months in the TotalNs calculation—if relativeTo is provided and uses the ISO calendar, convert years/months to days/nanoseconds using the calendar-aware conversion logic before computing TotalNs1/TotalNs2 (weeks remain fixed as 7 days and do not require relativeTo), and then compare the computed totals. Ensure the RangeError logic and relativeTo handling are implemented in the Duration.compare function that computes TotalNs1/TotalNs2 so comparisons of calendar-unit durations follow the TC39 Temporal spec.
🧹 Nitpick comments (2)
source/units/Goccia.Builtins.Temporal.pas (2)
243-247: Consider adding property name constants for Duration's plural field names.Duration uses plural property names (
'years','months', etc.) per TC39 spec, distinct from the singularPROP_YEAR,PROP_MONTHconstants used by other Temporal types. For consistency with the project's property-name constant pattern, consider addingPROP_YEARS,PROP_MONTHS,PROP_WEEKS, etc. toGoccia.Constants.PropertyNames.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/units/Goccia.Builtins.Temporal.pas` around lines 243 - 247, Add plural property-name constants for Duration to Goccia.Constants.PropertyNames (e.g., PROP_YEARS, PROP_MONTHS, PROP_WEEKS, PROP_DAYS, PROP_HOURS, PROP_MINUTES, PROP_SECONDS, PROP_MILLISECONDS, PROP_MICROSECONDS, PROP_NANOSECONDS) and update usages in duration construction sites to use those constants instead of hard-coded strings; specifically replace the string keys passed to GetFieldOr in TGocciaTemporalDurationValue.Create with the new PROP_* constants and ensure the constants follow the same naming and visibility pattern as existing PROP_YEAR/PROP_MONTH so other code can reuse them.
1096-1117: Consider adding object coercion toPlainYearMonthComparefor consistency.
PlainDateCompare,PlainTimeCompare, andPlainDateTimeComparenow accept object inputs with property-bag coercion, butPlainYearMonthCompareonly acceptsTGocciaTemporalPlainYearMonthValueor string inputs. For API consistency, consider adding object coercion that readsyearandmonthproperties.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/units/Goccia.Builtins.Temporal.pas` around lines 1096 - 1117, CoerceYearMonth currently only handles TGocciaTemporalPlainYearMonthValue and string inputs; add object coercion like other comparators by detecting TGocciaObjectValue in CoerceYearMonth, extracting the "year" and "month" properties (accept number or string), convert them to integers (or use existing CoerceToISOYearMonth helper/validation), and construct TGocciaTemporalPlainYearMonthValue.Create(Y, M) on success; on bad/missing props call ThrowRangeError(SErrorInvalidISOYearMonth, SSuggestTemporalISOFormat) or ThrowTypeError(SErrorTemporalPlainYearMonthCompareArg, SSuggestTemporalCompareArg) consistent with existing behavior.
🤖 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.Temporal.Options.pas`:
- Around line 559-594: ResolveTemporalToStringOptions currently silently accepts
primitive options by treating them like nil; change the initial validation so
that AOptions = nil (undefined) still returns, but if AOptions is a primitive
(not an object) the routine raises a TypeError (use ThrowTypeError with a
message like "options must be an object or undefined"); keep the existing
precedence where smallestUnit (handled by
GetOptionString/GetTemporalUnitFromString) overrides fractionalSecondDigits
(GetFractionalSecondDigits) and retain ARoundingMode via GetRoundingMode, and
update the comment to remove the incorrect "mutually exclusive" wording.
In `@source/units/Goccia.Temporal.Utils.pas`:
- Around line 938-957: The loop that extracts bracket annotations currently
assigns the first non-calendar annotation to ATimeZone and silently drops any
subsequent zone-style annotations; change it to reject a second zone-style
annotation by detecting when AnnotContent (after stripping a leading '!') is a
non-'u-ca=' annotation and ATimeZone is already non-empty, then raise/return an
error (or set an invalid flag) instead of overwriting or ignoring it; update the
logic inside the while loop that processes AnnotContent/ATimeZone/Rest so the
parser fails on a second zone annotation rather than silently accepting only the
first.
- Around line 1101-1125: CoerceToISOInstant currently accepts offsetless
datetimes by falling back to TryParseISODateTime and forcing AOffsetSeconds :=
0; remove that fallback so only inputs parsed by TryParseISODateTimeWithOffset
succeed. In practice, delete the block that calls TryParseISODateTime (lines
using TryParseISODateTime, setting AOffsetSeconds := 0 and returning True), and
ensure the function returns False when TryParseISODateTimeWithOffset fails so
offsetless strings cause failure; keep the existing annotation stripping and
TimeZone-aware parsing in CoerceToISOInstant intact.
In `@source/units/Goccia.Values.TemporalPlainDate.pas`:
- Around line 657-668: In TGocciaTemporalPlainDateValue.DateWithCalendar guard
access to Arg before calling Arg.ToStringLiteral.Value: after Arg :=
AArgs.GetElement(0) check that Arg is not nil (or represents undefined/missing
in your runtime) and if it is, raise a JS-visible error (e.g. call
ThrowTypeError with an appropriate message) instead of dereferencing; then
proceed to read CalId and the existing calendar validation and result creation.
In `@source/units/Goccia.Values.TemporalPlainDateTime.pas`:
- Around line 871-881: In
TGocciaTemporalPlainDateTimeValue.DateTimeWithCalendar, guard the argument
returned by AArgs.GetElement(0) before calling Arg.ToStringLiteral: check if Arg
is nil (or represents undefined) and throw the appropriate JS-visible error (use
the same throw helper pattern as other methods, e.g. ThrowTypeError or
ThrowRangeError with SSuggestTemporalFromArg) instead of dereferencing it, then
only call Arg.ToStringLiteral.Value and validate CalId.
In `@source/units/Goccia.Values.TemporalPlainMonthDay.pas`:
- Around line 292-307: MonthDayToString currently emits only "MM-DD" then
FormatCalendarAnnotation, which drops the instance's FReferenceYear and breaks
round-tripping when a calendar annotation is present; modify
TGocciaTemporalPlainMonthDayValue.MonthDayToString so that if
GetCalendarDisplay(OptionsObj) indicates a calendar annotation should be shown
(i.e., CalDisp is not the value meaning "none"), the string uses the full
"YYYY-MM-DD" form by including PadFourOrSigned(FReferenceYear) (or otherwise
formatting FReferenceYear as a four-digit year) before '-' + PadTwo(FMonth) +
'-' + PadTwo(FDay) and then append FormatCalendarAnnotation(CalDisp) when
creating the TGocciaStringLiteralValue; leave the existing "MM-DD" behavior only
when no calendar annotation is requested.
In `@source/units/Goccia.Values.TemporalPlainYearMonth.pas`:
- Around line 475-490: The YearMonthToString implementation
(TGocciaTemporalPlainYearMonthValue.YearMonthToString) must include the
reference day when the calendar is non-ISO; update the code that builds the
string (currently using FormatYearMonthString(YM.FYear, YM.FMonth) +
FormatCalendarAnnotation(CalDisp)) to insert a "-DD" reference day before the
calendar annotation for non-ISO calendars—obtain the day from the PlainYearMonth
instance (YM) via its stored reference day field or accessor (e.g.,
YM.ReferenceDay or the existing field) and format it as two digits; keep using
GetCalendarDisplay(OptionsObj) to decide when to include the calendar annotation
and only append the reference day when the calendar is not ISO so the result
becomes "YYYY-MM-DD" (plus FormatCalendarAnnotation(CalDisp) when required).
---
Outside diff comments:
In `@source/units/Goccia.Builtins.Temporal.pas`:
- Around line 884-898: The PlainDateTimeFrom branch is missing required-field
validation: when Arg is a TGocciaObjectValue you must ensure PROP_YEAR,
PROP_MONTH and PROP_DAY are present and not TGocciaUndefinedLiteralValue and
raise a TypeError if any are missing (same behavior as PlainDateFrom), before
extracting and Trunc()-ing values and creating
TGocciaTemporalPlainDateTimeValue; update the code that reads V :=
Obj.GetProperty(PROP_YEAR)/PROP_MONTH/PROP_DAY to check for Assigned(V) AND not
(V is TGocciaUndefinedLiteralValue) and if that check fails for year, month or
day call the runtime TypeError throw path used elsewhere.
- Around line 310-316: The Duration.compare implementation must be updated:
change the signature of Duration.compare to accept an optional third parameter
(options) carrying relativeTo, validate options.relativeTo (or its absence), and
if either input duration (D1 or D2) has nonzero years or months and no valid
relativeTo is provided, raise a RangeError; do not silently ignore years/months
in the TotalNs calculation—if relativeTo is provided and uses the ISO calendar,
convert years/months to days/nanoseconds using the calendar-aware conversion
logic before computing TotalNs1/TotalNs2 (weeks remain fixed as 7 days and do
not require relativeTo), and then compare the computed totals. Ensure the
RangeError logic and relativeTo handling are implemented in the Duration.compare
function that computes TotalNs1/TotalNs2 so comparisons of calendar-unit
durations follow the TC39 Temporal spec.
In `@source/units/Goccia.Values.TemporalZonedDateTime.pas`:
- Around line 1111-1151: Recompute and use the zone offset after rounding:
currently OffsetSeconds is obtained before calling RoundTimeForToString, so if
rounding changes the wall-clock instant across an offset boundary the formatted
string will show a stale offset. Move or repeat the call to GetUtcOffsetSeconds
after RoundTimeForToString (and after adjusting DateRec when ExtraDays != 0)
using Zdt.FTimeZone and the possibly adjusted epoch/instant (derived from the
rounded local fields or by computing the shifted epoch milliseconds
corresponding to DateRec and LHour/LMinute/etc.), then use that recomputed
OffsetSeconds in FormatOffsetString; ensure the new OffsetSeconds replaces the
original variable used when building S.
---
Nitpick comments:
In `@source/units/Goccia.Builtins.Temporal.pas`:
- Around line 243-247: Add plural property-name constants for Duration to
Goccia.Constants.PropertyNames (e.g., PROP_YEARS, PROP_MONTHS, PROP_WEEKS,
PROP_DAYS, PROP_HOURS, PROP_MINUTES, PROP_SECONDS, PROP_MILLISECONDS,
PROP_MICROSECONDS, PROP_NANOSECONDS) and update usages in duration construction
sites to use those constants instead of hard-coded strings; specifically replace
the string keys passed to GetFieldOr in TGocciaTemporalDurationValue.Create with
the new PROP_* constants and ensure the constants follow the same naming and
visibility pattern as existing PROP_YEAR/PROP_MONTH so other code can reuse
them.
- Around line 1096-1117: CoerceYearMonth currently only handles
TGocciaTemporalPlainYearMonthValue and string inputs; add object coercion like
other comparators by detecting TGocciaObjectValue in CoerceYearMonth, extracting
the "year" and "month" properties (accept number or string), convert them to
integers (or use existing CoerceToISOYearMonth helper/validation), and construct
TGocciaTemporalPlainYearMonthValue.Create(Y, M) on success; on bad/missing props
call ThrowRangeError(SErrorInvalidISOYearMonth, SSuggestTemporalISOFormat) or
ThrowTypeError(SErrorTemporalPlainYearMonthCompareArg,
SSuggestTemporalCompareArg) consistent with existing behavior.
🪄 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: 5376652e-4d07-4de6-8af0-4d96244e4608
📒 Files selected for processing (13)
scripts/test262_syntax_filter.pysource/units/Goccia.Builtins.Temporal.passource/units/Goccia.Temporal.Options.passource/units/Goccia.Temporal.TimeZone.passource/units/Goccia.Temporal.Utils.passource/units/Goccia.Values.TemporalDuration.passource/units/Goccia.Values.TemporalInstant.passource/units/Goccia.Values.TemporalPlainDate.passource/units/Goccia.Values.TemporalPlainDateTime.passource/units/Goccia.Values.TemporalPlainMonthDay.passource/units/Goccia.Values.TemporalPlainTime.passource/units/Goccia.Values.TemporalPlainYearMonth.passource/units/Goccia.Values.TemporalZonedDateTime.pas
- Fix misleading "mutually exclusive" comment in ResolveTemporalToStringOptions (smallestUnit takes precedence, not mutually exclusive) - Reject duplicate timezone annotations in TryParseISODateTimeWithOffset - Remove offsetless datetime fallback from CoerceToISOInstant (spec requires explicit Z or ±HH:MM offset for Instant coercion) - Guard withCalendar() against nil/undefined argument in PlainDate and PlainDateTime to prevent access violation - Include reference year in PlainMonthDay.toString() when calendar annotation is present (required for round-tripping per spec) - Include reference day in PlainYearMonth.toString() when calendar annotation is present (required for round-tripping per spec) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
coderabbitai
Bot
added
the
documentation
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
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.Temporal.Utils.pas (1)
989-1009:⚠️ Potential issue | 🟠 MajorRequire the UTC offset token to be fully consumed.
Once the hour/minute fields parse, any trailing bytes in the offset token are ignored.
2020-01-01T00:00+01:00junkcurrently succeeds instead of being rejected as an invalid Temporal string, violating the TC39 Temporal ISO8601 parsing specification.🐛 Minimal fix
else if ParsePos + 1 <= Length(OffsetPart) then begin if not TryParseDigits(OffsetPart, ParsePos, 2, OffsetM) then Exit; end; + if ParsePos <= Length(OffsetPart) then Exit; // Validate ranges if (OffsetH > 23) or (OffsetM > 59) then Exit;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/units/Goccia.Temporal.Utils.pas` around lines 989 - 1009, The offset parsing currently allows trailing characters because ParsePos isn't checked after parsing hours/minutes; inside the block that handles OffsetPart (use symbols OffsetPart, ParsePos, TryParseDigits, OffsetH, OffsetM, OffsetSign, AOffsetSeconds, ATimeZone), after validating ranges and before setting AOffsetSeconds/ATimeZone add a check that ParsePos > Length(OffsetPart) (or equivalently that ParsePos is past the last character) and Exit if there are leftover characters so tokens like " +01:00junk " are rejected; keep the existing range checks and assignments otherwise.
♻️ Duplicate comments (1)
source/units/Goccia.Temporal.Utils.pas (1)
1108-1116:⚠️ Potential issue | 🟠 Major
CoerceToISOInstantstill accepts offsetless date-times.The old fallback block is gone, but
TryParseISODateTimeWithOffsetitself still succeeds onYYYY-MM-DDTHH:mmwithoutZ/±HH:MM, leaving this path to coerce an invalid Instant string as UTC.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/units/Goccia.Temporal.Utils.pas` around lines 1108 - 1116, CoerceToISOInstant currently accepts offsetless strings because TryParseISODateTimeWithOffset still succeeds on them; update CoerceToISOInstant to enforce an explicit UTC offset by checking the input AStr for a timezone designator before calling or accepting TryParseISODateTimeWithOffset (e.g. require trailing 'Z' or a '+'/'-' offset after the time portion), and if none is present set Result := False and Exit; alternatively, modify TryParseISODateTimeWithOffset to return a flag indicating whether an offset was present and make CoerceToISOInstant reject parses with no offset; reference AStr, StripAnnotations, TryParseISODateTimeWithOffset and CoerceToISOInstant when applying the change.
🤖 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.Temporal.Utils.pas`:
- Around line 449-452: In both StripAnnotations and
TryParseISODateTimeWithOffset, after extracting AnnotContent and stripping the
critical '!' prefix (variables AnnotContent and IsCritical), add a check that
rejects empty annotation content (Length(AnnotContent) = 0) so that inputs like
"2020-01-01[]" or "2020-01-01T00:00[!]" are treated as invalid; implement this
by returning failure/false from TryParseISODateTimeWithOffset and by not
treating the annotation as a timezone-annotation in StripAnnotations (or
otherwise signalling an invalid annotation).
In `@source/units/Goccia.Values.TemporalPlainDate.pas`:
- Around line 529-533: The toString implementation currently treats any
non-object argument as undefined; before calling GetCalendarDisplay you must
validate the first argument (Arg := AArgs.GetElement(0)): if an argument is
present and is not TGocciaObjectValue and is not an explicit undefined, raise a
TypeError (use the existing SSuggestTemporalFromArg message or equivalent)
instead of silently treating it as nil; only set OptionsObj when Arg is an
object and then call GetCalendarDisplay(OptionsObj).
In `@source/units/Goccia.Values.TemporalPlainDateTime.pas`:
- Around line 749-752: Replace the current loose type-check with a ToObject
coercion: after fetching Arg := AArgs.GetElement(0), call the value's ToObject
coercion and assign/convert that result to OptionsObj (e.g. Arg := Arg.ToObject
or OptionsObj := TGocciaObjectValue(Arg.ToObject)); this will cause primitives
to raise the required TypeError per TC39 Temporal and ensure only proper objects
reach the TGocciaObjectValue cast used by the toString('auto') path. Update the
same pattern wherever similar OptionsObj/Arg handling occurs in other Temporal
types.
---
Outside diff comments:
In `@source/units/Goccia.Temporal.Utils.pas`:
- Around line 989-1009: The offset parsing currently allows trailing characters
because ParsePos isn't checked after parsing hours/minutes; inside the block
that handles OffsetPart (use symbols OffsetPart, ParsePos, TryParseDigits,
OffsetH, OffsetM, OffsetSign, AOffsetSeconds, ATimeZone), after validating
ranges and before setting AOffsetSeconds/ATimeZone add a check that ParsePos >
Length(OffsetPart) (or equivalently that ParsePos is past the last character)
and Exit if there are leftover characters so tokens like " +01:00junk " are
rejected; keep the existing range checks and assignments otherwise.
---
Duplicate comments:
In `@source/units/Goccia.Temporal.Utils.pas`:
- Around line 1108-1116: CoerceToISOInstant currently accepts offsetless strings
because TryParseISODateTimeWithOffset still succeeds on them; update
CoerceToISOInstant to enforce an explicit UTC offset by checking the input AStr
for a timezone designator before calling or accepting
TryParseISODateTimeWithOffset (e.g. require trailing 'Z' or a '+'/'-' offset
after the time portion), and if none is present set Result := False and Exit;
alternatively, modify TryParseISODateTimeWithOffset to return a flag indicating
whether an offset was present and make CoerceToISOInstant reject parses with no
offset; reference AStr, StripAnnotations, TryParseISODateTimeWithOffset and
CoerceToISOInstant when applying the change.
🪄 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: bf796e17-17c6-43df-893e-d871d8ee62d5
📒 Files selected for processing (6)
source/units/Goccia.Temporal.Options.passource/units/Goccia.Temporal.Utils.passource/units/Goccia.Values.TemporalPlainDate.passource/units/Goccia.Values.TemporalPlainDateTime.passource/units/Goccia.Values.TemporalPlainMonthDay.passource/units/Goccia.Values.TemporalPlainYearMonth.pas
✅ Files skipped from review due to trivial changes (1)
- source/units/Goccia.Values.TemporalPlainYearMonth.pas
🚧 Files skipped from review as they are similar to previous changes (2)
- source/units/Goccia.Values.TemporalPlainMonthDay.pas
- source/units/Goccia.Temporal.Options.pas
- Reject empty bracket annotations ([] and [!]) in both StripAnnotations and TryParseISODateTimeWithOffset - Throw TypeError when toString options is a primitive (not object or undefined) across all 7 Temporal types with toString options Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
source/units/Goccia.Values.TemporalPlainTime.pas (1)
502-550: Please add spec annotations above the updated PlainTime stringification methods.
TimeToStringnow has a new option-resolution/rounding flow, andPlainTimeToLocaleStringis a new Temporal entry point, but neither changed body has the required spec-section comment immediately above it.Based on learnings, “Annotate each function implementing ECMAScript-specified behavior with
// ESYYYY §X.Y.Z SpecMethodName(specParams)immediately above the function body.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/units/Goccia.Values.TemporalPlainTime.pas` around lines 502 - 550, Add the required ECMAScript spec annotations as single-line comments immediately above each updated Temporal PlainTime method: place lines like "// ES2023 §X.Y.Z TimeToString(this, options)" above function TimeToString, "// ES2023 §X.Y.Z toJSON" above TimeToJSON, "// ES2023 §X.Y.Z valueOf" above TimeValueOf, and "// ES2023 §X.Y.Z PlainTimeToLocaleString(this, locales, options)" above PlainTimeToLocaleString (use the actual spec section numbers if known); ensure the comment format matches the project's convention exactly and is placed immediately above each function header.source/units/Goccia.Values.TemporalPlainDate.pas (1)
521-675: Add spec-section comments for the changed Temporal methods.The modified
DateToStringimplementation and the newDateToLocaleString/DateWithCalendarbodies are ECMAScript-defined entry points, but this changed surface still has no spec-section comments above the functions. Please add them while touching these methods.Based on learnings, “Annotate each function implementing ECMAScript-specified behavior with
// ESYYYY §X.Y.Z SpecMethodName(specParams)immediately above the function body.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/units/Goccia.Values.TemporalPlainDate.pas` around lines 521 - 675, Add ES spec-section comments immediately above each ECMAScript entry-point function added/modified: DateToString, DateToLocaleString, DateWithCalendar (and any other Temporal methods in this diff such as DateToJSON, DateValueOf, DateToPlainDateTime, DateToPlainYearMonth, DateToPlainMonthDay, DateToZonedDateTime if applicable). For each function insert a single-line comment in the form "// ESYYYY §X.Y.Z SpecMethodName(specParams)" (use the correct year/section and the exact spec method name like PlainDate.prototype.toString, PlainDate.prototype.toLocaleString, PlainDate.prototype.withCalendar) directly above the function declaration (e.g. above "function TGocciaTemporalPlainDateValue.DateToString(...)" etc.). Ensure formatting matches existing project convention and update only the comment lines.source/units/Goccia.Values.TemporalPlainYearMonth.pas (1)
474-543: Keep the Temporal spec annotations in sync with the new method surface.Line 475 now implements an options-taking
toString, but the adjacent spec comment still documentstoString(), and the newYearMonthToLocaleStringbody has no spec-section comment at all. Please add/update the annotations here so the changed Temporal entry points keep the required spec traceability.Based on learnings, “Annotate each function implementing ECMAScript-specified behavior with
// ESYYYY §X.Y.Z SpecMethodName(specParams)immediately above the function body.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/units/Goccia.Values.TemporalPlainYearMonth.pas` around lines 474 - 543, Update the spec-comment annotations above the Temporal PlainYearMonth methods to the required ES-style lines: replace the TC39 comments for YearMonthToString, YearMonthToJSON, YearMonthValueOf, YearMonthToPlainDate with lines of the form "// ESYYYY §X.Y.Z YearMonthToString(options)" (and the correct spec method names/params) immediately above each corresponding function (YearMonthToString, YearMonthToJSON, YearMonthValueOf, YearMonthToPlainDate) and add a new spec annotation line for YearMonthToLocaleString immediately above its function; ensure the annotation text matches the new method signatures (e.g., include "options" for toString and the correct SpecMethodName for toLocaleString) so each implementation has an ESYYYY-style spec traceability comment.source/units/Goccia.Values.TemporalPlainDateTime.pas (1)
875-890: LGTM with minor suggestion: consider using error message constants.The implementation is correct—the nil/undefined guard is in place, and rejecting non-
iso8601calendars matches the engine's ISO-only calendar support.The hardcoded error messages on lines 884 and 887 could be moved to
Goccia.Error.Messagesfor consistency with other Temporal units, but this is low priority since the messages are clear and the method works correctly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/units/Goccia.Values.TemporalPlainDateTime.pas` around lines 875 - 890, Move the two hardcoded error strings in TGocciaTemporalPlainDateTimeValue.DateTimeWithCalendar into message constants in Goccia.Error.Messages and use those constants in the ThrowTypeError and ThrowRangeError calls; specifically, add descriptive constant identifiers (e.g., PlainDateTimeWithCalendar_MissingCalendar and PlainDateTimeWithCalendar_UnknownCalendar) to Goccia.Error.Messages, replace the literal 'PlainDateTime.prototype.withCalendar requires a calendar argument' and 'Unknown calendar: ' + CalId with those constants (concatenating CalId to the unknown-calendar constant if needed), and leave the existing SSuggestTemporalFromArg parameter and ThrowTypeError/ThrowRangeError call sites unchanged.
🤖 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.TemporalInstant.pas`:
- Around line 525-528: TGocciaTemporalInstantValue.InstantToLocaleString
currently forwards the incoming AArgs (which may contain locales/options) into
InstantToString; instead, implement an adapter that discards locales/options and
calls the normal toString path. Change InstantToLocaleString to ignore/clear the
TGocciaArgumentsCollection parameters (do not forward locales/options) and call
InstantToString in its no-locale form (i.e., pass an empty/default
TGocciaArgumentsCollection or the overload that only uses AThisValue), and apply
the same pattern to the other Temporal type methods (PlainDate, PlainDateTime,
PlainTime, YearMonth, MonthDay, ZonedDateTime) so toLocaleString always returns
toString() without using locales/options.
- Around line 530-549: In InstantToZonedDateTimeISO, accept a
TGocciaTemporalZonedDateTimeValue in addition to TGocciaStringLiteralValue: when
Arg is TGocciaStringLiteralValue use its Value as TZ, and when Arg is
TGocciaTemporalZonedDateTimeValue extract the time zone via its TimeZone
property and assign to TZ; otherwise keep throwing the existing type error
(ThrowTypeError(..., SSuggestTemporalTimezone)); then construct the
TGocciaTemporalZonedDateTimeValue result using Inst.FEpochMilliseconds,
Inst.FSubMillisecondNanoseconds and the extracted TZ.
In `@source/units/Goccia.Values.TemporalPlainDate.pas`:
- Around line 661-673: In TGocciaTemporalPlainDateValue.DateWithCalendar, Arg is
coerced via Arg.ToStringLiteral.Value without rejecting Symbol values; add a
pre-check for TGocciaSymbolValue and call ThrowTypeError('Cannot convert symbol
to string', SSuggestTemporalFromArg) if Arg is a symbol before performing the
ToStringLiteral conversion so Symbol inputs produce a TypeError instead of being
stringified and causing a RangeError.
In `@source/units/Goccia.Values.TemporalPlainMonthDay.pas`:
- Around line 356-359: The MonthDayToLocaleString implementation incorrectly
forwards AArgs to MonthDayToString causing locale string to be treated as
options; change TGocciaTemporalPlainMonthDayValue.MonthDayToLocaleString to call
MonthDayToString with an empty TGocciaArgumentsCollection (or strip out the
locales/options parameters) so that the fallback toString ignores both locale
and options. Apply the same fix pattern to the equivalent methods on
PlainMonthDay, PlainDate, PlainDateTime, PlainTime, PlainYearMonth,
ZonedDateTime, Instant and Duration types (use the corresponding ToLocaleString
methods and their ToString helpers like MonthDayToString) so locales/options are
discarded in non-Intl fallback.
In `@source/units/Goccia.Values.TemporalZonedDateTime.pas`:
- Around line 1116-1155: The code computes OffsetSeconds before calling
RoundTimeForToString, causing the final formatted string to use a stale offset
after rounding; after RoundTimeForToString (which may set ExtraDays) compute a
new rounded instant by adding ExtraDays * MILLISECONDS_PER_DAY to
Zdt.FEpochMilliseconds, then call ComputeLocalComponents on that rounded instant
and recompute OffsetSeconds via GetUtcOffsetSeconds(using roundedEpoch div
MILLISECONDS_PER_SECOND) before building TimeStr and S so both the local fields
and the offset reflect the rounded instant (update references around
OffsetSeconds, RoundTimeForToString, ComputeLocalComponents and
Zdt.FEpochMilliseconds).
- Around line 1184-1187: The
TGocciaTemporalZonedDateTimeValue.ZonedDateTimeToLocaleString implementation is
incorrectly forwarding AArgs to ZonedDateTimeToString (treating the first
locales param as options); change it to discard both locales and options by
creating an empty TGocciaArgumentsCollection and pass that to
ZonedDateTimeToString so the non-Intl fallback returns the same value as
toString() while ignoring parameters; ensure you reference ZonedDateTimeToString
and use a new/empty local arguments collection instead of AArgs inside
ZonedDateTimeToLocaleString.
---
Nitpick comments:
In `@source/units/Goccia.Values.TemporalPlainDate.pas`:
- Around line 521-675: Add ES spec-section comments immediately above each
ECMAScript entry-point function added/modified: DateToString,
DateToLocaleString, DateWithCalendar (and any other Temporal methods in this
diff such as DateToJSON, DateValueOf, DateToPlainDateTime, DateToPlainYearMonth,
DateToPlainMonthDay, DateToZonedDateTime if applicable). For each function
insert a single-line comment in the form "// ESYYYY §X.Y.Z
SpecMethodName(specParams)" (use the correct year/section and the exact spec
method name like PlainDate.prototype.toString,
PlainDate.prototype.toLocaleString, PlainDate.prototype.withCalendar) directly
above the function declaration (e.g. above "function
TGocciaTemporalPlainDateValue.DateToString(...)" etc.). Ensure formatting
matches existing project convention and update only the comment lines.
In `@source/units/Goccia.Values.TemporalPlainDateTime.pas`:
- Around line 875-890: Move the two hardcoded error strings in
TGocciaTemporalPlainDateTimeValue.DateTimeWithCalendar into message constants in
Goccia.Error.Messages and use those constants in the ThrowTypeError and
ThrowRangeError calls; specifically, add descriptive constant identifiers (e.g.,
PlainDateTimeWithCalendar_MissingCalendar and
PlainDateTimeWithCalendar_UnknownCalendar) to Goccia.Error.Messages, replace the
literal 'PlainDateTime.prototype.withCalendar requires a calendar argument' and
'Unknown calendar: ' + CalId with those constants (concatenating CalId to the
unknown-calendar constant if needed), and leave the existing
SSuggestTemporalFromArg parameter and ThrowTypeError/ThrowRangeError call sites
unchanged.
In `@source/units/Goccia.Values.TemporalPlainTime.pas`:
- Around line 502-550: Add the required ECMAScript spec annotations as
single-line comments immediately above each updated Temporal PlainTime method:
place lines like "// ES2023 §X.Y.Z TimeToString(this, options)" above function
TimeToString, "// ES2023 §X.Y.Z toJSON" above TimeToJSON, "// ES2023 §X.Y.Z
valueOf" above TimeValueOf, and "// ES2023 §X.Y.Z PlainTimeToLocaleString(this,
locales, options)" above PlainTimeToLocaleString (use the actual spec section
numbers if known); ensure the comment format matches the project's convention
exactly and is placed immediately above each function header.
In `@source/units/Goccia.Values.TemporalPlainYearMonth.pas`:
- Around line 474-543: Update the spec-comment annotations above the Temporal
PlainYearMonth methods to the required ES-style lines: replace the TC39 comments
for YearMonthToString, YearMonthToJSON, YearMonthValueOf, YearMonthToPlainDate
with lines of the form "// ESYYYY §X.Y.Z YearMonthToString(options)" (and the
correct spec method names/params) immediately above each corresponding function
(YearMonthToString, YearMonthToJSON, YearMonthValueOf, YearMonthToPlainDate) and
add a new spec annotation line for YearMonthToLocaleString immediately above its
function; ensure the annotation text matches the new method signatures (e.g.,
include "options" for toString and the correct SpecMethodName for
toLocaleString) so each implementation has an ESYYYY-style spec traceability
comment.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 37fcab77-419b-47c2-865f-de3b6497c55b
📒 Files selected for processing (8)
source/units/Goccia.Temporal.Utils.passource/units/Goccia.Values.TemporalInstant.passource/units/Goccia.Values.TemporalPlainDate.passource/units/Goccia.Values.TemporalPlainDateTime.passource/units/Goccia.Values.TemporalPlainMonthDay.passource/units/Goccia.Values.TemporalPlainTime.passource/units/Goccia.Values.TemporalPlainYearMonth.passource/units/Goccia.Values.TemporalZonedDateTime.pas
🚧 Files skipped from review as they are similar to previous changes (1)
- source/units/Goccia.Temporal.Utils.pas
Comment on lines
+1116
to
+1155
| Arg: TGocciaValue; | ||
| OptionsObj: TGocciaObjectValue; | ||
| FracDigits, ExtraDays: Integer; | ||
| Mode: TTemporalRoundingMode; | ||
| CalDisp: TTemporalCalendarDisplay; | ||
| DateRec: TTemporalDateRecord; | ||
| TimeStr, S: string; | ||
| begin | ||
| Zdt := AsZonedDateTime(AThisValue, 'ZonedDateTime.prototype.toString'); | ||
| ComputeLocalComponents(Zdt, LYear, LMonth, LDay, LHour, LMinute, LSecond, LMs, LUs, LNs); | ||
| OffsetSeconds := GetUtcOffsetSeconds(Zdt.FTimeZone, Zdt.FEpochMilliseconds div MILLISECONDS_PER_SECOND); | ||
|
|
||
| S := FormatDateString(LYear, LMonth, LDay) + 'T' + | ||
| FormatTimeString(LHour, LMinute, LSecond, LMs, LUs, LNs) + | ||
| FormatOffsetString(OffsetSeconds) + | ||
| '[' + Zdt.FTimeZone + ']'; | ||
| OptionsObj := nil; | ||
| Arg := AArgs.GetElement(0); | ||
| if Assigned(Arg) and not (Arg is TGocciaUndefinedLiteralValue) then | ||
| begin | ||
| if not (Arg is TGocciaObjectValue) then | ||
| ThrowTypeError('options must be an object or undefined', SSuggestTemporalFromArg); | ||
| OptionsObj := TGocciaObjectValue(Arg); | ||
| end; | ||
| ResolveTemporalToStringOptions(OptionsObj, FracDigits, Mode); | ||
| CalDisp := GetCalendarDisplay(OptionsObj); | ||
| ExtraDays := 0; | ||
| RoundTimeForToString(LHour, LMinute, LSecond, LMs, LUs, LNs, ExtraDays, FracDigits, Mode); | ||
| if ExtraDays <> 0 then | ||
| DateRec := AddDaysToDate(LYear, LMonth, LDay, ExtraDays) | ||
| else | ||
| begin | ||
| DateRec.Year := LYear; | ||
| DateRec.Month := LMonth; | ||
| DateRec.Day := LDay; | ||
| end; | ||
| if FracDigits = -2 then // smallestUnit: minute | ||
| TimeStr := PadTwo(LHour) + ':' + PadTwo(LMinute) | ||
| else | ||
| TimeStr := FormatTimeWithPrecision(LHour, LMinute, LSecond, LMs, LUs, LNs, FracDigits); | ||
|
|
||
| S := FormatDateString(DateRec.Year, DateRec.Month, DateRec.Day) + 'T' + | ||
| TimeStr + FormatOffsetString(OffsetSeconds) + | ||
| '[' + Zdt.FTimeZone + ']' + FormatCalendarAnnotation(CalDisp); |
Contributor
There was a problem hiding this comment.
Recompute the offset after rounding.
OffsetSeconds is captured before RoundTimeForToString, but the rounded wall time can carry into another date or across a zone transition. The final string then combines the rounded local date/time with the old offset, which can emit impossible combinations around DST boundaries. Build the rounded instant first, then derive both the local fields and the offset from that rounded instant before formatting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/units/Goccia.Values.TemporalZonedDateTime.pas` around lines 1116 -
1155, The code computes OffsetSeconds before calling RoundTimeForToString,
causing the final formatted string to use a stale offset after rounding; after
RoundTimeForToString (which may set ExtraDays) compute a new rounded instant by
adding ExtraDays * MILLISECONDS_PER_DAY to Zdt.FEpochMilliseconds, then call
ComputeLocalComponents on that rounded instant and recompute OffsetSeconds via
GetUtcOffsetSeconds(using roundedEpoch div MILLISECONDS_PER_SECOND) before
building TimeStr and S so both the local fields and the offset reflect the
rounded instant (update references around OffsetSeconds, RoundTimeForToString,
ComputeLocalComponents and Zdt.FEpochMilliseconds).
Owner
Author
There was a problem hiding this comment.
Acknowledged — the offset recomputation after rounding is a valid concern for DST boundaries. Currently our timezone implementation uses a fixed offset per-call, but this should be revisited when we add full DST transition support. Tracked for follow-up.
Contributor
There was a problem hiding this comment.
@frostney, understood — thanks for the context. The stale-offset issue is a real DST edge case, but if the current implementation uses a fixed offset per-call, the risk is limited to transition zones and won't affect the majority of test262 cases passing today. Deferring this alongside full DST transition support makes sense.
Would you like me to open a follow-up issue to track the offset recomputation work so it doesn't get lost?
✏️ Learnings added
Learnt from: frostney
Repo: frostney/GocciaScript PR: 354
File: source/units/Goccia.Values.TemporalZonedDateTime.pas:1116-1155
Timestamp: 2026-04-19T18:53:34.108Z
Learning: In `source/units/Goccia.Values.TemporalZonedDateTime.pas` (frostney/GocciaScript, PR `#354`), `ZonedDateTimeToString` computes `OffsetSeconds` via `GetUtcOffsetSeconds` **before** calling `RoundTimeForToString`. After rounding, `ExtraDays` may push the instant across a DST boundary, making the captured offset stale. The fix (recompute `OffsetSeconds` from the rounded epoch after applying `ExtraDays`) is intentionally deferred to the follow-up PR that adds full DST transition support. Do not re-flag the pre-rounding offset capture in `ZonedDateTimeToString` until that DST PR lands.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: frostney
Repo: frostney/GocciaScript PR: 346
File: source/units/Goccia.Values.TemporalZonedDateTime.pas:622-628
Timestamp: 2026-04-18T22:41:49.426Z
Learning: In `source/units/Goccia.Values.TemporalZonedDateTime.pas` (frostney/GocciaScript), `ZonedDateTimeUntil` and `ZonedDateTimeSince` were fixed in commit 70a89a69 (PR `#346`). They no longer collapse the delta into a total `DiffNs: Int64`. Instead they use the same two-field `{DiffMs: Int64, DiffSubMs: Integer}` decomposition as `Instant`: normalize sign between the two fields, then decompose `DiffMs` into hours/minutes/seconds/milliseconds and `DiffSubMs` into microseconds/nanoseconds directly. Do not flag `Int64` overflow risk in these methods — the total-nanosecond collapse has been removed.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 308
File: units/Goccia.Values.TemporalPlainMonthDay.pas:0-0
Timestamp: 2026-04-15T20:56:34.721Z
Learning: In frostney/GocciaScript Temporal value units (e.g., units/Goccia.Values.TemporalPlainMonthDay.pas), use `SSuggestTemporalThisType` when throwing a TypeError because `AThisValue` (or the first arg) is not the expected Temporal type (wrong-receiver / wrong-type-for-helper checks). Use `SSuggestTemporalToPlainDateYear` when throwing because a `toPlainDate` call is missing or receives a non-object/undefined `year` property. Do NOT use the broader `SSuggestTemporalFromArg` for these two specific cases.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 324
File: units/Goccia.Values.TemporalDuration.pas:630-656
Timestamp: 2026-04-17T14:39:13.057Z
Learning: In frostney/GocciaScript, `ParseRelativeTo` in `units/Goccia.Values.TemporalDuration.pas` intentionally supports only `TGocciaTemporalPlainDateValue` and ISO date/date-time string inputs for `relativeTo` in `Duration.prototype.total()`. Support for `TGocciaTemporalZonedDateTimeValue` and property-bag/object inputs is deferred to a dedicated follow-up PR, as it requires importing the ZonedDateTime unit, extracting the plain date, and handling DST-aware day lengths. Do not re-flag missing ZonedDateTime/property-bag handling in `ParseRelativeTo` until that follow-up PR lands.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 308
File: units/Goccia.Values.TemporalPlainDateTime.pas:144-145
Timestamp: 2026-04-16T06:33:07.065Z
Learning: In frostney/GocciaScript Temporal unit implementations (e.g., units/Goccia.Values.TemporalPlainDateTime.pas, units/Goccia.Values.TemporalPlainDate.pas, units/Goccia.Values.TemporalPlainTime.pas, units/Goccia.Values.TemporalPlainYearMonth.pas, units/Goccia.Values.TemporalPlainMonthDay.pas, units/Goccia.Values.TemporalInstant.pas, units/Goccia.Values.TemporalZonedDateTime.pas, units/Goccia.Builtins.Temporal.pas), the correction of ThrowTypeError → ThrowRangeError for invalid ISO/time/duration string parsing failures (per TC39 Temporal spec) is intentionally deferred to a dedicated follow-up PR. PRs that only centralize error message constants and add suggestion metadata must not change error types (TypeError vs RangeError). Do not re-flag the ThrowTypeError usage for ISO/time/duration parse failures in these Temporal units until the dedicated behavioral-change PR lands.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 324
File: units/Goccia.Values.TemporalDuration.pas:891-920
Timestamp: 2026-04-17T14:39:20.659Z
Learning: In frostney/GocciaScript (units/Goccia.Values.TemporalDuration.pas), the engine supports only the ISO 8601 calendar. Weeks are always exactly 7 days in this calendar, so Duration.prototype.total does NOT require `relativeTo` for the 'weeks' unit or for durations with non-zero FWeeks components. The fixed `weeks * 7 * NANOSECONDS_PER_DAY` calculation is intentional and correct. Do not flag missing relativeTo guards for week units/components in this file. If/when non-ISO calendar support is added, this guard should be revisited.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 346
File: source/units/Goccia.Values.TemporalInstant.pas:178-184
Timestamp: 2026-04-18T22:29:31.789Z
Learning: In `source/units/Goccia.Values.TemporalInstant.pas` (frostney/GocciaScript), the `GetEpochNanoseconds` getter is safe from overflow. `FEpochMilliseconds` is `Int64` (max ≈ ±9.2×10¹⁸ ms) and the valid Temporal instant range is ±8.64×10¹⁵ ms, so `TBigInteger.FromInt64(Inst.FEpochMilliseconds)` is always lossless. Do NOT flag `TBigInteger.FromInt64` on `FEpochMilliseconds` in this getter as a potential overflow.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 327
File: units/Goccia.Values.TemporalDuration.pas:541-561
Timestamp: 2026-04-17T16:31:38.698Z
Learning: In frostney/GocciaScript (units/Goccia.Values.TemporalDuration.pas), `DurationRound` intentionally parses and validates the `relativeTo` option eagerly (before `NeedCalendar` is computed) because the TC39 Temporal spec calls `ToRelativeTemporalObject` as an early step of `Duration.prototype.round` — before any check for whether calendar math is needed. An invalid `relativeTo` must throw even for a pure time/day round. Do NOT flag the eager parsing in `DurationRound` as incorrect or suggest deferring it until `NeedCalendar` is known. If `DurationTotal` uses lazy parsing, that is the less spec-compliant path, not `DurationRound`.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 277
File: units/Goccia.Builtins.Temporal.pas:0-0
Timestamp: 2026-04-12T13:19:32.780Z
Learning: For all Temporal-related units in GocciaScript (e.g., files like Goccia.Builtins.Temporal.pas and Goccia.Values.TemporalPlainTime.pas), use the shared Temporal property-name constants defined in units/Goccia.Constants.PropertyNames.pas (PROP_YEAR, PROP_MONTH, PROP_MONTH_CODE, PROP_DAY, PROP_CALENDAR_ID, plus a full set of time unit constants: PROP_HOUR, PROP_MINUTE, PROP_SECOND, PROP_MILLISECOND, PROP_MICROSECOND, PROP_NANOSECOND) when accessing Temporal properties and when constructing any related error messages. Do not use hardcoded string literals for these Temporal property names in Temporal units.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 324
File: units/Goccia.Values.TemporalDuration.pas:691-693
Timestamp: 2026-04-17T14:59:54.438Z
Learning: In frostney/GocciaScript (units/Goccia.Values.TemporalDuration.pas), TC39 Temporal Duration magnitude validation (calendar units < 2^32, normalized seconds < 2^53) is intentionally deferred to a follow-up PR. The validation belongs in the `TGocciaTemporalDurationValue` constructor and `DurationFromObject`, not in `DurationEndDate`, `AddMonthsToDate`, or `DurationTotal`. Do not re-flag missing magnitude validation in `DurationEndDate` or `total()` path — the arithmetic there is safe for spec-compliant inputs; the gap is upstream validation not yet implemented.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 327
File: units/Goccia.Values.TemporalDuration.pas:667-671
Timestamp: 2026-04-17T16:42:55.749Z
Learning: In `units/Goccia.Values.TemporalDuration.pas` (`DurationRound`, `tuYear`/`tuMonth` calendar-rounding path), when aligning a fractional `WholeUnits` count to an `Increment`-sized bucket for **negative** durations, use `WholeUnits - 1` as the floor-division base instead of `WholeUnits`. For negative durations the walk stops at `WholeUnits` (e.g. -2) but the actual fractional position is further negative (e.g. -3), so `floor(-2 / Increment) * Increment` selects the wrong (too-high) bucket. Subtracting 1 before the floor division (i.e. `floor((WholeUnits - 1) / Increment) * Increment`) selects the bucket whose interval actually contains the fractional position. Do not re-flag this alignment logic and do not suggest adjusting `RoundWithMode` inputs as a sign-preservation fix — the correct fix is the bucket-base adjustment. Regression test: `-P3M` from 2021-07-31 with `roundingIncrement: 2, roundingMode: "halfExpand"` must round to `-P4M`.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 337
File: source/units/Goccia.Builtins.TSV.pas:315-319
Timestamp: 2026-04-18T13:58:01.524Z
Learning: In `source/units/Goccia.Builtins.TSV.pas` (and the parallel `source/units/Goccia.Builtins.CSV.pas`), `startOffset`/`endOffset` numeric arguments passed to `parseChunk` are intentionally truncated with `Trunc()` without a prior NaN/Infinity guard. The resulting integer (however large or negative) is immediately passed to `ClampOffset` in `TGocciaTSVParser`/`TGocciaCSVParser`, which clamps negative values to 0 and values beyond `TextLength` to `TextLength`. This makes a separate finite-number validation unnecessary. Do not re-flag missing NaN/Infinity guards on `Trunc()` calls for offset arguments in these `parseChunk` methods.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 351
File: source/units/Goccia.Engine.pas:1388-1402
Timestamp: 2026-04-19T03:29:53.111Z
Learning: In frostney/GocciaScript (PR `#351`), `Goccia.gc.bytesAllocated` (implemented as `TGocciaEngine.GocciaGCBytesAllocatedGetter` in `source/units/Goccia.Engine.pas`) returns a freshly allocated `TGocciaNumberLiteralValue`, which itself goes through `AfterConstruction` → `RegisterObject` → `Inc(FBytesAllocated, InstanceSize)`. This means each read of `bytesAllocated` bumps the counter by ~48 bytes (one `TGocciaNumberLiteralValue` InstanceSize). This is a known, accepted limitation: (1) there is no untracked/unregistered allocation path in the codebase, (2) the metric is explicitly documented as "approximate", and (3) the ~48-byte per-read noise is negligible against any realistic heap size. Do NOT re-flag this self-inflation as a correctness or accuracy issue until a broader architectural decision is made about whether to add an untracked value path.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 277
File: units/Goccia.Values.TemporalPlainMonthDay.pas:0-0
Timestamp: 2026-04-12T10:23:58.234Z
Learning: In frostney/GocciaScript, Temporal-specific property name constants `PROP_YEAR`, `PROP_MONTH`, `PROP_MONTH_CODE`, `PROP_DAY`, and `PROP_CALENDAR_ID` were added to `Goccia.Constants.PropertyNames` (as of commit d59dd03, PR `#277`). All Temporal value units (e.g. `Goccia.Values.TemporalPlainMonthDay.pas`, `Goccia.Values.TemporalPlainYearMonth.pas`, etc.) must use these constants instead of hardcoded string literals for property accesses (`GetProperty`, `SetProperty`, accessor registration, `ThrowTypeError` messages referencing property names, etc.).
Learnt from: frostney
Repo: frostney/GocciaScript PR: 63
File: units/Goccia.Runtime.Operations.pas:0-0
Timestamp: 2026-03-11T01:50:11.071Z
Learning: Replace hardcoded string literals 'now', 'toJSON', and 'timeOrigin' with the new constants from Goccia.Constants.PropertyNames (PROP_NOW, PROP_TO_JSON, PROP_TIME_ORIGIN). Apply this refactor across the repository wherever these literals are used, including but not limited to units/Goccia.Runtime.Operations.pas and units/Goccia.Builtins.Performance.pas. Use a project-wide search (e.g., regex or code navigation) to identify all occurrences, update to the respective constants, and adjust imports or uses accordingly. Ensure all references compile and behavior remains unchanged.
Learnt from: CR
Repo: frostney/GocciaScript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-12T15:16:53.061Z
Learning: Applies to {**/Goccia.Values.*.pas,**/Goccia.Evaluator.pas,**/Goccia.Runtime.*.pas} : Annotate each function implementing ECMAScript-specified behavior with `// ESYYYY §X.Y.Z SpecMethodName(specParams)` immediately above the function body, using the spec's pseudo-code method name and parameters exactly as written
Learnt from: frostney
Repo: frostney/GocciaScript PR: 343
File: source/units/Goccia.Evaluator.Arithmetic.pas:49-55
Timestamp: 2026-04-18T21:08:03.166Z
Learning: In frostney/GocciaScript (commit 3344dede, PR `#343`), `TGocciaBigIntObjectValue` was added to support BigInt boxing via `Object(1n)`. `Box` in `ClassHelper.pas` now returns a `TGocciaBigIntObjectValue` for `TGocciaBigIntValue` primitives. `ToPrimitive` calls were added to `EvaluateSubtraction`, `EvaluateMultiplication`, `EvaluateDivision`, `EvaluateModulo`, and `EvaluateExponentiation` in `source/units/Goccia.Evaluator.Arithmetic.pas` so that boxed BigInts (e.g. `Object(1n) - 1n`) are correctly unboxed via `valueOf()` before BigInt type checks. `Object()` was also fixed to call `Box` for primitive arguments per ES2026 §20.1.1.1. Future reviews of arithmetic operators SHOULD expect `ToPrimitive` before BigInt checks.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 343
File: source/units/Goccia.Builtins.GlobalBigInt.pas:213-223
Timestamp: 2026-04-18T20:48:02.918Z
Learning: In `source/units/Goccia.Builtins.GlobalBigInt.pas` (frostney/GocciaScript, landed in commit 477b5609 on PR `#343`), `BigInt.asIntN` and `BigInt.asUintN` use proper `ToIndex` abstract-op coercion for the `bits` parameter: `undefined`/`NaN` → 0, `Infinity`/negative/oversized → `RangeError`. This is fully implemented and tested. Do not flag missing `ToIndex` coercion for these methods.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 343
File: source/units/Goccia.JSON.pas:841-844
Timestamp: 2026-04-18T20:08:00.723Z
Learning: In frostney/GocciaScript (source/units/Goccia.JSON.pas), `ApplyToJSON` intentionally only probes `toJSON` when the value is a `TGocciaObjectValue`. For BigInt primitives (`TGocciaBigIntValue`), `JSON.stringify(1n)` correctly throws TypeError per ES2026 §25.5.2.5 step 10 — this is the expected default behavior. Extending `ApplyToJSON` to call `GetProperty('toJSON')` directly on BigInt primitives (per ES2026 §25.5.2.2) is a known gap deferred to a follow-up PR. Do not re-flag the missing `toJSON` probe for BigInt in `ApplyToJSON` until that follow-up is scoped.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 346
File: source/shared/BigInteger.Test.pas:0-0
Timestamp: 2026-04-18T23:32:30.735Z
Learning: In `source/shared/BigInteger.Test.pas` (frostney/GocciaScript PR `#346`), `TestFromDoubleMaxSafeInteger` and `TestFromDoubleNegMaxSafeInteger` intentionally exercise `2^53 = 9007199254740992.0` (and its negation), not `Number.MAX_SAFE_INTEGER = 9007199254740991`. The value `2^53` is the upper boundary of the `FromDouble` Int64 fast-path added in `source/shared/BigInteger.pas` (commit c3fee5b0). The test labels were renamed to "2^53 (Int64 fast-path boundary)" to reflect this intent. Do not re-flag these test values as off-by-one for MAX_SAFE_INTEGER.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 109
File: units/Goccia.Runtime.Operations.pas:2797-2828
Timestamp: 2026-03-23T15:57:21.755Z
Learning: Repo frostney/GocciaScript — PR `#109`: Native Map/Set scaffolding
- Current behavior: ToSouffleValue wraps TGocciaMapValue/TGocciaSetValue as TGocciaWrappedValue; TGocciaSouffleMap/TGocciaSouffleSet instances are not produced in normal execution.
- Pre-wired fast paths: GetProperty, GetIterator, and IteratorNext recognize TGocciaSouffleMap/TGocciaSouffleSet and their iterator types; these are staged for future activation.
- Deferred work: When Construct is updated to produce native collection types, add corresponding fast paths in HasProperty, SetProperty, and DeleteProperty.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 53
File: souffle/Souffle.Wasm.Emitter.pas:1467-1477
Timestamp: 2026-03-09T17:21:38.193Z
Learning: In this codebase (Frostney/GocciaScript), for Pascal/Delphi (FPC) files using AnsiString with {$mode delphi} {H+}, treat String as byte-indexed. Length(str) returns byte count and Byte(str[I]) is a no-op cast. Do not replace per-byte copy loops like Data[I-1] := Byte(AStr[I]) with TEncoding.GetBytes, since TEncoding is a Delphi RTL feature unavailable in FPC's SysUtils and would break the build. Apply this guidance to all .pas files under the project; ensure manual per-byte copying logic remains correct for UTF-8 content encoded in AnsiString, and avoid introducing RTL dependencies not supported by FPC.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 66
File: units/BaseMap.pas:22-22
Timestamp: 2026-03-18T08:12:24.165Z
Learning: Shared/utility units under the repository (e.g., units/BaseMap.pas, HashMap.pas, OrderedMap.pas, etc.) that are used across multiple parts of the project should not be required to use the Goccia.* prefix. A separate naming convention will be defined in a follow-up issue. Do not flag missing Goccia.* prefix for these shared infrastructure units in reviews.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 66
File: units/BaseMap.pas:22-22
Timestamp: 2026-03-18T08:12:24.165Z
Learning: Do not flag missing Goccia.* prefixes for shared/utility units that are used across multiple parts of the project or can be extracted as standalone infrastructure (e.g., BaseMap.pas, HashMap.pas, OrderedMap.pas, OrderedStringMap.pas, GC, test assertion units). A separate naming convention (to be defined in issue `#104`) should apply to these targets. When reviewing, apply this guideline to all similar shared utility units under the units directory; for one-off implementations outside this scope, continue with the existing naming expectations.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 110
File: units/Goccia.Runtime.Operations.pas:0-0
Timestamp: 2026-03-23T11:25:00.381Z
Learning: In this repo’s GocciaScript Pascal code, when implementing thenable detection/callability checks (e.g., around Await/unwrap logic), always read the thenable’s `then` property using `Goccia.Constants.PropertyNames.PROP_THEN` rather than a string literal. If the value being inspected is a `TSouffleRecord`, unwrap the retrieved `then` property via `UnwrapToGocciaValue` before testing whether it is callable (e.g., in `TGocciaRuntimeOperations.AwaitValue`).
Learnt from: frostney
Repo: frostney/GocciaScript PR: 200
File: REPL.dpr:0-0
Timestamp: 2026-04-07T17:12:07.324Z
Learning: When reviewing timing/metrics code in the GocciaScript repo (e.g., REPL.dpr, ScriptLoader, TestRunner, BenchmarkRunner, and TGocciaEngine.Execute), follow the established timing convention: capture `StartTime` before the JSX pre-pass (`TGocciaJSXTransformer.Transform`) and `StringListToLFText`, then capture `LexEnd` after `ScanTokens`. The `Lex` duration (`LexEnd - StartTime`) is intended to include the JSX pre-pass because it is treated as part of the source→tokens pipeline. Do not flag this ordering as a bug and do not recommend moving `StartTime` to after the JSX transform, as it would break the bucket semantics.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 209
File: units/Goccia.FFI.DynamicLibrary.pas:7-21
Timestamp: 2026-04-08T13:56:59.262Z
Learning: In Free Pascal (FPC), `TLibHandle` is defined in the `System` unit (`TLibHandle = PtrInt`), not in `Dynlibs`. Since `System` is implicitly available in all Pascal units, referencing `TLibHandle` in the interface section (e.g., as a private field type of a class declared in the interface/`interface` section) does not require `Dynlibs` to be listed in the interface `uses` clause. Only ensure `Dynlibs` is included in the implementation `uses` clause when you actually use `LoadLibrary`, `GetProcAddress`, or `FreeLibrary`. Therefore, do not flag a missing `Dynlibs` entry in the interface `uses` clause when the interface section references only `TLibHandle`.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 242
File: AGENTS.md:396-400
Timestamp: 2026-04-09T11:50:01.452Z
Learning: In .pas code, avoid unsafe Int64→Double conversions/workarounds that trigger FPC 3.2.2 miscompilations. Specifically, do not rely on `Double(Int64Var)` when it performs bit reinterpretation (Delphi-mode front-end bug on affected versions/platforms). Also do not use arithmetic patterns like `Int64Var * 1.0` / `Int64Var * Double` that can produce incorrect results near ±2^31 on AArch64. Prefer value-safe conversions via implicit assignment (e.g., `var D: Double; D := SomeInt64`) or implicit promotion through function/constructor parameter passing (e.g., `TGocciaNumberLiteralValue.Create(SomeInt64)`).
Learnt from: frostney
Repo: frostney/GocciaScript PR: 253
File: units/Goccia.Values.Iterator.Concat.pas:84-88
Timestamp: 2026-04-10T12:44:59.067Z
Learning: In frostney/GocciaScript (GocciaScript), `TGocciaFunctionBase(callable).Call(args, thisValue)` is the project-standard way to invoke callable values. During code review, do not flag `TGocciaFunctionBase(...)` casts at call sites as unsafe and do not recommend replacing them with an alternative “generic callable path”—for this codebase, this cast-and-`Call` pattern is the intended generic callable invocation mechanism.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 285
File: units/Goccia.Lexer.pas:207-213
Timestamp: 2026-04-12T15:46:28.683Z
Learning: In this Frostney/GocciaScript project (Free Pascal / FPC, including files under units/), do not recommend adding explicit `{$B-}` directives (either inside units or in `Goccia.inc`) to “fix” short-circuit evaluation safety. `{$B-}` is already FPC’s default in all compiler modes (`fpc`, `delphi`, `objfpc`). Treat missing `{$B-}` as expected and do not flag it as a maintainability or safety issue. Only note `{$B-}` overrides if an explicit `{$B+}` is present in the file or via a highly visible project-level `fpc.cfg` setting.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 296
File: units/Goccia.REPL.Formatter.pas:15-29
Timestamp: 2026-04-13T16:53:02.191Z
Learning: For Pascal units in frostney/GocciaScript, enforce `uses`-clause grouping order as follows (with blank lines between groups): (1) System units (e.g., `SysUtils`), (2) Third-party units, then (3) Project units (e.g., `Goccia.*`). Specifically, `StringBuffer` (from `StringBuffer.pas`) must be treated as a Third-party unit and placed in the Third-party group. Do not report a `uses` ordering violation when `StringBuffer` appears between the System group and the `Goccia.*` group, since `./format.pas` and the Lefthook pre-commit hook enforce this grouping.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 302
File: docs/contributing/code-style.md:31-37
Timestamp: 2026-04-14T13:36:54.107Z
Learning: When reviewing Free Pascal (FPC) sources in this project, do not flag the compiler directive `{H+}` as a syntax error or as an inconsistency caused by a missing `$`. In FPC, `{H+}` (short-form) is valid and equivalent to `{$H+}` for enabling long/AnsiStrings. This project’s own `Goccia.inc` and documentation intentionally use `{H+}`, so the reviewer should only consider it an issue if it’s not valid FPC syntax for the surrounding context.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 308
File: units/Goccia.VM.pas:0-0
Timestamp: 2026-04-15T15:41:55.194Z
Learning: In this repo’s Pascal code, when handling “assigning to a read-only” errors, use the exact suggestion type `SSuggestReadOnlyProperty`, paired with `SErrorCannotAssignReadOnly`. Keep this mapping consistent everywhere it applies (e.g., for `ObjectValue`, `ClassValue`, and similar value/descriptor assignments), so the same read-only assignment diagnostic always yields the same suggestion/supported error combination.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 333
File: source/shared/OrderedMap.Test.pas:387-401
Timestamp: 2026-04-18T03:14:07.382Z
Learning: For Free Pascal (FPC) in Delphi mode (`{$mode delphi}`) used in this repo (Pascal `.pas` files), do not treat a bare indexed-property read used as a standalone statement (e.g., `Map[999];`) as a compile error. In this mode, the getter call is executed and its result is silently discarded; if the getter raises an exception, it still propagates to the surrounding `except` handler as expected. For readability/portability, assigning the result to a dummy local (e.g., `Dummy := Map[999];`) is preferred, but bare indexed-property reads should remain unflagged as long as they are valid under FPC Delphi mode.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 337
File: source/units/Goccia.Builtins.TSV.pas:382-388
Timestamp: 2026-04-18T13:58:20.551Z
Learning: When reviewing GocciaScript Pascal code, do not flag missing `Args.Free`/`try/finally` around `TGocciaArgumentsCollection` instances. `TGocciaArgumentsCollection` (in `source/units/Goccia.Arguments.Collection.pas`) descends from `TInterfacedObject` and is reference-counted, so objects created via `CreateWithCapacity` or `Create` are automatically freed when the last reference goes out of scope. An explicit `Free` call (or wrapping in `try/finally`) is not appropriate for this type.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 351
File: source/units/Goccia.Engine.pas:1364-1368
Timestamp: 2026-04-19T15:42:49.575Z
Learning: In GocciaScript (e.g., source/units/Goccia.Engine.pas), treat `Goccia.gc()` / `TGocciaEngine.GocciaGC` as intentionally thread-safe: it should be a no-op on worker threads by guarding the manual `GC.Collect` call with `not GIsWorkerThread`. This prevents races on shared singleton state that uses a single, non-thread-safe `FGCMark` field during mark-sweep. Reviewers should not flag the `GIsWorkerThread` guard as a regression or as a missing manual-GC path; only the full/manual collection triggered by `Goccia.gc()` is skipped on workers, while other worker-thread protections (e.g., the memory-limit ceiling check in `TGocciaValue.AfterConstruction`) should still apply.
Owner
Author
There was a problem hiding this comment.
Please create an issue for this
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
gh issue create \
--repo frostney/GocciaScript \
--title "ZonedDateTimeToString: recompute UTC offset after rounding (DST boundary correctness)" \
--body "## Summary
In \`source/units/Goccia.Values.TemporalZonedDateTime.pas\`, \`ZonedDateTimeToString\` captures \`OffsetSeconds\` via \`GetUtcOffsetSeconds\` **before** calling \`RoundTimeForToString\`. When rounding produces a non-zero \`ExtraDays\` carry, the rounded instant may cross a DST boundary, making the captured offset stale. The formatted string can then contain an impossible local-time/offset combination.
## Affected code
File: \`source/units/Goccia.Values.TemporalZonedDateTime.pas\`, around lines 1116–1155 (\`ZonedDateTimeToString\`).
## Expected fix
After \`RoundTimeForToString\` (and applying \`ExtraDays\` to the date), compute a new rounded epoch milliseconds value and recompute \`OffsetSeconds\` via \`GetUtcOffsetSeconds\` using the rounded epoch before building \`TimeStr\` and \`S\`.
## Context
Identified in PR `#354` (comment: https://github.com/frostney/GocciaScript/pull/354#discussion_r3107322466). The fix is intentionally deferred until full DST transition support is added, as the current timezone implementation uses a fixed offset per-call and the risk is limited to transition zones.
Requested by `@frostney`." \
--assignee frostney
Length of output: 51
@frostney, the follow-up issue has been created: #358
It tracks the stale-offset recomputation in ZonedDateTimeToString and links back to this PR comment for context.
🧠 Learnings used
Learnt from: frostney
Repo: frostney/GocciaScript PR: 354
File: source/units/Goccia.Values.TemporalZonedDateTime.pas:1116-1155
Timestamp: 2026-04-19T18:53:34.108Z
Learning: In `source/units/Goccia.Values.TemporalZonedDateTime.pas` (frostney/GocciaScript, PR `#354`), `ZonedDateTimeToString` computes `OffsetSeconds` via `GetUtcOffsetSeconds` **before** calling `RoundTimeForToString`. After rounding, `ExtraDays` may push the instant across a DST boundary, making the captured offset stale. The fix (recompute `OffsetSeconds` from the rounded epoch after applying `ExtraDays`) is intentionally deferred to the follow-up PR that adds full DST transition support. Do not re-flag the pre-rounding offset capture in `ZonedDateTimeToString` until that DST PR lands.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 354
File: source/units/Goccia.Values.TemporalPlainDate.pas:668-680
Timestamp: 2026-04-19T18:53:31.289Z
Learning: For this project (frostney/GocciaScript), do not raise review findings at individual call sites that use `Arg.ToStringLiteral.Value` on user-supplied arguments for missing Symbol guards (e.g., ensuring `Arg is TGocciaSymbolValue`). This implicit Symbol→string coercion gap is intentionally deferred to a project-wide fix that should be implemented in `ToStringLiteral` itself (or a shared coercion helper). Until that `ToStringLiteral` fix lands, suppress/skip re-flagging the same issue in methods like `DateWithCalendar` and other `withCalendar`/`withTimeZone`-style methods in `source/units`.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 346
File: source/units/Goccia.Values.TemporalZonedDateTime.pas:622-628
Timestamp: 2026-04-18T22:41:49.426Z
Learning: In `source/units/Goccia.Values.TemporalZonedDateTime.pas` (frostney/GocciaScript), `ZonedDateTimeUntil` and `ZonedDateTimeSince` were fixed in commit 70a89a69 (PR `#346`). They no longer collapse the delta into a total `DiffNs: Int64`. Instead they use the same two-field `{DiffMs: Int64, DiffSubMs: Integer}` decomposition as `Instant`: normalize sign between the two fields, then decompose `DiffMs` into hours/minutes/seconds/milliseconds and `DiffSubMs` into microseconds/nanoseconds directly. Do not flag `Int64` overflow risk in these methods — the total-nanosecond collapse has been removed.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 308
File: units/Goccia.Values.TemporalPlainMonthDay.pas:0-0
Timestamp: 2026-04-15T20:56:34.721Z
Learning: In frostney/GocciaScript Temporal value units (e.g., units/Goccia.Values.TemporalPlainMonthDay.pas), use `SSuggestTemporalThisType` when throwing a TypeError because `AThisValue` (or the first arg) is not the expected Temporal type (wrong-receiver / wrong-type-for-helper checks). Use `SSuggestTemporalToPlainDateYear` when throwing because a `toPlainDate` call is missing or receives a non-object/undefined `year` property. Do NOT use the broader `SSuggestTemporalFromArg` for these two specific cases.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 324
File: units/Goccia.Values.TemporalDuration.pas:630-656
Timestamp: 2026-04-17T14:39:13.057Z
Learning: In frostney/GocciaScript, `ParseRelativeTo` in `units/Goccia.Values.TemporalDuration.pas` intentionally supports only `TGocciaTemporalPlainDateValue` and ISO date/date-time string inputs for `relativeTo` in `Duration.prototype.total()`. Support for `TGocciaTemporalZonedDateTimeValue` and property-bag/object inputs is deferred to a dedicated follow-up PR, as it requires importing the ZonedDateTime unit, extracting the plain date, and handling DST-aware day lengths. Do not re-flag missing ZonedDateTime/property-bag handling in `ParseRelativeTo` until that follow-up PR lands.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 308
File: units/Goccia.Values.TemporalPlainDateTime.pas:144-145
Timestamp: 2026-04-16T06:33:07.065Z
Learning: In frostney/GocciaScript Temporal unit implementations (e.g., units/Goccia.Values.TemporalPlainDateTime.pas, units/Goccia.Values.TemporalPlainDate.pas, units/Goccia.Values.TemporalPlainTime.pas, units/Goccia.Values.TemporalPlainYearMonth.pas, units/Goccia.Values.TemporalPlainMonthDay.pas, units/Goccia.Values.TemporalInstant.pas, units/Goccia.Values.TemporalZonedDateTime.pas, units/Goccia.Builtins.Temporal.pas), the correction of ThrowTypeError → ThrowRangeError for invalid ISO/time/duration string parsing failures (per TC39 Temporal spec) is intentionally deferred to a dedicated follow-up PR. PRs that only centralize error message constants and add suggestion metadata must not change error types (TypeError vs RangeError). Do not re-flag the ThrowTypeError usage for ISO/time/duration parse failures in these Temporal units until the dedicated behavioral-change PR lands.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 324
File: units/Goccia.Values.TemporalDuration.pas:891-920
Timestamp: 2026-04-17T14:39:20.659Z
Learning: In frostney/GocciaScript (units/Goccia.Values.TemporalDuration.pas), the engine supports only the ISO 8601 calendar. Weeks are always exactly 7 days in this calendar, so Duration.prototype.total does NOT require `relativeTo` for the 'weeks' unit or for durations with non-zero FWeeks components. The fixed `weeks * 7 * NANOSECONDS_PER_DAY` calculation is intentional and correct. Do not flag missing relativeTo guards for week units/components in this file. If/when non-ISO calendar support is added, this guard should be revisited.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 346
File: source/units/Goccia.Values.TemporalInstant.pas:178-184
Timestamp: 2026-04-18T22:29:31.789Z
Learning: In `source/units/Goccia.Values.TemporalInstant.pas` (frostney/GocciaScript), the `GetEpochNanoseconds` getter is safe from overflow. `FEpochMilliseconds` is `Int64` (max ≈ ±9.2×10¹⁸ ms) and the valid Temporal instant range is ±8.64×10¹⁵ ms, so `TBigInteger.FromInt64(Inst.FEpochMilliseconds)` is always lossless. Do NOT flag `TBigInteger.FromInt64` on `FEpochMilliseconds` in this getter as a potential overflow.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 327
File: units/Goccia.Values.TemporalDuration.pas:541-561
Timestamp: 2026-04-17T16:31:38.698Z
Learning: In frostney/GocciaScript (units/Goccia.Values.TemporalDuration.pas), `DurationRound` intentionally parses and validates the `relativeTo` option eagerly (before `NeedCalendar` is computed) because the TC39 Temporal spec calls `ToRelativeTemporalObject` as an early step of `Duration.prototype.round` — before any check for whether calendar math is needed. An invalid `relativeTo` must throw even for a pure time/day round. Do NOT flag the eager parsing in `DurationRound` as incorrect or suggest deferring it until `NeedCalendar` is known. If `DurationTotal` uses lazy parsing, that is the less spec-compliant path, not `DurationRound`.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 277
File: units/Goccia.Builtins.Temporal.pas:0-0
Timestamp: 2026-04-12T13:19:32.780Z
Learning: For all Temporal-related units in GocciaScript (e.g., files like Goccia.Builtins.Temporal.pas and Goccia.Values.TemporalPlainTime.pas), use the shared Temporal property-name constants defined in units/Goccia.Constants.PropertyNames.pas (PROP_YEAR, PROP_MONTH, PROP_MONTH_CODE, PROP_DAY, PROP_CALENDAR_ID, plus a full set of time unit constants: PROP_HOUR, PROP_MINUTE, PROP_SECOND, PROP_MILLISECOND, PROP_MICROSECOND, PROP_NANOSECOND) when accessing Temporal properties and when constructing any related error messages. Do not use hardcoded string literals for these Temporal property names in Temporal units.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 200
File: REPL.dpr:0-0
Timestamp: 2026-04-07T17:12:07.324Z
Learning: When reviewing timing/metrics code in the GocciaScript repo (e.g., REPL.dpr, ScriptLoader, TestRunner, BenchmarkRunner, and TGocciaEngine.Execute), follow the established timing convention: capture `StartTime` before the JSX pre-pass (`TGocciaJSXTransformer.Transform`) and `StringListToLFText`, then capture `LexEnd` after `ScanTokens`. The `Lex` duration (`LexEnd - StartTime`) is intended to include the JSX pre-pass because it is treated as part of the source→tokens pipeline. Do not flag this ordering as a bug and do not recommend moving `StartTime` to after the JSX transform, as it would break the bucket semantics.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 343
File: source/units/Goccia.Lexer.pas:1429-1437
Timestamp: 2026-04-18T20:08:21.207Z
Learning: In `source/units/Goccia.Lexer.pas` (frostney/GocciaScript PR `#343`), the `gttBigInt` lexeme emitted by the lexer preserves the radix prefix (e.g., `0xFF`, `0b1010`, `0o77`) after stripping separators and the `n` suffix. The parser's private helper `ConvertBigIntLiteral` (source/units/Goccia.Parser.pas, line ~4339) is radix-aware: it detects `0x`/`0X`, `0b`/`0B`, `0o`/`0O` prefixes and dispatches to `TBigInteger.FromHexString`, `FromBinaryString`, or `FromOctalString` respectively, falling back to `FromDecimalString` for plain decimal literals. The lexer/parser contract is correct and intentional. Do NOT flag the preserved prefix in `gttBigInt` lexemes as inconsistent with the parser's BigInt construction.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 343
File: source/units/Goccia.JSON.pas:841-844
Timestamp: 2026-04-18T20:08:00.723Z
Learning: In frostney/GocciaScript (source/units/Goccia.JSON.pas), `ApplyToJSON` intentionally only probes `toJSON` when the value is a `TGocciaObjectValue`. For BigInt primitives (`TGocciaBigIntValue`), `JSON.stringify(1n)` correctly throws TypeError per ES2026 §25.5.2.5 step 10 — this is the expected default behavior. Extending `ApplyToJSON` to call `GetProperty('toJSON')` directly on BigInt primitives (per ES2026 §25.5.2.2) is a known gap deferred to a follow-up PR. Do not re-flag the missing `toJSON` probe for BigInt in `ApplyToJSON` until that follow-up is scoped.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 306
File: TestRunner.dpr:634-636
Timestamp: 2026-04-15T06:27:56.026Z
Learning: In frostney/GocciaScript (PR `#306`, TestRunner.dpr), the `WriteLn` calls inside the error-handling paths of `RunGocciaScriptInterpreted` and `RunGocciaScriptBytecode` (e.g. on EStreamError, TGocciaError, TGocciaThrowValue, and generic Exception) execute on worker threads during parallel directory runs, causing potentially interleaved stdout. This is an accepted trade-off: these paths are rare (script-load failures / uncaught exceptions only), messages include filenames for identification, and a proper output-capture refactor (routing error text through AConsoleOutput/AErrorMessage and printing on the main thread) is deferred to a follow-up. Do NOT re-flag these WriteLn calls in TestRunner.dpr parallel worker context until the output-capture refactor lands.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 336
File: source/units/Goccia.Engine.StrictTypes.Test.pas:45-47
Timestamp: 2026-04-18T11:56:12.474Z
Learning: In frostney/GocciaScript Pascal test files (e.g. `Goccia.Lexer.Test.pas`, `Goccia.Error.Test.pas`, `Goccia.OrderedStringMap.Test.pas`, `Goccia.TextFiles.Test.pas`, `Goccia.Engine.StrictTypes.Test.pas`), the construct-before-try pattern (allocating objects before the `try` block without pre-initializing refs to `nil`) is intentional and is the established project-wide convention for test code. The constructors used are simple and the leak risk is considered negligible in the test context. Do NOT flag this pattern as an exception-safety issue or suggest the nil-init / try-before-construct alternative in these test files.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 209
File: units/Goccia.Values.FFILibrary.pas:73-82
Timestamp: 2026-04-08T13:55:31.119Z
Learning: In frostney/GocciaScript (units/Goccia.Values.FFILibrary.pas, PR `#209`), `TGocciaFFIBoundFunction` intentionally holds only a raw `CodePointer` with no back-reference to the owning `TGocciaFFILibraryHandle`. Calling `close()` while bound functions or raw symbol pointers still exist is documented UB — the same contract as Deno/Bun FFI. This is an explicit v1 design decision, gated behind the `ggFFI` opt-in flag so only embedders who accept the contract enable it. A ref-counted handle guard is planned as a v2 enhancement. Do NOT re-flag the missing library back-reference or "bind() leaks" in future reviews of this unit.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 299
File: TestRunner.dpr:132-139
Timestamp: 2026-04-14T06:46:13.250Z
Learning: In frostney/GocciaScript, `EscapeJSONString` (escaping `\`, `"`, `\n`, `\r`, `\t` only) is intentionally duplicated across ~5 CLI tool files (e.g., TestRunner.dpr, BenchmarkRunner.dpr). The incomplete control-character coverage (missing U+0000–U+001F beyond the five named characters) is a known pre-existing issue tracked for fix as part of a future consolidation into a shared helper. Do NOT re-flag missing control-character escaping in individual per-tool copies of `EscapeJSONString` until that consolidation PR lands.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 306
File: ScriptLoader.dpr:692-707
Timestamp: 2026-04-15T06:26:13.084Z
Learning: In frostney/GocciaScript (ScriptLoader.dpr), `TScriptLoaderApp.ScriptWorkerProc` intentionally calls `RunScriptFromFile` directly in worker threads, which causes interleaved/nondeterministic stdout in parallel directory runs. This is an accepted trade-off for the initial parallel implementation — ScriptLoader parallel mode is only used for directory inputs where output ordering is less critical. A proper fix (refactoring `RunSource`/`ExecuteInterpreted` to support output capture and returning text via `AConsoleOutput`/`AErrorMessage`) has been deferred to a follow-up. Do not re-flag the direct `RunScriptFromFile` call in `ScriptWorkerProc` as a regression or issue until the output-capture refactor lands.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 285
File: units/Goccia.Lexer.pas:207-213
Timestamp: 2026-04-12T15:46:28.683Z
Learning: In this Frostney/GocciaScript project (Free Pascal / FPC, including files under units/), do not recommend adding explicit `{$B-}` directives (either inside units or in `Goccia.inc`) to “fix” short-circuit evaluation safety. `{$B-}` is already FPC’s default in all compiler modes (`fpc`, `delphi`, `objfpc`). Treat missing `{$B-}` as expected and do not flag it as a maintainability or safety issue. Only note `{$B-}` overrides if an explicit `{$B+}` is present in the file or via a highly visible project-level `fpc.cfg` setting.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 66
File: units/BaseMap.pas:22-22
Timestamp: 2026-03-18T08:12:24.165Z
Learning: Shared/utility units under the repository (e.g., units/BaseMap.pas, HashMap.pas, OrderedMap.pas, etc.) that are used across multiple parts of the project should not be required to use the Goccia.* prefix. A separate naming convention will be defined in a follow-up issue. Do not flag missing Goccia.* prefix for these shared infrastructure units in reviews.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 66
File: units/BaseMap.pas:22-22
Timestamp: 2026-03-18T08:12:24.165Z
Learning: Do not flag missing Goccia.* prefixes for shared/utility units that are used across multiple parts of the project or can be extracted as standalone infrastructure (e.g., BaseMap.pas, HashMap.pas, OrderedMap.pas, OrderedStringMap.pas, GC, test assertion units). A separate naming convention (to be defined in issue `#104`) should apply to these targets. When reviewing, apply this guideline to all similar shared utility units under the units directory; for one-off implementations outside this scope, continue with the existing naming expectations.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 107
File: units/Goccia.Runtime.Operations.pas:211-212
Timestamp: 2026-03-23T12:37:30.936Z
Learning: Repo: frostney/GocciaScript — units/Goccia.Runtime.Operations.pas (PR `#107`)
Learning: In TGocciaRuntimeOperations.MarkExternalRoots, entries in FModuleCache (cached module namespace/JSON records) must be traversed and their TSouffleValue references marked as external roots to survive GC. This behavior was added in commit 36f69cc; do not re‑flag missing FModuleCache rooting unless the logic regresses.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 302
File: docs/contributing/code-style.md:43-46
Timestamp: 2026-04-14T13:31:59.106Z
Learning: In frostney/GocciaScript (docs/contributing/code-style.md and related docs), units like `BaseMap.pas`, `HashMap.pas`, `OrderedMap.pas`, `OrderedStringMap.pas` are described as "shared infrastructure units that are not GocciaScript-specific" and intentionally live outside the `Goccia.*` namespace. They are NOT "legacy units." Only project-specific units follow the `Goccia.<Category>.<Name>.pas` convention. The naming convention for these shared infrastructure units is tracked in issue `#104`. Do not describe these units as "legacy" in review comments.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 306
File: units/Goccia.Builtins.GlobalPromise.pas:56-57
Timestamp: 2026-04-15T06:26:35.760Z
Learning: In frostney/GocciaScript (PR `#306`), ~30 builtin units (e.g., Goccia.Builtins.GlobalPromise.pas, Goccia.Builtins.GlobalArray.pas, Goccia.Builtins.Math.pas, and ~27 others) use `threadvar FStaticMembers: TArray<TGocciaMemberDefinition>` in their `implementation` sections. FPC does NOT finalize managed threadvar contents on thread exit, so the TArray shell (~few hundred bytes per unit) leaks per worker thread. This is an accepted/deferred trade-off: (1) threads are short-lived spawn-and-join workers, (2) the GC-managed TGocciaMemberDefinition objects are cleaned up by ShutdownThreadRuntime, (3) OS reclaims all memory on process exit, (4) a proper fix requires a registration-based cleanup system or per-unit finalization hooks since the threadvars are in implementation sections. Do NOT re-flag this managed-threadvar leak until long-lived thread pools are introduced and a cleanup registry is in place.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 147
File: units/Goccia.VM.pas:3042-3084
Timestamp: 2026-03-31T18:38:23.367Z
Learning: Repo: frostney/GocciaScript — units/Goccia.VM.pas (PR `#147`)
As of commit 457e712: (1) TGocciaVM.GetPropertyValue performs own-only private static accessor lookup on '#…' for TGocciaClassValue and throws TypeError on class-brand misses; (2) TGocciaVM.SetPropertyValue avoids inherited private accessor resolution, but still allows raw private static writes via SetRawPrivateValue to support current class initialization that emits OP_SET_PROP_CONST for private static fields. Planned follow-up is to add dedicated opcodes to define private static fields at class-declaration time and then make SetPropertyValue strict (throw on brand miss with no fallback). Do not re-flag the current fallback until those opcodes exist.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 136
File: docs/adding-built-in-types.md:105-133
Timestamp: 2026-03-27T14:09:32.826Z
Learning: Applies to units/**/*.pas and docs/**/*.md (frostney/GocciaScript): The documented and enforced convention for shared prototype singleton initialization IS the early-exit guard `if Assigned(FShared) then Exit` at the top of `InitializePrototype`. Do NOT flag this pattern or suggest replacing it with `if not Assigned(FShared) then` wrapping. The project docs (adding-built-in-types.md, code-style.md) explicitly use the early-exit form as the canonical template.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 60
File: units/Goccia.Runtime.Operations.pas:8694-8696
Timestamp: 2026-03-10T18:05:46.926Z
Learning: Repo: frostney/GocciaScript PR: 60 — units/Goccia.Runtime.Operations.pas: The GC rooting of FArrayBridgeReverse (marking values in MarkExternalRoots and keys in MarkWrappedGocciaValues) is intentional to preserve array identity across async/microtask boundaries (e.g., Promise.resolve(arr).then(v => v === arr)). Do not prune this map during normal clearing; per-file GC.Collect in runners bounds cross-file memory growth.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 277
File: units/Goccia.Values.TemporalPlainMonthDay.pas:0-0
Timestamp: 2026-04-12T10:23:58.234Z
Learning: In frostney/GocciaScript, Temporal-specific property name constants `PROP_YEAR`, `PROP_MONTH`, `PROP_MONTH_CODE`, `PROP_DAY`, and `PROP_CALENDAR_ID` were added to `Goccia.Constants.PropertyNames` (as of commit d59dd03, PR `#277`). All Temporal value units (e.g. `Goccia.Values.TemporalPlainMonthDay.pas`, `Goccia.Values.TemporalPlainYearMonth.pas`, etc.) must use these constants instead of hardcoded string literals for property accesses (`GetProperty`, `SetProperty`, accessor registration, `ThrowTypeError` messages referencing property names, etc.).
Learnt from: frostney
Repo: frostney/GocciaScript PR: 324
File: units/Goccia.Values.TemporalDuration.pas:691-693
Timestamp: 2026-04-17T14:59:54.438Z
Learning: In frostney/GocciaScript (units/Goccia.Values.TemporalDuration.pas), TC39 Temporal Duration magnitude validation (calendar units < 2^32, normalized seconds < 2^53) is intentionally deferred to a follow-up PR. The validation belongs in the `TGocciaTemporalDurationValue` constructor and `DurationFromObject`, not in `DurationEndDate`, `AddMonthsToDate`, or `DurationTotal`. Do not re-flag missing magnitude validation in `DurationEndDate` or `total()` path — the arithmetic there is safe for spec-compliant inputs; the gap is upstream validation not yet implemented.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 63
File: units/Goccia.Runtime.Operations.pas:0-0
Timestamp: 2026-03-11T01:50:11.071Z
Learning: Replace hardcoded string literals 'now', 'toJSON', and 'timeOrigin' with the new constants from Goccia.Constants.PropertyNames (PROP_NOW, PROP_TO_JSON, PROP_TIME_ORIGIN). Apply this refactor across the repository wherever these literals are used, including but not limited to units/Goccia.Runtime.Operations.pas and units/Goccia.Builtins.Performance.pas. Use a project-wide search (e.g., regex or code navigation) to identify all occurrences, update to the respective constants, and adjust imports or uses accordingly. Ensure all references compile and behavior remains unchanged.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 343
File: source/units/Goccia.Evaluator.Arithmetic.pas:49-55
Timestamp: 2026-04-18T21:08:03.166Z
Learning: In frostney/GocciaScript (commit 3344dede, PR `#343`), `TGocciaBigIntObjectValue` was added to support BigInt boxing via `Object(1n)`. `Box` in `ClassHelper.pas` now returns a `TGocciaBigIntObjectValue` for `TGocciaBigIntValue` primitives. `ToPrimitive` calls were added to `EvaluateSubtraction`, `EvaluateMultiplication`, `EvaluateDivision`, `EvaluateModulo`, and `EvaluateExponentiation` in `source/units/Goccia.Evaluator.Arithmetic.pas` so that boxed BigInts (e.g. `Object(1n) - 1n`) are correctly unboxed via `valueOf()` before BigInt type checks. `Object()` was also fixed to call `Box` for primitive arguments per ES2026 §20.1.1.1. Future reviews of arithmetic operators SHOULD expect `ToPrimitive` before BigInt checks.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 343
File: source/units/Goccia.Builtins.GlobalBigInt.pas:213-223
Timestamp: 2026-04-18T20:48:02.918Z
Learning: In `source/units/Goccia.Builtins.GlobalBigInt.pas` (frostney/GocciaScript, landed in commit 477b5609 on PR `#343`), `BigInt.asIntN` and `BigInt.asUintN` use proper `ToIndex` abstract-op coercion for the `bits` parameter: `undefined`/`NaN` → 0, `Infinity`/negative/oversized → `RangeError`. This is fully implemented and tested. Do not flag missing `ToIndex` coercion for these methods.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 346
File: source/shared/BigInteger.Test.pas:0-0
Timestamp: 2026-04-18T23:32:30.735Z
Learning: In `source/shared/BigInteger.Test.pas` (frostney/GocciaScript PR `#346`), `TestFromDoubleMaxSafeInteger` and `TestFromDoubleNegMaxSafeInteger` intentionally exercise `2^53 = 9007199254740992.0` (and its negation), not `Number.MAX_SAFE_INTEGER = 9007199254740991`. The value `2^53` is the upper boundary of the `FromDouble` Int64 fast-path added in `source/shared/BigInteger.pas` (commit c3fee5b0). The test labels were renamed to "2^53 (Int64 fast-path boundary)" to reflect this intent. Do not re-flag these test values as off-by-one for MAX_SAFE_INTEGER.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 53
File: souffle/Souffle.Wasm.Emitter.pas:1467-1477
Timestamp: 2026-03-09T17:21:38.193Z
Learning: In this codebase (Frostney/GocciaScript), for Pascal/Delphi (FPC) files using AnsiString with {$mode delphi} {H+}, treat String as byte-indexed. Length(str) returns byte count and Byte(str[I]) is a no-op cast. Do not replace per-byte copy loops like Data[I-1] := Byte(AStr[I]) with TEncoding.GetBytes, since TEncoding is a Delphi RTL feature unavailable in FPC's SysUtils and would break the build. Apply this guidance to all .pas files under the project; ensure manual per-byte copying logic remains correct for UTF-8 content encoded in AnsiString, and avoid introducing RTL dependencies not supported by FPC.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 110
File: units/Goccia.Runtime.Operations.pas:0-0
Timestamp: 2026-03-23T11:25:00.381Z
Learning: In this repo’s GocciaScript Pascal code, when implementing thenable detection/callability checks (e.g., around Await/unwrap logic), always read the thenable’s `then` property using `Goccia.Constants.PropertyNames.PROP_THEN` rather than a string literal. If the value being inspected is a `TSouffleRecord`, unwrap the retrieved `then` property via `UnwrapToGocciaValue` before testing whether it is callable (e.g., in `TGocciaRuntimeOperations.AwaitValue`).
Learnt from: frostney
Repo: frostney/GocciaScript PR: 209
File: units/Goccia.FFI.DynamicLibrary.pas:7-21
Timestamp: 2026-04-08T13:56:59.262Z
Learning: In Free Pascal (FPC), `TLibHandle` is defined in the `System` unit (`TLibHandle = PtrInt`), not in `Dynlibs`. Since `System` is implicitly available in all Pascal units, referencing `TLibHandle` in the interface section (e.g., as a private field type of a class declared in the interface/`interface` section) does not require `Dynlibs` to be listed in the interface `uses` clause. Only ensure `Dynlibs` is included in the implementation `uses` clause when you actually use `LoadLibrary`, `GetProcAddress`, or `FreeLibrary`. Therefore, do not flag a missing `Dynlibs` entry in the interface `uses` clause when the interface section references only `TLibHandle`.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 242
File: AGENTS.md:396-400
Timestamp: 2026-04-09T11:50:01.452Z
Learning: In .pas code, avoid unsafe Int64→Double conversions/workarounds that trigger FPC 3.2.2 miscompilations. Specifically, do not rely on `Double(Int64Var)` when it performs bit reinterpretation (Delphi-mode front-end bug on affected versions/platforms). Also do not use arithmetic patterns like `Int64Var * 1.0` / `Int64Var * Double` that can produce incorrect results near ±2^31 on AArch64. Prefer value-safe conversions via implicit assignment (e.g., `var D: Double; D := SomeInt64`) or implicit promotion through function/constructor parameter passing (e.g., `TGocciaNumberLiteralValue.Create(SomeInt64)`).
Learnt from: frostney
Repo: frostney/GocciaScript PR: 253
File: units/Goccia.Values.Iterator.Concat.pas:84-88
Timestamp: 2026-04-10T12:44:59.067Z
Learning: In frostney/GocciaScript (GocciaScript), `TGocciaFunctionBase(callable).Call(args, thisValue)` is the project-standard way to invoke callable values. During code review, do not flag `TGocciaFunctionBase(...)` casts at call sites as unsafe and do not recommend replacing them with an alternative “generic callable path”—for this codebase, this cast-and-`Call` pattern is the intended generic callable invocation mechanism.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 296
File: units/Goccia.REPL.Formatter.pas:15-29
Timestamp: 2026-04-13T16:53:02.191Z
Learning: For Pascal units in frostney/GocciaScript, enforce `uses`-clause grouping order as follows (with blank lines between groups): (1) System units (e.g., `SysUtils`), (2) Third-party units, then (3) Project units (e.g., `Goccia.*`). Specifically, `StringBuffer` (from `StringBuffer.pas`) must be treated as a Third-party unit and placed in the Third-party group. Do not report a `uses` ordering violation when `StringBuffer` appears between the System group and the `Goccia.*` group, since `./format.pas` and the Lefthook pre-commit hook enforce this grouping.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 302
File: docs/contributing/code-style.md:31-37
Timestamp: 2026-04-14T13:36:54.107Z
Learning: When reviewing Free Pascal (FPC) sources in this project, do not flag the compiler directive `{H+}` as a syntax error or as an inconsistency caused by a missing `$`. In FPC, `{H+}` (short-form) is valid and equivalent to `{$H+}` for enabling long/AnsiStrings. This project’s own `Goccia.inc` and documentation intentionally use `{H+}`, so the reviewer should only consider it an issue if it’s not valid FPC syntax for the surrounding context.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 308
File: units/Goccia.VM.pas:0-0
Timestamp: 2026-04-15T15:41:55.194Z
Learning: In this repo’s Pascal code, when handling “assigning to a read-only” errors, use the exact suggestion type `SSuggestReadOnlyProperty`, paired with `SErrorCannotAssignReadOnly`. Keep this mapping consistent everywhere it applies (e.g., for `ObjectValue`, `ClassValue`, and similar value/descriptor assignments), so the same read-only assignment diagnostic always yields the same suggestion/supported error combination.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 333
File: source/shared/OrderedMap.Test.pas:387-401
Timestamp: 2026-04-18T03:14:07.382Z
Learning: For Free Pascal (FPC) in Delphi mode (`{$mode delphi}`) used in this repo (Pascal `.pas` files), do not treat a bare indexed-property read used as a standalone statement (e.g., `Map[999];`) as a compile error. In this mode, the getter call is executed and its result is silently discarded; if the getter raises an exception, it still propagates to the surrounding `except` handler as expected. For readability/portability, assigning the result to a dummy local (e.g., `Dummy := Map[999];`) is preferred, but bare indexed-property reads should remain unflagged as long as they are valid under FPC Delphi mode.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 337
File: source/units/Goccia.Builtins.TSV.pas:382-388
Timestamp: 2026-04-18T13:58:20.551Z
Learning: When reviewing GocciaScript Pascal code, do not flag missing `Args.Free`/`try/finally` around `TGocciaArgumentsCollection` instances. `TGocciaArgumentsCollection` (in `source/units/Goccia.Arguments.Collection.pas`) descends from `TInterfacedObject` and is reference-counted, so objects created via `CreateWithCapacity` or `Create` are automatically freed when the last reference goes out of scope. An explicit `Free` call (or wrapping in `try/finally`) is not appropriate for this type.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 351
File: source/units/Goccia.Engine.pas:1364-1368
Timestamp: 2026-04-19T15:42:49.575Z
Learning: In GocciaScript (e.g., source/units/Goccia.Engine.pas), treat `Goccia.gc()` / `TGocciaEngine.GocciaGC` as intentionally thread-safe: it should be a no-op on worker threads by guarding the manual `GC.Collect` call with `not GIsWorkerThread`. This prevents races on shared singleton state that uses a single, non-thread-safe `FGCMark` field during mark-sweep. Reviewers should not flag the `GIsWorkerThread` guard as a regression or as a missing manual-GC path; only the full/manual collection triggered by `Goccia.gc()` is skipped on workers, while other worker-thread protections (e.g., the memory-limit ceiling check in `TGocciaValue.AfterConstruction`) should still apply.
- toLocaleString across all 8 Temporal types now creates empty args instead of forwarding AArgs to toString, preventing locale strings from being misinterpreted as toString options - Instant.prototype.toZonedDateTimeISO now accepts ZonedDateTime objects in addition to strings, extracting the timezone Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This was referenced Apr 19, 2026
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
source/units/Goccia.Values.TemporalInstant.pas (1)
551-554:⚠️ Potential issue | 🟡 MinorUpdate the error text to match the broadened input contract.
This method now accepts
TGocciaTemporalZonedDateTimeValue, but the thrown message still says “string time zone”. That will point callers in the wrong direction when they pass an unsupported non-string value.🛠️ Suggested wording fix
- ThrowTypeError('Instant.prototype.toZonedDateTimeISO requires a string time zone', SSuggestTemporalTimezone); + ThrowTypeError('Instant.prototype.toZonedDateTimeISO requires a time zone or ZonedDateTime', SSuggestTemporalTimezone);🤖 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 551 - 554, The error message thrown in Instant.prototype.toZonedDateTimeISO is stale (it says "requires a string time zone") even though the method now accepts TGocciaTemporalZonedDateTimeValue; update the ThrowTypeError call (the line invoking ThrowTypeError with SSuggestTemporalTimezone) to use a clearer message that reflects the broadened input contract (e.g., require "a time zone (string or TGocciaTemporalZonedDateTimeValue)" or similar), keeping the same ThrowTypeError invocation and SSuggestTemporalTimezone usage and leaving TZ handling unchanged.
🧹 Nitpick comments (3)
source/units/Goccia.Values.TemporalPlainMonthDay.pas (2)
173-177: Avoid duplicating the Temporal type-name literal here.This adds a second hardcoded
'Temporal.PlainMonthDay'in the unit, which can drift fromToStringTag. Prefer a shared type-name constant and reuse it from both registration sites.As per coding guidelines, "Use split constant units for runtime constants instead of hardcoded string literals:
Goccia.Constants.PropertyNamesfor property names,Goccia.Constants.TypeNamesfor type names,Goccia.Constants.ErrorNamesfor error names,Goccia.Constants.ConstructorNamesfor constructor names, andGoccia.Constants.SymbolNamesfor symbol names."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/units/Goccia.Values.TemporalPlainMonthDay.pas` around lines 173 - 177, The literal 'Temporal.PlainMonthDay' is duplicated here; replace the hardcoded string passed to TGocciaStringLiteralValue.Create with the shared type-name constant from the split constants unit (e.g. use the appropriate identifier in Goccia.Constants.TypeNames) so both the ToStringTag registration and any other registrations reuse the same constant; update the code around Members.AddSymbolDataProperty and keep MonthDayToLocaleString unchanged, referencing the TypeNames constant instead of the literal.
356-365: Drop the manualFreeonTGocciaArgumentsCollection.
TGocciaArgumentsCollectionis ref-counted in this repo, so the explicitFree/try...finallyhere is the wrong lifetime pattern. Let the local reference go out of scope instead.♻️ Proposed cleanup
function TGocciaTemporalPlainMonthDayValue.MonthDayToLocaleString(const AArgs: TGocciaArgumentsCollection; const AThisValue: TGocciaValue): TGocciaValue; var EmptyArgs: TGocciaArgumentsCollection; begin EmptyArgs := TGocciaArgumentsCollection.Create([]); - try - Result := MonthDayToString(EmptyArgs, AThisValue); - finally - EmptyArgs.Free; - end; + Result := MonthDayToString(EmptyArgs, AThisValue); end;Based on learnings, "
TGocciaArgumentsCollection(insource/units/Goccia.Arguments.Collection.pas) descends fromTInterfacedObjectand is reference-counted, so objects created viaCreateWithCapacityorCreateare automatically freed when the last reference goes out of scope. An explicitFreecall (or wrapping intry/finally) is not appropriate for this type."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/units/Goccia.Values.TemporalPlainMonthDay.pas` around lines 356 - 365, The method TGocciaTemporalPlainMonthDayValue.MonthDayToLocaleString currently creates a TGocciaArgumentsCollection and manually calls Free in a try/finally; since TGocciaArgumentsCollection is ref-counted (descends from TInterfacedObject) remove the try..finally and the explicit EmptyArgs.Free, simply create the EmptyArgs local (via TGocciaArgumentsCollection.Create([])) and pass it to MonthDayToString so the reference-counting releases it when the variable goes out of scope.source/units/Goccia.Values.TemporalDuration.pas (1)
1233-1242: Drop the manualFree/try...finallyaroundTGocciaArgumentsCollectionhere.
TGocciaArgumentsCollectionis reference-counted in this codebase, so this adapter does not need explicit lifetime management. The same pattern was copied into the other new TemporaltoLocaleStringadapters in this PR, so it’s worth normalizing all of them the same way.♻️ Suggested simplification
function TGocciaTemporalDurationValue.DurationToLocaleString(const AArgs: TGocciaArgumentsCollection; const AThisValue: TGocciaValue): TGocciaValue; var EmptyArgs: TGocciaArgumentsCollection; begin EmptyArgs := TGocciaArgumentsCollection.Create([]); - try - Result := DurationToString(EmptyArgs, AThisValue); - finally - EmptyArgs.Free; - end; + Result := DurationToString(EmptyArgs, AThisValue); end;Based on learnings,
TGocciaArgumentsCollectiondescends fromTInterfacedObject, so explicitFree/try...finallyis not appropriate for this type.🤖 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 1233 - 1242, In TGocciaTemporalDurationValue.DurationToLocaleString replace the try/finally manual lifetime handling for the TGocciaArgumentsCollection with simple reference-counted usage: create EmptyArgs via TGocciaArgumentsCollection.Create([]) and pass it to DurationToString(AArgs, AThisValue) (or DurationToString(EmptyArgs, AThisValue)) without calling EmptyArgs.Free or wrapping in try..finally, since TGocciaArgumentsCollection is reference-counted (descends from TInterfacedObject); do the same normalization for the other Temporal toLocaleString adapters following this pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@source/units/Goccia.Values.TemporalInstant.pas`:
- Around line 551-554: The error message thrown in
Instant.prototype.toZonedDateTimeISO is stale (it says "requires a string time
zone") even though the method now accepts TGocciaTemporalZonedDateTimeValue;
update the ThrowTypeError call (the line invoking ThrowTypeError with
SSuggestTemporalTimezone) to use a clearer message that reflects the broadened
input contract (e.g., require "a time zone (string or
TGocciaTemporalZonedDateTimeValue)" or similar), keeping the same ThrowTypeError
invocation and SSuggestTemporalTimezone usage and leaving TZ handling unchanged.
---
Nitpick comments:
In `@source/units/Goccia.Values.TemporalDuration.pas`:
- Around line 1233-1242: In TGocciaTemporalDurationValue.DurationToLocaleString
replace the try/finally manual lifetime handling for the
TGocciaArgumentsCollection with simple reference-counted usage: create EmptyArgs
via TGocciaArgumentsCollection.Create([]) and pass it to DurationToString(AArgs,
AThisValue) (or DurationToString(EmptyArgs, AThisValue)) without calling
EmptyArgs.Free or wrapping in try..finally, since TGocciaArgumentsCollection is
reference-counted (descends from TInterfacedObject); do the same normalization
for the other Temporal toLocaleString adapters following this pattern.
In `@source/units/Goccia.Values.TemporalPlainMonthDay.pas`:
- Around line 173-177: The literal 'Temporal.PlainMonthDay' is duplicated here;
replace the hardcoded string passed to TGocciaStringLiteralValue.Create with the
shared type-name constant from the split constants unit (e.g. use the
appropriate identifier in Goccia.Constants.TypeNames) so both the ToStringTag
registration and any other registrations reuse the same constant; update the
code around Members.AddSymbolDataProperty and keep MonthDayToLocaleString
unchanged, referencing the TypeNames constant instead of the literal.
- Around line 356-365: The method
TGocciaTemporalPlainMonthDayValue.MonthDayToLocaleString currently creates a
TGocciaArgumentsCollection and manually calls Free in a try/finally; since
TGocciaArgumentsCollection is ref-counted (descends from TInterfacedObject)
remove the try..finally and the explicit EmptyArgs.Free, simply create the
EmptyArgs local (via TGocciaArgumentsCollection.Create([])) and pass it to
MonthDayToString so the reference-counting releases it when the variable goes
out of scope.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6102a5f1-29c4-4500-b01a-826b3162141d
📒 Files selected for processing (8)
source/units/Goccia.Values.TemporalDuration.passource/units/Goccia.Values.TemporalInstant.passource/units/Goccia.Values.TemporalPlainDate.passource/units/Goccia.Values.TemporalPlainDateTime.passource/units/Goccia.Values.TemporalPlainMonthDay.passource/units/Goccia.Values.TemporalPlainTime.passource/units/Goccia.Values.TemporalPlainYearMonth.passource/units/Goccia.Values.TemporalZonedDateTime.pas
🚧 Files skipped from review as they are similar to previous changes (3)
- source/units/Goccia.Values.TemporalPlainYearMonth.pas
- source/units/Goccia.Values.TemporalPlainDate.pas
- source/units/Goccia.Values.TemporalPlainDateTime.pas
The TypeError message now correctly reflects that the method accepts both string time zones and ZonedDateTime objects. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
source/units/Goccia.Values.TemporalInstant.pas (1)
525-535: Remove explicitFreecall onTGocciaArgumentsCollection.
TGocciaArgumentsCollectiondescends fromTInterfacedObjectand is reference-counted. The explicitFreecall is not appropriate per project conventions — the object will be automatically freed when the reference goes out of scope.♻️ Suggested fix
function TGocciaTemporalInstantValue.InstantToLocaleString(const AArgs: TGocciaArgumentsCollection; const AThisValue: TGocciaValue): TGocciaValue; var EmptyArgs: TGocciaArgumentsCollection; begin EmptyArgs := TGocciaArgumentsCollection.Create([]); - try - Result := InstantToString(EmptyArgs, AThisValue); - finally - EmptyArgs.Free; - end; + Result := InstantToString(EmptyArgs, AThisValue); end;Based on learnings: "TGocciaArgumentsCollection... descends from TInterfacedObject and is reference-counted, so objects created via CreateWithCapacity or Create are automatically freed when the last reference goes out of scope. An explicit Free call (or wrapping in try/finally) is not appropriate for this type."
🤖 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 525 - 535, In TGocciaTemporalInstantValue.InstantToLocaleString replace the manual creation + try/finally + EmptyArgs.Free pattern with a simple local interfaced variable for TGocciaArgumentsCollection so the reference-counting frees it automatically: remove the try/finally and the explicit EmptyArgs.Free, just assign EmptyArgs := TGocciaArgumentsCollection.Create([]) and call InstantToString(EmptyArgs, AThisValue) returning the result; this uses the interface semantics of TGocciaArgumentsCollection and avoids manual freeing.
🤖 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.Values.TemporalInstant.pas`:
- Around line 525-535: In TGocciaTemporalInstantValue.InstantToLocaleString
replace the manual creation + try/finally + EmptyArgs.Free pattern with a simple
local interfaced variable for TGocciaArgumentsCollection so the
reference-counting frees it automatically: remove the try/finally and the
explicit EmptyArgs.Free, just assign EmptyArgs :=
TGocciaArgumentsCollection.Create([]) and call InstantToString(EmptyArgs,
AThisValue) returning the result; this uses the interface semantics of
TGocciaArgumentsCollection and avoids manual freeing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f7df226a-7a05-400b-8d90-01fd798f7ce0
📒 Files selected for processing (1)
source/units/Goccia.Values.TemporalInstant.pas
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
CoerceToISO*) and toString options (ResolveTemporalToStringOptions,RoundTimeForToString,GetCalendarDisplay) to eliminate duplicated parsing logic across all Temporal typestoLocaleString,Symbol.toStringTag,toZonedDateTimeISO,withCalendar,expandrounding modetest262: 6,612 → 6,837 passed (+225), Temporal 1,787 → 2,012 (+225, 42.5% → 47.9%)
Test plan
./build.pas testrunner && ./build/GocciaTestRunner tests --asi --unsafe-ffi— 5,739 pass, 5 fail (pre-existing FFI fixture)./format.pas --check— 274 files, all clean🤖 Generated with Claude Code