RoundDuration: Fractional parts of "days" variable

[anba]

The following test case fails in the spec polyfill, because the fractional part of the days variable in RoundDuration is ignored.

Relevant steps:

6. If unit is one of "year", "month", "week", or "day", then
 ...
 e. Set days to days + result.[[Days]] + result.[[Nanoseconds]] / result.[[DayLength]].
 ...
11. Else if unit is "week", then
 ...
 g. Repeat, while abs(days) ≥ abs(oneWeekDays),
 ...

Note 1: This case can only happen a user-defined calendar overrides dateAdd, because it requires that the fractional part has a different sign than the integer part of days.
Note 2: Also applies to the loop conditions in steps 9-10 and the truncate(days) call in step 9.i.

Should this be fixed in the polyfill or do we want to change the spec to keep the fractional part result.[[Nanoseconds]] / result.[[DayLength]] in a separate variable, so we don't have to worry about it in comparisons and the truncate call?

function assertEq(actual, expected, message = "") {
  if (!Object.is(actual, expected)) {
    console.log(`${message}: ${actual} != ${expected}`);
  }
}

let dateAddCalledWithMonth = 0;
let dateAddCalledWithWeeks = 0;

let cal = new class extends Temporal.Calendar {
  dateAdd(date, duration, options) {
    if (duration.toString() === "P1M") {
      dateAddCalledWithMonth++;
      duration = "-P7D";
    } else {
      dateAddCalledWithWeeks++;
      assertEq(duration.toString(), "-P1W", "duration is one week");
    }
    return super.dateAdd(date, duration, options);
  }
}("iso8601");

let d = Temporal.Duration.from({
  months: 1,
  hours: 10,
});

let r = d.round({
  smallestUnit: "weeks",
  largestUnit: "weeks",
  relativeTo: new Temporal.PlainDate(1970, 1, 1, cal),
});

assertEq(r.toString(), "-P1W", "result");
assertEq(dateAddCalledWithMonth, 1, "dateAddCalledWithMonth");
assertEq(dateAddCalledWithWeeks, 2, "dateAddCalledWithWeeks");

Test case for the truncate call:

function assertEq(actual, expected, message = "") {
  if (!Object.is(actual, expected)) {
    console.log(`${message}: ${actual} != ${expected}`);
  }
}

let expectedDurations = [
  "PT0S",
  "P1M",
  "-P6D",  // "-P7D" when the fractional part is ignored.
  "PT0S",
  "-P1Y",
];

let cal = new class extends Temporal.Calendar {
  dateAdd(date, duration, options) {
    assertEq(duration.toString(), expectedDurations.shift(), "duration");
    if (duration.toString() === "P1M") {
      duration = "-P7D";
    }
    return super.dateAdd(date, duration, options);
  }
}("iso8601");

let d = Temporal.Duration.from({
  months: 1,
  hours: 10,
});

let r = d.round({
  smallestUnit: "years",
  largestUnit: "years",
  relativeTo: new Temporal.PlainDate(1970, 1, 1, cal),
});

[ptomato]

I haven't had time to look carefully, but is this a regression from #2454?

[anba]

No, it's not a regression from that PR.

[Mrashes]

Hey, I have some free time and want to contribute - mind if I look into this bug?

[ptomato]

@Mrashes thanks for the offer, that's very kind! I think that would be great.

Note that #2519, #2571, and #2612 are pending to be merged once they make their way through review and gain enough test coverage, and they will affect this part of both the spec and the reference polyfill. So I'd recommend that you base your work on top of that.

Probably it's best to investigate first if this can be solved with only a fix to the polyfill.

I think this might also be related to @anba's test case in tc39/test262#3933.

[Mrashes]

@ptomato On digging into this I think i'm confused - seems like all tests are passing ok. Unsure if this is related to test262 or the cookbook as well but I think that due to my inexperience in the code base.

My ten cents is this should be fixed in the polyfill as the use case sounds pretty edge case and adding the fraction could be confusing in the actual spec.

[ptomato]

@Mrashes Thanks for taking a look and sorry I didn't respond earlier. The test cases that aren't passing are the ones that Anba provided in this issue. They aren't (yet) part of the test suite. That probably means a gap in test262 coverage, which we should address.

[ptomato]

This seems to no longer be a problem after merging the pending PRs that I mentioned above. We now have two variables, days (an integer) and fractionalDays (possibly not an integer) and the loops are gone.