Replace BalanceISODate(Time) and rearrange time zone offset checks by anba · Pull Request #3014 · tc39/proposal-temporal

anba · 2024-10-17T17:23:22Z

Disclaimer: I haven't yet implemented these changes, so it's possible that there are still some bugs in the proposed changes.

The first three commits replace BalanceISODateTime and BalanceISODate with less general operations.
- BalanceISODateTime is only used to add a time zone offset to a date-time. It seems like all but one occurrence can be replaced by plain subtraction. For that one caller where subtraction isn't possible, either rename BalanceISODateTime to AddOffsetNanosecondsToISODateTime or alternatively just inline it into GetISODateTimeFor. Commits for both alternatives are prepared.
- And BalanceISODate is only used to add a days amount to an ISO Date record. Replace it with AddDaysToISODate, so it's easier to see what this operation does.
The remaining commits reorder CheckISODaysRange calls to happen earlier, so it's easier to see (and implement) when ISO Date-Time records contain out-of-range values.
- CheckISODaysRange currently allows inputs from nsMinInstant (inclusive) to nsMaxInstant + nsPerDay (exclusive). That looks like a bug, I think the lower limit should be nsMinInstant - nsPerDay (exclusive). This range can be checked through ISODateTimeWithinLimits.
- In two places ISODateTimeWithinLimits is used to check GetUTCEpochNanoseconds inputs instead of CheckISODaysRange. Either use CheckISODaysRange consistently or inline CheckISODaysRange everywhere. Commits for both alternative are also prepared here.
- All changes are editorial, because they only reorder when RangeError exceptions are thrown, but this isn't visible from user-code, because no other side-effects can happen.

codecov · 2024-10-17T17:26:06Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.96%. Comparing base (53731c2) to head (014eb8b).
⚠️ Report is 32 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3014   +/-   ##
=======================================
  Coverage   96.96%   96.96%           
=======================================
  Files          22       22           
  Lines       10219    10219           
  Branches     1841     1841           
=======================================
  Hits         9909     9909           
  Misses        261      261           
  Partials       49       49

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

ptomato

Thanks for the PR! I like most of the changes but some of the stuff around CheckISODaysRange I'm not sure is correct. I implemented it in this repo's reference code and I get different behaviour around the edges of the representable range.

ptomato · 2024-10-25T20:29:21Z

spec/instant.html

-        1. Let _epochNanoseconds_ be GetUTCEpochNanoseconds(_balanced_).
+        1. Let _isoDate_ be CreateISODateRecord(_parsed_.[[Year]], _parsed_.[[Month]], _parsed_.[[Day]]).
+        1. Let _isoDateTime_ be CombineISODateAndTimeRecord(_isoDate_, _time_).
+        1. Perform ? CheckISODaysRange(_isoDate_).


In some cases, this will be incorrect, because the parsed YMD may be different from the actual YMD after balancing. e.g. Temporal.Instant.from("-271821-04-19T23:00-01:00"), the parsed YMD is out of range, but the instant itself is in range.

This shouldn't be a problem after 77bef95 when the allowed input range is changed from [nsMinInstant, nsMaxInstant + nsPerDay) to (nsMinInstant - nsPerDay, nsMaxInstant + nsPerDay).

ptomato · 2024-10-25T20:29:49Z

spec/zoneddatetime.html

        1. If _offsetBehaviour_ is ~wall~, or _offsetBehaviour_ is ~option~ and _offsetOption_ is ~ignore~, then
          1. Return ? GetEpochNanosecondsFor(_timeZone_, _isoDateTime_, _disambiguation_).
        1. If _offsetBehaviour_ is ~exact~, or _offsetBehaviour_ is ~option~ and _offsetOption_ is ~use~, then
-          1. Let _balanced_ be BalanceISODateTime(_isoDate_.[[Year]], _isoDate_.[[Month]], _isoDate_.[[Day]], _time_.[[Hour]], _time_.[[Minute]], _time_.[[Second]], _time_.[[Millisecond]], _time_.[[Microsecond]], _time_.[[Nanosecond]] - _offsetNanoseconds_).


ptomato · 2024-10-25T23:08:55Z

spec/abstractops.html

-      1. If abs(ISODateToEpochDays(_isoDate_.[[Year]], _isoDate_.[[Month]] - 1, _isoDate_.[[Day]])) > 10<sup>8</sup>, then
+      1. Let _dateTime_ be CombineISODateAndTimeRecord(_isoDate_, MidnightTimeRecord()).
+      1. If ISODateTimeWithinLimits(_dateTime_) is *false*, then


This isn't equivalent. (I know that's intentional from your description, but I'm not sure it's correct.) CheckISODaysRange has that weird range because that's the range that will avoid running into the undefined behaviour from tc39/ecma262#1087.

The current CheckISODaysRange definition effectively allows inputs in the range [nsMinInstant, nsMaxInstant + nsPerDay), but I think the allowed range should instead be (nsMinInstant - nsPerDay, nsMaxInstant + nsPerDay).

Confusingly the ECMA-262 spec sometimes mentions that inputs resp. outputs are time values (i.e. times within the [nsMinInstant, nsMaxInstant] range), but the actually allowed times are with the local time zone offset applied. For example https://tc39.es/ecma262/#sec-date.prototype.getdate:

Return DateFromTime(LocalTime(t)).

LocalTime(t) returns a local time zone adjusted value, which can be outside the valid time value bounds, but per the DateFromTime description, DateFromTime should only be called with time values.

spec/abstractops.html

spec/zoneddatetime.html

ptomato

I took another look at this today and tried to implement it in JS. I do get some differing test results, though that may be my mistake. If you implement it like this, do you pass e.g. all of the relativeto-string-limits.js tests?

I think the one thing that I am hesitating about, is that I prefer to keep CheckISODaysRange separate from ISODateTimeWithinLimits, so that we can later remove CheckISODaysRange when tc39/ecma262#1087 is fixed. (Depending on what behaviour is chosen for what is currently unspecified behaviour.)

A more minor point is that I'd prefer, in a series of refactor commits, that every commit leaves the whole in a correct state. This is not the case for the first commit "Replace BalanceISODateTime with subtraction" which gives different behaviour on strings such as Temporal.Instant.from("-271821-04-19T23:00-01:00"). Is there any way to arrange the order of the commits so that this doesn't happen?

As I said in the previous review, I think a number of these refactors are worth having, so it would be good to get this in. The two main sticking points for me are that I'd like to be more certain that the refactors are not changing any behaviour, and I'd prefer to keep CheckISODaysRange.

ptomato · 2025-02-18T06:08:01Z

spec/plaindatetime.html

        1. Let _adjustedDate_ be _isoDateTime2_.[[ISODate]].
-        1. If _timeSign_ = _dateSign_, then
-          1. Set _adjustedDate_ to BalanceISODate(_adjustedDate_.[[Year]], _adjustedDate_.[[Month]], _adjustedDate_.[[Day]] + _timeSign_).
+        1. If _timeSign_ = -_dateSign_, then


I'm not sure this negation is correct.

anba · 2025-02-20T10:27:50Z

I think the one thing that I am hesitating about, is that I prefer to keep CheckISODaysRange separate from ISODateTimeWithinLimits, so that we can later remove CheckISODaysRange when tc39/ecma262#1087 is fixed. (Depending on what behaviour is chosen for what is currently unspecified behaviour.)

The proposed changes are unrelated to tc39/ecma262#1087. Instead they are related to tc39/ecma262#3464.

…ateTime

…bleEpochNanoseconds

… callers

…meFor

…ore GetEpochNanosecondsFor

ptomato · 2025-11-06T01:04:30Z

I rebased this and will take another look in the context of https://bugzilla.mozilla.org/show_bug.cgi?id=1998020

catamorphism · 2025-11-13T02:31:35Z

I went through and implemented the changes in the polyfill. Some tests failed and necessitated spec changes. #3184 shows what had to be changed.

ptomato · 2025-12-11T19:28:06Z

We discussed this in the Temporal meeting of 2025-12-11. First of all, thanks for bearing with us for this long. The conclusion is that we'll adopt some of the editorial refactors from this PR (which @catamorphism has split out in #3204) but we would prefer to keep CheckISODaysRange as it is.

@anba If you still feel it is necessary to make a normative change, please open a narrower PR that just contains the normative change.

fabon-f · 2026-01-22T13:18:31Z

Is it planned to fix CheckISODaysRange behavior as a spec bug, or did champions already agree to leave it as-is?

Personally I feel current spec is inconsistent. For example, off-by-one error in CheckISODaysRange breaks the roundtrippability between serialization and deserialization:

const zdt = new Temporal.ZonedDateTime(-8640000000000000000000n, 'America/New_York');
Temporal.ZonedDateTime.from(zdt.toString()); // RangeError!!!

ptomato · 2026-01-23T02:18:21Z

Is it planned to fix CheckISODaysRange behavior as a spec bug, or did champions already agree to leave it as-is?

Personally I feel current spec is inconsistent. For example, off-by-one error in CheckISODaysRange breaks the roundtrippability between serialization and deserialization:
const zdt = new Temporal.ZonedDateTime(-8640000000000000000000n, 'America/New_York');
Temporal.ZonedDateTime.from(zdt.toString()); // RangeError!!!

The bug is in MakeDay which behaves differently in different JS implementations (tc39/ecma262#1087) because of underspecified language. Obviously we'd like this to be fixed, but are not going to block Temporal on it. CheckISODaysRange exists because we think it's preferable that code like the above snippet throws, instead of returning potentially different results in each browser.

fabon-f · 2026-01-23T07:56:00Z

ISODateWithinLimits accepts -271821-04-19 but CheckISODaysRange rejects it. Is such discrepancy also intentional?

The spec says ISODateTimeWithinLimits can call GetUTCEpochNanoseconds with the date time such as -271821-04-19T00:00:01 and expects the correct result. Is there any reason to reject -271821-04-19 in CheckISODaysRange?

ptomato · 2026-02-05T20:43:44Z

@fabon-f I appreciate you digging into this as it helps me put all the information into one place 😄

ISODateWithinLimits accepts -271821-04-19 but CheckISODaysRange rejects it. Is such discrepancy also intentional?

Yes.

The spec says ISODateTimeWithinLimits can call GetUTCEpochNanoseconds with the date time such as -271821-04-19T00:00:01 and expects the correct result.

You are probably right that this is inconsistent, and it would be safer to reject -271821-04-19T00:00:01 in ISODateTimeWithinLimits. But if implementations seem to be doing this correctly, I'd say we probably don't need to fix it at this point. (Technically even a date as late as -271821-04-30 hits underspecified behaviour in MakeDay, so CheckISODaysRange could be even worse...)

The other inconsistency you mentioned

const zdt = new Temporal.ZonedDateTime(-8640000000000000000000n, 'America/New_York');
Temporal.ZonedDateTime.from(zdt.toString()); // RangeError!!!

is unfortunate IMO, but I think it's the best choice given the constraints. If you supply the epoch ns directly, it's in range. If you parse the string, the wall-clock part is out of range for MakeDay, then you add the 4h offset to get back in range. If MakeDay was fixed, this could be changed to not throw.

Here is a list of commits with further context:

anba · 2026-03-03T20:15:49Z

As mentioned last year:

The proposed changes are unrelated to tc39/ecma262#1087. Instead they are related to tc39/ecma262#3464.

The current Temporal spec tries to constrain milliseconds to the time value range ±8,640,000,000,000,000, because that's the limit spec'ed in ECMA-262 for some Date abstract operations. But that's a bug in ECMA-262, the actual permissible range is ±8,640,000,086,399,999 milliseconds for some of these operations. (In fact, ECMA-262 is already calling some abstract operations which have parameters defined as time value with values outside the time value range. That's the spec bug tc39/ecma262#3464 tries to fix.) Anyway, Temporal tries to be strict and only pass valid time values, so it imposes on itself the ±8,640,000,000,000,000 limit. I'd say it's now too late to try fixing this in Temporal, instead let's simply wait until Temporal is integrated into ECMA-262, then I (or someone else) can finish tc39/ecma262#3464, and then CheckISODaysRange can be replaced with ISODateTimeWithinLimits.

ptomato requested changes Oct 26, 2024

View reviewed changes

anba force-pushed the balance-iso-date branch from 15f103a to 9578597 Compare November 5, 2024 10:46

ptomato force-pushed the balance-iso-date branch from 9578597 to e0d9cb0 Compare February 15, 2025 01:51

ptomato requested changes Feb 18, 2025

View reviewed changes

anba added 15 commits November 5, 2025 16:37

Editorial: Replace BalanceISODateTime with subtraction

e85649e

Editorial: Replace BalanceISODateTime with AddOffsetNanosecondsToISOD…

d4d48ea

…ateTime

Editorial: Replace BalanceISODate with AddDaysToISODate

e3d180c

Editorial: Switch +1/-1 to avoid negating sign

c319ae2

Editorial: Move CheckISODaysRange to the top

66a1ab1

Editorial: Define CheckISODaysRange in terms of ISODateTimeWithinLimits

a69662a

Editorial: Pass ISO Date-Time Record to InterpretISODateTimeOffset

9629a58

Editorial: Pass ISO Date-Time Record to CheckISODaysRange

cae3d1a

Editorial: Assert ISO date-time is within limits in DisambiguatePossi…

1b176b1

…bleEpochNanoseconds

Editorial: Add assertion that epoch nanoseconds are valid

a929065

Editorial: Move CheckISODaysRange from GetPossibleEpochNanoseconds to…

dc59301

… callers

Editorial: Move CheckISODaysRange from GetEpochNanosecondsFor to callers

1b4484d

Editorial: Inline AddOffsetNanosecondsToISODateTime into GetISODateTi…

10cadf9

…meFor

Editorial: Replace ISODateTimeWithinLimits with CheckISODaysRange bef…

d1529f5

…ore GetEpochNanosecondsFor

Editorial: Inline CheckISODaysRange

014eb8b

ptomato force-pushed the balance-iso-date branch from e0d9cb0 to 014eb8b Compare November 6, 2025 01:03

catamorphism mentioned this pull request Nov 13, 2025

Replace BalanceISODate(Time) and rearrange time zone offset checks #3184

Closed

ptomato closed this Dec 11, 2025

Conversation

anba commented Oct 17, 2024

Uh oh!

codecov bot commented Oct 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ptomato left a comment

Choose a reason for hiding this comment

Uh oh!

ptomato Oct 25, 2024

Choose a reason for hiding this comment

Uh oh!

anba Oct 28, 2024

Choose a reason for hiding this comment

Uh oh!

ptomato Oct 25, 2024

Choose a reason for hiding this comment

Uh oh!

ptomato Oct 25, 2024

Choose a reason for hiding this comment

Uh oh!

anba Oct 28, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ptomato left a comment

Choose a reason for hiding this comment

Uh oh!

ptomato Feb 18, 2025

Choose a reason for hiding this comment

Uh oh!

anba commented Feb 20, 2025

Uh oh!

ptomato commented Nov 6, 2025

Uh oh!

catamorphism commented Nov 13, 2025

Uh oh!

ptomato commented Dec 11, 2025

Uh oh!

fabon-f commented Jan 22, 2026

Uh oh!

ptomato commented Jan 23, 2026

Uh oh!

fabon-f commented Jan 23, 2026

Uh oh!

ptomato commented Feb 5, 2026

Uh oh!

anba commented Mar 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Oct 17, 2024 •

edited

Loading