Fix -u-sd (#7341)

Validate that the subdivision matches the region, and derive the region
from the subdivision in region-priority mode.

The latter step might need to be done by the fallbacker actually. For a
locale like `en-u-sd-gbsct`, should the chain be
* `en-u-sd-gbsct` -> `en-GB` -> `en`, or
* `en-GB-u-sd-gbsct` -> `en-GB` -> `en`?

#4413

Author: robertbastian

components/locale/src/fallback/algorithms.rs

@@ -9,12 +9,13 @@ use super::*;
 
 impl LocaleFallbackerWithConfig<'_> {
     pub(crate) fn normalize(&self, locale: &mut DataLocale, default_script: &mut Option<Script>) {
-        // 0. If there is an invalid "sd" subtag, drop it
+        // 0. If there is an invalid or trivial "sd" subtag, drop it
         if let Some(subdivision) = locale.subdivision.take() {
             if let Some(region) = locale.region {
                 if subdivision
                     .as_str()
                     .starts_with(region.to_tinystr().to_ascii_lowercase().as_str())
+                    && !subdivision.as_str().ends_with("zzzz")
                 {
                     locale.subdivision = Some(subdivision);
                 }
@@ -240,6 +241,8 @@ impl LocaleFallbackIteratorInner<'_> {
 #[cfg(test)]
 mod tests {
     use super::*;
+    use icu_locale_core::preferences::LocalePreferences;
+    use icu_locale_core::Locale;
     use writeable::Writeable;
 
     struct TestCase {
@@ -472,13 +475,23 @@ mod tests {
         let fallbacker_no_data = fallbacker_no_data.as_borrowed();
         let fallbacker_with_data = LocaleFallbacker::new();
         for cas in TEST_CASES {
-            for (priority, expected_chain) in [
+            let prefs = LocalePreferences::from(&cas.input.parse::<Locale>().unwrap());
+            for (priority, data_locale, expected_chain) in [
                 (
                     LocaleFallbackPriority::Language,
+                    prefs.to_data_locale_language_priority(),
                     cas.expected_language_chain,
                 ),
-                (LocaleFallbackPriority::Script, cas.expected_script_chain),
-                (LocaleFallbackPriority::Region, cas.expected_region_chain),
+                (
+                    LocaleFallbackPriority::Script,
+                    prefs.to_data_locale_language_priority(),
+                    cas.expected_script_chain,
+                ),
+                (
+                    LocaleFallbackPriority::Region,
+                    prefs.to_data_locale_region_priority(),
+                    cas.expected_region_chain,
+                ),
             ] {
                 let mut config = LocaleFallbackConfig::default();
                 config.priority = priority;
@@ -487,9 +500,7 @@ mod tests {
                 } else {
                     fallbacker_no_data
                 };
-                let mut it = fallbacker
-                    .for_config(config)
-                    .fallback_for(cas.input.parse().unwrap());
+                let mut it = fallbacker.for_config(config).fallback_for(data_locale);
                 let mut actual_chain = Vec::new();
                 for i in 0..20 {
                     if i == 19 {

components/locale_core/src/data.rs

@@ -49,7 +49,7 @@ use core::str::FromStr;
 ///     DataLocale::from(locale!("hi-IN-u-sd-inas"))
 /// );
 /// ```
-#[derive(Clone, Copy, PartialEq, Hash, Eq)]
+#[derive(Clone, Copy)]
 #[non_exhaustive]
 pub struct DataLocale {
     /// Language subtag
@@ -61,9 +61,24 @@ pub struct DataLocale {
     /// Variant subtag
     pub variant: Option<Variant>,
     /// Subivision (-u-sd-) subtag
+    // TODO(3.0): Use `SubdivisionSuffix` type
     pub subdivision: Option<Subtag>,
 }
 
+impl PartialEq for DataLocale {
+    fn eq(&self, other: &Self) -> bool {
+        self.as_tuple() == other.as_tuple()
+    }
+}
+
+impl Eq for DataLocale {}
+
+impl Hash for DataLocale {
+    fn hash<H: core::hash::Hasher>(&self, state: &mut H) {
+        self.as_tuple().hash(state);
+    }
+}
+
 impl Default for DataLocale {
     fn default() -> Self {
         Self {
@@ -205,24 +220,46 @@ impl DataLocale {
         Ok(())
     }
 
+    fn region_and_subdivision(&self) -> Option<unicode_ext::SubdivisionId> {
+        self.subdivision
+            .and_then(|s| unicode_ext::SubdivisionId::try_from_str(s.as_str()).ok())
+            .or(self.region.map(|region| unicode_ext::SubdivisionId {
+                region,
+                suffix: unicode_ext::SubdivisionSuffix::UNKNOWN,
+            }))
+    }
+
     fn as_tuple(
         &self,
     ) -> (
         Language,
         Option<Script>,
-        Option<Region>,
+        Option<unicode_ext::SubdivisionId>,
         Option<Variant>,
-        Option<Subtag>,
     ) {
         (
             self.language,
             self.script,
-            self.region,
+            self.region_and_subdivision(),
             self.variant,
-            self.subdivision,
         )
     }
 
+    pub(crate) fn from_parts(
+        language: Language,
+        script: Option<Script>,
+        region: Option<unicode_ext::SubdivisionId>,
+        variant: Option<Variant>,
+    ) -> Self {
+        Self {
+            language,
+            script,
+            region: region.map(|sd| sd.region),
+            variant,
+            subdivision: region.map(|sd| sd.into_subtag()),
+        }
+    }
+
     /// Returns an ordering suitable for use in [`BTreeSet`].
     ///
     /// [`BTreeSet`]: alloc::collections::BTreeSet
@@ -360,8 +397,8 @@ impl DataLocale {
                 keywords: unicode_ext::Keywords::new_single(
                     RegionalSubdivision::UNICODE_EXTENSION_KEY,
                     RegionalSubdivision(
-                        unicode_ext::SubdivisionId::try_from_str(self.subdivision?.as_str())
-                            .ok()?,
+                        self.region_and_subdivision()
+                            .filter(|sd| !sd.suffix.is_unknown())?,
                     )
                     .into(),
                 ),
@@ -386,12 +423,16 @@ fn test_data_locale_to_string() {
         },
         TestCase {
             locale: "und-u-sd-sdd",
-            expected: "und-u-sd-sdd",
+            expected: "und-SD-u-sd-sdd",
         },
         TestCase {
             locale: "en-ZA-u-sd-zaa",
             expected: "en-ZA-u-sd-zaa",
         },
+        TestCase {
+            locale: "en-ZA-u-sd-sdd",
+            expected: "en-ZA",
+        },
     ] {
         let locale = cas.locale.parse::<DataLocale>().unwrap();
         writeable::assert_writeable_eq!(locale, cas.expected);

components/locale_core/src/extensions/unicode/subdivision.rs

@@ -46,6 +46,14 @@ impl_tinystr_subtag!(
     ["toolooong"],
 );
 
+impl SubdivisionSuffix {
+    pub(crate) const UNKNOWN: Self = subdivision_suffix!("zzzz");
+
+    pub(crate) fn is_unknown(self) -> bool {
+        self == Self::UNKNOWN
+    }
+}
+
 /// A Subivision Id as defined in [`Unicode Locale Identifier`].
 ///
 /// Subdivision Id is used in [`Unicode`] extensions:

components/locale_core/src/preferences/locale.rs

@@ -26,31 +26,46 @@ pub struct LocalePreferences {
 }
 
 impl LocalePreferences {
-    fn to_data_locale_maybe_region_priority(self, region_priority: bool) -> DataLocale {
-        DataLocale {
-            language: self.language,
-            script: self.script,
-            region: match (self.region, self.region_override) {
-                (_, Some(r)) if region_priority => Some(r.region),
-                (r, _) => r,
-            },
-            variant: self.variant,
-            subdivision: self.subdivision.map(|r| r.into_subtag()),
-        }
-    }
-
     /// Convert to a DataLocale, with region-based fallback priority
     ///
     /// Most users should use `icu_provider::marker::make_locale()` instead.
     pub fn to_data_locale_region_priority(self) -> DataLocale {
-        self.to_data_locale_maybe_region_priority(true)
+        if let Some(ro) = self.region_override {
+            return DataLocale::from_parts(self.language, self.script, Some(*ro), self.variant);
+        }
+
+        self.to_data_locale_language_priority()
     }
 
     /// Convert to a DataLocale, with language-based fallback priority
     ///
     /// Most users should use `icu_provider::marker::make_locale()` instead.
     pub fn to_data_locale_language_priority(self) -> DataLocale {
-        self.to_data_locale_maybe_region_priority(false)
+        use crate::extensions::unicode::{SubdivisionId, SubdivisionSuffix};
+
+        let region = if let Some(sd) = self.subdivision {
+            if let Some(region) = self.region {
+                // Discard the subdivison if it doesn't match the region
+                Some(SubdivisionId {
+                    region,
+                    suffix: if sd.region == region {
+                        sd.suffix
+                    } else {
+                        SubdivisionSuffix::UNKNOWN
+                    },
+                })
+            } else {
+                // Use the subdivision's region if there's no region
+                Some(*sd)
+            }
+        } else {
+            self.region.map(|region| SubdivisionId {
+                region,
+                suffix: SubdivisionSuffix::UNKNOWN,
+            })
+        };
+
+        DataLocale::from_parts(self.language, self.script, region, self.variant)
     }
 }
 impl Default for LocalePreferences {
@@ -201,8 +216,8 @@ mod tests {
             },
             TestCase {
                 input: "en-u-sd-ustx",
-                language_priority: "en-u-sd-ustx",
-                region_priority: "en-u-sd-ustx",
+                language_priority: "en-US-u-sd-ustx",
+                region_priority: "en-US-u-sd-ustx",
             },
             TestCase {
                 input: "en-US-u-sd-ustx",
@@ -219,15 +234,25 @@ mod tests {
                 language_priority: "en-US",
                 region_priority: "en-GB",
             },
+            TestCase {
+                input: "en-US-u-sd-gbzzzz",
+                language_priority: "en-US",
+                region_priority: "en-US",
+            },
             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?
+                language_priority: "en-US-u-sd-ustx",
+                region_priority: "en-GB",
             },
             TestCase {
                 input: "en-US-u-rg-gbzzzz-sd-ustx",
                 language_priority: "en-US-u-sd-ustx",
-                region_priority: "en-GB-u-sd-ustx", // does this make sense?
+                region_priority: "en-GB",
+            },
+            TestCase {
+                input: "en-US-u-rg-gbeng-sd-ustx",
+                language_priority: "en-US-u-sd-ustx",
+                region_priority: "en-GB-u-sd-gbeng",
             },
         ];
         for test_case in test_cases.iter() {