Repository navigation
fix: expand typographic ligatures instead of dropping them (#172) - #185
viwe-monai wants to merge 3 commits into
Conversation
…#172) Unicode ligatures (U+FB00-FB06: ff, fi, fl, ffi, ffl, st) were dropped entirely during text extraction, silently corrupting words (classifies -> 'classi es', affected -> 'a ected'). Expand them to their constituent ASCII letters in clean_text() and add a regression test covering all six code points. Fixes firecrawl#172
There was a problem hiding this comment.
All reported issues were addressed across 1 file
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
… review) Address review feedback: - P2: The PDF path (formats::pdf::to_markdown) bypasses clean_text(). Add expand_ligatures() in the PDF module and apply it to the extracted markdown before returning. - P3: Add U+FB05 and U+FB06 test coverage to ligatures_expanded.
|
Thanks for catching both issues! Fixed in 312b6c6: P2 (PDF path bypasses P3 (missing U+FB05/FB06 coverage): Added |
rahul05ranjan
left a comment
There was a problem hiding this comment.
The expansion table and the motivation are right, but as written this PR's fast path shouldn't compile on stable Rust:
if !text.contains('\u{fb00}'..='\u{fb06}') {str::contains takes a Pattern, and there is no impl Pattern for RangeInclusive<char> — I checked the std sources on 1.45, 1.70, and current master; the only stable pattern implementors are char, &str, &[char]/[char; N], and FnMut(char) -> bool. The range pattern impl people remember never landed. Since the only check that ran on this PR is the cubic reviewer (no cargo CI), nothing caught it. The receiver and argument just need to swap — the correct form is:
if !text.chars().any(|c| ('\u{fb00}'..='\u{fb06}').contains(&c)) {(or text.contains(&['\u{fb00}', '\u{fb01}', …][..]), which is a valid pattern). Notably, downstream forks of this same function in other PDF-extraction crates use exactly the .chars().any(...) form, which is what sent me to check in the first place.
Two more things while you're in here:
- The ligature table now exists twice inside anydoc — once in
pdf.rs::expand_ligatures, once inclean_text. Makingexpand_ligaturesapub(crate)helper inshared/text.rsthat both call would keep the two tables from drifting (you'd still need the separate pdf.rs path, sinceclean_textcollapses line breaks, which PDF markdown must keep). - The PR description frames the change as PDF-specific ("the PDF extraction path bypasses clean_text"), but adding the arms to
clean_textchanges output for every format — a DOCX text run containing a literal fi now comes out asfitoo. Probably the consistency you want, but the description should say so; also worth keeping in mind that pdf-inspector itself has an internalexpand_ligatureswith slightly different behavior (it also strips ZWNJ/ZWJ, whichclean_textdeliberately preserves), so the anydoc-side table is now a third variant of the same mapping across the two crates.
Couldn't verify the end-to-end fix for #172 from here since the build can't pass as-is — the unit tests cover clean_text only, and there's no test for the pdf.rs path specifically.
…eview) Address review feedback from @rahul05ranjan: - The fast path `text.contains('\u{fb00}'..='\u{fb06}')` does not compile on stable Rust: `str::contains` takes a `Pattern` and `RangeInclusive<char>` only implements `Pattern` on nightly (E0277, reproduced with rustc 1.99.1). Use `chars().any(...)` instead. The range endpoints U+FB00–U+FB06 match the expansion table exactly (ff fi fl ffi ffl st st), so the set of expanded characters is unchanged. - Deduplicate the ligature table: `expand_ligatures` is now `pub(crate)` in `shared/text.rs`, backed by a per-char `ligature_expansion` helper that `clean_text` also uses. pdf.rs keeps its own call site because `clean_text` collapses line breaks, which PDF Markdown must keep. - Fix a latent test typo present since 219f2b5: `clean_text("waf\u{fb04}e")` cannot yield "waffle" because U+FB04 expands to "ffl"; the ligature form of "waffle" is `"wa\u{fb04}e"`. - Add `expand_ligatures_pdf_path` covering all seven ligature code points, line-break preservation, and the no-ligature fast path. Verified locally on stable (rustc 1.99.1), mirroring CI: cargo fmt --all --check, cargo clippy --workspace --all-targets --all-features -- -D warnings, cargo clippy -p anydoc-wasm --target wasm32-unknown-unknown, and cargo test --locked (all green).
|
@rahul05ranjan Thanks for the careful read — all three points landed in the latest push ( Compile error — confirmed and reproduced exactly as described: if !text.chars().any(|c| ligature_expansion(c).is_some()) {The range endpoints were right: U+FB00–U+FB06 matches the expansion table exactly (ff→ff, fi→fi, fl→fl, ffi→ffi, ffl→ffl, ſt/st→st), so the set of expanded characters is unchanged. Table duplication — done as suggested: Description — the body now states that the Also fixed while in here: Verified locally on stable, mirroring CI: |
|
Great catch on the
Thanks for the detailed review — the std-source check on the |
What
Expand Unicode typographic ligatures (U+FB00–U+FB06) to their constituent ASCII letters in
clean_text(), instead of dropping them entirely.Scope note:
clean_text()feeds every format's text path, so this changes output beyond PDF — e.g. a DOCX text run containing a literal fi now comes out asfitoo. That cross-format consistency is intended. The PDF extraction path additionally runsexpand_ligatures()(now insrc/shared/text.rs, shared withclean_text) because it bypasses the normalizer:clean_textcollapses line breaks, which PDF Markdown must keep. (pdf-inspector's internal variant also strips ZWNJ/ZWJ; the anydoc-side expansion deliberately preserves them, asclean_textalways has.)Why
Fixes #172 — PDFs using typographic ligatures silently corrupt words in the output:
classifies→classi es,affected→a ected.How it was verified
ligatures_expandedcovers all seven code points viaclean_text;expand_ligatures_pdf_pathcovers the PDF path (all seven ligatures, line breaks preserved, no-ligature fast path)layout_invisibles_strippedandjoin_controls_preservedtests pass (no changes to their behavior)cargo fmt --all --check,cargo clippy --workspace --all-targets --all-features -- -D warnings, andcargo test --lockedall pass locally on stable (rustc 1.99.1), mirroring CIFollow-ups deliberately left out
pdf_inspector's ToUnicode mapping would prevent the characters from being dropped at the parsing level. This anydoc-level fix is a safety net regardless of the upstream behavior.Summary by cubic
Expands Unicode typographic ligatures (U+FB00–U+FB06) to their ASCII letters instead of dropping them, fixing silent word corruption in PDF extraction (
classifies→classi es,affected→a ected).expand_ligatures()insrc/shared/text.rsbacks bothclean_text()and the PDF path, which uses it directly since it must keep line breaks.expand_ligatures()uses a fast path that returns the text unchanged when no ligatures are present.layout_invisibles_strippedandjoin_controls_preservedbehavior is unchanged.pdf_inspector-level ToUnicode fix is deliberately out of scope; this anydoc-side change is a safety net regardless of upstream behavior.Written for commit 0686d87. Summary will update on new commits.
Update after review (@rahul05ranjan):
clean_textchanges output for every format, not just PDF — a DOCX text run containing a literal fi now comes out asfitoo. This is intentional consistency. Note also thatpdf-inspectorhas its own internalexpand_ligatureswith slightly different behavior (it strips ZWNJ/ZWJ, whichclean_textdeliberately preserves) — the anydoc-side table is a separate variant by design.shared::text::ligature_expansion/pub(crate) expand_ligatures— used by bothclean_textand the PDF path (src/formats/pdf.rs), so the two can't drift.text.chars().any(|c| ligature_expansion(c).is_some())(thePattern for RangeInclusive<char>form never landed); verified with stablecargo 1.99.1.shared::text::tests::expand_ligatures_pdf_pathcovers the PDF entry point directly (all 7 ligatures U+FB00–U+FB06, line breaks preserved, fast path on plain text).Verification:
cargo test— 288 lib tests + 10 integration tests, 0 failed on stable 1.99.1.