-
Notifications
You must be signed in to change notification settings - Fork 160
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.prototype.{round,total}: Missing checks for too large date-time values #3015
Comments
@anba Are you sure it already has test coverage? I tried adding the assertion steps to the polyfill, and got no new test failures. Which ones fail on your end? |
Edit: Forgot to reenable the test262 tests which were failing because of this missing check. 😅 This is covered by
When the input is
creates a date-time valid outside the valid ISO date-time valids, because the time part is midnight. |
See #3015, which posits that these assertions are hit in existing test262 tests.
See #3015, which posits that these assertions are hit in existing test262 tests.
See #3015, which posits that these assertions are hit in existing test262 tests.
See #3015, which posits that these assertions are hit in existing test262 tests.
Reported as <tc39/proposal-temporal#3015>. Differential Revision: https://phabricator.services.mozilla.com/D228028
So here's what I think is happening. The assertion is not tripped in those two test262 tests because the duration is zero, and DifferencePlainDateTimeWithRounding/DifferencePlainDateTimeWithTotal have an early return for the case of two identical PlainDateTimes. However, if you run the same operations with e.g. I will first adapt the three |
These tests did not fully cover Temporal.Duration.prototype.round and Temporal.Duration.prototype.total because they called those methods on a blank duration (all components zero), for which there is an early return in round() and total(). This meant that we missed an assertion that would be hit after the early return. This makes sure to test both blank and non-blank Durations with these relativeTo strings, and expect some strings to fail at different steps with the two cases. See: tc39/proposal-temporal#3015
These tests did not fully cover Temporal.Duration.prototype.round and Temporal.Duration.prototype.total because they called those methods on a blank duration (all components zero), for which there is an early return in round() and total(). This meant that we missed an assertion that would be hit after the early return. This makes sure to test both blank and non-blank Durations with these relativeTo strings, and expect some strings to fail at different steps with the two cases. See: tc39/proposal-temporal#3015
DifferenceISODateTime asserts that the ISO Date-Time Records passed to it are within limits. Previously there were two places where this was not guaranteed to be the case. Add checks that throw RangeError so the assertion is not hit. Closes: #3015
Reported as <tc39/proposal-temporal#3015>. Differential Revision: https://phabricator.services.mozilla.com/D228028
Temporal.Duration.prototype.round, steps 27.f-h:
Temporal.Duration.prototype.total, steps 12.f-h:
isoDateTime
andtargetDateTime
can be be outside the valid date-time limits, which breaks assertions inDifferencePlainDateTimeWithRounding
andDifferencePlainDateTimeWithTotal
when callingDifferenceISODateTime
.(This is already covered by test262 tests, but probably didn't get noticed because the polyfill doesn't implement the assertion steps.)
The text was updated successfully, but these errors were encountered: