Skip to content

Add coverage for uvalue "true" and "yes" - #5016

Merged
Ms2ger merged 2 commits into
tc39:mainfrom
anba:locale-yes
Sep 17, 2026
Merged

Ms2ger merged 2 commits into
tc39:mainfrom
anba:locale-yes

Conversation

@anba

@anba anba commented Apr 9, 2026

Copy link
Copy Markdown
Contributor

Ensure yes is only canonicalized to true for some ukeys.

Ensure `yes` is only canonicalized to `true` for some ukeys.
@anba
anba requested a review from a team as a code owner April 9, 2026 08:19
@anba

anba commented Apr 9, 2026

Copy link
Copy Markdown
Contributor Author

"constructor-unicode-extension-uvalue-yes.js" fails in JSC and V8 because of ICU4C bugs.

linusg
linusg previously requested changes Apr 9, 2026
Comment thread test/intl402/Locale/constructor-unicode-extension-uvalue-true.js
Comment thread test/intl402/Locale/constructor-unicode-extension-uvalue-yes.js
@anba

anba commented Apr 9, 2026

Copy link
Copy Markdown
Contributor Author

No other test file in "test/intl402" mentions either for-of or generators, so for consistency let's omit them here, too.

@github-actions

Copy link
Copy Markdown

2 new or modified tests were run on 7 engines.

View workflow run

Test Engine262 GraalJS JavaScriptCore QuickJS SpiderMonkey V8 XS
intl402/Locale/constructor-unicode-extension-uvalue-true.js ❌ ✅ ✅ ❌ ✅ ✅ ❌
intl402/Locale/constructor-unicode-extension-uvalue-yes.js ❌ ✅ ❌ ❌ ✅ ❌ ❌

@jugglinmike jugglinmike left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No conflicts and no overlap with more recent additions; this all looks good to me!

For those curious: here's the source for which fields have "yes" canonicalized to "true".

(We're missing a bit of coverage, but this patch has waited long enough--I'll follow up with one of my own.)

Comment on lines +18 to +28
// Generate all possible `ukey` values.
function* ukeys() {
const lowerA = 'a'.charCodeAt(0);
const lowerZ = 'z'.charCodeAt(0);

for (let first = lowerA; first <= lowerZ; ++first) {
for (let second = lowerA; second <= lowerZ; ++second) {
yield String.fromCharCode(first, second);
}
}
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ukey can start with a digit.

Suggested change
// Generate all possible `ukey` values.
function* ukeys() {
const lowerA = 'a'.charCodeAt(0);
const lowerZ = 'z'.charCodeAt(0);
for (let first = lowerA; first <= lowerZ; ++first) {
for (let second = lowerA; second <= lowerZ; ++second) {
yield String.fromCharCode(first, second);
}
}
}
// Generate all possible `ukey` values.
// https://unicode.org/reports/tr35/#ukey
function* ukeys() {
const lowerAlpha = 'abcdefghijklmnopqrstuvwxyz';
const lowerAlphanum = lowerAlpha + '0123456789';
for (let i = 0; i < lowerAlphanum.length; ++i) {
for (let j = 0; j < lowerAlpha.length; ++j) {
yield lowerAlphanum[i] + lowerAlpha[j];
}
}
}

Comment on lines +31 to +35
assert.sameValue(
new Intl.Locale(`en-u-${ukey}-true`).toString(),
`en-u-${ukey}`,
`new Intl.Locale("en-u-${ukey}-true").toString() returns "en-u-${ukey}"`
);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggestion:

Suggested change
assert.sameValue(
new Intl.Locale(`en-u-${ukey}-true`).toString(),
`en-u-${ukey}`,
`new Intl.Locale("en-u-${ukey}-true").toString() returns "en-u-${ukey}"`
);
const localeId = `en-u-${ukey}-true`;
const normalizedLocaleId = `en-u-${ukey}`;
assert.sameValue(
new Intl.Locale(localeId).toString(),
normalizedLocaleId,
`new Intl.Locale("${localeId}").toString() must be "${normalizedLocaleId}"`
);

Comment on lines +30 to +62
function canonicalizeUValue(ukey, uvalue) {
assert.sameValue(uvalue, "yes", "unexpected uvalue");

switch (ukey) {
case "kb":
case "kc":
case "kh":
case "kk":
case "kn":
// Canonicalized to "true", which then gets removed.
return "";
default:
return uvalue;
}
}

for (let ukey of ukeys()) {
let canonicalized = new Intl.Locale(`en-u-${ukey}-yes`).toString();

if (canonicalizeUValue(ukey, "yes") === "yes") {
assert.sameValue(
canonicalized,
`en-u-${ukey}-yes`,
`new Intl.Locale("en-u-${ukey}-yes").toString() returns "en-u-${ukey}-yes"`
);
} else {
assert.sameValue(
canonicalized,
`en-u-${ukey}`,
`new Intl.Locale("en-u-${ukey}-yes").toString() returns "en-u-${ukey}"`
);
}
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggestion:

Suggested change
function canonicalizeUValue(ukey, uvalue) {
assert.sameValue(uvalue, "yes", "unexpected uvalue");
switch (ukey) {
case "kb":
case "kc":
case "kh":
case "kk":
case "kn":
// Canonicalized to "true", which then gets removed.
return "";
default:
return uvalue;
}
}
for (let ukey of ukeys()) {
let canonicalized = new Intl.Locale(`en-u-${ukey}-yes`).toString();
if (canonicalizeUValue(ukey, "yes") === "yes") {
assert.sameValue(
canonicalized,
`en-u-${ukey}-yes`,
`new Intl.Locale("en-u-${ukey}-yes").toString() returns "en-u-${ukey}-yes"`
);
} else {
assert.sameValue(
canonicalized,
`en-u-${ukey}`,
`new Intl.Locale("en-u-${ukey}-yes").toString() returns "en-u-${ukey}"`
);
}
}
// Whether ukey has a "yes" alias for "true" in
// https://github.com/unicode-org/cldr/blob/maint/maint-48/common/bcp47/collation.xml
function hasAlias(ukey) {
return ukey === "kb" || ukey === "kc" || ukey === "kh" || ukey === "kk" || ukey === "kn";
}
for (let ukey of ukeys()) {
const localeId = `en-u-${ukey}-yes`;
const normalizedLocaleId = hasAlias(ukey) ? `en-u-${ukey}` : localeId;
assert.sameValue(
new Intl.Locale(localeId).toString(),
normalizedLocaleId,
`new Intl.Locale("${localeId}").toString() must be "${normalizedLocaleId}"`
);
}

Comment on lines +18 to +28
// Generate all possible `ukey` values.
function* ukeys() {
const lowerA = 'a'.charCodeAt(0);
const lowerZ = 'z'.charCodeAt(0);

for (let first = lowerA; first <= lowerZ; ++first) {
for (let second = lowerA; second <= lowerZ; ++second) {
yield String.fromCharCode(first, second);
}
}
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
// Generate all possible `ukey` values.
function* ukeys() {
const lowerA = 'a'.charCodeAt(0);
const lowerZ = 'z'.charCodeAt(0);
for (let first = lowerA; first <= lowerZ; ++first) {
for (let second = lowerA; second <= lowerZ; ++second) {
yield String.fromCharCode(first, second);
}
}
}
// Generate all possible `ukey` values.
// https://unicode.org/reports/tr35/#ukey
function* ukeys() {
const lowerAlpha = 'abcdefghijklmnopqrstuvwxyz';
const lowerAlphanum = lowerAlpha + '0123456789';
for (let i = 0; i < lowerAlphanum.length; ++i) {
for (let j = 0; j < lowerAlpha.length; ++j) {
yield lowerAlphanum[i] + lowerAlpha[j];
}
}
}

Comment on lines +6 to +7
description: >
Remove any uvalue (aka type) equal to "true".

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
description: >
Remove any uvalue (aka type) equal to "true".
description: >
Remove any uvalue (aka type) equal to "yes" if and only if "yes" is an alias for "true".

@jugglinmike

Copy link
Copy Markdown
Contributor

@gibson042 A few details make this a good candidate for landing as-is: your suggestions are additive rather than corrective, this has been waiting for some time, @anba's public contribution history suggests they may not be available right now, and I have immediate availability to make the requested changes. Would you be open to landing this patch as-is with the understanding that I will submit a follow-up later today?

@gibson042

Copy link
Copy Markdown
Member

@jugglinmike Yes, I left a comment rather than a request for changes specifically to avoid blocking.

@jugglinmike

Copy link
Copy Markdown
Contributor

Right on!

@Ms2ger
Ms2ger merged commit ea076b1 into tc39:main Sep 17, 2026
17 checks passed
@jugglinmike

Copy link
Copy Markdown
Contributor

Follow-up patch: #5138

Ms2ger pushed a commit that referenced this pull request Sep 21, 2026
This patch was originally authored by Richard Gibson:

#5016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants