Replace BalanceISODate(Time) and rearrange time zone offset checks

[anba]

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

1. 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.
2. 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]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.14%. Comparing base (69f46d5) to head (e0d9cb0).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3014   +/-   ##
=======================================
  Coverage   96.14%   96.14%           
=======================================
  Files          21       21           
  Lines        9928     9928           
  Branches     1801     1801           
=======================================
  Hits         9545     9545           
  Misses        336      336           
  Partials       47       47           

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

[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]

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.

[anba]

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]

Ditto.

[ptomato]

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.

[anba]

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:

5. 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.

[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]

I'm not sure this negation is correct.

[anba]

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.