Normative: Read date-time options only once when creating DateTimeFormat objects

[anba]

The first two commits are purely editorial and only move some things around, so it's easier to review the third commit, which contains the actual change.

Fixes #237 Fixes #706

[sffc]

TG2 discussion: https://github.com/tc39/ecma402/blob/master/meetings/notes-2022-10-06.md#normative-read-date-time-options-only-once-when-creating-datetimeformat-objects

Conclusion: Review the Test262 impact. Further discussion is not necessary in this group unless red flags show up. Approved with that condition.

[ryzokuken]

@gibson042 any updates?

[Ms2ger]

Please also note the changes going in a similar direction in Temporal. (I don't have the time right now to review if any changes would be helpful here or there.)

[anba]

Conclusion: Review the Test262 impact.

Test262 PR created at https://github.com/tc39/test262/pull/3768.

[justingrant]

Please also note the changes going in a similar direction in Temporal. (I don't have the time right now to review if any changes would be helpful here or there.)

I filed https://github.com/tc39/proposal-temporal/issues/2473 to adapt to this change on the Temporal side.

[gibson042]

I confirmed with engine262 that the test changes at https://github.com/tc39/test262/pull/3768 are the correct updates for this PR: https://github.com/engine262/engine262/compare/3248ccc6793a4de3ca6cab1d3a16a113ddc8d0c9...gibson042:ecma402-gh-709-initializedatetimeformat

And since test changes are required, it's worth looking critically at the new read order proposed here [spoiler—I think slight modification is warranted]:

1. localeMatcher [affects locale resolution, common to all service constructors]
2. calendar [affects locale resolution]
3. numberingSystem [affects locale resolution]
4. hour12 [affects locale resolution]
5. hourCycle [affects locale resolution]
6. timeZone [e.g., "America/New_York"]
7. component-specific configuration
   1. weekday
   2. era
   3. year
   4. month
   5. day
   6. dayPeriod
   7. hour
   8. minute
   9. second
   10. fractionalSecondDigits
   11. timeZoneName
8. formatMatcher ["basic" vs. "best fit" output pattern selection hint]
9. dateStyle [date-specific output pattern override]
10. timeStyle [time-specific output pattern override]

Cross-facility comparison:

• DisplayNames reads localeMatcher, style ["narrow", "short", "long"], type ["language", "region", etc.], and then narrow configuration.
• ListFormat reads localeMatcher, type ["conjunction", "disjunction", "unit"], style ["long", "short", "narrow"].
• NumberFormat reads localeMatcher, numberingSystem, style ["decimal", "currency", "unit", etc.—analogous to type in other facilities], style detail fields, and then a hodgepodge of other configuration (notation, compactDisplay ["short", "long"—somewhat similar to style in other facilities], comma and sign control, etc.).
• RelativeTimeFormat reads localeMatcher, numberingSystem, style ["long", "short", "narrow"], and then configuration detail.

---

Considering the above, I think it does make sense to read calendar and numberingSystem where they are. formatMatcher and {date,time}Style form a style/type-like group that should probably precede reading component-specific fields, better aligning with DisplayNames and NumberFormat and RelativeTimeFormat. That leaves timeZone, which acts as input data augmentation and doesn't really have a great analogue in other facilities—I think it can make sense either immediately before or immediately after the aforementioned style group. I'm inclined to say "before", making the overall pattern "locale" then "context" then "style/type" then "details".

Concretely, I propose the narrow change here of moving the formatMatcher/dateStyle/timeStyle block immediately before the components-table loop and also reordering it to dateStyle then timeStyle then formatMatcher (since formatMatcher is subordinate to the other fields, only having an effect when they are both undefined).

EDIT: But I'm also fine with not reordering, since it may not survive #747 anyway.

[anba]

EDIT: But I'm also fine with not reordering, since it may not survive #747 anyway.

Yeah, let's do any additional reordering as part of #747. That way this PR is restricted to fix the TOCTU issue described in #237.

[anba]

@ryzokuken I think everything is ready to land this change. It's not necessary to have this for the 2023 release, but I'd like to see this landed soon-ish, because it should help to reduce the Intl.DateTimeFormat changes for Temporal.

[ryzokuken]

@anba I understand, I missed that Richard had finished the review above. Two final things: this needs a rebase and did we get consensus from both TG1 and TG2?

[anba]

TG2 approved unless test262 changes show a major impact. From Oct 06 2022 meeting notes:

Review the Test262 impact. Further discussion is not necessary in this group unless red flags show up. Approved with conditions.

[anba]

this needs a rebase and did we get consensus from both TG1 and TG2?

Rebase is done. I don't think this was presented to TG1, at least I didn't see any references to this PR in the published notes.

[ryzokuken]

@anba I went through the over a year's worth of editors' updates and couldn't find this PR in any, so there's a very high chance this was never presented to plenary. I'll do so at the next meeting.

[sffc]

TG2 agreement: https://github.com/tc39/ecma402/blob/master/meetings/notes-2023-04-06.md#normative-read-date-time-options-only-once-when-creating-datetimeformat-objects-709

[anba]

Hi @ryzokuken, I don't see this issue being discussed at the May meeting. Do you plan to present it at the July meeting?

Thanks, André

[ryzokuken]

Hi @anba! I indeed missed the fact that there's consensus here. I'll present it in the July meeting.

[ryzokuken]

This got approval at TG1, merging after merge conflict is fixed.

[FrankYFTang]

The following tests in test262 need to be updated to match this PR

'intl402/DateTimeFormat/constructor-options-order-dayPeriod': [FAIL], 'intl402/DateTimeFormat/constructor-options-order': [FAIL], 'intl402/DateTimeFormat/constructor-options-order-fractionalSecondDigits': [FAIL], 'intl402/DateTimeFormat/constructor-options-order-timedate-style': [FAIL],

[FrankYFTang]

Fix tests in https://github.com/tc39/test262/pull/3879

[anba]

The following tests in test262 need to be updated to match this PR

The test updates are in https://github.com/tc39/test262/pull/3768.

[FrankYFTang]

please rebase, resolve conflict and merge. Thanks

[justingrant]

@ryzokuken - why is there a merge commit in this PR?

[ryzokuken]

@justingrant because I tried giving the GitHub interface a chance :/ Will rebase manually next time.

[ljharb]

@ryzokuken for future reference, there's an arrow next to "update branch" that lets you select "rebase branch", and it should work fine from the UI.

[ryzokuken]

After a lot of confusion, I realized that part of my troubles fixing up the merge issues was that the commits were already in the main branch? https://github.com/tc39/ecma402/commit/02bd03a6ada4377c0bf414d65016a37defc7bd1b. Closing the PR, phew.