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

Duration.p.round doesn't balance from hours (or smaller) to weeks (or larger) units #2508

Closed
lionel-rowe opened this issue Feb 21, 2023 · 1 comment · Fixed by #2517
Closed
Assignees
Labels
bug non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! spec-text Specification text involved
Milestone

Comments

@lionel-rowe
Copy link

lionel-rowe commented Feb 21, 2023

Duration#round appears to give the wrong answer for certain durations and options. It's also not idempotent — rounding a second time with the same options yields a different result (the originally expected one).

Example:

const now  = Temporal.ZonedDateTime.from('2023-02-21T00:00:00+00:00[UTC]')
const then = Temporal.ZonedDateTime.from('2025-06-15T00:00:00+00:00[UTC]')

const duration = now.until(then)
// PT20280H
const rounded1 = duration.round({ relativeTo: now, largestUnit: 'year' })
// P845D
const rounded2 = rounded1.round({ relativeTo: now, largestUnit: 'year' })
// P2Y3M25D

The unexpected behavior affects both:

Surely calling round twice with the same options should yield the same result as calling it once? "845 days" is a very unexpected result given largestUnit is year.

@justingrant justingrant changed the title Bug - Duration#round gives wrong answer and is not idempotent? Duration.p.round doesn't balance from hours (or smaller) to weeks (or larger) units Feb 21, 2023
@justingrant
Copy link
Collaborator

Thanks @lionel-rowe for catching this. Your second result is the correct result. Your first result is unexpected.

We'll get this fixed, although if this is a spec bug we'll need to wait to fix it in the polyfills until the spec change is OK-ed at the next TC39 committee meeting in late March.

@ptomato If I'm reading the spec correctly, this looks like a bug in the spec that the polyfills are faithfully reproducing. In Duration.p.round, hours=>days balancing happens in the call to BalanceDuration, but there's no call after that that could turn days into weeks/months/years. So if there's more than a week (or month, or year) of hours (or smaller units), then the result ends up with a large number of unbalanced days instead of balancing those days up to larger units.

Here's a simpler repro:

Temporal.Duration.from('P100D').round({ relativeTo: '2023-02-21', largestUnit: 'month' })
// P3M11D - correct result
Temporal.Duration.from('PT2400H').round({ relativeTo: '2023-02-21', largestUnit: 'month' })
// P100D - expected P3M11D

@justingrant justingrant added bug non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! spec-text Specification text involved meeting-agenda labels Feb 21, 2023
@ptomato ptomato added this to the Stage "3.5" milestone Feb 23, 2023
guijemont added a commit to guijemont/proposal-temporal that referenced this issue Mar 8, 2023
…on.prototype.round

If we balance the bigger units before the smaller ones, then we never
properly balance the days that are added from balancing units up to day
length. Changing the order of operations ensures proper balancing of all
units.

Closes: tc39#2508
guijemont added a commit to guijemont/proposal-temporal that referenced this issue Mar 9, 2023
…on.prototype.round

If we balance the bigger units before the smaller ones, then we never
properly balance the days that are added from balancing units up to day
length. Changing the order of operations ensures proper balancing of all
units.
There is also no more need for the call to MoveRelativeZonedDateTime()
in the end of the method, so it's been removed.

Closes: tc39#2508
guijemont added a commit to guijemont/test262 that referenced this issue Mar 9, 2023
Before the issue is fixed, rounding a duration such as 'PT2400h' with
months as largest unit fails to balance it properly and gives a result
with days being the largest unit. The test ensures that such rounding
happens properly.
guijemont added a commit to guijemont/test262 that referenced this issue Mar 9, 2023
Before the issue is fixed, rounding a duration such as 'PT2400h' with
months as largest unit fails to balance it properly and gives a result
with days being the largest unit. The test ensures that such rounding
happens properly.
ptomato pushed a commit to guijemont/test262 that referenced this issue Apr 10, 2023
Before the issue is fixed, rounding a duration such as 'PT2400h' with
months as largest unit fails to balance it properly and gives a result
with days being the largest unit. The test ensures that such rounding
happens properly.
ptomato pushed a commit to tc39/test262 that referenced this issue Apr 10, 2023
Before the issue is fixed, rounding a duration such as 'PT2400h' with
months as largest unit fails to balance it properly and gives a result
with days being the largest unit. The test ensures that such rounding
happens properly.
ptomato pushed a commit to guijemont/proposal-temporal that referenced this issue Apr 10, 2023
…on.prototype.round

If we balance the bigger units before the smaller ones, then we never
properly balance the days that are added from balancing units up to day
length. Changing the order of operations ensures proper balancing of all
units.
There is also no more need for the call to MoveRelativeZonedDateTime()
in the end of the method, so it's been removed.

Closes: tc39#2508
ptomato pushed a commit that referenced this issue Apr 10, 2023
…on.prototype.round

If we balance the bigger units before the smaller ones, then we never
properly balance the days that are added from balancing units up to day
length. Changing the order of operations ensures proper balancing of all
units.
There is also no more need for the call to MoveRelativeZonedDateTime()
in the end of the method, so it's been removed.

Closes: #2508
justingrant pushed a commit that referenced this issue Apr 20, 2023
…on.prototype.round

If we balance the bigger units before the smaller ones, then we never
properly balance the days that are added from balancing units up to day
length. Changing the order of operations ensures proper balancing of all
units.
There is also no more need for the call to MoveRelativeZonedDateTime()
in the end of the method, so it's been removed.

Closes: #2508

UPSTREAM_COMMIT=36746e5ef8da4688237c195e51c8081e3304ab0f
ptomato added a commit that referenced this issue Jan 23, 2024
When the time portion of a duration, rounded relative to a ZonedDateTime,
would land on a non-24-hour day, we'd get incorrect results. This is a
regression from #2508 where although switching the order of
BalanceDateDurationRelative and BalanceTimeDurationRelative was correct,
we should not have removed the MoveRelativeZonedDateTime call.

Closes: #2742
ptomato added a commit that referenced this issue Feb 1, 2024
When the time portion of a duration, rounded relative to a ZonedDateTime,
would land on a non-24-hour day, we'd get incorrect results. This is a
regression from #2508 where although switching the order of
BalanceDateDurationRelative and BalanceTimeDurationRelative was correct,
we should not have removed the MoveRelativeZonedDateTime call.

Closes: #2742
ptomato added a commit that referenced this issue Feb 3, 2024
When the time portion of a duration, rounded relative to a ZonedDateTime,
would land on a non-24-hour day, we'd get incorrect results. This is a
regression from #2508 where although switching the order of
BalanceDateDurationRelative and BalanceTimeDurationRelative was correct,
we should not have removed the MoveRelativeZonedDateTime call.

Closes: #2742
ptomato added a commit that referenced this issue Apr 2, 2024
When the time portion of a duration, rounded relative to a ZonedDateTime,
would land on a non-24-hour day, we'd get incorrect results. This is a
regression from #2508 where although switching the order of
BalanceDateDurationRelative and BalanceTimeDurationRelative was correct,
we should not have removed the MoveRelativeZonedDateTime call.

Closes: #2742
ptomato added a commit that referenced this issue Apr 4, 2024
When the time portion of a duration, rounded relative to a ZonedDateTime,
would land on a non-24-hour day, we'd get incorrect results. This is a
regression from #2508 where although switching the order of
BalanceDateDurationRelative and BalanceTimeDurationRelative was correct,
we should not have removed the MoveRelativeZonedDateTime call.

Closes: #2742
ptomato added a commit that referenced this issue Apr 10, 2024
When the time portion of a duration, rounded relative to a ZonedDateTime,
would land on a non-24-hour day, we'd get incorrect results. This is a
regression from #2508 where although switching the order of
BalanceDateDurationRelative and BalanceTimeDurationRelative was correct,
we should not have removed the MoveRelativeZonedDateTime call.

Closes: #2742
ptomato added a commit that referenced this issue Apr 27, 2024
When the time portion of a duration, rounded relative to a ZonedDateTime,
would land on a non-24-hour day, we'd get incorrect results. This is a
regression from #2508 where although switching the order of
BalanceDateDurationRelative and BalanceTimeDurationRelative was correct,
we should not have removed the MoveRelativeZonedDateTime call.

Closes: #2742
ptomato added a commit that referenced this issue May 1, 2024
When the time portion of a duration, rounded relative to a ZonedDateTime,
would land on a non-24-hour day, we'd get incorrect results. This is a
regression from #2508 where although switching the order of
BalanceDateDurationRelative and BalanceTimeDurationRelative was correct,
we should not have removed the MoveRelativeZonedDateTime call.

Closes: #2742
ptomato added a commit that referenced this issue May 2, 2024
When the time portion of a duration, rounded relative to a ZonedDateTime,
would land on a non-24-hour day, we'd get incorrect results. This is a
regression from #2508 where although switching the order of
BalanceDateDurationRelative and BalanceTimeDurationRelative was correct,
we should not have removed the MoveRelativeZonedDateTime call.

Closes: #2742
ptomato added a commit that referenced this issue May 3, 2024
When the time portion of a duration, rounded relative to a ZonedDateTime,
would land on a non-24-hour day, we'd get incorrect results. This is a
regression from #2508 where although switching the order of
BalanceDateDurationRelative and BalanceTimeDurationRelative was correct,
we should not have removed the MoveRelativeZonedDateTime call.

Closes: #2742
ptomato added a commit that referenced this issue May 7, 2024
When the time portion of a duration, rounded relative to a ZonedDateTime,
would land on a non-24-hour day, we'd get incorrect results. This is a
regression from #2508 where although switching the order of
BalanceDateDurationRelative and BalanceTimeDurationRelative was correct,
we should not have removed the MoveRelativeZonedDateTime call.

Closes: #2742
ptomato added a commit that referenced this issue May 14, 2024
When the time portion of a duration, rounded relative to a ZonedDateTime,
would land on a non-24-hour day, we'd get incorrect results. This is a
regression from #2508 where although switching the order of
BalanceDateDurationRelative and BalanceTimeDurationRelative was correct,
we should not have removed the MoveRelativeZonedDateTime call.

Closes: #2742
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! spec-text Specification text involved
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants