Support no-fs saving

[cmyr]

This is modelled on the new loading API, and allows the user to 'save' a UFO in contexts where a filesystem is not available.

@yanone does this work for you?

[RickyDaMa]

Looks good to me, just a few questions really

[RickyDaMa]

I like that you've made a method out of this. Thoughts on making it public API?

[cmyr]

no strong feelings? Would it be genuinely useful?

You can get the contents here in a slightly annoying way, by using save_to_writer to dump to a Vec, and then using String::from_utf8 🤷

[RickyDaMa]

Yeah it's the fact that you can effectively do this (but in a really inefficient way) is why it could make sense to open this up and save someone (if anyone) that cruft. It doesn't really cost anything beyond the effort to write to the doc-string, so I vote we do it.

[cmyr]

the argument against is only a sort of general one: as a principle I prefer to avoid adding public API unless it's clear it will be used, since removing it in the future becomes a breaking change; that's the 'cost' we'd be avoiding.

Given that, let me know if it still seems preferable to you and I'm happy to make the change :)

[RickyDaMa]

Yeah I think that's fair and generally speaking I agree, but when we're likely to be maintaining this internal API for the foreseeable future and it has a somewhat-plausible public API use case I like to open it up. Not a blocking issue for the PR though.

[RickyDaMa]

Why did this trait bound need to be introduced?

[cmyr]

This is also something I was talked into by claude, with a reasonably convincing argument:

Both FontSource and FontSink carry a Send + Sync supertrait bound. Only Sync is actually compile-required by the current code — the rayon-parallel glyph load/write paths only ever capture the source/sink as a shared &dyn reference, and &T: Send + Sync follows from T: Sync alone. Send was added deliberately, for these reasons:

1. Ecosystem symmetry. The closest prior-art traits are all Send + Sync + 'static: tantivy's Directory, object_store, and rust-vfs. They require it because real consumers end up holding a Box<dyn _> / Arc<dyn _> and moving it across threads.
2. It's a breaking change to add later. Adding a supertrait bound after release breaks downstream impls. Both traits are unreleased today, so this is the one moment it's free.
3. No implementor loses anything. No reasonable FontSource/FontSink implementor is non-Send, so the bound costs callers nothing while keeping owned trait objects movable across threads.

'static was deliberately not added. That would break the borrowing-closure pattern the tests and doctests rely on — e.g. a closure capturing &Mutex<HashMap<PathBuf, Vec<u8>>> as an ad-hoc in-memory sink.

[RickyDaMa]

I disagree with Claude and would say that fewer trait bounds is better where they're not needed. If I had a !Send type and I wanted to implement this trait and I saw the bound was there for consistency and not because the code can't compile, I'd be miffed. Not to mention that if the user's code requires that their type be Send for some other reason then the compiler will ensure that. I'll leave it up to you on the final call for this one though.

[cmyr]

works for me!

[yanone]

This PR looks great for the basic FontSink/memory I/O — the prepare_save() + write_to_sink_impl() refactoring is cleaner than my fork's standalone save_font_with_sink().

This PR uses &self + Send + Sync + io::Error. My old fork uses &mut self + generic Error. babelfont-rs uses FnMut closures as sinks, so &mut self is the natural fit.

Feature file include() support is the missing component. That's the only real gap. Earlier you asked me to explain better:

• Loading: features.fea uses include(path/to/file.fea) to pull in other files. My fork recursively resolves these from a FontSource (e.g. a HashMap), with cycle detection and missing-file errors. The result is stored in a Font::feature_files field — a BTreeMap<PathBuf, String> keeping each file separate.
• Expanding: Font::features_expanded() inlines all includes into a single flat string for fea-rs compilation.
• Writing back: save_with_sink() writes each included file alongside features.fea as a separate entry, so the UFO round-trips correctly.

This PR covers ~60% of my needs. The feature file includes are the remaining 40% and the actual reason I still need my fork. If Colin adds that, I can drop my fork entirely.

Here are the annotated code snippets showing the full feature file include lifecycle. Wherever babelfont-rs is mentioned, this is a fork on my computer, not the live babelfont-rs. After this is done, I need to make a separate PR to babelfont to use the new API.

1. The data model — Font::feature_files field
   This is the central addition. Each included file is stored as a separate (virtual_path → content) entry so it can round-trip:

// src/font.rs
pub struct Font {
    // ... existing fields ...

    /// Structured feature files included via `include()` directives in
    /// [`Font::features`].
    ///
    /// The keys are virtual paths relative to the UFO root (e.g.
    /// `"features/includes/shared.fea"`) and the values are the raw file
    /// contents.
    ///
    /// Use [`Font::features_expanded`] to get a single flattened string with
    /// all includes inlined.
    pub feature_files: BTreeMap<PathBuf, String>,
}

2. Loading — load_feature_files() + collect_feature_includes()
   During Font::load_from_source(), instead of just reading features.fea as a single string, we recursively resolve every include() directive:

// src/non_file_io.rs
pub(crate) fn load_feature_files(
    source: &dyn FontSource,
    path: &Path,
) -> Result<FeatureFilesLoadResult, FontLoadError> {
    // Read features.fea from the source (HashMap in WASM, directory on disk)
    let Some(data) = source.try_read(path) else { return Ok(None); };
    let contents = String::from_utf8(data.map_err(FontLoadError::FeatureFile)?)?;

    // DFS: walk every include() directive, with cycle detection
    let mut included_files = BTreeMap::new();
    collect_feature_includes(source, &normalize_virtual_path(path),
        &contents, &mut HashSet::new(), &mut included_files)?;

    Ok(Some((contents, included_files)))
}

fn collect_feature_includes(
    source: &dyn FontSource,
    current_path: &Path,
    contents: &str,
    stack: &mut HashSet<PathBuf>,
    included_files: &mut BTreeMap<PathBuf, String>,
) -> Result<(), FontLoadError> {
    for line in contents.split_inclusive('\n') {
        if let Some(include_target) = parse_include_target(line) {
            let include_path = resolve_virtual(current_path, &include_target);

            // Cycle detection
            if stack.contains(&include_path) {
                return Err(FontLoadError::FeatureIncludeCycle { path: include_path });
            }
            // Already seen this file? Skip it (DAG dedup).
            if included_files.contains_key(&include_path) { continue; }

            // Read from the same FontSource (HashMap or filesystem)
            let data = source.try_read(&include_path)
                .ok_or(FontLoadError::MissingIncludedFeatureFile { path: include_path.clone() })?;
            let contents = String::from_utf8(data.map_err(FontLoadError::FeatureFile)?)?;

            // Recurse into this included file's own includes
            stack.insert(include_path.clone());
            collect_feature_includes(source, &include_path, &contents, stack, included_files)?;
            stack.remove(&include_path);

            included_files.insert(include_path, contents);
        }
    }
}

The parser for include(...) directives is deliberately simple — it matches include(...) at the start of a line, strips quotes, and normalizes the path:

fn parse_include_target(line: &str) -> Option<PathBuf> {
    let trimmed = line.trim();
    if !trimmed.starts_with("include") || !trimmed.ends_with(';') { return None; }
    let open = trimmed.find('(')?;
    let close = trimmed.rfind(')')?;
    if close < open { return None; }
    let inner = trimmed[open + 1..close].trim().trim_matches('"').trim_matches('\'');
    if inner.is_empty() { return None; }
    Some(normalize_virtual_path(Path::new(inner)))
}

This is called from Font::load_from_source() where features.fea is loaded:

// src/font.rs (inside Font::load_from_source)
let features_path = Path::new(FEATURES_FILE);
let (features, feature_files) = if request.features {
    match crate::non_file_io::load_feature_files(source, features_path)? {
        Some((main, included)) => (main, included),
        None => (String::new(), BTreeMap::new()),
    }
} else {
    (String::new(), BTreeMap::new())
};

3. Expanding — Font::features_expanded()
   When the compiler needs a single flat feature string (fea-rs can't resolve includes from a HashMap), we inline everything:

// src/font.rs
pub fn features_expanded(&self) -> Result<String, FontLoadError> {
    crate::non_file_io::expand_feature_text(&self.features, &self.feature_files)
}

This walks the same include() lines, but instead of reading from a FontSource, it reads from the feature_files map:

// src/non_file_io.rs
fn expand_feature_text_from_map(
    current_path: &Path,
    contents: &str,
    feature_files: &BTreeMap<PathBuf, String>,
    stack: &mut HashSet<PathBuf>,
) -> Result<String, FontLoadError> {
    let mut out = String::new();
    for line in contents.split_inclusive('\n') {
        if let Some(include_target) = parse_include_target(line) {
            let include_path = resolve_virtual(current_path, &include_target);
            if !stack.insert(include_path.clone()) {
                return Err(FontLoadError::FeatureIncludeCycle { path: include_path });
            }
            let include_contents = feature_files.get(&include_path)
                .ok_or(FontLoadError::MissingIncludedFeatureFile { path: include_path.clone() })?;
            out.push_str(&expand_feature_text_from_map(
                &include_path, include_contents, feature_files, stack)?);
            stack.remove(&include_path);
        } else {
            out.push_str(line);  // passthrough for non-include lines
        }
    }
    Ok(out)
}

Used in babelfont-rs to feed the feature compiler:

// babelfont-rs/babelfont/src/convertors/ufo.rs
let expanded_features = ufo.features_expanded()?;
font.features = Features::from_fea(&expanded_features);

4. Writing back — save_font_with_sink()
   When saving to a FontSink, we write features.fea and each included file as a separate entry:

// src/non_file_io.rs (inside save_font_with_sink)
if !font.features.is_empty() || !font.feature_files.is_empty() {
    // Write the main features.fea
    write_sink_file(sink, Path::new(FEATURES_FILE),
        normalize_feature_text(&font.features).as_bytes())?;

    // Write each included file as a separate entry
    for (feature_path, contents) in &font.feature_files {
        write_sink_file(sink, feature_path,
            normalize_feature_text(contents).as_bytes())?;
    }
}

This is what babelfont-rs calls via save_with_sink:

// babelfont-rs/babelfont/src/convertors/ufo.rs
pub fn save_entries(font: &Font) -> Result<HashMap<String, String>, BabelfontError> {
    let ufo = as_norad(font, 0)?;
    let mut written: HashMap<String, Vec<u8>> = HashMap::new();
    let mut sink = |path: &FsPath, data: &[u8]| -> Result<(), std::io::Error> {
        written.insert(path.to_string_lossy().to_string(), data.to_vec());
        Ok(())
    };
    ufo.save_with_sink(&norad::WriteOptions::default(), &mut sink)?;
    // Returns e.g. {
    //   "features.fea" -> "languagesystem DFLT dflt;\ninclude(includes/shared.fea);\n...",
    //   "includes/shared.fea" -> "@shared = [A B C];\n",
    //   "metainfo.plist" -> "...",
    //   "glyphs/contents.plist" -> "...",
    //   ...
    // }
}

5. The full round-trip
   Putting it all together, a font with include() directives survives a load→save cycle intact:

Source (HashMap or disk)
  │
  ├─ Font::load_from_source()
  │   ├─ reads features.fea → Font::features
  │   ├─ reads includes/shared.fea → Font::feature_files["includes/shared.fea"]
  │   └─ reads includes/liga.fea    → Font::feature_files["includes/liga.fea"]
  │
  ├─ font.features_expanded()
  │   └─ inlines everything → single flat string for fea-rs
  │
  └─ font.save_with_sink(sink)
      ├─ sink.write("features.fea", ...)           ← main file
      ├─ sink.write("includes/shared.fea", ...)     ← preserved
      └─ sink.write("includes/liga.fea", ...)        ← preserved

The key invariant: every file that features.fea references via include() is tracked as a separate feature_files entry, so it can be loaded, expanded, and written back independently. Without this, a UFO that uses include() would silently lose data on save, and loading from an in-memory HashMap would fail because the included files are simply not there.

Note: I'll be away tomorrow (Friday) and Monday. I can react again on Tuesday.
Thank you!

[cmyr]

Okay, so the FontSink isn't &mut because we support concurrent saving of glyphs, and that breaks if we're trying to mutate.

Your version of this feature added the sink as an alternative API,a but in this patch it's the core API, with the default file-based behaviour built on top of it, so we do want the parallelism.

I acknowledge this will be a tiny bit annoying; you'll have to use interior mutability on your impl type.

For the include resolution business: this just feels like a mixing of concerns, and my soft position is that this is not something that should live in norad. It should be easy enough to build on top, if needed, unless I'm missing something.

Some things that are slightly confusing, that I've had to look up:

• the UFO spec says that Any include() statements must be relative to the UFO path, not to the features.fea file itself.; if a UFO says include!(derp.fea), that is assumed to be a file that lives beside the UFO, not inside the UFO.
• I've gone and done a quick look at the 723 UFO files on my computer where there is an include statement inside of features.fea. Of these, only one include file points to a location inside the UFO.

In any case, fea-rs already provides filesystem-agnostic logic for navigating includes, generating a combined parse tree, and other relevant functionality. If you would like to have more of the internals exposed there that might make sense? This would save you from duplicating the logic around resolving includes, at the very least?

[RickyDaMa]

Thanks for addressing my feedback ❤️