Merge pull request #695 from googlefonts/feature-parsing-work

Split feature work into separate parsing & compilation steps

fontbe/src/features.rs

@@ -15,13 +15,13 @@ use log::{debug, error, trace, warn};
 use ordered_float::OrderedFloat;
 
 use fea_rs::{
-    compile::{Compilation, FeatureBuilder, FeatureProvider, VariationInfo},
-    parse::{SourceLoadError, SourceResolver},
-    Compiler,
+    compile::{error::CompilerError, Compilation, FeatureBuilder, FeatureProvider, VariationInfo},
+    parse::{FileSystemResolver, SourceLoadError, SourceResolver},
+    DiagnosticSet, ParseTree,
 };
 
 use fontir::{
-    ir::{FeaturesSource, StaticMetadata},
+    ir::{FeaturesSource, GlyphOrder, StaticMetadata},
     orchestration::{Flags, WorkId as FeWorkId},
 };
 
@@ -36,11 +36,14 @@ use write_fonts::{
 
 use crate::{
     error::Error,
-    orchestration::{AnyWorkId, BeWork, Context, FeaRsKerns, FeaRsMarks, WorkId},
+    orchestration::{AnyWorkId, BeWork, Context, FeaAst, FeaRsKerns, FeaRsMarks, WorkId},
 };
 
 #[derive(Debug)]
-pub struct FeatureWork {}
+pub struct FeatureParsingWork {}
+
+#[derive(Debug)]
+pub struct FeatureCompilationWork {}
 
 // I did not want to make a struct
 // I did not want to clone the content
@@ -54,6 +57,16 @@ struct InMemoryResolver {
     include_dir: Option<PathBuf>,
 }
 
+impl InMemoryResolver {
+    fn empty() -> Self {
+        InMemoryResolver {
+            content_path: Default::default(),
+            content: "".into(),
+            include_dir: None,
+        }
+    }
+}
+
 impl SourceResolver for InMemoryResolver {
     fn get_contents(&self, rel_path: &OsStr) -> Result<Arc<str>, SourceLoadError> {
         if rel_path == &*self.content_path {
@@ -342,61 +355,41 @@ impl<'a> VariationInfo for FeaVariationInfo<'a> {
     }
 }
 
-impl FeatureWork {
+impl FeatureCompilationWork {
     pub fn create() -> Box<BeWork> {
-        Box::new(FeatureWork {})
+        Box::new(FeatureCompilationWork {})
     }
 
     fn compile(
         &self,
         static_metadata: &StaticMetadata,
-        features: &FeaturesSource,
+        ast: &FeaAst,
         kerns: &FeaRsKerns,
         marks: &FeaRsMarks,
     ) -> Result<Compilation, Error> {
         let var_info = FeaVariationInfo::new(static_metadata);
         let feature_writer = FeatureWriter::new(kerns, marks);
-        match features {
-            FeaturesSource::File {
-                fea_file,
-                include_dir,
-            } => {
-                let mut compiler = Compiler::new(OsString::from(fea_file), &marks.glyphmap);
-                if let Some(include_dir) = include_dir {
-                    compiler = compiler.with_project_root(include_dir)
-                }
-                compiler
-            }
-            FeaturesSource::Memory {
-                fea_content,
-                include_dir,
-            } => {
-                let root = OsString::new();
-                let mut compiler =
-                    Compiler::new(root.clone(), &marks.glyphmap).with_resolver(InMemoryResolver {
-                        content_path: root,
-                        content: Arc::from(fea_content.as_str()),
-                        include_dir: include_dir.clone(),
-                    });
-                if let Some(include_dir) = include_dir {
-                    compiler = compiler.with_project_root(include_dir)
-                }
-                compiler
-            }
-            FeaturesSource::Empty => {
-                // There is no user feature file but we could still generate kerning, marks, etc
-                let root = OsString::new();
-                Compiler::new(root.clone(), &marks.glyphmap).with_resolver(InMemoryResolver {
-                    content_path: root,
-                    content: Arc::from(""),
-                    include_dir: None,
-                })
+        // first do the validation pass
+        let diagnostics = fea_rs::compile::validate(&ast.ast, &marks.glyphmap, Some(&var_info));
+        if diagnostics.has_errors() {
+            return Err(CompilerError::ValidationFail(diagnostics).into());
+        }
+        log_fea_warnings("validation", &diagnostics);
+        // then we do the actual compilation pass:
+        match fea_rs::compile::compile(
+            &ast.ast,
+            &marks.glyphmap,
+            Some(&var_info),
+            Some(&feature_writer),
+        ) {
+            Ok((result, warnings)) => {
+                log_fea_warnings("compilation", &warnings);
+                Ok(result)
             }
+            Err(errors) => Err(Error::FeaCompileError(CompilerError::CompilationFail(
+                errors,
+            ))),
         }
-        .with_variable_info(&var_info)
-        .with_feature_writer(&feature_writer)
-        .compile()
-        .map_err(Error::FeaCompileError)
     }
 }
 
@@ -420,16 +413,92 @@ fn write_debug_fea(context: &Context, is_error: bool, why: &str, fea_content: &s
     };
 }
 
-impl Work<Context, AnyWorkId, Error> for FeatureWork {
+impl Work<Context, AnyWorkId, Error> for FeatureParsingWork {
     fn id(&self) -> AnyWorkId {
-        WorkId::Features.into()
+        WorkId::FeaturesAst.into()
     }
 
     fn read_access(&self) -> Access<AnyWorkId> {
         AccessBuilder::new()
             .variant(FeWorkId::GlyphOrder)
             .variant(FeWorkId::StaticMetadata)
             .variant(FeWorkId::Features)
+            .build()
+    }
+
+    fn exec(&self, context: &Context) -> Result<(), Error> {
+        let features = context.ir.features.get();
+        let glyph_order = context.ir.glyph_order.get();
+        let result = self.parse(&features, &glyph_order);
+
+        if let FeaturesSource::Memory { fea_content, .. } = features.as_ref() {
+            write_debug_fea(context, result.is_err(), "compile failed", fea_content);
+        }
+        context.fea_ast.set(result?.into());
+        Ok(())
+    }
+}
+
+impl FeatureParsingWork {
+    pub fn create() -> Box<BeWork> {
+        Box::new(Self {})
+    }
+
+    fn parse(
+        &self,
+        features: &FeaturesSource,
+        glyph_order: &GlyphOrder,
+    ) -> Result<ParseTree, Error> {
+        let (resolver, root_path) = get_resolver_and_root_path(features);
+        let glyph_map = glyph_order.iter().map(|g| g.clone().into_inner()).collect();
+        let (tree, diagnostics) = fea_rs::parse::parse_root(root_path, Some(&glyph_map), resolver)
+            .map_err(CompilerError::SourceLoad)?;
+        if diagnostics.has_errors() {
+            return Err(CompilerError::ParseFail(diagnostics).into());
+        }
+        log_fea_warnings("parsing", &diagnostics);
+        Ok(tree)
+    }
+}
+
+fn get_resolver_and_root_path(features: &FeaturesSource) -> (Box<dyn SourceResolver>, OsString) {
+    match features {
+        FeaturesSource::File {
+            fea_file,
+            include_dir,
+        } => {
+            let project_root = include_dir
+                .clone()
+                .or_else(|| fea_file.parent().map(PathBuf::from))
+                .unwrap_or_default();
+            (
+                Box::new(FileSystemResolver::new(project_root)),
+                fea_file.to_owned().into_os_string(),
+            )
+        }
+        FeaturesSource::Memory {
+            fea_content,
+            include_dir,
+        } => (
+            Box::new(InMemoryResolver {
+                include_dir: include_dir.to_owned(),
+                content_path: OsString::new(),
+                content: fea_content.as_str().into(),
+            }),
+            OsString::new(),
+        ),
+        FeaturesSource::Empty => (Box::new(InMemoryResolver::empty()), Default::default()),
+    }
+}
+
+impl Work<Context, AnyWorkId, Error> for FeatureCompilationWork {
+    fn id(&self) -> AnyWorkId {
+        WorkId::Features.into()
+    }
+
+    fn read_access(&self) -> Access<AnyWorkId> {
+        AccessBuilder::new()
+            .variant(WorkId::FeaturesAst)
             .variant(WorkId::GatherBeKerning)
             .variant(WorkId::Marks)
             .build()
@@ -445,22 +514,11 @@ impl Work<Context, AnyWorkId, Error> for FeatureWork {
 
     fn exec(&self, context: &Context) -> Result<(), Error> {
         let static_metadata = context.ir.static_metadata.get();
-        let features = context.ir.features.get();
+        let ast = context.fea_ast.get();
         let kerns = context.fea_rs_kerns.get();
         let marks = context.fea_rs_marks.get();
 
-        let result = self.compile(
-            &static_metadata,
-            features.as_ref(),
-            kerns.as_ref(),
-            marks.as_ref(),
-        );
-        if result.is_err() || context.flags.contains(Flags::EMIT_DEBUG) {
-            if let FeaturesSource::Memory { fea_content, .. } = features.as_ref() {
-                write_debug_fea(context, result.is_err(), "compile failed", fea_content);
-            }
-        }
-        let result = result?;
+        let result = self.compile(&static_metadata, &ast, kerns.as_ref(), marks.as_ref())?;
 
         debug!(
             "Built features, gpos? {} gsub? {} gdef? {}",
@@ -493,6 +551,17 @@ impl Work<Context, AnyWorkId, Error> for FeatureWork {
     }
 }
 
+fn log_fea_warnings(stage: &str, warnings: &DiagnosticSet) {
+    assert!(!warnings.has_errors(), "of course we checked this already");
+    if !warnings.is_empty() {
+        log::debug!(
+            "FEA {stage} produced {} warnings:\n{}",
+            warnings.len(),
+            warnings.display()
+        );
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use std::collections::{HashMap, HashSet};

fontbe/src/orchestration.rs

@@ -12,7 +12,7 @@ use fea_rs::{
     compile::{
         MarkToBaseBuilder, MarkToMarkBuilder, PairPosBuilder, ValueRecord as ValueRecordBuilder,
     },
-    GlyphMap, GlyphSet,
+    GlyphMap, GlyphSet, ParseTree,
 };
 use fontdrasil::{
     coords::NormalizedLocation,
@@ -81,6 +81,7 @@ type KernBlock = usize;
 #[derive(Clone, Debug, PartialEq, Eq, Hash, PartialOrd, Ord)]
 pub enum WorkId {
     Features,
+    FeaturesAst,
     Avar,
     Cmap,
     Font,
@@ -114,6 +115,7 @@ impl Identifier for WorkId {
     fn discriminant(&self) -> IdentifierDiscriminant {
         match self {
             WorkId::Features => "BeFeatures",
+            WorkId::FeaturesAst => "BeFeaturesAst",
             WorkId::Avar => "BeAvar",
             WorkId::Cmap => "BeCmap",
             WorkId::Font => "BeFont",
@@ -373,6 +375,20 @@ pub struct FeaRsKerns {
     pub lookups: Vec<PairPosBuilder>,
 }
 
+/// The Ast for any user features.
+///
+/// This is a newtype so that we can implement `Persistable`
+#[derive(Clone, Debug, Serialize, Deserialize, PartialEq)]
+pub struct FeaAst {
+    pub ast: ParseTree,
+}
+
+impl From<ParseTree> for FeaAst {
+    fn from(ast: ParseTree) -> FeaAst {
+        FeaAst { ast }
+    }
+}
+
 impl FeaRsKerns {
     pub fn is_empty(&self) -> bool {
         self.lookups.is_empty()
@@ -389,6 +405,16 @@ impl Persistable for FeaRsKerns {
     }
 }
 
+impl Persistable for FeaAst {
+    fn read(from: &mut dyn Read) -> Self {
+        bincode::deserialize_from(from).unwrap()
+    }
+
+    fn write(&self, to: &mut dyn Write) {
+        bincode::serialize_into(to, self).unwrap()
+    }
+}
+
 /// Kerning adjustments at various locations
 pub type KernAdjustments = BTreeMap<NormalizedLocation, OrderedFloat<f32>>;
 
@@ -608,6 +634,7 @@ pub struct Context {
     pub mvar: BeContextItem<BeValue<Mvar>>,
     pub all_kerning_pairs: BeContextItem<AllKerningPairs>,
     pub kern_fragments: BeContextMap<KernFragment>,
+    pub fea_ast: BeContextItem<FeaAst>,
     pub fea_rs_kerns: BeContextItem<FeaRsKerns>,
     pub fea_rs_marks: BeContextItem<FeaRsMarks>,
     pub stat: BeContextItem<BeValue<Stat>>,
@@ -647,6 +674,7 @@ impl Context {
             fea_rs_kerns: self.fea_rs_kerns.clone_with_acl(acl.clone()),
             fea_rs_marks: self.fea_rs_marks.clone_with_acl(acl.clone()),
             stat: self.stat.clone_with_acl(acl.clone()),
+            fea_ast: self.fea_ast.clone_with_acl(acl.clone()),
             font: self.font.clone_with_acl(acl),
         }
     }
@@ -702,6 +730,11 @@ impl Context {
                 acl.clone(),
                 persistent_storage.clone(),
             ),
+            fea_ast: ContextItem::new(
+                WorkId::FeaturesAst.into(),
+                acl.clone(),
+                persistent_storage.clone(),
+            ),
             stat: ContextItem::new(WorkId::Stat.into(), acl.clone(), persistent_storage.clone()),
             font: ContextItem::new(WorkId::Font.into(), acl, persistent_storage),
         }

fontbe/src/paths.rs

@@ -65,6 +65,7 @@ impl Paths {
     pub fn target_file(&self, id: &WorkId) -> PathBuf {
         match id {
             WorkId::Features => self.build_dir.join("features.marker"),
+            WorkId::FeaturesAst => self.build_dir.join("features_ast.marker"),
             WorkId::GlyfFragment(name) => self.glyph_glyf_file(name.as_str()),
             WorkId::GvarFragment(name) => self.glyph_gvar_file(name.as_str()),
             WorkId::Avar => self.build_dir.join("avar.table"),