Skip to content

Intl.DateTimeFormat: do not set [[DayPeriod]] from the AM/PM pattern field - #373

Open
robobun wants to merge 1 commit into
mainfrom
farm/e88055a8/intl-dayperiod-ampm
Open

Intl.DateTimeFormat: do not set [[DayPeriod]] from the AM/PM pattern field#373
robobun wants to merge 1 commit into
mainfrom
farm/e88055a8/intl-dayperiod-ampm

Conversation

@robobun

@robobun robobun commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator

setFormatsFromPattern maps the resolved ICU pattern back into the fields that resolvedOptions() 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-402 dayPeriod option actually requests). As a result any 12-hour component formatter reported resolvedOptions().dayPeriod === "short" even though no dayPeriod option was passed.

Feeding those options back into the constructor then genuinely requests dayPeriod, and buildSkeleton turns that into 'B', so the round-trip changes "10:05 AM" into "10:05 in the morning":

const t = Date.UTC(2026, 0, 1, 10, 5);
const a = new Intl.DateTimeFormat("en-US", { timeZone: "UTC", hour: "numeric", minute: "2-digit" });
const ro = a.resolvedOptions();
const b = new Intl.DateTimeFormat("en-US", ro);
console.log(ro.dayPeriod, a.format(t), b.format(t));
// before: "short" "10:05 AM" "10:05 in the morning"
// after:  undefined "10:05 AM" "10:05 AM"   (matches V8/Node)

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 to the "dayPeriod" part type independently) is unaffected. The one stress-test expectation that encoded the old behaviour is updated.

…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.
@coderabbitai

coderabbitai Bot commented Jul 29, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 11 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 5556bf38-f834-428c-bf7f-12270e1f2e8a

📥 Commits

Reviewing files that changed from the base of the PR and between 34c01d1 and 0c1a9be.

📒 Files selected for processing (2)
  • JSTests/stress/intl-datetimeformat.js
  • Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp

Comment @coderabbitai help to get the list of available commands.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM — small, well-scoped ECMA-402 conformance fix.

What was reviewed:

  • Confirmed m_dayPeriod is only read by resolvedOptions() (IntlDateTimeFormat.cpp:1352); no other consumers.
  • Confirmed formatToParts maps UDAT_AM_PM_FIELD"dayPeriod" independently (line 1450), so part types are unaffected.
  • Confirmed buildSkeleton reads the user-supplied dayPeriod local, not m_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, dayPeriod maps to pattern 'B', and 'a' is the hour-cycle-driven AM/PM marker with no corresponding resolvedOptions field — the fix is spec-correct.
  • The existing stress test already asserts that formatToParts still 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.

@github-actions

Copy link
Copy Markdown

Preview Builds

Commit Release Date
0c1a9be2 autobuild-preview-pr-373-0c1a9be2 2026-07-29 12:42:14 UTC

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.

1 participant