IntlDateTimeFormat: only pass -u-hc- to UDateIntervalFormat when explicitly requested - #366
IntlDateTimeFormat: only pass -u-hc- to UDateIntervalFormat when explicitly requested#366robobun wants to merge 1 commit into
Conversation
…icitly requested createDateIntervalFormatIfNecessary and createTemporalIntervalFormat were appending `-u-hc-<m_hourCycle>` to the locale passed to udtitvfmt_open whenever m_hourCycle was populated. m_hourCycle is derived from the generated pattern in setFormatsFromPattern, so it is always non-None when an hour field is present, even when the user never specified hourCycle, hour12, or a -u-hc- locale extension. In ICU < 76 (before ICU-22669), DateTimePatternGenerator::addICUPatterns loads the locale's style patterns by creating DateFormat instances, and when the locale carries an hours keyword those instances regenerate their patterns from skeletons. For locales whose timeFormats patterns use literal separators (ja `H時mm分ss秒 zzzz`, ko `a h시 m분 s초 zzzz`, th `H นาฬิกา mm นาที ss วินาที zzzz`) the regenerated patterns fall back to the colon-separated availableFormats entries, so the generator's pattern map no longer contains the style pattern. DateIntervalFormat then produces `H:mm:ss` for range endpoints while the single-date UDateFormat (which never received -u-hc-) still produces `H時mm分ss秒`, and format(a) / formatRange(a,b) disagree. Store the hour cycle resolved from the locale extension and hourCycle option separately as m_explicitHourCycle (None when neither was specified) and use that when building the UDateIntervalFormat locale. The skeleton already encodes the 12/24-hour choice, and the locale default covers the h11/h12 and h23/h24 preference, so -u-hc- is only needed when the user explicitly overrode that preference.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughChangesThe formatter now records explicit hour-cycle input separately from pattern-derived state and uses that value when constructing ICU date and temporal interval formatters. Hour-cycle interval formatting
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any issues — the change is small, well-documented, and the root-cause analysis (ICU-22669) checks out. Deferring to a human only because this is a behavioral change in Intl spec-compliance code that interacts with ICU version-specific quirks.
What was reviewed:
- Traced
initializeDateTimeFormat—resolved.extensions[Hc]is null when neither-u-hc-norhourCycleis given (localeData default is null), som_explicitHourCyclecorrectly staysNonein the ja-JP repro. hour12path: setslocaleOptions[Hc] = String(), som_explicitHourCycleisNone; the 12/24 choice is still carried by the skeleton's h/H, matching the PR's reasoning.- Both interval-format call sites updated consistently;
m_hourCyclestill drivesresolvedOptions()and pattern rewriting, unchanged.
Extended reasoning...
Overview
Adds a new m_explicitHourCycle field to IntlDateTimeFormatImpl that captures the hour cycle as resolved from the input locale's -u-hc- extension and the hourCycle option (via resolveLocale), before setFormatsFromPattern overwrites m_hourCycle from the generated pattern. The two UDateIntervalFormat construction sites (createDateIntervalFormatIfNecessary and createTemporalIntervalFormat) now consult m_explicitHourCycle instead of m_hourCycle when deciding whether to append -u-hc- to the ICU locale string.
Security risks
None. This is pure Intl formatting output; no parsing of untrusted input beyond what already flows through resolveLocale/parseHourCycle, no memory management changes, no new allocations.
Level of scrutiny
Moderate. The diff itself is tiny (one field assignment, two use-site swaps, plus comments), but it changes observable formatRange output and works around an ICU-version-specific bug (ICU-22669, fixed in ICU 76). The correctness argument depends on resolveLocale semantics: the Hc keyLocaleData default is a null String() (IntlDateTimeFormat.cpp:225), so when the user supplies neither -u-hc- nor hourCycle, resolved.extensions[Hc] is null and parseHourCycle returns None — which is exactly the case the fix targets. When hour12 is set, localeOptions[Hc] is forced to null String() (line ~785), so m_explicitHourCycle is again None, and the 12/24 choice is carried by the skeleton's h/H character instead — consistent with the PR description's claim that the skeleton already encodes this and the locale default handles h11/h12 vs h23/h24 within each group.
Other factors
m_hourCycleretains all its other uses (resolvedOptions(),replaceHourCycleInPattern), so spec-visibleresolvedOptions().hourCycleis unchanged.- The PR description includes a direct ICU 75.1 reproduction of the underlying ICU bug and byte-for-byte verification against node/V8 for ja/ko/th plus all four explicit
hourCyclevalues, and notes tests land in oven-sh/bun. - Given this is a behavioral change in a spec-compliance area with ICU-version-dependent effects, I'd prefer a human familiar with the Intl code to sign off rather than auto-approving.
Preview Builds
|
…formatRange fix) Picks up oven-sh/WebKit#366: only pass -u-hc- to UDateIntervalFormat when the hour cycle was explicitly requested, so formatRange keeps the locale's timeStyle literal separators (ja/ko/th) instead of falling back to H:mm:ss. One commit on top of 549170099226f816.
2 participants
Problem
Intl.DateTimeFormat.prototype.formatRangewithtimeStyle: "full"(or"long") onja-JP,ko-KR,th-THrenders time with colon separators instead of the locale's literal-separated style pattern, whileformat()on the same formatter renders correctly:Same shape for
ko-KR(오후 1시 4분 5초becomes오후 1:04:05) andth-TH(13 นาฬิกา 04 นาที 05 วินาทีbecomes13:04:05).Cause
createDateIntervalFormatIfNecessaryappends-u-hc-<m_hourCycle>to the locale passed toudtitvfmt_openwheneverm_hourCycle != None. Butm_hourCycleis derived from the generated pattern insetFormatsFromPattern, so it is always populated when an hour field is present, even when the user never specifiedhourCycle,hour12, or a-u-hc-locale extension. For ja-JP withtimeStyle: "full"the locale becomesja-u-hc-h23instead ofja.In ICU < 76 (before ICU-22669),
DateTimePatternGenerator::addICUPatternsloads the locale's standard style patterns by creatingDateFormatinstances, and when the locale carries anhourskeyword those instances go through a path that regenerates their patterns from skeletons. For locales whosetimeFormats/fullpattern uses literal separators the regenerated pattern falls back to the colon-separatedavailableFormatsentry.DateIntervalFormat::initializePatternthen builds its fallback patterns from that generator and producesH:mm:ssfor range endpoints, while the single-dateUDateFormat(which was opened without-u-hc-) still hasH時mm分ss秒.Confirmed directly against stock ICU 75.1:
Fix
Store the hour cycle resolved from the locale's
-u-hc-extension and thehourCycleoption separately asm_explicitHourCycle(None when neither was specified), and use that instead of the pattern-derivedm_hourCyclewhen building theUDateIntervalFormatlocale in bothcreateDateIntervalFormatIfNecessaryandcreateTemporalIntervalFormat.The skeleton passed to
udtitvfmt_openalready encodes the 12/24-hour choice (h vs H), and the locale default covers the h11/h12 and h23/h24 preference within each group, so-u-hc-is only needed to override that preference. ExplicithourCycle: "h11"etc. continues to work.Verification
Built Bun against local WebKit + stock ICU 75.1. Repro now matches node output byte-for-byte for ja/ko/th, and all
hourCycle: "h11"|"h12"|"h23"|"h24"cases still render the correct midnight hour informatRange. Test coverage lands in oven-sh/bun alongside the WEBKIT_VERSION bump.