Code review, new stack test

src/layout/grid.rs

@@ -56,6 +56,8 @@ struct GridLayouter<'a> {
     cross: SpecAxis,
     /// The axis of the main direction.
     main: SpecAxis,
+    /// The original expand state of the target region.
+    expand: Spec<bool>,
     /// The column tracks including gutter tracks.
     cols: Vec<TrackSizing>,
     /// The row tracks including gutter tracks.
@@ -64,8 +66,6 @@ struct GridLayouter<'a> {
     children: &'a [AnyNode],
     /// The region to layout into.
     regions: Regions,
-    /// The original expand state of the target region.
-    original_expand: Spec<bool>,
     /// Resolved column sizes.
     rcols: Vec<Length>,
     /// The full main size of the current region.
@@ -138,7 +138,7 @@ impl<'a> GridLayouter<'a> {
         let rcols = vec![Length::zero(); cols.len()];
 
         // We use the regions only for auto row measurement and constraints.
-        let original_expand = regions.expand;
+        let expand = regions.expand;
         regions.expand = Gen::new(true, false).to_spec(main);
 
         Self {
@@ -147,9 +147,9 @@ impl<'a> GridLayouter<'a> {
             cols,
             rows,
             children: &grid.children,
-            constraints: Constraints::new(original_expand),
+            constraints: Constraints::new(expand),
             regions,
-            original_expand,
+            expand,
             rcols,
             lrows: vec![],
             full,
@@ -510,7 +510,7 @@ impl<'a> GridLayouter<'a> {
         self.used.main = Length::zero();
         self.fr = Fractional::zero();
         self.finished.push(output.constrain(self.constraints));
-        self.constraints = Constraints::new(self.original_expand);
+        self.constraints = Constraints::new(self.expand);
     }
 
     /// Get the node in the cell in column `x` and row `y`.

src/layout/incremental.rs

@@ -1,4 +1,5 @@
-use std::{collections::HashMap, ops::Deref};
+use std::collections::{hash_map::Entry, HashMap};
+use std::ops::Deref;
 
 use super::*;
 
@@ -6,7 +7,9 @@ use super::*;
 #[derive(Default, Debug, Clone)]
 pub struct LayoutCache {
     /// Maps from node hashes to the resulting frames and regions in which the
-    /// frames are valid.
+    /// frames are valid. The right hand side of the hash map is a vector of
+    /// results because across one or more compilations, multiple different
+    /// layouts of the same node may have been requested.
     pub frames: HashMap<u64, Vec<FramesEntry>>,
     /// In how many compilations this cache has been used.
     age: usize,
@@ -28,16 +31,11 @@ impl LayoutCache {
     where
         F: FnMut(usize) -> bool,
     {
-        for (_, hash_ident) in self.frames.iter_mut() {
-            hash_ident.retain(|entry| f(entry.level));
+        for (_, entries) in self.frames.iter_mut() {
+            entries.retain(|entry| f(entry.level));
         }
     }
 
-    /// Amount of items in the cache.
-    pub fn len(&self) -> usize {
-        self.frames.iter().map(|(_, e)| e.len()).sum()
-    }
-
     /// Prepare the cache for the next round of compilation
     pub fn turnaround(&mut self) {
         self.age += 1;
@@ -45,11 +43,11 @@ impl LayoutCache {
             for i in 0 .. (entry.temperature.len() - 1) {
                 entry.temperature[i] = entry.temperature[i + 1];
             }
-            *entry.temperature.last_mut().unwrap() = Some(0);
+            *entry.temperature.last_mut().unwrap() = 0;
         }
     }
 
-    /// What is the deepest level in the cache?
+    /// The amount of levels stored in the cache.
     pub fn levels(&self) -> usize {
         self.frames
             .iter()
@@ -85,16 +83,17 @@ impl LayoutCache {
         level: usize,
     ) {
         let entry = FramesEntry::new(frames, level);
-        if let Some(variants) = self.frames.get_mut(&hash) {
-            variants.push(entry);
-        } else {
-            self.frames.insert(hash, vec![entry]);
+        match self.frames.entry(hash) {
+            Entry::Occupied(o) => o.into_mut().push(entry),
+            Entry::Vacant(v) => {
+                v.insert(vec![entry]);
+            }
         }
     }
 }
 
-#[derive(Debug, Clone)]
 /// Cached frames from past layouting.
+#[derive(Debug, Clone)]
 pub struct FramesEntry {
     /// The cached frames for a node.
     pub frames: Vec<Constrained<Rc<Frame>>>,
@@ -103,15 +102,20 @@ pub struct FramesEntry {
     /// How much the element was accessed during the last five compilations, the
     /// most recent one being the last element. `None` variants indicate that
     /// the element is younger than five compilations.
-    temperature: [Option<usize>; 5],
+    temperature: [usize; 5],
+    /// For how long the element already exists.
+    age: usize,
 }
 
 impl FramesEntry {
     /// Construct a new instance.
     pub fn new(frames: Vec<Constrained<Rc<Frame>>>, level: usize) -> Self {
-        let mut temperature = [None; 5];
-        temperature[4] = Some(0);
-        Self { frames, level, temperature }
+        Self {
+            frames,
+            level,
+            temperature: [0; 5],
+            age: 1,
+        }
     }
 
     /// Checks if the cached [`Frame`] is valid for the given regions.
@@ -122,8 +126,7 @@ impl FramesEntry {
             }
         }
 
-        let tmp = self.temperature.get_mut(4).unwrap();
-        *tmp = Some(tmp.map_or(1, |x| x + 1));
+        self.temperature[4] = self.temperature[4] + 1;
 
         Some(self.frames.clone())
     }
@@ -131,43 +134,35 @@ impl FramesEntry {
     /// Get the amount of compilation cycles this item has remained in the
     /// cache.
     pub fn age(&self) -> usize {
-        let mut age = 0;
-        for &temp in self.temperature.iter().rev() {
-            if temp.is_none() {
-                break;
-            }
-            age += 1;
-        }
-        age
+        self.age
     }
 
     /// Get the amount of consecutive cycles in which this item has not
     /// been used.
     pub fn cooldown(&self) -> usize {
-        let mut age = 0;
+        let mut cycle = 0;
         for (i, &temp) in self.temperature.iter().enumerate().rev() {
-            match temp {
-                Some(temp) => {
-                    if temp > 0 {
-                        return self.temperature.len() - 1 - i;
-                    }
-                }
-                None => {
-                    return age;
+            if self.age > i {
+                if temp > 0 {
+                    return self.temperature.len() - 1 - i;
                 }
+            } else {
+                return cycle;
             }
-            age += 1
+
+            cycle += 1
         }
 
-        age
+        cycle
     }
 
     /// Whether this element was used in the last compilation cycle.
     pub fn hit(&self) -> bool {
-        self.temperature.last().unwrap().unwrap_or(0) != 0
+        self.temperature.last().unwrap() != &0
     }
 }
 
+/// These constraints describe regions that match them.
 #[derive(Debug, Copy, Clone, PartialEq)]
 pub struct Constraints {
     /// The minimum available length in the region.
@@ -218,49 +213,45 @@ impl Constraints {
         let base = regions.base.to_spec();
         let current = regions.current.to_spec();
 
-        let res = current.eq_by(&self.min, |&x, y| y.map_or(true, |y| x.fits(y)))
+        current.eq_by(&self.min, |&x, y| y.map_or(true, |y| x.fits(y)))
             && current.eq_by(&self.max, |x, y| y.map_or(true, |y| x < &y))
             && current.eq_by(&self.exact, |&x, y| y.map_or(true, |y| x.approx_eq(y)))
-            && base.eq_by(&self.base, |&x, y| y.map_or(true, |y| x.approx_eq(y)));
-
-        res
+            && base.eq_by(&self.base, |&x, y| y.map_or(true, |y| x.approx_eq(y)))
     }
 
-    /// Changes all constraints by adding the argument to them if they are set.
+    /// Changes all constraints by adding the `size` to them if they are `Some`.
     pub fn mutate(&mut self, size: Size, regions: &Regions) {
-        for x in std::array::IntoIter::new([
+        for spec in std::array::IntoIter::new([
             &mut self.min,
             &mut self.max,
             &mut self.exact,
             &mut self.base,
         ]) {
-            if let Some(horizontal) = x.horizontal.as_mut() {
+            if let Some(horizontal) = spec.horizontal.as_mut() {
                 *horizontal += size.width;
             }
-            if let Some(vertical) = x.vertical.as_mut() {
+            if let Some(vertical) = spec.vertical.as_mut() {
                 *vertical += size.height;
             }
         }
 
-        self.exact = zip(self.exact, regions.current.to_spec(), |_, o| o);
-        self.base = zip(self.base, regions.base.to_spec(), |_, o| o);
+        self.exact = override_if_some(self.exact, regions.current.to_spec());
+        self.base = override_if_some(self.base, regions.base.to_spec());
     }
 }
 
-fn zip<F>(
+fn override_if_some(
     one: Spec<Option<Length>>,
     other: Spec<Length>,
-    mut f: F,
-) -> Spec<Option<Length>>
-where
-    F: FnMut(Length, Length) -> Length,
-{
+) -> Spec<Option<Length>> {
     Spec {
-        vertical: one.vertical.map(|r| f(r, other.vertical)),
-        horizontal: one.horizontal.map(|r| f(r, other.horizontal)),
+        vertical: one.vertical.map(|_| other.vertical),
+        horizontal: one.horizontal.map(|_| other.horizontal),
     }
 }
 
+/// Carries an item that only applies to certain regions and the constraints
+/// that describe these regions.
 #[derive(Debug, Copy, Clone, PartialEq)]
 pub struct Constrained<T> {
     pub item: T,
@@ -275,8 +266,14 @@ impl<T> Deref for Constrained<T> {
     }
 }
 
+/// Extends length-related options by providing convenience methods for setting
+/// minimum and maximum lengths on them, even if they are `None`.
 pub trait OptionExt {
+    // Sets `other` as the value if the Option is `None` or if it contains a
+    // value larger than `other`.
     fn set_min(&mut self, other: Length);
+    // Sets `other` as the value if the Option is `None` or if it contains a
+    // value smaller than `other`.
     fn set_max(&mut self, other: Length);
 }
 

src/layout/pad.rs

@@ -15,7 +15,7 @@ impl Layout for PadNode {
         ctx: &mut LayoutContext,
         regions: &Regions,
     ) -> Vec<Constrained<Rc<Frame>>> {
-        let original = regions.clone();
+        let mut original = regions.clone();
         let mut regions = regions.map(|size| size - self.padding.resolve(size).size());
 
         let mut frames = self.child.layout(ctx, &regions);
@@ -39,6 +39,7 @@ impl Layout for PadNode {
             }
 
             regions.next();
+            original.next();
             *Rc::make_mut(&mut frame.item) = new;
         }
         frames

src/layout/par.rs

@@ -190,6 +190,7 @@ impl<'a> ParLayouter<'a> {
             // line cannot be broken up further.
             if !stack.regions.current.fits(line.size) {
                 if let Some((last_line, last_end)) = last.take() {
+                    // The region must not fit this line for the result to be valid.
                     if !stack.regions.current.width.fits(line.size.width) {
                         stack.constraints.max.horizontal.set_min(line.size.width);
                     }
@@ -213,6 +214,9 @@ impl<'a> ParLayouter<'a> {
             while !stack.regions.current.height.fits(line.size.height)
                 && !stack.regions.in_full_last()
             {
+                // Again, the line must not fit. It would if the space taken up
+                // plus the line height would fit, therefore the constraint
+                // below.
                 stack.constraints.max.vertical.set_min(
                     stack.full.height - stack.regions.current.height + line.size.height,
                 );
@@ -225,7 +229,7 @@ impl<'a> ParLayouter<'a> {
                     stack.overflowing = true;
                     break;
                 }
-                
+
                 stack.constraints.max.vertical.set_min(
                     stack.full.height - stack.regions.current.height + line.size.height,
                 );

src/layout/stack.rs

@@ -62,6 +62,8 @@ struct StackLayouter<'a> {
     ruler: Align,
     /// The constraints for the current region.
     constraints: Constraints,
+    /// Whether the last region can fit all the remaining content.
+    overflowing: bool,
     /// Offset, alignment and frame for all children that fit into the current
     /// region. The exact positions are not known yet.
     frames: Vec<(Length, Gen<Align>, Rc<Frame>)>,
@@ -92,6 +94,7 @@ impl<'a> StackLayouter<'a> {
             full,
             used: Gen::zero(),
             ruler: Align::Start,
+            overflowing: false,
             frames: vec![],
             finished: vec![],
         }
@@ -142,9 +145,12 @@ impl<'a> StackLayouter<'a> {
         }
 
         // Find a fitting region.
-        while !self.regions.current.get(self.main).fits(size.main)
-            && !self.regions.in_full_last()
-        {
+        while !self.regions.current.get(self.main).fits(size.main) {
+            if self.regions.in_full_last() {
+                self.overflowing = true;
+                break;
+            }
+
             self.constraints
                 .max
                 .get_mut(self.main)
@@ -203,6 +209,12 @@ impl<'a> StackLayouter<'a> {
             size = Size::new(width, width / aspect.into_inner());
         }
 
+        if self.overflowing {
+            self.constraints.min.vertical = None;
+            self.constraints.max.vertical = None;
+            self.constraints.exact = self.full.to_spec().map(Some);
+        }
+
         let mut output = Frame::new(size, size.height);
         let mut first = true;
 

tests/typ/layout/stack.typ

@@ -7,3 +7,12 @@
     rect(3cm, forest),
     rect(1cm, conifer),
 )
+
+---
+
+#let rect(width, color) = rect(width: 1cm, height: 0.4cm, fill: color)
+// This stack overflows.
+#box(height: 0.5cm, stack(
+    rect(3cm, forest),
+    rect(1cm, conifer),
+))

tests/typeset.rs

@@ -15,14 +15,14 @@ use walkdir::WalkDir;
 
 use typst::cache::Cache;
 use typst::color;
-use typst::diag::{Diag, DiagSet, Level};
-use typst::eval::{EvalContext, FuncArgs, FuncValue, Scope, Value};
-use typst::exec::State;
+use typst::diag::{Diag, DiagSet, Level, Pass};
+use typst::eval::{eval, EvalContext, FuncArgs, FuncValue, Scope, Value};
+use typst::exec::{exec, State};
 use typst::geom::{self, Length, Point, Sides, Size};
 use typst::image::ImageId;
-use typst::layout::{Element, Fill, Frame, Shape, Text};
+use typst::layout::{layout, Element, Fill, Frame, Shape, Text};
 use typst::loading::FsLoader;
-use typst::parse::{LineMap, Scanner};
+use typst::parse::{parse, LineMap, Scanner};
 use typst::syntax::{Location, Pos};
 
 const TYP_DIR: &str = "./typ";
@@ -224,12 +224,22 @@ fn test_part(
     state.page.size = Size::new(Length::pt(120.0), Length::inf());
     state.page.margins = Sides::splat(Some(Length::pt(10.0).into()));
 
-    let mut pass =
-        typst::typeset(loader, cache, Some(src_path), &src, &scope, state.clone());
+    let parsed = parse(src);
+    let evaluated = eval(
+        loader,
+        cache,
+        Some(src_path),
+        Rc::new(parsed.output),
+        &scope,
+    );
+    let executed = exec(&evaluated.output.template, state.clone());
+    let layouted = layout(loader, cache, &executed.output);
 
-    if !compare_ref {
-        pass.output.clear();
-    }
+    let mut diags = parsed.diags;
+    diags.extend(evaluated.diags);
+    diags.extend(executed.diags);
+
+    let mut pass = Pass::new(layouted, diags);
 
     let mut ok = true;
 
@@ -274,35 +284,25 @@ fn test_part(
 
         cache.layout.turnaround();
 
-        let mut cached_result =
-            typst::typeset(loader, cache, Some(src_path), &src, &scope, state.clone());
+        let cached_result = layout(loader, cache, &executed.output);
 
-        if !compare_ref {
-            cached_result.output.clear();
-        }
-
-        let misses: Vec<_> = cache
+        let misses = cache
             .layout
             .frames
             .iter()
             .flat_map(|(_, e)| e)
             .filter(|e| e.level == level && !e.hit() && e.age() == 2)
-            .collect();
+            .count();
 
-        if !misses.is_empty() {
+        if misses > 0 {
             ok = false;
-            let mut miss_count = 0;
-            for miss in misses {
-                dbg!(miss.frames.iter().map(|f| f.constraints).collect::<Vec<_>>());
-                miss_count += 1;
-            }
             println!(
                 "    Recompilation had {} cache misses on level {} (Subtest {}) ❌",
-                miss_count, level, i
+                misses, level, i
             );
         }
 
-        if cached_result != pass {
+        if cached_result != pass.output {
             ok = false;
             println!(
                 "    Recompilation of subtest {} differs from clean pass ❌",
@@ -314,6 +314,10 @@ fn test_part(
     cache.layout = reference_cache;
     cache.layout.turnaround();
 
+    if !compare_ref {
+        pass.output.clear();
+    }
+
     (ok, compare_ref, pass.output)
 }