Skip to content

Run all three paragraph directions in the BidiTest conformance suite - #152

Merged
Manishearth merged 1 commit into
servo:mainfrom
youdie006:fix-conformance-bitset-filter
Sep 10, 2026
Merged

Manishearth merged 1 commit into
servo:mainfrom
youdie006:fix-conformance-bitset-filter

Conversation

@youdie006

Copy link
Copy Markdown
Contributor

This does not fix a library bug — unicode-bidi is fully conformant. It fixes
the conformance runner, which has been asserting a third of BidiTest.txt and
reporting success. I want that stated first so the PR is judged as what it is.

The predicate

gen_base_levels_for_base_tests (tests/conformance_tests.rs:204):

.filter(|bit| bitset & (1u8 << bit) == 1)

can only ever be true for bit == 0. For bit == 1 the expression is
bitset & 2, which is 0 or 2; for bit == 2 it is bitset & 4, which is 0
or 4. Neither is ever 1. So VALUES[1] (LTR) and VALUES[2] (RTL) are
unreachable.

It contradicts its own surroundings: the doc comment on line 200 names all three
values ("auto-LTR, LTR, RTL"), the assert on line 202 requires all three bits to be
meaningful, and tests/data/BidiTest.txt:71 documents the field as
1 = auto-LTR, 2 = LTR, 4 = RTL.

What that costs

Counted two independent ways — a script over the data file, and an AtomicUsize
instrumenting the loop, which agreed exactly:

data lines in BidiTest.txt 490,846
conformance cases (sum of popcount(bitset)) 770,241
cases asserted today 256,747 (exactly 1/3.00)
skipped 513,494 (66.7%)
data lines producing zero assertions 234,099 (47.7%)

The bitset distribution in the file is {2: 51303, 3: 182796, 4: 182796, 5: 51303, 7: 22648} — no line has bitset 1, so every line with bitset 2 or 4 currently
produces an empty vector, the for body never runs, and passed_num never
increments, so the panic at :179 cannot fire.

The runtime is corroborating evidence: cargo test --release --test conformance_tests goes from 0.75s to 1.87s on an unchanged library.

The change

== 1 → != 0, plus a unit test for the helper.

The test rows pin the boundary from both sides: bitset 1 pins bits 1 and 2
excluded when unset, bitsets 2 and 4 pin them included, bitset 5 pins a
non-contiguous mask so an off-by-one cannot survive, and bitset 7 pins ordering.
Reverting the predicate fails it with left: [], right: [Some(Level(0))].

Cost to CI, up front

The test suite gets about 2.5–3x slower — that is the point, but you should
hear it from me rather than notice it. Prebuilt binaries, alternating order, 5
reps, plus a byte-identical second build of master as a noise control:

min median
master 3.85s 4.34s
master, rebuilt (noise control) 4.28s 4.63s
this PR 10.43s 10.60s

Noise floor +11.2% min; this PR +170.9% min. It lands on the 770241/256747 = 3.00
case-count ratio, which is what you would expect if the work simply was not being
done before. Roughly +7s per test row.

Verified rows

cargo test, --features serde, --no-default-features --features hardcoded-data,
and MSRV 1.47.0 all pass with the fix (4 passed each). cargo fmt --check is
clean. I also checked --no-default-features explicitly rather than assuming:
[[test]] required-features = ["hardcoded-data"] means the conformance test is not
built on that row at all.

Two related gaps I found and did not include

Both are real, both currently pass, and I left them out so this diff stays one
thing. Happy to add either here or separately:

  1. exp_base_level is parsed and never asserted. BidiCharacterTest.txt field
    2 is read at :237 and only stored into the Fail struct at :273, while
    actual_base_level is hardcoded None at :278. I added the missing assertion
    against para.level: 0 mismatches across all 91,709 lines.
  2. test_character_conformance never exercises the UTF-16 API, while
    test_basic_conformance does at :138-145 (added in f04d399, never mirrored
    to the sibling). I cross-checked BidiInfoU16 levels, paragraph level,
    reorder_visual and has_rtl across all 91,709 lines: 0 divergences.
    Corrupting a UTF-16 unit as a control fired on 47,082 / 20,596 / 9,863 / 17 lines
    respectively, so that zero is real. Worth knowing it buys less than it sounds
    like: BidiCharacterTest.txt contains no astral code points, so no
    surrogate-pair path is covered either way.

Also worth flagging separately

src/char_data/tables.rs:8 says UNICODE_VERSION = (17, 0, 0), but both vendored
conformance files are Unicode 15.0.0 (BidiTest-15.0.0.txt,
BidiCharacterTest-15.0.0.txt). I did not touch that here — refreshing the fixtures
is a separate change, and it is the place where a genuine library divergence would
most plausibly turn up.

Disclosure

I used an AI assistant to help find and prepare this change. I reviewed and tested
it myself, and the counts and timings above are measurements I ran.

gen_base_levels_for_base_tests filtered with

    bitset & (1u8 << bit) == 1

which can only ever be true for bit 0. For bit 1 the expression is
bitset & 2, which is 0 or 2, and for bit 2 it is bitset & 4, which is
0 or 4. So VALUES[1] (LTR) and VALUES[2] (RTL) were unreachable and the
LTR and RTL paragraph-direction rows were discarded.

BidiTest.txt documents the bitset as "1 = auto-LTR, 2 = LTR, 4 = RTL",
the doc comment on the VALUES array names all three, and the assert
just above the filter requires all three bits to be meaningful.

256747 of the file's 770241 cases were being asserted; the other
513494 were not. 234099 of the 490846 data lines produced no
assertions at all.

unicode-bidi passes all 770241 once they run.
@Manishearth
Manishearth merged commit 774dea2 into servo:main Sep 10, 2026
6 checks passed
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