Auto merge of #36 - behnam:runseq, r=mbrubeck

Improve basic conformance and add more unit tests

Continue my review of code based on spec and fix various steps that
drop the basic conformance failures from 12827 to 250.

* X10: Fix incorrect index for `succ_level` resolution (off by
`end_of_seq` because of sub-sequencing) and add unit tests from the spec
text, from both BD13 and X10 sections.

* N1/N2: Fix resetting rule for NI levels by definint `e`, as noted in
the spec.

* L1: Fix not resetting leading whitespace/formatting chars in a line.

* Use non-ASCII sample chars for `gen_char_from_bidi_class()`, which
increases the failure rate from 250 to 314.

* This allows easier testing of the logic, specially having specific cases
from the conformance test as unit test, as corner bugs unfold.

* There's almost no extra cost in expanding the shortcut condition to
check all the isolating run sequences to be LTR, if there's more than
one. This can prevent a bunch of string manipulation when all isolating
run sequences are LTR.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/unicode-bidi/36)
<!-- Reviewable:end -->

Author: bors-servo

benches/basic.rs

@@ -18,15 +18,10 @@ use test::Bencher;
 use unicode_bidi::BidiInfo;
 
 
-const LTR_TEXTS: &[&str] = &[
-    "abc\ndef\nghi",
-    "abc 123\ndef 456\nghi 789",
-];
+const LTR_TEXTS: &[&str] = &["abc\ndef\nghi", "abc 123\ndef 456\nghi 789"];
 
-const BIDI_TEXTS: &[&str] = &[
-    "ابجد\nهوز\nحتی",
-    "ابجد ۱۲۳\nهوز ۴۵۶\nحتی ۷۸۹",
-];
+const BIDI_TEXTS: &[&str] =
+    &["ابجد\nهوز\nحتی", "ابجد ۱۲۳\nهوز ۴۵۶\nحتی ۷۸۹"];
 
 
 fn bench_bidi_info_new(b: &mut Bencher, texts: &[&str]) {

src/explicit.rs

@@ -23,11 +23,11 @@ use BidiClass::*;
 pub fn compute(
     text: &str,
     para_level: Level,
-    initial_classes: &[BidiClass],
+    original_classes: &[BidiClass],
     levels: &mut [Level],
     processing_classes: &mut [BidiClass],
 ) {
-    assert!(text.len() == initial_classes.len());
+    assert!(text.len() == original_classes.len());
 
     // http://www.unicode.org/reports/tr9/#X1
     let mut stack = DirectionalStatusStack::new();
@@ -38,14 +38,14 @@ pub fn compute(
     let mut valid_isolate_count = 0u32;
 
     for (i, c) in text.char_indices() {
-        match initial_classes[i] {
+        match original_classes[i] {
 
             // Rules X2-X5c
             RLE | LRE | RLO | LRO | RLI | LRI | FSI => {
                 let last_level = stack.last().level;
 
                 // X5a-X5c: Isolate initiators get the level of the last entry on the stack.
-                let is_isolate = matches!(initial_classes[i], RLI | LRI | FSI);
+                let is_isolate = matches!(original_classes[i], RLI | LRI | FSI);
                 if is_isolate {
                     levels[i] = last_level;
                     match stack.last().status {
@@ -55,7 +55,7 @@ pub fn compute(
                     }
                 }
 
-                let new_level = if is_rtl(initial_classes[i]) {
+                let new_level = if is_rtl(original_classes[i]) {
                     last_level.new_explicit_next_rtl()
                 } else {
                     last_level.new_explicit_next_ltr()
@@ -64,7 +64,7 @@ pub fn compute(
                    overflow_embedding_count == 0 {
                     let new_level = new_level.unwrap();
                     stack.push(
-                        new_level, match initial_classes[i] {
+                        new_level, match original_classes[i] {
                             RLO => OverrideStatus::RTL,
                             LRO => OverrideStatus::LTR,
                             RLI | LRI | FSI => OverrideStatus::Isolate,

src/format_chars.rs

@@ -34,9 +34,9 @@ pub const PDI: char = '\u{2069}';
 pub const LRE: char = '\u{202A}';
 /// RIGHT-TO-LEFT EMBEDDING
 pub const RLE: char = '\u{202B}';
+/// POP DIRECTIONAL FORMATTING
+pub const PDF: char = '\u{202C}';
 /// LEFT-TO-RIGHT OVERRIDE
 pub const LRO: char = '\u{202D}';
 /// RIGHT-TO-LEFT OVERRIDE
 pub const RLO: char = '\u{202E}';
-/// POP DIRECTIONAL FORMATTING
-pub const PDF: char = '\u{202C}';

src/implicit.rs

@@ -26,6 +26,9 @@ pub fn resolve_weak(sequence: &IsolatingRunSequence, processing_classes: &mut [B
     // by an "earlier" rule.  We should either split this into separate passes, or preserve
     // extra state so each rule can see the correct previous class.
 
+    // FIXME: Also, this could be the cause of increased failure for using longer-UTF-8 chars in
+    // conformance tests, like BidiTest:69635 (AL ET EN)
+
     let mut prev_class = sequence.sos;
     let mut last_strong_is_al = false;
     let mut et_run_indices = Vec::new(); // for W5
@@ -139,22 +142,17 @@ pub fn resolve_neutral(
     levels: &[Level],
     processing_classes: &mut [BidiClass],
 ) {
+    let e: BidiClass = levels[sequence.runs[0].start].bidi_class();
     let mut indices = sequence.runs.iter().flat_map(Clone::clone);
     let mut prev_class = sequence.sos;
 
-    // Neutral or Isolate formatting characters (NI).
-    // http://www.unicode.org/reports/tr9/#NI
-    fn ni(class: BidiClass) -> bool {
-        matches!(class, B | S | WS | ON | FSI | LRI | RLI | PDI)
-    }
-
     while let Some(mut i) = indices.next() {
         // N0. Process bracket pairs.
         // TODO
 
         // Process sequences of NI characters.
         let mut ni_run = Vec::new();
-        if ni(processing_classes[i]) {
+        if is_NI(processing_classes[i]) {
             // Consume a run of consecutive NI characters.
             ni_run.push(i);
             let mut next_class;
@@ -166,7 +164,7 @@ pub fn resolve_neutral(
                             continue;
                         }
                         next_class = processing_classes[j];
-                        if ni(next_class) {
+                        if is_NI(next_class) {
                             ni_run.push(i);
                         } else {
                             break;
@@ -180,11 +178,14 @@ pub fn resolve_neutral(
             }
 
             // N1-N2.
+            //
+            // http://www.unicode.org/reports/tr9/#N1
+            // http://www.unicode.org/reports/tr9/#N2
             let new_class = match (prev_class, next_class) {
                 (L, L) => L,
                 (R, R) | (R, AN) | (R, EN) | (AN, R) | (AN, AN) | (AN, EN) | (EN, R) |
                 (EN, AN) | (EN, EN) => R,
-                (_, _) => levels[i].bidi_class(),
+                (_, _) => e,
             };
             for j in &ni_run {
                 processing_classes[*j] = new_class;
@@ -218,3 +219,11 @@ pub fn resolve_levels(original_classes: &[BidiClass], levels: &mut [Level]) -> L
 
     max_level
 }
+
+/// Neutral or Isolate formatting character (B, S, WS, ON, FSI, LRI, RLI, PDI)
+///
+/// http://www.unicode.org/reports/tr9/#NI
+#[allow(non_snake_case)]
+fn is_NI(class: BidiClass) -> bool {
+    matches!(class, B | S | WS | ON | FSI | LRI | RLI | PDI)
+}

src/lib.rs

@@ -291,16 +291,31 @@ impl<'text> BidiInfo<'text> {
         }
     }
 
-    /// Re-order a line based on resolved levels and return only the embedding levels.
+    /// Re-order a line based on resolved levels and return only the embedding levels, one `Level`
+    /// per *byte*.
     pub fn reordered_levels(&self, para: &ParagraphInfo, line: Range<usize>) -> Vec<Level> {
         let (levels, _) = self.visual_runs(para, line.clone());
         levels
     }
 
+    /// Re-order a line based on resolved levels and return only the embedding levels, one `Level`
+    /// per *character*.
+    pub fn reordered_levels_per_char(
+        &self,
+        para: &ParagraphInfo,
+        line: Range<usize>,
+    ) -> Vec<Level> {
+        let levels = self.reordered_levels(para, line);
+        self.text.char_indices().map(|(i, _)| levels[i]).collect()
+    }
+
+
     /// Re-order a line based on resolved levels and return the line in display order.
     pub fn reorder_line(&self, para: &ParagraphInfo, line: Range<usize>) -> Cow<'text, str> {
         let (levels, runs) = self.visual_runs(para, line.clone());
-        if runs.len() == 1 && levels[runs[0].start].is_ltr() {
+
+        // If all isolating run sequences are LTR, no reordering is needed
+        if runs.iter().all(|run| levels[run.start].is_ltr()) {
             return self.text[line.clone()].into();
         }
 
@@ -333,10 +348,12 @@ impl<'text> BidiInfo<'text> {
         // Reset some whitespace chars to paragraph level.
         // http://www.unicode.org/reports/tr9/#L1
         let line_str: &str = &self.text[line.clone()];
-        let mut reset_from: Option<usize> = None;
+        let mut reset_from: Option<usize> = Some(0);
         let mut reset_to: Option<usize> = None;
         for (i, c) in line_str.char_indices() {
             match self.original_classes[i] {
+                // Ignored by X9
+                RLE | LRE | RLO | LRO | PDF | BN => {}
                 // Segment separator, Paragraph separator
                 B | S => {
                     assert!(reset_to == None);
@@ -635,6 +652,10 @@ mod tests {
                 ],
             }
         );
+
+        /// BidiTest:69635 (AL ET EN)
+        let bidi_info = BidiInfo::new("\u{060B}\u{20CF}\u{06F9}", None);
+        assert_eq!(bidi_info.original_classes, vec![AL, AL, ET, ET, ET, EN, EN]);
     }
 
     #[test]
@@ -703,6 +724,18 @@ mod tests {
         // Numbers being weak LTR characters, cannot reorder strong RTL
         assert_eq!(reorder_paras("123 אבג"), vec!["גבא 123"]);
 
+        assert_eq!(reorder_paras("abc\u{202A}def"), vec!["abc\u{202A}def"]);
+
+        assert_eq!(
+            reorder_paras("abc\u{202A}def\u{202C}ghi"),
+            vec!["abc\u{202A}def\u{202C}ghi"]
+        );
+
+        assert_eq!(
+            reorder_paras("abc\u{2066}def\u{2069}ghi"),
+            vec!["abc\u{2066}def\u{2069}ghi"]
+        );
+
         // Testing for RLE Character
         assert_eq!(
             reorder_paras("\u{202B}abc אבג\u{202C}"),
@@ -725,6 +758,7 @@ mod tests {
             reorder_paras("abc\u{2067}.-\u{2069}ghi"),
             vec!["abc\u{2067}-.\u{2069}ghi"]
         );
+
         assert_eq!(
             reorder_paras("Hello, \u{2068}\u{202E}world\u{202C}\u{2069}!"),
             vec!["Hello, \u{2068}\u{202E}\u{202C}dlrow\u{2069}!"]
@@ -739,6 +773,60 @@ mod tests {
             vec!["ef].)gh&[דג(בא"]
         );
     }
+
+    fn reordered_levels_for_paras(text: &str) -> Vec<Vec<Level>> {
+        let bidi_info = BidiInfo::new(text, None);
+        bidi_info
+            .paragraphs
+            .iter()
+            .map(|para| bidi_info.reordered_levels(para, para.range.clone()))
+            .collect()
+    }
+
+    fn reordered_levels_per_char_for_paras(text: &str) -> Vec<Vec<Level>> {
+        let bidi_info = BidiInfo::new(text, None);
+        bidi_info
+            .paragraphs
+            .iter()
+            .map(|para| bidi_info.reordered_levels_per_char(para, para.range.clone()))
+            .collect()
+    }
+
+    #[test]
+    fn test_reordered_levels() {
+
+        /// BidiTest:946 (LRI PDI)
+        let text = "\u{2067}\u{2069}";
+        assert_eq!(
+            reordered_levels_for_paras(text),
+            vec![Level::vec(&[0, 0, 0, 0, 0, 0])]
+        );
+        assert_eq!(
+            reordered_levels_per_char_for_paras(text),
+            vec![Level::vec(&[0, 0])]
+        );
+
+        /* TODO
+        /// BidiTest:69635 (AL ET EN)
+        let text = "\u{060B}\u{20CF}\u{06F9}";
+        assert_eq!(
+            reordered_levels_for_paras(text),
+            vec![Level::vec(&[1, 1, 1, 1, 1, 2, 2])]
+        );
+        assert_eq!(
+            reordered_levels_per_char_for_paras(text),
+            vec![Level::vec(&[1, 1, 2])]
+        );
+         */
+
+        /* TODO
+        // BidiTest:291284 (AN RLI PDF R)
+        assert_eq!(
+            reordered_levels_per_char_for_paras("\u{0605}\u{2067}\u{202C}\u{0590}"),
+            vec![&["2", "0", "x", "1"]]
+        );
+         */
+    }
 }