Duration rounding throws an error

[gibson042]

I haven't checked if this also appears in the spec, but the polyfill at least throws a RangeError when it should not:

const dur = Temporal.Duration.from({ months: 11 });
dur.round({ relativeTo: "2023-05-31", smallestUnit: "days", roundingMode: "ceil" });
// => P10M30D
dur.round({ relativeTo: "2023-05-31", smallestUnit: "months", roundingMode: "ceil" });
// => RangeError: custom calendar reported a month that is 0 days long at NudgeToCalendarUnit

[ptomato]

This is a regression from #2884. So while the bad news is that we may have inadvertently introduced a bug, the good news is that there wasn't TC39 consensus on the buggy behaviour, so we can just make it work again without having to go to plenary.

I'll investigate whether the best fix is a revert or something else, because there is definitely some stuff from #2884 that we should keep.

[anba]

Temporal.PlainDate.from("2023-05-31").until("2024-04-30", {largestUnit: "month"}).toString()

returns "P10M30D", but used to return "P11M" at some point. SpiderMonkey still uses the old definition of DifferenceISODate, which is probably why it doesn't throw a RangeError for the round call.

---

Temporal.Duration.prototype.round calls DifferencePlainDateTimeWithRounding with one = 2023-05-31 and two = 2024-04-30. DifferenceISODateTime(one, two, largestUnit = "months") returns P10M30D. So RoundRelativeDuration is called with duration = P10M30D and increment = 1. In NudgeToCalendarUnit, r1 = 10 (months from P10M30D) and r2 = 11. Adding r2 months to the start date ("2023-05-31") returns the end date ("2024-04-30"), which leads to evaluating true for destEpochNs ≥ endEpochNs in NudgeToCalendarUnit, step 13.a.

If DifferenceISODateTime had returned "P11M" instead of "P10M30D", then r1 = 11 and r2 = 12, and everything would have worked correctly.

[arshaw]

We probably just need to relax the assertions. See #2924

[anba]

No, we can't just relax these range checks, because they ensure that ApplyUnsignedRoundingMode can be called without hitting assertions within ApplyUnsignedRoundingMode, specifically the Assert: r1 < x < r2. step.

Edit: I didn't see that you also propose to change ApplyUnsignedRoundingMode. I don't have enough time to check if ApplyUnsignedRoundingMode can be changed to allow r1 ≤ x ≤ r2 instead of the current requirement r1 ≤ x < r2.

That still leaves the question unanswered if Temporal.PlainDate.from("2023-05-31").until("2024-04-30", {largestUnit: "month"}).toString() should return "P10M30D" or "P11M". I'd prefer if this question is first answered before applying #2924.

[arshaw]

Hi @anba, apologies for the narrow response. In the last champions meeting I was tasked with figuring out why the reference implementation throws an error while fullcalendar's temporal-polyfill does not.

To add more context: the intermediate P10M30D result is correct. It was decided in this ticket (and this PR) earlier this year. In essence, the lexically smallest possible balanced duration should be returned. CC @BurntSushi

Given the mechanics of both .until() and Duration rounding (refactored earlier this year), we need to ensure all downstream computations are compatible with r1 ≤ x ≤ r2. I'll devote some time to this later today or tomorrow.

[arshaw]

@ptomato, I was wondering if you could help out with this. The question is whether having numerator === denominator is acceptable at this point:

proposal-temporal/polyfill/lib/ecmascript.mjs

Lines 3895 to 3920 in 3b47598

	const numerator = TimeDuration.fromEpochNsDiff(destEpochNs, startEpochNs);
	const denominator = TimeDuration.fromEpochNsDiff(endEpochNs, startEpochNs);
	const unsignedRoundingMode = GetUnsignedRoundingMode(roundingMode, sign < 0 ? 'negative' : 'positive');
	const cmp = numerator.add(numerator).abs().subtract(denominator.abs()).sign();
	const even = (MathAbs(r1) / increment) % 2 === 0;
	const roundedUnit = numerator.isZero()
	? MathAbs(r1)
	: ApplyUnsignedRoundingMode(MathAbs(r1), MathAbs(r2), cmp, even, unsignedRoundingMode);
	// Trick to minimize rounding error, due to the lack of fma() in JS
	const fakeNumerator = new TimeDuration(denominator.totalNs.times(r1).add(numerator.totalNs.times(increment * sign)));
	const total = fakeNumerator.fdiv(denominator.totalNs);
	if (MathAbs(total) < MathAbs(r1) || MathAbs(total) >= MathAbs(r2)) {
	throw new Error('assertion failed: r1 ≤ total < r2');
	}
	// Determine whether expanded or contracted
	const didExpandCalendarUnit = roundedUnit === MathAbs(r2);
	duration = { ...(didExpandCalendarUnit ? endDuration : startDuration), norm: TimeDuration.ZERO };
	return {
	duration,
	total,
	nudgedEpochNs: didExpandCalendarUnit ? endEpochNs : startEpochNs,
	didExpandCalendarUnit
	};

To my eyes, the computation of cmp and the call to ApplyUnsignedRoundingMode are probably fine.

I am a bit confused about what the cmp value actually signifies, but I'm guessing it's supposed to be the sign of numerator / denominator while using a hack to avoid precision loss? If that's the case, and hypothetically numerator=-2 and denominator=-4, won't that result in cmp=0?

[arshaw]

Hey @ptomato, any feedback on this?

[ptomato]

@arshaw Sorry about dropping the ball on this.

ApplyUnsignedRoundingMode is written a bit differently in ECMA-402 than it is implemented in code. That's because I tried to find a way that would work for rounding bigints as well as FP numbers. The code

const cmp = numerator.add(numerator).abs().subtract(denominator.abs()).sign();
if (cmp < 0) return r1;
if (cmp > 0) return r2;

is intended to have the same effect as the spec steps
6. Let d1 be x – r1.
7. Let d2 be r2 – x.
8. If d1 < d2, return r1.
9. If d2 < d1, return r2.

When cmp === 0 that means that x is exactly as far away from r1 as from r2, in other words a tie so you need to consult the rounding mode's tie-breaking behaviour.

I wonder if it'd be simpler to leave ApplyUnsignedRoundingMode unchanged and instead treat x === r2 as a special case, returning endEpochNs if destEpochNs === endEpochNs. I haven't tested that idea out, though.

[arshaw]

@ptomato, thank you for that explanation. All understood. I've updated the PR to special-case x === r2, leaving ApplyUnsignedRoundingMode unchanged. Feels fine because it's similar to how x === r1 is already special cased.