Prefer the closest locale in language negotiation

CHANGELOG.md

@@ -78,6 +78,18 @@
   when the pattern references a term. Behavior is unchanged.
 
 ### Fixed
+- **Language negotiation ignored regions.** With both `en` and `en-gb`,
+  `L10n::langneg("en-GB")` returned `en`, because the first locale with the same
+  language won. Now the closest locale wins: the exact one, then the same region
+  with another script, then the generic locale (`en` for `en-AU`), then another
+  region, and a locale with a conflicting script (`sr-Cyrl` for `sr-Latn`) only
+  as a last resort. `L10nLanguageVec::langneg` matches the same way. Scripts
+  aren't inferred from regions, so `zh-TW` doesn't prefer `zh-Hant` over
+  `zh-Hans`.
+- `Accept-Language` entries with `q=0` ("not acceptable") or an invalid `q` are
+  now skipped. Before, `q=0` entries could still be picked and invalid weights
+  counted as `q=1`.
+- `negotiate_languages` no longer requires `PartialEq` on the language type.
 - **Translations that passed validation could still make an accessor panic.**
   Neither the build nor `new_external` checked references to other messages
   (`{ inbox-link }`, `{ other.title }`) or function calls (`{ DATETIME($d) }`;

docs/languages.md

@@ -54,10 +54,29 @@ assert_eq!(lang, L10n::Fr);
 let strs = lang.load();
 ```
 
-Entries are tried in order of their `q` weight, and each is compared by its language
-subtag only: `fr-CA` matches a `fr` locale. Regions are ignored, so if you have both
-`en` and `en-gb`, a request for `en-GB` gets whichever comes first in `L10n::iter()`
-(here `en`), not the exact match.
+The header's entries are tried in order of their `q` weight (entries with `q=0` mean
+"not acceptable" and are skipped), and the first one that matches any locale wins. For
+an entry such as `en-GB`, the closest locale is, in order:
+
+1. the exact locale, `en-gb`;
+2. the same region with another script, such as `en-latn-gb`;
+3. the generic locale of the language, `en`;
+4. another region, such as `en-us`;
+5. last, a locale with a different script than the one asked for (`sr-cyrl` for a
+   request for `sr-Latn`).
+
+Scripts aren't inferred from regions: a request for `zh-TW` doesn't know it means
+Traditional Chinese. If you have both `zh-hans` and `zh-hant`, also add regional
+folders that name the script, such as `zh-hant-tw`.
+
+With `en`, `en-gb` and `fr`:
+
+```rust
+assert_eq!(L10n::langneg("en-GB"), L10n::EnGb); // exact
+assert_eq!(L10n::langneg("en-US"), L10n::En);   // no en-us: the generic en
+assert_eq!(L10n::langneg("fr-CA"), L10n::Fr);   // no fr-ca: fr
+assert_eq!(L10n::langneg("de"), L10n::En);      // no match: the default language
+```
 
 ## Serving every language from one server
 

playground/docs-check/locales/en-gb/main.ftl

@@ -0,0 +1,27 @@
+language-name = English (UK)
+greeting = Welcome!
+
+# $name (String) - the user's name
+hello = Hello { $name }
+
+unread-messages = { $count ->
+    [one] You have one message
+   *[other] You have { $count } messages
+}
+
+login = Log in
+    .placeholder = Enter your email
+
+# $enabled (Bool) - Whether the feature is enabled.
+feature-status = { $enabled ->
+    [true] Enabled
+   *[false] Disabled
+}
+
+greeting-for = Hi { $who }
+
+# $count (Number) - How many unread.
+# $icon (Element) - An icon injected by the app.
+# -privacy-link (Element) - Link text the app wraps in a link.
+notice = { $icon } You have { $count } unread, see { -privacy-link }.
+-privacy-link = our privacy policy

src/l10n_language_vec.rs

@@ -77,7 +77,7 @@ impl L10nLanguageVec {
     }
 
     /// Negotiate the best of the *loaded* languages for an `Accept-Language`
-    /// header — including any added at runtime with [`Self::insert`], which
+    /// header, matching like [`negotiate_languages`](crate::prelude::negotiate_languages) — including any added at runtime with [`Self::insert`], which
     /// the generated `L10n::langneg` (compiled from the build-time language
     /// set) can never return.
     ///
@@ -88,17 +88,10 @@ impl L10nLanguageVec {
     pub fn langneg(&self, accept_language: &str) -> Option<&L10nBundle> {
         use icu_locale_core::LanguageIdentifier;
 
-        let available: Vec<Option<LanguageIdentifier>> =
+        let parsed: Vec<Option<LanguageIdentifier>> =
             self.langs.iter().map(|b| b.lang().parse().ok()).collect();
-        for req in crate::prelude::requested_languages(accept_language) {
-            let found = available
-                .iter()
-                .position(|a| a.as_ref().is_some_and(|a| a.language == req.language));
-            if let Some(idx) = found {
-                return Some(&self.langs[idx]);
-            }
-        }
-        None
+        crate::prelude::best_match(accept_language, parsed.len(), |i| parsed[i].as_ref())
+            .map(|idx| &self.langs[idx])
     }
 
     /// Get the bundle for `lang`, or `None` if no such language was loaded.

src/lib.rs

@@ -100,25 +100,26 @@ pub mod prelude {
     pub use icu_locale_core::{LanguageIdentifier, langid};
 
     /// Parse an `Accept-Language` header into language identifiers, sorted by
-    /// quality (highest first). Unparseable entries are skipped.
+    /// quality (highest first; equal qualities keep their order). Entries that
+    /// don't parse, have an invalid `q`, or have `q=0` ("not acceptable") are
+    /// skipped.
     #[cfg(feature = "langneg")]
     pub(crate) fn requested_languages(accept_language: &str) -> Vec<LanguageIdentifier> {
         let mut requested: Vec<(LanguageIdentifier, u16)> = accept_language
             .split(',')
             .filter_map(|entry| {
-                let entry = entry.trim();
-                if entry.is_empty() {
-                    return None;
-                }
-                let (tag, quality) = if let Some((tag, params)) = entry.split_once(';') {
-                    let q = params
-                        .trim()
-                        .strip_prefix("q=")
-                        .and_then(|v| v.parse::<f32>().ok())
-                        .unwrap_or(1.0);
-                    (tag.trim(), (q * 1000.0) as u16)
-                } else {
-                    (entry, 1000)
+                let mut parts = entry.split(';');
+                let tag = parts.next()?.trim();
+                let quality = match parts.find_map(|p| p.trim().strip_prefix("q=")) {
+                    None => 1000,
+                    Some(q) => {
+                        let q = q.trim().parse::<f32>().ok()?;
+                        // q=0 means "not acceptable", and NaN isn't a weight.
+                        if q.is_nan() || q <= 0.0 {
+                            return None;
+                        }
+                        (q.min(1.0) * 1000.0) as u16
+                    }
                 };
                 tag.parse::<LanguageIdentifier>()
                     .ok()
@@ -129,24 +130,79 @@ pub mod prelude {
         requested.into_iter().map(|(lid, _)| lid).collect()
     }
 
-    /// Pick the first of `available` whose language subtag matches an entry of
-    /// the `Accept-Language` header (highest quality first), or
-    /// `A::default()` when none matches. Regions and scripts are ignored:
-    /// `en-GB` matches the first `en*` entry of `available`. The generated `L10n::langneg` calls
+    /// Pick the best of `available` for an `Accept-Language` header, or
+    /// `A::default()` when none matches. The generated `L10n::langneg` calls
     /// this with every built-in language.
+    ///
+    /// The header's entries are tried highest quality first, and the first
+    /// entry with any match wins. For one entry, such as `en-GB`, the best
+    /// match is, in order:
+    ///
+    /// 1. `en-GB` itself;
+    /// 2. the same region, with no conflicting script (`en-Latn-GB`);
+    /// 3. the generic locale of the language (`en`);
+    /// 4. another region with no conflicting script (`en-US`);
+    /// 5. a locale with a different script than the one requested (for
+    ///    `sr-Latn`: `sr-Cyrl`).
+    ///
+    /// Ties go to the locale without a script the request didn't ask for, then
+    /// to the order of `available`. Scripts are not inferred from regions:
+    /// `zh-TW` doesn't know it means Traditional Chinese, so with `zh-Hans`
+    /// and `zh-Hant` available, name the script in the locale folders that
+    /// have regions (`zh-Hant-TW`) or add the regional locale.
     #[cfg(feature = "langneg")]
-    pub fn negotiate_languages<'a, A>(accept_language: &str, available: &'a [A]) -> A
+    pub fn negotiate_languages<A>(accept_language: &str, available: &[A]) -> A
     where
-        A: 'a + AsRef<LanguageIdentifier> + PartialEq + Default + Copy,
+        A: AsRef<LanguageIdentifier> + Default + Copy,
     {
-        // Find the first available language whose language subtag matches a requested one
-        for req in requested_languages(accept_language) {
-            for avail in available {
-                if avail.as_ref().language == req.language {
-                    return *avail;
-                }
-            }
+        match best_match(accept_language, available.len(), |i| {
+            Some(available[i].as_ref())
+        }) {
+            Some(idx) => available[idx],
+            None => A::default(),
         }
-        A::default()
+    }
+
+    /// The index of the best match among `len` candidates for an
+    /// `Accept-Language` header, as described on [`negotiate_languages`].
+    /// `candidate(i)` returns the `i`th candidate's id; `None` never matches.
+    /// Shared by the generated `L10n::langneg` and `L10nLanguageVec::langneg`.
+    #[cfg(feature = "langneg")]
+    pub(crate) fn best_match<'a>(
+        accept_language: &str,
+        len: usize,
+        candidate: impl Fn(usize) -> Option<&'a LanguageIdentifier>,
+    ) -> Option<usize> {
+        requested_languages(accept_language).iter().find_map(|req| {
+            (0..len)
+                .filter_map(|i| Some((rank(req, candidate(i)?)?, i)))
+                .min()
+                .map(|(_, i)| i)
+        })
+    }
+
+    /// How well `a` matches the request `req`: lower is better, `None` is no
+    /// match. The first element is the tier listed on [`negotiate_languages`];
+    /// the second prefers a locale without a script `req` didn't ask for.
+    #[cfg(feature = "langneg")]
+    fn rank(req: &LanguageIdentifier, a: &LanguageIdentifier) -> Option<(u8, bool)> {
+        if a.language != req.language {
+            return None;
+        }
+        if a == req {
+            return Some((0, false));
+        }
+        let script_conflict = matches!((req.script, a.script), (Some(r), Some(s)) if r != s);
+        let extra_script = req.script.is_none() && a.script.is_some();
+        let tier = if script_conflict {
+            5
+        } else if req.region.is_some() && a.region == req.region {
+            2
+        } else if a.region.is_none() {
+            3
+        } else {
+            4
+        };
+        Some((tier, extra_script))
     }
 }

src/tests/runtime.rs

@@ -264,3 +264,83 @@ fn try_get_returns_none_for_unloaded_language() {
         "Hello"
     );
 }
+
+/// Negotiate `header` over `available` language ids; `None` if nothing matches.
+#[cfg(feature = "langneg")]
+fn negotiate<'a>(header: &str, available: &[&'a str]) -> Option<&'a str> {
+    let parsed: Vec<Option<crate::prelude::LanguageIdentifier>> =
+        available.iter().map(|a| a.parse().ok()).collect();
+    crate::prelude::best_match(header, parsed.len(), |i| parsed[i].as_ref())
+        .map(|idx| available[idx])
+}
+
+#[cfg(feature = "langneg")]
+#[test]
+fn langneg_prefers_the_exact_locale() {
+    // The reported case: `en` came first, so `en-GB` got `en`.
+    assert_eq!(negotiate("en-GB", &["en", "en-gb", "fr"]), Some("en-gb"));
+    assert_eq!(negotiate("en", &["en", "en-gb", "fr"]), Some("en"));
+    // Case and order in the header don't matter for an exact match.
+    assert_eq!(
+        negotiate("fr;q=0.2, EN-gb", &["en", "en-gb", "fr"]),
+        Some("en-gb")
+    );
+}
+
+#[cfg(feature = "langneg")]
+#[test]
+fn langneg_falls_back_to_the_generic_locale_then_any_region() {
+    // No `en-AU`: the generic `en` beats another region.
+    assert_eq!(negotiate("en-AU", &["en-gb", "en", "fr"]), Some("en"));
+    // No generic `en` either: the first `en-*`.
+    assert_eq!(negotiate("en-AU", &["en-gb", "en-us", "fr"]), Some("en-gb"));
+    // A plain `en` request takes a regional locale when that's all there is.
+    assert_eq!(negotiate("en", &["fr", "en-us"]), Some("en-us"));
+    // The same region with another script beats the generic locale.
+    assert_eq!(negotiate("sr-Latn-RS", &["sr", "sr-RS"]), Some("sr-RS"));
+}
+
+#[cfg(feature = "langneg")]
+#[test]
+fn langneg_follows_header_quality_before_match_quality() {
+    // `de` is preferred by the header, so a mere language match on `de`
+    // beats an exact match on the lower-weighted `en-GB`.
+    assert_eq!(
+        negotiate("de-AT, en-GB;q=0.5", &["en-gb", "de"]),
+        Some("de")
+    );
+    assert_eq!(negotiate("pt, it", &["en", "fr"]), None);
+}
+
+#[cfg(feature = "langneg")]
+#[test]
+fn langneg_respects_scripts() {
+    // A conflicting script only wins when nothing else of the language does.
+    assert_eq!(
+        negotiate("sr-Latn", &["sr-Cyrl", "sr-Latn-RS"]),
+        Some("sr-Latn-RS")
+    );
+    assert_eq!(
+        negotiate("zh-Hant-HK", &["zh-Hans", "zh-Hant-TW"]),
+        Some("zh-Hant-TW")
+    );
+    assert_eq!(negotiate("sr-Latn", &["en", "sr-Cyrl"]), Some("sr-Cyrl"));
+    // Without a requested script, the plain locale beats a script variant.
+    assert_eq!(negotiate("en", &["en-Latn", "en"]), Some("en"));
+    // Known limit: no script is inferred from a region, so `zh-TW` can't tell
+    // `zh-Hant` from `zh-Hans` and takes the first.
+    assert_eq!(negotiate("zh-TW", &["zh-Hans", "zh-Hant"]), Some("zh-Hans"));
+}
+
+#[cfg(feature = "langneg")]
+#[test]
+fn langneg_skips_unacceptable_and_malformed_weights() {
+    assert_eq!(negotiate("de-CH, en;q=0", &["en", "fr"]), None);
+    assert_eq!(negotiate("en;q=abc, fr;q=0.5", &["en", "fr"]), Some("fr"));
+    assert_eq!(negotiate("fr;q=0.5, en;q=2", &["en", "fr"]), Some("en"));
+    // Other parameters next to q are fine.
+    assert_eq!(
+        negotiate("en;level=1;q=0.1, fr;q=0.5", &["en", "fr"]),
+        Some("fr")
+    );
+}