Repository navigation
Run all three paragraph directions in the BidiTest conformance suite - #152
Merged
Merged
Conversation
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
approved these changes
Sep 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This does not fix a library bug —
unicode-bidiis fully conformant. It fixesthe conformance runner, which has been asserting a third of
BidiTest.txtandreporting 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):can only ever be true for
bit == 0. Forbit == 1the expression isbitset & 2, which is0or2; forbit == 2it isbitset & 4, which is0or
4. Neither is ever1. SoVALUES[1](LTR) andVALUES[2](RTL) areunreachable.
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:71documents the field as1 = auto-LTR, 2 = LTR, 4 = RTL.What that costs
Counted two independent ways — a script over the data file, and an
AtomicUsizeinstrumenting the loop, which agreed exactly:
BidiTest.txtThe 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 currentlyproduces an empty vector, the
forbody never runs, andpassed_numneverincrements, so the panic at
:179cannot fire.The runtime is corroborating evidence:
cargo test --release --test conformance_testsgoes 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:
Noise floor +11.2% min; this PR +170.9% min. It lands on the
770241/256747 = 3.00case-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 --checkisclean. I also checked
--no-default-featuresexplicitly rather than assuming:[[test]] required-features = ["hardcoded-data"]means the conformance test is notbuilt 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:
exp_base_levelis parsed and never asserted.BidiCharacterTest.txtfield2 is read at
:237and only stored into theFailstruct at:273, whileactual_base_levelis hardcodedNoneat:278. I added the missing assertionagainst
para.level: 0 mismatches across all 91,709 lines.test_character_conformancenever exercises the UTF-16 API, whiletest_basic_conformancedoes at:138-145(added inf04d399, never mirroredto the sibling). I cross-checked
BidiInfoU16levels, paragraph level,reorder_visualandhas_rtlacross 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.txtcontains no astral code points, so nosurrogate-pair path is covered either way.
Also worth flagging separately
src/char_data/tables.rs:8saysUNICODE_VERSION = (17, 0, 0), but both vendoredconformance files are Unicode 15.0.0 (
BidiTest-15.0.0.txt,BidiCharacterTest-15.0.0.txt). I did not touch that here — refreshing the fixturesis 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.