poc: resolve record offsets without passing FontData

docs/codegen-tour.md

@@ -621,7 +621,7 @@ has a size which can be computed at runtime from some set of arguments.
 
 Records, like tables, can contain offsets. Unlike tables, records do not have
 access to the raw data against which those offsets should be resolved. For the
-purpose of consistency across our geneerated code, however, it *is* important
+purpose of consistency across our generated code, however, it *is* important
 that we have a consistent way of resolving offsets contained in records, and we
 do: you have to pass it in.
 
@@ -637,15 +637,33 @@ The equivalent getter on a record looks like,
 fn coverage(&self, data: FontData<'a>) -> Result<CoverageTable<'a>, ReadError>;
 ```
 
-This... honestly, this is not great ergonomics. It is, however, simple, and is
-relied on by codegen in various places, and when we're generating code we aren't
-too bothered by how ergonomic it is. We might want to revisit this at some
-point; one simple improvement would be to have the caller pass in the parent
-table, but I'm not sure how this would work in cases where a type might be
-referenced by multiple parents. Another option would be to have some kind of
-fancy `RecordData` struct that would be a thin wrapper around a record plus the
-parent data, and which would implement the record getters, but deref to the
-record otherwise.... I'm really not sure.
+This is not great ergonomics, and it is easy to misuse: nothing stops you from
+passing the *wrong* table's data, in which case you silently get a
+wrong-but-plausible answer. To address this, table fields containing arrays of
+offset-bearing fixed-size records do not return a bare `&[T]`; they return the
+`ArrayOfRecordsWithOffsetData` type, which pairs the records with the data of
+the enclosing table. This is a vec-like type (`len`/`get`/`iter`, with an
+`as_slice` escape hatch) whose items are `OffsetResolving<T>`: a thin wrapper
+around a record plus the parent data. For each offset field in the record, we
+generate a no-argument getter on `OffsetResolving<T>` that resolves the offset
+against the stored data; for everything else it derefs to the record.
+
+```rust
+impl<'a> OffsetResolving<'a, ScriptRecord> {
+    pub fn script(&self) -> Result<Script<'a>, ReadError> {
+        self.record().script(self.offset_data())
+    }
+}
+```
+
+The data-taking getters on the record itself remain (codegen and some callers
+rely on them), but most code should never need to supply `FontData` by hand.
+Records whose offsets resolve against something other than the parent table
+(those using the `#[offset_getter]` attribute, like `NameRecord`) are excluded
+and keep the plain `&[T]` representation. This mechanism covers *fixed-size*
+records, which are lazily byte-cast and so cannot store the parent data
+themselves; for non-fixed-size records (which are instantiated on read) the
+plan is for the record to hold the parent data internally.
 
 ### <a id="arrays"></a> arrays
 
@@ -655,7 +673,9 @@ the size and contents of the array:
 - if the contents of an array have a fixed uniform size, known at compile time, then we
   represent the array as a rust slice: `&[T]`. This is true for all scalars
   (including offsets) as well as records that are composed of a fixed number of
-  scalars.
+  scalars. (For fixed-size records that contain offsets, the table getter wraps
+  the slice in `ArrayOfRecordsWithOffsetData`; see [offsets in
+  records](#offsets-in-records).)
 - if the contents of an array have a uniform size, but the size can only be
   determined at runtime, we represent the array using the [`ComputedArray`][] type.
   This requires the inner type to implement [`FontRead`][] with a non-empty

font-codegen/src/fields.rs

@@ -375,11 +375,15 @@ fn traversal_arm_for_field(
                 let offset_data = pass_data
                     .cloned()
                     .unwrap_or_else(|| fld.offset_getter_data_src());
+                // the wrapper is not a slice, but carries one
+                let maybe_as_slice = fld
+                    .array_of_offset_bearing_records
+                    .then(|| quote!(.as_slice()));
                 quote!(Field::new(
                         #name_str,
                         traversal::FieldType::array_of_records(
                             stringify!(#typ),
-                            self.#name()#maybe_try,
+                            self.#name()#maybe_try #maybe_as_slice,
                             #offset_data,
                         )
                 ))
@@ -663,6 +667,9 @@ impl Field {
                     let be = big_endian(typ);
                     quote!(&'a [#be])
                 }
+                FieldType::Struct { typ } if self.array_of_offset_bearing_records => {
+                    quote!(ArrayOfRecordsWithOffsetData<'a, #typ>)
+                }
                 FieldType::Struct { typ } => quote!(&'a [#typ]),
                 FieldType::PendingResolution { typ } => quote!( &'a [#typ] ),
                 _ => unreachable!("An array should never contain {:#?}", inner_typ),
@@ -732,7 +739,18 @@ impl Field {
         } else if is_var_array {
             quote!( self.data.split_off(range.start).and_then(|d| VarLenArray::read(d).ok()) #maybe_unwrap_or_def )
         } else if is_array {
-            quote!(self.data.read_array(range).ok() #maybe_unwrap #maybe_unwrap_or_def)
+            if self.array_of_offset_bearing_records {
+                // pair the records with the table's data, so that they can
+                // resolve their offsets without being passed it
+                let data_src = self.offset_getter_data_src();
+                quote! {
+                    self.data.read_array(range).ok()
+                        .map(|records| ArrayOfRecordsWithOffsetData::new(records, #data_src))
+                        #maybe_unwrap #maybe_unwrap_or_def
+                }
+            } else {
+                quote!(self.data.read_array(range).ok() #maybe_unwrap #maybe_unwrap_or_def)
+            }
         } else {
             quote!(self.data.read_at(range.start).ok() #maybe_unwrap #maybe_unwrap_or_def)
         };
@@ -924,6 +942,39 @@ impl Field {
         }
     }
 
+    /// A getter on `OffsetResolving<Record>` that resolves this offset field
+    /// against the stored data.
+    ///
+    /// This is the same getter generated by [`typed_offset_field_getter`] for
+    /// the record itself, minus the `data` argument (to which it delegates,
+    /// passing the stored data).
+    ///
+    /// [`typed_offset_field_getter`]: Self::typed_offset_field_getter
+    pub(crate) fn offset_resolving_getter(&self, record_name: &syn::Ident) -> Option<TokenStream> {
+        let target = match &self.typ {
+            _ if self.attrs.offset_getter.is_some() => return None,
+            FieldType::Offset { target, .. } => target,
+            // a zerocopy record cannot contain an array
+            _ => return None,
+        };
+        let getter_name = self.offset_getter_name().unwrap();
+        let mut return_type = target.getter_return_type(false);
+        if self.is_nullable() {
+            return_type = quote!(Option<#return_type>);
+        }
+        let raw_name = &self.name;
+        let docs = format!(
+            " Attempt to resolve [`{raw_name}`][{record_name}::{raw_name}] \
+             against the data of the enclosing table."
+        );
+        Some(quote! {
+            #[doc = #docs]
+            pub fn #getter_name(&self) -> #return_type {
+                self.record().#getter_name(self.offset_data())
+            }
+        })
+    }
+
     fn is_count(&self) -> bool {
         self.attrs.count.is_some()
     }
@@ -1305,7 +1356,11 @@ impl Field {
             }
             FieldType::Array { inner_typ } => match inner_typ.as_ref() {
                 FieldType::Scalar { .. } | FieldType::Struct { .. } => {
-                    quote!(obj.#name().to_owned_obj(offset_data))
+                    // the wrapper is not a slice, but carries one
+                    let maybe_as_slice = self
+                        .array_of_offset_bearing_records
+                        .then(|| quote!(.as_slice()));
+                    quote!(obj.#name()#maybe_as_slice.to_owned_obj(offset_data))
                 }
                 FieldType::Offset { .. } => {
                     let offset_getter = self.offset_getter_name().unwrap();

font-codegen/src/parsing.rs

@@ -483,6 +483,38 @@ impl Items {
             }
         }
 
+        // Now that struct references are resolved, find the table fields that
+        // are arrays of records with generated offset getters; these are
+        // wrapped in `ArrayOfRecordsWithOffsetData` so that the records can
+        // resolve their offsets without being passed the table's data.
+        //
+        // Records defined in another module (`extern record`) are opaque to
+        // us, so their arrays stay bare slices.
+        let offset_bearing_records = self
+            .items
+            .values()
+            .filter_map(|item| match item {
+                Item::Record(record) if record.has_offset_resolving_getters() => {
+                    Some(record.name.clone())
+                }
+                _ => None,
+            })
+            .collect::<std::collections::HashSet<_>>();
+        for item in self.iter_mut() {
+            // only table fields: a record has no data of its own to pair
+            // an array of records with
+            let Item::Table(table) = item else { continue };
+            for field in table.fields.fields.iter_mut() {
+                if let FieldType::Array { inner_typ } = &field.typ {
+                    if matches!(inner_typ.as_ref(),
+                        FieldType::Struct { typ } if offset_bearing_records.contains(typ))
+                    {
+                        field.array_of_offset_bearing_records = true;
+                    }
+                }
+            }
+        }
+
         Ok(())
     }
 

font-codegen/src/parsing/fields.rs

@@ -35,6 +35,15 @@ pub(crate) struct Field {
     /// These fields must be present, which means reads can unwrap (and could even
     /// be unsafe.)
     pub(crate) validated_at_parse: bool,
+    /// `true` for a table field that is an array of records with generated
+    /// offset getters.
+    ///
+    /// Such fields are wrapped in `ArrayOfRecordsWithOffsetData`, which pairs
+    /// the records with the table's data so that their offsets can be resolved
+    /// without the caller needing to pass that data in.
+    ///
+    /// Like `validated_at_parse`, this is computed during resolution.
+    pub(crate) array_of_offset_bearing_records: bool,
 }
 
 #[derive(Debug, Clone, PartialEq, Eq, Hash)]
@@ -168,6 +177,7 @@ impl Parse for Field {
             typ,
             // computed later
             validated_at_parse: false,
+            array_of_offset_bearing_records: false,
         })
     }
 }

font-codegen/src/record.rs

@@ -48,6 +48,7 @@ pub(crate) fn generate(item: &Record, all_items: &Items) -> syn::Result<TokenStr
         }
     });
     let maybe_impl_read_with_args = (has_read_args).then(|| generate_read_with_args(item));
+    let maybe_offset_resolving_impl = generate_offset_resolving_impl(item);
     let maybe_extra_traits = item
         .gets_extra_traits(all_items)
         .then(|| quote!(PartialEq, Eq, PartialOrd, Ord, Hash,));
@@ -65,10 +66,30 @@ pub(crate) fn generate(item: &Record, all_items: &Items) -> syn::Result<TokenStr
 
     #maybe_impl_fixed_size
     #maybe_impl_read_with_args
+    #maybe_offset_resolving_impl
     #traversal_impl
         })
 }
 
+/// For a fixed-size record with offsets, getters on `OffsetResolving<Record>`
+/// that resolve those offsets against the stored data.
+fn generate_offset_resolving_impl(item: &Record) -> Option<TokenStream> {
+    if !item.has_offset_resolving_getters() {
+        return None;
+    }
+    let name = &item.name;
+    let getters = item
+        .fields
+        .iter()
+        .filter_map(|fld| fld.offset_resolving_getter(name))
+        .collect::<Vec<_>>();
+    Some(quote! {
+        impl<'a> OffsetResolving<'a, #name> {
+            #( #getters )*
+        }
+    })
+}
+
 fn generate_read_with_args(item: &Record) -> TokenStream {
     assert!(item.attrs.read_args.is_some()); // expected this to be checked already
                                              //
@@ -393,6 +414,22 @@ impl Record {
         self.fields.iter().all(Field::is_zerocopy_compatible)
     }
 
+    /// `true` if this is a fixed-size record with at least one generated
+    /// offset getter.
+    ///
+    /// Arrays of such records are wrapped in `ArrayOfRecordsWithOffsetData`,
+    /// and the record gets resolving getters on `OffsetResolving<Self>`.
+    /// Offsets with an `#[offset_getter]` attribute don't count: their getters
+    /// are written by hand, and codegen cannot know what data they resolve
+    /// against (`NameRecord` resolves against a separate storage area, for
+    /// instance).
+    pub(crate) fn has_offset_resolving_getters(&self) -> bool {
+        self.is_zerocopy()
+            && self.fields.iter().any(|fld| {
+                matches!(fld.typ, FieldType::Offset { .. }) && fld.attrs.offset_getter.is_none()
+            })
+    }
+
     fn gets_extra_traits(&self, all_items: &Items) -> bool {
         self.fields
             .iter()