Skip to content

Conversation

@catamorphism
Copy link
Contributor

@catamorphism catamorphism commented Oct 13, 2025

f37c4da

[Temporal] Implement `with` method for PlainMonthDay

https://bugs.webkit.org/show_bug.cgi?id=300640

Reviewed by Yusuke Suzuki.

with() calls TemporalCalendar::monthDayFromFields(), which calls
TemporalCalendar::isoDateFromFields() -- hence the changes to the type
signature of isoDateFromFields(), which also change the type signatures
of various from() methods.

* JSTests/stress/temporal-plainmonthday.js:
(shouldBe):
* JSTests/test262/config.yaml:
* JSTests/test262/expectations.yaml:
* Source/JavaScriptCore/runtime/ISO8601.cpp:
(JSC::ISO8601::parseMonthCode):
(JSC::ISO8601::validMonthCode):
(JSC::ISO8601::monthFromCode): Deleted.
* Source/JavaScriptCore/runtime/ISO8601.h:
* Source/JavaScriptCore/runtime/TemporalCalendar.cpp:
(JSC::calendarResolveFields):
(JSC::TemporalCalendar::isoDateFromFields):
(JSC::TemporalCalendar::monthDayFromFields):
* Source/JavaScriptCore/runtime/TemporalCalendar.h:
* Source/JavaScriptCore/runtime/TemporalCalendarPrototype.cpp:
(JSC::JSC_DEFINE_HOST_FUNCTION):
* Source/JavaScriptCore/runtime/TemporalDuration.cpp:
(JSC::getEpochNanosecondsFor):
* Source/JavaScriptCore/runtime/TemporalDuration.h:
* Source/JavaScriptCore/runtime/TemporalObject.cpp:
(JSC::getMonthCode):
* Source/JavaScriptCore/runtime/TemporalObject.h:
* Source/JavaScriptCore/runtime/TemporalPlainDate.cpp:
(JSC::TemporalPlainDate::from):
(JSC::TemporalPlainDate::toPartialDate):
(JSC::TemporalPlainDate::with):
* Source/JavaScriptCore/runtime/TemporalPlainDate.h:
* Source/JavaScriptCore/runtime/TemporalPlainDateConstructor.cpp:
(JSC::JSC_DEFINE_HOST_FUNCTION):
* Source/JavaScriptCore/runtime/TemporalPlainDatePrototype.cpp:
(JSC::JSC_DEFINE_HOST_FUNCTION):
* Source/JavaScriptCore/runtime/TemporalPlainDateTime.cpp:
(JSC::TemporalPlainDateTime::from):
(JSC::TemporalPlainDateTime::with):
* Source/JavaScriptCore/runtime/TemporalPlainDateTime.h:
* Source/JavaScriptCore/runtime/TemporalPlainDateTimeConstructor.cpp:
(JSC::JSC_DEFINE_HOST_FUNCTION):
* Source/JavaScriptCore/runtime/TemporalPlainMonthDay.cpp:
(JSC::TemporalPlainMonthDay::with):
* Source/JavaScriptCore/runtime/TemporalPlainMonthDay.h:
* Source/JavaScriptCore/runtime/TemporalPlainMonthDayConstructor.cpp:
(JSC::JSC_DEFINE_HOST_FUNCTION):
* Source/JavaScriptCore/runtime/TemporalPlainMonthDayPrototype.cpp:
(JSC::JSC_DEFINE_HOST_FUNCTION):
(JSC::JSC_DEFINE_CUSTOM_GETTER):

Canonical link: https://commits.webkit.org/303360@main

dce8c5b

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 🧪 win-tests
✅ 🧪 webkitperl 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
🧪 ios-wk2-wpt 🧪 api-mac-debug ✅ 🛠 wpe-cairo
✅ 🛠 🧪 jsc ✅ 🧪 api-ios ✅ 🧪 mac-wk1 ✅ 🛠 gtk
✅ 🛠 🧪 jsc-arm64 ✅ 🛠 vision ✅ 🧪 mac-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim 🧪 mac-AS-debug-wk2 ✅ 🧪 api-gtk
✅ 🛠 🧪 merge ✅ 🧪 vision-wk2 ✅ 🧪 mac-wk2-stress 🛠 playstation
✅ 🛠 tv ✅ 🧪 mac-intel-wk2 ✅ 🛠 jsc-armv7
✅ 🛠 tv-sim ✅ 🛠 mac-safer-cpp ✅ 🧪 jsc-armv7-tests
✅ 🛠 watch
✅ 🛠 watch-sim

@catamorphism catamorphism requested a review from a team as a code owner October 13, 2025 18:29
@catamorphism catamorphism self-assigned this Oct 13, 2025
@catamorphism catamorphism added the JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues. label Oct 13, 2025
Copy link
Member

@Constellation Constellation left a comment

Choose a reason for hiding this comment

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

Overall, I'm very concerning about these double year / month / day.
It is very inefficient.
Can you provide what part of the spec makes it double instead of using integers?

@catamorphism
Copy link
Contributor Author

Overall, I'm very concerning about these double year / month / day. It is very inefficient. Can you provide what part of the spec makes it double instead of using integers?

@Constellation In the spec, Calendar Fields Records ( https://tc39.es/proposal-temporal/#sec-temporal-calendar-fields-records ) have integer fields; these are mathematical integers and not 32-bit integers.

In the code, isoDateFromFields (the overload that takes a JSObject* as the second argument) gets the year, month and day values directly from the JSObject, by calling toIntegerOrInfinity(), which returns a double. These double values are eventually converted to either int32_t or unsigned when the PlainDate (or PlainMonthDay, etc.) constructor is called, but I don't see a drawback to re-using these values so we can follow the spec in terms of exactly when validation is performed. No calculations are done on these double values; they're just passed around.

I can move the validation earlier if absolutely necessary, so that the doubles returned by toIntegerOrInfinity() are converted earlier, but I'm concerned about potential code duplication, potentially breaking tests that rely on checks being performed in a particular order, and departing (further) from the spec.

@catamorphism
Copy link
Contributor Author

@Constellation In 68c64c7 I changed the functions in questions to take int32_t / unsigned instead of double for year/month/day values. There's some code duplication, but not as much as I had thought there would be. Please take a look when you get a chance.

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Oct 18, 2025
@catamorphism catamorphism force-pushed the plainmonthday-plainyearmonth-part-4 branch from 68c64c7 to 89e2078 Compare October 18, 2025 21:51
Copy link
Member

@Constellation Constellation left a comment

Choose a reason for hiding this comment

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

Commented. Please check all error cases are annotated as [[unlikely]] and checking whether there is missing error cases.

@catamorphism catamorphism force-pushed the plainmonthday-plainyearmonth-part-4 branch from 3c4d9d5 to 88d508b Compare November 14, 2025 01:49
@catamorphism catamorphism force-pushed the plainmonthday-plainyearmonth-part-4 branch from 88d508b to dce8c5b Compare November 20, 2025 20:17
@catamorphism
Copy link
Contributor Author

Squashed and rebased.

@catamorphism catamorphism added request-merge-queue Request a pull request to be added to merge-queue once ready and removed merging-blocked Applied to prevent a change from being merged labels Nov 20, 2025
@ptomato ptomato added merge-queue Applied to send a pull request to merge-queue and removed request-merge-queue Request a pull request to be added to merge-queue once ready labels Nov 20, 2025
https://bugs.webkit.org/show_bug.cgi?id=300640

Reviewed by Yusuke Suzuki.

with() calls TemporalCalendar::monthDayFromFields(), which calls
TemporalCalendar::isoDateFromFields() -- hence the changes to the type
signature of isoDateFromFields(), which also change the type signatures
of various from() methods.

* JSTests/stress/temporal-plainmonthday.js:
(shouldBe):
* JSTests/test262/config.yaml:
* JSTests/test262/expectations.yaml:
* Source/JavaScriptCore/runtime/ISO8601.cpp:
(JSC::ISO8601::parseMonthCode):
(JSC::ISO8601::validMonthCode):
(JSC::ISO8601::monthFromCode): Deleted.
* Source/JavaScriptCore/runtime/ISO8601.h:
* Source/JavaScriptCore/runtime/TemporalCalendar.cpp:
(JSC::calendarResolveFields):
(JSC::TemporalCalendar::isoDateFromFields):
(JSC::TemporalCalendar::monthDayFromFields):
* Source/JavaScriptCore/runtime/TemporalCalendar.h:
* Source/JavaScriptCore/runtime/TemporalCalendarPrototype.cpp:
(JSC::JSC_DEFINE_HOST_FUNCTION):
* Source/JavaScriptCore/runtime/TemporalDuration.cpp:
(JSC::getEpochNanosecondsFor):
* Source/JavaScriptCore/runtime/TemporalDuration.h:
* Source/JavaScriptCore/runtime/TemporalObject.cpp:
(JSC::getMonthCode):
* Source/JavaScriptCore/runtime/TemporalObject.h:
* Source/JavaScriptCore/runtime/TemporalPlainDate.cpp:
(JSC::TemporalPlainDate::from):
(JSC::TemporalPlainDate::toPartialDate):
(JSC::TemporalPlainDate::with):
* Source/JavaScriptCore/runtime/TemporalPlainDate.h:
* Source/JavaScriptCore/runtime/TemporalPlainDateConstructor.cpp:
(JSC::JSC_DEFINE_HOST_FUNCTION):
* Source/JavaScriptCore/runtime/TemporalPlainDatePrototype.cpp:
(JSC::JSC_DEFINE_HOST_FUNCTION):
* Source/JavaScriptCore/runtime/TemporalPlainDateTime.cpp:
(JSC::TemporalPlainDateTime::from):
(JSC::TemporalPlainDateTime::with):
* Source/JavaScriptCore/runtime/TemporalPlainDateTime.h:
* Source/JavaScriptCore/runtime/TemporalPlainDateTimeConstructor.cpp:
(JSC::JSC_DEFINE_HOST_FUNCTION):
* Source/JavaScriptCore/runtime/TemporalPlainMonthDay.cpp:
(JSC::TemporalPlainMonthDay::with):
* Source/JavaScriptCore/runtime/TemporalPlainMonthDay.h:
* Source/JavaScriptCore/runtime/TemporalPlainMonthDayConstructor.cpp:
(JSC::JSC_DEFINE_HOST_FUNCTION):
* Source/JavaScriptCore/runtime/TemporalPlainMonthDayPrototype.cpp:
(JSC::JSC_DEFINE_HOST_FUNCTION):
(JSC::JSC_DEFINE_CUSTOM_GETTER):

Canonical link: https://commits.webkit.org/303360@main
@webkit-commit-queue webkit-commit-queue force-pushed the plainmonthday-plainyearmonth-part-4 branch from dce8c5b to f37c4da Compare November 20, 2025 23:53
@webkit-commit-queue
Copy link
Collaborator

Committed 303360@main (f37c4da): https://commits.webkit.org/303360@main

Reviewed commits have been landed. Closing PR #52251 and removing active labels.

@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Nov 20, 2025
@webkit-commit-queue webkit-commit-queue merged commit f37c4da into WebKit:main Nov 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants