Merge pull request #1460 from anthrotype/fix-null-valuerecord1-format-a

[feaLib] distinguish missing value and explicit '<NULL>' for PairPos2 format A

Lib/fontTools/feaLib/ast.py

@@ -1115,6 +1115,9 @@ class ValueRecord(Expression):
                 hash(self.xAdvDevice) ^ hash(self.yAdvDevice))
 
     def makeString(self, vertical=None):
+        if not self:
+            return "<NULL>"
+
         x, y = self.xPlacement, self.yPlacement
         xAdvance, yAdvance = self.xAdvance, self.yAdvance
         xPlaDevice, yPlaDevice = self.xPlaDevice, self.yPlaDevice
@@ -1140,6 +1143,23 @@ class ValueRecord(Expression):
             deviceToString(xPlaDevice), deviceToString(yPlaDevice),
             deviceToString(xAdvDevice), deviceToString(yAdvDevice))
 
+    def __bool__(self):
+        return any(
+            getattr(self, v) is not None
+            for v in [
+                "xPlacement",
+                "yPlacement",
+                "xAdvance",
+                "yAdvance",
+                "xPlaDevice",
+                "yPlaDevice",
+                "xAdvDevice",
+                "yAdvDevice",
+            ]
+        )
+
+    __nonzero__ = __bool__
+
 
 class ValueRecordDefinition(Statement):
     def __init__(self, name, value, location=None):

Lib/fontTools/feaLib/builder.py

@@ -1114,7 +1114,7 @@ _VALUEREC_ATTRS = {
 
 def makeOpenTypeValueRecord(v, pairPosContext):
     """ast.ValueRecord --> (otBase.ValueRecord, int ValueFormat)"""
-    if v is None:
+    if not v:
         return None, 0
 
     vr = {}

Lib/fontTools/feaLib/parser.py

@@ -1153,7 +1153,7 @@ class Parser(object):
             name = self.expect_name_()
             if name == "NULL":
                 self.expect_symbol_(">")
-                return None
+                return self.ast.ValueRecord()
             vrd = self.valuerecords_.resolve(name)
             if vrd is None:
                 raise FeatureLibError("Unknown valueRecordDef \"%s\"" % name,

Tests/feaLib/builder_test.py

@@ -65,7 +65,7 @@ class BuilderTest(unittest.TestCase):
         spec9a spec9b spec9c1 spec9c2 spec9c3 spec9d spec9e spec9f spec9g
         spec10
         bug453 bug457 bug463 bug501 bug502 bug504 bug505 bug506 bug509
-        bug512 bug514 bug568 bug633 bug1307
+        bug512 bug514 bug568 bug633 bug1307 bug1459
         name size size2 multiple_feature_blocks omitted_GlyphClassDef
         ZeroValue_SinglePos_horizontal ZeroValue_SinglePos_vertical
         ZeroValue_PairPos_horizontal ZeroValue_PairPos_vertical

Tests/feaLib/data/bug1459.fea

@@ -0,0 +1,7 @@
+# A pair position lookup where only the second glyph has a non-empty valuerecord
+# while the first glyph has a NULL valuerecord. The ValueFormat1 for the first
+# glyph is expected to be 0.
+# https://github.com/fonttools/fonttools/issues/1459
+feature kern {
+    pos A <NULL> V <-180 0 -90 0>;
+} kern;

Tests/feaLib/data/bug1459.ttx

@@ -0,0 +1,55 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<ttFont>
+
+  <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="kern"/>
+        <Feature>
+          <!-- LookupCount=1 -->
+          <LookupListIndex index="0" value="0"/>
+        </Feature>
+      </FeatureRecord>
+    </FeatureList>
+    <LookupList>
+      <!-- LookupCount=1 -->
+      <Lookup index="0">
+        <LookupType value="2"/>
+        <LookupFlag value="0"/>
+        <!-- SubTableCount=1 -->
+        <PairPos index="0" Format="1">
+          <Coverage>
+            <Glyph value="A"/>
+          </Coverage>
+          <ValueFormat1 value="0"/>
+          <ValueFormat2 value="5"/>
+          <!-- PairSetCount=1 -->
+          <PairSet index="0">
+            <!-- PairValueCount=1 -->
+            <PairValueRecord index="0">
+              <SecondGlyph value="V"/>
+              <Value2 XPlacement="-180" XAdvance="-90"/>
+            </PairValueRecord>
+          </PairSet>
+        </PairPos>
+      </Lookup>
+    </LookupList>
+  </GPOS>
+
+</ttFont>

Tests/feaLib/parser_test.py

@@ -800,7 +800,22 @@ class ParserTest(unittest.TestCase):
         self.assertEqual(pos.valuerecord2.makeString(vertical=False),
                          "<1 2 3 4>")
 
-    def test_gpos_type_2_format_a_with_null(self):
+    def test_gpos_type_2_format_a_with_null_first(self):
+        doc = self.parse("feature kern {"
+                         "    pos [T V] <NULL> [a b c] <1 2 3 4>;"
+                         "} kern;")
+        pos = doc.statements[0].statements[0]
+        self.assertEqual(type(pos), ast.PairPosStatement)
+        self.assertFalse(pos.enumerated)
+        self.assertEqual(glyphstr([pos.glyphs1]), "[T V]")
+        self.assertFalse(pos.valuerecord1)
+        self.assertEqual(pos.valuerecord1.makeString(), "<NULL>")
+        self.assertEqual(glyphstr([pos.glyphs2]), "[a b c]")
+        self.assertEqual(pos.valuerecord2.makeString(vertical=False),
+                         "<1 2 3 4>")
+        self.assertEqual(pos.asFea(), "pos [T V] <NULL> [a b c] <1 2 3 4>;")
+
+    def test_gpos_type_2_format_a_with_null_second(self):
         doc = self.parse("feature kern {"
                          "    pos [T V] <1 2 3 4> [a b c] <NULL>;"
                          "} kern;")
@@ -811,7 +826,8 @@ class ParserTest(unittest.TestCase):
         self.assertEqual(pos.valuerecord1.makeString(vertical=False),
                          "<1 2 3 4>")
         self.assertEqual(glyphstr([pos.glyphs2]), "[a b c]")
-        self.assertIsNone(pos.valuerecord2)
+        self.assertFalse(pos.valuerecord2)
+        self.assertEqual(pos.asFea(), "pos [T V] [a b c] <1 2 3 4>;")
 
     def test_gpos_type_2_format_b(self):
         doc = self.parse("feature kern {"
@@ -1523,7 +1539,8 @@ class ParserTest(unittest.TestCase):
     def test_valuerecord_format_d(self):
         doc = self.parse("feature test {valueRecordDef <NULL> foo;} test;")
         value = doc.statements[0].statements[0].value
-        self.assertIsNone(value)
+        self.assertFalse(value)
+        self.assertEqual(value.makeString(), "<NULL>")
 
     def test_valuerecord_named(self):
         doc = self.parse("valueRecordDef <1 2 3 4> foo;"