Small fixes ♻

7 files changed, 81 insertions, 84 deletions

diff --git a/bench/src/bench.rs b/bench/src/bench.rs
index 88e2388d..0999b246 100644
--- a/bench/src/bench.rs
+++ b/bench/src/bench.rs
@@ -44,7 +44,7 @@ fn benchmarks(c: &mut Criterion) {
     bench!("exec-coma": exec(&mut env, &syntax_tree, &expr_map, state.clone()));
     bench!("layout-coma": layout(&mut env, &layout_tree));
     bench!("typeset-coma": typeset(&mut env, COMA, &scope, state.clone()));
-    bench!("export-pdf-coma": pdf::export(&frames, &env));
+    bench!("export-pdf-coma": pdf::export(&env, &frames));
 }
 
 criterion_group!(benches, benchmarks);
diff --git a/src/eval/capture.rs b/src/eval/capture.rs
index bbabc503..163aa24e 100644
--- a/src/eval/capture.rs
+++ b/src/eval/capture.rs
@@ -16,12 +16,12 @@ impl<'a> CapturesVisitor<'a> {
     pub fn new(external: &'a Scopes) -> Self {
         Self {
             external,
-            internal: Scopes::default(),
+            internal: Scopes::new(),
             captures: Scope::new(),
         }
     }
 
-    /// Return the scope of capture variables.
+    /// Return the scope of captured variables.
     pub fn finish(self) -> Scope {
         self.captures
     }
diff --git a/src/eval/mod.rs b/src/eval/mod.rs
index 57a37f38..47f997eb 100644
--- a/src/eval/mod.rs
+++ b/src/eval/mod.rs
@@ -30,7 +30,7 @@ pub fn eval(env: &mut Env, tree: &Tree, scope: &Scope) -> Pass<ExprMap> {
     Pass::new(map, ctx.diags)
 }
 
-/// A map from expression to values to evaluated to.
+/// A map from expressions to the values they evaluated to.
 ///
 /// The raw pointers point into the expressions contained in some [tree](Tree).
 /// Since the lifetime is erased, the tree could go out of scope while the hash
@@ -89,7 +89,7 @@ impl Eval for Tree {
             }
         }
 
-        let mut visitor = ExprVisitor { map: HashMap::new(), ctx };
+        let mut visitor = ExprVisitor { map: ExprMap::new(), ctx };
         visitor.visit_tree(self);
         visitor.map
     }
@@ -260,9 +260,8 @@ impl ExprBinary {
     where
         F: FnOnce(Value, Value) -> Value,
     {
-        let lhs = self.lhs.eval(ctx);
-
         // Short-circuit boolean operations.
+        let lhs = self.lhs.eval(ctx);
         match (self.op, &lhs) {
             (BinOp::And, Value::Bool(false)) => return lhs,
             (BinOp::Or, Value::Bool(true)) => return lhs,
@@ -270,16 +269,15 @@ impl ExprBinary {
         }
 
         let rhs = self.rhs.eval(ctx);
-
         if lhs == Value::Error || rhs == Value::Error {
             return Value::Error;
         }
 
-        let (l, r) = (lhs.type_name(), rhs.type_name());
-
+        // Save type names before we consume the values in case of error.
+        let types = (lhs.type_name(), rhs.type_name());
         let out = op(lhs, rhs);
         if out == Value::Error {
-            self.error(ctx, l, r);
+            self.error(ctx, types);
         }
 
         out
@@ -290,54 +288,47 @@ impl ExprBinary {
     where
         F: FnOnce(Value, Value) -> Value,
     {
-        let rhs = self.rhs.eval(ctx);
-
-        let lhs_span = self.lhs.span();
         let slot = if let Expr::Ident(id) = self.lhs.as_ref() {
             match ctx.scopes.get(id) {
-                Some(slot) => slot,
+                Some(slot) => Rc::clone(slot),
                 None => {
-                    ctx.diag(error!(lhs_span, "unknown variable"));
+                    ctx.diag(error!(self.lhs.span(), "unknown variable"));
                     return Value::Error;
                 }
             }
         } else {
-            ctx.diag(error!(lhs_span, "cannot assign to this expression"));
+            ctx.diag(error!(self.lhs.span(), "cannot assign to this expression"));
             return Value::Error;
         };
 
-        let (constant, err, value) = if let Ok(mut inner) = slot.try_borrow_mut() {
-            let lhs = std::mem::take(&mut *inner);
-            let types = (lhs.type_name(), rhs.type_name());
-
-            *inner = op(lhs, rhs);
-            if *inner == Value::Error {
-                (false, Some(types), Value::Error)
-            } else {
-                (false, None, Value::None)
+        let rhs = self.rhs.eval(ctx);
+        let mut mutable = match slot.try_borrow_mut() {
+            Ok(mutable) => mutable,
+            Err(_) => {
+                ctx.diag(error!(self.lhs.span(), "cannot assign to a constant"));
+                return Value::Error;
             }
-        } else {
-            (true, None, Value::Error)
         };
 
-        if constant {
-            ctx.diag(error!(lhs_span, "cannot assign to a constant"));
-        }
+        let lhs = std::mem::take(&mut *mutable);
+        let types = (lhs.type_name(), rhs.type_name());
+        *mutable = op(lhs, rhs);
 
-        if let Some((l, r)) = err {
-            self.error(ctx, l, r);
+        if *mutable == Value::Error {
+            self.error(ctx, types);
+            return Value::Error;
         }
 
-        value
+        Value::None
     }
 
-    fn error(&self, ctx: &mut EvalContext, l: &str, r: &str) {
+    fn error(&self, ctx: &mut EvalContext, (a, b): (&str, &str)) {
         ctx.diag(error!(self.span, "{}", match self.op {
-            BinOp::Add => format!("cannot add {} and {}", l, r),
-            BinOp::Sub => format!("cannot subtract {1} from {0}", l, r),
-            BinOp::Mul => format!("cannot multiply {} with {}", l, r),
-            BinOp::Div => format!("cannot divide {} by {}", l, r),
-            _ => format!("cannot apply '{}' to {} and {}", self.op.as_str(), l, r),
+            BinOp::Add => format!("cannot add {} and {}", a, b),
+            BinOp::Sub => format!("cannot subtract {1} from {0}", a, b),
+            BinOp::Mul => format!("cannot multiply {} with {}", a, b),
+            BinOp::Div => format!("cannot divide {} by {}", a, b),
+            _ => format!("cannot apply '{}' to {} and {}", self.op.as_str(), a, b),
         }));
     }
 }
@@ -350,6 +341,7 @@ impl Eval for ExprCall {
 
         if let Value::Func(func) = callee {
             let func = func.clone();
+
             let mut args = self.args.eval(ctx);
             let returned = func(ctx, &mut args);
             args.finish(ctx);
@@ -371,25 +363,28 @@ impl Eval for ExprArgs {
     type Output = ValueArgs;
 
     fn eval(&self, ctx: &mut EvalContext) -> Self::Output {
-        let items = self
-            .items
-            .iter()
-            .map(|arg| match arg {
-                ExprArg::Pos(expr) => ValueArg {
-                    name: None,
-                    value: Spanned::new(expr.eval(ctx), expr.span()),
-                },
-                ExprArg::Named(Named { name, expr }) => ValueArg {
-                    name: Some(Spanned::new(name.string.clone(), name.span)),
-                    value: Spanned::new(expr.eval(ctx), expr.span()),
-                },
-            })
-            .collect();
-
+        let items = self.items.iter().map(|arg| arg.eval(ctx)).collect();
         ValueArgs { span: self.span, items }
     }
 }
 
+impl Eval for ExprArg {
+    type Output = ValueArg;
+
+    fn eval(&self, ctx: &mut EvalContext) -> Self::Output {
+        match self {
+            Self::Pos(expr) => ValueArg {
+                name: None,
+                value: Spanned::new(expr.eval(ctx), expr.span()),
+            },
+            Self::Named(Named { name, expr }) => ValueArg {
+                name: Some(Spanned::new(name.string.clone(), name.span)),
+                value: Spanned::new(expr.eval(ctx), expr.span()),
+            },
+        }
+    }
+}
+
 impl Eval for ExprLet {
     type Output = Value;
 
@@ -408,6 +403,7 @@ impl Eval for ExprIf {
 
     fn eval(&self, ctx: &mut EvalContext) -> Self::Output {
         let condition = self.condition.eval(ctx);
+
         if let Value::Bool(boolean) = condition {
             return if boolean {
                 self.if_body.eval(ctx)
@@ -432,7 +428,7 @@ impl Eval for ExprFor {
     type Output = Value;
 
     fn eval(&self, ctx: &mut EvalContext) -> Self::Output {
-        macro_rules! iterate {
+        macro_rules! iter {
             (for ($($binding:ident => $value:ident),*) in $iter:expr) => {{
                 let mut output = vec![];
 
@@ -454,16 +450,16 @@ impl Eval for ExprFor {
         let iter = self.iter.eval(ctx);
         let value = match (self.pattern.clone(), iter) {
             (ForPattern::Value(v), Value::Str(string)) => {
-                iterate!(for (v => value) in string.chars().map(|c| Value::Str(c.into())))
+                iter!(for (v => value) in string.chars().map(|c| Value::Str(c.into())))
             }
             (ForPattern::Value(v), Value::Array(array)) => {
-                iterate!(for (v => value) in array.into_iter())
+                iter!(for (v => value) in array.into_iter())
             }
             (ForPattern::Value(v), Value::Dict(dict)) => {
-                iterate!(for (v => value) in dict.into_iter().map(|p| p.1))
+                iter!(for (v => value) in dict.into_iter().map(|p| p.1))
             }
             (ForPattern::KeyValue(k, v), Value::Dict(dict)) => {
-                iterate!(for (k => key, v => value) in dict.into_iter())
+                iter!(for (k => key, v => value) in dict.into_iter())
             }
 
             (ForPattern::KeyValue(_, _), Value::Str(_))
diff --git a/src/eval/value.rs b/src/eval/value.rs
index ab54bf53..ce89d453 100644
--- a/src/eval/value.rs
+++ b/src/eval/value.rs
@@ -124,16 +124,16 @@ pub type ValueTemplate = Vec<TemplateNode>;
 
 /// One chunk of a template.
 ///
-/// Evaluating a template expression creates only a single chunk. Adding two
-/// such templates yields a two-chunk template.
+/// Evaluating a template expression creates only a single node. Adding multiple
+/// templates can yield multi-node templates.
 #[derive(Debug, Clone, PartialEq)]
 pub enum TemplateNode {
     /// A template that consists of a syntax tree plus already evaluated
     /// expression.
     Tree {
-        /// The tree of this template part.
+        /// The syntax tree of the corresponding template expression.
         tree: Rc<Tree>,
-        /// The evaluated expressions.
+        /// The evaluated expressions for the `tree`.
         map: ExprMap,
     },
     /// A template that can implement custom behaviour.
@@ -242,7 +242,7 @@ impl ValueArgs {
     where
         T: Cast<Spanned<Value>>,
     {
-        (0 .. self.items.len()).find_map(move |i| self.try_take(ctx, i))
+        (0 .. self.items.len()).find_map(move |i| self.try_take(&mut ctx.diags, i))
     }
 
     /// Find and remove the first convertible positional argument, producing an
@@ -259,17 +259,18 @@ impl ValueArgs {
     }
 
     /// Filter out and remove all convertible positional arguments.
-    pub fn filter<'a, 'b: 'a, T>(
+    pub fn filter<'a, T>(
         &'a mut self,
-        ctx: &'a mut EvalContext<'b>,
-    ) -> impl Iterator<Item = T> + Captures<'a> + Captures<'b>
+        ctx: &'a mut EvalContext,
+    ) -> impl Iterator<Item = T> + 'a
     where
         T: Cast<Spanned<Value>>,
     {
+        let diags = &mut ctx.diags;
         let mut i = 0;
         std::iter::from_fn(move || {
             while i < self.items.len() {
-                if let Some(val) = self.try_take(ctx, i) {
+                if let Some(val) = self.try_take(diags, i) {
                     return Some(val);
                 }
                 i += 1;
@@ -328,7 +329,7 @@ impl ValueArgs {
 
     /// Try to take and cast a positional argument in the i'th slot into `T`,
     /// putting it back if the conversion fails.
-    fn try_take<T>(&mut self, ctx: &mut EvalContext, i: usize) -> Option<T>
+    fn try_take<T>(&mut self, diags: &mut DiagSet, i: usize) -> Option<T>
     where
         T: Cast<Spanned<Value>>,
     {
@@ -346,7 +347,7 @@ impl ValueArgs {
             }
             CastResult::Warn(t, m) => {
                 self.items.remove(i);
-                ctx.diag(warning!(span, "{}", m));
+                diags.insert(warning!(span, "{}", m));
                 Some(t)
             }
             CastResult::Err(value) => {
diff --git a/src/export/pdf.rs b/src/export/pdf.rs
index b2a6bbfd..9f355278 100644
--- a/src/export/pdf.rs
+++ b/src/export/pdf.rs
@@ -25,8 +25,8 @@ use crate::layout::{Element, Fill, Frame, Shape};
 /// images can be included in the _PDF_.
 ///
 /// Returns the raw bytes making up the _PDF_ document.
-pub fn export(frames: &[Frame], env: &Env) -> Vec<u8> {
-    PdfExporter::new(frames, env).write()
+pub fn export(env: &Env, frames: &[Frame]) -> Vec<u8> {
+    PdfExporter::new(env, frames).write()
 }
 
 struct PdfExporter<'a> {
@@ -39,7 +39,7 @@ struct PdfExporter<'a> {
 }
 
 impl<'a> PdfExporter<'a> {
-    fn new(frames: &'a [Frame], env: &'a Env) -> Self {
+    fn new(env: &'a Env, frames: &'a [Frame]) -> Self {
         let mut writer = PdfWriter::new(1, 7);
         writer.set_indent(2);
 
diff --git a/src/main.rs b/src/main.rs
index c221f3b6..ae655253 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -65,7 +65,7 @@ fn main() -> anyhow::Result<()> {
         }
     }
 
-    let pdf_data = pdf::export(&frames, &env);
+    let pdf_data = pdf::export(&env, &frames);
     fs::write(&dest_path, pdf_data).context("Failed to write PDF file.")?;
 
     Ok(())
diff --git a/tests/typeset.rs b/tests/typeset.rs
index 8932c483..7f8646be 100644
--- a/tests/typeset.rs
+++ b/tests/typeset.rs
@@ -168,11 +168,11 @@ fn test(
     }
 
     if !frames.is_empty() {
-        let pdf_data = pdf::export(&frames, &env);
+        let pdf_data = pdf::export(&env, &frames);
         fs::create_dir_all(&pdf_path.parent().unwrap()).unwrap();
         fs::write(pdf_path, pdf_data).unwrap();
 
-        let canvas = draw(&frames, &env, 2.0);
+        let canvas = draw(&env, &frames, 2.0);
         fs::create_dir_all(&png_path.parent().unwrap()).unwrap();
         canvas.pixmap.save_png(png_path).unwrap();
 
@@ -339,7 +339,7 @@ fn print_diag(diag: &Diag, map: &LineMap, lines: u32) {
     println!("{}: {}-{}: {}", diag.level, start, end, diag.message);
 }
 
-fn draw(frames: &[Frame], env: &Env, pixel_per_pt: f32) -> Canvas {
+fn draw(env: &Env, frames: &[Frame], pixel_per_pt: f32) -> Canvas {
     let pad = Length::pt(5.0);
 
     let height = pad + frames.iter().map(|l| l.size.height + pad).sum::<Length>();
@@ -380,13 +380,13 @@ fn draw(frames: &[Frame], env: &Env, pixel_per_pt: f32) -> Canvas {
             let pos = origin + pos;
             match element {
                 Element::Text(shaped) => {
-                    draw_text(&mut canvas, pos, env, shaped);
+                    draw_text(env, &mut canvas, pos, shaped);
                 }
                 Element::Image(image) => {
-                    draw_image(&mut canvas, pos, env, image);
+                    draw_image(env, &mut canvas, pos, image);
                 }
                 Element::Geometry(geom) => {
-                    draw_geometry(&mut canvas, pos, env, geom);
+                    draw_geometry(env, &mut canvas, pos, geom);
                 }
             }
         }
@@ -397,7 +397,7 @@ fn draw(frames: &[Frame], env: &Env, pixel_per_pt: f32) -> Canvas {
     canvas
 }
 
-fn draw_text(canvas: &mut Canvas, pos: Point, env: &Env, shaped: &Shaped) {
+fn draw_text(env: &Env, canvas: &mut Canvas, pos: Point, shaped: &Shaped) {
     let face = env.fonts.face(shaped.face).get();
 
     for (&glyph, &offset) in shaped.glyphs.iter().zip(&shaped.offsets) {
@@ -423,7 +423,7 @@ fn draw_text(canvas: &mut Canvas, pos: Point, env: &Env, shaped: &Shaped) {
     }
 }
 
-fn draw_geometry(canvas: &mut Canvas, pos: Point, _: &Env, element: &Geometry) {
+fn draw_geometry(_: &Env, canvas: &mut Canvas, pos: Point, element: &Geometry) {
     let x = pos.x.to_pt() as f32;
     let y = pos.y.to_pt() as f32;
 
@@ -443,7 +443,7 @@ fn draw_geometry(canvas: &mut Canvas, pos: Point, _: &Env, element: &Geometry) {
     };
 }
 
-fn draw_image(canvas: &mut Canvas, pos: Point, env: &Env, element: &Image) {
+fn draw_image(env: &Env, canvas: &mut Canvas, pos: Point, element: &Image) {
     let img = &env.resources.loaded::<ImageResource>(element.res);
 
     let mut pixmap = Pixmap::new(img.buf.width(), img.buf.height()).unwrap();