Merge pull request #1271 from googlefonts/gsub-context-format-choice

[fea-rs] Handle edge case in chain context compilation
googlefonts · Feb 13, 2025 · bd5c030 · bd5c030
2 parents 51f1bd8 + 84240b9
commit bd5c030
Show file tree

Hide file tree

Showing 4 changed files with 111 additions and 8 deletions.
diff --git a/fea-rs/src/common.rs b/fea-rs/src/common.rs
@@ -88,6 +88,15 @@ impl GlyphOrClass {
         }
     }
 
+    /// If this is a glyph or a class with exactly one, return it.
+    pub(crate) fn single_glyph(&self) -> Option<GlyphId16> {
+        match self {
+            GlyphOrClass::Glyph(gid) => Some(*gid),
+            GlyphOrClass::Class(class) if class.len() == 1 => class.iter().next(),
+            _ => None,
+        }
+    }
+
     pub(crate) fn iter(&self) -> impl Iterator<Item = GlyphId16> + '_ {
         let mut idx = 0;
         std::iter::from_fn(move || {

diff --git a/fea-rs/src/compile/lookups/contextual.rs b/fea-rs/src/compile/lookups/contextual.rs
@@ -325,8 +325,10 @@ impl ContextBuilder {
         self.rules.iter().any(ContextRule::is_chain_rule)
     }
 
-    fn has_glyph_classes(&self) -> bool {
-        self.rules.iter().any(ContextRule::has_glyph_classes)
+    fn has_glyph_classes_with_more_than_one_glyph(&self) -> bool {
+        self.rules
+            .iter()
+            .any(ContextRule::has_glyph_classes_with_more_than_one_glyph)
     }
 
     /// If the input sequence can be represented as a class def, return it
@@ -345,13 +347,13 @@ impl ContextBuilder {
     }
 
     fn format_1_coverage(&self) -> Option<CoverageTableBuilder> {
-        if self.has_glyph_classes() {
+        if self.has_glyph_classes_with_more_than_one_glyph() {
             return None;
         }
         Some(
             self.rules
                 .iter()
-                .map(|rule| rule.first_input_sequence_item().to_glyph().unwrap())
+                .map(|rule| rule.first_input_sequence_item().single_glyph().unwrap())
                 .collect::<CoverageTableBuilder>(),
         )
     }
@@ -360,7 +362,7 @@ impl ContextBuilder {
         let coverage = self.format_1_coverage()?.build();
         let mut rule_sets = HashMap::<_, Vec<_>>::new();
         for rule in &self.rules {
-            let key = rule.first_input_sequence_item().to_glyph().unwrap();
+            let key = rule.first_input_sequence_item().single_glyph().unwrap();
             let seq_lookups = rule.lookup_records(in_gpos);
             let rule = write_layout::SequenceRule::new(
                 rule.context
@@ -404,12 +406,12 @@ impl ContextRule {
         !self.backtrack.is_empty() || !self.lookahead.is_empty()
     }
 
-    fn has_glyph_classes(&self) -> bool {
+    fn has_glyph_classes_with_more_than_one_glyph(&self) -> bool {
         self.backtrack
             .iter()
             .chain(self.lookahead.iter())
             .chain(self.context.iter().map(|(glyphs, _)| glyphs))
-            .any(|x| x.is_class())
+            .any(|x| x.len() > 1)
     }
 
     fn first_input_sequence_item(&self) -> &GlyphOrClass {
@@ -525,7 +527,7 @@ impl ChainContextBuilder {
 
         let mut rule_sets = HashMap::<_, Vec<_>>::new();
         for rule in &self.0.rules {
-            let key = rule.first_input_sequence_item().to_glyph().unwrap();
+            let key = rule.first_input_sequence_item().single_glyph().unwrap();
             let seq_lookups = rule.lookup_records(in_gpos);
             let rule = write_layout::ChainedSequenceRule::new(
                 rule.backtrack.iter().flat_map(|cls| cls.iter()).collect(),

diff --git a/fea-rs/test-data/compile-tests/mini-latin/good/chain_sub_format_choice.fea b/fea-rs/test-data/compile-tests/mini-latin/good/chain_sub_format_choice.fea
@@ -0,0 +1,13 @@
+
+feature ccmp {
+    @one = [a];
+    @two = [b];
+
+    # this should compile to format 1, because the class has only a single glyph
+    lookup test {
+        sub @one' @two by @two;
+        sub @two @one' by @two;
+    } test;
+
+} ccmp;
+
diff --git a/fea-rs/test-data/compile-tests/mini-latin/good/chain_sub_format_choice.ttx b/fea-rs/test-data/compile-tests/mini-latin/good/chain_sub_format_choice.ttx
@@ -0,0 +1,79 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<ttFont>
+
+  <GSUB>
+    <Version value="0x00010000"/>
+    <ScriptList>
+      <!-- ScriptCount=1 -->
+      <ScriptRecord index="0">
+        <ScriptTag value="DFLT"/>
+        <Script>
+          <DefaultLangSys>
+            <ReqFeatureIndex value="65535"/>
+            <!-- FeatureCount=1 -->
+            <FeatureIndex index="0" value="0"/>
+          </DefaultLangSys>
+          <!-- LangSysCount=0 -->
+        </Script>
+      </ScriptRecord>
+    </ScriptList>
+    <FeatureList>
+      <!-- FeatureCount=1 -->
+      <FeatureRecord index="0">
+        <FeatureTag value="ccmp"/>
+        <Feature>
+          <!-- LookupCount=1 -->
+          <LookupListIndex index="0" value="0"/>
+        </Feature>
+      </FeatureRecord>
+    </FeatureList>
+    <LookupList>
+      <!-- LookupCount=2 -->
+      <Lookup index="0">
+        <LookupType value="6"/>
+        <LookupFlag value="0"/>
+        <!-- SubTableCount=1 -->
+        <ChainContextSubst index="0" Format="1">
+          <Coverage>
+            <Glyph value="a"/>
+          </Coverage>
+          <!-- ChainSubRuleSetCount=1 -->
+          <ChainSubRuleSet index="0">
+            <!-- ChainSubRuleCount=2 -->
+            <ChainSubRule index="0">
+              <!-- BacktrackGlyphCount=0 -->
+              <!-- InputGlyphCount=1 -->
+              <!-- LookAheadGlyphCount=1 -->
+              <LookAhead index="0" value="b"/>
+              <!-- SubstCount=1 -->
+              <SubstLookupRecord index="0">
+                <SequenceIndex value="0"/>
+                <LookupListIndex value="1"/>
+              </SubstLookupRecord>
+            </ChainSubRule>
+            <ChainSubRule index="1">
+              <!-- BacktrackGlyphCount=1 -->
+              <Backtrack index="0" value="b"/>
+              <!-- InputGlyphCount=1 -->
+              <!-- LookAheadGlyphCount=0 -->
+              <!-- SubstCount=1 -->
+              <SubstLookupRecord index="0">
+                <SequenceIndex value="0"/>
+                <LookupListIndex value="1"/>
+              </SubstLookupRecord>
+            </ChainSubRule>
+          </ChainSubRuleSet>
+        </ChainContextSubst>
+      </Lookup>
+      <Lookup index="1">
+        <LookupType value="1"/>
+        <LookupFlag value="0"/>
+        <!-- SubTableCount=1 -->
+        <SingleSubst index="0">
+          <Substitution in="a" out="b"/>
+        </SingleSubst>
+      </Lookup>
+    </LookupList>
+  </GSUB>
+
+</ttFont>