fix: compare non-ASCII digits by numeric value in NaturalStringComparer [patch] - #51
Merged
Merged
Conversation
…er [patch]
Chunk detection used char.IsDigit and \d+, which match any Unicode Nd
digit, but CompareNumericChunks then normalized with TrimStart('0') and
fell back to an ordinal compare — both of which assume ASCII 0x30-0x39.
A chunk containing a non-ASCII digit silently degraded to a raw UTF-16
code-point comparison, so Compare("٥", "9") ranked five above nine,
contradicting the type's own documented numeric ordering.
Compare digit chunks by the value each digit spells, via
CharUnicodeInfo.GetDecimalDigitValue, and skip leading zeros by value
rather than by the ASCII '0' character. ASCII ordering is unchanged:
for equal-length ASCII chunks, comparing digit values gives the same
result as the ordinal compare it replaces.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FyZyutu7Xna2FUQK6o8KAC
|
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
2 participants
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.
Fixes #50
The bug
NaturalStringComparerdetects a "numeric" chunk withchar.IsDigit(...)and the regex\d+— both of which match any UnicodeNddigit, not just ASCII0-9. ButCompareNumericChunksthen normalized leading zeros withTrimStart('0')(ASCII zero only) and fell back tostring.Compare(..., StringComparison.Ordinal), both of which assume the digits are ASCII code points0x30-0x39.So a chunk containing a non-ASCII digit silently degraded to a raw UTF-16 code-point comparison, which has nothing to do with numeric magnitude:
Compare("٥", "9")— Arabic-Indic 5 vs. ASCII 9Compare("٠0", "0")— leading Arabic-Indic zeroThis contradicts the class's own XML doc, which promises "embedded numbers are compared as numeric values".
The fix
Of the two options the issue offers, this takes the one that honours the documented contract rather than narrowing it: the chunking already treats a run of
Nddigits as one number, so compare it by the number it spells.CompareNumericChunksnow:'0'character (SkipLeadingZeros), so"٠٠"reduces to a single zero just as"00"does.CharUnicodeInfo.GetDecimalDigitValue.ASCII behaviour is unchanged. For equal-length ASCII chunks, comparing digit values gives exactly the same result as the ordinal compare it replaces, since ASCII digit code points are already in numeric order. All 11 pre-existing tests pass untouched.
Both chunks reaching this method come from the
\d+alternative of the chunk regex, so every character is categoryNdandGetDecimalDigitValueis guaranteed to return 0-9 — noted in a remark on the method.The type's XML doc now states that a run of Unicode decimal digits is a number whatever script it is written in.
Tests
Four new tests in
NaturalStringComparerTests.cs:Compare_NonAsciiDigits_ComparedByNumericValue— the issue's"٥"vs"9"case, plus within-script and Devanagari5 < 30.Compare_NonAsciiDigits_EqualValuesAreEqual—"٥"equals"5", bare and embedded.Compare_NonAsciiLeadingZeros_NormalizedLikeAsciiZeros— the issue's"٠0"vs"0"case, plus all-zero chunks.Compare_MixedScriptDigits_ComparedByNumericValue— a single chunk mixing scripts ("1٥"= 15) still spells one number.Verified by reverting only the
NaturalStringComparer.cschange and re-running: 4 failed / 11 passed. With the fix: 15 passed, 0 failed.🤖 Generated with Claude Code
https://claude.ai/code/session_01FyZyutu7Xna2FUQK6o8KAC
Generated by Claude Code