Auto merge of rust-lang#148436 - jieyouxu:revert-unicode-generator, r=Zalathar

Revert "unicode_data refactors RUST-147622"

This PR reverts RUST-147622 for several reasons:

1. The RUST-147622 PR would format the generated core library code using an arbitrary `rustfmt` picked up from `PATH`, which will cause hard-to-debug failures when the `rustfmt` used to format the generated unicode data code versus the `rustfmt` used to format the in-tree library code produce incompatible formatting.
2. Previously, the `unicode-table-generator` tests were not run under CI as part of `coretests`, and since for `x86_64-gnu-aux` job we run library `coretests` with `miri`, the generated tests unfortunately caused an unacceptably large Merge CI time regression from ~2 hours to ~3.5 hours, making it the slowest Merge CI job (and thus the new bottleneck).
3. This PR also has an unintended effect of causing a diagnostic regression (RUST-148387), though that's mostly an edge case not properly handled by `rustc` diagnostics.

Given that these are three distinct causes with non-trivial fixes, I'm proposing to revert this PR to return us to baseline. **This is not prejudice against relanding the changes with these issues addressed, but to alleviate time pressure to address these non-trivial issues.**

FYI `@Kmeakin` `@joboet` (PR author/review). Note that these issues are very subtle, so you cannot be reasonably expected to know about them beforehand.

This was discussed in:

- [#t-infra/bootstrap > &rust-lang#96;unicode_data&rust-lang#96; refactors PR](https://rust-lang.zulipchat.com/#narrow/channel/326414-t-infra.2Fbootstrap/topic/.60unicode_data.60.20refactors.20PR/with/553360227)
- [#t-infra > x86_64-gnu-aux job went from ~2 to ~3.5 hours](https://rust-lang.zulipchat.com/#narrow/channel/242791-t-infra/topic/x86_64-gnu-aux.20job.20went.20from.20~2.20to.20~3.2E5.20hours/with/553354955)

Author: bors

library/core/src/unicode/mod.rs

@@ -18,9 +18,8 @@ pub(crate) use unicode_data::white_space::lookup as White_Space;
 
 pub(crate) mod printable;
 
-mod rt;
 #[allow(unreachable_pub)]
-pub mod unicode_data;
+mod unicode_data;
 
 /// The version of [Unicode](https://www.unicode.org/) that the Unicode parts of
 /// `char` and `str` methods are based on.

library/core/src/unicode/unicode_data.rs

@@ -116,7 +116,6 @@
 #![feature(try_find)]
 #![feature(try_trait_v2)]
 #![feature(uint_bit_width)]
-#![feature(unicode_internals)]
 #![feature(unsize)]
 #![feature(unwrap_infallible)]
 // tidy-alphabetical-end

library/coretests/tests/lib.rs

@@ -1,101 +1,5 @@
-use core::unicode::unicode_data;
-use std::ops::RangeInclusive;
-
-mod test_data;
-
 #[test]
 pub fn version() {
     let (major, _minor, _update) = core::char::UNICODE_VERSION;
     assert!(major >= 10);
 }
-
-#[track_caller]
-fn test_boolean_property(ranges: &[RangeInclusive<char>], lookup: fn(char) -> bool) {
-    let mut start = '\u{80}';
-    for range in ranges {
-        for c in start..*range.start() {
-            assert!(!lookup(c), "{c:?}");
-        }
-        for c in range.clone() {
-            assert!(lookup(c), "{c:?}");
-        }
-        start = char::from_u32(*range.end() as u32 + 1).unwrap();
-    }
-    for c in start..=char::MAX {
-        assert!(!lookup(c), "{c:?}");
-    }
-}
-
-#[track_caller]
-fn test_case_mapping(ranges: &[(char, [char; 3])], lookup: fn(char) -> [char; 3]) {
-    let mut start = '\u{80}';
-    for &(key, val) in ranges {
-        for c in start..key {
-            assert_eq!(lookup(c), [c, '\0', '\0'], "{c:?}");
-        }
-        assert_eq!(lookup(key), val, "{key:?}");
-        start = char::from_u32(key as u32 + 1).unwrap();
-    }
-    for c in start..=char::MAX {
-        assert_eq!(lookup(c), [c, '\0', '\0'], "{c:?}");
-    }
-}
-
-#[test]
-#[cfg_attr(miri, ignore)]
-fn alphabetic() {
-    test_boolean_property(test_data::ALPHABETIC, unicode_data::alphabetic::lookup);
-}
-
-#[test]
-#[cfg_attr(miri, ignore)]
-fn case_ignorable() {
-    test_boolean_property(test_data::CASE_IGNORABLE, unicode_data::case_ignorable::lookup);
-}
-
-#[test]
-#[cfg_attr(miri, ignore)]
-fn cased() {
-    test_boolean_property(test_data::CASED, unicode_data::cased::lookup);
-}
-
-#[test]
-#[cfg_attr(miri, ignore)]
-fn grapheme_extend() {
-    test_boolean_property(test_data::GRAPHEME_EXTEND, unicode_data::grapheme_extend::lookup);
-}
-
-#[test]
-#[cfg_attr(miri, ignore)]
-fn lowercase() {
-    test_boolean_property(test_data::LOWERCASE, unicode_data::lowercase::lookup);
-}
-
-#[test]
-fn n() {
-    test_boolean_property(test_data::N, unicode_data::n::lookup);
-}
-
-#[test]
-#[cfg_attr(miri, ignore)]
-fn uppercase() {
-    test_boolean_property(test_data::UPPERCASE, unicode_data::uppercase::lookup);
-}
-
-#[test]
-#[cfg_attr(miri, ignore)]
-fn white_space() {
-    test_boolean_property(test_data::WHITE_SPACE, unicode_data::white_space::lookup);
-}
-
-#[test]
-#[cfg_attr(miri, ignore)]
-fn to_lowercase() {
-    test_case_mapping(test_data::TO_LOWER, unicode_data::conversions::to_lower);
-}
-
-#[test]
-#[cfg_attr(miri, ignore)]
-fn to_uppercase() {
-    test_case_mapping(test_data::TO_UPPER, unicode_data::conversions::to_upper);
-}

library/coretests/tests/unicode.rs

@@ -374,7 +374,6 @@ impl Step for UnicodeTableGenerator {
     fn run(self, builder: &Builder<'_>) {
         let mut cmd = builder.tool_cmd(Tool::UnicodeTableGenerator);
         cmd.arg(builder.src.join("library/core/src/unicode/unicode_data.rs"));
-        cmd.arg(builder.src.join("library/coretests/tests/unicode/test_data.rs"));
         cmd.run(builder);
     }
 }

library/coretests/tests/unicode/test_data.rs

@@ -1,8 +1,9 @@
 use std::collections::HashMap;
+use std::fmt::Write as _;
 use std::ops::Range;
 
+use crate::fmt_list;
 use crate::raw_emitter::RawEmitter;
-use crate::writeln;
 
 impl RawEmitter {
     pub fn emit_cascading_map(&mut self, ranges: &[Range<u32>]) -> bool {
@@ -23,6 +24,8 @@ impl RawEmitter {
             .flat_map(|r| (r.start..r.end).collect::<Vec<u32>>())
             .collect::<Vec<u32>>();
 
+        println!("there are {} points", points.len());
+
         // how many distinct ranges need to be counted?
         let mut codepoints_by_high_bytes = HashMap::<usize, Vec<u32>>::new();
         for point in points {
@@ -34,41 +37,41 @@ impl RawEmitter {
         }
 
         let mut bit_for_high_byte = 1u8;
-        let mut arms = String::new();
+        let mut arms = Vec::<String>::new();
 
         let mut high_bytes: Vec<usize> = codepoints_by_high_bytes.keys().copied().collect();
         high_bytes.sort();
         for high_byte in high_bytes {
             let codepoints = codepoints_by_high_bytes.get_mut(&high_byte).unwrap();
             if codepoints.len() == 1 {
                 let ch = codepoints.pop().unwrap();
-                writeln!(arms, "{high_byte:#04x} => c as u32 == {ch:#04x},");
+                arms.push(format!("{high_byte} => c as u32 == {ch:#04x}"));
                 continue;
             }
             // more than 1 codepoint in this arm
             for codepoint in codepoints {
                 map[(*codepoint & 0xff) as usize] |= bit_for_high_byte;
             }
-            writeln!(
-                arms,
-                "{high_byte:#04x} => WHITESPACE_MAP[c as usize & 0xff] & {bit_for_high_byte} != 0,"
-            );
+            arms.push(format!(
+                "{high_byte} => WHITESPACE_MAP[c as usize & 0xff] & {bit_for_high_byte} != 0"
+            ));
             bit_for_high_byte <<= 1;
         }
 
+        writeln!(&mut self.file, "static WHITESPACE_MAP: [u8; 256] = [{}];", fmt_list(map.iter()))
+            .unwrap();
         self.bytes_used += 256;
-        self.file = format!(
-            "static WHITESPACE_MAP: [u8; 256] = {map:?};
 
-            #[inline]
-            pub const fn lookup(c: char) -> bool {{
-                debug_assert!(!c.is_ascii());
-                match c as u32 >> 8 {{
-                    {arms}\
-                    _ => false,
-                }}
-            }}"
-        );
+        writeln!(&mut self.file, "#[inline]").unwrap();
+        writeln!(&mut self.file, "pub const fn lookup(c: char) -> bool {{").unwrap();
+        writeln!(&mut self.file, "    debug_assert!(!c.is_ascii());").unwrap();
+        writeln!(&mut self.file, "    match c as u32 >> 8 {{").unwrap();
+        for arm in arms {
+            writeln!(&mut self.file, "        {arm},").unwrap();
+        }
+        writeln!(&mut self.file, "        _ => false,").unwrap();
+        writeln!(&mut self.file, "    }}").unwrap();
+        writeln!(&mut self.file, "}}").unwrap();
 
         true
     }

src/bootstrap/src/core/build_steps/run.rs

@@ -1,25 +1,27 @@
 use std::char;
 use std::collections::BTreeMap;
+use std::fmt::{self, Write};
 
-use crate::fmt_helpers::Hex;
-use crate::{CharEscape, UnicodeData, fmt_list};
+use crate::{UnicodeData, fmt_list};
 
 const INDEX_MASK: u32 = 1 << 22;
 
 pub(crate) fn generate_case_mapping(data: &UnicodeData) -> (String, [usize; 2]) {
+    let mut file = String::new();
+
+    write!(file, "const INDEX_MASK: u32 = 0x{INDEX_MASK:x};").unwrap();
+    file.push_str("\n\n");
+    file.push_str(HEADER.trim_start());
+    file.push('\n');
     let (lower_tables, lower_size) = generate_tables("LOWER", &data.to_lower);
+    file.push_str(&lower_tables);
+    file.push_str("\n\n");
     let (upper_tables, upper_size) = generate_tables("UPPER", &data.to_upper);
-    let file = format!(
-        "{lower_tables}
-        {upper_tables}"
-    );
+    file.push_str(&upper_tables);
     (file, [lower_size, upper_size])
 }
 
 fn generate_tables(case: &str, data: &BTreeMap<u32, [u32; 3]>) -> (String, usize) {
-    let case_lower = case.to_lowercase();
-    let case_upper = case.to_uppercase();
-
     let mut mappings = Vec::with_capacity(data.len());
     let mut multis = Vec::new();
 
@@ -42,49 +44,77 @@ fn generate_tables(case: &str, data: &BTreeMap<u32, [u32; 3]>) -> (String, usize
             INDEX_MASK | (u32::try_from(multis.len()).unwrap() - 1)
         };
 
-        mappings.push((CharEscape(key), Hex(value)));
+        mappings.push((CharEscape(key), value));
+    }
+
+    let mut tables = String::new();
+    let mut size = 0;
+
+    size += size_of_val(mappings.as_slice());
+    write!(
+        tables,
+        "static {}CASE_TABLE: &[(char, u32); {}] = &[{}];",
+        case,
+        mappings.len(),
+        fmt_list(mappings),
+    )
+    .unwrap();
+
+    tables.push_str("\n\n");
+
+    size += size_of_val(multis.as_slice());
+    write!(
+        tables,
+        "static {}CASE_TABLE_MULTI: &[[char; 3]; {}] = &[{}];",
+        case,
+        multis.len(),
+        fmt_list(multis),
+    )
+    .unwrap();
+
+    (tables, size)
+}
+
+struct CharEscape(char);
+
+impl fmt::Debug for CharEscape {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        write!(f, "'{}'", self.0.escape_default())
+    }
+}
+
+static HEADER: &str = r"
+pub fn to_lower(c: char) -> [char; 3] {
+    if c.is_ascii() {
+        [(c as u8).to_ascii_lowercase() as char, '\0', '\0']
+    } else {
+        LOWERCASE_TABLE
+            .binary_search_by(|&(key, _)| key.cmp(&c))
+            .map(|i| {
+                let u = LOWERCASE_TABLE[i].1;
+                char::from_u32(u).map(|c| [c, '\0', '\0']).unwrap_or_else(|| {
+                    // SAFETY: Index comes from statically generated table
+                    unsafe { *LOWERCASE_TABLE_MULTI.get_unchecked((u & (INDEX_MASK - 1)) as usize) }
+                })
+            })
+            .unwrap_or([c, '\0', '\0'])
     }
+}
 
-    let size = size_of_val(mappings.as_slice()) + size_of_val(multis.as_slice());
-    let file = format!(
-        "
-#[rustfmt::skip]
-static {case}CASE_TABLE: &[(char, u32); {mappings_len}] = &[{mappings}];
-
-#[rustfmt::skip]
-static {case}CASE_TABLE_MULTI: &[[char; 3]; {multis_len}] = &[{multis}];
-
-#[inline]
-pub fn to_{case_lower}(c: char) -> [char; 3] {{
-    const {{
-        let mut i = 0;
-        while i < {case_upper}CASE_TABLE.len() {{
-            let (_, val) = {case_upper}CASE_TABLE[i];
-            if val & (1 << 22) == 0 {{
-                assert!(char::from_u32(val).is_some());
-            }} else {{
-                let index = val & ((1 << 22) - 1);
-                assert!((index as usize) < {case_upper}CASE_TABLE_MULTI.len());
-            }}
-            i += 1;
-        }}
-    }}
-
-    // SAFETY: Just checked that the tables are valid
-    unsafe {{
-        super::case_conversion(
-            c,
-            |c| c.to_ascii_{case_lower}case(),
-            {case_upper}CASE_TABLE,
-            {case_upper}CASE_TABLE_MULTI,
-        )
-    }}
-}}",
-        mappings = fmt_list(&mappings),
-        mappings_len = mappings.len(),
-        multis = fmt_list(&multis),
-        multis_len = multis.len(),
-    );
-
-    (file, size)
+pub fn to_upper(c: char) -> [char; 3] {
+    if c.is_ascii() {
+        [(c as u8).to_ascii_uppercase() as char, '\0', '\0']
+    } else {
+        UPPERCASE_TABLE
+            .binary_search_by(|&(key, _)| key.cmp(&c))
+            .map(|i| {
+                let u = UPPERCASE_TABLE[i].1;
+                char::from_u32(u).map(|c| [c, '\0', '\0']).unwrap_or_else(|| {
+                    // SAFETY: Index comes from statically generated table
+                    unsafe { *UPPERCASE_TABLE_MULTI.get_unchecked((u & (INDEX_MASK - 1)) as usize) }
+                })
+            })
+            .unwrap_or([c, '\0', '\0'])
+    }
 }
+";