Fix implementation of locale region override by sffc · Pull Request #7335 · unicode-org/icu4x

sffc · 2025-12-21T07:42:34Z

We had partial claims for supporting this, but there were zero tests, and it turns out our region override code was not in working condition. I fixed the bugs and added test coverage.

Fixes #4413

sffc · 2025-12-21T08:04:17Z

            script: self.script,
            region: match (self.region, self.ue_region) {
-                (Some(_), Some(r)) if region_priority => Some(r),
+                (_, Some(r)) if region_priority => Some(r),


@zbraniecki: was there a reason you were matching Some(_) on the regular region subtag here?

No, I think it was a bug! Thanks for catching it!

sffc · 2025-12-21T08:05:08Z

+    /// ```
+    ///
+    /// [UTS #35]: https://www.unicode.org/reports/tr35/#RegionOverride
+    pub fn try_from_extension_value(value: &Value) -> Result<Self, RegionExtensionError> {


@zbraniecki wdyt of this new API. the signature and where it lives?

I'd prefer if this lived on Value, or Subtag, or was internal to the preference parser. Something like parse_unicode_subdivision_id(self) -> Option<(Region, TinyAsciiStr<4>)>.

I also think it should be private for now.

zbraniecki

lgtm!

zbraniecki · 2025-12-22T05:55:27Z

+                match Region::try_from_extension_value(v) {
+                    Ok(r) => Some(r),
+                    Err(_e) => {
+                        // How should we handle the error? Ignore?


yes. It's inherently lossy

zbraniecki · 2025-12-22T05:57:44Z

+    /// ```
+    ///
+    /// [UTS #35]: https://www.unicode.org/reports/tr35/#RegionOverride
+    pub fn try_from_extension_value(value: &Value) -> Result<Self, RegionExtensionError> {


robertbastian · 2025-12-22T09:01:55Z

+            TestCase {
+                input: "en-u-rg-gbzzzz-sd-ustx",
+                language_priority: "en-u-sd-ustx",
+                region_priority: "en-GB-u-sd-ustx", // does this make sense?


about as much sense as the input

please don't submit these comments

actually this doesn't make sense, as the rg override overrides the subdivision as well

I think we might want to remove the subdivision entirely for language priority, as that should only be used for region-specific data?

Handled in the context of #7337 and its follow-ups

robertbastian · 2025-12-22T09:03:22Z

 use size_test_macro::size_test;

+#[cfg(test)]
+use icu_locale_core::locale;


nit: move this into test_numbering_resolution_fallback

robertbastian · 2025-12-22T09:05:44Z

+    ///
+    /// ⚠️ This constructor ignores fallback priority and Unicode extension keywords,
+    /// and it reads only the first variant.
+    pub fn from_content_language_identifier(langid: &LanguageIdentifier) -> Self {


I would prefer not to make the content/user locale disctinction on the DataLocale level (which I still consider to be an icu_provider type). Each component decides for itself how to map locales into DataLocales, and what the semantics of its input locales are.

If you really want to name these methods (maybe something else), then we should deprecate the From implementations.

I wasn't able to figure out how to deprecate the From impls.

Moved to #7346

robertbastian · 2025-12-22T09:07:40Z

+    /// ⚠️ This constructor ignores fallback priority and Unicode extension keywords,
+    /// and it reads only the first variant.
+    pub fn from_content_language_identifier(langid: &LanguageIdentifier) -> Self {
+        Self {


this duplicates code in From<&LanguageIdentifier>

robertbastian · 2025-12-22T09:16:30Z

observation: unrelated cleanup

robertbastian · 2025-12-22T09:25:33Z

+    /// ```
+    ///
+    /// [UTS #35]: https://www.unicode.org/reports/tr35/#RegionOverride
+    pub fn try_from_extension_value(value: &Value) -> Result<Self, RegionExtensionError> {


I'd prefer if this lived on Value, or Subtag, or was internal to the preference parser. Something like parse_unicode_subdivision_id(self) -> Option<(Region, TinyAsciiStr<4>)>.

I also think it should be private for now.

robertbastian · 2025-12-22T09:28:52Z

+    ///
+    /// [UTS #35]: https://www.unicode.org/reports/tr35/#RegionOverride
+    #[displaydoc("Invalid region or subdivision extension value")]
+    Invalid,


Can we just use ParseError::InvalidExtension here? We don't have per-type or per method parsing errors

It's a different level than a syntax error. I was going to use ParseError but thought you would give the opposite feedback that we should instead use a narrow error type. If you confirm that you prefer ParseError then I'll switch over.

This is obsolete since we found the existing function that returns ParseError

robertbastian · 2025-12-22T09:32:09Z

        let payload_locale_override = if let Some(locale) = options.content_locale {
-            let locale = DataLocale::from(locale);
+            let locale = DataLocale::from_content_language_identifier(locale);


I think renaming the identifier to content_language has a similar readability effect as renaming from

Suggested change

let payload_locale_override = if let Some(locale) = options.content_locale {

let locale = DataLocale::from(locale);

let locale = DataLocale::from_content_language_identifier(locale);

let payload_locale_override = if let Some(content_language) = options.content_locale {

let locale = DataLocale::from(content_language);

Let's discuss in the context of #7346

robertbastian · 2025-12-22T09:33:55Z

    #[diplomat::opaque]
    /// An ICU4X Locale, capable of representing strings like `"en-US"`.
+    ///
+    /// In `icu_capi`, this type also covers the uses of `DataLocale`.


NPM/Dart/MVN users won't know what icu_capi is

Suggested change

/// In `icu_capi`, this type also covers the uses of `DataLocale`.

/// This type also covers the Rust type `DataLocale`.

robertbastian · 2025-12-22T09:36:20Z

 /// ```
-/// use icu_locale_core::{locale, Locale};
-/// use icu_provider::DataLocale;
+/// use icu_locale_core::locale;


why split these?

why change the DataLocale import? I like importing it from icu_provider

robertbastian · 2025-12-22T09:45:53Z

+/// // For en-US with a Germany preference override
+/// let info = WeekInformation::try_new(locale!("en-US-u-rg-dezzzz").into()).unwrap();
+/// assert_eq!(info.first_weekday, Weekday::Monday);
+/// ```


please add an example of the fw override as well, and how it interacts with rg.

robertbastian

We already have code to parse unicode_subdivision_id.

https://docs.rs/icu/latest/icu/locale/extensions/unicode/struct.SubdivisionId.html

Simpler version of #7335

sffc · 2025-12-24T21:28:38Z

This PR has been fully split into:

#7337
#7346
#7347
#7348

Split from #7335

Feedback from #7335 Demonstrates that #4413 is fixed

Split from #7335

sffc added 9 commits December 20, 2025 22:00

Add Region::try_from_extension_value

20d19ad

Correctly implement region override in LocalePreferences and add test

da618f0

Add DataLocale::from_content_* and migrate components to use it

bce85af

Resolve TODO in fallback code by adding docs

a428b62

Add test in the icu_calendar crate

b7ace35

Add comment about error handling

2eabbfe

fmt

e1adf69

clippy

e03c04b

diplomat links

243a4ba

sffc force-pushed the region-override branch from f6f621a to 243a4ba Compare December 21, 2025 08:00

sffc changed the title ~~Region override~~ Fix implementation of locale region override Dec 21, 2025

sffc marked this pull request as ready for review December 21, 2025 08:01

sffc requested review from a team, Manishearth, aethanyc, dminor, makotokato, nciric and zbraniecki as code owners December 21, 2025 08:01

sffc removed request for a team, aethanyc, dminor, makotokato and nciric December 21, 2025 08:01

sffc commented Dec 21, 2025

View reviewed changes

zbraniecki approved these changes Dec 22, 2025

View reviewed changes

robertbastian reviewed Dec 22, 2025

View reviewed changes

robertbastian requested changes Dec 22, 2025

View reviewed changes

robertbastian added a commit to robertbastian/icu4x that referenced this pull request Dec 22, 2025

tests from unicode-org#7335

494a491

robertbastian added a commit to robertbastian/icu4x that referenced this pull request Dec 22, 2025

tests from unicode-org#7335

7752391

robertbastian mentioned this pull request Dec 22, 2025

Fix locale region override #7337

Merged

robertbastian added a commit to robertbastian/icu4x that referenced this pull request Dec 22, 2025

tests from unicode-org#7335

da1c540

robertbastian added a commit that referenced this pull request Dec 22, 2025

Fix locale region override (#7337)

f16ea51

Simpler version of #7335

This was referenced Dec 24, 2025

Add DataLocale::from_content_* and migrate components to use it #7346

Closed

Add docs tests for SubdivisionId and preferences types #7347

Merged

Resolve TODO in fallback code by adding docs #7348

Merged

sffc closed this Dec 24, 2025

sffc deleted the region-override branch December 24, 2025 21:28

sffc mentioned this pull request Dec 24, 2025

Improve the WeekInformation test #7350

Merged

sffc added a commit that referenced this pull request Dec 29, 2025

Resolve TODO in fallback code by adding docs (#7348)

685d6ac

Split from #7335

sffc added a commit that referenced this pull request Dec 31, 2025

Improve the WeekInformation test (#7350)

04a93b1

Feedback from #7335 Demonstrates that #4413 is fixed

sffc added a commit that referenced this pull request Jan 7, 2026

Add docs tests for SubdivisionId and preferences types (#7347)

28f125c

Split from #7335

	/// In `icu_capi`, this type also covers the uses of `DataLocale`.
	/// This type also covers the Rust type `DataLocale`.

Conversation

sffc commented Dec 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zbraniecki left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robertbastian Dec 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robertbastian left a comment

Choose a reason for hiding this comment

Uh oh!

sffc commented Dec 24, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

sffc commented Dec 21, 2025 •

edited

Loading

robertbastian Dec 22, 2025 •

edited

Loading