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

Bug: ZonedDateTime::round incorrect results with smallestUnit:day #2790

Closed
arshaw opened this issue Feb 27, 2024 · 3 comments
Closed

Bug: ZonedDateTime::round incorrect results with smallestUnit:day #2790

arshaw opened this issue Feb 27, 2024 · 3 comments
Assignees
Labels
non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! normative Would be a normative change to the proposal spec-text Specification text involved
Milestone

Comments

@arshaw
Copy link
Contributor

arshaw commented Feb 27, 2024

Currently, the ZonedDateTime::round() methods operates like this:

  1. convert ZonedDateTime -> PlainDateTime
  2. compute nanoseconds-in-day for the ZonedDateTime (takes two getPossibleInstantsFor calls)
  3. round the PlainDateTime using this nanoseconds-in-day as the max time-of-day
  4. convert the rounded PlainDateTime -> ZonedDateTime

I don't understand why step 2 is necessary and why 24:00 can't be used for step 3. In step 4, the TimeZone will naturally nudge nonexistent times out of DST gaps anyway.

FWIW, temporal-polyfill skips those steps. All tests pass except those expecting more getPossibleInstantsFor calls.

This would indicate to me that either: a) those steps can be eliminated or b) those steps are required but don't have test coverage.

Thank you for considering this ticket!

If you agree, I'd be happy to make a PR for both proposal-temporal and test262. I'd just like to validate my idea first.

@arshaw arshaw changed the title Maybe fewer getPossibleInstantsFor calls during ZonedDateTime::round Bug & Fewer Calls: ZonedDateTime::round & GetPossibleInstantsFor Feb 29, 2024
@arshaw
Copy link
Contributor Author

arshaw commented Feb 29, 2024

Actually, I've found a bug with the current algorithm:

const zdt0 = Temporal.ZonedDateTime.from('2024-03-10T23:00:01[America/New_York]')
const zdt1 = zdt0.round({ smallestUnit: 'day', roundingMode: 'ceil' })
console.log(zdt1.toString()) // '2024-03-12T00:00:00-04:00[America/New_York]'

It gets rounded up by 24 hours and 59 seconds, which is clearly wrong.

This is happening because:

  1. 23:00:01 is considered the numerator
  2. 23:00:00 is considered the denominator (because 2024-03-10 has 23 hours in this tz)
  3. Dividing results in 1.00001 or whatever
  4. The ceil func nudges it to 2 (i.e. 2 days from 2024-03-10T00:00:00)

If the algorithm is trying to test how far the ZDT has progressed through all the nanoseconds of the current day, the location of the DST-gap within the 23-hour would need to be considered (in this example, it happens at 2am on the 10th).

OR, in my opinion, we should simply round the wallclock-time, and then let the TimeZone massage it into an epochNanoseconds. The algorithm is simpler for this and its what most people would expect in my opinion. This would mean that 11:59 am rounded with a halfExpand would be rounded down in all timezones.

@arshaw
Copy link
Contributor Author

arshaw commented Feb 29, 2024

@ptomato and @justingrant I'll implement that fix we discussed in the meeting. I'll open a PR soon.

@arshaw arshaw changed the title Bug & Fewer Calls: ZonedDateTime::round & GetPossibleInstantsFor Bug & Fewer GetPossibleInstantsFor calls during ZonedDateTime::round Feb 29, 2024
@ptomato
Copy link
Collaborator

ptomato commented Feb 29, 2024

Champions meeting 2024-02-29: All are agreed that is definitely a bug. So unfortunately that probably means queuing up another normative change for next plenary.

@arshaw arshaw changed the title Bug & Fewer GetPossibleInstantsFor calls during ZonedDateTime::round Bug: ZonedDateTime::round incorrect results with smallestUnit:day Mar 7, 2024
ptomato added a commit to fullcalendar/proposal-temporal that referenced this issue Mar 22, 2024
The following code shows how ZonedDateTime would previously get rounded up
by two days instead of the intended 1 day:

const zdt0 = Temporal.ZonedDateTime.from('2024-03-10T23:00:01[America/New_York]')
const zdt1 = zdt0.round({ smallestUnit: 'day', roundingMode: 'ceil' })
console.log(zdt1.toString()) // '2024-03-12T00:00:00-04:00[America/New_York]'

- 23:00:01 is considered the numerator
- 23:00:00 is considered the denominator (because 2024-03-10 has 23 hours
  in this tz)
- Dividing results in a number slightly larger than 1
- The ceil func nudges it to 2 (i.e. 2 days from 2024-03-10T00:00:00)

This is the spec text corresponding to the reference code change in the
previous commit.

Closes: tc39#2790
@ptomato ptomato added this to the Stage "3.5" milestone Mar 28, 2024
@ptomato ptomato added spec-text Specification text involved non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! normative Would be a normative change to the proposal labels Mar 28, 2024
ptomato added a commit to fullcalendar/proposal-temporal that referenced this issue Apr 9, 2024
The following code shows how ZonedDateTime would previously get rounded up
by two days instead of the intended 1 day:

const zdt0 = Temporal.ZonedDateTime.from('2024-03-10T23:00:01[America/New_York]')
const zdt1 = zdt0.round({ smallestUnit: 'day', roundingMode: 'ceil' })
console.log(zdt1.toString()) // '2024-03-12T00:00:00-04:00[America/New_York]'

- 23:00:01 is considered the numerator
- 23:00:00 is considered the denominator (because 2024-03-10 has 23 hours
  in this tz)
- Dividing results in a number slightly larger than 1
- The ceil func nudges it to 2 (i.e. 2 days from 2024-03-10T00:00:00)

This is the spec text corresponding to the reference code change in the
previous commit.

Closes: tc39#2790
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! normative Would be a normative change to the proposal spec-text Specification text involved
Projects
None yet
Development

No branches or pull requests

2 participants