fix: prevent byte/char index mismatch panic in jaro_winkler_distance#1056
Open
xuanwujian wants to merge 1 commit into
Open
fix: prevent byte/char index mismatch panic in jaro_winkler_distance#1056xuanwujian wants to merge 1 commit into
xuanwujian wants to merge 1 commit into
Conversation
jaro_winkler_distance mixed character indices (from .chars().enumerate())
with byte indices (str::len() and &str slicing) when locating matched
characters. Slicing a &str at a byte offset that falls inside a
multi-byte UTF-8 character panics, so any non-ASCII input (e.g.
jaro_winkler_distance("ab", "céd")) crashed the caller's process.
Rewrite the function to operate on Vec<char> throughout instead of
byte-indexed &str, so both the matching-window logic and the score
formula's length denominators use character counts consistently. This
fixes the panic and also corrects the Jaro score itself for non-ASCII
input (byte-length denominators would otherwise under-weight
multi-byte strings even without panicking). The placeholder-based
match removal (replacing a matched character rather than deleting it)
is preserved so existing ASCII results are unchanged.
Added regression tests for the exact repro from the issue plus
CJK/accented-character cases.
Fixes TheAlgorithms#1047
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1056 +/- ##
=======================================
Coverage 95.89% 95.89%
=======================================
Files 396 396
Lines 30442 30458 +16
=======================================
+ Hits 29191 29208 +17
+ Misses 1251 1250 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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.
Fixes #1047
Description
jaro_winkler_distancemixed character indices (from.chars().enumerate()) with byte indices (str::len()and&strslicing) insideget_matched_characters. When a computed slice boundary fell inside a multi-byte UTF-8 character,&strslicing panics (byte index N is not a char boundary). Any non-ASCII input — e.g.jaro_winkler_distance("ab", "céd")— crashed the caller's process.Beyond the panic, the score formula's length denominators (
str1.len()/str2.len()) and the prefix-matching bound were also byte lengths, so a fix that only silenced the panic would still compute a mathematically wrong Jaro score for any multi-byte input (byte length over-counts relative to character count).Fix: rewrote the function to operate on
Vec<char>consistently instead of byte-indexed&str:str1/str2are converted toVec<char>once at the top;get_matched_charactersnow takes&[char]and does all windowing/searching by character position.Vec<char>instead of aString— this preserves the exact existing algorithmic behavior/scores for ASCII input (all 6 pre-existing ASCII test cases produce identical results).pub fn jaro_winkler_distance(str1: &str, str2: &str) -> f64.findsemantics for repeated characters) untouched, as noted in the issue as "not the main bug."Added 3 regression tests: the exact issue repro (
"ab"/"céd", no panic), two fully non-ASCII identical strings (CJK测试/测试and accentedáé/áé), and a partial multi-byte match (测试/测a) with a hand-verified expected score.Type of change
Checklist:
cargo clippy --all -- -D warningsjust before my last commit and fixed any issue that was found. —cargo clippy(nightly) is clean for the file I changed (src/string/jaro_winkler_distance.rs); running it repo-wide with--all -- -D warningscurrently fails due to pre-existing warnings in unrelated files onmaster, unaffected by this change.cargo fmtjust before my last commit.cargo testjust before my last commit and all tests passed.mod.rsfile within its own folder, and in any parent folder(s). (n/a — existing algorithm, already registered)DIRECTORY.mdwith the correct link. (n/a — existing algorithm, already listed)CONTRIBUTING.mdand my code follows its guidelines.