[fea-rs] Better handling of featureNames & cvParam

Previously we would error if there were more than one of these blocks,
but the spec merely says that they must occur before any rules.

This now supports multiple featureNames blocks, and matches the
behaviour of fonttools (last writer wins).

For CvParameters, we will only look at the first such block, and skip
the rest; no source in our collection has multiple such blocks defined
within a given feature, and updating the logic is more complicated so I
would like to defer it for now.

fea-rs/src/compile/compile_ctx.rs

@@ -1362,71 +1362,85 @@ impl<'a, F: FeatureProvider, V: VariationInfo> CompilationCtx<'a, F, V> {
     }
 
     fn resolve_stylistic_set_feature(&mut self, tag: Tag, feature: &typed::Feature) {
-        let mut names = Vec::new();
-        if let Some(feature_name) = feature.stylistic_set_feature_names() {
-            for name_spec in feature_name.statements() {
-                let resolved = self.resolve_name_spec(&name_spec);
-                names.push(resolved);
+        let mut names = BTreeMap::new();
+        for item in feature.statements() {
+            if let Some(feature_name) = typed::FeatureNames::cast(item) {
+                for name_spec in feature_name.statements() {
+                    let resolved = self.resolve_name_spec(&name_spec);
+                    // in the case of duplicate names, last writer wins, see
+                    // https://github.com/googlefonts/fontc/issues/1019
+                    let key = resolved.key();
+                    names.insert(key, resolved);
+                }
+                continue;
             }
+            self.resolve_statement(item);
         }
         if !names.is_empty() {
-            self.features.stylistic_sets.insert(tag, names);
-        }
-        for item in feature
-            .statements()
-            .filter(|node| node.kind() != Kind::FeatureNamesKw)
-        {
-            self.resolve_statement(item);
+            self.features
+                .stylistic_sets
+                .insert(tag, names.into_values().collect());
         }
     }
 
     fn resolve_character_variant_feature(&mut self, tag: Tag, feature: &typed::Feature) {
-        if let Some(cv_params) = feature.character_variant_params() {
-            let mut params = CvParams::default();
-            if let Some(node) = cv_params.feat_ui_label_name() {
-                params.feat_ui_label_name = node
-                    .statements()
-                    .map(|x| self.resolve_name_spec(&x))
-                    .collect();
-            }
-            if let Some(node) = cv_params.feat_tooltip_text_name() {
-                params.feat_ui_tooltip_text_name = node
-                    .statements()
-                    .map(|x| self.resolve_name_spec(&x))
-                    .collect();
-            }
-            if let Some(node) = cv_params.sample_text_name() {
-                params.sample_text_name = node
-                    .statements()
-                    .map(|x| self.resolve_name_spec(&x))
-                    .collect();
-            }
-            if let Some(node) = cv_params.sample_text_name() {
-                params.sample_text_name = node
-                    .statements()
-                    .map(|x| self.resolve_name_spec(&x))
-                    .collect();
-            }
-            for node in cv_params.param_ui_label_name() {
-                params.param_ui_label_names.push(
-                    node.statements()
-                        .map(|x| self.resolve_name_spec(&x))
-                        .collect(),
-                );
-            }
-            for c in cv_params.characters() {
-                params.characters.push(c.value().parse_char().unwrap());
+        let mut seen_cv_params = false;
+        for item in feature.statements() {
+            if let Some(cv_params) = typed::CvParameters::cast(item) {
+                if seen_cv_params {
+                    self.warning(
+                        cv_params.range(),
+                        "Duplicate cvParameters block will be ignored. \
+                        This is not disallowed by the spec, but is not currently supported.",
+                    );
+                } else {
+                    seen_cv_params = true;
+                    let params = self.resolve_cv_params(&cv_params);
+                    self.features.character_variants.insert(tag, params);
+                }
+                continue;
             }
-
-            self.features.character_variants.insert(tag, params);
+            self.resolve_statement(item);
         }
+    }
 
-        for item in feature
-            .statements()
-            .filter(|node| node.kind() != Kind::CvParametersKw)
-        {
-            self.resolve_statement(item);
+    fn resolve_cv_params(&mut self, cv_params: &typed::CvParameters) -> CvParams {
+        let mut params = CvParams::default();
+        if let Some(node) = cv_params.feat_ui_label_name() {
+            params.feat_ui_label_name = node
+                .statements()
+                .map(|x| self.resolve_name_spec(&x))
+                .collect();
+        }
+        if let Some(node) = cv_params.feat_tooltip_text_name() {
+            params.feat_ui_tooltip_text_name = node
+                .statements()
+                .map(|x| self.resolve_name_spec(&x))
+                .collect();
+        }
+        if let Some(node) = cv_params.sample_text_name() {
+            params.sample_text_name = node
+                .statements()
+                .map(|x| self.resolve_name_spec(&x))
+                .collect();
+        }
+        if let Some(node) = cv_params.sample_text_name() {
+            params.sample_text_name = node
+                .statements()
+                .map(|x| self.resolve_name_spec(&x))
+                .collect();
+        }
+        for node in cv_params.param_ui_label_name() {
+            params.param_ui_label_names.push(
+                node.statements()
+                    .map(|x| self.resolve_name_spec(&x))
+                    .collect(),
+            );
+        }
+        for c in cv_params.characters() {
+            params.characters.push(c.value().parse_char().unwrap());
         }
+        params
     }
 
     fn resolve_size_feature(&mut self, feature: &typed::Feature) {

fea-rs/src/compile/tables/name.rs

@@ -70,6 +70,12 @@ impl NameSpec {
         Encoding::new(self.platform_id, self.encoding_id) != Encoding::Unknown
     }
 
+    // used to ensure we only choose one name for a given platform/encoding
+    // when multiple are provided
+    pub(crate) fn key(&self) -> (u16, u16, u16) {
+        (self.platform_id, self.encoding_id, self.language_id)
+    }
+
     pub fn build(&self, name_id: NameId) -> write_fonts::tables::name::NameRecord {
         let string = parse_string(self.platform_id, self.string.trim_matches('"'));
         write_fonts::tables::name::NameRecord::new(

fea-rs/src/compile/validate.rs

@@ -599,7 +599,7 @@ impl<'a, V: VariationInfo> ValidationCtx<'a, V> {
         feature_tag: Tag,
         iter: impl Iterator<Item = &'b NodeOrToken>,
     ) {
-        let mut is_first_item = true;
+        let mut has_seen_rule = false;
         for item in iter {
             if item.kind() == Kind::ScriptNode
                 || item.kind() == Kind::LanguageNode
@@ -613,10 +613,10 @@ impl<'a, V: VariationInfo> ValidationCtx<'a, V> {
                         node.keyword().range(),
                         "cvParameters block only valid in cv01-cv99 features",
                     );
-                } else if !is_first_item {
+                } else if has_seen_rule {
                     self.error(
                         node.keyword().range(),
-                        "cvParameters must be first statement in feature block",
+                        "cvParameters must precede any rules",
                     );
                 } else {
                     self.validate_character_variant_items(&node);
@@ -627,24 +627,28 @@ impl<'a, V: VariationInfo> ValidationCtx<'a, V> {
                         node.keyword().range(),
                         "featureNames block only valid in ss01-ss20 features",
                     );
-                } else if !is_first_item {
+                } else if has_seen_rule {
                     self.error(
                         node.keyword().range(),
-                        "featureNames must be first statement in feature block",
+                        "featureNames must precede any rules",
                     );
                 } else {
                     self.validate_stylistic_set_items(&node);
                 }
             } else if let Some(node) = typed::LookupRef::cast(item) {
                 self.validate_lookup_ref(&node);
+                has_seen_rule = true;
             } else if let Some(node) = typed::LookupBlock::cast(item) {
                 self.validate_lookup_block(&node, Some(feature_tag));
+                has_seen_rule = true;
             } else if let Some(node) = typed::LookupFlag::cast(item) {
                 self.validate_lookupflag(&node);
             } else if let Some(node) = typed::GsubStatement::cast(item) {
                 self.validate_gsub_statement(&node);
+                has_seen_rule = true;
             } else if let Some(node) = typed::GposStatement::cast(item) {
                 self.validate_gpos_statement(&node);
+                has_seen_rule = true;
             } else if let Some(node) = typed::GlyphClassDef::cast(item) {
                 self.validate_glyph_class_def(&node);
             } else if let Some(node) = typed::MarkClassDef::cast(item) {
@@ -662,7 +666,6 @@ impl<'a, V: VariationInfo> ValidationCtx<'a, V> {
                     format!("unhandled item '{}' in feature", item.kind()),
                 );
             }
-            is_first_item = false;
         }
     }
 

fea-rs/src/token_tree/typed.rs

@@ -671,14 +671,6 @@ impl Feature {
         self.iter().find_map(Tag::cast).unwrap()
     }
 
-    pub(crate) fn stylistic_set_feature_names(&self) -> Option<FeatureNames> {
-        self.statements().next().and_then(FeatureNames::cast)
-    }
-
-    pub(crate) fn character_variant_params(&self) -> Option<CvParameters> {
-        self.statements().next().and_then(CvParameters::cast)
-    }
-
     pub(crate) fn statements(&self) -> impl Iterator<Item = &NodeOrToken> {
         self.iter()
             .skip_while(|t| t.kind() != Kind::LBrace)

fea-rs/test-data/compile-tests/mini-latin/bad/cv_params.ERR

@@ -4,7 +4,7 @@ in ./test-data/compile-tests/mini-latin/bad/cv_params.fea at 3:4
 3 |     cvParameters {
   |     ^^^^^^^^^^^^
 
-error: cvParameters must be first statement in feature block
+error: cvParameters must precede any rules
 in ./test-data/compile-tests/mini-latin/bad/cv_params.fea at 9:4
   | 
 9 |     cvParameters {

fea-rs/test-data/compile-tests/mini-latin/bad/stylistic_set_names.ERR

@@ -4,7 +4,7 @@ in ./test-data/compile-tests/mini-latin/bad/stylistic_set_names.fea at 3:4
 3 |     featureNames {
   |     ^^^^^^^^^^^^
 
-error: featureNames must be first statement in feature block
+error: featureNames must precede any rules
 in ./test-data/compile-tests/mini-latin/bad/stylistic_set_names.fea at 10:4
    | 
 10 |     featureNames {

fea-rs/test-data/compile-tests/mini-latin/good/duplicate_feature_names.fea

@@ -0,0 +1,9 @@
+# https://github.com/googlefonts/fontc/issues/1019
+feature ss01 {
+    featureNames {
+        name "ss17";
+    };
+    featureNames {
+        name "Bulgarian letters";
+    };
+} ss01;

fea-rs/test-data/compile-tests/mini-latin/good/duplicate_feature_names.ttx

@@ -0,0 +1,10 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<ttFont>
+
+  <name>
+    <namerecord nameID="256" platformID="3" platEncID="1" langID="0x409">
+      Bulgarian letters
+    </namerecord>
+  </name>
+
+</ttFont>