fix: an empty mapping is a decision, not a failure, so Preserve honours it (#602) - #626
Merged
Conversation
…rs it (#602) 134 characters that transliterate() deletes were kept verbatim by search_key, catalog_key and sort_key. A character the table maps to the empty string is not unknown — it is a decision the table already made: "this has no ASCII form, drop it". ErrorMode governs what happens to characters the table has nothing to say about, and this one has an explicit entry. But the mapped/unmapped test excepted Preserve from empty mappings, on the reading that an empty mapping is a kind of failure the caller asked to keep. So the three presets that pass Preserve kept those characters, while TextPipeline(transliterate=True), which passes Ignore, dropped them correctly. That divergence is what made the bug visible. catalog_key made it worse by running the confusable fold AFTER transliteration, so a leaked Cyrillic soft sign was folded onto Latin `b`: `Пьеса` became `pbesa`, a key containing a letter present in neither the input nor its romanisation. Now `pesa`. The fix is the mapped test itself: `mapped.is_some()`. A genuinely unmapped code point still reaches handle_unmapped, so Preserve keeps doing its job for the case it exists for — asserted by a test rather than left implied. find_untranslatable_impl passes Ignore and already treated these as translatable, so the two consumers of this predicate now agree instead of disagreeing. Verified by a full-range scan reproducing the issue's own predicate: zero leaks. One property test asserted Preserve never returns empty output. That held only because of the exception removed here, and could not have been true in general: a string of nothing but empty-mapped characters legitimately transliterates to nothing, which is why its generator already excluded \p{M} "which legitimately map to empty". Restated as the invariant that actually defines the mode — Preserve output is never shorter than Ignore output — which needs no generator exclusions and says something stronger. Signed-off-by: Richard Quinn <quinn.richard@gmail.com> Assisted-by: Claude Code:claude-opus-5
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new full-range Python regression test performs ~1.1M iterations with repeated per-iteration work and should be refactored to avoid unnecessary overhead to keep pytest runtime under control.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes a transliteration/preset-policy mismatch where characters with an explicit empty table mapping ("" = “drop”) were incorrectly treated as “unmapped” under ErrorMode::Preserve, causing search_key/catalog_key/sort_key to leak 134 code points that transliterate() deletes.
Changes:
- Treat “present but empty” mappings as mapped (
mapped.is_some()), so all error modes honor deliberate empty mappings. - Restate the Rust property test to assert the defining invariant of
PreservevsIgnore(output length never shorter), instead of incorrectly asserting non-empty output. - Add Python regression coverage for #602, including a targeted soft-sign case and a full-range scan, plus a CHANGELOG entry documenting the behavior fix.
File summaries
| File | Description |
|---|---|
src/transliterate.rs |
Fixes mapped/unmapped classification for empty mappings and updates the relevant property test invariant. |
tests/test_presets.py |
Adds regression tests ensuring presets don’t leak empty-mapped characters and that genuinely unmapped code points are still preserved. |
CHANGELOG.md |
Documents the behavioral fix and its impact on preset key functions. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…urrogates (#602) From review on #626. `chr(cp)` and the probe string were rebuilt for each of their three uses inside a 1.1M-iteration loop. Both are now bound once, and the surrogate range is skipped for consistency with the other full-range scans — a lone surrogate is not a transliteration input, it is an encoding error. Measured effect is small: 1.76s to 1.73s. The loop is dominated by two FFI calls per code point, not by the string building, so this is a readability and consistency fix rather than the performance one it looks like. Recording that here so nobody re-derives it. Signed-off-by: Richard Quinn <quinn.richard@gmail.com> Assisted-by: Claude Code:claude-opus-5
raeq
enabled auto-merge (squash)
August 26, 2026 19:05
…aeq/disarm into fix/602-preserve-empty-mapping
# Conflicts: # CHANGELOG.md
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.
Closes #602.
The confusion
A character the table maps to the empty string is not unknown. It is a decision the table already made: "this has no ASCII form, drop it."
ErrorModegoverns what happens to characters the table has nothing to say about. But the mapped/unmapped test exceptedPreservefrom empty mappings, reading an empty mapping as a kind of failure the caller had asked to keep:So the three presets that pass
Preserve—search_key,catalog_key,sort_key— kept 134 code points thattransliterate()deletes.TextPipeline(transliterate=True)passesIgnoreand dropped them correctly, and that divergence is what made the bug visible.catalog_keymade it worseIt runs the confusable fold after transliteration, so a leaked Cyrillic soft sign was folded onto Latin
b:pbesacontains a letter that appears in neither the input nor its romanisation. For a deduplication key, that is the sharpest form of the problem.The fix
One expression:
let is_mapped = mapped.is_some();A genuinely unmapped code point still reaches
handle_unmapped, soPreservekeeps doing its job for the case it exists for —U+3400still survives, and there is a test asserting it rather than leaving it implied.find_untranslatable_implpassesIgnoreand already treated these as translatable, so the two consumers of this predicate now agree instead of disagreeing.Verified by a full-range scan reproducing the issue's own predicate over
U+0020–U+10FFFF: zero leaks.A property test had to be restated, not just updated
transliterate_preserve_nonemptyasserted thatPreservenever returns empty output. That held only because of the exception removed here — and it could not have been true in general anyway: a string of nothing but empty-mapped characters legitimately transliterates to nothing. Its generator already excluded\p{M}"which legitimately map to empty", which was the same problem showing through in a narrower form.It is now stated as the invariant that actually defines the mode:
That needs no generator exclusions at all, and says something stronger than the original.
Verification
Full local gate: 4,709 pytest, 24 Rust test targets,
cargo fmt --check, clippy clean on both feature sets,scripts/perf_lint.shclean, mypy clean, ruff clean, language-consistency audit clean.Blast radius is four call sites:
catalog_key,search_key,transliterate_preserving_latin_into(used bysort_key), and publictransliterate(on_unknown="preserve"). The last is a documented behaviour change for 134 characters — a fix with a CHANGELOG note at 0.x, not a break.