Change NudgeToCalendarUnit to use relative date when comparing durations

[catamorphism]

See #3168

[codecov]

Codecov Report

❌ Patch coverage is 56.81818% with 38 lines in your changes missing coverage. Please review.
✅ Project coverage is 96.49%. Comparing base (0df570c) to head (5dd0b0d).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
polyfill/lib/ecmascript.mjs	56.81%	38 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3172      +/-   ##
==========================================
- Coverage   96.84%   96.49%   -0.35%     
==========================================
  Files          22       22              
  Lines       10235    10305      +70     
  Branches     1841     1846       +5     
==========================================
+ Hits         9912     9944      +32     
- Misses        273      311      +38     
  Partials       50       50              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[ptomato]

So I haven't sat down yet to think through the code but I did run this on my in-progress snapshot tests. It definitely fixes the assertion failures, and the results seem correct:

Temporal.Duration.from({ months: 1, hours: 10 }).round({ smallestUnit: 'months', roundingMode: 'expand', relativeTo: '2020-01-31' })
  // old: assertion failure
  // new: P2M (seems correct)

Temporal.Duration.from({ years: 2345, hours: 12 }).round({ smallestUnit: 'years', roundingMode: 'expand', relativeTo: '2020-02-29' })
  // old: assertion failure
  // new: P2346Y (seems correct)

Temporal.Duration.from({ months: 1, hours: 10 }).total({ unit: 'months', relativeTo: '2020-01-31' })
  // old: assertion failure
  // new: 1.0134408602150538
  // (seems correct: the bounding box is from 2020-02-29T00:00 (relativeTo + 1 month)
  // to 2020-03-31T00:00 (relativeTo + 2 months), which is 744 hours, and the result is
  // the floating point approximation of 1+10/744)

(Additionally, these cases produced wrong answers in a build of the existing code with assertions disabled)

But I also found one case that seems to produce a wrong result where the old code was correct:

Temporal.Duration.from({ years: 1 }).round({ smallestUnit: 'months', relativeTo: '2020-02-29' })
  // old: P1Y
  // new: P12M (seems wrong, should balance back up to the implicit largestUnit in the original duration)

(I'd have liked to push an in-progress branch with the snapshot tests but currently the testing space is too large, and the snapshots run afoul of GitHub's file size limit. So here's what I've currently got, without snapshots: bd34a53 To recreate the snapshots, run node polyfill/test/thorough/durationrounding.mjs -u and node polyfill/test/thorough/durationtotal.mjs -u, then rebase your fixes on top of it and run them again without -u to compare)

[catamorphism]

But I also found one case that seems to produce a wrong result where the old code was correct:

This should be fixed now.

[ptomato]

@arshaw If you have a moment to weigh in on this, I'd really appreciate it — you probably have a deeper understanding of the NudgeCalendarUnit algorithm than I do.

[ptomato]

Thanks, I went through the spec text and that made me revisit some things that I didn't catch before — sorry about that!

[ptomato]

Thanks!

[ptomato]

Just nitpicks at this point. Ready to present to TC39 in the November meeting.

[catamorphism]

Probably for consistency we should pass them in. (Otherwise it won't follow the same code path as Duration.compare I think?)

@ptomato Okay, done in 62e1ed2

[ptomato]

Draft until presented to TC39

[arshaw]

I finally got a chance to look at this, sorry for the delay. I think it's really good and the same general approach I would have taken, which is to shift the r1->r2 window by one increment if an overflow situation occurs. I feel there might be room for some minor code DRY-ing and optimization however.

I see that an out-of-bounds situation is preemptively detected via compare > CompareDurations. This will call DateDurationDays > CalendarDateAdd. Regardless of whether an overflow happened, when end is computed towards the end of NudgeToCalendarUnit, CalendarDateAdd will be called again. This second call could be avoided if the prior result where somehow saved.

Also, I see that IF an overflow is detected, the logic for shifting the window by an increment is rather repetitive across the unit:year and unit:month cases. It'd be nice if we could unify that.

So, instead of a new CompareDurations abstract operation to achieve unification, I'd propose a different new abstract operation called something like ComputeNudgeWindow. It would pull out most of the logic from here to here and yield a result like { r1, r2, startEpochNs, endEpochNs }. If the NudgeToCalendarUnit caller detects that an overflow happens, a "retry" call to ComputeNudgeWindow could be called with an additional flag/int to imply an additional shift should occur.

I'm interested to hear of @catamorphism and @ptomato think this refactor would be worthwhile. Unfortunately I don't have time to try this myself, and who knows, maybe this new abstraction makes things look overly confusing after all is said and done, but could be worth a shot.

[catamorphism]

@arshaw Thanks! I do think it's better. If you get a chance, take a look at 90bb808, but if not, no worries.

[arshaw]

@catamorphism the new code looks great to me!

I'll leave the nitpicking & bike shedding to @ptomato :)

[ptomato]

This PR reached consensus in the 2025-11-18 TC39 plenary.

I was making one last pass through the spec text and I noticed some spec notation nitpicks. Otherwise good to go.

[ptomato]

A couple of stragglers from the last round of comments.