Repository: googlefonts/ufo2ft
Pull request #719 (2023-03-01 20:07:11)

Warn on questionable (dis)use of `# Automatic Code`
1. Warn about mis-cased `# Automatic Code` comments (see #701)
2. Warn if there is no insertion marker but kerning.plist data and a manual kern/dist feature in the fea code (because kerning.plist will be dropped)

anthrotype: @madig are you going to finish this? i'd like to cut a release soon-ish
madig: Yes, one day. Currently looking at something else but hopefully this week.

Patch:
diff --git a/Lib/ufo2ft/featureCompiler.py b/Lib/ufo2ft/featureCompiler.py
index ae35adc..964ee08 100644
--- a/Lib/ufo2ft/featureCompiler.py
+++ b/Lib/ufo2ft/featureCompiler.py
@@ -1,5 +1,8 @@
+from __future__ import annotations
+
 import logging
 import os
+import re
 from collections import OrderedDict
 from inspect import isclass
 from io import StringIO
@@ -21,6 +24,7 @@ from ufo2ft.featureWriters import (
     isValidFeatureWriter,
     loadFeatureWriters,
 )
+from ufo2ft.util import describe_ufo
 
 logger = logging.getLogger(__name__)
 timer = Timer(logging.getLogger("ufo2ft.timer"), level=logging.DEBUG)
@@ -281,6 +285,17 @@ class FeatureCompiler(BaseFeatureCompiler):
             if self.featureWriters:
                 featureFile = parseLayoutFeatures(self.ufo, self.feaIncludeDir)
 
+                # Insertion markers are only considered in "skip" mode.
+                if any(writer.mode == "skip" for writer in self.featureWriters):
+                    markers = {
+                        writer.insertFeatureMarker
+                        for writer in self.featureWriters
+                        if writer.insertFeatureMarker is not None
+                    }
+                    warn_about_miscased_insertion_markers(
+                        describe_ufo(self.ufo), featureFile, markers
+                    )
+
                 path = self.ufo.path
                 for writer in self.featureWriters:
                     try:
@@ -354,3 +369,33 @@ class MtiFeatureCompiler(BaseFeatureCompiler):
             table = mtiLib.build(features.splitlines(), self.ttFont)
             assert table.tableTag == tag
             self.ttFont[tag] = table
+
+
+def warn_about_miscased_insertion_markers(
+    ufo_description: str, feaFile: ast.FeatureFile, patterns: set[str]
+) -> None:
+    """Warn the user about potentially mistyped feature insertion markers."""
+
+    patterns_compiled = tuple(
+        (re.compile(pattern), re.compile(pattern, re.IGNORECASE))
+        for pattern in patterns
+    )
+
+    # NOTE: Insertion markers can only meaningfully occur in top-level feature
+    # blocks.
+    for block in ast.iterFeatureBlocks(feaFile):
+        for statement in block.statements:
+            if not isinstance(statement, ast.Comment):
+                continue
+            for pattern_case, pattern_ignore_case in patterns_compiled:
+                text = str(statement)
+                match_case = re.match(pattern_case, text)
+                match_ignore_case = re.match(pattern_ignore_case, text)
+                if match_ignore_case and not match_case:
+                    logger.warning(
+                        "%s: The insertion comment '%s' in the feature file is "
+                        "miscased (search pattern: %s), ignoring it.",
+                        ufo_description,
+                        text,
+                        pattern_case.pattern,
+                    )
diff --git a/Lib/ufo2ft/featureWriters/baseFeatureWriter.py b/Lib/ufo2ft/featureWriters/baseFeatureWriter.py
index ba79d38..4a2250a 100644
--- a/Lib/ufo2ft/featureWriters/baseFeatureWriter.py
+++ b/Lib/ufo2ft/featureWriters/baseFeatureWriter.py
@@ -80,11 +80,13 @@ class BaseFeatureWriter:
           instantiated from a FeatureCompiler);
         - a set of features (tags) to be generated. If self.mode is "skip",
           these are all the features which are _not_ already present.
+        - a set of all existing features (tags) in the feaFile.
 
         Returns the context namespace instance.
         """
         todo = set(self.features)
         insertComments = None
+        existing = set()
         if self.mode == "skip":
             if self.insertFeatureMarker is not None:
                 insertComments = self.collectInsertMarkers(
@@ -104,6 +106,7 @@ class BaseFeatureWriter:
             compiler=compiler,
             todo=todo,
             insertComments=insertComments,
+            existingFeatures=existing,
         )
 
         return self.context
@@ -234,21 +237,23 @@ class BaseFeatureWriter:
             statements.insert(index, feature)
             indices.append(index)
 
-        minindex = min(indices)
-        if lookups:
-            feaFile.statements = statements = (
-                statements[:minindex] + lookups + statements[minindex:]
-            )
-
-        # Write classDefs, anchorsDefs, markClassDefs, lookups at
-        # the very top of the feature file.
+        # Write classDefs, anchorsDefs, markClassDefs, lookups at earliest
+        # opportunity.
         others = []
+        minindex = min(indices)
         for defs in [classDefs, anchorDefs, markClassDefs]:
             if defs:
                 others.extend(defs)
                 others.append(ast.Comment(""))
-
-        feaFile.statements = statements = others + statements
+        # Insert lookups
+        if lookups:
+            if minindex > 0 and not others:
+                others.append(ast.Comment(""))
+            others.extend(lookups)
+        if others:
+            feaFile.statements = statements = (
+                statements[:minindex] + others + statements[minindex:]
+            )
 
     @staticmethod
     def collectInsertMarkers(feaFile, insertFeatureMarker, featureTags):
diff --git a/Lib/ufo2ft/featureWriters/kernFeatureWriter.py b/Lib/ufo2ft/featureWriters/kernFeatureWriter.py
index b4c42a0..af3b080 100644
--- a/Lib/ufo2ft/featureWriters/kernFeatureWriter.py
+++ b/Lib/ufo2ft/featureWriters/kernFeatureWriter.py
@@ -11,7 +11,13 @@ from fontTools.unicodedata import script_horizontal_direction
 
 from ufo2ft.constants import COMMON_SCRIPT, INDIC_SCRIPTS, USE_SCRIPTS
 from ufo2ft.featureWriters import BaseFeatureWriter, ast
-from ufo2ft.util import DFLT_SCRIPTS, classifyGlyphs, quantize, unicodeScriptExtensions
+from ufo2ft.util import (
+    DFLT_SCRIPTS,
+    classifyGlyphs,
+    describe_ufo,
+    quantize,
+    unicodeScriptExtensions,
+)
 
 LOGGER = logging.getLogger(__name__)
 
@@ -198,6 +204,23 @@ class KernFeatureWriter(BaseFeatureWriter):
         ctx.gdefClasses = self.getGDEFGlyphClasses()
         ctx.glyphSet = self.getOrderedGlyphSet()
 
+        # Unless we use the legacy append mode (which ignores insertion
+        # markers), if the font contains kerning and the feaFile contains `kern`
+        # or `dist` feature blocks, but we have no insertion markers (or they
+        # were misspelt and ignored), warn the user that the kerning blocks in
+        # the feaFile take precedence and other kerning is dropped.
+        if (
+            self.mode == "skip"
+            and font.kerning
+            and ctx.existingFeatures & self.features
+            and not ctx.insertComments
+        ):
+            LOGGER.warning(
+                "%s: font has kerning, but also manually written kerning features "
+                "without an insertion comment. Dropping the former.",
+                describe_ufo(font),
+            )
+
         # Remember which languages are defined for which OT tag, as all
         # generated kerning needs to be registered for the script's `dflt`
         # language, but also all those the designer defined manually. Otherwise,
diff --git a/Lib/ufo2ft/util.py b/Lib/ufo2ft/util.py
index c91f90e..56072a2 100644
--- a/Lib/ufo2ft/util.py
+++ b/Lib/ufo2ft/util.py
@@ -5,7 +5,7 @@ import logging
 import re
 from copy import deepcopy
 from inspect import currentframe, getfullargspec
-from typing import Mapping, Set
+from typing import Any, Mapping, Set
 
 from fontTools import subset, ttLib, unicodedata
 from fontTools.designspaceLib import DesignSpaceDocument
@@ -612,3 +612,16 @@ def unicodeScriptExtensions(
     defines "Hrkt" as an alias for both scripts.
     """
     return {aliases.get(s, s) for s in unicodedata.script_extension(chr(codepoint))}
+
+
+def describe_ufo(ufo: Any) -> str:
+    """Returns a description of a UFO suitable for logging."""
+    if (
+        hasattr(ufo, "reader")
+        and hasattr(ufo.reader, "fs")
+        and hasattr(ufo.reader.fs, "root_path")
+    ):
+        return ufo.reader.fs.root_path
+    elif ufo.info.familyName or ufo.info.styleName:
+        return " ".join(n for n in (ufo.info.familyName, ufo.info.styleName) if n)
+    return repr(ufo)


Test patch:
diff --git a/tests/featureCompiler_test.py b/tests/featureCompiler_test.py
index 8ed92a4..6d6bead 100644
--- a/tests/featureCompiler_test.py
+++ b/tests/featureCompiler_test.py
@@ -341,6 +341,7 @@ class FeatureCompilerTest:
                 sub f f by f_f;
             } liga;
 
+
             lookup kern_Default {
                 lookupflag IgnoreMarks;
                 pos a v -40;
diff --git a/tests/featureWriters/__init__.py b/tests/featureWriters/__init__.py
index 8783a2c..881bb1d 100644
--- a/tests/featureWriters/__init__.py
+++ b/tests/featureWriters/__init__.py
@@ -13,12 +13,8 @@ class FeatureWriterTest:
         """
         writer = cls.FeatureWriter(**kwargs)
         feaFile = parseLayoutFeatures(ufo)
-        old_statements = [st.asFea() for st in feaFile.statements]
-
+        n = len(feaFile.statements)
         if writer.write(ufo, feaFile):
             new = ast.FeatureFile()
-
-            for statement in feaFile.statements:
-                if statement.asFea() not in old_statements:
-                    new.statements.append(statement)
+            new.statements = feaFile.statements[n:]
             return new
diff --git a/tests/featureWriters/kernFeatureWriter2_test.py b/tests/featureWriters/kernFeatureWriter2_test.py
index 6230126..284745e 100644
--- a/tests/featureWriters/kernFeatureWriter2_test.py
+++ b/tests/featureWriters/kernFeatureWriter2_test.py
@@ -147,7 +147,7 @@ class KernFeatureWriterTest(FeatureWriterTest):
         # default is ignoreMarks=True
         feaFile = self.writeFeatures(font)
         assert str(feaFile) == dedent(
-            """\
+            """
             lookup kern_ltr {
                 lookupflag IgnoreMarks;
                 pos B C -30;
@@ -166,7 +166,7 @@ class KernFeatureWriterTest(FeatureWriterTest):
 
         feaFile = self.writeFeatures(font, ignoreMarks=False)
         assert str(feaFile) == dedent(
-            """\
+            """
             lookup kern_ltr {
                 pos A acutecomb -55;
                 pos B C -30;
@@ -198,7 +198,7 @@ class KernFeatureWriterTest(FeatureWriterTest):
         # default is ignoreMarks=True
         feaFile = self.writeFeatures(font)
         assert str(feaFile) == dedent(
-            """\
+            """
             lookup kern_ltr_marks {
                 pos A acutecomb -55;
             } kern_ltr_marks;
@@ -236,6 +236,7 @@ class KernFeatureWriterTest(FeatureWriterTest):
 
         expected = existing + dedent(
             """
+
             lookup kern_ltr {
                 lookupflag IgnoreMarks;
                 pos seven six 25;
@@ -300,7 +301,7 @@ class KernFeatureWriterTest(FeatureWriterTest):
         # test append mode ignores insert marker
         generated = self.writeFeatures(ufo, mode="append")
         assert str(generated) == dedent(
-            """\
+            """
             lookup kern_ltr {
                 lookupflag IgnoreMarks;
                 pos seven six 25;
@@ -383,6 +384,7 @@ class KernFeatureWriterTest(FeatureWriterTest):
                 #
             } kern;
 
+
             lookup kern_ltr {
                 lookupflag IgnoreMarks;
                 pos seven six 25;
@@ -399,7 +401,7 @@ class KernFeatureWriterTest(FeatureWriterTest):
         # test append mode ignores insert marker
         generated = self.writeFeatures(ufo, mode="append")
         assert str(generated) == dedent(
-            """\
+            """
             lookup kern_ltr {
                 lookupflag IgnoreMarks;
                 pos seven six 25;
@@ -442,7 +444,7 @@ class KernFeatureWriterTest(FeatureWriterTest):
         # test append mode ignores insert marker
         generated = self.writeFeatures(ufo, mode="append")
         assert str(generated) == dedent(
-            """\
+            """
             lookup kern_ltr {
                 lookupflag IgnoreMarks;
                 pos seven six 25;
@@ -474,7 +476,7 @@ class KernFeatureWriterTest(FeatureWriterTest):
         generated = self.writeFeatures(ufo)
 
         assert str(generated) == dedent(
-            """\
+            """
             lookup kern_rtl {
                 lookupflag IgnoreMarks;
                 pos four-ar seven-ar -30;
@@ -904,7 +906,7 @@ class KernFeatureWriterTest(FeatureWriterTest):
         generated = self.writeFeatures(ufo)
 
         assert str(generated) == dedent(
-            """\
+            """
             lookup kern_ltr {
                 lookupflag IgnoreMarks;
                 pos A V -40;
@@ -931,7 +933,7 @@ class KernFeatureWriterTest(FeatureWriterTest):
         generated = self.writeFeatures(ufo)
 
         assert str(generated) == dedent(
-            """\
+            """
             lookup kern_ltr {
                 lookupflag IgnoreMarks;
                 pos A V -40;
@@ -1023,7 +1025,7 @@ class KernFeatureWriterTest(FeatureWriterTest):
         generated = self.writeFeatures(ufo)
 
         assert str(generated) == dedent(
-            """\
+            """
             lookup kern_rtl {
                 lookupflag IgnoreMarks;
                 pos u10A1E u10A06 <117 0 117 0>;
@@ -1151,7 +1153,7 @@ class KernFeatureWriterTest(FeatureWriterTest):
         generated = self.writeFeatures(ufo)
 
         assert str(generated) == dedent(
-            """\
+            """
             lookup kern_dflt {
                 lookupflag IgnoreMarks;
                 pos seven four -25;
diff --git a/tests/featureWriters/kernFeatureWriter_test.py b/tests/featureWriters/kernFeatureWriter_test.py
index d99258b..b2fee84 100644
--- a/tests/featureWriters/kernFeatureWriter_test.py
+++ b/tests/featureWriters/kernFeatureWriter_test.py
@@ -7,7 +7,7 @@ from fontTools import unicodedata
 
 from ufo2ft.constants import UNICODE_SCRIPT_ALIASES
 from ufo2ft.errors import InvalidFeaturesData
-from ufo2ft.featureCompiler import parseLayoutFeatures
+from ufo2ft.featureCompiler import FeatureCompiler, parseLayoutFeatures
 from ufo2ft.featureWriters import KernFeatureWriter, ast
 from ufo2ft.util import DFLT_SCRIPTS, unicodeScriptExtensions
 
@@ -155,7 +155,7 @@ class KernFeatureWriterTest(FeatureWriterTest):
         # default is ignoreMarks=True
         feaFile = self.writeFeatures(font)
         assert str(feaFile) == dedent(
-            """\
+            """
             lookup kern_Latn {
                 lookupflag IgnoreMarks;
                 pos B C -30;
@@ -176,7 +176,7 @@ class KernFeatureWriterTest(FeatureWriterTest):
 
         feaFile = self.writeFeatures(font, ignoreMarks=False)
         assert str(feaFile) == dedent(
-            """\
+            """
             lookup kern_Latn {
                 pos A acutecomb -55;
                 pos B C -30;
@@ -210,7 +210,7 @@ class KernFeatureWriterTest(FeatureWriterTest):
         # default is ignoreMarks=True
         feaFile = self.writeFeatures(font)
         assert str(feaFile) == dedent(
-            """\
+            """
             lookup kern_Default_marks {
                 pos A acutecomb -55;
             } kern_Default_marks;
@@ -250,6 +250,7 @@ class KernFeatureWriterTest(FeatureWriterTest):
 
         expected = existing + dedent(
             """
+
             lookup kern_Default {
                 lookupflag IgnoreMarks;
                 pos seven six 25;
@@ -318,7 +319,7 @@ class KernFeatureWriterTest(FeatureWriterTest):
         # test append mode ignores insert marker
         generated = self.writeFeatures(ufo, mode="append")
         assert str(generated) == dedent(
-            """\
+            """
             lookup kern_Default {
                 lookupflag IgnoreMarks;
                 pos seven six 25;
@@ -332,6 +333,42 @@ class KernFeatureWriterTest(FeatureWriterTest):
             """
         )
 
+    def test_comment_wrong_case_or_missing(self, FontClass, caplog):
+        ufo = FontClass()
+        for name in ("a", "b"):
+            ufo.newGlyph(name)
+        ufo.kerning.update({("a", "b"): 25.0})
+        ufo.features.text = dedent(
+            """
+            feature kern {
+                # Automatic code
+            } kern;
+            """
+        ).strip()
+
+        with caplog.at_level(logging.WARNING):
+            compiler = FeatureCompiler(ufo, featureWriters=[KernFeatureWriter])
+            font = compiler.compile()
+
+        # We mis-cased the insertion marker above, so it's ignored and we end up
+        # with an empty `kern` block overriding the other kerning in the font
+        # source and therefore no `GPOS` table.
+        assert "miscased" in caplog.text
+        assert "Dropping the former" in caplog.text
+        assert "GPOS" not in font
+
+        # Append mode ignores insertion markers and so should not log warnings
+        # and have kerning in the final font.
+        caplog.clear()
+        with caplog.at_level(logging.WARNING):
+            compiler = FeatureCompiler(
+                ufo, featureWriters=[KernFeatureWriter(mode="append")]
+            )
+            font = compiler.compile()
+
+        assert not caplog.text
+        assert "GPOS" in font
+
     def test_insert_comment_before_extended(self, FontClass):
         ufo = FontClass()
         for name in ("one", "four", "six", "seven"):
@@ -405,6 +442,7 @@ class KernFeatureWriterTest(FeatureWriterTest):
                 #
             } kern;
 
+
             lookup kern_Default {
                 lookupflag IgnoreMarks;
                 pos seven six 25;
@@ -423,7 +461,7 @@ class KernFeatureWriterTest(FeatureWriterTest):
         # test append mode ignores insert marker
         generated = self.writeFeatures(ufo, mode="append")
         assert str(generated) == dedent(
-            """\
+            """
             lookup kern_Default {
                 lookupflag IgnoreMarks;
                 pos seven six 25;
@@ -468,7 +506,7 @@ class KernFeatureWriterTest(FeatureWriterTest):
         # test append mode ignores insert marker
         generated = self.writeFeatures(ufo, mode="append")
         assert str(generated) == dedent(
-            """\
+            """
             lookup kern_Default {
                 lookupflag IgnoreMarks;
                 pos seven six 25;
@@ -545,7 +583,7 @@ class KernFeatureWriterTest(FeatureWriterTest):
         generated = self.writeFeatures(ufo)
 
         assert dedent(str(generated)) == dedent(
-            """\
+            """
             lookup kern_Arab {
                 lookupflag IgnoreMarks;
                 pos four-ar seven-ar -30;
@@ -572,7 +610,7 @@ class KernFeatureWriterTest(FeatureWriterTest):
         generated = self.writeFeatures(ufo)
 
         assert dedent(str(generated)) == dedent(
-            """\
+            """
             lookup kern_Thaa {
                 lookupflag IgnoreMarks;
                 pos four-ar seven-ar -30;
@@ -1089,7 +1127,7 @@ class KernFeatureWriterTest(FeatureWriterTest):
         generated = self.writeFeatures(ufo)
 
         assert str(generated) == dedent(
-            """\
+            """
             lookup kern_Khar {
                 lookupflag IgnoreMarks;
                 pos u10A1E u10A06 <117 0 117 0>;
@@ -1209,7 +1247,7 @@ class KernFeatureWriterTest(FeatureWriterTest):
         generated = self.writeFeatures(ufo)
 
         assert dedent(str(generated)) == dedent(
-            """\
+            """
             lookup kern_Hebr {
                 lookupflag IgnoreMarks;
                 pos yod-hb bet-hb <-100 0 -100 0>;
@@ -2113,7 +2151,7 @@ def test_dflt_language(FontClass):
     newFeatures = KernFeatureWriterTest.writeFeatures(ufo)
 
     assert dedent(str(newFeatures)) == dedent(
-        """\
+        """
         lookup kern_Latn {
             lookupflag IgnoreMarks;
             pos a a 1;
diff --git a/tests/featureWriters/markFeatureWriter_test.py b/tests/featureWriters/markFeatureWriter_test.py
index 1dc9044..b698779 100644
--- a/tests/featureWriters/markFeatureWriter_test.py
+++ b/tests/featureWriters/markFeatureWriter_test.py
@@ -282,9 +282,9 @@ class MarkFeatureWriterTest(FeatureWriterTest):
 
         assert str(feaFile) == dedent(
             """\
+            markClass acutecomb <anchor 100 200> @MC_top;
             markClass tildecomb <anchor 100 200> @MC_top;
 
-            markClass acutecomb <anchor 100 200> @MC_top;
             feature mark {
                 lookup mark2base {
                     pos base a
@@ -377,8 +377,6 @@ class MarkFeatureWriterTest(FeatureWriterTest):
 
         assert str(feaFile) == dedent(
             """\
-            markClass tildecomb <anchor 100 200> @MC_top;
-
             markClass acutecomb <anchor 100 200> @MC_top;
             feature mark {
                 lookup mark1 {
@@ -390,6 +388,8 @@ class MarkFeatureWriterTest(FeatureWriterTest):
                 #
             } mark;
 
+            markClass tildecomb <anchor 100 200> @MC_top;
+
             feature mark {
                 lookup mark2base {
                     pos base a