Improve PairPos validation

font-codegen/README.md

@@ -159,6 +159,9 @@ The following annotations are supported on top-level objects:
   between GPOS and GSUB.
 - `#[write_fonts_only]` Indicate that this table should only be generated for
   `write-fonts` (i.e. should be ignored in `read-fonts`).
+- `#[validate(method)]` Provide a method to perform additional pre-compilation
+  validation for this type. The method must be manually implemented on the type,
+  with the signature `fn(&self, &mut ValidationCtx)`.
 
 #### field attributes
 - `#[nullable]`: only allowed on offsets or arrays of offsets, and indicates

resources/codegen_inputs/gpos.rs

@@ -201,6 +201,7 @@ table PairPosFormat1 {
     /// of PairPos subtable, ordered by Coverage Index.
     #[count($pair_set_count)]
     #[read_offset_with($value_format1, $value_format2)]
+    #[validate(check_format_consistency)]
     pair_set_offsets: [Offset16<PairSet>],
 }
 
@@ -232,6 +233,7 @@ record PairValueRecord {
 }
 
 /// [Pair Adjustment Positioning Format 2](https://docs.microsoft.com/en-us/typography/opentype/spec/gpos#pair-adjustment-positioning-format-2-class-pair-adjustment): Class Pair Adjustment
+#[validate(check_length_and_format_conformance)]
 table PairPosFormat2 {
     /// Format identifier: format = 2
     #[format = 2]

write-fonts/generated/generated_gpos.rs

@@ -953,7 +953,7 @@ impl Validate for PairPosFormat1 {
                 if self.pair_sets.len() > (u16::MAX as usize) {
                     ctx.report("array exceeds max length");
                 }
-                self.pair_sets.validate_impl(ctx);
+                self.check_format_consistency(ctx);
             });
         })
     }
@@ -1154,6 +1154,7 @@ impl Validate for PairPosFormat2 {
                 }
                 self.class1_records.validate_impl(ctx);
             });
+            self.check_length_and_format_conformance(ctx);
         })
     }
 }

write-fonts/src/tables/gpos.rs

@@ -107,6 +107,27 @@ impl PairPosFormat1 {
             .map(|rec| rec.value_record2.format())
             .unwrap_or(ValueFormat::empty())
     }
+
+    fn check_format_consistency(&self, ctx: &mut ValidationCtx) {
+        let vf1 = self.compute_value_format1();
+        let vf2 = self.compute_value_format2();
+        ctx.in_array(|ctx| {
+            for pairset in &self.pair_sets {
+                ctx.array_item(|ctx| {
+                    ctx.in_field("pair_value_records", |ctx| {
+                        ctx.in_array(|ctx| {
+                            if pairset.pair_value_records.iter().any(|pairset| {
+                                pairset.value_record1.format() != vf1
+                                    || pairset.value_record2.format() != vf2
+                            }) {
+                                ctx.report("all ValueRecords must have same format")
+                            }
+                        })
+                    })
+                })
+            }
+        })
+    }
 }
 
 impl PairPosFormat2 {
@@ -133,6 +154,35 @@ impl PairPosFormat2 {
     fn compute_class2_count(&self) -> u16 {
         self.class_def2.class_count()
     }
+
+    fn check_length_and_format_conformance(&self, ctx: &mut ValidationCtx) {
+        let n_class_1s = self.class_def1.class_count();
+        let n_class_2s = self.class_def2.class_count();
+        let format_1 = self.compute_value_format1();
+        let format_2 = self.compute_value_format2();
+        if self.class1_records.len() != n_class_1s as usize {
+            ctx.report("class1_records length must match number of class1 classes");
+        }
+        ctx.in_field("class1_records", |ctx| {
+            ctx.in_array(|ctx| {
+                for record in &self.class1_records {
+                    ctx.array_item(|ctx| {
+                        if record.class2_records.len() != n_class_2s as usize {
+                            ctx.report(
+                                "class2_records length must match number of class2 classes ",
+                            );
+                        }
+                        if record.class2_records.iter().any(|rec| {
+                            rec.value_record1.format() != format_1
+                                || rec.value_record2.format() != format_2
+                        }) {
+                            ctx.report("all value records should report the same format");
+                        }
+                    })
+                }
+            })
+        });
+    }
 }
 
 impl MarkBasePosFormat1 {
@@ -168,6 +218,8 @@ mod tests {
 
     use read_fonts::tables::{gpos as read_gpos, layout::LookupFlag};
 
+    use crate::tables::layout::VariationIndex;
+
     use super::*;
 
     // adapted from/motivated by https://github.com/fonttools/fonttools/issues/471
@@ -206,4 +258,124 @@ mod tests {
             }
         );
     }
+
+    // shared between a pair of tests below
+    fn make_rec(i: u16) -> ValueRecord {
+        // '0' here is shorthand for 'no device table'
+        if i == 0 {
+            return ValueRecord::new().with_explicit_value_format(ValueFormat::X_ADVANCE_DEVICE);
+        }
+        ValueRecord::new().with_x_advance_device(VariationIndex::new(0xff, i))
+    }
+
+    #[test]
+    fn compile_devices_pairpos2() {
+        let class1 = ClassDef::from_iter([(GlyphId::new(5), 0), (GlyphId::new(6), 1)]);
+        // class 0 is 'all the rest', here, always implicitly present
+        let class2 = ClassDef::from_iter([(GlyphId::new(8), 1)]);
+
+        // two c1recs, each with two c2recs
+        let class1recs = vec![
+            Class1Record::new(vec![
+                Class2Record::new(make_rec(0), make_rec(0)),
+                Class2Record::new(make_rec(1), make_rec(2)),
+            ]),
+            Class1Record::new(vec![
+                Class2Record::new(make_rec(0), make_rec(0)),
+                Class2Record::new(make_rec(2), make_rec(3)),
+            ]),
+        ];
+        let coverage = class1.iter().map(|(gid, _)| gid).collect();
+        let a_table = PairPos::format_2(coverage, class1, class2, class1recs);
+
+        let bytes = crate::dump_table(&a_table).unwrap();
+        let read_back = PairPosFormat2::read(bytes.as_slice().into()).unwrap();
+
+        assert!(read_back.class1_records[0].class2_records[0]
+            .value_record1
+            .x_advance_device
+            .is_none());
+        assert!(read_back.class1_records[1].class2_records[1]
+            .value_record1
+            .x_advance_device
+            .is_some());
+
+        let DeviceOrVariationIndex::VariationIndex(dev2) = read_back.class1_records[0]
+            .class2_records[1]
+            .value_record2
+            .x_advance_device
+            .as_ref()
+            .unwrap()
+        else {
+            panic!("not a variation index")
+        };
+        assert_eq!(dev2.delta_set_inner_index, 2);
+    }
+
+    #[should_panic(expected = "all value records should report the same format")]
+    #[test]
+    fn validate_bad_pairpos2() {
+        let class1 = ClassDef::from_iter([(GlyphId::new(5), 0), (GlyphId::new(6), 1)]);
+        // class 0 is 'all the rest', here, always implicitly present
+        let class2 = ClassDef::from_iter([(GlyphId::new(8), 1)]);
+        let coverage = class1.iter().map(|(gid, _)| gid).collect();
+
+        // two c1recs, each with two c2recs
+        let class1recs = vec![
+            Class1Record::new(vec![
+                Class2Record::new(make_rec(0), make_rec(0)),
+                Class2Record::new(make_rec(1), make_rec(2)),
+            ]),
+            Class1Record::new(vec![
+                Class2Record::new(make_rec(0), make_rec(0)),
+                // this is now the wrong type
+                Class2Record::new(make_rec(2), make_rec(3).with_x_advance(0x514)),
+            ]),
+        ];
+        let ppf2 = PairPos::format_2(coverage, class1, class2, class1recs);
+        crate::dump_table(&ppf2).unwrap();
+    }
+
+    #[test]
+    fn validate_pairpos1() {
+        let coverage: CoverageTable = [1, 2].into_iter().map(GlyphId::new).collect();
+        let good_table = PairPosFormat1::new(
+            coverage.clone(),
+            vec![
+                PairSet::new(vec![PairValueRecord::new(
+                    GlyphId::new(5),
+                    ValueRecord::new().with_x_advance(5),
+                    ValueRecord::new(),
+                )]),
+                PairSet::new(vec![PairValueRecord::new(
+                    GlyphId::new(1),
+                    ValueRecord::new().with_x_advance(42),
+                    ValueRecord::new(),
+                )]),
+            ],
+        );
+
+        let bad_table = PairPosFormat1::new(
+            coverage,
+            vec![
+                PairSet::new(vec![PairValueRecord::new(
+                    GlyphId::new(5),
+                    ValueRecord::new().with_x_advance(5),
+                    ValueRecord::new(),
+                )]),
+                PairSet::new(vec![PairValueRecord::new(
+                    GlyphId::new(1),
+                    //this is a different format, which is not okay
+                    ValueRecord::new().with_x_placement(42),
+                    ValueRecord::new(),
+                )]),
+            ],
+        );
+
+        assert!(crate::dump_table(&good_table).is_ok());
+        assert!(matches!(
+            crate::dump_table(&bad_table),
+            Err(crate::error::Error::ValidationFailed(_))
+        ));
+    }
 }

write-fonts/src/tables/layout.rs

@@ -454,6 +454,12 @@ impl FromIterator<(GlyphId, u16)> for ClassDefBuilder {
     }
 }
 
+impl FromIterator<(GlyphId, u16)> for ClassDef {
+    fn from_iter<T: IntoIterator<Item = (GlyphId, u16)>>(iter: T) -> Self {
+        ClassDefBuilder::from_iter(iter).build()
+    }
+}
+
 impl ClassDefBuilder {
     fn prefer_format_1(&self) -> bool {
         // calculate our format2 size:

write-fonts/src/tables/value_record.rs

@@ -227,8 +227,8 @@ mod tests {
     use read_fonts::FontRead;
 
     use crate::tables::{
-        gpos::{Class1Record, Class2Record, PairPos, PairPosFormat2, SinglePos, SinglePosFormat1},
-        layout::{ClassDefBuilder, CoverageTableBuilder, VariationIndex},
+        gpos::{SinglePos, SinglePosFormat1},
+        layout::{CoverageTableBuilder, VariationIndex},
     };
 
     use super::*;
@@ -261,34 +261,4 @@ mod tests {
             matches!(read_back.value_record.x_advance_device.as_ref(), Some(DeviceOrVariationIndex::VariationIndex(var_idx)) if var_idx.delta_set_inner_index == 0xee)
         )
     }
-
-    #[test]
-    fn compile_devices_pairpos2() {
-        let rec1 = ValueRecord::new().with_x_advance_device(VariationIndex::new(0xff, 0xee));
-        let rec2 = ValueRecord::new().with_x_advance_device(VariationIndex::new(0xaa, 0xbb));
-        let class1 =
-            ClassDefBuilder::from_iter([(GlyphId::new(5), 2), (GlyphId::new(6), 2)]).build();
-        let class2 =
-            ClassDefBuilder::from_iter([(GlyphId::new(8), 3), (GlyphId::new(9), 3)]).build();
-        let class1recs = vec![Class1Record::new(vec![Class2Record::new(rec1, rec2)])];
-        let a_table = PairPos::format_2(
-            CoverageTableBuilder::from_glyphs(vec![GlyphId::new(42)]).build(),
-            class1,
-            class2,
-            class1recs,
-        );
-
-        let bytes = crate::dump_table(&a_table).unwrap();
-        let read_back = PairPosFormat2::read(bytes.as_slice().into()).unwrap();
-        let DeviceOrVariationIndex::VariationIndex(dev2) = read_back.class1_records[0]
-            .class2_records[0]
-            .value_record2
-            .x_advance_device
-            .as_ref()
-            .unwrap()
-        else {
-            panic!("not a variation index")
-        };
-        assert_eq!(dev2.delta_set_outer_index, 0xaa);
-    }
 }