Rebase through early October 2024 #312

ptomato · 2025-03-17T20:13:30Z

Includes a refactor of calendar operations.

In #2758 we kept the code paths for Duration.p.round() and Duration.p.total() the same. However, it is actually simpler to separate them in most places, except for NudgeToCalendarUnit. - Splits TotalRelativeDuration out of RoundRelativeDuration - Splits TotalTimeDuration out of RoundTimeDuration - Splits DifferenceZonedDateTimeWithTotal out of DifferenceZonedDateTimeWithRounding - Splits DifferencePlainDateTimeWithTotal out of DifferencePlainDateTimeWithRounding - Removes [[Total]] field from Nudge Result Record - Changes NudgeToCalendarUnit to return a record of { [[NudgeResult]], [[Total]] } Closes: #2947 UPSTREAM_COMMIT=ae6edf0262fed5adb35157e82c268254dff69c24

Fixes #2865 UPSTREAM_COMMIT=2c1e94e835b44286953d46fd5b3aeb3934234980

Move ISO Date Record creation out of the two branches of the algorithm. UPSTREAM_COMMIT=deff8b11db190c5ee5be5e22133db3b682532211

While investigating #2985 I noticed that ISODateToEpochDays was defined differently between the spec text and the reference code. Use 0-based months as in the spec text, to avoid confusion when comparing the two. Also I noticed that it can now be implemented more efficiently with GetUTCEpochMilliseconds. UPSTREAM_COMMIT=981ab0f374840bf7ae29e93b0f76641052aa7d16

We must avoid calling GetUTCEpochNanoseconds with dates that are too large because it is ill-defined in ECMA-262 what is supposed to happen. (See tc39/ecma262#1087). We had a condition in ToTemporalInstant that was supposed to prevent this, but was incorrect because subtracting the UTC offset could push the date out of range. Note that this is editorial, because IsValidEpochNanoseconds would throw anyway in this case even if GetUTCEpochNanoseconds was fully defined for large inputs. See: #2985 UPSTREAM_COMMIT=a2a16bccac9d79b2b7d2d24f8f8ab51ddbd49d3c

Similarly to the previous commit, we must avoid calling GetUTCEpochNanoseconds with dates that are too large because it is ill-defined in ECMA-262 what is supposed to happen. (See tc39/ecma262#1087). Previously to PR #2925, that could not happen because GetPossibleInstantsFor took a PlainDateTime object, but now it takes an ISO Date-Time Record. Note that this is editorial, because IsValidEpochNanoseconds would throw anyway in this case even if GetUTCEpochNanoseconds was fully defined for large inputs. UPSTREAM_COMMIT=80dd0718544f1dae6fe61ee2be81609544cb7319

Similarly to the previous commit, we must avoid calling GetUTCEpochNanoseconds with dates that are too large because it is ill-defined in ECMA-262 what is supposed to happen. (See tc39/ecma262#1087). Previously to PR #2925, that could not happen because we used a PlainDateTime object in InterpretISODateTimeOffset, but now we use an ISO Date-Time Record. Note that this is editorial, because IsValidEpochNanoseconds would throw anyway in this case even if GetUTCEpochNanoseconds was fully defined for large inputs. UPSTREAM_COMMIT=5d5582897686697862824f551645e03d1bb38e05

UPSTREAM_COMMIT=086d5f700f7851cba60d45b6ac33edf940c50e85

This is a shorthand operation, so use it instead of writing out all the fields with CreateISODateRecord. UPSTREAM_COMMIT=1b9ecf23492961fb69f8fd27b45bf4b99e140b09

In most cases, this means calling ISODateTimeToDateRecord on the ISO Date-Time Record. However, in the case of CombineISODateAndTimeRecord, I think it'd be more readable to just permit ISO Date-Time Records there, as we already did with the _time_ argument. Closes: #2990 UPSTREAM_COMMIT=a73f2482ae23b7f43d658fd63eb7df935c7e47d2

This is the only place it's called from. This refactor is part of consolidating the many calendar AOs. See: #2948 UPSTREAM_COMMIT=ae86a91b4c900cc9a3124874189620883359270e

They are identical except for clamping the day value, so if we pass 1 as the day value we get the same effect. The only caller was in CalendarYearMonthFromFields. See: #2948 UPSTREAM_COMMIT=444ec937cca777c2225132df6ff5d860da66bb72

This is the only place it's called from. This refactor is part of consolidating the many calendar AOs. See: #2948 UPSTREAM_COMMIT=0406eb1342197aa51385e9dde8d657ac577c5a63

Move the ISO 8601 code from CalendarDateFromFields and CalendarYearMonthFromFields into CalendarDateToISO. At this point it just consists of RegulateISODate anyway. To achieve this, we also drop the requirement that CalendarDateToISO checks its return value is in the representable range, instead moving that to CalendarDateFromFields and CalendarYearMonthFromFields since it is different in both cases. See: #2948 Closes: #2998 UPSTREAM_COMMIT=f3997ebab3e5ecfd018167337f14d48a924fdb55

Having two separate operations for this was already confusing just because of the names. Combine them into an ISO 8601 part and an implementation- defined part, as in CalendarDateToISO. See: #2948 UPSTREAM_COMMIT=2eb79f1dbd6837a136457e81d0c827d6d7464502

We had two versions of this code, one in the calendarImpl object and one in the helper object. Simply use the one in the helper object. UPSTREAM_COMMIT=435876ef6918d178d8bb436725eff51b4822edca

Move the ISO 8601-specific code from CalendarEra, CalendarYear, etc., into CalendarISOToDate. See: #2948 UPSTREAM_COMMIT=a35d4767925ee91708276c26d507a88138ca659b

Move the ISO 8601 code from ISOResolveMonth into CalendarResolveFields. See: #2948 UPSTREAM_COMMIT=8f903388cb23b2635201ead261d15e99c429e14b

These were all one line now, and should just call CalendarISOToDate. See: #2948 UPSTREAM_COMMIT=9202f1f4ffc436e0948097092cff6c1ec37ba530

Add a comment on where the formula (probably) came from, and give the variables more self-documenting names. UPSTREAM_COMMIT=8424055541b191589aacf4c97a33444a87ccb6d0

Split out some things into local variables for readability. UPSTREAM_COMMIT=8a790146e505254fbbeb93b1d5647aa562fff63f

…tion More hacks to deal with avoiding the circular import between calendar.mjs and ecmascript.mjs. UPSTREAM_COMMIT=b0e9b3a35313c6128b1fa122f12b6312999f7902

justingrant

Overall looks good. Nice work with the compex TS.

I had a few optional suggestions and questions.

justingrant · 2025-03-20T00:25:45Z

lib/calendar.ts

+  day: number;
+  dayOfWeek: number;
+  dayOfYear: number;
+  weekOfYear: { week: number; year: number } | { week: undefined; year: undefined };


Just to make sure I understand the intent: this type is either an object with both week and year present, or an object with week and year properties that are undefined ?

If that's correct, why not just use undefined (not an object) for that latter case?

Makes sense. I'll leave it as is so that the types match the upstream code for the time being, and I'll add this to my to-do list of minor fixes once the rebase is finished.

justingrant · 2025-03-20T00:28:25Z

lib/calendar.ts

+
+type ResolveFieldsReturn<Type extends ISODateToFieldsType> = Resolve<
+  CalendarFieldsRecord & {
+    year: Type extends 'date' ? number : never;


Yep, this seems correct. Another option would be to put the entire object types into the conditional type (one which would have year and one which wouldn't), but think how you do it here is good because any access of year should result in a compiler error.

justingrant · 2025-03-20T00:29:31Z

lib/calendar.ts

+  isoToDate<
+    Request extends Partial<Record<keyof CalendarDateRecord, true>>,
+    T extends {
+      [Field in keyof CalendarDateRecord]: Request extends { [K in Field]: true } ? CalendarDateRecord[Field] : never;


This also looks correct, although it's complex enough that I can't be 100% sure.

I'm not 100% sure either, but I'm confident enough to merge it.

justingrant · 2025-03-20T01:12:25Z

lib/ecmascript.ts

+  // The caller wants the offset to always win ('use') OR the caller is OK
+  // with the offset winning ('prefer' or 'reject') as long as it's valid
+  // for this timezone and date/time.
+  if (offsetBehaviour === 'exact' || offsetOpt === 'use') {


I forget: did we agree to standardize on British or American spellings in our code? I have no preference, other than we should match one standard throughout if possible.

TC39 uses British spelling in the specifications, so that's what I've been using.

justingrant · 2025-03-20T01:13:16Z

lib/ecmascript.ts

@@ -3391,8 +3421,8 @@ export function UnbalanceDateDurationRelative(dateDuration: DateDuration, plainR
  // balance years, months, and weeks down to days
  const isoDate = TemporalObjectToISODateRecord(plainRelativeTo);
  const later = CalendarDateAdd(GetSlot(plainRelativeTo, CALENDAR), isoDate, yearsMonthsWeeksDuration, 'constrain');
-  const epochDaysEarlier = ISODateToEpochDays(isoDate.year, isoDate.month, isoDate.day);
-  const epochDaysLater = ISODateToEpochDays(later.year, later.month, later.day);
+  const epochDaysEarlier = ISODateToEpochDays(isoDate.year, isoDate.month - 1, isoDate.day);


I don't know why these -1s are here, so I'll assume they're correct!

Just to match the spec text: https://tc39.es/proposal-temporal/#sec-isodatetoepochdays

justingrant · 2025-03-20T01:20:10Z

lib/plaindatetime.ts

  }
  get inLeapYear(): Return['inLeapYear'] {
    if (!ES.IsTemporalDateTime(this)) throw new TypeErrorCtor('invalid receiver');
    const isoDate = ES.TemporalObjectToISODateRecord(this);
-    return ES.CalendarInLeapYear(GetSlot(this, CALENDAR), isoDate);
+    return ES.calendarImplForObj(this).isoToDate(isoDate, { inLeapYear: true }).inLeapYear;


There's a lot of repetitive code here. Could you explain a bit about this pattern? If only one scalar is needed, I wonder if there's a simpler way to get it than this { fieldName: true } parameter followed by a property getter. Also the same pattern is repeated in every method, could it be DRY-ed? Unless I'm missing something, could all these 2-line repeated calls be coalesced into a single shared function with a signature like this?

ES.getCalendarProperty(this, 'inLeapYear')

There are a couple of other places where we fetch multiple scalars at once (e.g. ISODateToFields, calendarDateWeekOfYear.) But I like the suggestion to reduce the repetition in the many public property getters. I've added it to my to-do list of cleanups after the rebase is done.

ptomato · 2025-03-20T02:08:34Z

Thanks for the review!

ptomato and others added 10 commits March 17, 2025 13:09

Editorial: Recommend "remapping" for dates exceeding era year bounds

0e4efbe

Fixes #2865 UPSTREAM_COMMIT=2c1e94e835b44286953d46fd5b3aeb3934234980

Editorial: Simplify ISO Date Record in PlainDate.p.toZonedDateTime

2907efa

Move ISO Date Record creation out of the two branches of the algorithm. UPSTREAM_COMMIT=deff8b11db190c5ee5be5e22133db3b682532211

Editorial: Deduplicate ISODateTimeToDateRecord in NudgeToCalendarUnit

ff5c5b1

UPSTREAM_COMMIT=086d5f700f7851cba60d45b6ac33edf940c50e85

Editorial: Use ISODateTimeToDateRecord in more places

0031284

This is a shorthand operation, so use it instead of writing out all the fields with CreateISODateRecord. UPSTREAM_COMMIT=1b9ecf23492961fb69f8fd27b45bf4b99e140b09

ptomato requested review from justingrant and 12wrigja March 17, 2025 20:13

ptomato added 12 commits March 17, 2025 13:29

Editorial: Move DifferenceISODate into CalendarDateDifference

cfdecf2

This is the only place it's called from. This refactor is part of consolidating the many calendar AOs. See: #2948 UPSTREAM_COMMIT=ae86a91b4c900cc9a3124874189620883359270e

Editorial: Move ISOMonthDayFromFields into CalendarMonthDayFromFields

07e8b25

This is the only place it's called from. This refactor is part of consolidating the many calendar AOs. See: #2948 UPSTREAM_COMMIT=0406eb1342197aa51385e9dde8d657ac577c5a63

Polyfill: Deduplicate calendar daysInMonth implementation

732d4de

We had two versions of this code, one in the calendarImpl object and one in the helper object. Simply use the one in the helper object. UPSTREAM_COMMIT=435876ef6918d178d8bb436725eff51b4822edca

Editorial: Include ISO 8601 algorithms in CalendarISOToDate

4b5bc52

Move the ISO 8601-specific code from CalendarEra, CalendarYear, etc., into CalendarISOToDate. See: #2948 UPSTREAM_COMMIT=a35d4767925ee91708276c26d507a88138ca659b

Editorial: Include ISO 8601 algorithm in CalendarResolveFields

12a54d8

Move the ISO 8601 code from ISOResolveMonth into CalendarResolveFields. See: #2948 UPSTREAM_COMMIT=8f903388cb23b2635201ead261d15e99c429e14b

Editorial: Remove individual AOs for each calendar field

b39125a

These were all one line now, and should just call CalendarISOToDate. See: #2948 UPSTREAM_COMMIT=9202f1f4ffc436e0948097092cff6c1ec37ba530

Polyfill: Make day-of-week algorithm less opaque

6ae3ca4

Add a comment on where the formula (probably) came from, and give the variables more self-documenting names. UPSTREAM_COMMIT=8424055541b191589aacf4c97a33444a87ccb6d0

Polyfill: Make buildMonthCode less opaque

506f504

Split out some things into local variables for readability. UPSTREAM_COMMIT=8a790146e505254fbbeb93b1d5647aa562fff63f

Polyfill: Refactor GetIntrinsic('%calendarImpl%')(calendar) into func…

81603f4

…tion More hacks to deal with avoiding the circular import between calendar.mjs and ecmascript.mjs. UPSTREAM_COMMIT=b0e9b3a35313c6128b1fa122f12b6312999f7902

ptomato force-pushed the rebase branch from 1b306fc to 81603f4 Compare March 17, 2025 20:29

justingrant approved these changes Mar 20, 2025

View reviewed changes

ptomato merged commit a5040b7 into main Mar 20, 2025
19 checks passed

ptomato deleted the rebase branch March 20, 2025 02:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Rebase through early October 2024 #312

Rebase through early October 2024 #312

Uh oh!

ptomato commented Mar 17, 2025

Uh oh!

justingrant left a comment

Uh oh!

justingrant Mar 20, 2025

Uh oh!

ptomato Mar 20, 2025

Uh oh!

justingrant Mar 20, 2025

Uh oh!

justingrant Mar 20, 2025

Uh oh!

ptomato Mar 20, 2025

Uh oh!

justingrant Mar 20, 2025

Uh oh!

ptomato Mar 20, 2025

Uh oh!

justingrant Mar 20, 2025

Uh oh!

ptomato Mar 20, 2025

Uh oh!

justingrant Mar 20, 2025

Uh oh!

ptomato Mar 20, 2025

Uh oh!

ptomato commented Mar 20, 2025

Uh oh!

Uh oh!

Uh oh!

Rebase through early October 2024 #312

Rebase through early October 2024 #312

Uh oh!

Conversation

ptomato commented Mar 17, 2025

Uh oh!

justingrant left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ptomato commented Mar 20, 2025

Uh oh!

Uh oh!

Uh oh!