Review One: A Set Rules Story

13 files changed, 131 insertions, 46 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
diff --git a/src/layout/mod.rs b/src/layout/mod.rs
index bc28e893..114e7491 100644
--- a/src/layout/mod.rs
+++ b/src/layout/mod.rs
@@ -119,7 +119,7 @@ impl Layout for EmptyNode {
     }
 }
 
-/// A packed layouting node with precomputed hash.
+/// A packed layouting node with style properties and a precomputed hash.
 #[derive(Clone)]
 pub struct PackedNode {
     /// The type-erased node.
diff --git a/src/library/heading.rs b/src/library/heading.rs
index c9777577..96ff2688 100644
--- a/src/library/heading.rs
+++ b/src/library/heading.rs
@@ -30,7 +30,7 @@ impl Construct for HeadingNode {
 }
 
 impl Set for HeadingNode {
-    fn set(styles: &mut Styles, args: &mut Args) -> TypResult<()> {
+    fn set(args: &mut Args, styles: &mut Styles) -> TypResult<()> {
         styles.set_opt(Self::FAMILY, args.named("family")?);
         styles.set_opt(Self::FILL, args.named("fill")?);
         Ok(())
diff --git a/src/library/list.rs b/src/library/list.rs
index 74f0abe8..25eb3600 100644
--- a/src/library/list.rs
+++ b/src/library/list.rs
@@ -35,7 +35,7 @@ impl<L: Labelling> Construct for ListNode<L> {
 }
 
 impl<L: Labelling> Set for ListNode<L> {
-    fn set(styles: &mut Styles, args: &mut Args) -> TypResult<()> {
+    fn set(args: &mut Args, styles: &mut Styles) -> TypResult<()> {
         styles.set_opt(Self::LABEL_INDENT, args.named("label-indent")?);
         styles.set_opt(Self::BODY_INDENT, args.named("body-indent")?);
         Ok(())
diff --git a/src/library/page.rs b/src/library/page.rs
index 7fbcd058..0e690770 100644
--- a/src/library/page.rs
+++ b/src/library/page.rs
@@ -12,7 +12,7 @@ pub fn pagebreak(_: &mut EvalContext, _: &mut Args) -> TypResult<Value> {
 }
 
 /// Layouts its child onto one or multiple pages.
-#[derive(Hash)]
+#[derive(Clone, PartialEq, Hash)]
 pub struct PageNode {
     /// The node producing the content.
     pub child: PackedNode,
@@ -44,12 +44,15 @@ impl PageNode {
 
 impl Construct for PageNode {
     fn construct(_: &mut EvalContext, args: &mut Args) -> TypResult<Node> {
-        Ok(Node::Page(args.expect::<Node>("body")?.into_block()))
+        Ok(Node::Page(Self {
+            child: args.expect::<Node>("body")?.into_block(),
+            styles: Styles::new(),
+        }))
     }
 }
 
 impl Set for PageNode {
-    fn set(styles: &mut Styles, args: &mut Args) -> TypResult<()> {
+    fn set(args: &mut Args, styles: &mut Styles) -> TypResult<()> {
         if let Some(paper) = args.named::<Paper>("paper")?.or_else(|| args.find()) {
             styles.set(Self::CLASS, paper.class());
             styles.set(Self::WIDTH, Smart::Custom(paper.width()));
@@ -79,6 +82,12 @@ impl Set for PageNode {
 }
 
 impl PageNode {
+    /// Style the node with styles from a style map.
+    pub fn styled(mut self, styles: Styles) -> Self {
+        self.styles.apply(&styles);
+        self
+    }
+
     /// Layout the page run into a sequence of frames, one per page.
     pub fn layout(&self, ctx: &mut LayoutContext) -> Vec<Rc<Frame>> {
         let prev = ctx.styles.clone();
diff --git a/src/library/par.rs b/src/library/par.rs
index 5dffd1c0..26280d8e 100644
--- a/src/library/par.rs
+++ b/src/library/par.rs
@@ -43,7 +43,7 @@ impl Construct for ParNode {
 }
 
 impl Set for ParNode {
-    fn set(styles: &mut Styles, args: &mut Args) -> TypResult<()> {
+    fn set(args: &mut Args, styles: &mut Styles) -> TypResult<()> {
         let spacing = args.named("spacing")?;
         let leading = args.named("leading")?;
 
diff --git a/src/library/spacing.rs b/src/library/spacing.rs
index 4c6c2017..b5ecce69 100644
--- a/src/library/spacing.rs
+++ b/src/library/spacing.rs
@@ -16,12 +16,12 @@ pub fn v(_: &mut EvalContext, args: &mut Args) -> TypResult<Value> {
     )))
 }
 
-/// A single run of text with the same style.
+/// Explicit spacing in a flow or paragraph.
 #[derive(Hash)]
 pub struct SpacingNode {
     /// The kind of spacing.
     pub kind: SpacingKind,
-    /// The rspacing's styles.
+    /// The spacing's styles.
     pub styles: Styles,
 }
 
diff --git a/src/library/stack.rs b/src/library/stack.rs
index 285ab9d5..8a1f0fd5 100644
--- a/src/library/stack.rs
+++ b/src/library/stack.rs
@@ -9,17 +9,17 @@ pub fn stack(_: &mut EvalContext, args: &mut Args) -> TypResult<Value> {
     let spacing = args.named("spacing")?;
 
     let mut children = vec![];
-    let mut delayed = None;
+    let mut deferred = None;
 
     // Build the list of stack children.
     for child in args.all() {
         match child {
-            StackChild::Spacing(_) => delayed = None,
+            StackChild::Spacing(_) => deferred = None,
             StackChild::Node(_) => {
-                if let Some(v) = delayed {
+                if let Some(v) = deferred {
                     children.push(StackChild::spacing(v));
                 }
-                delayed = spacing;
+                deferred = spacing;
             }
         }
         children.push(child);
diff --git a/src/library/text.rs b/src/library/text.rs
index 4ff9b5cd..99c68f79 100644
--- a/src/library/text.rs
+++ b/src/library/text.rs
@@ -133,7 +133,7 @@ impl Construct for TextNode {
 }
 
 impl Set for TextNode {
-    fn set(styles: &mut Styles, args: &mut Args) -> TypResult<()> {
+    fn set(args: &mut Args, styles: &mut Styles) -> TypResult<()> {
         let list = args.named("family")?.or_else(|| {
             let families: Vec<_> = args.all().collect();
             (!families.is_empty()).then(|| families)
diff --git a/tests/typ/style/set-site.typ b/tests/typ/style/set-site.typ
index 97a5672d..0a00e199 100644
--- a/tests/typ/style/set-site.typ
+++ b/tests/typ/style/set-site.typ
@@ -2,7 +2,7 @@
 // definition site of a template.
 
 ---
-// Test that text is affected by instantion-site bold.
+// Test that text is affected by instantiation-site bold.
 #let x = [World]
 Hello *{x}*