Skip to content

fix: expand typographic ligatures instead of dropping them (#172) - #185

Open
viwe-monai wants to merge 3 commits into
firecrawl:mainfrom
viwe-monai:fix/pdf-ligature-expansion
Open

viwe-monai wants to merge 3 commits into
firecrawl:mainfrom
viwe-monai:fix/pdf-ligature-expansion

Conversation

@viwe-monai

@viwe-monai viwe-monai commented Sep 30, 2026 •

Copy link
Copy Markdown

What

Expand Unicode typographic ligatures (U+FB00–U+FB06) to their constituent ASCII letters in clean_text(), instead of dropping them entirely.

Code point Glyph Expansion
U+FB00 ff ff
U+FB01 fi fi
U+FB02 fl fl
U+FB03 ffi ffi
U+FB04 ffl ffl
U+FB05/06 ſt/st st

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 as fi too. That cross-format consistency is intended. The PDF extraction path additionally runs expand_ligatures() (now in src/shared/text.rs, shared with clean_text) because it bypasses the normalizer: clean_text collapses line breaks, which PDF Markdown must keep. (pdf-inspector's internal variant also strips ZWNJ/ZWJ; the anydoc-side expansion deliberately preserves them, as clean_text always 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_expanded covers all seven code points via clean_text; expand_ligatures_pdf_path covers the PDF path (all seven ligatures, line breaks preserved, no-ligature fast path)
  • Existing layout_invisibles_stripped and join_controls_preserved tests pass (no changes to their behavior)
  • cargo fmt --all --check, cargo clippy --workspace --all-targets --all-features -- -D warnings, and cargo test --locked all pass locally on stable (rustc 1.99.1), mirroring CI

Follow-ups deliberately left out

  • A fix in 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).

  • A shared expand_ligatures() in src/shared/text.rs backs both clean_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.
  • Adds regression tests covering all six code points, line-break preservation, and the fast path; layout_invisibles_stripped and join_controls_preserved behavior is unchanged.
  • A 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.

View guided diff


Update after review (@rahul05ranjan):

  • Scope note (was missing): adding the expansion arms to clean_text changes output for every format, not just PDF — a DOCX text run containing a literal fi now comes out as fi too. This is intentional consistency. Note also that pdf-inspector has its own internal expand_ligatures with slightly different behavior (it strips ZWNJ/ZWJ, which clean_text deliberately preserves) — the anydoc-side table is a separate variant by design.
  • Single table: the mapping now lives in one place — shared::text::ligature_expansion / pub(crate) expand_ligatures — used by both clean_text and the PDF path (src/formats/pdf.rs), so the two can't drift.
  • Stable Rust: the fast path uses text.chars().any(|c| ligature_expansion(c).is_some()) (the Pattern for RangeInclusive<char> form never landed); verified with stable cargo 1.99.1.
  • PDF-path test: shared::text::tests::expand_ligatures_pdf_path covers 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.

…#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

@cubic-dev-ai cubic-dev-ai Bot 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.

All reported issues were addressed across 1 file

Reply with feedback, questions, or to request a fix.

Fix all with cubic | Re-trigger cubic

Comment thread src/shared/text.rs
Comment thread src/shared/text.rs Outdated
… 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.
@viwe-monai

Copy link
Copy Markdown
Author

Thanks for catching both issues! Fixed in 312b6c6:

P2 (PDF path bypasses clean_text): You are right — to_markdown_bytes routes Format::Pdf directly to formats::pdf::to_markdown, which returns pdf_inspector's markdown without any shared normalization. Added a dedicated expand_ligatures() in src/formats/pdf.rs and applied it to the extracted markdown before returning. The clean_text() expansion remains as defense-in-depth for the document-model path.

P3 (missing U+FB05/FB06 coverage): Added fa\u{fb05} → "fast" and ju\u{fb06} → "just" to ligatures_expanded.

@rahul05ranjan rahul05ranjan 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.

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:

  1. The ligature table now exists twice inside anydoc — once in pdf.rs::expand_ligatures, once in clean_text. Making expand_ligatures a pub(crate) helper in shared/text.rs that both call would keep the two tables from drifting (you'd still need the separate pdf.rs path, since clean_text collapses line breaks, which PDF markdown must keep).
  2. The PR description frames the change as PDF-specific ("the PDF extraction path bypasses clean_text"), but adding the arms to clean_text changes output for every format — a DOCX text run containing a literal fi now comes out as fi too. Probably the consistency you want, but the description should say so; also worth keeping in mind that pdf-inspector itself has an internal expand_ligatures with slightly different behavior (it also strips ZWNJ/ZWJ, which clean_text deliberately 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).
@viwe-monai

Copy link
Copy Markdown
Author

@rahul05ranjan Thanks for the careful read — all three points landed in the latest push (0686d87).

Compile error — confirmed and reproduced exactly as described: cargo check on stable (rustc 1.99.1) fails at src/formats/pdf.rs:18 with E0277: the trait bound std::ops::RangeInclusive<char>: Pattern is not satisfied. Adopted your chars().any(...) shape, with membership delegated to the table (below) so the fast path can't drift from it:

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: expand_ligatures is now pub(crate) in shared/text.rs, backed by a per-char ligature_expansion(c) -> Option<&'static str> that clean_text also uses, so there is a single table. pdf.rs keeps its own call site since clean_text collapses line breaks while PDF Markdown must keep them. Noted on pdf-inspector's variant stripping ZWNJ/ZWJ — anydoc-side behavior is unchanged there; ZWNJ/ZWJ are still preserved.

Description — the body now states that the clean_text change affects every format, not just PDF (a literal fi in a DOCX run now comes out as fi too), and points at the shared helper.

Also fixed while in here: ligatures_expanded's ffl case could never have passed as written (it was wrong from its introduction in 219f2b5) — "waf\u{fb04}e" expands to "wafffle" because U+FB04 is ffl; the ligature form of "waffle" is "wa\u{fb04}e". And per your note about the missing PDF-path coverage, expand_ligatures_pdf_path now covers all seven code points, line-break preservation, and the no-ligature fast path.

Verified locally on stable, mirroring CI: cargo fmt --all --check, cargo clippy --workspace --all-targets --all-features -- -D warnings, cargo clippy -p anydoc-wasm --target wasm32-unknown-unknown -- -D warnings, and cargo test --locked (288 lib tests + integration tests, all green).

@viwe-monai

Copy link
Copy Markdown
Author

Great catch on the Pattern issue — you were right that the fast path couldn't compile on stable. All points addressed in 0686d87:

  1. Stable Rust: the fast path is now !text.chars().any(|c| ligature_expansion(c).is_some()) — your suggested .chars().any(...) shape, keyed off the same table as the expansion so membership and mapping can't drift. Verified: cargo test green on stable cargo 1.99.1 (288 lib + 10 integration tests, 0 failed).
  2. Single table: ligature_expansion + pub(crate) fn expand_ligatures now live in shared/text.rs, called by both clean_text and the PDF path (src/formats/pdf.rs), so the two anydoc copies can't diverge. Kept the separate PDF entry point, as you said — clean_text collapses line breaks, PDF markdown must keep them.
  3. Description updated to say the change affects every format (a DOCX text run with a literal fi now comes out fi too), and notes that pdf-inspector's internal expand_ligatures is intentionally a different variant (it strips ZWNJ/ZWJ, which clean_text preserves).
  4. PDF-path test: shared::text::tests::expand_ligatures_pdf_path exercises the PDF entry point directly — all seven ligatures U+FB00–U+FB06, line breaks preserved, fast path on plain text.

Thanks for the detailed review — the std-source check on the Pattern impls saved this from landing broken.

This branch has not been deployed

No deployments
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.

Ligature characters (fi/fl/ffi) are dropped instead of expanded when extracting PDF text

2 participants