diff --git a/Lib/fontTools/feaLib/ast.py b/Lib/fontTools/feaLib/ast.py index af23ecd42..1273343da 100644 --- a/Lib/fontTools/feaLib/ast.py +++ b/Lib/fontTools/feaLib/ast.py @@ -1331,10 +1331,16 @@ class PairPosStatement(Statement): """ if self.enumerated: g = [self.glyphs1.glyphSet(), self.glyphs2.glyphSet()] + seen_pair = False for glyph1, glyph2 in itertools.product(*g): + seen_pair = True builder.add_specific_pair_pos( self.location, glyph1, self.valuerecord1, glyph2, self.valuerecord2 ) + if not seen_pair: + raise FeatureLibError( + "Empty glyph class in positioning rule", self.location + ) return is_specific = isinstance(self.glyphs1, GlyphName) and isinstance( diff --git a/Lib/fontTools/feaLib/builder.py b/Lib/fontTools/feaLib/builder.py index 84ff066ef..a16448759 100644 --- a/Lib/fontTools/feaLib/builder.py +++ b/Lib/fontTools/feaLib/builder.py @@ -1195,39 +1195,6 @@ class Builder(object): for glyph in glyphs: self.attachPoints_.setdefault(glyph, set()).update(contourPoints) - def add_chain_context_pos(self, location, prefix, glyphs, suffix, lookups): - lookup = self.get_lookup_(location, ChainContextPosBuilder) - lookup.rules.append( - ChainContextualRule( - prefix, glyphs, suffix, self.find_lookup_builders_(lookups) - ) - ) - - def add_chain_context_subst(self, location, prefix, glyphs, suffix, lookups): - lookup = self.get_lookup_(location, ChainContextSubstBuilder) - lookup.rules.append( - ChainContextualRule( - prefix, glyphs, suffix, self.find_lookup_builders_(lookups) - ) - ) - - def add_alternate_subst(self, location, prefix, glyph, suffix, replacement): - if self.cur_feature_name_ == "aalt": - alts = self.aalt_alternates_.setdefault(glyph, set()) - alts.update(replacement) - return - if prefix or suffix: - chain = self.get_lookup_(location, ChainContextSubstBuilder) - lookup = self.get_chained_lookup_(location, AlternateSubstBuilder) - chain.rules.append(ChainContextualRule(prefix, [{glyph}], suffix, [lookup])) - else: - lookup = self.get_lookup_(location, AlternateSubstBuilder) - if glyph in lookup.alternates: - raise FeatureLibError( - 'Already defined alternates for glyph "%s"' % glyph, location - ) - lookup.alternates[glyph] = replacement - def add_feature_reference(self, location, featureName): if self.cur_feature_name_ != "aalt": raise FeatureLibError( @@ -1272,53 +1239,9 @@ class Builder(object): key = (script, lang, self.cur_feature_name_) self.features_.setdefault(key, []) - def add_ligature_subst( - self, location, prefix, glyphs, suffix, replacement, forceChain - ): - if prefix or suffix or forceChain: - chain = self.get_lookup_(location, ChainContextSubstBuilder) - lookup = self.get_chained_lookup_(location, LigatureSubstBuilder) - chain.rules.append(ChainContextualRule(prefix, glyphs, suffix, [lookup])) - else: - lookup = self.get_lookup_(location, LigatureSubstBuilder) - - # OpenType feature file syntax, section 5.d, "Ligature substitution": - # "Since the OpenType specification does not allow ligature - # substitutions to be specified on target sequences that contain - # glyph classes, the implementation software will enumerate - # all specific glyph sequences if glyph classes are detected" - for g in sorted(itertools.product(*glyphs)): - lookup.ligatures[g] = replacement - - def add_multiple_subst( - self, location, prefix, glyph, suffix, replacements, forceChain=False - ): - if prefix or suffix or forceChain: - chain = self.get_lookup_(location, ChainContextSubstBuilder) - sub = self.get_chained_lookup_(location, MultipleSubstBuilder) - sub.mapping[glyph] = replacements - chain.rules.append(ChainContextualRule(prefix, [{glyph}], suffix, [sub])) - return - lookup = self.get_lookup_(location, MultipleSubstBuilder) - if glyph in lookup.mapping: - if replacements == lookup.mapping[glyph]: - log.info( - "Removing duplicate multiple substitution from glyph" - ' "%s" to %s%s', - glyph, - replacements, - f" at {location}" if location else "", - ) - else: - raise FeatureLibError( - 'Already defined substitution for glyph "%s"' % glyph, location - ) - lookup.mapping[glyph] = replacements - - def add_reverse_chain_single_subst(self, location, old_prefix, old_suffix, mapping): - lookup = self.get_lookup_(location, ReverseChainSingleSubstBuilder) - lookup.rules.append((old_prefix, old_suffix, mapping)) + # GSUB rules + # GSUB 1 def add_single_subst(self, location, prefix, suffix, mapping, forceChain): if self.cur_feature_name_ == "aalt": for (from_glyph, to_glyph) in mapping.items(): @@ -1347,7 +1270,87 @@ class Builder(object): ) lookup.mapping[from_glyph] = to_glyph + # GSUB 2 + def add_multiple_subst( + self, location, prefix, glyph, suffix, replacements, forceChain=False + ): + if prefix or suffix or forceChain: + chain = self.get_lookup_(location, ChainContextSubstBuilder) + sub = self.get_chained_lookup_(location, MultipleSubstBuilder) + sub.mapping[glyph] = replacements + chain.rules.append(ChainContextualRule(prefix, [{glyph}], suffix, [sub])) + return + lookup = self.get_lookup_(location, MultipleSubstBuilder) + if glyph in lookup.mapping: + if replacements == lookup.mapping[glyph]: + log.info( + "Removing duplicate multiple substitution from glyph" + ' "%s" to %s%s', + glyph, + replacements, + f" at {location}" if location else "", + ) + else: + raise FeatureLibError( + 'Already defined substitution for glyph "%s"' % glyph, location + ) + lookup.mapping[glyph] = replacements + + # GSUB 3 + def add_alternate_subst(self, location, prefix, glyph, suffix, replacement): + if self.cur_feature_name_ == "aalt": + alts = self.aalt_alternates_.setdefault(glyph, set()) + alts.update(replacement) + return + if prefix or suffix: + chain = self.get_lookup_(location, ChainContextSubstBuilder) + lookup = self.get_chained_lookup_(location, AlternateSubstBuilder) + chain.rules.append(ChainContextualRule(prefix, [{glyph}], suffix, [lookup])) + else: + lookup = self.get_lookup_(location, AlternateSubstBuilder) + if glyph in lookup.alternates: + raise FeatureLibError( + 'Already defined alternates for glyph "%s"' % glyph, location + ) + # We allow empty replacement glyphs here. + lookup.alternates[glyph] = replacement + + # GSUB 4 + def add_ligature_subst( + self, location, prefix, glyphs, suffix, replacement, forceChain + ): + if prefix or suffix or forceChain: + chain = self.get_lookup_(location, ChainContextSubstBuilder) + lookup = self.get_chained_lookup_(location, LigatureSubstBuilder) + chain.rules.append(ChainContextualRule(prefix, glyphs, suffix, [lookup])) + else: + lookup = self.get_lookup_(location, LigatureSubstBuilder) + + if not all(glyphs): + raise FeatureLibError("Empty glyph class in substitution", location) + + # OpenType feature file syntax, section 5.d, "Ligature substitution": + # "Since the OpenType specification does not allow ligature + # substitutions to be specified on target sequences that contain + # glyph classes, the implementation software will enumerate + # all specific glyph sequences if glyph classes are detected" + for g in sorted(itertools.product(*glyphs)): + lookup.ligatures[g] = replacement + + # GSUB 5/6 + def add_chain_context_subst(self, location, prefix, glyphs, suffix, lookups): + if not all(glyphs) or not all(prefix) or not all(suffix): + raise FeatureLibError("Empty glyph class in contextual substitution", location) + lookup = self.get_lookup_(location, ChainContextSubstBuilder) + lookup.rules.append( + ChainContextualRule( + prefix, glyphs, suffix, self.find_lookup_builders_(lookups) + ) + ) + def add_single_subst_chained_(self, location, prefix, suffix, mapping): + if not mapping or not all(prefix) or not all(suffix): + raise FeatureLibError("Empty glyph class in contextual substitution", location) # https://github.com/fonttools/fonttools/issues/512 chain = self.get_lookup_(location, ChainContextSubstBuilder) sub = chain.find_chainable_single_subst(set(mapping.keys())) @@ -1358,7 +1361,54 @@ class Builder(object): ChainContextualRule(prefix, [list(mapping.keys())], suffix, [sub]) ) + # GSUB 8 + def add_reverse_chain_single_subst(self, location, old_prefix, old_suffix, mapping): + if not mapping: + raise FeatureLibError("Empty glyph class in substitution", location) + lookup = self.get_lookup_(location, ReverseChainSingleSubstBuilder) + lookup.rules.append((old_prefix, old_suffix, mapping)) + + # GPOS rules + + # GPOS 1 + def add_single_pos(self, location, prefix, suffix, pos, forceChain): + if prefix or suffix or forceChain: + self.add_single_pos_chained_(location, prefix, suffix, pos) + else: + lookup = self.get_lookup_(location, SinglePosBuilder) + for glyphs, value in pos: + if not glyphs: + raise FeatureLibError("Empty glyph class in positioning rule", location) + otValueRecord = self.makeOpenTypeValueRecord(location, value, pairPosContext=False) + for glyph in glyphs: + try: + lookup.add_pos(location, glyph, otValueRecord) + except OpenTypeLibError as e: + raise FeatureLibError(str(e), e.location) from e + + # GPOS 2 + def add_class_pair_pos(self, location, glyphclass1, value1, glyphclass2, value2): + if not glyphclass1 or not glyphclass2: + raise FeatureLibError( + "Empty glyph class in positioning rule", location + ) + lookup = self.get_lookup_(location, PairPosBuilder) + v1 = self.makeOpenTypeValueRecord(location, value1, pairPosContext=True) + v2 = self.makeOpenTypeValueRecord(location, value2, pairPosContext=True) + lookup.addClassPair(location, glyphclass1, v1, glyphclass2, v2) + + def add_specific_pair_pos(self, location, glyph1, value1, glyph2, value2): + if not glyph1 or not glyph2: + raise FeatureLibError("Empty glyph class in positioning rule", location) + lookup = self.get_lookup_(location, PairPosBuilder) + v1 = self.makeOpenTypeValueRecord(location, value1, pairPosContext=True) + v2 = self.makeOpenTypeValueRecord(location, value2, pairPosContext=True) + lookup.addGlyphPair(location, glyph1, v1, glyph2, v2) + + # GPOS 3 def add_cursive_pos(self, location, glyphclass, entryAnchor, exitAnchor): + if not glyphclass: + raise FeatureLibError("Empty glyph class in positioning rule", location) lookup = self.get_lookup_(location, CursivePosBuilder) lookup.add_attachment( location, @@ -1367,34 +1417,23 @@ class Builder(object): self.makeOpenTypeAnchor(location, exitAnchor), ) - def add_marks_(self, location, lookupBuilder, marks): - """Helper for add_mark_{base,liga,mark}_pos.""" - for _, markClass in marks: - for markClassDef in markClass.definitions: - for mark in markClassDef.glyphs.glyphSet(): - if mark not in lookupBuilder.marks: - otMarkAnchor = self.makeOpenTypeAnchor(location, markClassDef.anchor) - lookupBuilder.marks[mark] = (markClass.name, otMarkAnchor) - else: - existingMarkClass = lookupBuilder.marks[mark][0] - if markClass.name != existingMarkClass: - raise FeatureLibError( - "Glyph %s cannot be in both @%s and @%s" - % (mark, existingMarkClass, markClass.name), - location, - ) - + # GPOS 4 def add_mark_base_pos(self, location, bases, marks): builder = self.get_lookup_(location, MarkBasePosBuilder) self.add_marks_(location, builder, marks) + if not bases: + raise FeatureLibError("Empty glyph class in positioning rule", location) for baseAnchor, markClass in marks: otBaseAnchor = self.makeOpenTypeAnchor(location, baseAnchor) for base in bases: builder.bases.setdefault(base, {})[markClass.name] = otBaseAnchor + # GPOS 5 def add_mark_lig_pos(self, location, ligatures, components): builder = self.get_lookup_(location, MarkLigPosBuilder) componentAnchors = [] + if not ligatures: + raise FeatureLibError("Empty glyph class in positioning rule", location) for marks in components: anchors = {} self.add_marks_(location, builder, marks) @@ -1404,9 +1443,12 @@ class Builder(object): for glyph in ligatures: builder.ligatures[glyph] = componentAnchors + # GPOS 6 def add_mark_mark_pos(self, location, baseMarks, marks): builder = self.get_lookup_(location, MarkMarkPosBuilder) self.add_marks_(location, builder, marks) + if not baseMarks: + raise FeatureLibError("Empty glyph class in positioning rule", location) for baseAnchor, markClass in marks: otBaseAnchor = self.makeOpenTypeAnchor(location, baseAnchor) for baseMark in baseMarks: @@ -1414,35 +1456,20 @@ class Builder(object): markClass.name ] = otBaseAnchor - def add_class_pair_pos(self, location, glyphclass1, value1, glyphclass2, value2): - lookup = self.get_lookup_(location, PairPosBuilder) - v1 = self.makeOpenTypeValueRecord(location, value1, pairPosContext=True) - v2 = self.makeOpenTypeValueRecord(location, value2, pairPosContext=True) - lookup.addClassPair(location, glyphclass1, v1, glyphclass2, v2) - - def add_subtable_break(self, location): - self.cur_lookup_.add_subtable_break(location) - - def add_specific_pair_pos(self, location, glyph1, value1, glyph2, value2): - lookup = self.get_lookup_(location, PairPosBuilder) - v1 = self.makeOpenTypeValueRecord(location, value1, pairPosContext=True) - v2 = self.makeOpenTypeValueRecord(location, value2, pairPosContext=True) - lookup.addGlyphPair(location, glyph1, v1, glyph2, v2) - - def add_single_pos(self, location, prefix, suffix, pos, forceChain): - if prefix or suffix or forceChain: - self.add_single_pos_chained_(location, prefix, suffix, pos) - else: - lookup = self.get_lookup_(location, SinglePosBuilder) - for glyphs, value in pos: - otValueRecord = self.makeOpenTypeValueRecord(location, value, pairPosContext=False) - for glyph in glyphs: - try: - lookup.add_pos(location, glyph, otValueRecord) - except OpenTypeLibError as e: - raise FeatureLibError(str(e), e.location) from e + # GPOS 7/8 + def add_chain_context_pos(self, location, prefix, glyphs, suffix, lookups): + if not all(glyphs) or not all(prefix) or not all(suffix): + raise FeatureLibError("Empty glyph class in contextual positioning rule", location) + lookup = self.get_lookup_(location, ChainContextPosBuilder) + lookup.rules.append( + ChainContextualRule( + prefix, glyphs, suffix, self.find_lookup_builders_(lookups) + ) + ) def add_single_pos_chained_(self, location, prefix, suffix, pos): + if not pos or not all(prefix) or not all(suffix): + raise FeatureLibError("Empty glyph class in contextual positioning rule", location) # https://github.com/fonttools/fonttools/issues/514 chain = self.get_lookup_(location, ChainContextPosBuilder) targets = [] @@ -1466,6 +1493,26 @@ class Builder(object): ChainContextualRule(prefix, [g for g, v in pos], suffix, subs) ) + def add_marks_(self, location, lookupBuilder, marks): + """Helper for add_mark_{base,liga,mark}_pos.""" + for _, markClass in marks: + for markClassDef in markClass.definitions: + for mark in markClassDef.glyphs.glyphSet(): + if mark not in lookupBuilder.marks: + otMarkAnchor = self.makeOpenTypeAnchor(location, markClassDef.anchor) + lookupBuilder.marks[mark] = (markClass.name, otMarkAnchor) + else: + existingMarkClass = lookupBuilder.marks[mark][0] + if markClass.name != existingMarkClass: + raise FeatureLibError( + "Glyph %s cannot be in both @%s and @%s" + % (mark, existingMarkClass, markClass.name), + location, + ) + + def add_subtable_break(self, location): + self.cur_lookup_.add_subtable_break(location) + def setGlyphClass_(self, location, glyph, glyphClass): oldClass, oldLocation = self.glyphClassDefs_.get(glyph, (None, None)) if oldClass and oldClass != glyphClass: diff --git a/Lib/fontTools/feaLib/parser.py b/Lib/fontTools/feaLib/parser.py index 6020b158f..fd53573da 100644 --- a/Lib/fontTools/feaLib/parser.py +++ b/Lib/fontTools/feaLib/parser.py @@ -684,6 +684,8 @@ class Parser(object): assert self.is_cur_keyword_("markClass") location = self.cur_token_location_ glyphs = self.parse_glyphclass_(accept_glyphname=True) + if not glyphs.glyphSet(): + raise FeatureLibError("Empty glyph class in mark class definition", location) anchor = self.parse_anchor_() name = self.expect_class_name_() self.expect_symbol_(";") @@ -872,7 +874,7 @@ class Parser(object): num_lookups = len([l for l in lookups if l is not None]) is_deletion = False - if len(new) == 1 and len(new[0].glyphSet()) == 0: + if len(new) == 1 and isinstance(new[0], ast.NullGlyph): new = [] # Deletion is_deletion = True @@ -918,6 +920,9 @@ class Parser(object): and max([len(n.glyphSet()) for n in new]) == 1 and num_lookups == 0 ): + for n in new: + if not list(n.glyphSet()): + raise FeatureLibError("Empty class in replacement", location) return self.ast.MultipleSubstStatement( old_prefix, tuple(old[0].glyphSet())[0], diff --git a/Tests/feaLib/builder_test.py b/Tests/feaLib/builder_test.py index 16674fe36..5c298e85c 100644 --- a/Tests/feaLib/builder_test.py +++ b/Tests/feaLib/builder_test.py @@ -925,6 +925,45 @@ class BuilderTest(unittest.TestCase): assert "GPOS" not in font assert "GSUB" not in font + def test_disable_empty_classes(self): + for test in [ + "sub a by c []", + "sub f f [] by f", + "ignore sub a []'", + "ignore sub [] a'", + "sub a []' by b", + "sub [] a' by b", + "rsub [] by a", + "pos [] 120", + "pos a [] 120", + "enum pos a [] 120", + "pos cursive [] ", + "pos base [] mark @TOPMARKS", + "pos ligature [] mark @TOPMARKS", + "pos mark [] mark @TOPMARKS", + "ignore pos a []'", + "ignore pos [] a'", + ]: + self.assertRaisesRegex( + FeatureLibError, + "Empty ", + self.build, + f"markClass a @TOPMARKS; lookup foo {{ {test}; }} foo;", + ) + self.assertRaisesRegex( + FeatureLibError, + "Empty glyph class in mark class definition", + self.build, + "markClass [] @TOPMARKS;" + ) + self.assertRaisesRegex( + FeatureLibError, + 'Expected a glyph class with 1 elements after "by", but found a glyph class with 0 elements', + self.build, + "feature test { sub a by []; test};" + ) + + def generate_feature_file_test(name): return lambda self: self.check_feature_file(name) diff --git a/Tests/feaLib/data/PairPosSubtable.fea b/Tests/feaLib/data/PairPosSubtable.fea index cb78801c9..1fcc1eba2 100644 --- a/Tests/feaLib/data/PairPosSubtable.fea +++ b/Tests/feaLib/data/PairPosSubtable.fea @@ -4,7 +4,6 @@ languagesystem latn dflt; @group1 = [b o]; @group2 = [c d]; @group3 = [v w]; -@group4 = []; lookup kernlookup { pos A V -34; @@ -13,9 +12,6 @@ lookup kernlookup { subtable; pos @group1 @group3 -10; pos @group3 @group2 -20; - subtable; - pos @group4 @group1 -10; - pos @group4 @group4 -10; } kernlookup; feature kern {