Skip to content

Compute ParagraphBidiInfo::has_rtl from the resolved levels - #153

Closed
youdie006 wants to merge 1 commit into
servo:mainfrom
youdie006:fix-paragraph-has-rtl
Closed

youdie006 wants to merge 1 commit into
servo:mainfrom
youdie006:fix-paragraph-has-rtl

Conversation

@youdie006

Copy link
Copy Markdown
Contributor

The inconsistency

Four places decide "does this text have RTL levels". Three ask the levels; one asks a flag:

src/lib.rs:595 BidiInfo::reorder_line level::has_rtl(&self.levels[line])
src/lib.rs:683 BidiInfo::has_rtl level::has_rtl(&self.levels)
src/lib.rs:821 ParagraphBidiInfo::reorder_line level::has_rtl(&self.levels[line])
src/lib.rs:858 ParagraphBidiInfo::has_rtl !self.is_pure_ltr

Identical split in src/utf16.rs: :271, :359, :497 use the levels, :532 uses the flag.

is_pure_ltr is documented as "Set to false if any RTL characters or bidi control characters are
encountered in the paragraph."
It is a character scan. It cannot see the paragraph level, which
is the other thing that produces RTL levels.

And the two has_rtl doc comments are byte-identical — both say "If processed text has any
computed RTL levels / This information is usually used to skip re-ordering of text when no RTL level
is present"
— while computing different predicates.

What a caller sees

ParagraphBidiInfo::new("()", Some(Level::rtl())).has_rtl()   // false

() has no RTL character and no bidi control, so is_pure_ltr stays true. But in an RTL paragraph
the neutrals resolve to level 1, so the text is entirely RTL. Both structs agree on that:

text="()" base=RTL   bidi.levels=[Level(1), Level(1)]
                     para.levels=[Level(1), Level(1)]     <- identical
                     BidiInfo::has_rtl=true   ParagraphBidiInfo::has_rtl=false

A caller doing exactly what the doc says — skip re-ordering when no RTL level is present — skips it
on text that is entirely RTL, and renders it in the wrong visual order.

Across all 91,707 BidiCharacterTest.txt inputs the two methods disagree 7,533 times:
7,515 under-reports (Paragraph says false where BidiInfo says true) and 18 the other way.

ParagraphBidiInfo::reorder_line in the same struct protects itself by calling level::has_rtl
directly, which is why this is invisible internally and only reaches external callers.

Why the tests did not catch it

test_bidi_info_has_rtl asserts only BidiInfo and BidiInfoU16, never either Paragraph type.
And every one of its 17 rows is ASCII or contains a real Hebrew character — there is no
neutral-only row
, which is the single shape that separates a character scan from a level check.

This PR adds ("()", RTL, true), ("()", LTR, false), ("()", None, false) and extends the loop
to assert ParagraphBidiInfo and ParagraphBidiInfoU16, guarded by if !t.0.contains('\n') since
that type is single-paragraph.

Reverting either fix fails the RTL row (left: false, right: true); replacing the body with true
fails the LTR and ASCII rows (left: true, right: false). Both sides pinned.

Verification

Every CI row run, not inferred:

row result
cargo test 3/3 suites ok
cargo test --features serde 3/3 ok
cargo test --no-default-features 2/2 ok (conformance test not built — required-features)
cargo test --no-default-features --features=hardcoded-data 3/3 ok
cargo +1.47.0 test (MSRV) 3/3 ok
cargo +1.47.0 test --no-default-features --features=hardcoded-data 3/3 ok
cargo fmt --check exit 0

The Verify job is unaffected: git status shows only src/lib.rs and src/utf16.rs, and
src/char_data/tables.rs is untouched — this changes a predicate over already-computed levels, not
any Unicode data.

Not benchmarked. It replaces a bool read with an #[inline] O(n) scan over a slice that is already
in cache, on a method whose whole purpose is to let callers skip an O(n) reorder. Say the word if
you want numbers.

The alternative

You could instead keep !self.is_pure_ltr and change the doc to describe a "has RTL characters"
probe. I would argue against it — ParagraphBidiInfo::reorder_line eleven lines above already votes
for the levels-based answer, and the 7,515:18 asymmetry means the levels answer is the one that
prevents mis-rendering — but it is your call and it is a one-line docs change if you prefer it.

Note

has_rtl() is public, so a caller using the current over-broad true as a cheap
has-any-bidi-content probe would see fewer trues in 18 of the 91,707 cases. The doc promises
"any computed RTL levels", so I think that is the correct direction, but it is worth stating.

How I found it

While checking whether refreshing the vendored conformance fixtures from Unicode 15 to 17 would
surface anything. It would not — the U17 BidiTest.txt data is byte-identical to the vendored U15
copy, and BidiCharacterTest.txt only removes two lines (both already gone in Unicode 16), so a
refresh would drop two currently-passing cases and add none. Instead I cross-checked the public APIs
the conformance suite never touches, and ParagraphBidiInfo turned out to be a genuine parallel
implementation — reordered_levels, reordered_levels_per_char, reorder_line and
paragraph_level all agree with BidiInfo across all 91,707 inputs. has_rtl was the only one
that did not.

Disclosure

I used an AI assistant to help find and prepare this change. I reviewed and tested it myself, and
every number above is from a run I performed.

ParagraphBidiInfo::has_rtl returned !self.is_pure_ltr, while the three
sibling call sites in the same two files ask level::has_rtl(&self.levels):

  src/lib.rs:595    BidiInfo::reorder_line
  src/lib.rs:683    BidiInfo::has_rtl
  src/lib.rs:821    ParagraphBidiInfo::reorder_line
  src/lib.rs:858    ParagraphBidiInfo::has_rtl      <- the odd one out

is_pure_ltr is a character-scan flag, documented as "set to false if any
RTL characters or bidi control characters are encountered". It is blind
to the paragraph level, which is the other source of RTL levels. So for
neutral-only text in an RTL paragraph:

  ParagraphBidiInfo::new("()", Some(Level::rtl())).has_rtl()  #=> false

while both structs compute the identical levels [Level(1), Level(1)] and
BidiInfo::has_rtl() returns true. The doc comments on the two has_rtl
methods are byte-identical and both promise "any computed RTL levels",
so a caller doing what they say - skipping re-ordering when no RTL level
is present - skips it on text that is entirely RTL.

Across the 91,707 BidiCharacterTest inputs the two disagree 7,533 times,
7,515 of them in this direction.

The existing test asserted only BidiInfo and BidiInfoU16, and every row
was ASCII or contained a real Hebrew character, so no row separated the
two predicates. Add neutral-only rows and assert the Paragraph types.
@Manishearth

Copy link
Copy Markdown
Member

Thanks for the investigation. I'd like to retain the fast path, so I adapted your code into #154.

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