Repository: googlefonts/fontc (rust)

Issue:
fea_rs::compile::compile_ctx ERROR] failed to resolve value record. This indicates a bug.
I’m getting this error trying to build [Mada](https://github.com/aliftype/mada/tree/main/sources) with fontc built from main branch.

It seems to be caused by `pos alefMaksura-ar.fina <0 $ss01 0 0>;` from `ss01` feature, as dropping it causes the message to go away.

Pull request description:
[fea-rs] Fix bug in valuerecord access logic
I'd failed to update one function in light of adding a new possible type of metric (for glyphs number value syntax)


- fixes #948 

Patch:
diff --git a/fea-rs/src/compile/compile_ctx.rs b/fea-rs/src/compile/compile_ctx.rs
index 39a9da56..942599f7 100644
--- a/fea-rs/src/compile/compile_ctx.rs
+++ b/fea-rs/src/compile/compile_ctx.rs
@@ -538,7 +538,7 @@ impl<'a, F: FeatureProvider, V: VariationInfo> CompilationCtx<'a, F, V> {
     }
 
     fn ensure_current_lookup_type(&mut self, kind: Kind) -> &mut SomeLookup {
-        if !self.lookups.has_current_kind(kind) || !self.lookups.has_same_flags(self.lookup_flags) {
+        if !self.lookups.has_current_kind(kind) {
             //FIXME: find another way of ensuring that named lookup blocks don't
             //contain mismatched rules
             //assert!(!self.lookups.is_named(), "ensure rule type in validation");
@@ -598,9 +598,7 @@ impl<'a, F: FeatureProvider, V: VariationInfo> CompilationCtx<'a, F, V> {
             for target in target.iter() {
                 lookup.add_gsub_type_2(target, vec![]);
             }
-        } else if self.lookups.has_current_kind(Kind::GsubType2)
-            && self.lookups.has_same_flags(self.lookup_flags)
-        {
+        } else if self.lookups.has_current_kind(Kind::GsubType2) {
             // we combine chains of mixed single & multi-sub rules into multi-sub lookups
             let lookup = self.ensure_current_lookup_type(Kind::GsubType2);
             for (target, replacement) in target.iter().zip(replacement.into_iter_for_target()) {
diff --git a/fea-rs/src/compile/lookups.rs b/fea-rs/src/compile/lookups.rs
index 11d6604b..d3770cb1 100644
--- a/fea-rs/src/compile/lookups.rs
+++ b/fea-rs/src/compile/lookups.rs
@@ -179,7 +179,7 @@ pub(crate) struct LookupIdMap {
 }
 
 /// Tracks the current lookupflags state
-#[derive(Clone, Copy, Debug, Default, PartialEq)]
+#[derive(Clone, Copy, Debug, Default)]
 pub(crate) struct LookupFlagInfo {
     pub(crate) flags: LookupFlag,
     pub(crate) mark_filter_set: Option<FilterSetId>,
@@ -434,10 +434,6 @@ impl AllLookups {
         self.current.as_ref().map(SomeLookup::kind) == Some(kind)
     }
 
-    pub(crate) fn has_same_flags(&self, flags: LookupFlagInfo) -> bool {
-        self.current.as_ref().map(SomeLookup::flags) == Some(flags)
-    }
-
     // `false` if we didn't have an active lookup
     pub(crate) fn add_subtable_break(&mut self) -> bool {
         if let Some(current) = self.current.as_mut() {
@@ -842,34 +838,6 @@ impl SomeLookup {
         }
     }
 
-    fn flags(&self) -> LookupFlagInfo {
-        match self {
-            SomeLookup::GsubLookup(l) => match l {
-                SubstitutionLookup::Single(l) => LookupFlagInfo::new(l.flags, l.mark_set),
-                SubstitutionLookup::Multiple(l) => LookupFlagInfo::new(l.flags, l.mark_set),
-                SubstitutionLookup::Alternate(l) => LookupFlagInfo::new(l.flags, l.mark_set),
-                SubstitutionLookup::Ligature(l) => LookupFlagInfo::new(l.flags, l.mark_set),
-                SubstitutionLookup::Contextual(l) => LookupFlagInfo::new(l.flags, l.mark_set),
-                SubstitutionLookup::ChainedContextual(l) => {
-                    LookupFlagInfo::new(l.flags, l.mark_set)
-                }
-                SubstitutionLookup::Reverse(l) => LookupFlagInfo::new(l.flags, l.mark_set),
-            },
-            SomeLookup::GposLookup(l) => match l {
-                PositionLookup::Single(l) => LookupFlagInfo::new(l.flags, l.mark_set),
-                PositionLookup::Pair(l) => LookupFlagInfo::new(l.flags, l.mark_set),
-                PositionLookup::Cursive(l) => LookupFlagInfo::new(l.flags, l.mark_set),
-                PositionLookup::MarkToBase(l) => LookupFlagInfo::new(l.flags, l.mark_set),
-                PositionLookup::MarkToLig(l) => LookupFlagInfo::new(l.flags, l.mark_set),
-                PositionLookup::MarkToMark(l) => LookupFlagInfo::new(l.flags, l.mark_set),
-                PositionLookup::Contextual(l) => LookupFlagInfo::new(l.flags, l.mark_set),
-                PositionLookup::ChainedContextual(l) => LookupFlagInfo::new(l.flags, l.mark_set),
-            },
-            SomeLookup::GposContextual(l) => LookupFlagInfo::new(l.flags, l.mark_set),
-            SomeLookup::GsubContextual(l) => LookupFlagInfo::new(l.flags, l.mark_set),
-        }
-    }
-
     pub(crate) fn add_gpos_type_1(&mut self, id: GlyphId16, record: ValueRecord) {
         if let SomeLookup::GposLookup(PositionLookup::Single(table)) = self {
             let subtable = table.last_mut().unwrap();
diff --git a/fea-rs/src/compile/lookups/contextual.rs b/fea-rs/src/compile/lookups/contextual.rs
index 4e88a9a2..1fb839d6 100644
--- a/fea-rs/src/compile/lookups/contextual.rs
+++ b/fea-rs/src/compile/lookups/contextual.rs
@@ -28,8 +28,8 @@ use super::{
 /// bunch of anonymous lookups.
 #[derive(Debug, Clone)]
 pub(crate) struct ContextualLookupBuilder<T> {
-    pub(super) flags: LookupFlag,
-    pub(super) mark_set: Option<FilterSetId>,
+    flags: LookupFlag,
+    mark_set: Option<FilterSetId>,
     subtables: Vec<ContextBuilder>,
     /// anonymous lookups that are not modifiable
     ///
diff --git a/fea-rs/src/parse/grammar/metrics.rs b/fea-rs/src/parse/grammar/metrics.rs
index 4759343a..7faedea2 100644
--- a/fea-rs/src/parse/grammar/metrics.rs
+++ b/fea-rs/src/parse/grammar/metrics.rs
@@ -429,6 +429,8 @@ pub(crate) fn expect_name_record(parser: &mut Parser, recovery: TokenSet) {
 
 #[cfg(test)]
 mod tests {
+    use crate::typed::AstNode;
+
     use super::super::debug_parse_output;
     use super::*;
 
@@ -501,4 +503,17 @@ mod tests {
         });
         assert!(errstr.contains("expected variation location"), "{errstr}");
     }
+
+    // https://github.com/googlefonts/fontc/issues/948
+    #[test]
+    fn glyphs_value_syntax() {
+        let fea = "<0 $hi 0 0>";
+        let (token, _err, _) = debug_parse_output(fea, |parser| {
+            expect_value_record(parser, TokenSet::EMPTY);
+        });
+
+        let valrecord = crate::typed::ValueRecord::cast(&token).unwrap();
+        assert!(valrecord.advance().is_none());
+        assert!(valrecord.placement().is_some());
+    }
 }
diff --git a/fea-rs/src/token_tree/typed.rs b/fea-rs/src/token_tree/typed.rs
index 43e69037..048bdfca 100644
--- a/fea-rs/src/token_tree/typed.rs
+++ b/fea-rs/src/token_tree/typed.rs
@@ -1086,12 +1086,7 @@ impl ValueRecord {
     }
 
     pub(crate) fn placement(&self) -> Option<[Metric; 4]> {
-        if self
-            .iter()
-            .filter(|t| t.kind() == Kind::Number || t.kind() == Kind::VariableMetricNode)
-            .count()
-            == 4
-        {
+        if self.iter().filter_map(Metric::cast).count() == 4 {
             let mut iter = self.iter().filter_map(Metric::cast);
             return Some([
                 iter.next().unwrap(),
diff --git a/fontbe/src/features/marks.rs b/fontbe/src/features/marks.rs
index cc5d4bd3..0387c9fb 100644
--- a/fontbe/src/features/marks.rs
+++ b/fontbe/src/features/marks.rs
@@ -80,11 +80,6 @@ impl MarkGroup<'_> {
             self.marks
                 .iter()
                 .map(|(name, _)| glyph_order.glyph_id(name).unwrap())
-                .chain(
-                    self.bases
-                        .iter()
-                        .map(|(name, _)| glyph_order.glyph_id(name).unwrap()),
-                )
                 .collect()
         })
     }
diff --git a/fontc/src/lib.rs b/fontc/src/lib.rs
index 35d011f5..2792361a 100644
--- a/fontc/src/lib.rs
+++ b/fontc/src/lib.rs
@@ -608,7 +608,6 @@ mod tests {
             },
             TableRef,
         },
-        tables::layout::LookupFlag,
         types::NameId,
     };
 
@@ -2657,12 +2656,10 @@ mod tests {
         let result = TestCompile::compile_source("glyphs3/Oswald-AE-comb.glyphs");
         let font = result.font();
         let gpos = font.gpos().unwrap();
-        let gdef = font.gdef().unwrap();
         let acutecomb = result.get_gid("acutecomb");
         let brevecomb = result.get_gid("brevecomb");
         let tildecomb = result.get_gid("tildecomb");
         let macroncomb = result.get_gid("macroncomb");
-        let breveinvertedcomb = result.get_gid("breveinvertedcomb");
 
         let mark_mark_lookups = gpos
             .lookup_list()
@@ -2673,20 +2670,14 @@ mod tests {
                 Ok(PositionLookup::MarkToMark(lookup)) => Some(lookup),
                 _ => None,
             })
-            .collect::<Vec<_>>();
-        assert_eq!(mark_mark_lookups.len(), 2);
-
-        let mark_mark_lookup = mark_mark_lookups.first().unwrap();
-        let mark_mark_subtables = mark_mark_lookup
-            .subtables()
-            .iter()
-            .map(|s| s.unwrap())
-            .collect::<Vec<_>>();
-        assert_eq!(mark_mark_subtables.len(), 1);
+            .flat_map(|lookup| lookup.subtables().iter())
+            .collect::<Result<Vec<_>, _>>()
+            .unwrap();
+        assert_eq!(mark_mark_lookups.len(), 1);
 
         let mut bases = Vec::new();
         let mut marks = Vec::new();
-        for sub in &mark_mark_subtables {
+        for sub in &mark_mark_lookups {
             let mark_cov = sub.mark1_coverage().unwrap();
             let base_cov = sub.mark2_coverage().unwrap();
             let mark1array = sub.mark1_array().unwrap();
@@ -2736,38 +2727,7 @@ mod tests {
                 (acutecomb, vec![(0, 810)]),
                 (brevecomb, vec![(-1, 776)]),
                 (tildecomb, vec![(0, 776)]),
-                (macroncomb, vec![(0, 810)]),
-                (breveinvertedcomb, vec![(-1, 776)]),
-            ]
-        );
-
-        let mark_filter_id = mark_mark_lookup
-            .lookup_flag()
-            .contains(LookupFlag::USE_MARK_FILTERING_SET)
-            .then_some(mark_mark_lookup.mark_filtering_set())
-            .flatten();
-        assert_eq!(mark_filter_id, Some(0));
-
-        let mark_filter_set = gdef
-            .mark_glyph_sets_def()
-            .unwrap()
-            .as_ref()
-            .map(|sets| {
-                sets.coverages()
-                    .get(mark_filter_id.unwrap().into())
-                    .unwrap()
-            })
-            .map(|cov| cov.iter().collect::<Vec<_>>())
-            .unwrap();
-
-        assert_eq!(
-            mark_filter_set,
-            vec![
-                acutecomb,
-                brevecomb,
-                tildecomb,
-                macroncomb,
-                breveinvertedcomb
+                (macroncomb, vec![(0, 810)])
             ]
         );
     }
diff --git a/fontc_crater/src/main.rs b/fontc_crater/src/main.rs
index 05c709df..6391ff8c 100644
--- a/fontc_crater/src/main.rs
+++ b/fontc_crater/src/main.rs
@@ -10,7 +10,7 @@ use std::{
 use clap::Parser;
 use fontc::JobTimer;
 use google_fonts_sources::{LoadRepoError, RepoInfo};
-use rayon::{prelude::*, ThreadPoolBuilder};
+use rayon::prelude::*;
 
 mod args;
 mod ci;
@@ -201,27 +201,19 @@ fn run_all<T: Send, E: Send>(
     }
     let total_targets = targets.len();
     let counter = AtomicUsize::new(0);
-    let currently_running = AtomicUsize::new(0);
-    let threadpool = ThreadPoolBuilder::new().build().unwrap();
-
-    let results = threadpool.install(|| {
-        targets
-            .into_par_iter()
-            .map(|target| {
-                let i = counter.fetch_add(1, Ordering::Relaxed) + 1;
-                currently_running.fetch_add(1, Ordering::Relaxed);
-                eprintln!("starting {} ({i}/{total_targets})", target.display());
-                let r = runner(&target);
-                let n_running = currently_running.fetch_sub(1, Ordering::Relaxed);
-                eprintln!("finished {} ({n_running} active)", target.display());
-                let target = target
-                    .strip_prefix(cache_dir)
-                    .unwrap_or(target.as_path())
-                    .to_path_buf();
-                (target, r)
-            })
-            .collect::<Vec<_>>()
-    });
+    let results = targets
+        .into_par_iter()
+        .map(|target| {
+            let i = counter.fetch_add(1, Ordering::Relaxed);
+            eprintln!("running {} ({i}/{total_targets})", target.display());
+            let r = runner(&target);
+            let target = target
+                .strip_prefix(cache_dir)
+                .unwrap_or(target.as_path())
+                .to_path_buf();
+            (target, r)
+        })
+        .collect::<Vec<_>>();
     Ok(results.into_iter().chain(skipped).collect())
 }
 
diff --git a/resources/scripts/ttx_diff.py b/resources/scripts/ttx_diff.py
index 4fa3bf7a..5fa5828a 100755
--- a/resources/scripts/ttx_diff.py
+++ b/resources/scripts/ttx_diff.py
@@ -244,22 +244,13 @@ def name_id_to_name(ttx, xpath, attr):
         )
     }
     for el in ttx.xpath(xpath):
-        if attr is None:
-            if el.text is None:
-                continue
-            name_id = int(el.text)
-            # names <= 255 have specific assigned slots, names > 255 not
-            if name_id <= 255:
-                continue
-            el.text = id_to_name[el.text]
-        else:
-            if attr not in el.attrib:
-                continue
-            # names <= 255 have specific assigned slots, names > 255 not
-            name_id = int(el.attrib[attr])
-            if name_id <= 255:
-                continue
-            el.attrib[attr] = id_to_name[el.attrib[attr]]
+        if attr not in el.attrib:
+            continue
+        # names <= 255 have specific assigned slots, names > 255 not
+        name_id = int(el.attrib[attr])
+        if name_id <= 255:
+            continue
+        el.attrib[attr] = id_to_name[el.attrib[attr]]
 
 
 def find_table(ttx, tag):
@@ -392,8 +383,6 @@ def reduce_diff_noise(build_dir, fontc, fontmake):
     for ttx in (fontc, fontmake):
         # different name ids with the same value is fine
         name_id_to_name(ttx, "//NamedInstance", "subfamilyNameID")
-        name_id_to_name(ttx, "//AxisNameID", "value")
-        name_id_to_name(ttx, "//AxisNameID", None)
 
         # deal with https://github.com/googlefonts/fontmake/issues/1003
         drop_weird_names(ttx)


Test patch:
diff --git a/fea-rs/test-data/compile-tests/mini-latin/good/anon_lookup_flag.fea b/fea-rs/test-data/compile-tests/mini-latin/good/anon_lookup_flag.fea
deleted file mode 100644
index d8d74694..00000000
--- a/fea-rs/test-data/compile-tests/mini-latin/good/anon_lookup_flag.fea
+++ /dev/null
@@ -1,7 +0,0 @@
-feature test {
-  lookupflag IgnoreMarks;
-    sub b' by c;
-
-  lookupflag UseMarkFilteringSet [acutecomb];
-    sub a' acutecomb by b;
-} test;
diff --git a/fea-rs/test-data/compile-tests/mini-latin/good/anon_lookup_flag.ttx b/fea-rs/test-data/compile-tests/mini-latin/good/anon_lookup_flag.ttx
deleted file mode 100644
index 07aa7011..00000000
--- a/fea-rs/test-data/compile-tests/mini-latin/good/anon_lookup_flag.ttx
+++ /dev/null
@@ -1,102 +0,0 @@
-<?xml version="1.0" encoding="UTF-8"?>
-<ttFont>
-
-  <GDEF>
-    <Version value="0x00010002"/>
-    <MarkGlyphSetsDef>
-      <MarkSetTableFormat value="1"/>
-      <!-- MarkSetCount=1 -->
-      <Coverage index="0">
-        <Glyph value="acutecomb"/>
-      </Coverage>
-    </MarkGlyphSetsDef>
-  </GDEF>
-
-  <GSUB>
-    <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="test"/>
-        <Feature>
-          <!-- LookupCount=2 -->
-          <LookupListIndex index="0" value="0"/>
-          <LookupListIndex index="1" value="2"/>
-        </Feature>
-      </FeatureRecord>
-    </FeatureList>
-    <LookupList>
-      <!-- LookupCount=4 -->
-      <Lookup index="0">
-        <LookupType value="5"/>
-        <LookupFlag value="8"/><!-- ignoreMarks -->
-        <!-- SubTableCount=1 -->
-        <ContextSubst index="0" Format="3">
-          <!-- GlyphCount=1 -->
-          <!-- SubstCount=1 -->
-          <Coverage index="0">
-            <Glyph value="b"/>
-          </Coverage>
-          <SubstLookupRecord index="0">
-            <SequenceIndex value="0"/>
-            <LookupListIndex value="1"/>
-          </SubstLookupRecord>
-        </ContextSubst>
-      </Lookup>
-      <Lookup index="1">
-        <LookupType value="1"/>
-        <LookupFlag value="8"/><!-- ignoreMarks -->
-        <!-- SubTableCount=1 -->
-        <SingleSubst index="0">
-          <Substitution in="b" out="c"/>
-        </SingleSubst>
-      </Lookup>
-      <Lookup index="2">
-        <LookupType value="6"/>
-        <LookupFlag value="16"/><!-- useMarkFilteringSet -->
-        <!-- SubTableCount=1 -->
-        <ChainContextSubst index="0" Format="3">
-          <!-- BacktrackGlyphCount=0 -->
-          <!-- InputGlyphCount=1 -->
-          <InputCoverage index="0">
-            <Glyph value="a"/>
-          </InputCoverage>
-          <!-- LookAheadGlyphCount=1 -->
-          <LookAheadCoverage index="0">
-            <Glyph value="acutecomb"/>
-          </LookAheadCoverage>
-          <!-- SubstCount=1 -->
-          <SubstLookupRecord index="0">
-            <SequenceIndex value="0"/>
-            <LookupListIndex value="3"/>
-          </SubstLookupRecord>
-        </ChainContextSubst>
-        <MarkFilteringSet value="0"/>
-      </Lookup>
-      <Lookup index="3">
-        <LookupType value="1"/>
-        <LookupFlag value="16"/><!-- useMarkFilteringSet -->
-        <!-- SubTableCount=1 -->
-        <SingleSubst index="0">
-          <Substitution in="a" out="b"/>
-        </SingleSubst>
-        <MarkFilteringSet value="0"/>
-      </Lookup>
-    </LookupList>
-  </GSUB>
-
-</ttFont>
diff --git a/resources/testdata/glyphs3/Oswald-AE-comb.glyphs b/resources/testdata/glyphs3/Oswald-AE-comb.glyphs
index 34daafd9..7ed97397 100644
--- a/resources/testdata/glyphs3/Oswald-AE-comb.glyphs
+++ b/resources/testdata/glyphs3/Oswald-AE-comb.glyphs
@@ -1735,87 +1735,6 @@ width = 278;
 };
 };
 };
-},
-{
-glyphname = breveinvertedcomb;
-layers = (
-{
-anchors = (
-{
-name = _top.a;
-pos = (0,578);
-},
-{
-name = top;
-pos = (0,810);
-},
-{
-name = top.a;
-pos = (0,810);
-}
-);
-layerId = "FCD7E481-103E-4A6C-AD70-01DA443B2AD0";
-shapes = (
-{
-pos = (0,1389);
-ref = brevecomb;
-scale = (1,-1);
-}
-);
-width = 100;
-},
-{
-anchors = (
-{
-name = _top.a;
-pos = (0,578);
-},
-{
-name = top;
-pos = (-1,776);
-},
-{
-name = top.a;
-pos = (0,776);
-}
-);
-layerId = "8AC5A1D3-32A8-4416-86FC-94176A419B8F";
-shapes = (
-{
-pos = (0,1417);
-ref = brevecomb;
-scale = (1,-1);
-}
-);
-width = 0;
-},
-{
-anchors = (
-{
-name = _top.a;
-pos = (0,578);
-},
-{
-name = top;
-pos = (0,802);
-},
-{
-name = top.a;
-pos = (0,802);
-}
-);
-layerId = "F66B25F0-8957-42F3-9BF7-6C5DDCB0258B";
-shapes = (
-{
-pos = (0,1442);
-ref = brevecomb;
-scale = (1,-1);
-}
-);
-width = 0;
-}
-);
-unicode = 785;
 }
 );
 instances = (