IntlLocale: normalize ICU's "yes" sentinel in keywordValue; scope "true"->"" to kf - #351
IntlLocale: normalize ICU's "yes" sentinel in keywordValue; scope "true"->"" to kf#351robobun wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 14 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 (1)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small, well-scoped fix that aligns Intl.Locale keyword getters with V8 and UTS #35.
What was reviewed:
- Confirmed
ASCIILiteral == ASCIILiteralis defined (ASCIILiteral.h:112), so thekey == "colcasefirst"_sguard compiles and matches onlycaseFirst(). - Traced all six
keywordValue()callers: onlycaseFirstpasses"colcasefirst";numeric()short-circuits viaisBooleanbefore the new code, soknhandling is unchanged. - Checked that no non-kf key has
"yes"as a legitimate BCP 47 type, so the"yes"→"true"normalization can't clobber a real value.
Extended reasoning...
Overview
This PR modifies a single function, IntlLocale::keywordValue() in Source/JavaScriptCore/runtime/IntlLocale.cpp, replacing one line with nine. It fixes two related issues in how Intl.Locale getters (calendar, caseFirst, collation, hourCycle, numberingSystem, firstDayOfWeek) surface Unicode extension keyword values: (1) ICU's internal "yes" sentinel — used to represent a -u- key written without a type — was leaking to JavaScript; it is now normalized to the UTS #35 implied value "true"; (2) the pre-existing "true" → "" mapping applied to all keys, but is now scoped to colcasefirst (the kf key), where "true" is not part of the valid value set {upper, lower, false}.
Security risks
None. This is pure string post-processing on values already extracted by ICU. There is no new parsing, no allocation-size arithmetic, no user-controlled indexing, and no auth/permissions surface. The comparison key == "colcasefirst"_s is ASCIILiteral == ASCIILiteral, which is a well-defined content comparison in WTF.
Level of scrutiny
Low-to-moderate. The diff is tiny and mechanical, mirrors V8's UnicodeKeywordValue line-for-line (if (value == "yes") value = "true"; if (value == "true" && key == "kf") return "";), and the PR description includes a before/after/V8 verification table plus a reference to test262 intl402/Locale/getters.js. The one intentional behavior change beyond the bug fix — new Intl.Locale("en-u-ca-true").calendar now returns "true" instead of "" — is explicitly called out and matches V8.
Other factors
- All six non-boolean callers of
keywordValue()were audited in the same file; onlycaseFirst()passes"colcasefirst", so the scoped emptying hits exactly the intended getter. numeric()passesisBoolean = trueand returns before the modified code, so its raw== "yes"check is unaffected.- No CODEOWNERS gating, no outstanding reviewer comments (only a coderabbit rate-limit notice), and companion tests are landing in
oven-sh/bun. - The bug hunting system found no issues.
Preview Builds
|
…ue" -> "" to colcasefirst uloc_forLanguageTag stores a -u- keyword written without a type as "@key=yes" (ICU's internal no-value marker), and uloc_toUnicodeLocaleType passes "yes" through unchanged because it is syntactically a valid BCP 47 type. The getters (calendar/caseFirst/collation/firstDayOfWeek/hourCycle/ numberingSystem) then surface the raw "yes" to JavaScript, and because -xx-true canonicalizes to -xx, the value does not survive a new Intl.Locale(loc.toString()) round-trip. Normalize "yes" to the UTS #35 implied value "true", and restrict the existing "true" -> "" mapping to colcasefirst (kf), whose value set is {upper, lower, false} and so has no meaningful "true". The other keys now return "true" for a bare/"true"/"yes" value, matching V8 and test262 intl402/Locale/getters.js.
d568d3a to
5b1aa0a
Compare
There was a problem hiding this comment.
LGTM — small, well-scoped spec-conformance fix that mirrors V8's UnicodeKeywordValue exactly.
What was reviewed:
- Verified all 7
keywordValue()call sites: onlycaseFirst()passes"colcasefirst"_s, so the scoped"true"→""mapping hits the intended getter and no others. - Confirmed
ASCIILiteral == ASCIILiteraldoes content comparison viaequalSpans(not pointer equality), so the key check is sound. numeric()(colnumeric) takes theisBooleanearly return before the new code — unaffected as stated."yes"is not a valid registered type for any of ca/co/hc/nu/fw/kf, so unconditionally folding it to"true"matches V8 and can't shadow a real value.
Extended reasoning...
Overview
This PR modifies a single function, IntlLocale::keywordValue() in Source/JavaScriptCore/runtime/IntlLocale.cpp, adding two small branches: (1) normalize ICU's internal "yes" sentinel to "true" after uloc_toUnicodeLocaleType, and (2) narrow the pre-existing "true" → "" mapping so it only applies when key == "colcasefirst" rather than to every keyword. Net diff is +9/-1 with two of the added lines being logic and the rest comments.
Security risks
None. This is pure string-constant comparison and assignment on WTF::String / ASCIILiteral values inside an Intl getter helper. There is no user-controlled buffer arithmetic, no allocation change, no pointer manipulation, and no auth/permission surface. The inputs are already-validated ICU output strings.
Level of scrutiny
Low-to-moderate. This is an ECMA-402 conformance fix in a leaf helper with a small, closed set of callers (all in the same file). The PR description includes a before/after/V8 comparison table for 9 cases plus a reference to the exact V8 source pattern being matched, and a companion test is landing in oven-sh/bun. The behavioral change for non-kf keys (en-u-ca-true now returns "true" instead of "") is intentional and spec-aligned per UTS #35.
Other factors
I grepped for all keywordValue( call sites — there are exactly 7, all in IntlLocale.cpp, and only caseFirst() passes "colcasefirst"_s, so the scoped check is correct. I also confirmed that operator==(ASCIILiteral, ASCIILiteral) in WTF/wtf/text/ASCIILiteral.h performs content comparison via equalSpans (not pointer identity), so the key == "colcasefirst"_s test is sound even though the two literals are distinct objects. The isBoolean path (colnumeric) returns before the new code and still compares the raw buffer to "yes" directly, so numeric is unaffected. No outstanding reviewer comments; CodeRabbit was rate-limited and left no substantive review.
|
Rebased onto |
2 participants
Problem
Intl.Localekeyword getters leak ICU's internal"yes"sentinel when a-u-key is written without a value:"en-u-kf"and"en-u-kf-true"are the same canonical tag (toString confirms), but the getters disagree (""vs"yes"), sonew Intl.Locale(String(loc)).caseFirst !== loc.caseFirst.Cause
uloc_forLanguageTag("en-u-ca")produces"en@calendar=yes": ICU represents a keyword-without-type with the sentinel"yes".keywordValue()then reads that withuloc_getKeywordValueand passes it throughuloc_toUnicodeLocaleType, which returns"yes"unchanged (it is a well-formed 3-char type not present in any key's type map). The existingresult == "true"_scheck never matches.That existing check is also too broad: it maps
"true"to""for every key, sonew Intl.Locale("en-u-ca-true").calendarreturns""where V8 returns"true".Fix
In
IntlLocale::keywordValue:"yes"to"true"afteruloc_toUnicodeLocaleTypeso the sentinel never reaches JS."true"->""mapping tocolcasefirstonly.kf's value set is{upper, lower, false}, so"true"there has no meaning; the other keys (ca/co/hc/nu/fw) should expose"true"per UTS Support typed arrays up to 512 GB #35.This matches V8's
UnicodeKeywordValue(if (value == "yes") value = "true"; if (value == "true" && strcmp(key, "kf") == 0) return "";).Verification
Before / after, against Node v26.3.0:
en-u-kf"yes"""""en-u-kf-true""""""en-u-kf-upper"upper""upper""upper"en-u-ca"yes""true""true"en-u-ca-true"""true""true"en-u-ca-gregory"gregory""gregory""gregory"en-u-hc"yes""true""true"en-u-nu"yes""true""true"enundefinedundefinedundefinednumeric(kn) is unaffected: it takes theisBooleanearly-return and compares the raw buffer to"yes"directly.Bun-side test landing in oven-sh/bun:
test/js/web/intl/intl.test.tscovers bare /-true/-yesfor all six getters plus thetoString()round-trip.