Intl.DateTimeFormat: do not set [[DayPeriod]] from the AM/PM pattern field - #373
Intl.DateTimeFormat: do not set [[DayPeriod]] from the AM/PM pattern field#373robobun wants to merge 1 commit into
Conversation
…field setFormatsFromPattern maps the ICU pattern back into the resolvedOptions fields. It was treating 'a' (the AM/PM marker, which is emitted whenever the resolved hour cycle is h11/h12) the same as 'b'/'B' (the flexible day-period fields that the ECMA-402 dayPeriod option actually requests), so any 12-hour component formatter reported resolvedOptions().dayPeriod === "short" even though no dayPeriod option was passed. Feeding those options back into the constructor then requests dayPeriod for real, and buildSkeleton turns that into 'B', so the round-trip changes "10:05 AM" into "10:05 in the morning". Only 'b'/'B' populate m_dayPeriod now; 'a' is left to hourCycle/hour12. m_dayPeriod is read exclusively by resolvedOptions(), so formatToParts (which maps UDAT_AM_PM_FIELD independently) is unaffected. Matches V8.
|
Warning Review limit reached
Next review available in: 11 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
There was a problem hiding this comment.
LGTM — small, well-scoped ECMA-402 conformance fix.
What was reviewed:
- Confirmed
m_dayPeriodis only read byresolvedOptions()(IntlDateTimeFormat.cpp:1352); no other consumers. - Confirmed
formatToPartsmapsUDAT_AM_PM_FIELD→"dayPeriod"independently (line 1450), so part types are unaffected. - Confirmed
buildSkeletonreads the user-supplieddayPeriodlocal, notm_dayPeriod, so skeleton generation is unchanged. - Test expectation update matches the new behavior (drops the spurious
"dayPeriod":"short").
Extended reasoning...
Overview
The PR changes one switch case in IntlDateTimeFormat::setFormatsFromPattern (Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp) so that the ICU pattern character 'a' (AM/PM marker) no longer falls through to the 'b'/'B' cases that populate m_dayPeriod. It adds an explicit break with a comment citing the ECMA-402 date/time components table. One stress-test expectation in JSTests/stress/intl-datetimeformat.js is updated to drop the now-absent "dayPeriod":"short" from the expected resolvedOptions() JSON.
Security risks
None. This is a pure formatting-metadata fix in the Intl subsystem — no memory management, no user-controlled buffers, no auth/crypto/permissions surface. The switch case now does strictly less work than before.
Level of scrutiny
Low-to-moderate. The change is a 4-line early break in a pattern-parsing switch, plus a test expectation update. I traced all uses of m_dayPeriod: it is written only in setFormatsFromPattern and read only in resolvedOptions() (line ~1352). buildSkeleton takes a separate dayPeriod argument sourced from the user's options object, not from m_dayPeriod, so the actual formatting pattern is unaffected. formatToParts classifies AM/PM via UDAT_AM_PM_FIELD → "dayPeriod" in partTypeString (line ~1450), which is independent of m_dayPeriod — matching the PR description's claim.
Other factors
- The PR description correctly explains the round-trip bug (
resolvedOptions()→ constructor changes'a'into'B', turning "10:05 AM" into "10:05 in the morning") and notes the fix aligns with V8/Node. - Per ECMA-402's components table,
dayPeriodmaps to pattern'B', and'a'is the hour-cycle-driven AM/PM marker with no correspondingresolvedOptionsfield — the fix is spec-correct. - The existing stress test already asserts that
formatToPartsstill emits{type: 'dayPeriod', value: 'AM'}for the AM/PM marker (unchanged expectations at lines ~682 and elsewhere), confirming no regression there. - No prior human reviews or outstanding comments; only a CodeRabbit rate-limit notice.
Preview Builds
|
setFormatsFromPatternmaps the resolved ICU pattern back into the fields thatresolvedOptions()reports. It was treating'a'(the AM/PM marker, emitted whenever the resolved hour cycle is h11/h12) the same as'b'/'B'(the flexible day-period fields that the ECMA-402dayPeriodoption actually requests). As a result any 12-hour component formatter reportedresolvedOptions().dayPeriod === "short"even though nodayPeriodoption was passed.Feeding those options back into the constructor then genuinely requests
dayPeriod, andbuildSkeletonturns that into'B', so the round-trip changes"10:05 AM"into"10:05 in the morning":Only
'b'/'B'populatem_dayPeriodnow;'a'is left tohourCycle/hour12.m_dayPeriodis read exclusively byresolvedOptions(), soformatToParts(which mapsUDAT_AM_PM_FIELDto the"dayPeriod"part type independently) is unaffected. The one stress-test expectation that encoded the old behaviour is updated.