From eb93b7688e99ff8872f8c499a6b3967f68710c2f Mon Sep 17 00:00:00 2001 From: Khaled Hosny Date: Tue, 10 Jan 2023 00:03:48 +0200 Subject: [PATCH] [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 | 13 +-- Lib/fontTools/feaLib/parser.py | 46 +++++---- Tests/feaLib/builder_test.py | 12 +++ Tests/feaLib/data/GSUB_5_formats.fea | 12 +-- Tests/feaLib/data/bug2949.fea | 20 ++++ Tests/feaLib/data/bug2949.ttx | 133 +++++++++++++++++++++++++++ Tests/feaLib/data/bug509.fea | 2 +- Tests/feaLib/parser_test.py | 9 ++ 8 files changed, 208 insertions(+), 39 deletions(-) create mode 100644 Tests/feaLib/data/bug2949.fea create mode 100644 Tests/feaLib/data/bug2949.ttx diff --git a/Lib/fontTools/feaLib/ast.py b/Lib/fontTools/feaLib/ast.py index 1273343da..ef068b82b 100644 --- a/Lib/fontTools/feaLib/ast.py +++ b/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): - res += " ".join(map(asFea, prefix)) + " " - res += " ".join(g.asFea() + "'" for g in glyphs) - if len(suffix): - res += " " + " ".join(map(asFea, suffix)) - else: - res += " ".join(map(asFea, glyphs)) + if len(prefix): + res += " ".join(map(asFea, prefix)) + " " + res += " ".join(g.asFea() + "'" for g in glyphs) + if len(suffix): + res += " " + " ".join(map(asFea, suffix)) contexts.append(res) return "ignore sub " + ", ".join(contexts) + ";" diff --git a/Lib/fontTools/feaLib/parser.py b/Lib/fontTools/feaLib/parser.py index 3c2cd7267..b325b3649 100644 --- a/Lib/fontTools/feaLib/parser.py +++ b/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)] - hasLookups = any(lookups) + if 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_(",") - ( - prefix, - glyphs, - lookups, - values, - suffix, - hasMarks, - ) = self.parse_glyph_pattern_(vertical=False) - chainContext.append((prefix, glyphs, suffix)) - hasLookups = hasLookups or any(lookups) + chainContext.append(self.parse_ignore_glyph_pattern_(sub)) 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_() - if hasLookups: - raise FeatureLibError( - 'No lookups can be specified for "ignore sub"', location - ) + chainContext = self.parse_ignore_context_("sub") return self.ast.IgnoreSubstStatement(chainContext, location=location) if self.cur_token_ in ["position", "pos"]: - chainContext, hasLookups = self.parse_chain_context_() - if hasLookups: - raise FeatureLibError( - 'No lookups can be specified for "ignore pos"', location - ) + chainContext = self.parse_ignore_context_("pos") return self.ast.IgnorePosStatement(chainContext, location=location) raise FeatureLibError( 'Expected "substitute" or "position"', self.cur_token_location_ diff --git a/Tests/feaLib/builder_test.py b/Tests/feaLib/builder_test.py index 02fb00c3b..96effdeb5 100644 --- a/Tests/feaLib/builder_test.py +++ b/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) diff --git a/Tests/feaLib/data/GSUB_5_formats.fea b/Tests/feaLib/data/GSUB_5_formats.fea index 3acb7edf1..e9b3ba8b9 100644 --- a/Tests/feaLib/data/GSUB_5_formats.fea +++ b/Tests/feaLib/data/GSUB_5_formats.fea @@ -1,16 +1,16 @@ lookup GSUB5f1 { - ignore sub three four; - ignore sub four five; + ignore sub three' four'; + 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] [I - Z] [A - H]; + ignore sub [a - z]' [A - H]' [I - Z]'; + ignore sub [a - z]' [A - H]' [I - Z]'; + ignore sub [a - z]' [I - Z]' [A - H]'; } GSUB5f2; lookup GSUB5f3 { - ignore sub e; + ignore sub e'; } GSUB5f3; feature test { diff --git a/Tests/feaLib/data/bug2949.fea b/Tests/feaLib/data/bug2949.fea new file mode 100644 index 000000000..cd0b7a868 --- /dev/null +++ b/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; diff --git a/Tests/feaLib/data/bug2949.ttx b/Tests/feaLib/data/bug2949.ttx new file mode 100644 index 000000000..8ad9ccb49 --- /dev/null +++ b/Tests/feaLib/data/bug2949.ttx @@ -0,0 +1,133 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/Tests/feaLib/data/bug509.fea b/Tests/feaLib/data/bug509.fea index b7af056fd..488e769a1 100644 --- a/Tests/feaLib/data/bug509.fea +++ b/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; diff --git a/Tests/feaLib/parser_test.py b/Tests/feaLib/parser_test.py index fe086c761..6bf9c6a87 100644 --- a/Tests/feaLib/parser_test.py +++ b/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)