Skip to content

Conversation

@catamorphism
Copy link
Contributor

@catamorphism catamorphism commented Dec 3, 2025

7af7345

[Temporal] Add PlainYearMonth `from` method
https://bugs.webkit.org/show_bug.cgi?id=303503

Reviewed by Darin Adler and Sosuke Suzuki.

Add this method.

Co-authored-by: Darin Adler <33187212+darinadler@users.noreply.github.com>

* JSTests/stress/temporal-plainyearmonth.js:
(shouldThrow):
* JSTests/test262/config.yaml:
* Source/JavaScriptCore/runtime/TemporalCalendar.cpp:
(JSC::TemporalCalendar::isoDateFromFields):
* Source/JavaScriptCore/runtime/TemporalPlainYearMonth.cpp:
(JSC::TemporalPlainYearMonth::from):
* Source/JavaScriptCore/runtime/TemporalPlainYearMonth.h:
* Source/JavaScriptCore/runtime/TemporalPlainYearMonthConstructor.cpp:
(JSC::JSC_DEFINE_HOST_FUNCTION):

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

1e13a17

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-libwebrtc
✅ 🛠 🧪 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 December 3, 2025 23:06
@catamorphism catamorphism self-assigned this Dec 3, 2025
@catamorphism catamorphism added the JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues. label Dec 3, 2025
@catamorphism catamorphism force-pushed the plainmonthday-plainyearmonth-part-10 branch from aff2a2f to f5d2caf Compare December 3, 2025 23:08
auto dateTime = ISO8601::parseCalendarDateTime(string, TemporalDateFormat::YearMonth);
if (dateTime) [[likely]] {
auto [plainDate, plainTimeOptional, timeZoneOptional, calendarOptional] = WTFMove(dateTime.value());
if (calendarOptional && StringView(calendarOptional.value()) != String::fromLatin1("iso8601")) [[unlikely]] {
Copy link
Member

Choose a reason for hiding this comment

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

Please do not use String::fromLatin1 for ASCII literals. We need to remove this from existing code and stop adding it in new code!

Suggested change
if (calendarOptional && StringView(calendarOptional.value()) != String::fromLatin1("iso8601")) [[unlikely]] {
if (calendarOptional && calendarOptional.value() != "iso8601"_s) [[unlikely]] {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Happy to take out the fromLatin1, but it won't compile without the StringView (calendarOptional.value() is a Vector<Latin1Character, maxCalendarLength>).

Copy link
Member

@darinadler darinadler Dec 4, 2025

Choose a reason for hiding this comment

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

Best idiom for comparing a Vector of Latin1Character with a string constant is to call the equal function that compares two spans, something like:

Suggested change
if (calendarOptional && StringView(calendarOptional.value()) != String::fromLatin1("iso8601")) [[unlikely]] {
if (calendarOptional && !equal(calendarOptional->span(), "iso8601"_span8)) [[unlikely]] {

Using StringView brings branches for handling of 16-bit characters into play, unnecessarily.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


DECLARE_INFO;

static TemporalPlainYearMonth* from(JSGlobalObject*, JSValue, std::optional<JSValue>);
Copy link
Member

Choose a reason for hiding this comment

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

I don’t see any code passing nullopt for this third argument. Are you sure that code path is exercised?

JSValue has a distinct zero value that is distinct from JavaScript null and undefined, so it can be used for a JSValue that might be omitted, passing zero value as appropriate. And often in other JavaScript contexts we can use null and undefined instead. Do we really need to use std::optional here? I can’t find where it is used and for what.

Typically when called from JavaScript, omitted arguments are represented with an JSValue of undefined.

Also, the argument meaning isn’t clear here without an argument name, so we should include that name here.

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 think the reason it was optional is that it's used that way in a future commit (I originally had a big monolithic PR for these changes and what I'm submitting now is the result of splitting it up), but using the zero value for JSValue seems fine.

Copy link
Member

Choose a reason for hiding this comment

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

I suspect we might want to use undefined rather than the zero value; I’d probably have to see more of the future code to know if that’s a good choice. But I also would have probably suggested making it non-optional and then add the ability to leave it out in that future commit.

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Dec 4, 2025
@darinadler
Copy link
Member

Could you squash the commits now?

@catamorphism catamorphism force-pushed the plainmonthday-plainyearmonth-part-10 branch from 4c22618 to 25e768d Compare December 4, 2025 22:44
@catamorphism
Copy link
Contributor Author

@darinadler Squashed.

}

JSObject* options = nullptr;
if (optionsValue) {
Copy link
Contributor

Choose a reason for hiding this comment

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

optionsValue is now just JSValue, so if (optionsValue) means checking if optionsValue is not zero value.
But I think what we should do here is checking if optionsValue is not undefined (or null) ?

Copy link
Contributor

Choose a reason for hiding this comment

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

If my comment makes sense, we should add more tests to check this behavior

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 think you're right, checking for undefined makes sense, because that's what would be passed in from temporalPlainYearMonthConstructorFuncFrom if no options argument was provided. I changed it. I also added stress tests for null and explicit-undefined options objects.

@catamorphism catamorphism force-pushed the plainmonthday-plainyearmonth-part-10 branch from dd954e8 to f924752 Compare December 8, 2025 20:06
@catamorphism catamorphism force-pushed the plainmonthday-plainyearmonth-part-10 branch from 11bbfd5 to 1e13a17 Compare December 8, 2025 21:52
@catamorphism catamorphism removed the merging-blocked Applied to prevent a change from being merged label Dec 8, 2025
@catamorphism catamorphism added the request-merge-queue Request a pull request to be added to merge-queue once ready label Dec 8, 2025
Copy link
Contributor

@sosukesuzuki sosukesuzuki left a comment

Choose a reason for hiding this comment

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

r=me

@sosukesuzuki sosukesuzuki 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 Dec 9, 2025
https://bugs.webkit.org/show_bug.cgi?id=303503

Reviewed by Darin Adler and Sosuke Suzuki.

Add this method.

Co-authored-by: Darin Adler <33187212+darinadler@users.noreply.github.com>

* JSTests/stress/temporal-plainyearmonth.js:
(shouldThrow):
* JSTests/test262/config.yaml:
* Source/JavaScriptCore/runtime/TemporalCalendar.cpp:
(JSC::TemporalCalendar::isoDateFromFields):
* Source/JavaScriptCore/runtime/TemporalPlainYearMonth.cpp:
(JSC::TemporalPlainYearMonth::from):
* Source/JavaScriptCore/runtime/TemporalPlainYearMonth.h:
* Source/JavaScriptCore/runtime/TemporalPlainYearMonthConstructor.cpp:
(JSC::JSC_DEFINE_HOST_FUNCTION):

Canonical link: https://commits.webkit.org/304135@main
@webkit-commit-queue webkit-commit-queue force-pushed the plainmonthday-plainyearmonth-part-10 branch from 1e13a17 to 7af7345 Compare December 9, 2025 02:06
@webkit-commit-queue
Copy link
Collaborator

Committed 304135@main (7af7345): https://commits.webkit.org/304135@main

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

@webkit-commit-queue webkit-commit-queue merged commit 7af7345 into WebKit:main Dec 9, 2025
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Dec 9, 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.

7 participants