Repository: googlefonts/ufo2ft (python)

Issue:
_featuresCompatible() check in featureCompiler.py is insufficient
_featuresCompatible() attempts to compare feature files after stripping comments and extraneous whitespace, returning true if only the master has content or if all files are the same. 

However, none of the steps that process the feature files expands any `include` statements. This will lead to false positives in many projects. 

One easy fix would be to check for includes in the case where there are multiple feature files and just return `False`. 

This is the underlying cause of #915, which can be closed in favor of this issue.

Pull request description:
featureCompiler: use IncludingLexer to check if _featuresCompatible
Fixes #928 #915

This seems to work with the test font provided in #915. I probably need to add more unit tests.

Patch:
diff --git a/Lib/ufo2ft/_compilers/baseCompiler.py b/Lib/ufo2ft/_compilers/baseCompiler.py
index c2adfec..fbc74a0 100644
--- a/Lib/ufo2ft/_compilers/baseCompiler.py
+++ b/Lib/ufo2ft/_compilers/baseCompiler.py
@@ -327,7 +327,7 @@ class BaseInterpolatableCompiler(BaseCompiler):
         # If the feature files are compatible between the sources, we can save
         # time by building a variable feature file right at the end.
         can_optimize_features = self.variableFeatures and all(
-            _featuresCompatible(doc) for doc in interpolableSubDocs
+            _featuresCompatible(doc, self.feaIncludeDir) for doc in interpolableSubDocs
         )
         if can_optimize_features:
             self.logger.info("Features are compatible across masters; building later")
diff --git a/Lib/ufo2ft/featureCompiler.py b/Lib/ufo2ft/featureCompiler.py
index f1d96ff..a18115b 100644
--- a/Lib/ufo2ft/featureCompiler.py
+++ b/Lib/ufo2ft/featureCompiler.py
@@ -9,9 +9,10 @@ from io import StringIO
 from tempfile import NamedTemporaryFile
 
 from fontTools import mtiLib
-from fontTools.designspaceLib import DesignSpaceDocument, SourceDescriptor
+from fontTools.designspaceLib import DesignSpaceDocument
 from fontTools.feaLib.builder import addOpenTypeFeaturesFromString
 from fontTools.feaLib.error import FeatureLibError, IncludedFeaNotFound
+from fontTools.feaLib.lexer import IncludingLexer, Lexer
 from fontTools.feaLib.parser import Parser
 from fontTools.misc.loggingTools import Timer
 
@@ -70,6 +71,41 @@ def parseLayoutFeatures(font, includeDir=None):
     return doc
 
 
+def tokenizeLayoutFeatures(
+    font, includeDir: str | None = None
+) -> list[tuple[str, str]]:
+    """Tokenize the font features.fea with a lexer that follows include statements.
+
+    Comments, newlines and whitespace characters are excluded.
+
+    Return a list of (type, token) tuples.
+    """
+    featxt = font.features.text or ""
+    if not featxt:
+        return []
+    buf = StringIO(featxt)
+    ufoPath = font.path
+    if includeDir is None and ufoPath is not None:
+        ufoPath = os.path.normpath(ufoPath)
+        buf.name = os.path.join(ufoPath, "features.fea")
+        includeDir = os.path.dirname(ufoPath) or "."
+    try:
+        return [
+            (typ, tok)
+            for typ, tok, _ in IncludingLexer(buf, includeDir=includeDir)
+            if typ != Lexer.COMMENT
+        ]
+    except IncludedFeaNotFound as e:
+        if ufoPath and os.path.exists(os.path.join(ufoPath, e.args[0])):
+            logger.warning(
+                "Please change the file name in the include(...); "
+                "statement to be relative to the UFO itself, "
+                "instead of relative to the 'features.fea' file "
+                "contained in it."
+            )
+        raise
+
+
 class BaseFeatureCompiler:
     """Base class for generating OpenType features and compiling OpenType
     layout tables from these.
@@ -222,8 +258,7 @@ class FeatureCompiler(BaseFeatureCompiler):
             import warnings
 
             warnings.warn(
-                "mtiFeatures argument is ignored; "
-                "you should use MtiLibFeatureCompiler",
+                "mtiFeatures argument is ignored; you should use MtiLibFeatureCompiler",
                 category=UserWarning,
                 stacklevel=2,
             )
@@ -445,7 +480,9 @@ class VariableFeatureCompiler(FeatureCompiler):
             self.features = self.ufo.features.text or ""
 
 
-def _featuresCompatible(designSpaceDoc: DesignSpaceDocument) -> bool:
+def _featuresCompatible(
+    designSpaceDoc: DesignSpaceDocument, includeDir: str | None = None
+) -> bool:
     """Returns True when the features of the individual source UFOs are the same,
     or when only the default source has features.
 
@@ -457,20 +494,11 @@ def _featuresCompatible(designSpaceDoc: DesignSpaceDocument) -> bool:
 
     assert all(hasattr(source.font, "features") for source in designSpaceDoc.sources)
 
-    def transform(f: SourceDescriptor) -> str:
-        # Strip comments
-        text = re.sub("(?m)#.*$", "", f.font.features.text or "")
-        # Strip extraneous whitespace
-        text = re.sub(r"\s+", " ", text)
-        return text
-
     sources = sorted(
         designSpaceDoc.sources, key=lambda source: source != designSpaceDoc.default
     )
     assert sources[0] == designSpaceDoc.default
 
-    transformed = [transform(s) for s in sources]
-    first = transformed[0]
-    return all(s == first for s in transformed[1:]) or all(
-        not s for s in transformed[1:]
-    )
+    tokenized = [tokenizeLayoutFeatures(s.font, includeDir) for s in sources]
+    first = tokenized[0]
+    return all(s == first for s in tokenized[1:]) or all(not s for s in tokenized[1:])


Test patch:
diff --git a/tests/featureCompiler_test.py b/tests/featureCompiler_test.py
index aaf3fb1..1d36043 100644
--- a/tests/featureCompiler_test.py
+++ b/tests/featureCompiler_test.py
@@ -7,7 +7,12 @@ import pytest
 from fontTools import ttLib
 from fontTools.feaLib.error import FeatureLibError, IncludedFeaNotFound
 
-from ufo2ft.featureCompiler import FeatureCompiler, logger, parseLayoutFeatures
+from ufo2ft.featureCompiler import (
+    FeatureCompiler,
+    logger,
+    parseLayoutFeatures,
+    tokenizeLayoutFeatures,
+)
 from ufo2ft.featureWriters import (
     FEATURE_WRITERS_KEY,
     BaseFeatureWriter,
@@ -113,6 +118,148 @@ class ParseLayoutFeaturesTest:
         assert "# hello world" in str(fea)
 
 
+class TokenizeLayoutFeaturesTest:
+    def tokenize(self, ufo, includeDir=None):
+        return [tok for _, tok in tokenizeLayoutFeatures(ufo, includeDir=includeDir)]
+
+    def test_include(self, FontClass, tmp_path):
+        (tmp_path / "test.fea").write_text(
+            dedent(
+                """\
+            # hello world
+            feature kern {
+                pos A V -40;
+            } kern;
+            """
+            ),
+            encoding="utf-8",
+        )
+        ufo = FontClass()
+        ufo.features.text = dedent(
+            """\
+            include(test.fea)
+            """
+        )
+        ufo.save(tmp_path / "Test.ufo")
+
+        assert self.tokenize(ufo) == [
+            "feature",
+            "kern",
+            "{",
+            "pos",
+            "A",
+            "V",
+            -40,
+            ";",
+            "}",
+            "kern",
+            ";",
+        ]
+
+    def test_include_does_not_exist(self, FontClass, tmp_path, monkeypatch):
+        ufo = FontClass()
+        ufo.features.text = dedent(
+            """\
+            include(test.fea)
+            """
+        )
+        with monkeypatch.context() as context:
+            context.chdir(tmp_path)
+            ufo.save("Test.ufo")
+            with pytest.raises(IncludedFeaNotFound):
+                tokenizeLayoutFeatures(ufo)
+
+    def test_include_no_ufo_path_nor_include_dir(self, FontClass, tmp_path):
+        ufo = FontClass()
+        ufo.features.text = dedent(
+            """\
+            include(test.fea)
+            """
+        )
+        assert ufo.path is None
+        with pytest.raises(IncludedFeaNotFound):
+            tokenizeLayoutFeatures(ufo, includeDir=None)
+
+    def test_include_not_found_with_warning(self, FontClass, tmp_path, caplog):
+        caplog.set_level(logging.ERROR)
+
+        (tmp_path / "test.fea").write_text(
+            dedent(
+                """\
+            # hello world
+            """
+            ),
+            encoding="utf-8",
+        )
+        ufo = FontClass()
+        ufo.features.text = dedent(
+            """\
+            include(../test.fea)
+            """
+        )
+        ufo.save(tmp_path / "Test.ufo")
+
+        # the included 'test.fea' is in the same directory as the ufo, but the include
+        # statement appears to be relative to the inner ufo/features.fea, treating ufo as
+        # if it were a directory. The spec wants include paths to be relative to the ufo
+        # itself as a whole, hence we expect the include error plus a warning
+        with caplog.at_level(logging.WARNING, logger=logger.name):
+            with pytest.raises(IncludedFeaNotFound):
+                tokenizeLayoutFeatures(ufo)
+
+        assert len(caplog.records) == 1
+        assert "change the file name in the include" in caplog.text
+
+    def test_include_dir(self, FontClass, tmp_path, caplog):
+        features_dir = tmp_path / "features"
+        features_dir.mkdir()
+        (features_dir / "test.fea").write_text(
+            dedent(
+                """\
+                # hello world
+                feature kern {
+                    pos A V -40;
+                } kern;
+                """
+            ),
+            encoding="utf-8",
+        )
+        ufo = FontClass()
+        ufo.features.text = dedent(
+            """\
+            include(test.fea)
+            """
+        )
+        ufo.save(tmp_path / "Test.ufo")
+
+        assert self.tokenize(ufo, includeDir=features_dir) == [
+            "feature",
+            "kern",
+            "{",
+            "pos",
+            "A",
+            "V",
+            -40,
+            ";",
+            "}",
+            "kern",
+            ";",
+        ]
+
+    def test_include_dir_defaults_to_cwd(self, FontClass, tmp_path, monkeypatch):
+        (tmp_path / "test.fea").write_text("# hello world", encoding="utf-8")
+        ufo = FontClass()
+        ufo.features.text = "include(test.fea)"
+
+        with monkeypatch.context() as context:
+            context.chdir(tmp_path)
+            # even without ufo.path or an explicit includeDir, we expect no exceptions
+            # because we cd to test.fea's directory.
+            # (result is empty because comments are ignored)
+            assert ufo.path is None
+            assert self.tokenize(ufo) == []
+
+
 class DummyFeatureWriter:
     tableTag = "GPOS"