From be77f3eeab4b3ef759d46c3205e49f80cf41bda2 Mon Sep 17 00:00:00 2001 From: Cosimo Lupo Date: Fri, 3 Apr 2020 11:46:36 +0100 Subject: [PATCH 1/4] feaLib/parser_test: rename {Unicode,String}IO, remove misc.py23 --- Tests/feaLib/parser_test.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/Tests/feaLib/parser_test.py b/Tests/feaLib/parser_test.py index cb4e689b7..927c7e4c7 100644 --- a/Tests/feaLib/parser_test.py +++ b/Tests/feaLib/parser_test.py @@ -1,7 +1,7 @@ # -*- coding: utf-8 -*- from fontTools.feaLib.error import FeatureLibError from fontTools.feaLib.parser import Parser, SymbolTable -from fontTools.misc.py23 import * +from io import StringIO import warnings import fontTools.feaLib.ast as ast import os @@ -62,7 +62,7 @@ class ParserTest(unittest.TestCase): glyphMap = {'a': 0, 'b': 1, 'c': 2} with warnings.catch_warnings(record=True) as w: warnings.simplefilter("always") - parser = Parser(UnicodeIO(), glyphMap=glyphMap) + parser = Parser(StringIO(), glyphMap=glyphMap) self.assertEqual(len(w), 1) self.assertEqual(w[-1].category, UserWarning) @@ -71,11 +71,11 @@ class ParserTest(unittest.TestCase): self.assertRaisesRegex( TypeError, "mutually exclusive", - Parser, UnicodeIO(), ("a",), glyphMap={"a": 0}) + Parser, StringIO(), ("a",), glyphMap={"a": 0}) self.assertRaisesRegex( TypeError, "unsupported keyword argument", - Parser, UnicodeIO(), foo="bar") + Parser, StringIO(), foo="bar") def test_comments(self): doc = self.parse( @@ -1720,7 +1720,7 @@ class ParserTest(unittest.TestCase): self.assertEqual(doc.statements[0].statements, []) def parse(self, text, glyphNames=GLYPHNAMES, followIncludes=True): - featurefile = UnicodeIO(text) + featurefile = StringIO(text) p = Parser(featurefile, glyphNames, followIncludes=followIncludes) return p.parse() From a913431ecdb32f0a4a203fee9e40f1808f129c07 Mon Sep 17 00:00:00 2001 From: Cosimo Lupo Date: Fri, 3 Apr 2020 12:38:35 +0100 Subject: [PATCH 2/4] feaLib.parser: if no glyphNames, treat dash as names, not ranges But print a warning about the possible ambiguity. Fixes https://github.com/fonttools/fonttools/issues/1768 --- Lib/fontTools/feaLib/parser.py | 11 ++++++++--- Tests/feaLib/parser_test.py | 14 ++++++++++++++ 2 files changed, 22 insertions(+), 3 deletions(-) diff --git a/Lib/fontTools/feaLib/parser.py b/Lib/fontTools/feaLib/parser.py index bda34e261..1d9c717fd 100644 --- a/Lib/fontTools/feaLib/parser.py +++ b/Lib/fontTools/feaLib/parser.py @@ -299,7 +299,7 @@ class Parser(object): if self.next_token_type_ is Lexer.NAME: glyph = self.expect_glyph_() location = self.cur_token_location_ - if '-' in glyph and glyph not in self.glyphNames_: + if '-' in glyph and self.glyphNames_ and glyph not in self.glyphNames_: start, limit = self.split_glyph_range_(glyph, location) self.check_glyph_name_in_glyph_set(start, limit) glyphs.add_range( @@ -314,6 +314,11 @@ class Parser(object): start, limit, self.make_glyph_range_(location, start, limit)) else: + if not self.glyphNames_: + log.warning(str(FeatureLibError( + f"Ambiguous glyph name that looks like a range: {glyph!r}", + location + ))) self.check_glyph_name_in_glyph_set(glyph) glyphs.append(glyph) elif self.next_token_type_ is Lexer.CID: @@ -350,8 +355,8 @@ class Parser(object): glyphs.add_class(gc) else: raise FeatureLibError( - "Expected glyph name, glyph range, " - "or glyph class reference", + f"Expected glyph name, glyph range, " + f"or glyph class reference, found {self.next_token_!r}", self.next_token_location_) self.expect_symbol_("]") return glyphs diff --git a/Tests/feaLib/parser_test.py b/Tests/feaLib/parser_test.py index 927c7e4c7..402a11dd4 100644 --- a/Tests/feaLib/parser_test.py +++ b/Tests/feaLib/parser_test.py @@ -1,4 +1,5 @@ # -*- coding: utf-8 -*- +from fontTools.misc.loggingTools import CapturingLogHandler from fontTools.feaLib.error import FeatureLibError from fontTools.feaLib.parser import Parser, SymbolTable from io import StringIO @@ -346,6 +347,19 @@ class ParserTest(unittest.TestCase): [gc] = self.parse("@range = [A-foo.sc - C-foo.sc];", gn).statements self.assertEqual(gc.glyphSet(), ("A-foo.sc", "B-foo.sc", "C-foo.sc")) + def test_glyphclass_ambiguous_dash_no_glyph_names(self): + # If Parser is initialized without a glyphNames parameter (or with empty one) + # it cannot distinguish between a glyph name containing an hyphen, or a + # range of glyph names; thus it will interpret them as literal glyph names + # while also outputting a logging warning to alert user about the ambiguity. + # https://github.com/fonttools/fonttools/issues/1768 + glyphNames = () + with CapturingLogHandler("fontTools.feaLib.parser", level="WARNING") as caplog: + [gc] = self.parse("@class = [A-foo.sc B-foo.sc];", glyphNames).statements + self.assertEqual(gc.glyphSet(), ("A-foo.sc", "B-foo.sc")) + self.assertEqual(len(caplog.records), 2) + caplog.assertRegex("Ambiguous glyph name that looks like a range:") + def test_glyphclass_glyph_name_should_win_over_range(self): # The OpenType Feature File Specification v1.20 makes it clear # that if a dashed name could be interpreted either as a glyph name From c1af75b80345ec35c385695eacb5ccd6c20d43b3 Mon Sep 17 00:00:00 2001 From: Cosimo Lupo Date: Fri, 3 Apr 2020 21:17:02 +0100 Subject: [PATCH 3/4] feaLib.parser: check that glyph name is actually ambiguous https://github.com/fonttools/fonttools/pull/1870#discussion_r403259450 --- Lib/fontTools/feaLib/parser.py | 2 +- Tests/feaLib/parser_test.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Lib/fontTools/feaLib/parser.py b/Lib/fontTools/feaLib/parser.py index 1d9c717fd..e24cde5ab 100644 --- a/Lib/fontTools/feaLib/parser.py +++ b/Lib/fontTools/feaLib/parser.py @@ -314,7 +314,7 @@ class Parser(object): start, limit, self.make_glyph_range_(location, start, limit)) else: - if not self.glyphNames_: + if '-' in glyph and not self.glyphNames_: log.warning(str(FeatureLibError( f"Ambiguous glyph name that looks like a range: {glyph!r}", location diff --git a/Tests/feaLib/parser_test.py b/Tests/feaLib/parser_test.py index 402a11dd4..f2c78635f 100644 --- a/Tests/feaLib/parser_test.py +++ b/Tests/feaLib/parser_test.py @@ -355,8 +355,8 @@ class ParserTest(unittest.TestCase): # https://github.com/fonttools/fonttools/issues/1768 glyphNames = () with CapturingLogHandler("fontTools.feaLib.parser", level="WARNING") as caplog: - [gc] = self.parse("@class = [A-foo.sc B-foo.sc];", glyphNames).statements - self.assertEqual(gc.glyphSet(), ("A-foo.sc", "B-foo.sc")) + [gc] = self.parse("@class = [A-foo.sc B-foo.sc C D];", glyphNames).statements + self.assertEqual(gc.glyphSet(), ("A-foo.sc", "B-foo.sc", "C", "D")) self.assertEqual(len(caplog.records), 2) caplog.assertRegex("Ambiguous glyph name that looks like a range:") From 40dcd1ebaa401a83fee41b85fd42ffb4db075208 Mon Sep 17 00:00:00 2001 From: Cosimo Lupo Date: Fri, 3 Apr 2020 21:19:01 +0100 Subject: [PATCH 4/4] minor: remove unnecessary f'' string --- Lib/fontTools/feaLib/parser.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/fontTools/feaLib/parser.py b/Lib/fontTools/feaLib/parser.py index e24cde5ab..07c778e52 100644 --- a/Lib/fontTools/feaLib/parser.py +++ b/Lib/fontTools/feaLib/parser.py @@ -355,7 +355,7 @@ class Parser(object): glyphs.add_class(gc) else: raise FeatureLibError( - f"Expected glyph name, glyph range, " + "Expected glyph name, glyph range, " f"or glyph class reference, found {self.next_token_!r}", self.next_token_location_) self.expect_symbol_("]")