Allow for chaining hotfixes

Author: simoncozens

fontspector-checkapi/src/check.rs

@@ -46,6 +46,9 @@ pub enum CheckImplementation<'a> {
     CheckAll(&'a CheckAllSignature),
 }
 
+/// The function signature for a hotfix function
+pub type HotfixFunction = dyn Fn(&mut Testable) -> FixFnResult;
+
 #[derive(Clone)]
 /// A check definition
 pub struct Check<'a> {
@@ -60,7 +63,7 @@ pub struct Check<'a> {
     /// Function pointer implementing the actual check
     pub implementation: CheckImplementation<'a>,
     /// Function pointer implementing a hotfix to the binary file
-    pub hotfix: Option<&'a dyn Fn(&Testable) -> FixFnResult>,
+    pub hotfix: Option<&'a HotfixFunction>,
     /// Function pointer implementing a hotfix to the font source file
     pub fix_source: Option<&'a dyn Fn(&Testable) -> FixFnResult>,
     /// A registered file type that this check applies to
@@ -114,6 +117,7 @@ impl<'a> Check<'a> {
         &'a self,
         fn_result: CheckFnResult,
         filename: Option<&str>,
+        source_filename: Option<&str>,
         section: Option<&str>,
         duration: Duration,
     ) -> CheckResult {
@@ -127,7 +131,7 @@ impl<'a> Check<'a> {
         } else {
             subresults
         };
-        CheckResult::new(self, filename, section, res, duration)
+        CheckResult::new(self, filename, source_filename, section, res, duration)
     }
 
     /// Run the check, either on a collection or a single file.
@@ -155,7 +159,8 @@ impl<'a> Check<'a> {
 
                 Some(self.clarify_result(
                     result,
-                    Some(f).and_then(|f| f.filename.to_str()),
+                    f.filename.to_str(),
+                    f.source.as_ref().and_then(|x| x.to_str()),
                     section,
                     duration,
                 ))
@@ -169,7 +174,7 @@ impl<'a> Check<'a> {
                 #[cfg(target_family = "wasm")]
                 let duration = Duration::from_secs(0);
 
-                Some(self.clarify_result(result, Some(&f.directory), section, duration))
+                Some(self.clarify_result(result, Some(&f.directory), None, section, duration))
             }
         }
     }

fontspector-checkapi/src/checkresult.rs

@@ -34,6 +34,8 @@ pub struct CheckResult {
     pub check_rationale: String,
     /// The file which was checked; if None, the check was run on all files
     pub filename: Option<String>,
+    /// The source where this file came from, if any
+    pub source_filename: Option<String>,
     /// The section of the profile this check belongs to
     pub section: Option<String>,
     /// The individual results of the check
@@ -73,6 +75,7 @@ impl CheckResult {
     pub fn new(
         check: &Check,
         filename: Option<&str>,
+        source_filename: Option<&str>,
         section: Option<&str>,
         subresults: Vec<Status>,
         duration: Duration,
@@ -82,6 +85,7 @@ impl CheckResult {
             check_name: check.title.to_string(),
             check_rationale: check.rationale.to_string(),
             filename: filename.map(|x| x.to_string()),
+            source_filename: source_filename.map(|x| x.to_string()),
             section: section.map(|x| x.to_string()),
             subresults,
             hotfix_result: None,

fontspector-checkapi/src/lib.rs

@@ -41,7 +41,7 @@ mod status;
 mod testable;
 /// Common utility functions for check implementors
 mod utils;
-pub use check::{return_result, Check, CheckFlags, CheckId, CheckImplementation};
+pub use check::{return_result, Check, CheckFlags, CheckId, CheckImplementation, HotfixFunction};
 pub use checkresult::{CheckResult, FixResult};
 pub use context::Context;
 pub use filetype::{FileType, FileTypeConvert};

fontspector-checkapi/src/testable.rs

@@ -76,6 +76,16 @@ impl Testable {
             .and_then(|x| x.to_str())
             .map(|x| x.to_string())
     }
+
+    /// Set the new contents of a file
+    pub fn set(&mut self, new_bytes: Vec<u8>) {
+        self.contents = new_bytes;
+    }
+
+    /// Save the contents of a file to disk
+    pub fn save(&self) -> Result<(), std::io::Error> {
+        std::fs::write(&self.filename, &self.contents)
+    }
 }
 
 /// A related set of files which will be checked together.

fontspector-cli/src/main.rs

@@ -14,7 +14,8 @@ use args::Args;
 use clap::Parser;
 use fontbakery_bridge::FontbakeryBridge;
 use fontspector_checkapi::{
-    Check, CheckResult, Context, FixResult, Plugin, Registry, TestableCollection, TestableType,
+    Check, CheckResult, Context, FixResult, HotfixFunction, Plugin, Registry, Status, StatusCode,
+    Testable, TestableCollection, TestableType,
 };
 use itertools::Either;
 use profile_googlefonts::GoogleFonts;
@@ -187,7 +188,7 @@ fn main() {
     );
     // }
 
-    // Run all the things! Check all the fonts! Fix all the binaries! Fix all the sources!
+    // Run all the things! Check all the fonts!
 
     // Do this in parallel for release, serial for debug
     #[cfg(debug_assertions)]
@@ -203,21 +204,22 @@ fn main() {
         Either::Right(checkorder.par_iter())
     };
 
-    #[allow(clippy::unwrap_used)] // We check for is_some before unwrapping
-    let results: RunResults = checkorder_iterator
+    let mut results: RunResults = checkorder_iterator
         .map(|(sectionname, testable, check, context)| {
             (
                 testable,
                 check,
                 check.run(testable, context, Some(sectionname)),
             )
         })
-        .filter_map(|(testable, check, result)| {
-            result.map(|result| apply_fixes(testable, check, result, &args))
-        })
+        .filter_map(|(_, _, result)| result)
         .collect::<Vec<CheckResult>>()
         .into();
 
+    if args.hotfix || args.fix_sources {
+        try_fixing_stuff(&mut results, &args, &registry);
+    }
+
     let worst_status = results.worst_status();
 
     let mut reporters: Vec<Box<dyn Reporter>> = vec![];
@@ -330,37 +332,88 @@ fn load_configuration(args: &Args) -> Map<String, serde_json::Value> {
         .unwrap_or_default()
 }
 
-fn apply_fixes(
-    testable: &&TestableType,
-    check: &&Check,
-    mut result: CheckResult,
-    args: &Args,
-) -> CheckResult {
-    if let TestableType::Single(testable) = testable {
-        if args.hotfix {
-            if let Some(fix) = check.hotfix {
-                result.hotfix_result = match fix(testable) {
-                    Ok(_) => Some(FixResult::Fixed),
-                    Err(e) => Some(FixResult::FixError(e)),
-                }
-            } else {
-                result.hotfix_result = Some(FixResult::Unfixable);
-            }
-        } else if check.hotfix.is_some() {
-            result.hotfix_result = Some(FixResult::Available);
+// fn apply_fixes(
+//     testable: &mut TestableType,
+//     check: &&Check,
+//     mut result: CheckResult,
+//     args: &Args,
+// ) -> CheckResult {
+//     if let TestableType::Single(testable) = testable {
+//         if args.hotfix {
+//             if let Some(fix) = check.hotfix {
+//                 result.hotfix_result = match fix(testable) {
+//                     Ok(_) => Some(FixResult::Fixed),
+//                     Err(e) => Some(FixResult::FixError(e)),
+//                 }
+//             } else {
+//                 result.hotfix_result = Some(FixResult::Unfixable);
+//             }
+//         } else if check.hotfix.is_some() {
+//             result.hotfix_result = Some(FixResult::Available);
+//         }
+//         if args.fix_sources {
+//             if let Some(fix) = check.fix_source {
+//                 result.sourcefix_result = match fix(testable) {
+//                     Ok(_) => Some(FixResult::Fixed),
+//                     Err(e) => Some(FixResult::FixError(e)),
+//                 }
+//             } else {
+//                 result.sourcefix_result = Some(FixResult::Unfixable);
+//             }
+//         } else if check.fix_source.is_some() {
+//             result.sourcefix_result = Some(FixResult::Available);
+//         }
+//     }
+//     result
+// }
+
+fn try_fixing_stuff(results: &mut RunResults, args: &Args, registry: &Registry) {
+    let failed_checks = results
+        .iter_mut()
+        .filter(|x| x.worst_status() >= StatusCode::Fail)
+        .collect::<Vec<_>>();
+    // Group the fixes by filename because we want to provide testables
+    // // let mut fix_sources = HashMap::new();
+    let mut fix_binaries: HashMap<String, Vec<(&HotfixFunction, &mut CheckResult)>> =
+        HashMap::new();
+    for result in failed_checks.into_iter() {
+        let Some(check) = registry.checks.get(&result.check_id) else {
+            log::warn!(
+                "A check called {} just mysteriously vanished",
+                result.check_id
+            );
+            continue;
+        };
+        if args.hotfix && result.filename.is_some() && check.hotfix.is_some() {
+            #[allow(clippy::unwrap_used)] // We know this is Some
+            fix_binaries
+                .entry(result.filename.clone().unwrap())
+                .or_default()
+                .push((check.hotfix.unwrap(), result));
         }
-        if args.fix_sources {
-            if let Some(fix) = check.fix_source {
-                result.sourcefix_result = match fix(testable) {
-                    Ok(_) => Some(FixResult::Fixed),
-                    Err(e) => Some(FixResult::FixError(e)),
+    }
+
+    for (file, fixes) in fix_binaries.into_iter() {
+        let mut testable = Testable::new(&file).unwrap_or_else(|e| {
+            log::error!("Could not load files from {:?}: {:}", file, e);
+            std::process::exit(1)
+        });
+        let mut modified = false;
+        for (fix, result) in fixes.into_iter() {
+            result.hotfix_result = match fix(&mut testable) {
+                Ok(hotfix_behaviour) => {
+                    modified |= hotfix_behaviour;
+                    Some(FixResult::Fixed)
                 }
-            } else {
-                result.sourcefix_result = Some(FixResult::Unfixable);
+                Err(e) => Some(FixResult::FixError(e)),
             }
-        } else if check.fix_source.is_some() {
-            result.sourcefix_result = Some(FixResult::Available);
+        }
+        if modified {
+            // save it
+            testable.save().unwrap_or_else(|e| {
+                log::error!("Could not save file {:?}: {:}", file, e);
+                std::process::exit(1)
+            });
         }
     }
-    result
 }

fontspector-cli/src/reporters/mod.rs

@@ -16,6 +16,10 @@ impl RunResults {
     pub fn iter(&self) -> impl Iterator<Item = &CheckResult> {
         self.results.iter()
     }
+    /// Iterate over each check mutably
+    pub fn iter_mut(&mut self) -> impl Iterator<Item = &mut CheckResult> {
+        self.results.iter_mut()
+    }
 
     /// Get the worst status of all checks
     pub fn worst_status(&self) -> StatusCode {

profile-universal/src/checks/name/trailing_spaces.rs

@@ -36,6 +36,6 @@ fn trailing_spaces(f: &Testable, _context: &Context) -> CheckFnResult {
     return_result(problems)
 }
 
-fn fix_trailing_spaces(_f: &Testable) -> FixFnResult {
+fn fix_trailing_spaces(_f: &mut Testable) -> FixFnResult {
     Ok(false)
 }

profile-universal/src/checks/nested_components.rs

@@ -82,7 +82,7 @@ fn get_depth(glyph_id: GlyphId, loca: &Loca, glyf: &Glyf) -> u32 {
     depth
 }
 
-fn decompose_nested_components(t: &Testable) -> FixFnResult {
+fn decompose_nested_components(t: &mut Testable) -> FixFnResult {
     let font = fixfont!(t);
     let loca = font
         .font()

profile-universal/src/checks/transformed_components.rs

@@ -100,7 +100,7 @@ fn transformed_components(f: &Testable, context: &Context) -> CheckFnResult {
     }
 }
 
-fn decompose_transformed_components(t: &Testable) -> FixFnResult {
+fn decompose_transformed_components(t: &mut Testable) -> FixFnResult {
     let f = fixfont!(t);
     let loca = f
         .font()
@@ -133,7 +133,10 @@ fn decompose_transformed_components(t: &Testable) -> FixFnResult {
     decompose_components_impl(t, &bad_composites)
 }
 
-pub(crate) fn decompose_components_impl(t: &Testable, decompose_order: &[GlyphId]) -> FixFnResult {
+pub(crate) fn decompose_components_impl(
+    t: &mut Testable,
+    decompose_order: &[GlyphId],
+) -> FixFnResult {
     let f = fixfont!(t);
     let mut new_font = FontBuilder::new();
     let mut builder = GlyfLocaBuilder::new();
@@ -180,8 +183,7 @@ pub(crate) fn decompose_components_impl(t: &Testable, decompose_order: &[GlyphId
     new_font.add_table(&new_loca).map_err(|x| x.to_string())?;
     new_font.copy_missing_tables(f.font());
     let new_bytes = new_font.build();
-    std::fs::write(&t.filename, new_bytes).map_err(|_| "Couldn't write file".to_string())?;
-
+    t.set(new_bytes);
     Ok(true)
 }
 

profile-universal/src/checks/unwanted_aat_tables.rs

@@ -49,7 +49,7 @@ fn unwanted_aat_tables(t: &Testable, _context: &Context) -> CheckFnResult {
     })
 }
 
-fn delete_unwanted_aat_tables(t: &Testable) -> FixFnResult {
+fn delete_unwanted_aat_tables(t: &mut Testable) -> FixFnResult {
     let f = fixfont!(t);
     let mut new_font = FontBuilder::new();
     for table in f.font().table_directory.table_records() {
@@ -61,6 +61,6 @@ fn delete_unwanted_aat_tables(t: &Testable) -> FixFnResult {
         }
     }
     let new_bytes = new_font.build();
-    std::fs::write(&t.filename, new_bytes).map_err(|_| "Couldn't write file".to_string())?;
+    t.set(new_bytes);
     Ok(true)
 }