Repository: fonttools/fonttools

Issue:
[feaLib] _upgrade_mixed_subst_statements does not account for all possible lookup breaks
Another little issue with #3805.

Consider the following FEA:


```fea
feature derp {
    sub x by y;
    lookupflag 4;
    sub f i by one;
} derp;
```

because the logic in `up` (https://github.com/fonttools/fonttools/blob/b21e538f2fb04f2397adde0c52fc7fd0d9e831c7/Lib/fontTools/feaLib/ast.py#L340) only considers the sub statements, it ends up turning the `sub x by y` rule into a ligature lookup, even though the subsequent lookupflag statement means it is not in the same lookup as the subsequent ligature sub rule.


The same miscompilation can also occur, for instance, with FEA like:

```fea
lookup ther {
sub l by j;
} ther ;

feature clig {
    sub a by b;
    sub e l l' lookup lookup_1;
    sub e l f by z;
} clig;
```

where the contextual rule in the middle forces a new lookup, and we end up giving the first rule the wrong lookup type.


(ultimately this is another one where the question is, 'do we care'?)

Discussion:
@khaledhosny: I think we need to move this logic further down into the builder. Last time I checked this seemed rather complicated.@khaledhosny: https://github.com/fonttools/fonttools/pull/3849

Solution patch:
diff --git a/Lib/fontTools/feaLib/ast.py b/Lib/fontTools/feaLib/ast.py
index 8479d7300..efcce8c68 100644
--- a/Lib/fontTools/feaLib/ast.py
+++ b/Lib/fontTools/feaLib/ast.py
@@ -337,76 +337,6 @@ class AnonymousBlock(Statement):
         return res
 
 
-def _upgrade_mixed_subst_statements(statements):
-    # https://github.com/fonttools/fonttools/issues/612
-    # A multiple substitution may have a single destination, in which case
-    # it will look just like a single substitution. So if there are both
-    # multiple and single substitutions, upgrade all the single ones to
-    # multiple substitutions. Similarly, a ligature substitution may have a
-    # single source glyph, so if there are both ligature and single
-    # substitutions, upgrade all the single ones to ligature substitutions.
-
-    has_single = False
-    has_multiple = False
-    has_ligature = False
-    for s in statements:
-        if isinstance(s, SingleSubstStatement):
-            has_single = not any([s.prefix, s.suffix, s.forceChain])
-        elif isinstance(s, MultipleSubstStatement):
-            has_multiple = not any([s.prefix, s.suffix, s.forceChain])
-        elif isinstance(s, LigatureSubstStatement):
-            has_ligature = not any([s.prefix, s.suffix, s.forceChain])
-
-    to_multiple = False
-    to_ligature = False
-
-    # If we have mixed single and multiple substitutions,
-    # upgrade all single substitutions to multiple substitutions.
-    if has_single and has_multiple and not has_ligature:
-        to_multiple = True
-
-    # If we have mixed single and ligature substitutions,
-    # upgrade all single substitutions to ligature substitutions.
-    elif has_single and has_ligature and not has_multiple:
-        to_ligature = True
-
-    if to_multiple or to_ligature:
-        ret = []
-        for s in statements:
-            if isinstance(s, SingleSubstStatement):
-                glyphs = s.glyphs[0].glyphSet()
-                replacements = s.replacements[0].glyphSet()
-                if len(replacements) == 1:
-                    replacements *= len(glyphs)
-                for glyph, replacement in zip(glyphs, replacements):
-                    if to_multiple:
-                        ret.append(
-                            MultipleSubstStatement(
-                                s.prefix,
-                                glyph,
-                                s.suffix,
-                                [replacement],
-                                s.forceChain,
-                                location=s.location,
-                            )
-                        )
-                    elif to_ligature:
-                        ret.append(
-                            LigatureSubstStatement(
-                                s.prefix,
-                                [GlyphName(glyph)],
-                                s.suffix,
-                                replacement,
-                                s.forceChain,
-                                location=s.location,
-                            )
-                        )
-            else:
-                ret.append(s)
-        return ret
-    return statements
-
-
 class Block(Statement):
     """A block of statements: feature, lookup, etc."""
 
@@ -418,8 +348,7 @@ class Block(Statement):
         """When handed a 'builder' object of comparable interface to
         :class:`fontTools.feaLib.builder`, walks the statements in this
         block, calling the builder callbacks."""
-        statements = _upgrade_mixed_subst_statements(self.statements)
-        for s in statements:
+        for s in self.statements:
             s.build(builder)
 
     def asFea(self, indent=""):
diff --git a/Lib/fontTools/feaLib/builder.py b/Lib/fontTools/feaLib/builder.py
index 8747b1d48..3563db6e3 100644
--- a/Lib/fontTools/feaLib/builder.py
+++ b/Lib/fontTools/feaLib/builder.py
@@ -29,6 +29,7 @@ from fontTools.otlLib.builder import (
     PairPosBuilder,
     SinglePosBuilder,
     ChainContextualRule,
+    AnySubstBuilder,
 )
 from fontTools.otlLib.error import OpenTypeLibError
 from fontTools.varLib.varStore import OnlineVarStoreBuilder
@@ -866,13 +867,22 @@ class Builder(object):
         for lookup in self.lookups_:
             if lookup.table != tag:
                 continue
-            lookup.lookup_index = len(lookups)
-            self.lookup_locations[tag][str(lookup.lookup_index)] = LookupDebugInfo(
-                location=str(lookup.location),
-                name=self.get_lookup_name_(lookup),
-                feature=None,
-            )
-            lookups.append(lookup)
+            name = self.get_lookup_name_(lookup)
+            resolved = lookup.promote_lookup_type(is_named_lookup=name is not None)
+            if resolved is None:
+                raise FeatureLibError(
+                    "Within a named lookup block, all rules must be of "
+                    "the same lookup type and flag",
+                    lookup.location,
+                )
+            for l in resolved:
+                lookup.lookup_index = len(lookups)
+                self.lookup_locations[tag][str(lookup.lookup_index)] = LookupDebugInfo(
+                    location=str(lookup.location),
+                    name=name,
+                    feature=None,
+                )
+                lookups.append(l)
         otLookups = []
         for l in lookups:
             try:
@@ -1294,6 +1304,24 @@ class Builder(object):
 
     # GSUB rules
 
+    def add_any_subst_(self, location, mapping):
+        lookup = self.get_lookup_(location, AnySubstBuilder)
+        for key, value in mapping.items():
+            if key in lookup.mapping:
+                if value == lookup.mapping[key]:
+                    log.info(
+                        'Removing duplicate substitution from "%s" to "%s" at %s',
+                        ", ".join(key),
+                        ", ".join(value),
+                        location,
+                    )
+                else:
+                    raise FeatureLibError(
+                        'Already defined substitution for "%s"' % ", ".join(key),
+                        location,
+                    )
+            lookup.mapping[key] = value
+
     # GSUB 1
     def add_single_subst(self, location, prefix, suffix, mapping, forceChain):
         if self.cur_feature_name_ == "aalt":
@@ -1305,24 +1333,11 @@ class Builder(object):
         if prefix or suffix or forceChain:
             self.add_single_subst_chained_(location, prefix, suffix, mapping)
             return
-        lookup = self.get_lookup_(location, SingleSubstBuilder)
-        for from_glyph, to_glyph in mapping.items():
-            if from_glyph in lookup.mapping:
-                if to_glyph == lookup.mapping[from_glyph]:
-                    log.info(
-                        "Removing duplicate single substitution from glyph"
-                        ' "%s" to "%s" at %s',
-                        from_glyph,
-                        to_glyph,
-                        location,
-                    )
-                else:
-                    raise FeatureLibError(
-                        'Already defined rule for replacing glyph "%s" by "%s"'
-                        % (from_glyph, lookup.mapping[from_glyph]),
-                        location,
-                    )
-            lookup.mapping[from_glyph] = to_glyph
+
+        self.add_any_subst_(
+            location,
+            {(key,): (value,) for key, value in mapping.items()},
+        )
 
     # GSUB 2
     def add_multiple_subst(
@@ -1331,21 +1346,10 @@ class Builder(object):
         if prefix or suffix or forceChain:
             self.add_multi_subst_chained_(location, prefix, glyph, suffix, replacements)
             return
-        lookup = self.get_lookup_(location, MultipleSubstBuilder)
-        if glyph in lookup.mapping:
-            if replacements == lookup.mapping[glyph]:
-                log.info(
-                    "Removing duplicate multiple substitution from glyph"
-                    ' "%s" to %s%s',
-                    glyph,
-                    replacements,
-                    f" at {location}" if location else "",
-                )
-            else:
-                raise FeatureLibError(
-                    'Already defined substitution for glyph "%s"' % glyph, location
-                )
-        lookup.mapping[glyph] = replacements
+        self.add_any_subst_(
+            location,
+            {(glyph,): tuple(replacements)},
+        )
 
     # GSUB 3
     def add_alternate_subst(self, location, prefix, glyph, suffix, replacement):
@@ -1375,9 +1379,6 @@ class Builder(object):
                 location, prefix, glyphs, suffix, replacement
             )
             return
-        else:
-            lookup = self.get_lookup_(location, LigatureSubstBuilder)
-
         if not all(glyphs):
             raise FeatureLibError("Empty glyph class in substitution", location)
 
@@ -1386,15 +1387,10 @@ class Builder(object):
         # substitutions to be specified on target sequences that contain
         # glyph classes, the implementation software will enumerate
         # all specific glyph sequences if glyph classes are detected"
-        for g in itertools.product(*glyphs):
-            existing = lookup.ligatures.get(g, replacement)
-            if existing != replacement:
-                raise FeatureLibError(
-                    f"Conflicting ligature sub rules: '{g}' maps to '{existing}' and '{replacement}'",
-                    location,
-                )
-
-            lookup.ligatures[g] = replacement
+        self.add_any_subst_(
+            location,
+            {g: (replacement,) for g in itertools.product(*glyphs)},
+        )
 
     # GSUB 5/6
     def add_chain_context_subst(self, location, prefix, glyphs, suffix, lookups):
diff --git a/Lib/fontTools/otlLib/builder.py b/Lib/fontTools/otlLib/builder.py
index dd95fb95e..da00e9c5e 100644
--- a/Lib/fontTools/otlLib/builder.py
+++ b/Lib/fontTools/otlLib/builder.py
@@ -170,6 +170,9 @@ class LookupBuilder(object):
             and self.extension == other.extension
         )
 
+    def promote_lookup_type(self, is_named_lookup):
+        return [self]
+
     def inferGlyphClasses(self):
         """Infers glyph glasses for the GDEF table, such as {"cedilla":3}."""
         return {}
@@ -1324,6 +1327,151 @@ class ReverseChainSingleSubstBuilder(LookupBuilder):
         pass
 
 
+class AnySubstBuilder(LookupBuilder):
+    """A temporary builder for Single, Multiple, or Ligature substitution lookup.
+
+    Users are expected to manually add substitutions to the ``mapping``
+    attribute after the object has been initialized, e.g.::
+
+        # sub x by y;
+        builder.mapping[("x",)] = ("y",)
+        # sub a by b c;
+        builder.mapping[("a",)] = ("b", "c")
+        # sub f i by f_i;
+        builder.mapping[("f", "i")] = ("f_i",)
+
+    Then call `promote_lookup_type()` to convert this builder into the
+    appropriate type of substitution lookup builder. This would promote single
+    substitutions to either multiple or ligature substitutions, depending on the
+    rest of the rules in the mapping.
+
+    Attributes:
+        font (``fontTools.TTLib.TTFont``): A font object.
+        location: A string or tuple representing the location in the original
+            source which produced this lookup.
+        mapping: An ordered dictionary mapping a tuple of glyph names to another
+            tuple of glyph names.
+        lookupflag (int): The lookup's flag
+        markFilterSet: Either ``None`` if no mark filtering set is used, or
+            an integer representing the filtering set to be used for this
+            lookup. If a mark filtering set is provided,
+            `LOOKUP_FLAG_USE_MARK_FILTERING_SET` will be set on the lookup's
+            flags.
+    """
+
+    def __init__(self, font, location):
+        LookupBuilder.__init__(self, font, location, "GSUB", 0)
+        self.mapping = OrderedDict()
+
+    def _add_to_single_subst(self, builder, key, value):
+        if key[0] != self.SUBTABLE_BREAK_:
+            key = key[0]
+        builder.mapping[key] = value[0]
+
+    def _add_to_multiple_subst(self, builder, key, value):
+        if key[0] != self.SUBTABLE_BREAK_:
+            key = key[0]
+        builder.mapping[key] = value
+
+    def _add_to_ligature_subst(self, builder, key, value):
+        builder.ligatures[key] = value[0]
+
+    def promote_lookup_type(self, is_named_lookup):
+        # https://github.com/fonttools/fonttools/issues/612
+        # A multiple substitution may have a single destination, in which case
+        # it will look just like a single substitution. So if there are both
+        # multiple and single substitutions, upgrade all the single ones to
+        # multiple substitutions. Similarly, a ligature substitution may have a
+        # single source glyph, so if there are both ligature and single
+        # substitutions, upgrade all the single ones to ligature substitutions.
+        builder_classes = []
+        for key, value in self.mapping.items():
+            if key[0] == self.SUBTABLE_BREAK_:
+                builder_classes.append(None)
+            elif len(key) == 1 and len(value) == 1:
+                builder_classes.append(SingleSubstBuilder)
+            elif len(key) == 1 and len(value) != 1:
+                builder_classes.append(MultipleSubstBuilder)
+            elif len(key) > 1 and len(value) == 1:
+                builder_classes.append(LigatureSubstBuilder)
+            else:
+                assert False, "Should not happen"
+
+        has_multiple = any(b is MultipleSubstBuilder for b in builder_classes)
+        has_ligature = any(b is LigatureSubstBuilder for b in builder_classes)
+
+        # If we have mixed single and multiple substitutions,
+        # upgrade all single substitutions to multiple substitutions.
+        to_multiple = has_multiple and not has_ligature
+
+        # If we have mixed single and ligature substitutions,
+        # upgrade all single substitutions to ligature substitutions.
+        to_ligature = has_ligature and not has_multiple
+
+        # If we have only single substitutions, we can keep them as is.
+        to_single = not has_ligature and not has_multiple
+
+        ret = []
+        if to_single:
+            builder = SingleSubstBuilder(self.font, self.location)
+            for key, value in self.mapping.items():
+                self._add_to_single_subst(builder, key, value)
+            ret = [builder]
+        elif to_multiple:
+            builder = MultipleSubstBuilder(self.font, self.location)
+            for key, value in self.mapping.items():
+                self._add_to_multiple_subst(builder, key, value)
+            ret = [builder]
+        elif to_ligature:
+            builder = LigatureSubstBuilder(self.font, self.location)
+            for key, value in self.mapping.items():
+                self._add_to_ligature_subst(builder, key, value)
+            ret = [builder]
+        elif is_named_lookup:
+            # This is a named lookup with mixed substitutions that can’t be promoted,
+            # since we can’t split it into multiple lookups, we return None here to
+            # signal that to the caller
+            return None
+        else:
+            curr_builder = None
+            for builder_class, (key, value) in zip(
+                builder_classes, self.mapping.items()
+            ):
+                if curr_builder is None or type(curr_builder) is not builder_class:
+                    curr_builder = builder_class(self.font, self.location)
+                    ret.append(curr_builder)
+                if builder_class is SingleSubstBuilder:
+                    self._add_to_single_subst(curr_builder, key, value)
+                elif builder_class is MultipleSubstBuilder:
+                    self._add_to_multiple_subst(curr_builder, key, value)
+                elif builder_class is LigatureSubstBuilder:
+                    self._add_to_ligature_subst(curr_builder, key, value)
+                else:
+                    assert False, "Should not happen"
+
+        for builder in ret:
+            builder.extension = self.extension
+            builder.lookupflag = self.lookupflag
+            builder.markFilterSet = self.markFilterSet
+        return ret
+
+    def equals(self, other):
+        return LookupBuilder.equals(self, other) and self.mapping == other.mapping
+
+    def build(self):
+        assert False
+
+    def getAlternateGlyphs(self):
+        return {
+            key[0]: value
+            for key, value in self.mapping.items()
+            if len(key) == 1 and len(value) == 1
+        }
+
+    def add_subtable_break(self, location):
+        self.mapping[(self.SUBTABLE_BREAK_, location)] = self.SUBTABLE_BREAK_
+
+
 class SingleSubstBuilder(LookupBuilder):
     """Builds a Single Substitution (GSUB1) lookup.
 


Test patch:
diff --git a/Tests/feaLib/builder_test.py b/Tests/feaLib/builder_test.py
index d0436881b..e2640d280 100644
--- a/Tests/feaLib/builder_test.py
+++ b/Tests/feaLib/builder_test.py
@@ -94,6 +94,7 @@ class BuilderTest(unittest.TestCase):
         single_pos_NULL
         class_pair_pos_duplicates
         useExtension
+        bug3846_1 bug3846_2
     """.split()
 
     VARFONT_AXES = [
@@ -276,14 +277,12 @@ class BuilderTest(unittest.TestCase):
                 "    sub A by A.sc;"
                 "} test;"
             )
-        captor.assertRegex(
-            'Removing duplicate single substitution from glyph "A" to "A.sc"'
-        )
+        captor.assertRegex('Removing duplicate substitution from "A" to "A.sc"')
 
     def test_multipleSubst_multipleSubstitutionsForSameGlyph(self):
         self.assertRaisesRegex(
             FeatureLibError,
-            'Already defined substitution for glyph "f_f_i"',
+            'Already defined substitution for "f_f_i"',
             self.build,
             "feature test {"
             "    sub f_f_i by f f i;"
@@ -302,9 +301,7 @@ class BuilderTest(unittest.TestCase):
                 "    sub f_f_i by f f i;"
                 "} test;"
             )
-        captor.assertRegex(
-            r"Removing duplicate multiple substitution from glyph \"f_f_i\" to \('f', 'f', 'i'\)"
-        )
+        captor.assertRegex('Removing duplicate substitution from "f_f_i" to "f, f, i"')
 
     def test_mixed_singleSubst_multipleSubst(self):
         font = self.build(
@@ -413,7 +410,7 @@ class BuilderTest(unittest.TestCase):
         self.assertEqual(st.LookupType, 1)
         self.assertEqual(st.mapping, {"A": "A.sc"})
 
-    def test_mixed_singleSubst_multipleSubst_ligatureSubst(self):
+    def test_mixed_singleSubst_multipleSubst_ligatureSubst_named_lookup(self):
         self.assertRaisesRegex(
             FeatureLibError,
             "Within a named lookup block, all rules must be of the "
@@ -426,6 +423,34 @@ class BuilderTest(unittest.TestCase):
             "} test;",
         )
 
+    def test_mixed_singleSubst_multipleSubst_ligatureSubst_feature(self):
+        font = self.build(
+            dedent(
+                """
+                feature test {
+                  sub A     by A.sc;
+                  sub f_f   by f f;
+                  sub f f i by f_f_i;
+                } test;
+                """
+            )
+        )
+
+        assert "GSUB" in font
+        lookups = font["GSUB"].table.LookupList.Lookup
+        self.assertEqual(len(lookups), 3)
+        st = lookups[0].SubTable[0]
+        self.assertEqual(st.LookupType, 1)
+        self.assertEqual(st.mapping, {"A": "A.sc"})
+        st = lookups[1].SubTable[0]
+        self.assertEqual(st.LookupType, 2)
+        self.assertEqual(st.mapping, {"f_f": ("f", "f")})
+        st = lookups[2].SubTable[0]
+        self.assertEqual(st.LookupType, 4)
+        self.assertEqual(len(st.ligatures), 1)
+        self.assertEqual(len(st.ligatures["f"]), 1)
+        self.assertEqual(st.ligatures["f"][0].LigGlyph, "f_f_i")
+
     def test_pairPos_redefinition_warning(self):
         # https://github.com/fonttools/fonttools/issues/1147
         logger = logging.getLogger("fontTools.otlLib.builder")
@@ -454,7 +479,7 @@ class BuilderTest(unittest.TestCase):
     def test_singleSubst_multipleSubstitutionsForSameGlyph(self):
         self.assertRaisesRegex(
             FeatureLibError,
-            'Already defined rule for replacing glyph "e" by "E.sc"',
+            'Already defined substitution for "e"',
             self.build,
             "feature test {"
             "    sub [a-z] by [A.sc-Z.sc];"
@@ -1067,7 +1092,7 @@ class BuilderTest(unittest.TestCase):
     def test_ligatureSubst_conflicting_rules(self):
         self.assertRaisesRegex(
             FeatureLibError,
-            "Conflicting ligature sub",
+            'Already defined substitution for "a, b"',
             self.build,
             "feature test {" "    sub a b by one;" "    sub a b by two;" "} test;",
         )
diff --git a/Tests/otlLib/maxContextCalc_test.py b/Tests/otlLib/maxContextCalc_test.py
index a94ba246e..6d2a73655 100644
--- a/Tests/otlLib/maxContextCalc_test.py
+++ b/Tests/otlLib/maxContextCalc_test.py
@@ -30,7 +30,6 @@ def test_max_ctx_calc_features():
         sub A B C by c;
         sub [A B] C by c;
         sub [A B] C [A B] by c;
-        sub A by A B;
         sub A' C by A B;
         sub a' by b;
         sub a' b by c;