Repository: fonttools/fonttools

Issue:
Empty `ConditionSet` can be omitted
[Per the spec](https://learn.microsoft.com/en-us/typography/opentype/spec/chapter2#:~:text=If%20the%20ConditionSet%20offset%20is%200%2C%20there%20is%20no%20condition%20set%20table.%20This%20is%20treated%20as%20the%20universal%20condition:%20all%20contexts%20are%20matched.), a `ConditionSet` offset of 0 is equivalent to an empty `ConditionSet`, and is more efficient.

Ideally then I think that empty `ConditionSet`s should be replaced with null offsets when writing.

Hints from issue comments:
@behdad: Empty condition set always matches, hence stopping the search. It cannot be dropped.@behdad: NULL offset is right. Just don't remove them.@cmyr: right, replace empty with null, which is equivalent.

Solution patch:
diff --git a/Lib/fontTools/feaLib/builder.py b/Lib/fontTools/feaLib/builder.py
index 1583f06d9..8747b1d48 100644
--- a/Lib/fontTools/feaLib/builder.py
+++ b/Lib/fontTools/feaLib/builder.py
@@ -1387,6 +1387,13 @@ class Builder(object):
         # 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
 
     # GSUB 5/6
@@ -1445,6 +1452,13 @@ class Builder(object):
             sub = self.get_chained_lookup_(location, LigatureSubstBuilder)
 
         for g in itertools.product(*glyphs):
+            existing = sub.ligatures.get(g, replacement)
+            if existing != replacement:
+                raise FeatureLibError(
+                    f"Conflicting ligature sub rules: '{g}' maps to '{existing}' and '{replacement}'",
+                    location,
+                )
+
             sub.ligatures[g] = replacement
 
         chain.rules.append(ChainContextualRule(prefix, glyphs, suffix, [sub]))
diff --git a/Lib/fontTools/pens/t2CharStringPen.py b/Lib/fontTools/pens/t2CharStringPen.py
index 41ab0f92f..6ee25ce66 100644
--- a/Lib/fontTools/pens/t2CharStringPen.py
+++ b/Lib/fontTools/pens/t2CharStringPen.py
@@ -1,10 +1,14 @@
 # Copyright (c) 2009 Type Supply LLC
 # Author: Tal Leming
 
-from fontTools.misc.roundTools import otRound, roundFunc
+from __future__ import annotations
+
+from typing import Any, List, Tuple
+
+from fontTools.cffLib.specializer import commandsToProgram, specializeCommands
 from fontTools.misc.psCharStrings import T2CharString
+from fontTools.misc.roundTools import otRound, roundFunc
 from fontTools.pens.basePen import BasePen
-from fontTools.cffLib.specializer import specializeCommands, commandsToProgram
 
 
 class T2CharStringPen(BasePen):
@@ -18,36 +22,52 @@ class T2CharStringPen(BasePen):
     which are close to their integral part within the tolerated range.
     """
 
-    def __init__(self, width, glyphSet, roundTolerance=0.5, CFF2=False):
+    def __init__(
+        self,
+        width: float,
+        glyphSet: dict[str, Any] | None,
+        roundTolerance: float = 0.5,
+        CFF2: bool = False,
+    ) -> None:
         super(T2CharStringPen, self).__init__(glyphSet)
         self.round = roundFunc(roundTolerance)
         self._CFF2 = CFF2
         self._width = width
-        self._commands = []
+        self._commands: List[Tuple[str | bytes, List[float]]] = []
         self._p0 = (0, 0)
 
-    def _p(self, pt):
+    def _p(self, pt: Tuple[float, float]) -> List[float]:
         p0 = self._p0
         pt = self._p0 = (self.round(pt[0]), self.round(pt[1]))
         return [pt[0] - p0[0], pt[1] - p0[1]]
 
-    def _moveTo(self, pt):
+    def _moveTo(self, pt: Tuple[float, float]) -> None:
         self._commands.append(("rmoveto", self._p(pt)))
 
-    def _lineTo(self, pt):
+    def _lineTo(self, pt: Tuple[float, float]) -> None:
         self._commands.append(("rlineto", self._p(pt)))
 
-    def _curveToOne(self, pt1, pt2, pt3):
+    def _curveToOne(
+        self,
+        pt1: Tuple[float, float],
+        pt2: Tuple[float, float],
+        pt3: Tuple[float, float],
+    ) -> None:
         _p = self._p
         self._commands.append(("rrcurveto", _p(pt1) + _p(pt2) + _p(pt3)))
 
-    def _closePath(self):
+    def _closePath(self) -> None:
         pass
 
-    def _endPath(self):
+    def _endPath(self) -> None:
         pass
 
-    def getCharString(self, private=None, globalSubrs=None, optimize=True):
+    def getCharString(
+        self,
+        private: dict | None = None,
+        globalSubrs: List | None = None,
+        optimize: bool = True,
+    ) -> T2CharString:
         commands = self._commands
         if optimize:
             maxstack = 48 if not self._CFF2 else 513
diff --git a/Lib/fontTools/varLib/featureVars.py b/Lib/fontTools/varLib/featureVars.py
index d1a3311d2..856f00bcb 100644
--- a/Lib/fontTools/varLib/featureVars.py
+++ b/Lib/fontTools/varLib/featureVars.py
@@ -600,9 +600,12 @@ def buildFeatureRecord(featureTag, lookupListIndices):
 def buildFeatureVariationRecord(conditionTable, substitutionRecords):
     """Build a FeatureVariationRecord."""
     fvr = ot.FeatureVariationRecord()
-    fvr.ConditionSet = ot.ConditionSet()
-    fvr.ConditionSet.ConditionTable = conditionTable
-    fvr.ConditionSet.ConditionCount = len(conditionTable)
+    if len(conditionTable) != 0:
+        fvr.ConditionSet = ot.ConditionSet()
+        fvr.ConditionSet.ConditionTable = conditionTable
+        fvr.ConditionSet.ConditionCount = len(conditionTable)
+    else:
+        fvr.ConditionSet = None
     fvr.FeatureTableSubstitution = ot.FeatureTableSubstitution()
     fvr.FeatureTableSubstitution.Version = 0x00010000
     fvr.FeatureTableSubstitution.SubstitutionRecord = substitutionRecords


Test patch:
diff --git a/Tests/feaLib/builder_test.py b/Tests/feaLib/builder_test.py
index d3dd28b74..d0436881b 100644
--- a/Tests/feaLib/builder_test.py
+++ b/Tests/feaLib/builder_test.py
@@ -1064,6 +1064,14 @@ class BuilderTest(unittest.TestCase):
             )
         captor.assertRegex("Already defined position for pair A V at")
 
+    def test_ligatureSubst_conflicting_rules(self):
+        self.assertRaisesRegex(
+            FeatureLibError,
+            "Conflicting ligature sub",
+            self.build,
+            "feature test {" "    sub a b by one;" "    sub a b by two;" "} test;",
+        )
+
     def test_ignore_empty_lookup_block(self):
         # https://github.com/fonttools/fonttools/pull/2277
         font = self.build(