Skip to content

fix(normalizer): make NormalizedString::prepend work when normalized is empty - #2339

Open
natedemoss wants to merge 1 commit into
huggingface:mainfrom
natedemoss:fix/normalized-string-prepend-after-clear
Open

natedemoss wants to merge 1 commit into
huggingface:mainfrom
natedemoss:fix/normalized-string-prepend-after-clear

Conversation

@natedemoss

Copy link
Copy Markdown

Problem

NormalizedString::prepend is a silent no-op when normalized is empty:

let mut n = NormalizedString::from("Hello");
n.clear();
n.prepend("World ");
assert_eq!(n.get(), "World "); // fails, n.get() == ""

Same through the Python binding:

s = NormalizedString("Hi."); s.clear(); s.prepend("Hello.")  # normalized stays empty

This is the unfixed half of #1636. That issue reported it for both append and prepend ("This is also a problem with prepend"); #1717 fixed append and added test_append_after_clear, but left prepend alone.

prepend anchors on the first character of normalized - it needs it both as the target range for transform_range and as the trailing element it re-emits:

if let Some(next) = self.normalized.chars().next() {
    ...
    self.transform_range(Range::Normalized(0..next.len_utf8()), transformations, 0);
}
// no else -> returns unchanged, no error

On an empty normalized there is no anchor, the body is skipped, and there is no else. clear() is exactly the operation that empties normalized while leaving original intact.

Fix

Mirror the merged append fix: with no anchor character to replace, emit every char of s as an insertion over the empty normalized range.

} else {
    let transformations = s.chars().map(|c| (c, 1));
    self.transform_range(Range::Normalized(..), transformations, 0);
}

Insertions with idx < 1 get alignment (0, 0), a zero-width span at the start of the original - correct, since none of the text originates from original. original and len_original() are untouched.

Blast radius is narrow: normalizers::prepend::Prepend::normalize already guards on !normalized.is_empty(), so the Prepend normalizer is unaffected. Only direct NormalizedString::prepend on an empty normalized string changes, and its prior behavior was to drop the input silently. No public API or serialization change, no new dependency.

Tests

test_prepend_after_clear, placed next to the existing test_append_after_clear and asserting the same properties. Fails on main (left: "" right: "World "), passes with the fix.

Full crate suite green: 256 tests, 0 failures (202 lib unit + 33 integration + 21 doc). cargo fmt -- --check and cargo clippy --all-targets --all-features -- -D warnings both clean.

…is empty

`prepend` only ran a transformation when `normalized` still had a first
character to anchor on, so on an empty normalized string (e.g. right
after `clear()`) it silently did nothing.

`append` had the same bug and was fixed in huggingface#1717, but the `prepend` half
of huggingface#1636 was left behind. Mirror that fix: when there is no first
character, splice the whole string in as new characters over the empty
normalized range.

The `Prepend` normalizer already guards on `!normalized.is_empty()`, so
this only changes direct `NormalizedString::prepend` calls (including the
Python `NormalizedString.prepend` binding).

Assisted-by: Claude Opus 5
Copilot AI lite review requested due to automatic review settings August 18, 2026 02:59

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a NormalizedString::prepend edge case where calling prepend() after clear() was a silent no-op because the implementation required an “anchor” first character in normalized. This brings prepend in line with the existing append behavior on empty normalized, restoring correct behavior for both Rust and downstream bindings (e.g., Python).

Changes:

  • Update NormalizedString::prepend to insert characters into an empty normalized range when normalized is empty.
  • Add a regression test test_prepend_after_clear mirroring the existing test_append_after_clear coverage.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@ArthurZucker

Copy link
Copy Markdown
Collaborator

ty! See #2119 if you want as we are moving towards v1

@natedemoss

Copy link
Copy Markdown
Author

Ok

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.

3 participants