Review One: A Set Rules Story

4 files changed, 107 insertions, 31 deletions

diff --git a/src/eval/class.rs b/src/eval/class.rs
index 45674933..c4393b8a 100644
--- a/src/eval/class.rs
+++ b/src/eval/class.rs
@@ -6,14 +6,42 @@ use super::{Args, EvalContext, Node, Styles};
 use crate::diag::TypResult;
 use crate::util::EcoString;
 
-/// A class of nodes.
+/// A class of [nodes](Node).
+///
+/// You can [construct] an instance of a class in Typst code by invoking the
+/// class as a callable. This always produces some node, but not necessarily one
+/// of fixed type. For example, the `text` constructor does not actually create
+/// a [`TextNode`]. Instead it applies styling to whatever node you pass in and
+/// returns it structurally unchanged.
+///
+/// The arguments you can pass to a class constructor fall into two categories:
+/// Data that is inherent to the instance (e.g. the text of a heading) and style
+/// properties (e.g. the fill color of a heading). As the latter are often
+/// shared by many instances throughout a document, they can also be
+/// conveniently configured through class's [`set`] rule. Then, they apply to
+/// all nodes that are instantiated into the template where the `set` was
+/// executed.
+///
+/// ```typst
+/// This is normal.
+/// [
+///   #set text(weight: "bold")
+///   #set heading(fill: blue)
+///   = A blue & bold heading
+/// ]
+/// Normal again.
+/// ```
+///
+/// [construct]: Self::construct
+/// [`TextNode`]: crate::library::TextNode
+/// [`set`]: Self::set
 #[derive(Clone)]
 pub struct Class(Rc<Inner<dyn Bounds>>);
 
 /// The unsized structure behind the [`Rc`].
 struct Inner<T: ?Sized> {
     name: EcoString,
-    dispatch: T,
+    shim: T,
 }
 
 impl Class {
@@ -22,10 +50,10 @@ impl Class {
     where
         T: Construct + Set + 'static,
     {
-        Self(Rc::new(Inner {
-            name,
-            dispatch: Dispatch::<T>(PhantomData),
-        }))
+        // By specializing the shim to `T`, its vtable will contain T's
+        // `Construct` and `Set` impls (through the `Bounds` trait), enabling us
+        // to use them in the class's methods.
+        Self(Rc::new(Inner { name, shim: Shim::<T>(PhantomData) }))
     }
 
     /// The name of the class.
@@ -34,13 +62,22 @@ impl Class {
     }
 
     /// Construct an instance of the class.
+    ///
+    /// This parses both property and data arguments (in this order) and styles
+    /// the node constructed from the data with the style properties.
     pub fn construct(&self, ctx: &mut EvalContext, args: &mut Args) -> TypResult<Node> {
-        self.0.dispatch.construct(ctx, args)
+        let mut styles = Styles::new();
+        self.set(args, &mut styles)?;
+        let node = self.0.shim.construct(ctx, args)?;
+        Ok(node.styled(styles))
     }
 
     /// Execute the class's set rule.
-    pub fn set(&self, styles: &mut Styles, args: &mut Args) -> TypResult<()> {
-        self.0.dispatch.set(styles, args)
+    ///
+    /// This parses property arguments and writes the resulting styles into the
+    /// given style map. There are no further side effects.
+    pub fn set(&self, args: &mut Args, styles: &mut Styles) -> TypResult<()> {
+        self.0.shim.set(args, styles)
     }
 }
 
@@ -54,7 +91,8 @@ impl Debug for Class {
 
 impl PartialEq for Class {
     fn eq(&self, other: &Self) -> bool {
-        // We cast to thin pointers for comparison.
+        // We cast to thin pointers for comparison because we don't want to
+        // compare vtables (there can be duplicate vtables across codegen units).
         std::ptr::eq(
             Rc::as_ptr(&self.0) as *const (),
             Rc::as_ptr(&other.0) as *const (),
@@ -75,19 +113,19 @@ pub trait Construct {
 pub trait Set {
     /// Parse the arguments and insert style properties of this class into the
     /// given style map.
-    fn set(styles: &mut Styles, args: &mut Args) -> TypResult<()>;
+    fn set(args: &mut Args, styles: &mut Styles) -> TypResult<()>;
 }
 
-/// Zero-sized struct whose vtable contains the constructor and set rule of a
-/// class.
-struct Dispatch<T>(PhantomData<T>);
-
+/// Rewires the operations available on a class in an object-safe way. This is
+/// only implemented by the zero-sized `Shim` struct.
 trait Bounds {
     fn construct(&self, ctx: &mut EvalContext, args: &mut Args) -> TypResult<Node>;
-    fn set(&self, styles: &mut Styles, args: &mut Args) -> TypResult<()>;
+    fn set(&self, args: &mut Args, styles: &mut Styles) -> TypResult<()>;
 }
 
-impl<T> Bounds for Dispatch<T>
+struct Shim<T>(PhantomData<T>);
+
+impl<T> Bounds for Shim<T>
 where
     T: Construct + Set,
 {
@@ -95,7 +133,7 @@ where
         T::construct(ctx, args)
     }
 
-    fn set(&self, styles: &mut Styles, args: &mut Args) -> TypResult<()> {
-        T::set(styles, args)
+    fn set(&self, args: &mut Args, styles: &mut Styles) -> TypResult<()> {
+        T::set(args, styles)
     }
 }
diff --git a/src/eval/mod.rs b/src/eval/mod.rs
index d05f2ddf..17cc46ef 100644
--- a/src/eval/mod.rs
+++ b/src/eval/mod.rs
@@ -167,8 +167,10 @@ impl Eval for Markup {
 
     fn eval(&self, ctx: &mut EvalContext) -> TypResult<Self::Output> {
         let prev = mem::take(&mut ctx.styles);
-        let mut seq = vec![];
-        for piece in self.nodes() {
+        let nodes = self.nodes();
+        let upper = nodes.size_hint().1.unwrap_or_default();
+        let mut seq = Vec::with_capacity(upper);
+        for piece in nodes {
             seq.push((piece.eval(ctx)?, ctx.styles.clone()));
         }
         ctx.styles = prev;
@@ -468,11 +470,9 @@ impl Eval for CallExpr {
             }
 
             Value::Class(class) => {
-                let mut styles = Styles::new();
-                class.set(&mut styles, &mut args)?;
                 let node = class.construct(ctx, &mut args)?;
                 args.finish()?;
-                Ok(Value::Node(node.styled(styles)))
+                Ok(Value::Node(node))
             }
 
             v => bail!(
@@ -651,7 +651,7 @@ impl Eval for SetExpr {
         let class = self.class();
         let class = class.eval(ctx)?.cast::<Class>().at(class.span())?;
         let mut args = self.args().eval(ctx)?;
-        class.set(&mut ctx.styles, &mut args)?;
+        class.set(&mut args, &mut ctx.styles)?;
         args.finish()?;
         Ok(Value::None)
     }
diff --git a/src/eval/node.rs b/src/eval/node.rs
index e2b02955..34a4f275 100644
--- a/src/eval/node.rs
+++ b/src/eval/node.rs
@@ -20,6 +20,10 @@ use crate::util::EcoString;
 /// A node is a composable intermediate representation that can be converted
 /// into a proper layout node by lifting it to a [block-level](PackedNode) or
 /// [root node](RootNode).
+///
+/// When you write `[Hi] + [you]` in Typst, this type's [`Add`] implementation
+/// is invoked. There, multiple nodes are combined into a single
+/// [`Sequence`](Self::Sequence) node.
 #[derive(Debug, PartialEq, Clone, Hash)]
 pub enum Node {
     /// A word space.
@@ -39,8 +43,24 @@ pub enum Node {
     /// A block node.
     Block(PackedNode),
     /// A page node.
-    Page(PackedNode),
-    /// A sequence of nodes (which may themselves contain sequences).
+    Page(PageNode),
+    /// Multiple nodes with attached styles.
+    ///
+    /// For example, the Typst template `[Hi *you!*]` would result in the
+    /// sequence:
+    /// ```ignore
+    /// Sequence([
+    ///   (Text("Hi"), {}),
+    ///   (Space, {}),
+    ///   (Text("you!"), { TextNode::STRONG: true }),
+    /// ])
+    /// ```
+    /// A sequence may contain nested sequences (meaning this variant
+    /// effectively allows nodes to form trees). All nested sequences can
+    /// equivalently be represented as a single flat sequence, but allowing
+    /// nesting doesn't hurt since we can just recurse into the nested sequences
+    /// during packing. Also, in theory, this allows better complexity when
+    /// adding (large) sequence nodes (just like for a text rope).
     Sequence(Vec<(Self, Styles)>),
 }
 
@@ -71,6 +91,7 @@ impl Node {
         match self {
             Self::Inline(inline) => Self::Inline(inline.styled(styles)),
             Self::Block(block) => Self::Block(block.styled(styles)),
+            Self::Page(page) => Self::Page(page.styled(styles)),
             other => Self::Sequence(vec![(other, styles)]),
         }
     }
@@ -224,11 +245,12 @@ impl Packer {
             Node::Block(block) => {
                 self.push_block(block.styled(styles));
             }
-            Node::Page(flow) => {
+            Node::Page(page) => {
                 if self.top {
                     self.pagebreak();
-                    self.pages.push(PageNode { child: flow, styles });
+                    self.pages.push(page.styled(styles));
                 } else {
+                    let flow = page.child.styled(page.styles);
                     self.push_block(flow.styled(styles));
                 }
             }
@@ -387,15 +409,27 @@ impl<T> Default for Builder<T> {
     }
 }
 
-/// Finite state machine for spacing coalescing.
+/// The kind of node that was last added to a flow or paragraph. A small finite
+/// state machine used to coalesce spaces.
+///
+/// Soft nodes can only exist when surrounded by `Any` nodes. Not at the
+/// start, end or next to hard nodes. This way, spaces at start and end of
+/// paragraphs and next to `#h(..)` goes away.
 enum Last<N> {
+    /// Start state, nothing there.
     None,
+    /// Text or a block node or something.
     Any,
+    /// Hard nodes: Linebreaks and explicit spacing.
     Hard,
+    /// Soft nodes: Word spaces and paragraph breaks. These are saved here
+    /// temporarily and then applied once an `Any` node appears.
     Soft(N),
 }
 
 impl<N> Last<N> {
+    /// Transition into the `Any` state and return a soft node to really add
+    /// now if currently in `Soft` state.
     fn any(&mut self) -> Option<N> {
         match mem::replace(self, Self::Any) {
             Self::Soft(soft) => Some(soft),
@@ -403,12 +437,16 @@ impl<N> Last<N> {
         }
     }
 
+    /// Transition into the `Soft` state, but only if in `Any`. Otherwise, the
+    /// soft node is discarded.
     fn soft(&mut self, soft: N) {
         if let Self::Any = self {
             *self = Self::Soft(soft);
         }
     }
 
+    /// Transition into the `Hard` state, discarding a possibly existing soft
+    /// node and preventing further soft nodes from being added.
     fn hard(&mut self) {
         *self = Self::Hard;
     }
diff --git a/src/eval/styles.rs b/src/eval/styles.rs
index 5304e0ad..1c4b17ae 100644
--- a/src/eval/styles.rs
+++ b/src/eval/styles.rs
@@ -3,7 +3,7 @@ use std::fmt::{self, Debug, Formatter};
 use std::hash::{Hash, Hasher};
 use std::rc::Rc;
 
-// Possible optimizations:
+// TODO(style): Possible optimizations:
 // - Ref-count map for cheaper cloning and smaller footprint
 // - Store map in `Option` to make empty maps non-allocating
 // - Store small properties inline