RoundRelativeDuration: Disallow edge case in roundingIncrement within year/month

[arshaw]

This ticket is prompted by fullcalendar/temporal-polyfill#43

The new diffing/rounding algorithm always rounds the smallestUnit within the scope of the parent unit. So, in the following example, the unrounded P2M4D has its 4D rounded according to roundingMode/roundingIncrement. It does NOT somehow round 65 days against the 35 roundingIncrement:

let d1 = Temporal.Duration.from({days: 65});
let dt1 = Temporal.PlainDateTime.from('2024-06-19 00:00:00');
let d2 = d1.round({
  relativeTo: dt1,
  largestUnit: 'year',
  smallestUnit: 'day',
  roundingIncrement: 35,
});
console.log(d2.toString());
// current result: P2M
// previous result before diffing/rounding refactor: 2M9D

When rounding down things are fine, but rounding up could yield some strange results:

let d2 = d1.round({
  relativeTo: dt1,
  largestUnit: 'year',
  smallestUnit: 'day',
  roundingIncrement: 200, // !!!
  roundingMode: 'ceil', // !!!!
});
console.log(d2.toString()); // P3M

let d2 = d1.round({
  relativeTo: dt1,
  largestUnit: 'year',
  smallestUnit: 'day',
  roundingIncrement: 100000, // !!!
  roundingMode: 'ceil', // !!!!
});
console.log(d2.toString()); // P1Y

As you can see, it's possible for a unit to not neatly nest within it's parent unit during rounding. The way BubbleRelativeDuration is designed, it only bubbles parent units by 1 maximum (that's why you get P1Y).

Does anyone have any thoughts about this? Should we disallow non-1 roundingIncrements when dealing with year/month? I'm hesitant to refactor the diffing/rounding algorithm to accommodate this edge case.

[justingrant]

Should we disallow non-1 roundingIncrements when dealing with year/month? I'm hesitant to refactor the diffing/rounding algorithm to accommodate this edge case.

My vague memory is that we already have this requirement. @ptomato will know for sure.

[ptomato]

Should we disallow non-1 roundingIncrements when dealing with year/month? I'm hesitant to refactor the diffing/rounding algorithm to accommodate this edge case.

My vague memory is that we already have this requirement. @ptomato will know for sure.

That's the requirement when rounding dates and times. Not when rounding durations: we considered it perfectly reasonable to want to round a duration to a multiple of, say, 10 years. (duration.round({ smallestUnit: 'years', roundingIncrement: 10 }) doesn't have this problem.

[ptomato]

We discussed this in the Temporal meeting of 2024-07-11.

I'm pretty sure we just never considered this case. (I'd guess that's why it has no test coverage: if it did, someone would've noticed the results were weird.) We did consider things like rounding a duration to a multiple of 10 years (like I described in the previous comment) or 5 months or whatever, but I think we just never thought about what should happen if largestUnit was larger than the unit being rounded.

There were two solutions suggested:

1. Rounding a duration to a multiple of a calendar unit truncates at the next highest unit when the unit bubbles up. For example:

Temporal.Duration.from('P1Y6M').round({smallestUnit: 'months', roundingIncrement: 7, relativeTo: '2024-01-01'})
// => 7 months
Temporal.Duration.from('P1Y8M').round({smallestUnit: 'months', roundingIncrement: 7, relativeTo: '2024-01-01'})
// => 1 year

2. Disallow this case. (That is, when smallestUnit is a calendar unit, largestUnit ≠ smallestUnit, and roundingIncrement ≠ 1, throw.)

We decided on option (2) because:

• The original report was because someone was implementing the algorithm and looking for edge cases, not from a real-world use case.
• It's hard to tell what someone encountering this edge case might be expecting, and if we wanted to allow it we should properly investigate it.
• We should avoid adding more complexity to the duration rounding algorithm.

[gibson042]

I dispute the ambiguity here—duration rounding is fully defined by relativeTo, smallestUnit, roundingIncrement, and roundingMode, with the result always being a (roundingIncrement × integer) multiple of smallestUnit, and largestUnit only applying balancing after that (e.g., adding largestUnit: 'year' would change P16M output to P1Y4M but not to P1Y). Or expressed as an invariant, d.round({ relativeTo, smallestUnit, roundingIncrement, roundingMode, largestUnit }) is equivalent to d.round({ relativeTo, smallestUnit, roundingIncrement, roundingMode }).round({ relativeTo, largestUnit })¹ (the latter separating balancing into a distinct method invocation).

Elaborating on expected results for the reported scenarios:

const showRounding = (d, relativeTo, smallestUnit, roundingIncrement, roundingMode) => {
  const largestUnit = 'year';
  const opts = { relativeTo, smallestUnit, roundingIncrement, roundingMode };
  const unbalanced = d.round(opts);
  const balanced = unbalanced.round({ relativeTo, largestUnit });
  console.log(`${unbalanced} (${balanced}) === ${d.round({ ...opts, largestUnit })}`);
};


const dur1 = Temporal.Duration.from({ days: 65 });
const pdt1 = Temporal.PlainDateTime.from('2024-06-19 00:00:00');

showRounding(dur1, pdt1, 'day', 1);
// P65D (P2M4D) === P2M4D
showRounding(dur1, pdt1, 'day', 35);
// P70D (P2M9D) === P2M9D

showRounding(dur1, pdt1, 'day', 1, 'ceil');
// P65D (P2M4D) === P2M4D
showRounding(dur1, pdt1, 'day', 200, 'ceil');
// P200D (P6M17D) === P6M17D
showRounding(dur1, pdt1, 'day', 100000, 'ceil');
// P100000D (P273Y9M16D) === P273Y9M16D


const dur2 = Temporal.Duration.from({ months: 9 });
const pd2 = '2024-01-01';
showRounding(dur2, pd2, 'month', 8, 'ceil');
// P16M (P1Y4M) === P1Y4M

A cause of e.g. the 200-days case in the original report above is NudgeToDayOrTime output including a Normalized Duration Record for P2M200D rather than P200D, a problem that is shared with at least NudgeToCalendarUnit and possibly also NudgeToZonedTime—which stems from failure to convert the input duration into one that is top-heavy at the input unit—and then BubbleRelativeDuration being wholly inadequate to the task of proper balancing (which also breaks the 8-months case).

This is fixable by actually using the calendar methods (which thankfully is now unobservable), but all the same I don't object to rejecting affected input for now. Mostly I just don't want the ambiguity claim to stand.

---

To that end, though, I checked for discrepancies just with roundingIncrement: 1, and found them (but only with smallestUnit "weeks"¹).

((limitTime, limitCount = Infinity) => {
  const randInt = n => Math.floor(Math.random() * n);
  const choose = arr => arr[randInt(arr.length)];
  const pdCompare = Temporal.PlainDate.compare;

  const units = ["days", "weeks", "months", "years"];
  const largestUnit = "year";
  const dates = Array(100).fill().map(() => {
    return Temporal.PlainDate.from(
      { year: 2000 + randInt(25), month: randInt(12) + 1, day: randInt(31) + 1 },
      { overflow: "constrain" },
    );
  });

  let count = 0;
  const timeout = performance.now() + limitTime;
  while (performance.now() < timeout && count < limitCount) {
    const dur = Temporal.Duration.from({ [choose(units)]: randInt(100) });
    const relativeTo = choose(dates);
    const smallestUnit = choose(units);
    const roundingIncrement = /*randInt(100) + 1*/ 1;
    const opts = { relativeTo, smallestUnit, roundingIncrement, roundingMode: "ceil" };
    const fullOpts = { ...opts, largestUnit };
    try {
      const unbalanced = dur.round(opts);
      const balanced = unbalanced.round({ relativeTo, largestUnit });
      const combined = dur.round(fullOpts);
      const atUnbalanced = relativeTo.add(unbalanced);
      const atBalanced = relativeTo.add(balanced);
      const atCombined = relativeTo.add(combined);
      if (!pdCompare(atUnbalanced, atBalanced) && !pdCompare(atBalanced, atCombined)) {
        continue;
      }
      console.log({ dur, fullOpts, unbalanced, balanced, combined });
    } catch (err) {
      continue; // https://github.com/tc39/proposal-temporal/issues/2919
      console.error({ dur, fullOpts });
      throw err;
    }
    count++;
  }
})(60000, 10)

For example, P60W relative to 2017-03-11 with smallestUnit "weeks" and roundingMode "ceil" gives P60W without largestUnit but P1Y1M4W with largestUnit "year", and P56D relative to 2015-05-02 with smallestUnit "weeks" and roundingMode "ceil" gives P8W without largestUnit but P1M4W with largestUnit "year". It's odd that values like P60W and P56D—which do divide evenly into weeks—round to values that don't, but it seems fair to say that the explicit non-"week" largestUnit opts in to such divergence as whole weeks from a month boundary¹.

I also found another issue in the same neighborhood that seems to actually need a fix: #2919

Footnotes

1. In a calendar where weeks do not evenly divide months (notably including the ISO 8601 calendar), we have implicitly chosen to have largestUnit bigger than "week" apply rounding logic only past a month boundary, thereby violating the invariant. ↩ ↩² ↩³

[gibson042]

For future reference, the potential ambiguity stems from a conception of rounding with largestUnit as a request for a balanced duration with smallestUnit component that satisfies the specified multiple (e.g., P9M ceil-rounded to smallestUnit "month" with increment 8 and largestUnit "year" would be the next-highest [per "ceil"] aYbM duration for which b mod 8 is 0, which would always be P1Y0M and thus P1Y).

But note that such an approach would make large increments meaningless (e.g., increment 400 days or 60 weeks or 13 months with largestUnit "year" would always result in output having a zero-valued component for the respective smallestUnit) and would for some inputs produce very large inflation (e.g., 8-week ceil-rounding with largestUnit "month" would produce P1M8W from P5W relativeTo 2024-01-01 or P4W1D relativeTo 2025-02-01 [because the simple interpretation of those duration–relativeTo pairs is a duration just slightly greater than one month past the relativeTo]).