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

NudgeToCalendarUnit: CreateTemporalDate is fallible #2881

Closed
anba opened this issue Jun 4, 2024 · 7 comments
Closed

NudgeToCalendarUnit: CreateTemporalDate is fallible #2881

anba opened this issue Jun 4, 2024 · 7 comments
Assignees
Labels
editorial spec-text Specification text involved
Milestone

Comments

@anba
Copy link
Contributor

anba commented Jun 4, 2024

Steps 3.a-d:

Let isoResult1 be BalanceISODate(dateTime.[[Year]] + duration.[[Years]], dateTime.[[Month]] + duration.[[Months]], dateTime.[[Day]]).
b. Let isoResult2 be BalanceISODate(dateTime.[[Year]] + duration.[[Years]], dateTime.[[Month]] + duration.[[Months]], dateTime.[[Day]] + duration.[[Days]]).
c. Let weeksStart be ! CreateTemporalDate(isoResult1.[[Year]], isoResult1.[[Month]], isoResult1.[[Day]], calendarRec.[[Receiver]]).
d. Let weeksEnd be ! CreateTemporalDate(isoResult2.[[Year]], isoResult2.[[Month]], isoResult2.[[Day]], calendarRec.[[Receiver]]).

The CreateTemporalDate calls are fallible.

@anba
Copy link
Contributor Author

anba commented Jun 5, 2024

I also don't (yet?) quite understand why the duration fields are interpreted as ISO-calendar units, so it might be help to add a note why directly calling BalanceISODate instead of going through AddDate is allowed.

@ptomato
Copy link
Collaborator

ptomato commented Jun 5, 2024

@anba That may actually be a mistake on my part. At first glance, it looks like we'd need to call CalendarDateAdd for isoResult1 and then BalanceISODate to just add duration.[[Days]] to isoResult1 to obtain isoResult2. Thanks for catching it; I guess we should treat it as your post-merge code review.

As for the original issue, I assumed the CreateTemporalDate calls would not be fallible because isoResult2 would be the equivalent plain date for destEpochNs, which would be within the limits.

(But if an arbitrary date could be returned from CalendarDateAdd, that no longer holds.)

@anba
Copy link
Contributor Author

anba commented Jun 5, 2024

As for the original issue, I assumed the CreateTemporalDate calls would not be fallible because isoResult2 would be the equivalent plain date for destEpochNs, which would be within the limits.

If duration.[[Years]] is let's say 1_000_000, then isoResult1 is outside the valid date-time limits, so the ISODateTimeWithinLimits check in CreateTemporalDate will throw a RangeError.

@ptomato
Copy link
Collaborator

ptomato commented Jun 5, 2024

But if you had a duration with calendar units that large, wouldn't it throw already before it got to this point? I should try stepping through this, to make sure...

@anba
Copy link
Contributor Author

anba commented Jun 5, 2024

DifferenceDate in DifferenceTemporalPlainDate calls user-code for user-defined calendars, so the duration can have arbitrary large values, DifferenceTemporalPlainDate then calls RoundRelativeDuration, which in turn calls NudgeToCalendarUnit. The duration values are unchanged along this call chain, so large values should be possible.

(I haven't yet implemented this part in SpiderMonkey, so I'm not 100% sure, but this looks plausible when just reading the spec text.)

@ptomato ptomato self-assigned this Sep 6, 2024
@ptomato ptomato added spec-text Specification text involved editorial labels Sep 6, 2024
@ptomato ptomato added this to the Stage "3.5" milestone Sep 6, 2024
@ptomato
Copy link
Collaborator

ptomato commented Sep 7, 2024

I think the BalanceISODate / CalendarDateAdd distinction doesn't matter here because weeks in all of the supported calendars are always 7 days, so the result is the same no matter whether you add ISO years/months or calendar years/months to get weeksStart. Nonetheless, it's conceptually incorrect and we should change it.

@ptomato
Copy link
Collaborator

ptomato commented Sep 7, 2024

(The CreateTemporalDate calls are gone after #2925, so that part no longer applies.)

ptomato added a commit that referenced this issue Sep 10, 2024
This was an overzealous replacement of CalendarDateAdd with BalanceISODate
spotted by Anba. It doesn't produce any observably different results,
because in all supported calendars weeks are always 7 days; but it's still
conceptually incorrect. Go back to using CalendarDateAdd here.

Closes: #2881
ptomato added a commit that referenced this issue Sep 10, 2024
This was an overzealous replacement of CalendarDateAdd with BalanceISODate
spotted by Anba. It doesn't produce any observably different results,
because in all supported calendars weeks are always 7 days; but it's still
conceptually incorrect. Go back to using CalendarDateAdd here.

Closes: #2881
ptomato added a commit that referenced this issue Sep 10, 2024
This was an overzealous replacement of CalendarDateAdd with BalanceISODate
spotted by Anba. It doesn't produce any observably different results,
because in all supported calendars weeks are always 7 days; but it's still
conceptually incorrect. Go back to using CalendarDateAdd here.

Closes: #2881
ptomato added a commit that referenced this issue Sep 11, 2024
This was an overzealous replacement of CalendarDateAdd with BalanceISODate
spotted by Anba. It doesn't produce any observably different results,
because in all supported calendars weeks are always 7 days; but it's still
conceptually incorrect. Go back to using CalendarDateAdd here.

Closes: #2881
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editorial spec-text Specification text involved
Projects
None yet
Development

No branches or pull requests

2 participants