[feaLib] Fix handled of "ignore" with unmarked glyphs

Fixes https://github.com/fonttools/fonttools/issues/2949

- If there are no marked glyphs in an "ignore" statement, issue a
  warning. The spec disallows this but makeotf allows it. It is most
likely a typo, so a warning is warranted.
- Mark the first glyph not all the glyphs, this matches makeotf.
- In asFea() always mark the input glyph.
- Udpate the tests.

Lib/fontTools/feaLib/ast.py

@@ -912,14 +912,11 @@ class IgnoreSubstStatement(Statement):
         contexts = []
         for prefix, glyphs, suffix in self.chainContexts:
             res = ""
-            if len(prefix) or len(suffix):
+            if len(prefix):
-                if len(prefix):
+                res += " ".join(map(asFea, prefix)) + " "
-                    res += " ".join(map(asFea, prefix)) + " "
+            res += " ".join(g.asFea() + "'" for g in glyphs)
-                res += " ".join(g.asFea() + "'" for g in glyphs)
+            if len(suffix):
-                if len(suffix):
+                res += " " + " ".join(map(asFea, suffix))
-                    res += " " + " ".join(map(asFea, suffix))
-            else:
-                res += " ".join(map(asFea, glyphs))
             contexts.append(res)
         return "ignore sub " + ", ".join(contexts) + ";"
 

Lib/fontTools/feaLib/parser.py

@@ -524,27 +524,33 @@ class Parser(object):
                 )
             return (prefix, glyphs, lookups, values, suffix, hasMarks)
 
-    def parse_chain_context_(self):
+    def parse_ignore_glyph_pattern_(self, sub):
         location = self.cur_token_location_
         prefix, glyphs, lookups, values, suffix, hasMarks = self.parse_glyph_pattern_(
             vertical=False
         )
-        chainContext = [(prefix, glyphs, suffix)]
+        if any(lookups):
-        hasLookups = any(lookups)
+            raise FeatureLibError(
+                f'No lookups can be specified for "ignore {sub}"', location
+            )
+        if not hasMarks:
+            error = FeatureLibError(
+                f'Ambiguous "ignore {sub}", there should be least one marked glyph',
+                location,
+            )
+            log.warning(str(error))
+            suffix, glyphs = glyphs[1:], glyphs[0:1]
+        chainContext = (prefix, glyphs, suffix)
+        return chainContext
+
+    def parse_ignore_context_(self, sub):
+        location = self.cur_token_location_
+        chainContext = [self.parse_ignore_glyph_pattern_(sub)]
         while self.next_token_ == ",":
             self.expect_symbol_(",")
-            (
+            chainContext.append(self.parse_ignore_glyph_pattern_(sub))
-                prefix,
-                glyphs,
-                lookups,
-                values,
-                suffix,
-                hasMarks,
-            ) = self.parse_glyph_pattern_(vertical=False)
-            chainContext.append((prefix, glyphs, suffix))
-            hasLookups = hasLookups or any(lookups)
         self.expect_symbol_(";")
-        return chainContext, hasLookups
+        return chainContext
 
     def parse_ignore_(self):
         # Parses an ignore sub/pos rule.
@@ -552,18 +558,10 @@ class Parser(object):
         location = self.cur_token_location_
         self.advance_lexer_()
         if self.cur_token_ in ["substitute", "sub"]:
-            chainContext, hasLookups = self.parse_chain_context_()
+            chainContext = self.parse_ignore_context_("sub")
-            if hasLookups:
-                raise FeatureLibError(
-                    'No lookups can be specified for "ignore sub"', location
-                )
             return self.ast.IgnoreSubstStatement(chainContext, location=location)
         if self.cur_token_ in ["position", "pos"]:
-            chainContext, hasLookups = self.parse_chain_context_()
+            chainContext = self.parse_ignore_context_("pos")
-            if hasLookups:
-                raise FeatureLibError(
-                    'No lookups can be specified for "ignore pos"', location
-                )
             return self.ast.IgnorePosStatement(chainContext, location=location)
         raise FeatureLibError(
             'Expected "substitute" or "position"', self.cur_token_location_

Tests/feaLib/builder_test.py

@@ -963,6 +963,18 @@ class BuilderTest(unittest.TestCase):
             "feature test { sub a by []; test};",
         )
 
+    def test_unmarked_ignore_statement(self):
+        name = "bug2949"
+        logger = logging.getLogger("fontTools.feaLib.parser")
+        with CapturingLogHandler(logger, level="WARNING") as captor:
+            self.check_feature_file(name)
+        self.check_fea2fea_file(name)
+
+        for line, sub in {(3, "sub"), (8, "pos"), (13, "sub")}:
+            captor.assertRegex(
+                f'{name}.fea:{line}:12: Ambiguous "ignore {sub}", there should be least one marked glyph'
+            )
+
 
 def generate_feature_file_test(name):
     return lambda self: self.check_feature_file(name)

Tests/feaLib/data/GSUB_5_formats.fea

@@ -1,16 +1,16 @@
 lookup GSUB5f1 {
-    ignore sub three four;
+    ignore sub three' four';
-    ignore sub four five;
+    ignore sub four' five';
 } GSUB5f1;
 
 lookup GSUB5f2 {
-    ignore sub [a - z] [A - H] [I - Z];
+    ignore sub [a - z]' [A - H]' [I - Z]';
-    ignore sub [a - z] [A - H] [I - Z];
+    ignore sub [a - z]' [A - H]' [I - Z]';
-    ignore sub [a - z] [I - Z] [A - H];
+    ignore sub [a - z]' [I - Z]' [A - H]';
 } GSUB5f2;
 
 lookup GSUB5f3 {
-    ignore sub e;
+    ignore sub e';
 } GSUB5f3;
 
 feature test {

Tests/feaLib/data/bug2949.fea

@@ -0,0 +1,20 @@
+lookup lookup1 {
+#test-fea2fea: ignore sub three' four;
+    ignore sub three four;
+} lookup1;
+
+lookup lookup2 {
+#test-fea2fea: ignore pos three' four;
+    ignore pos three four;
+} lookup2;
+
+lookup lookup3 {
+#test-fea2fea: ignore sub one' two, three' four;
+    ignore sub one two, three four;
+} lookup3;
+
+feature test {
+    lookup lookup1;
+    lookup lookup2;
+    lookup lookup3;
+} test;

Tests/feaLib/data/bug2949.ttx

@@ -0,0 +1,133 @@
+<?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="test"/>
+        <Feature>
+          <!-- LookupCount=2 -->
+          <LookupListIndex index="0" value="0"/>
+          <LookupListIndex index="1" value="1"/>
+        </Feature>
+      </FeatureRecord>
+    </FeatureList>
+    <LookupList>
+      <!-- LookupCount=2 -->
+      <Lookup index="0">
+        <LookupType value="6"/>
+        <LookupFlag value="0"/>
+        <!-- SubTableCount=1 -->
+        <ChainContextSubst index="0" Format="3">
+          <!-- BacktrackGlyphCount=0 -->
+          <!-- InputGlyphCount=1 -->
+          <InputCoverage index="0">
+            <Glyph value="three"/>
+          </InputCoverage>
+          <!-- LookAheadGlyphCount=1 -->
+          <LookAheadCoverage index="0">
+            <Glyph value="four"/>
+          </LookAheadCoverage>
+          <!-- SubstCount=0 -->
+        </ChainContextSubst>
+      </Lookup>
+      <Lookup index="1">
+        <LookupType value="6"/>
+        <LookupFlag value="0"/>
+        <!-- SubTableCount=1 -->
+        <ChainContextSubst index="0" Format="1">
+          <Coverage>
+            <Glyph value="one"/>
+            <Glyph value="three"/>
+          </Coverage>
+          <!-- ChainSubRuleSetCount=2 -->
+          <ChainSubRuleSet index="0">
+            <!-- ChainSubRuleCount=1 -->
+            <ChainSubRule index="0">
+              <!-- BacktrackGlyphCount=0 -->
+              <!-- InputGlyphCount=1 -->
+              <!-- LookAheadGlyphCount=1 -->
+              <LookAhead index="0" value="two"/>
+              <!-- SubstCount=0 -->
+            </ChainSubRule>
+          </ChainSubRuleSet>
+          <ChainSubRuleSet index="1">
+            <!-- ChainSubRuleCount=1 -->
+            <ChainSubRule index="0">
+              <!-- BacktrackGlyphCount=0 -->
+              <!-- InputGlyphCount=1 -->
+              <!-- LookAheadGlyphCount=1 -->
+              <LookAhead index="0" value="four"/>
+              <!-- SubstCount=0 -->
+            </ChainSubRule>
+          </ChainSubRuleSet>
+        </ChainContextSubst>
+      </Lookup>
+    </LookupList>
+  </GSUB>
+
+  <GPOS>
+    <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="test"/>
+        <Feature>
+          <!-- LookupCount=1 -->
+          <LookupListIndex index="0" value="0"/>
+        </Feature>
+      </FeatureRecord>
+    </FeatureList>
+    <LookupList>
+      <!-- LookupCount=1 -->
+      <Lookup index="0">
+        <LookupType value="8"/>
+        <LookupFlag value="0"/>
+        <!-- SubTableCount=1 -->
+        <ChainContextPos index="0" Format="3">
+          <!-- BacktrackGlyphCount=0 -->
+          <!-- InputGlyphCount=1 -->
+          <InputCoverage index="0">
+            <Glyph value="three"/>
+          </InputCoverage>
+          <!-- LookAheadGlyphCount=1 -->
+          <LookAheadCoverage index="0">
+            <Glyph value="four"/>
+          </LookAheadCoverage>
+          <!-- PosCount=0 -->
+        </ChainContextPos>
+      </Lookup>
+    </LookupList>
+  </GPOS>
+
+</ttFont>

Tests/feaLib/data/bug509.fea

@@ -1,5 +1,5 @@
 @LETTER_A = [A A.sc A.alt1];
 feature test {
-    ignore sub A;
+    ignore sub A';
     sub @LETTER_A' by a;
 } test;

Tests/feaLib/parser_test.py

@@ -2083,6 +2083,15 @@ class ParserTest(unittest.TestCase):
         doc = Parser(fea_path, includeDir=include_dir).parse()
         assert len(doc.statements) == 1 and doc.statements[0].text == "# Nothing"
 
+    def test_unmarked_ignore_statement(self):
+        with CapturingLogHandler("fontTools.feaLib.parser", level="WARNING") as caplog:
+            doc = self.parse("lookup foo { ignore sub A; } foo;")
+        self.assertEqual(doc.statements[0].statements[0].asFea(), "ignore sub A';")
+        self.assertEqual(len(caplog.records), 1)
+        caplog.assertRegex(
+            'Ambiguous "ignore sub", there should be least one marked glyph'
+        )
+
     def parse(self, text, glyphNames=GLYPHNAMES, followIncludes=True):
         featurefile = StringIO(text)
         p = Parser(featurefile, glyphNames, followIncludes=followIncludes)