Mark BalancePossiblyInfiniteTimeDuration and CreateTimeDurationRecord as infallible

[anba]

4edfed1 can be seen as either normative or editorial. For actual implementations it's definitely just an editorial change.

[ptomato]

Thanks. I am trying to get rid of all possibly-infinite loops as part of the last revision putting an upper bound on duration times, but I didn't realize the loops in NanosecondsToDays could go infinitely.

Do you have any suggestions for limiting these?

[ptomato]

Oof, I didn't clock that the loop above this could be made infinite with a special calendar/time zone combination.

I think that in built-in time zones and calendars it's only possible for the loop body to execute once, because days would be at most one day longer if start was at, say, 23:59 and end was at 00:00. Is that your understanding as well?

[ptomato]

I did a bit more digging into this. In the existing test suite, the above loop is only ever executed at most once, except in tests that you wrote which were contrived specifically for this loop. However, the comment in the polyfill code suggests that it could loop twice for built-ins under some circumstances:

If clock time after addition was in the middle of a skipped period, the endpoint was disambiguated to a later clock time. So it's possible that the resulting disambiguated result is later than endNs. If so, then back up one day and try again. Repeat if necessary (some transitions are > 24 hours) until either there's zero days left or the date duration is back inside the period where it belongs. Note that this case only can happen for positive durations because the only direction that disambiguation: 'compatible' can change clock time is forwards.

So it seems that either the comment is wrong, or we don't have coverage in test262 for that case. I wonder if it's safe to throw after 2 iterations or there could still be some situation we don't know about with built-in time zones / calendars, or non-contrived use cases with custom time zones or calendars. I believe this code was originally written by @justingrant who may have some insight?

The second loop, in step 13, is executed at most twice in test262 (also not counting the contrived tests). Here also I wonder if we could cut it off after 2 iterations.

[anba]

The test case for 4edfed1 iterates over the second loop in NanosecondsToDays (step 17).

The comment you've mentioned from the polyfill implementation of the first loop of NanosecondsToDays (step 13) mentions that transitions >24h are possible:

proposal-temporal/polyfill/lib/ecmascript.mjs

Lines 3165 to 3172 in bea08cd

	// If clock time after addition was in the middle of a skipped period, the
	// endpoint was disambiguated to a later clock time. So it's possible that
	// the resulting disambiguated result is later than endNs. If so, then back
	// up one day and try again. Repeat if necessary (some transitions are
	// > 24 hours) until either there's zero days left or the date duration is
	// back inside the period where it belongs. Note that this case only can
	// happen for positive durations because the only direction that
	// `disambiguation: 'compatible'` can change clock time is forwards.

This doesn't seem to be the case for built-in time zones. Executing this test shows a couple of 24h transitions, but no transitions exceeding 24 hours. (Note: The polyfill implementation shows fewer transitions than the SM/V8 implementations, because transitions before 1847-01-01 are ignored in the polyfill, cf. BEFORE_FIRST_DST variable.)

const start = Temporal.Instant.from("1800-01-01T00:00:00Z");
const end = Temporal.Instant.from("2100-01-01T00:00:00Z");
const nanosPerDay = Temporal.Duration.from("P1D").total("nanoseconds");
const oneWeek = Temporal.Duration.from("P7D").round({largestUnit: "hours"});

for (let id of Intl.supportedValuesOf("timeZone")) {
  let tz = new Temporal.TimeZone(id);

  let t = start;
  while (true) {
    let next = tz.getNextTransition(t);
    if (next === null || Temporal.Instant.compare(next, end) >= 0) {
      break;
    }
    t = next;

    // Don't test directly at the transition point.
    let before = tz.getOffsetNanosecondsFor(t.subtract(oneWeek));
    let at = tz.getOffsetNanosecondsFor(t.add(oneWeek));

    let diff = before - at;
    if (diff === nanosPerDay || diff === -nanosPerDay) {
      console.log("24h:", id, next, before, at);
    }
    if (diff > nanosPerDay || diff < -nanosPerDay) {
      console.log(">24h:", id, next, before, at);
    }
  }
}

[ptomato]

I guess the stock exchange time zone use case would require the ability to have a transition >24h.

[justingrant]

I guess the stock exchange time zone use case would require the ability to have a transition >24h.

This is correct. I think it'd be fine to impose some kind of limit on transitions, but 24 hours seems too short for a limit because it forecloses cases like the stock exchange zone. I could possibly imagine time zones being used for similar cases like school schedules, e.g. grades are assigned to an "instructional day" which can't exist during summer break in the school calendar.

So if we do impose a limit (which I agree would be a reasonable thing to do), then maybe be conservative and set the limit to something like a year (maybe 500 days to accommodate hypothetical weird lunisolar calendars)?

[ptomato]

In theory I agree, but now I'm not sure that it really forecloses these cases! I made a local modification which would throw if either of these loops iterated more than once, and the stock exchange time zone continued to work fine. Even when I added more arithmetic:

More stock exchange tests

// Monday lasts 24 hours
const monday = Temporal.ZonedDateTime.from('2022-08-22T09:30[America/New_York]').withTimeZone(tzNYSE);
assert.equal(monday.hoursInDay, 24);

// Friday lasts 72 hours, until Monday starts again
const friday = monday.add({ days: 4 });
assert.equal(friday.hoursInDay, 72);

// Adding 1 day to Friday gets you the next Monday (disambiguates forward)
assert.equal(friday.add({ days: 1 }).toString(), '2022-08-29T09:30:00-04:00[NYSE]');
// Adding 3 days to Friday also gets you the next Monday
assert.equal(friday.add({ days: 3 }).toString(), '2022-08-29T09:30:00-04:00[NYSE]');

const nextMonday = monday.add({ weeks: 1 });

// Subtracting 1 day from Monday gets you the same day (disambiguates forward)
assert.equal(nextMonday.subtract({ days: 1 }).toString(), '2022-08-29T09:30:00-04:00[NYSE]');
// Subtracting 3 days from Monday gets you the previous Friday
assert.equal(nextMonday.subtract({ days: 3 }).toString(), '2022-08-26T09:30:00-04:00[NYSE]');

// Difference between Friday and Monday is 72 hours or 3 days
const fridayUntilMonday = friday.until(nextMonday);
assert.equal(fridayUntilMonday.toString(), 'PT72H');
assert.equal(fridayUntilMonday.total('hours'), 72);
assert.equal(fridayUntilMonday.total('days'), 3);

const mondaySinceFriday = nextMonday.since(friday);
assert.equal(mondaySinceFriday.toString(), 'PT72H');
assert.equal(mondaySinceFriday.total('hours'), 72);
assert.equal(mondaySinceFriday.total('days'), 3);

// One week is still 7 days
const oneWeek = Temporal.Duration.from({ weeks: 1 });
assert.equal(oneWeek.total({ unit: 'days', relativeTo: monday }), 7);
assert.equal(oneWeek.total({ unit: 'days', relativeTo: friday }), 7);

So how about this. I'd be in favour of accepting this PR for now, including the check for infinite days which I'd assume is not normative since as Anba points out, it doesn't affect implementations.

Then, in my normative PR for removing loops and BigInt arithmetic, which I'm planning to open today, I'll put upper limits on these loops that throw if iterated more than once, which is a normative change. If we find a non-contrived use case that throws there, or find that it precludes one of our existing use cases after all, then we can always withdraw that commit from the PR, or put a limit of 365 or 500 instead. Hopefully the public code review gets us many eyes on it 😄

[justingrant]

Oh, weird. Not sure why those tests work (and can't check right now) but if they work then I agree with you!

[ptomato]

I added it to the agenda for the next Temporal meeting - I won't be there though.

[codecov]

Codecov Report

Merging #2611 (d11b2de) into main (c895534) will decrease coverage by 0.08%.
The diff coverage is n/a.

❗ Current head d11b2de differs from pull request most recent head 1bd6156. Consider uploading reports for the commit 1bd6156 to get more accurate results

@@            Coverage Diff             @@
##             main    #2611      +/-   ##
==========================================
- Coverage   96.06%   95.98%   -0.08%     
==========================================
  Files          20       20              
  Lines       11553    11573      +20     
  Branches     2195     2198       +3     
==========================================
+ Hits        11098    11108      +10     
- Misses        391      401      +10     
  Partials       64       64              

see 8 files with indirect coverage changes

[ptomato]

If the second commit is indeed not normative, could you remove the "Normative:" from the commit message before merging?

[anba]

If the second commit is indeed not normative, could you remove the "Normative:" from the commit message before merging?

It's normative when reading the spec text with a pure mathematical mindset, because just based on the math, it's possible to iterate from 0 to 10²⁹². Physically it's impossible to do so, though.

[justingrant]

What's the latest on this PR? It's been open for a while.

[ptomato]

I made the change to the commit message and will now merge it.