From 6a24c8fed1d96542f4c4514354b8850f4d5cefdf Mon Sep 17 00:00:00 2001
From: Khaled Hosny <khaledhosny@eglug.org>
Date: Mon, 20 Apr 2020 23:09:53 +0200
Subject: [PATCH 1/2] [feaLib] Raise for script/language in standalone lookup

They are not allowed per spec and makeotf, but currently we would fail
later with some cryptic error message.
---
 Lib/fontTools/feaLib/builder.py |  8 ++++++++
 Tests/feaLib/builder_test.py    | 12 ++++++++++++
 2 files changed, 20 insertions(+)

diff --git a/Lib/fontTools/feaLib/builder.py b/Lib/fontTools/feaLib/builder.py
index 51d57c792..0a64cdc5d 100644
--- a/Lib/fontTools/feaLib/builder.py
+++ b/Lib/fontTools/feaLib/builder.py
@@ -706,6 +706,10 @@ class Builder(object):
             raise FeatureLibError(
                 "Language statements are not allowed "
                 "within \"feature %s\"" % self.cur_feature_name_, location)
+        if self.cur_feature_name_ is None:
+            raise FeatureLibError(
+                "Language statements are not allowed "
+                "within standalone lookup blocks", location)
         self.cur_lookup_ = None
 
         key = (self.script_, language, self.cur_feature_name_)
@@ -772,6 +776,10 @@ class Builder(object):
             raise FeatureLibError(
                 "Script statements are not allowed "
                 "within \"feature %s\"" % self.cur_feature_name_, location)
+        if self.cur_feature_name_ is None:
+            raise FeatureLibError(
+                "Script statements are not allowed "
+                "within standalone lookup blocks", location)
         self.cur_lookup_ = None
         self.script_ = script
         self.lookupflag_ = 0
diff --git a/Tests/feaLib/builder_test.py b/Tests/feaLib/builder_test.py
index a00525cfd..eead46a1b 100644
--- a/Tests/feaLib/builder_test.py
+++ b/Tests/feaLib/builder_test.py
@@ -337,6 +337,12 @@ class BuilderTest(unittest.TestCase):
             "Script statements are not allowed within \"feature size\"",
             self.build, "feature size { script latn; } size;")
 
+    def test_script_in_standalone_lookup(self):
+        self.assertRaisesRegex(
+            FeatureLibError,
+            "Script statements are not allowed within standalone lookup blocks",
+            self.build, "lookup test { script latn; } test;")
+
     def test_language(self):
         builder = Builder(makeTTFont(), (None, None))
         builder.add_language_system(None, 'latn', 'FRA ')
@@ -364,6 +370,12 @@ class BuilderTest(unittest.TestCase):
             "Language statements are not allowed within \"feature size\"",
             self.build, "feature size { language FRA; } size;")
 
+    def test_language_in_standalone_lookup(self):
+        self.assertRaisesRegex(
+            FeatureLibError,
+            "Language statements are not allowed within standalone lookup blocks",
+            self.build, "lookup test { language FRA; } test;")
+
     def test_language_required_duplicate(self):
         self.assertRaisesRegex(
             FeatureLibError,

From 259e586f5df6f6e1cf86dbdf6f6db65be96fc1b3 Mon Sep 17 00:00:00 2001
From: Khaled Hosny <khaledhosny@eglug.org>
Date: Mon, 20 Apr 2020 23:45:45 +0200
Subject: [PATCH 2/2] [feaLib] Ignore superfluous script statements

Setting script that is the same as current language system should make
no effect. This is not documented in the spec, but it is what makeotf
does. This as the effect of preserving lookupflag when set before such a
script statement.

Fixes https://github.com/fonttools/fonttools/issues/1824
---
 Lib/fontTools/feaLib/builder.py  |  3 ++
 Tests/feaLib/data/lookupflag.fea | 13 +++++++++
 Tests/feaLib/data/lookupflag.ttx | 49 ++++++++++++++++++++++++++++++--
 3 files changed, 62 insertions(+), 3 deletions(-)

diff --git a/Lib/fontTools/feaLib/builder.py b/Lib/fontTools/feaLib/builder.py
index 0a64cdc5d..b23fcf752 100644
--- a/Lib/fontTools/feaLib/builder.py
+++ b/Lib/fontTools/feaLib/builder.py
@@ -780,6 +780,9 @@ class Builder(object):
             raise FeatureLibError(
                 "Script statements are not allowed "
                 "within standalone lookup blocks", location)
+        if self.language_systems == {(script, 'dflt')}:
+            # Nothing to do.
+            return
         self.cur_lookup_ = None
         self.script_ = script
         self.lookupflag_ = 0
diff --git a/Tests/feaLib/data/lookupflag.fea b/Tests/feaLib/data/lookupflag.fea
index ced046b67..1828c43ed 100644
--- a/Tests/feaLib/data/lookupflag.fea
+++ b/Tests/feaLib/data/lookupflag.fea
@@ -134,3 +134,16 @@ feature test {
         pos one 1;
     } V;
 } test;
+
+feature test {
+    lookup W {
+        lookupflag IgnoreMarks;
+        script latn;
+        pos one 1;
+    } W;
+    lookup X {
+        lookupflag IgnoreMarks;
+        script latn;
+        pos two 2;
+    } X;
+} test;
diff --git a/Tests/feaLib/data/lookupflag.ttx b/Tests/feaLib/data/lookupflag.ttx
index 82f1945eb..760eab313 100644
--- a/Tests/feaLib/data/lookupflag.ttx
+++ b/Tests/feaLib/data/lookupflag.ttx
@@ -43,7 +43,7 @@
   <GPOS>
     <Version value="0x00010000"/>
     <ScriptList>
-      <!-- ScriptCount=1 -->
+      <!-- ScriptCount=2 -->
       <ScriptRecord index="0">
         <ScriptTag value="DFLT"/>
         <Script>
@@ -55,9 +55,20 @@
           <!-- LangSysCount=0 -->
         </Script>
       </ScriptRecord>
+      <ScriptRecord index="1">
+        <ScriptTag value="latn"/>
+        <Script>
+          <DefaultLangSys>
+            <ReqFeatureIndex value="65535"/>
+            <!-- FeatureCount=1 -->
+            <FeatureIndex index="0" value="1"/>
+          </DefaultLangSys>
+          <!-- LangSysCount=0 -->
+        </Script>
+      </ScriptRecord>
     </ScriptList>
     <FeatureList>
-      <!-- FeatureCount=1 -->
+      <!-- FeatureCount=2 -->
       <FeatureRecord index="0">
         <FeatureTag value="test"/>
         <Feature>
@@ -86,9 +97,17 @@
           <LookupListIndex index="21" value="21"/>
         </Feature>
       </FeatureRecord>
+      <FeatureRecord index="1">
+        <FeatureTag value="test"/>
+        <Feature>
+          <!-- LookupCount=2 -->
+          <LookupListIndex index="0" value="22"/>
+          <LookupListIndex index="1" value="23"/>
+        </Feature>
+      </FeatureRecord>
     </FeatureList>
     <LookupList>
-      <!-- LookupCount=22 -->
+      <!-- LookupCount=24 -->
       <Lookup index="0">
         <LookupType value="1"/>
         <LookupFlag value="1"/><!-- rightToLeft -->
@@ -357,6 +376,30 @@
           <Value XAdvance="1"/>
         </SinglePos>
       </Lookup>
+      <Lookup index="22">
+        <LookupType value="1"/>
+        <LookupFlag value="0"/>
+        <!-- SubTableCount=1 -->
+        <SinglePos index="0" Format="1">
+          <Coverage>
+            <Glyph value="one"/>
+          </Coverage>
+          <ValueFormat value="4"/>
+          <Value XAdvance="1"/>
+        </SinglePos>
+      </Lookup>
+      <Lookup index="23">
+        <LookupType value="1"/>
+        <LookupFlag value="8"/><!-- ignoreMarks -->
+        <!-- SubTableCount=1 -->
+        <SinglePos index="0" Format="1">
+          <Coverage>
+            <Glyph value="two"/>
+          </Coverage>
+          <ValueFormat value="4"/>
+          <Value XAdvance="2"/>
+        </SinglePos>
+      </Lookup>
     </LookupList>
   </GPOS>