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

Add tests for RoundDuration when unit is "month" and the fractionalDays sign gets flipped #3933

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

anba
Copy link
Contributor

@anba anba commented Sep 27, 2023

The sign of fractionalDays can get flipped when adding weeksInDays in step 8.j. Ensure implementations correctly handle this case in the loop condition of step 8.p.

…ys sign gets flipped

The sign of `fractionalDays` can get flipped when adding `weeksInDays`
in step 8.j. Ensure implementations correctly handle this case in the
loop condition of step 8.p.
@anba anba requested a review from a team as a code owner September 27, 2023 08:21
@anba
Copy link
Contributor Author

anba commented Sep 27, 2023

@ptomato The spec polyfill doesn't handle this case correctly.

@ptomato
Copy link
Contributor

ptomato commented Sep 27, 2023

@anba Thanks for another excellent test case.

In tc39/proposal-temporal#2612 note that we actually are removing this loop: tc39/proposal-temporal@705daa3 (commit needs a rebase, but you get the idea.)

The RoundDuration algorithm with unit being "month" will then read:

  1. Let yearsMonths be ! CreateTemporalDuration(years, months, 0, 0, 0, 0, 0, 0, 0, 0).
  2. Let yearsMonthsLater be ? AddDate(calendarRec, plainRelativeTo, yearsMonths).
  3. Let yearsMonthsWeeks be ! CreateTemporalDuration(years, months, weeks, 0, 0, 0, 0, 0, 0, 0).
  4. Let yearsMonthsWeeksLater be ? AddDate(calendarRec, plainRelativeTo, yearsMonthsWeeks).
  5. Let weeksInDays be DaysUntil(yearsMonthsLater, yearsMonthsWeeksLater).
  6. Set plainRelativeTo to yearsMonthsLater.
  7. Set fractionalDays to fractionalDays + weeksInDays.
  8. Let isoResult be ! AddISODate(plainRelativeTo.[[ISOYear]], plainRelativeTo.[[ISOMonth]], plainRelativeTo.[[ISODay]], 0, 0, 0, truncate(fractionalDays), "constrain").
  9. Let wholeDaysLater be ! CreateTemporalDate(isoResult.[[Year]], isoResult.[[Month]], isoResult.[[Day]], calendarRec.[[Receiver]]).
  10. Let untilOptions be OrdinaryObjectCreate(null).
  11. Perform ! CreateDataPropertyOrThrow(untilOptions, "largestUnit", "month").
  12. Let timePassed be ? DifferenceDate(calendarRec, plainRelativeTo, wholeDaysLater, untilOptions).
  13. Let monthsPassed be timePassed.[[Months]].
  14. Set months to months + monthsPassed.
  15. Let monthsPassedDuration be ! CreateTemporalDuration(0, monthsPassed, 0, 0, 0, 0, 0, 0, 0, 0).
  16. Let moveResult be ? MoveRelativeDate(calendarRec, plainRelativeTo, monthsPassedDuration).
  17. Set plainRelativeTo to moveResult.[[RelativeTo]].
  18. Let daysPassed be moveResult.[[Days]].
  19. Set fractionalDays to fractionalDays - daysPassed.
  20. If fractionalDays < 0, let sign be -1; else, let sign be 1.
  21. Let oneMonth be ! CreateTemporalDuration(0, sign, 0, 0, 0, 0, 0, 0, 0, 0).
  22. Let moveResult be ? MoveRelativeDate(calendarRec, plainRelativeTo, oneMonth).
  23. Let oneMonthDays be moveResult.[[Days]].
  24. Let fractionalMonths be months + fractionalDays / abs(oneMonthDays).
  25. Set months to RoundNumberToIncrement(fractionalMonths, increment, roundingMode).
  26. Set total to fractionalMonths.
  27. Set weeks to 0.

At first glance it still seems like it should be possible to cause the condition where fractionalDays is sign-flipped by returning some particular value from dateAdd in the MoveRelativeDate call in what is here numbered step 16.

I can merge this and make sure that the pending PRs accommodate the case correctly, but I'd appreciate your insight on whether my hunch is correct that it'll still be possible to trigger this case after the pending PRs land.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants