Skip to content

Apply GB5 before GB9b in GraphemeCursor::provide_context - #180

Open
jaideeppyne wants to merge 3 commits into
unicode-rs:masterfrom
jaideeppyne:fix/gb5-precedence-over-gb9b-in-provide-context
Open

Apply GB5 before GB9b in GraphemeCursor::provide_context#180
jaideeppyne wants to merge 3 commits into
unicode-rs:masterfrom
jaideeppyne:fix/gb5-precedence-over-gb9b-in-provide-context

Conversation

@jaideeppyne

Copy link
Copy Markdown

provide_context short-circuits to "no break" whenever the last codepoint of the supplied pre-context chunk is Prepend, applying GB9b directly. GB4 and GB5 are ordered before GB9b in UAX #29, so that shortcut is wrong when the codepoint after the cursor is Control, CR or LF: there is a break there regardless of the Prepend.

GraphemeBreakTest-17.0.0.txt spells this out and attributes the break to rule 5.0:

÷ 06DD ÷ 000D ÷  #  ÷ [0.2] ARABIC END OF AYAH (Prepend) ÷ [5.0] <CARRIAGE RETURN (CR)> (CR) ÷ [0.3]

check_pair already orders the rules correctly, so the Graphemes iterator and any cursor handed the whole string are unaffected. Only a cursor answering the query across a chunk boundary disagreed:

let s = "\u{06dd}\r";
assert_eq!(s.graphemes(true).count(), 2);

let mut c = GraphemeCursor::new(2, s.len(), true);
assert_eq!(c.is_boundary(&s[2..], 2), Err(GraphemeIncomplete::PreContext(2)));
c.provide_context(&s[..2], 0);
c.is_boundary(&s[2..], 2);  // Ok(false) on master, should be Ok(true)

The same happens for \u{06dd}\n and \u{06dd}\0, and for every other Prepend followed by Control/CR/LF. It reproduces on the published 1.13.3, and the shortcut has been there since f0df6be (2017).

The fix skips the shortcut when the following category is one GB4/GB5 already decides. The _ arm of the match self.state below then records cat_before as it does for every other category, so the retried is_boundary reaches check_pair and applies the rules in the right order. The InCbConsonant, Regional and Emoji states can't have Control/CR/LF after the cursor, so their reliance on the shortcut (needed, since handle_emoji and friends would otherwise mis-decide on a preceding Prepend) is untouched.

Found by driving GraphemeCursor over the whole of the bundled GraphemeBreakTest.txt with the input split into one chunk per codepoint and comparing against graphemes(). That harness plus a 300k-case random fuzz (one representative codepoint per GCB category, both extended and legacy, across is_boundary/next_boundary/prev_boundary) reports exactly this one root cause, and is clean afterwards. Happy to submit the harness as a separate test-only PR if you'd want it in CI.

Disclosure: this patch was prepared with AI assistance.

`provide_context` short-circuits to "no break" whenever the last codepoint
of the supplied pre-context chunk is `Prepend`, applying GB9b directly.
GB4 and GB5 are ordered before GB9b in UAX unicode-rs#29, so that shortcut is wrong
when the codepoint after the cursor is `Control`, `CR` or `LF`: there is a
break there regardless of the `Prepend`.

GraphemeBreakTest-17.0.0.txt spells this out, attributing the break to
rule 5.0:

    ÷ 06DD ÷ 000D ÷  # ÷ [0.2] ARABIC END OF AYAH (Prepend) ÷ [5.0] <CR> (CR) ÷ [0.3]

`check_pair` already orders the rules correctly, so the `Graphemes`
iterator and any cursor given the whole string are unaffected; only a
cursor answering the query across a chunk boundary disagreed:

    let s = "\u{06dd}\r";
    let mut c = GraphemeCursor::new(2, s.len(), true);
    c.is_boundary(&s[2..], 2);        // Err(PreContext(2))
    c.provide_context(&s[..2], 0);
    c.is_boundary(&s[2..], 2);        // Ok(false), should be Ok(true)

Skip the shortcut when the following category is one GB4/GB5 decides.
The remaining `_` arm of the `match self.state` below already records
`cat_before`, so the retried `is_boundary` reaches `check_pair` and
applies the rules in the right order. The `InCbConsonant`, `Regional`
and `Emoji` states are unreachable with a `Control`/`CR`/`LF` after the
cursor, so their reliance on the shortcut is untouched.

Found by running the cursor over the whole of GraphemeBreakTest.txt with
the input split into one chunk per codepoint.
@Manishearth

Copy link
Copy Markdown
Member

Could you add a couple manual testcases for this behavior in https://github.com/unicode-rs/unicode-segmentation/blob/master/tests/test.rs#L36 ? Makes it easier to check against other reference impls

Adds the segmentation expectations to EXTRA_SAME so they can be read off
and compared against other implementations, plus a chunked GraphemeCursor
test, since tests/test.rs previously only exercised the graphemes iterator
and so never reached provide_context.
The previous commit accidentally reverted src/grapheme.rs while adding the
integration tests.
@jaideeppyne

Copy link
Copy Markdown
Author

Added. Two parts, since tests/test.rs only exercised the graphemes iterator:

The segmentation expectations are now in EXTRA_SAME\u{06DD}\r, \u{06DD}\n, \u{06DD}\0 and \u{0600}\r, each two clusters — so they can be read off directly and compared against another implementation.

Those alone don't reach the bug though, because the iterator sees the whole string and check_pair already orders the rules correctly. So I also added test_grapheme_cursor_chunked_matches_iterator, which walks a GraphemeCursor one codepoint at a time and compares every offset against the iterator for both extended and legacy. That one fails on master at "\u{6dd}\r" extended=true offset=2.

Apologies for the noise in the middle commit — it reverted src/grapheme.rs by accident and the third commit puts it back. Happy to squash if you'd prefer a clean history.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants