Skip to content

Rebase through early October 2024 #312

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

Merged
merged 22 commits into from
Mar 20, 2025
Merged

Rebase through early October 2024 #312

merged 22 commits into from
Mar 20, 2025

Conversation

ptomato
Copy link
Contributor

@ptomato ptomato commented Mar 17, 2025

Includes a refactor of calendar operations.

ptomato and others added 10 commits March 17, 2025 13:09
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
@ptomato ptomato requested review from justingrant and 12wrigja March 17, 2025 20:13
ptomato added 12 commits March 17, 2025 13:29
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
Copy link
Contributor

@justingrant justingrant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good. Nice work with the compex TS.

I had a few optional suggestions and questions.

day: number;
dayOfWeek: number;
dayOfYear: number;
weekOfYear: { week: number; year: number } | { week: undefined; year: undefined };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// 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') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}
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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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')

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

ptomato commented Mar 20, 2025

Thanks for the review!

@ptomato ptomato merged commit a5040b7 into main Mar 20, 2025
19 checks passed
@ptomato ptomato deleted the rebase branch March 20, 2025 02:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants