Intl: apply CLDR alias mappings during locale canonicalization - #381
Intl: apply CLDR alias mappings during locale canonicalization#381robobun wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 3 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 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline finding, I also checked the Darwin branch: the extern "C" forward-declaration of ualoc_canonicalForm matches the signature already used by the existing USE(APPLE_INTERNAL_SDK) callBufferProducingFunction call, so that path is fine — only the non-Darwin icu::Locale branch needs the U_SHOW_CPLUSPLUS_API / @no-unify treatment.
Extended reasoning...
Verified the inline bug is real: Source/WTF/wtf/Platform.h:121 sets U_SHOW_CPLUSPLUS_API 0 and the vendored locid.h guards its entire body on it, with no build-system override anywhere in the tree; Sources.txt:922 shows the JSDateMath.cpp @no-unify precedent for the fix. Separately confirmed the Darwin forward-declare path is sound — the C signature matches what callBufferProducingFunction already invokes under USE(APPLE_INTERNAL_SDK), and being C-linkage there's no ABI mismatch risk against system libicucore. Deferring to human review given the compile break on Linux/Windows.
ae1361c to
84b45c3
Compare
There was a problem hiding this comment.
LGTM — the revision addresses my earlier U_SHOW_CPLUSPLUS_API concern by dropping the icu::Locale call and forward-declaring a weak Bun__canonicalizeLocaleID C shim instead.
Checked: the weak-extern Bun__* pattern matches existing precedent (JSMicrotask.cpp, JSModuleLoader.cpp, ErrorInstance.cpp); the signature matches uloc_canonicalize so callBufferProducingFunction handles buffer growth identically; the runtime null-check keeps the jsc shell on the uloc_canonicalize fallback; and the #if/#elif refactor preserves the ualoc_canonicalForm path under USE(APPLE_INTERNAL_SDK).
Extended reasoning...
Overview
The PR reroutes canonicalizeLocaleIDWithoutNullTerminator in IntlObject.cpp so that Bun builds call a Bun-provided C wrapper (Bun__canonicalizeLocaleID) that applies CLDR alias mappings, instead of ICU's uloc_canonicalize which explicitly does not. The wrapper is declared extern "C" __attribute__((weak)) and null-checked at runtime so the standalone jsc shell — which doesn't link the Bun runtime — falls back to the old behavior. The trailing U_FAILURE check and return buffer were hoisted out of the #if ladder so all three branches share them; net diff is ~20 lines, all inside #if USE(BUN_JSC_ADDITIONS) guards.
Prior review
My earlier review flagged that the first revision included <unicode/locid.h> and called icu::Locale::createCanonical directly, which cannot compile under WebKit's U_SHOW_CPLUSPLUS_API=0. The author took option (b) from that feedback — avoid the ICU C++ API entirely and expose a C shim from the Bun side — so the concern is fully resolved. No icu:: symbols or extra ICU headers remain in the diff.
Security risks
None. The change only swaps one ICU-signature buffer-producing function for another inside callBufferProducingFunction, which already handles the resize-and-retry protocol. No new user-controlled data paths, no auth/crypto/permissions.
Level of scrutiny
Low. The weak-extern-C Bun__* hook pattern is already established in three other runtime files (Bun__reportUnhandledError, Bun__analyzeTranspiledModule, Bun__errorInstance__finalize), so both the __attribute__((weak)) usage and the address-of-weak-symbol null test are proven to build across Bun's CI matrix. The new branch is gated behind USE(BUN_JSC_ADDITIONS) && !USE(APPLE_INTERNAL_SDK), leaving upstream/Apple paths byte-identical. The actual CLDR alias logic lives in the Bun repo, so this PR is pure plumbing.
Other factors
The wrapper's signature is identical to uloc_canonicalize (const char*, char*, int32_t, UErrorCode* → int32_t), so callBufferProducingFunction's template instantiation and needsToGrowToProduceBuffer retry loop behave exactly as before. The #if/#elif/#else restructuring is mechanically correct — each arm assigns auto status, and the shared tail checks it and returns.
There was a problem hiding this comment.
Thanks — the revised approach resolves the earlier U_SHOW_CPLUSPLUS_API compile break by moving the ICU C++ call out to a Bun-side TU behind a weak extern "C" hook, matching the existing Bun__analyzeTranspiledModule / Bun__reportUnhandledError pattern. I didn't find any issues in this revision, but since it introduces a cross-repo ABI contract and changes user-visible Intl canonicalization output on all Bun platforms, it's worth a human sign-off.
Checked: weak-symbol pattern matches the four existing Bun__ hooks in runtime/ and wtf/bun/ (clang on all Bun targets, including Windows via xwin/clang-cl); callBufferProducingFunction accepts the function-pointer form and the signature matches uloc_canonicalize; the null-check preserves current behavior for the jsc shell; the hoisted U_FAILURE/return buffer tail is reachable from all three #if branches; and the declaration guard !USE(APPLE_INTERNAL_SDK) lines up with the #elif (Bun never sets APPLE_INTERNAL_SDK).
Extended reasoning...
Overview
The PR reworks canonicalizeLocaleIDWithoutNullTerminator in Source/JavaScriptCore/runtime/IntlObject.cpp so that Bun builds route locale canonicalization through a Bun-provided C wrapper (Bun__canonicalizeLocaleID) instead of ICU's uloc_canonicalize, which does not apply CLDR alias mappings. The wrapper is forward-declared as a weak extern "C" symbol; when absent (e.g. the standalone jsc shell) the code falls back to the existing uloc_canonicalize path. The trailing U_FAILURE check and return buffer were hoisted out of the #if ladder so all three branches share them.
This is a rewrite of an earlier revision that tried to call icu::Locale::createCanonical directly from this file; my prior review flagged that as a Linux/Windows compile break because Platform.h sets U_SHOW_CPLUSPLUS_API=0. The author took the suggested option (b) — avoid the ICU C++ API in this TU and delegate to a C shim implemented on the Bun side.
Security risks
None. The input (localeID) is already null-terminated and comes from localeIDBufferForLanguageTagWithNullTerminator after structural validation of the language tag; the wrapper has the same buffer-length/UErrorCode contract as uloc_canonicalize and is invoked through callBufferProducingFunction, which handles overflow-and-retry. No new attack surface.
Level of scrutiny
Medium. On the WebKit side this is a small, pattern-following hook (identical shape to Bun__reportUnhandledError in JSMicrotask.cpp, Bun__errorInstance__finalize in ErrorInstance.cpp, Bun__analyzeTranspiledModule in JSModuleLoader.cpp, and the WTFTimer__* hooks in RunLoopBun.cpp). Non-Bun builds are entirely unaffected by the #if guards. However, the effect is a user-visible change to Intl.Locale / Intl.getCanonicalLocales output on every Bun platform, and correctness depends on the Bun-side implementation (in a separate repo) matching the declared signature and semantics. That cross-repo contract is the piece a human should confirm lands in lockstep.
Other factors
- The
__attribute__((weak))+ runtime null-check idiom is already relied on by this fork on all its targets (macOS/Linux/Windows-via-clang-cl), so no new toolchain concern is introduced here. - I verified the declaration guard (
USE(BUN_JSC_ADDITIONS) && !USE(APPLE_INTERNAL_SDK)) is consistent with the use site: the#elif USE(BUN_JSC_ADDITIONS)branch is only reached whenUSE(APPLE_INTERNAL_SDK)is false (or ICU < 68, which Bun doesn't ship), and Bun never definesAPPLE_INTERNAL_SDK, so the symbol is always declared where it is used. - The refactor collapsing the two duplicate
if (U_FAILURE(status)) return std::nullopt; return buffer;tails into one after#endifis safe: each branch declaresauto statusin the enclosing scope. - My earlier inline comment on this PR is now resolved by the new approach; nothing from that review remains outstanding.
…nonicalizeLocaleID uloc_canonicalize is documented as "does NOT map aliased names in any way", so Intl.Locale and Intl.getCanonicalLocales skip CLDR language/region alias replacement on Bun builds (fre -> fr, en-UK -> en-GB, sh -> sr-Latn, SU -> RU/AM). Safari avoids this via Apple's ualoc_canonicalForm SPI, gated behind USE(APPLE_INTERNAL_SDK). The CLDR-aware canonicalization lives in icu::Locale::createCanonical (runs ICU's AliasReplacer since ICU 68), but WebKit compiles with U_SHOW_CPLUSPLUS_API=0 so the ICU C++ API is not reachable from JSC. Instead, forward-declare a C wrapper with the uloc_canonicalize signature and let Bun implement it: on macOS Bun forwards to ualoc_canonicalForm (exported from libicucore on every supported macOS version), and elsewhere Bun statically links ICU and can safely call icu::Locale::createCanonical. Fixes oven-sh/bun#14713.
84b45c3 to
edc9ed3
Compare
There was a problem hiding this comment.
Thanks for reworking this — the weak Bun__canonicalizeLocaleID shim resolves the U_SHOW_CPLUSPLUS_API compile-break I flagged on the earlier revision, and the WebKit-side change now looks correct to me. Deferring to a human since this changes user-facing Intl canonicalization semantics and introduces a cross-repo ABI contract that the Bun side needs to satisfy.
What was reviewed:
- Weak-symbol + runtime null-check pattern matches existing
Bun__errorInstance__finalize/Bun__reportUnhandledErrorexactly, including the paired-Wl,-U/-ulinker flags. callBufferProducingFunctionsignature —Bun__canonicalizeLocaleIDmirrorsuloc_canonicalize's(const char*, char*, int32_t, UErrorCode*), so the existing call shape applies.#ifrestructure — all three branches now share the trailingU_FAILUREcheck; no path lost its error handling.- Declaration guard
&& !USE(APPLE_INTERNAL_SDK)vs. usage guard#elif USE(BUN_JSC_ADDITIONS)— only diverges ifAPPLE_INTERNAL_SDKis on with ICU < 68, which isn't a Bun configuration.
Extended reasoning...
Overview
The PR routes canonicalizeLocaleIDWithoutNullTerminator through a Bun-provided C wrapper so Intl.Locale / Intl.getCanonicalLocales apply CLDR alias mappings (fre→fr, en-UK→en-GB, sh→sr-Latn). Two files touched: a ~15-line addition in IntlObject.cpp (weak extern declaration + ternary in the canonicalize helper + shared error tail) and two one-line linker-flag additions in WebKitCompilerFlags.cmake.
This is the second revision. The first called icu::Locale::createCanonical directly, which I flagged as unbuildable under U_SHOW_CPLUSPLUS_API=0; that comment is now resolved. The author took option (b) from my suggestion — avoid the ICU C++ API entirely and delegate to a Bun-side C shim — and updated the PR description to match.
Security risks
None identified. The input reaching Bun__canonicalizeLocaleID is the output of uloc_forLanguageTag on a string that already passed isStructurallyValidLanguageTag, so it is bounded and null-terminated. callBufferProducingFunction handles buffer sizing/growth. The weak symbol falls back to the current uloc_canonicalize path when unresolved, so there is no new attack surface in the standalone jsc shell.
Level of scrutiny
Medium. On the WebKit side this is now a small, pattern-following change: the extern "C" __attribute__((weak)) + runtime null-check idiom already appears for Bun__errorInstance__finalize, Bun__reportUnhandledError, Bun__analyzeTranspiledModule, and the WTFTimer__* hooks, and Dockerfile.windows documents Windows handling for it. The CMake edit slots the new symbol into the existing -Wl,-U/-u list alongside its peers.
What keeps this out of auto-approve territory is that it changes the observable output of spec-defined Intl APIs across every Bun build, and correctness hinges on the Bun-side implementation of Bun__canonicalizeLocaleID (in oven-sh/bun) honoring the uloc_canonicalize contract — buffer/status semantics, ICU-locale-ID input format, and CLDR-consistent output. That contract lives in another repo and can't be verified here, so a human should sign off on the pair.
Other factors
- The
#if/#elif/#elserestructure is a net simplification: theU_FAILUREcheck andreturn buffertail were previously duplicated per branch and are now shared. I traced each branch and none lost its error path. - Minor guard asymmetry: the declaration is behind
USE(BUN_JSC_ADDITIONS) && !USE(APPLE_INTERNAL_SDK), while the call site is reached underUSE(BUN_JSC_ADDITIONS) && !(ICU>=68 && APPLE_INTERNAL_SDK). These only disagree forAPPLE_INTERNAL_SDK && ICU<68, which is not a configuration Bun ships, so I did not flag it. - No test changes here; presumably test262 / Bun's Intl tests cover the behavioral fix on the Bun side.
Preview Builds
|
1 participant
uloc_canonicalizeis documented as "does NOT map aliased names in any way", so on Bun buildsIntl.LocaleandIntl.getCanonicalLocalesskip CLDR language/region alias replacement:Safari avoids this via Apple's
ualoc_canonicalFormSPI, currently gated behindUSE(APPLE_INTERNAL_SDK).The CLDR-aware canonicalization lives in
icu::Locale::createCanonical(runs ICU'sAliasReplacersince ICU 68), but WebKit compiles withU_SHOW_CPLUSPLUS_API=0(wtf/Platform.h), so the ICU C++ API is not reachable from JSC and cannot be made reachable without breaking unified-source header include order.Instead, forward-declare a weak C wrapper with the
uloc_canonicalizesignature and let Bun provide the implementation:ualoc_canonicalForm(exported fromlibicucoreon every supported macOS version; verified in the MacOSX 12.3/13.3/15.4/26.4 SDK.tbdfiles).icu::Locale::createCanonicalfrom a standalone TU compiled without the WebKit PCH.The
jscshell links without the symbol and falls back touloc_canonicalize.Fixes oven-sh/bun#14713.