Fixes Temporal Duration total edge case and GC collection

[frostney]

Summary by CodeRabbit

• Bug Fixes
  • Temporal.Duration.total() now enforces that durations containing years or months require a relativeTo reference and throws a clear error when missing or unsupported, with improved validation and messaging.
• Tests
  • Added tests verifying total() throws when years or months are present (including combined-component cases) to ensure correct behavior.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

---

📝 Walkthrough

Walkthrough

Adds tests and runtime guards for Temporal.Duration.total to forbid totals when years/months are present without relativeTo, and changes Temporal initialization and PlainDate subtraction to use temporary GC roots for safer lifetime management.

Changes

Cohort / File(s)	Summary
Tests tests/built-ins/Temporal/duration.js	Adds assertions that Duration.prototype.total() throws RangeError when called on durations containing years or months (and when mixed with other units) without an appropriate relativeTo.
Duration total guard units/Goccia.Values.TemporalDuration.pas	Parses options object for unit and optional relativeTo; enforces that durations containing years or months require relativeTo (or otherwise throw) and updates error messaging and guard logic before computing total.
Temporal builtin initialization (GC rooting) units/Goccia.Builtins.Temporal.pas	Wraps temporal namespace setup in a temporary GC root via TGocciaGC.Instance.AddTempRoot/RemoveTempRoot and moves lexical binding inside the guarded initialization block; adds GC unit to uses.
PlainDate memory handling (GC root) units/Goccia.Values.TemporalPlainDate.pas	Replaces immediate argument allocation/free with adding NegatedDur as a temporary GC root while building inner Args for DateAdd, adjusting finally blocks to remove the temp root and explicitly free inner collections.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• Add Temporal API #3: Modifies the same Temporal Duration total logic and tests, related to the years/months guard and test coverage added here.

Poem

🐇 In rabbit hops through temporal fields I go,

I guard the months and years from flowing so;
GC roots snug, durations held tight,
total() knows when it's wrong or right,
A carrot for tests that catch the light.

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the two main changes: fixing an edge case in Temporal Duration's total() method and improving garbage collection handling across multiple files.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix-temporal-issues

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[github-actions]

Benchmark Results

113 benchmarks · 113 unchanged · avg +0.1%

arrays.js — 11 unchanged · avg -0.2%

Benchmark	Base (ops/sec)	PR (ops/sec)	Change
Array.from length 100	145,325	144,354	-0.7%
Array.of 10 elements	160,888	159,815	-0.7%
spread into new array	170,224	168,573	-1.0%
map over 50 elements	91,269	91,481	+0.2%
filter over 50 elements	90,285	91,466	+1.3%
reduce sum 50 elements	95,848	95,845	-0.0%
forEach over 50 elements	95,032	95,266	+0.2%
find in 50 elements	99,809	101,791	+2.0%
sort 20 elements	4,177	4,118	-1.4%
flat nested array	81,227	80,371	-1.1%
flatMap	39,858	39,480	-0.9%

classes.js — 10 unchanged · avg +0.5%

Benchmark	Base (ops/sec)	PR (ops/sec)	Change
simple class new	74,621	75,094	+0.6%
class with defaults	57,042	57,236	+0.3%
50 instances via Array.from	79,605	80,025	+0.5%
instance method call	34,451	34,813	+1.1%
static method call	60,220	60,282	+0.1%
single-level inheritance	29,269	29,360	+0.3%
two-level inheritance	27,537	27,490	-0.2%
private field access	36,617	36,801	+0.5%
private methods	42,427	42,620	+0.5%
getter/setter access	38,628	39,204	+1.5%

closures.js — 11 unchanged · avg -0.2%

Benchmark	Base (ops/sec)	PR (ops/sec)	Change
closure over single variable	57,255	57,162	-0.2%
closure over multiple variables	56,509	57,075	+1.0%
nested closures	61,691	62,265	+0.9%
function as argument	43,216	43,111	-0.2%
function returning function	57,383	57,120	-0.5%
compose two functions	35,156	35,132	-0.1%
fn.call	83,108	83,493	+0.5%
fn.apply	62,185	61,158	-1.7%
fn.bind	72,144	71,320	-1.1%
recursive sum to 50	5,031	5,044	+0.3%
recursive tree traversal	8,879	8,800	-0.9%

collections.js — 12 unchanged · avg +0.2%

Benchmark	Base (ops/sec)	PR (ops/sec)	Change
add 50 elements	84,049	85,226	+1.4%
has lookup (50 elements)	66,970	66,420	-0.8%
delete elements	66,601	65,714	-1.3%
forEach iteration	80,566	82,281	+2.1%
spread to array	88,410	88,965	+0.6%
deduplicate array	56,842	56,041	-1.4%
set 50 entries	83,446	84,268	+1.0%
get lookup (50 entries)	64,852	64,352	-0.8%
has check	71,149	70,350	-1.1%
delete entries	64,086	64,935	+1.3%
forEach iteration	76,783	77,257	+0.6%
keys/values/entries	50,563	51,201	+1.3%

destructuring.js — 14 unchanged · avg -0.8%

Benchmark	Base (ops/sec)	PR (ops/sec)	Change
simple array destructuring	186,202	181,607	-2.5%
with rest element	136,792	134,129	-1.9%
with defaults	187,666	186,214	-0.8%
skip elements	201,518	198,149	-1.7%
nested array destructuring	111,641	110,999	-0.6%
swap variables	218,445	217,941	-0.2%
simple object destructuring	139,584	136,446	-2.2%
with defaults	166,838	164,171	-1.6%
with renaming	162,140	160,428	-1.1%
nested object destructuring	82,615	83,234	+0.7%
rest properties	86,131	86,208	+0.1%
object parameter	51,753	51,543	-0.4%
array parameter	66,606	66,259	-0.5%
mixed destructuring in map	87,023	87,859	+1.0%

fibonacci.js — 3 unchanged · avg +0.7%

Benchmark	Base (ops/sec)	PR (ops/sec)	Change
recursive fib(15)	137	137	+0.4%
recursive fib(20)	12	12	+0.2%
iterative fib(20) via reduce	65,064	66,054	+1.5%

json.js — 11 unchanged · avg +1.5%

Benchmark	Base (ops/sec)	PR (ops/sec)	Change
parse simple object	117,532	121,047	+3.0%
parse nested object	73,893	75,409	+2.1%
parse array of objects	47,079	47,837	+1.6%
parse large flat object	48,812	50,221	+2.9%
parse mixed types	63,139	64,253	+1.8%
stringify simple object	97,533	98,663	+1.2%
stringify nested object	55,949	56,420	+0.8%
stringify array of objects	108,891	108,427	-0.4%
stringify mixed types	47,267	47,614	+0.7%
parse then stringify	36,968	37,546	+1.6%
stringify then parse	46,613	47,330	+1.5%

numbers.js — 11 unchanged · avg -0.4%

Benchmark	Base (ops/sec)	PR (ops/sec)	Change
integer arithmetic	181,538	181,833	+0.2%
floating point arithmetic	204,313	203,646	-0.3%
number coercion	105,453	104,404	-1.0%
toFixed	72,217	73,286	+1.5%
toString	98,782	99,847	+1.1%
valueOf	132,641	132,049	-0.4%
toPrecision	93,941	94,251	+0.3%
Number.isNaN	158,888	156,699	-1.4%
Number.isFinite	152,368	150,247	-1.4%
Number.isInteger	157,345	153,361	-2.5%
Number.parseInt and parseFloat	141,138	140,601	-0.4%

objects.js — 7 unchanged · avg +0.0%

Benchmark	Base (ops/sec)	PR (ops/sec)	Change
create simple object	228,612	225,729	-1.3%
create nested object	120,397	119,492	-0.8%
create 50 objects via Array.from	141,008	141,187	+0.1%
property read	104,363	103,988	-0.4%
Object.keys	73,212	73,008	-0.3%
Object.entries	47,470	48,344	+1.8%
spread operator	91,656	92,504	+0.9%

promises.js — 12 unchanged · avg -0.1%

Benchmark	Base (ops/sec)	PR (ops/sec)	Change
Promise.resolve(value)	305,747	305,729	-0.0%
new Promise(resolve => resolve(value))	119,791	120,107	+0.3%
Promise.reject(reason)	316,668	318,170	+0.5%
resolve + then (1 handler)	101,588	102,453	+0.9%
resolve + then chain (3 deep)	42,620	42,654	+0.1%
resolve + then chain (10 deep)	87,729	87,817	+0.1%
reject + catch + then	63,009	62,849	-0.3%
resolve + finally + then	56,032	55,810	-0.4%
Promise.all (5 resolved)	22,996	22,583	-1.8%
Promise.race (5 resolved)	23,711	23,784	+0.3%
Promise.allSettled (5 mixed)	18,230	18,336	+0.6%
Promise.any (5 mixed)	22,726	22,511	-0.9%

strings.js — 11 unchanged · avg +0.0%

Benchmark	Base (ops/sec)	PR (ops/sec)	Change
string concatenation	248,317	247,168	-0.5%
template literal	245,294	245,382	+0.0%
string repeat	239,236	239,822	+0.2%
split and join	102,940	102,149	-0.8%
indexOf and includes	108,510	107,897	-0.6%
toUpperCase and toLowerCase	155,627	157,634	+1.3%
slice and substring	96,246	94,924	-1.4%
trim operations	115,488	116,876	+1.2%
replace and replaceAll	136,822	136,909	+0.1%
startsWith and endsWith	86,317	86,442	+0.1%
padStart and padEnd	123,585	124,168	+0.5%

_{Measured on ubuntu-latest x64. Changes within ±7% are considered insignificant.}

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

units/Goccia.Values.TemporalDuration.pas (1)

513-529: ⚠️ Potential issue | 🟠 Major

Extract and validate relativeTo before checking year/month duration in total()

The guard at lines 528–529 throws "requires relativeTo" but the preceding code never reads relativeTo from the options object. Calls like duration.total({ unit: "days", relativeTo }) will incorrectly throw the error even though relativeTo was provided. Additionally, the subsequent calculation ignores years/months without relativeTo support (which is scientifically invalid—month-to-day conversion requires date context).

Extract relativeTo from the options object and either implement year/month calculation with it, or throw a clearer "not supported yet" error before the computation.

Suggested approach

  else if Arg is TGocciaObjectValue then
  begin
+   OptsObj := TGocciaObjectValue(Arg);
-   Arg := TGocciaObjectValue(Arg).GetProperty('unit');
+   Arg := OptsObj.GetProperty('unit');
    if (Arg = nil) or (Arg is TGocciaUndefinedLiteralValue) then
      ThrowRangeError('total() requires a unit option');
    UnitStr := Arg.ToStringLiteral.Value;
+   Arg := OptsObj.GetProperty('relativeTo');
+   HasRelativeTo := Assigned(Arg) and not (Arg is TGocciaUndefinedLiteralValue);
  end

-  if (D.FYears <> 0) or (D.FMonths <> 0) then
-    ThrowRangeError('Duration with years or months requires relativeTo for total()');
+  if (D.FYears <> 0) or (D.FMonths <> 0) then
+  begin
+    if not HasRelativeTo then
+      ThrowRangeError('Duration with years or months requires relativeTo for total()')
+    else
+      ThrowRangeError('Duration.total with relativeTo is not supported yet');
+  end;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@units/Goccia.Values.TemporalDuration.pas` around lines 513 - 529, The total()
implementation reads the 'unit' from Arg when Arg is a TGocciaObjectValue but
never extracts or validates a 'relativeTo' property before rejecting durations
with years/months; update the TGocciaObjectValue branch to also call
GetProperty('relativeTo') (e.g. store to a local like RelToArg), treat nil or
TGocciaUndefinedLiteralValue as “no relativeTo provided”, and only
ThrowRangeError('Duration with years or months requires relativeTo for total()')
if D.FYears or D.FMonths are non‑zero and RelToArg is absent; if RelToArg is
present, pass it into the subsequent year/month conversion path or throw a
clearer “not supported yet” error before attempting conversion.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@units/Goccia.Values.TemporalDuration.pas`:
- Around line 513-529: The total() implementation reads the 'unit' from Arg when
Arg is a TGocciaObjectValue but never extracts or validates a 'relativeTo'
property before rejecting durations with years/months; update the
TGocciaObjectValue branch to also call GetProperty('relativeTo') (e.g. store to
a local like RelToArg), treat nil or TGocciaUndefinedLiteralValue as “no
relativeTo provided”, and only ThrowRangeError('Duration with years or months
requires relativeTo for total()') if D.FYears or D.FMonths are non‑zero and
RelToArg is absent; if RelToArg is present, pass it into the subsequent
year/month conversion path or throw a clearer “not supported yet” error before
attempting conversion.