[write-fonts] Improve validation API

This significantly reduces indent drift when dealing with arrays;
where previously you needed to indent 1) to start an array, 2) to manually loop
and 3) to declare each item, we now have a single method that lets you
pass in an iterator and a closure to be called on each item, and we
handle the rest.

The diff shows that this is significantly less painful.

write-fonts/src/tables/gpos.rs

@@ -111,21 +111,14 @@ impl PairPosFormat1 {
     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")
-                            }
-                        })
-                    })
-                })
-            }
+        ctx.with_array_items(self.pair_sets.iter(), |ctx, item| {
+            ctx.in_field("pair_value_records", |ctx| {
+                if item.pair_value_records.iter().any(|pairset| {
+                    pairset.value_record1.format() != vf1 || pairset.value_record2.format() != vf2
+                }) {
+                    ctx.report("all ValueRecords must have same format")
+                }
+            })
         })
     }
 }
@@ -164,21 +157,14 @@ impl PairPosFormat2 {
             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");
-                        }
-                    })
+            ctx.with_array_items(self.class1_records.iter(), |ctx, c1rec| {
+                if c1rec.class2_records.len() != n_class_2s as usize {
+                    ctx.report("class2_records length must match number of class2 classes ");
+                }
+                if c1rec.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");
                 }
             })
         });

write-fonts/src/validate.rs

@@ -105,23 +105,17 @@ impl ValidationCtx {
         self.with_elem(LocationElem::Field(name), f);
     }
 
-    /// Run the provided closer in the context of an array.
-    pub fn in_array(&mut self, f: impl FnOnce(&mut ValidationCtx)) {
-        self.with_elem(LocationElem::Index(0), f);
-    }
-
-    /// Run the provided closer in the context of a new array item.
+    /// Run the provided closer for each item in an array.
     ///
-    /// This must only be called in a closure passed to [in_array][Self::in_array].
-    pub fn array_item(&mut self, f: impl FnOnce(&mut ValidationCtx)) {
-        assert!(matches!(
-            self.cur_location.last(),
-            Some(LocationElem::Index(_))
-        ));
-        f(self);
-        match self.cur_location.last_mut() {
-            Some(LocationElem::Index(i)) => *i += 1,
-            _ => panic!("array_item called outside of array"),
+    /// This handles tracking the active item, so that validation errors can
+    /// be associated with the correct index.
+    pub fn with_array_items<'a, T: 'a>(
+        &mut self,
+        iter: impl Iterator<Item = &'a T>,
+        mut f: impl FnMut(&mut ValidationCtx, &T),
+    ) {
+        for (i, item) in iter.enumerate() {
+            self.with_elem(LocationElem::Index(i), |ctx| f(ctx, item))
         }
     }
 
@@ -166,6 +160,9 @@ impl Display for ValidationError {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         writeln!(f, "\"{}\"", self.error)?;
         let mut indent = 0;
+        if self.location.len() < 2 {
+            return f.write_str("no parent location available");
+        }
 
         for (i, window) in self.location.windows(2).enumerate() {
             let prev = &window[0];
@@ -194,13 +191,7 @@ impl Display for ValidationError {
 
 impl<T: Validate> Validate for Vec<T> {
     fn validate_impl(&self, ctx: &mut ValidationCtx) {
-        ctx.in_array(|ctx| {
-            for item in self.iter() {
-                ctx.array_item(|ctx| {
-                    item.validate_impl(ctx);
-                })
-            }
-        });
+        ctx.with_array_items(self.iter(), |ctx, item| item.validate_impl(ctx))
     }
 }
 
@@ -227,12 +218,46 @@ impl<T: Validate> Validate for Option<T> {
 
 impl<T: Validate> Validate for BTreeSet<T> {
     fn validate_impl(&self, ctx: &mut ValidationCtx) {
-        ctx.in_array(|ctx| {
-            for item in self.iter() {
-                ctx.array_item(|ctx| {
-                    item.validate_impl(ctx);
+        ctx.with_array_items(self.iter(), |ctx, item| item.validate_impl(ctx))
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn sanity_check_array_validation() {
+        #[derive(Clone, Debug, Copy)]
+        struct Derp(i16);
+
+        struct DerpStore {
+            derps: Vec<Derp>,
+        }
+
+        impl Validate for Derp {
+            fn validate_impl(&self, ctx: &mut ValidationCtx) {
+                if self.0 > 7 {
+                    ctx.report("this derp is too big!!");
+                }
+            }
+        }
+
+        impl Validate for DerpStore {
+            fn validate_impl(&self, ctx: &mut ValidationCtx) {
+                ctx.in_table("DerpStore", |ctx| {
+                    ctx.in_field("derps", |ctx| self.derps.validate_impl(ctx))
                 })
             }
-        });
+        }
+
+        let my_derps = DerpStore {
+            derps: [1i16, 0, 3, 4, 12, 7, 6].into_iter().map(Derp).collect(),
+        };
+
+        let report = my_derps.validate().err().unwrap();
+        assert_eq!(report.errors.len(), 1);
+        // ensure that the index is being calculated correctly
+        assert!(report.to_string().contains(".derps[4]"));
     }
 }