unicodedata: Fix Bidirectional, improve parser (#8548)

We can save space by calculating the diffs between modern Unicode and
3.2.0 and storing membership information where valid. This avoids
storing entire tables for 3.2.0, and is also more correct in the long
run since it handles absence from 3.2.0 correctly. I switched over Bidi
to this new method which partially fixed the test. Unfortunately, our
Unicode data is more up to date than Python 3.14 so the test fails for
modern Unicode. I updated the test to 3.15 as per review, and now our
bidi passes the test.

Lib/test/test_unicodedata.py

@@ -306,9 +306,9 @@ class UnicodeFunctionsTest(unittest.TestCase):
         self.assertRaises(TypeError, self.db.category)
         self.assertRaises(TypeError, self.db.category, 'xx')
 
-    @unittest.expectedFailure  # TODO: RUSTPYTHON; - 'BN' != ''
+    # NOTE: RUSTPYTHON; This test is from 3.15. See RustPython#8548 for motivation.
     def test_bidirectional(self):
-        self.assertEqual(self.db.bidirectional('\uFFFE'), '')
+        self.assertEqual(self.db.bidirectional('\uFFFE'), '' if self.old else 'BN')
         self.assertEqual(self.db.bidirectional(' '), 'WS')
         self.assertEqual(self.db.bidirectional('A'), 'L')
         self.assertEqual(self.db.bidirectional('\U00020000'), 'L')
@@ -329,6 +329,9 @@ class UnicodeFunctionsTest(unittest.TestCase):
         # New in 16.0.0
         self.assertEqual(self.db.bidirectional('\u0897'), '' if self.old else 'NSM')
         self.assertEqual(self.db.bidirectional('\U0001fbef'), '' if self.old else 'ON')
+        # New in 17.0.0
+        self.assertEqual(self.db.bidirectional('\u088f'), '' if self.old else 'AL')
+        self.assertEqual(self.db.bidirectional('\U0001fbfa'), '' if self.old else 'ON')
 
         self.assertRaises(TypeError, self.db.bidirectional)
         self.assertRaises(TypeError, self.db.bidirectional, 'xx')

crates/unicode/build.rs

@@ -14,7 +14,9 @@ use std::{
     thread,
 };
 
-use icu_properties::props::{EnumeratedProperty, GeneralCategory, NumericType};
+use icu_properties::props::{
+    BidiClass, EnumeratedProperty, GeneralCategory, NamedEnumeratedProperty, NumericType,
+};
 
 fn generate_unicode_3_2() {
     let path = PathBuf::from(env::var("OUT_DIR").unwrap())
@@ -80,17 +82,23 @@ fn generate_unicode_3_2() {
     write_derived(
         &base,
         "DerivedBidiClass-3.2.0.txt",
-        "BIDI_CLASS",
+        "BIDI_CLASS_DIFF",
         "(u32, u32, BidiClass)",
         NonZeroUsize::new(1).unwrap(),
         &mut writer,
         |start, end, id, _| {
             let id = parse_bidi(id);
-            if id != "BidiClass::LeftToRight" {
-                Some((start, end, id))
-            } else {
-                None
+            for i in start..=end {
+                let legacy = BidiClass::try_from_str(id.rsplit_once("::").unwrap().1)
+                    .expect("Unicode data contains valid variants");
+                let modern = char::from_u32(i).map(BidiClass::for_char);
+
+                if Some(legacy) != modern {
+                    return Some((start, end, id));
+                }
             }
+
+            None
         },
         |writer, mut values| {
             values.sort_unstable_by_key(|(start, _, _)| *start);
@@ -197,6 +205,75 @@ fn generate_numeric_type() {
     );
 }
 
+/// Generate a compressed array of Unicode 3.2 membership.
+///
+/// Membership + diff checks is more efficient than storing the full table for 3.2. The logic is to
+/// default to the latest Unicode if a character exists in 3.2 but isn't different. Membership
+/// is needed because diffs aren't enough - a character may be absent in 3.2 which is different
+/// than returning a default.
+fn generate_membership_3_2() {
+    let path = Path::new(env!("CARGO_MANIFEST_DIR"))
+        .join("unicode")
+        .join("ucd32")
+        .join("UnicodeData-3.2.0.txt");
+    let reader = BufReader::new(File::open(path).unwrap());
+
+    // Parse membership from the first data. Unfortunately, this is largely uncompressed.
+    let mut membership_set = BTreeSet::new();
+    // TODO: Oh my, is this hacky and ugly...
+    let mut range_membership = Vec::new();
+    parse_unicode_3_2(
+        reader,
+        NonZeroUsize::new(1).unwrap(),
+        &mut io::empty(),
+        |start, end, value, _| {
+            if value.ends_with("First>") | value.ends_with("Last>") {
+                // Some lines (literally 20) are compressed ranges, so we have to handle those separately
+                range_membership.push(start);
+            } else {
+                membership_set.insert((start, end));
+            }
+
+            Option::<()>::None
+        },
+        |_writer, _values| {},
+    );
+
+    // Second pass. Compress the ranges.
+    let mut iter = membership_set.iter();
+    let &(mut start_prev, mut end_prev) = iter.next().unwrap();
+    let mut membership = Vec::new();
+
+    for &(start, end) in iter {
+        if start <= end_prev + 1 {
+            end_prev = end_prev.max(end);
+        } else {
+            membership.push((start_prev, end_prev));
+            start_prev = start;
+            end_prev = end;
+        }
+    }
+    membership.push((start_prev, end_prev));
+
+    let (chunks, &[]) = range_membership.as_chunks::<2>() else {
+        panic!("Range membership is always in pairs")
+    };
+    for &chunk in chunks {
+        membership.push(chunk.into());
+    }
+
+    membership.sort_unstable_by_key(|&(start, _)| start);
+
+    let path = PathBuf::from(env::var("OUT_DIR").unwrap())
+        .join("generated")
+        .join("membership_3_2.rs");
+    fs::create_dir_all(path.parent().unwrap()).unwrap();
+    let mut writer = BufWriter::new(File::create(&path).unwrap());
+
+    writeln!(writer, "static MEMBERSHIP_3_2: &[(u32, u32)] = &").unwrap();
+    write!(writer, "{membership:?};").unwrap();
+}
+
 fn generate_numeric_value() {
     let path = PathBuf::from(env::var("OUT_DIR").unwrap())
         .join("generated")
@@ -204,14 +281,10 @@ fn generate_numeric_value() {
     fs::create_dir_all(path.parent().unwrap()).unwrap();
     let mut writer = BufWriter::new(File::create(&path).unwrap());
 
-    // Ideally, this would store the diffs between the two tables. However, we need 3.2.0
-    // membership as well as different chars. The final tables are both smaller than storing the
-    // full 3.2.0 value table.
     let ucd32 = Path::new(env!("CARGO_MANIFEST_DIR"))
         .join("unicode")
         .join("ucd32");
     let mut ucd32_diffs = BTreeMap::new();
-    let mut ucd32_member = BTreeSet::new();
     let numeric_32 =
         BufReader::new(File::open(ucd32.join("DerivedNumericValues-3.2.0.txt")).unwrap());
     parse_unicode_3_2(
@@ -223,7 +296,6 @@ fn generate_numeric_value() {
                 .parse()
                 .expect("Unicode data contains valid properties");
             ucd32_diffs.insert((start, end), value);
-            ucd32_member.insert((start, end));
             Option::<()>::None
         },
         |_writer, _values| {},
@@ -270,26 +342,6 @@ fn generate_numeric_value() {
         write!(writer, "({start}, {end}, {value:?}),").unwrap();
     }
     writeln!(writer, "];").unwrap();
-
-    // Compress membership table
-    let mut iter = ucd32_member.iter();
-    let &(mut start_prev, mut end_prev) = iter.next().unwrap();
-    let mut membership = Vec::new();
-
-    for &(start, end) in iter {
-        if start <= end_prev + 1 {
-            end_prev = end_prev.max(end);
-        } else {
-            membership.push((start_prev, end_prev));
-            start_prev = start;
-            end_prev = end;
-        }
-    }
-    membership.push((start_prev, end_prev));
-    membership.sort_unstable_by_key(|&(start, _)| start);
-
-    writeln!(writer, "static NUMERIC_VAL_EXISTS_32: &[(u32, u32)] = &").unwrap();
-    write!(writer, "{membership:?};").unwrap();
 }
 
 fn generate_unicode_latest() {
@@ -602,10 +654,12 @@ fn main() {
     println!("cargo:rerun-if-changed=unicode/latest");
 
     let t_32 = thread::spawn(generate_unicode_3_2);
+    let t_32_membership = thread::spawn(generate_membership_3_2);
     let t_numeric_type = thread::spawn(generate_numeric_type);
     let t_numeric_value = thread::spawn(generate_numeric_value);
     let t_latest = thread::spawn(generate_unicode_latest);
     t_32.join().unwrap();
+    t_32_membership.join().unwrap();
     t_numeric_type.join().unwrap();
     t_numeric_value.join().unwrap();
     t_latest.join().unwrap();

crates/unicode/src/data.rs

@@ -19,6 +19,7 @@ use icu_properties::props::{
 };
 use rustpython_wtf8::CodePoint;
 
+include!(concat!(env!("OUT_DIR"), "/generated/membership_3_2.rs"));
 include!(concat!(env!("OUT_DIR"), "/generated/unicode_3_2.rs"));
 include!(concat!(env!("OUT_DIR"), "/generated/unicode_latest.rs"));
 include!(concat!(env!("OUT_DIR"), "/generated/unicode_num_type.rs"));
@@ -70,7 +71,7 @@ impl DecompositionType {
     }
 }
 
-fn lookup_property<T: Copy>(table: &[(u32, u32, T)], ch: char) -> Option<T> {
+fn lookup_table<T: Copy>(table: &[(u32, u32, T)], ch: char) -> Option<T> {
     let ch = ch as u32;
     table
         .binary_search_by(|&(start, end, _)| {
@@ -86,25 +87,34 @@ fn lookup_property<T: Copy>(table: &[(u32, u32, T)], ch: char) -> Option<T> {
         .map(|i| table[i].2)
 }
 
-fn lookup_numeric_val(ch: char, modern: bool) -> Option<f64> {
+fn membership_3_2(ch: u32) -> bool {
+    MEMBERSHIP_3_2
+        .binary_search_by(|&(start, end)| {
+            if ch > end {
+                Ordering::Less
+            } else if ch < start {
+                Ordering::Greater
+            } else {
+                Ordering::Equal
+            }
+        })
+        .is_ok()
+}
+
+fn lookup_property_diff<T: Copy>(
+    table: &[(u32, u32, T)],
+    diff: &[(u32, u32, T)],
+    ch: char,
+    modern: bool,
+) -> Option<T> {
     if modern {
-        lookup_property(NUMERIC_VALUES, ch)
+        lookup_table(table, ch)
     } else {
         cold_path();
-        lookup_property(NUMERIC_VALUES_DIFF, ch).or_else(|| {
-            NUMERIC_VAL_EXISTS_32
-                .binary_search_by(|&(start, end)| {
-                    let ch = ch as u32;
-                    if ch > end {
-                        Ordering::Less
-                    } else if ch < start {
-                        Ordering::Greater
-                    } else {
-                        Ordering::Equal
-                    }
-                })
-                .ok()
-                .and_then(|_| lookup_property(NUMERIC_VALUES, ch))
+        lookup_table(diff, ch).or_else(|| {
+            membership_3_2(ch as u32)
+                .then(|| lookup_table(table, ch))
+                .flatten()
         })
     }
 }
@@ -154,7 +164,7 @@ impl Ucd {
             Some(GeneralCategory::for_char(c))
         } else {
             cold_path();
-            lookup_property(GENERAL_CATEGORY, c)
+            lookup_table(GENERAL_CATEGORY, c)
         }
         .unwrap_or(GeneralCategory::Unassigned)
         .short_name()
@@ -168,11 +178,13 @@ impl Ucd {
                     Some(BidiClass::for_char(c))
                 } else {
                     cold_path();
-                    lookup_property(BIDI_CLASS, c)
+                    lookup_table(BIDI_CLASS_DIFF, c)
+                        .or_else(|| membership_3_2(c as u32).then(|| BidiClass::for_char(c)))
                 }
             })
-            .unwrap_or(BidiClass::LeftToRight)
-            .short_name()
+            .as_ref()
+            .map(BidiClass::short_name)
+            .unwrap_or_default()
     }
 
     #[must_use]
@@ -192,7 +204,7 @@ impl Ucd {
                     //
                     // Currently, this implementation is incomplete because I can't figure
                     // out what CPython is doing.
-                    lookup_property(EAST_ASIAN_WIDTH, c)
+                    lookup_table(EAST_ASIAN_WIDTH, c)
                 }
             })
             .unwrap_or(EastAsianWidth::Neutral)
@@ -230,7 +242,7 @@ impl Ucd {
                     Some(CanonicalCombiningClass::for_char(c))
                 } else {
                     cold_path();
-                    lookup_property(COMBINING_CLASS, c)
+                    lookup_table(COMBINING_CLASS, c)
                 }
             })
             .unwrap_or(CanonicalCombiningClass::NotReordered)
@@ -296,7 +308,7 @@ impl Ucd {
             NumericType::for_char(ch)
         } else {
             cold_path();
-            lookup_property(NUMERIC_TYPE_DIFF, ch).unwrap_or_else(|| NumericType::for_char(ch))
+            lookup_table(NUMERIC_TYPE_DIFF, ch).unwrap_or_else(|| NumericType::for_char(ch))
         };
 
         expected.contains(&actual).then_some(ch)
@@ -307,7 +319,7 @@ impl Ucd {
     pub fn digit(&self, c: CodePoint) -> Option<u64> {
         let expected = [NumericType::Decimal, NumericType::Digit];
         self.numeric_type_matches(c, &expected).and_then(|ch| {
-            let value = lookup_numeric_val(ch, true)?;
+            let value = lookup_property_diff(NUMERIC_VALUES, &[], ch, true)?;
             let int = value as u64;
             (int as f64 == value).then_some(int)
         })
@@ -318,7 +330,7 @@ impl Ucd {
     pub fn decimal(&self, c: CodePoint) -> Option<u64> {
         let expected = [NumericType::Decimal];
         self.numeric_type_matches(c, &expected).and_then(|ch| {
-            let value = lookup_numeric_val(ch, self.modern)?;
+            let value = lookup_property_diff(NUMERIC_VALUES, NUMERIC_VALUES_DIFF, ch, self.modern)?;
             let int = value as u64;
             (int as f64 == value).then_some(int)
         })
@@ -328,8 +340,9 @@ impl Ucd {
     #[must_use]
     pub fn numeric(&self, c: CodePoint) -> Option<f64> {
         let expected = &NumericType::ALL_VALUES[1..];
-        self.numeric_type_matches(c, expected)
-            .and_then(|ch| lookup_numeric_val(ch, self.modern))
+        self.numeric_type_matches(c, expected).and_then(|ch| {
+            lookup_property_diff(NUMERIC_VALUES, NUMERIC_VALUES_DIFF, ch, self.modern)
+        })
     }
 
     #[must_use]