Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ZonedDateTime.startOfDay: is it guaranteed to be the first instant in a calendar day? #2910

Open
BurntSushi opened this issue Jul 3, 2024 · 17 comments
Assignees
Labels
normative Would be a normative change to the proposal spec-text Specification text involved
Milestone

Comments

@BurntSushi
Copy link

I've been trying to write down a contract for the analog to ZonedDateTime.startOfDay in my own Temporal-inspired Rust library. The main contract in the Temporal docs is:

Returns: A new Temporal.ZonedDateTime instance representing the earliest valid local clock time during the current calendar day and time zone of zonedDateTime.

I am wondering whether this is true in all cases. I don't know of any such real world case where the above contract wouldn't be upheld, so it's not easy for me to test, but what happens if there is a time zone transition forwards that includes midnight but doesn't start at midnight? For example, maybe there is a 1 hour gap starting at 23:30:00. Since startOfDay will use the "compatible" disambiguation strategy and since the implementation (as I understand it) works by looking for an instant at 00:00:00, I believe this will result in returning the instant corresponding to 01:00:00. But the first instant of the day in this particular example is 00:30:00.

cc @arshaw I believe the fullcalendar polyfill uses the same implementation technique of starting from midnight and using the "compatible" disambiguation strategy.

@justingrant
Copy link
Collaborator

Hmmm, good catch @BurntSushi! I think you're correct that this is a spec bug. We should be resolving to the first instant of the calendar day, but your example shows how we're not doing that.

@ptomato @arshaw FYI this looks like a bug we may want to fix. I added to the next meeting agenda.

Note that (because we no longer support custom time zones) it may not be a normative change if there are no IANA time zones that have a transition that spans (but does not begin or end at) midnight in any time zones.

@anba
Copy link
Contributor

anba commented Jul 5, 2024

Note that (because we no longer support custom time zones) it may not be a normative change if there are no IANA time zones that have a transition that spans (but does not begin or end at) midnight in any time zones.

There's only a single transition that matches this characteristic:

From https://github.com/eggert/tz/blob/7eb5bf887927e35079e5cc12e2252819a7c47bb0/northamerica#L1684:

# Rule	NAME	FROM	TO	-	IN	ON	AT	SAVE	LETTER/S
Rule	Toronto	1919	only	-	Mar	30	23:30	1:00	D

Code to find this transition (will have to be updated after the removal of Temporal.TimeZone):

let start = new Temporal.PlainDate(1800, 1, 1).toZonedDateTime("UTC").toInstant();
let end = new Temporal.PlainDate(2050, 1, 1).toZonedDateTime("UTC").toInstant();

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

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

    let zdt = next.toZonedDateTimeISO(timeZone);
    let start = zdt.startOfDay();
    if (Temporal.ZonedDateTime.compare(zdt, start) !== 0 && start.hour !== 0) {
      console.log(timeZone, zdt, start, start.hour);
    }
  }
}

@arshaw
Copy link
Contributor

arshaw commented Jul 8, 2024

Good catch @BurntSushi. I acknowledge this is a bug. A similar algorithm is used in ZonedDateTime.p.hoursInDay

The solution might entail introducing a new internal utility function called GetStartOfDay:

function GetStartOfDay(y, m, d, nativeTimeZone) { // returns an Instant
  const instants = nativeTimeZone.getPossibleInstants(
    new PlainDateTime(y, m, d, 0, 0, 0, 0, 0, 0)
  )

  // if one or more instants, always return the earlier
  if (instants.length) {
    return instants[0]
  }
  // otherwise, 00:00:00 lies within a DST gap

  // compute an instant that's guaranteed to be before the transition-instant,
  // similar to what DisambiguatePossibleInstants already does
  const utcns = GetUTCEpochNanoseconds(y, m, d, 0, 0, 0, 0, 0, 0)
  const dayBefore = new Instant(utcns.minus(DAY_NANOS))
  
  // the next transition is guaranteed to be the start of the day
  // (a.k.a. the instant the transition that swallows the 00:00:00 wallclock time ends)
  return nativeTimeZone.getNextTransition(dayBefore)
}

Anyone see any shortcomings with this?

I'm happy to write a proof of concept in the reference-polyfill and then write the spec modifications once it's sound.

@BurntSushi
Copy link
Author

@arshaw In terms of the underlying data model, I think TZif would technically permit a second transition on the day before, such that the next transition in that code could come before the "midnight jump" transition you're looking for. But I don't think the zic compiler supports expressing two transitions on the same day? Anyway, I don't know how connected Temporal is to tzdb specifically, so consider this comment a shot in the dark. Other than that, I think I buy your technique.

Maybe another technique is to find the instant immediately after the gap via the "later" disambiguation strategy and then ask for the previous transition?

@arshaw
Copy link
Contributor

arshaw commented Jul 9, 2024

@BurntSushi, I know the current spec is assuming that two consecutive timezone transitions cannot be closer than 48 hours apart, here:

const offsetBefore = GetOffsetNanosecondsFor(timeZoneRec, dayBefore);
const offsetAfter = GetOffsetNanosecondsFor(timeZoneRec, dayAfter);
const nanoseconds = offsetAfter - offsetBefore;

(nanoseconds is later uses to derive the the "earlier" or "later" instants outside of the gap)

Though I'm not sure the basis for this presumption. Maybe someone else on the team with more context knows.

But if it turns out we cannot assume this, yes, your modification to the algorithm sounds like a good idea. Could also be done by doing getting the "earlier" moment outside the gap and calling getNextTransition.

@ptomato
Copy link
Collaborator

ptomato commented Jul 11, 2024

Discussed in the champions meeting of 2024-07-11. This is a bug that needs to be fixed.

When fixing this we should examine whether there's anywhere else besides startOfDay and hoursInDay where the start of day is calculated for a ZonedDateTime, using "compatible".

@ptomato ptomato added this to the Stage "3.5" milestone Jul 11, 2024
@BurntSushi
Copy link
Author

Possibly YYYY-MM-DD[TimeZone]. For example:

>>> zdt = Temporal.ZonedDateTime.from('1919-03-31[America/Montreal]')
Object { … }
>>> zdt.toString()
"1919-03-31T01:00:00-04:00[America/Montreal]" 

But I guess that should probably be 00:30:00.

@justingrant
Copy link
Collaborator

But I guess that should probably be 00:30:00.

Note that CLDR and ICU don't necessarily use historical data for time zones like America/Montreal that are Links in IANA. So the ID used is America/Montreal, but IIRC the actual data is for America/Toronto.

This doesn't mean that there isn't also a bug in the spec that we need to fix for this case, but it might be a data issue not a spec issue.

@ptomato
Copy link
Collaborator

ptomato commented Jul 15, 2024

I found the following additional places where we previously used GetInstantFor on a PlainDateTime record with midnight and "compatible" disambiguation strategy:

  1. PlainDate.prototype.toZonedDateTime() if no plainTime option is given.
  2. ZonedDateTime.prototype.withPlainTime() if no argument is given.
  3. Duration.prototype.round() with smallestUnit: "day" to calculate whether to round the fraction of the day up or down

I'd say (3) is definitely in the "bug" category along with startOfDay/hoursInDay, and must be fixed.

(1) and (2) are in a gray area IMO, maybe along with the string parsing case that @BurntSushi identified. Because it means that we start treating absent time semantically differently than we were before. Previously it always meant "midnight". With the proposed change it'd mean "start of day", but you could still specify "midnight" explicitly. In other words:

const z = Temporal.ZonedDateTime.from('1919-03-31T12:00[America/Toronto]')
z.withPlainTime()  // => jump to start of day, 00:30
z.withPlainTime('00:00')
  // => use 'compatible' disambiguation strategy from skipped 00:00, giving 01:00
z.withPlainTime('00:01')
  // => use 'compatible' disambiguation strategy from skipped 00:01, giving 01:01

I'm not opposed to this and I think it makes sense, but wanted to check, because it's odd to have a parameter with a default value that can't be specified explicitly. (other than explicitly specifying undefined, of course.) I could also see leaving toZonedDateTime and withPlainTime as-is, and just advising people to use startOfDay explicitly if they want the start of day.

I'd lean towards changing them though, because who on earth is going to go to the trouble of calling an extra method if it only makes a difference for one single day in one single time zone in all of recorded history. Better just to return what will almost certainly be the desired use case and avoid the countless Stack Overflow answers saying, "well ACTUALLY you have to tack startOfDay() on to there."

Could go either way on parsing YYYY-MM-DD[Zone], I'd prefer changing it even if we don't change toZonedDateTime and withPlainTime, because it's not the presence or absence of a function parameter that makes the difference in this case.

@ptomato ptomato self-assigned this Jul 15, 2024
@ptomato
Copy link
Collaborator

ptomato commented Jul 16, 2024

...although, do we treat ZonedDateTime.from({ year: 1919, month: 3, day: 31, timeZone: 'America/Toronto' }) differently from ZonedDateTime.from({ year: 1919, month: 3, day: 31, hour: 0, timeZone: 'America/Toronto' })? IMO absolutely not.

@justingrant
Copy link
Collaborator

I agree that changing everything (including the string parsing case) to match startOfDay seems like the best approach.

Also seems fine to treat undefined in property bag as if it's zero.

ptomato added a commit to ptomato/test262 that referenced this issue Jul 16, 2024
These tests add coverage for a corner case in the TZDB. In spring 1919,
the America/Toronto time zone switched to DST at 23:30 on March 30th,
skipping an hour ahead to 00:30 on March 31st. This meant that both March
30th and March 31st were 23.5-hour days.

See: tc39/proposal-temporal#2910
ptomato added a commit that referenced this issue Jul 16, 2024
Fixes calculations of start of day in ZonedDateTime.prototype.hoursInDay,
ZonedDateTime.prototype.startOfDay(), and ZonedDateTime.prototype.round()
with smallestUnit: "day".

Updates the following operations to explicitly mean "start of day" and not
"midnight":
- PlainDate.prototype.toZonedDateTime(timeZoneString)
- PlainDate.prototype.toZonedDateTime(options) with omitted or undefined
  plainTime option
- ZonedDateTime.prototype.withPlainTime() with no argument or undefined
- Parsing 'YYYY-MM-DD[Zone]' anywhere a ZonedDateTime string is expected

This is in order to handle one corner case from the TZDB where clocks were
set one hour ahead in America/Toronto at 23:30 on March 30, 1919, meaning
that the DST skipped hour encompassed midnight but did not start or end at
midnight. (This was probably just in the city of Toronto and maybe some
other towns, because time was still locally administered at that time.)

Closes: #2910
@ptomato
Copy link
Collaborator

ptomato commented Jul 16, 2024

Unfortunately, this was way more complex than I expected.

Here's the list of what changed in #2918. None of this makes a difference except for this particular America/Toronto edge case on March 30-31, 1919.

  • PlainDate.p.toZonedDateTime: interprets string-only argument, or options bag argument with missing or undefined plainTime, as start-of-day.
  • ZonedDateTime.from and everywhere else a ZonedDateTime string is parsed, including relativeTo options: YYYY-MM-DD[Zone] is interpreted as start-of-day. No change to property bags.
  • ZonedDateTime.p.hoursInDay: hours-in-day calculation corrected.
  • ZonedDateTime.p.startOfDay: start-of-day calculation corrected.
  • ZonedDateTime.p.round: With smallestUnit: "day", start-of-day and end-of-day calculations corrected, so some times on these two dates round up or down differently than they did before.
  • ZonedDateTime.p.withPlainTime: interprets omitted or undefined argument as start-of-day.

Note that the offset option in ZonedDateTime.from has no effect on a YYYY-MM-DD[Zone] string. You can't specify a UTC offset without a time because YYYY-MM-DD+UU:UU[Zone] is grammatically illegal. So we do what we already did if the UTC offset wasn't specified and treat it as offset: "ignore". This seems obvious in hindsight but I thought I'd highlight it because it was surprising to me at first.

You'll notice that the tests in tc39/test262#4158 contain two more cases in addition to the above, for Duration.p.round and Duration.p.total. Essentially, they boil down to

// DST spring-forward hour skipped at 1919-03-30T23:30 (23.5 hour day)
// 11.75 hours is 0.5
const duration = Temporal.Duration.from('PT11H45M');
const relativeTo = Temporal.ZonedDateTime.from('1919-03-30T00:00[America/Toronto]');
assert.sameValue(duration.total({ relativeTo, unit: "days" }), 0.5);

These test cases currently fail. I'm actually not sure whether they are correct. It seems to me like the above result should be expected, but I'm not clear on whether that should be affected by the way the semantics of total({ unit: 'days', relativeTo: zdt }) changed in the last refactor. @arshaw I was hoping you could help with this.

@arshaw
Copy link
Contributor

arshaw commented Jul 17, 2024

Hi @ptomato, I think it should fail, based on this breakdown of the internal operations:

const duration = Temporal.Duration.from('PT11H45M');
const relativeTo = Temporal.ZonedDateTime.from('1919-03-30T00:00[America/Toronto]');

const epochStart = relativeTo.epochNanoseconds;
const epochEnd = relativeTo.add({ days: 1 }).epochNanoseconds; // instant at 1919-03-31T01:00:00-04:00[America/Toronto]
const epochMid = relativeTo.add(duration).epochNanoseconds;

const frac = Number(epochMid - epochStart) / Number(epochEnd - epochStart); // 0.4895833333333333

const effectiveHoursInDay = Number(epochEnd - epochStart) / 3_600_000_000_000 // 24

The new start-of-day resolution in the PR isn't applying to the .add({ days: 1 }) operation. It's still using compat disambiguation:

relativeTo.toString() // 1919-03-30T00:00:00-05:00[America/Toronto]
relativeTo.add({ days: 1 }).toString() // 1919-03-31T01:00:00-04:00[America/Toronto]

The difference between those two instants is 24 hours, not 23:30 :(

To make these operations adhere to the new start-of-day stuff, you'd need to ensure relativeTo.add({ days: 1 }) and its internal counterparts result in 1919-03-31T00:30:00-04:00[America/Toronto]

Do you think we should make that change? I'm happy to help if so. You're right, this stuff is ballooning in complexity.

@ptomato
Copy link
Collaborator

ptomato commented Jul 17, 2024

To make these operations adhere to the new start-of-day stuff, you'd need to ensure relativeTo.add({ days: 1 }) and its internal counterparts result in 1919-03-31T00:30:00-04:00[America/Toronto]

Do you think we should make that change? I'm happy to help if so. You're right, this stuff is ballooning in complexity.

The thing is, in the normal case I think we actually do want relativeTo.add({ days: 1 }) to use the compatible disambiguation strategy! Unlike all the other APIs that changed in this PR, there's no way to provide an explicit marker here that it's supposed to be start-of-day.

I was thinking we could do something like this (pseudocode)

let nextDay = relativeTo.add({ days: 1 });
if (relativeTo.equals(relativeTo.startOfDay()) {
  nextDay = nextDay.startOfDay();
}
const epochEnd = nextDay.epochNanoseconds;

That gives the correct result for the case I mentioned above, but does not fix the equally weird case where you round PT11H45M relative to 1919-03-30T00:01[America/Toronto], one minute into the 23.5-hour day instead of at midnight. (But maybe that's not so bad?)

@ptomato
Copy link
Collaborator

ptomato commented Jul 17, 2024

For the record, if we are considering alternative solutions, I'd also support:

  1. Make the other changes, but just don't change Duration round/total. The algorithm still makes sense as it is, and is self-consistent. If you specifically want "what percentage of the day is this time span", use duration.total('hours') / relativeTo.hoursInDay.
  2. Drop ZonedDateTime relativeTo and only accept PlainDate. I know we already considered this and I'm the only one who thinks ZonedDateTime relativeTo isn't worth it, but this weirdness is a great example of why 😄

@arshaw
Copy link
Contributor

arshaw commented Jul 17, 2024

@ptomato, I'm in favor of leaving round and total as-is. So, I'm only in favor of using this new GetStartOfDay operation in the following cases:

A) When start-of-day in a timezone is pretty explicitly needed:

  • ZonedDateTime.p.startOfDay
  • ZonedDateTime.p.hoursInDay (essentially today's start-of-day -> tomorrow's start-of-day)

B) When needing to resolve an "undefined" time

  • PlainDate.p.toZonedDateTime
  • ZonedDateTime.from when given a YYYY-MM-DD[Zone] string (and anywhere else a zdt string is accepted)
  • ZonedDateTime.p.withPlainTime when argument is undefined

For the Duration.p.round/total operations that use a ZonedDateTime as a relativeTo, as well as ZonedDateTime.p.add (which is leveraged by Duration.p.round/total), I'd say the ZDT starting-point ALREADY has a time, and we shouldn't try to pretend it's "undefined". And just use our "compatible" disambiguation strategy for all arithmetics thereafter.

@ptomato
Copy link
Collaborator

ptomato commented Jul 17, 2024

That reasoning seems sound to me. Unless anyone else weighs in soon to say otherwise, I think we should go with that.

ptomato added a commit to ptomato/test262 that referenced this issue Jul 20, 2024
These tests add coverage for a corner case in the TZDB. In spring 1919,
the America/Toronto time zone switched to DST at 23:30 on March 30th,
skipping an hour ahead to 00:30 on March 31st. This meant that both March
30th and March 31st were 23.5-hour days.

See: tc39/proposal-temporal#2910
ptomato added a commit that referenced this issue Jul 20, 2024
Fixes calculations of start of day in ZonedDateTime.prototype.hoursInDay,
ZonedDateTime.prototype.startOfDay(), and ZonedDateTime.prototype.round()
with smallestUnit: "day".

Updates the following operations to explicitly mean "start of day" and not
"midnight":
- PlainDate.prototype.toZonedDateTime(timeZoneString)
- PlainDate.prototype.toZonedDateTime(options) with omitted or undefined
  plainTime option
- ZonedDateTime.prototype.withPlainTime() with no argument or undefined
- Parsing 'YYYY-MM-DD[Zone]' anywhere a ZonedDateTime string is expected

This is in order to handle one corner case from the TZDB where clocks were
set one hour ahead in America/Toronto at 23:30 on March 30, 1919, meaning
that the DST skipped hour encompassed midnight but did not start or end at
midnight. (This was probably just in the city of Toronto and maybe some
other towns, because time was still locally administered at that time.)

Closes: #2910
@ptomato ptomato added spec-text Specification text involved normative Would be a normative change to the proposal and removed meeting-agenda labels Jul 25, 2024
ptomato added a commit to ptomato/test262 that referenced this issue Jul 30, 2024
These tests add coverage for a corner case in the TZDB. In spring 1919,
the America/Toronto time zone switched to DST at 23:30 on March 30th,
skipping an hour ahead to 00:30 on March 31st. This meant that both March
30th and March 31st were 23.5-hour days.

See: tc39/proposal-temporal#2910
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
normative Would be a normative change to the proposal spec-text Specification text involved
Projects
None yet
Development

No branches or pull requests

5 participants