Fix ligature anchor numbering when component lacks intermediate source#1889
Conversation
…t location
Reproduces the bug where a ligature composite (e.g. 'fi') has an intermediate
layer that its component ('i') lacks. The missing base_glyph_counts entry at
that location causes ligature anchor renumbering to be skipped, producing
inconsistent anchor names ('bottom' vs 'bottom_2') across locations, which
then crashes AnchorBuilder::build() with "no value at default location".
This test currently fails — the fix follows in the next commit.
When a ligature composite (e.g. 'fi') has an intermediate layer that its
component ('i') doesn't have, base_glyph_counts had no entry at that location.
This caused ligature anchor renumbering to be skipped (the count defaulted to 0),
producing inconsistent anchor names across locations and crashing
AnchorBuilder::build() with "no value at default location".
Fix: fall back to the component's default-location count, which is a structural
property consistent across locations. Also ensure both early-return paths (empty
glyphs and marks) populate base_glyph_counts so the invariant holds universally.
Fixes #1661
|
repro fails as expected on CI: https://github.com/googlefonts/fontc/actions/runs/21992260523/job/63542377632?pr=1889#step:5:246 The actual fix commit is incoming. I also verified RedditSansItalics, NotoSans, NotoSans-Italic no longer crash. |
| }) | ||
| .copied() | ||
| .unwrap_or(0); | ||
| .expect("base_glyph_counts should have a default-location entry for every component"); |
There was a problem hiding this comment.
I wonder if this could error instead of panic?
Or ... perhaps we might encapsulate the establishment of the data with the invariant and querying of it, have a struct that owns the data, builds the valid data, and exposes a query api that we call here?
(np if that doesn't work, not super familiar with this code)
There was a problem hiding this comment.
I don't think a Result would make sense in this case. The expect() at line 356 fires when looking up component.base in base_glyph_counts. For it to succeed, component.base must have an entry at either location or the default location.
Now:
depth_sorted_composite_glyphsreturns all glyphs (simple and composite) in dependency order: components before composites.- The outer loop iterates
todoin this depth order. For each glyph, ititerates all of that glyph's source locations. anchors_traversing_componentsis called for every (glyph, location) pair and always inserts intobase_glyph_countsbefore returning:
- Early return 1 (line 278-280): empty glyph -> inserts 0
- Early return 2 (line 284-287): mark with anchors -> inserts 0
- Normal return (line 451-454): inserts computed count
- Glyph invariants require a source at the default location (this is enforced at construction).
- Therefore, by the time we process a composite glyph and reach line 347, all its components have been fully processed at all their locations (including default). The
base_glyph_countsentry at the default location is guaranteed to exist.
The only way to break this would be to add a new early return to anchors_traversing_components without inserting into base_glyph_counts. Which is exactly what happened before this PR, but now all three exit points do insert.
The previous unwrap_or(0) was doing triple duty:
- Handling the legitimate early returns for marks and empty glyphs, that never inserted a 0 count
- Silently papering over any depth-sorting bugs (component processed after composite)
- Silently papering over missing default locations that violate Glyph invariants
Cases 2 and 3 would produce silently wrong output: i.e. anchors numbered as if every component has 0 base glyphs. The expect turns those into loud failures.
Reverting to unwrap_or(0) would re-introduce the silent wrong-output risk. I'd rather keep the loud failure for a situation I can't currently trigger. Returning a Result from anchors_traversing_components for a condition I cannot produce (and that would indicate a logic bug in our own code) doesn't seem like the right tool to me.
Fixes a crash introduced in #1887 where ligature composites (e.g.
fi.ss01) with intermediate/brace layers would fail with "no value at default location" when a component (e.g.i.ss01) didn't have a source at that intermediate location.This is because the
base_glyph_countswas only populated at locations where the component had explicit sources, and before #1887 we didn't support sparse component locations at all. At missing locations the count defaulted to 0, skipping ligature anchor renumbering e.g. givingbottominstead ofbottom_2.The inconsistent names across locations then crashed
AnchorBuilder::build()with "no value at default location".The fix is to fall back to the component's default-location
base_glyph_count(normally expected to be consistent across locations), and also to ensure that the early-return paths (for empty and mark glyphs) also populatebase_glyph_counts(0) so the invariant holds universally (and we panic loudly if it's ever violated).