feat(errors): Report context during render

epage · epage · commit 73e26cf786a5 · 2018-01-20T13:01:19.000-07:00
Fixes #105, mostly. You get enough context. This is also more universal (hard to track line numbers in every context). Any further improvements will be a part of #138. This locks mostly locks down the error API, minus `FilterError` which won't happen until filters get refactored. See #114.
diff --git a/src/compiler/parser.rs b/src/compiler/parser.rs
@@ -51,7 +51,7 @@ pub fn unexpected_token_error<S: ToString>(expected: &str, actual: Option<S>) ->
 
 pub fn unexpected_token_error_string(expected: &str, actual: Option<String>) -> Error {
     let actual = actual.unwrap_or_else(|| "nothing".to_owned());
-    Error::with_msg(format!("Expected {}, found {}", expected, actual))
+    Error::with_msg(format!("Expected {}, found `{}`", expected, actual))
 }
 
 // creates an expression, which wraps everything that gets rendered
@@ -230,7 +230,7 @@ pub fn expect<'a, T>(tokens: &mut T, expected: &Token) -> Result<&'a Token>
 {
     match tokens.next() {
         Some(x) if x == expected => Ok(x),
-        x => Err(unexpected_token_error(&expected.to_string(), x)),
+        x => Err(unexpected_token_error(&format!("`{}`", expected), x)),
     }
 }
 
@@ -336,7 +336,7 @@ mod test_parse_output {
 
         let result = parse_output(&tokens);
         assert_eq!(result.unwrap_err().to_string(),
-                   "Parsing error: Expected an identifier, found 1");
+                   "liquid: Expected identifier, found `1`\n");
     }
 
     #[test]
@@ -345,7 +345,7 @@ mod test_parse_output {
 
         let result = parse_output(&tokens);
         assert_eq!(result.unwrap_err().to_string(),
-                   "Parsing error: Expected a comma or a pipe, found blabla");
+                   "liquid: Expected `,` | `|`, found `blabla`\n");
     }
 
     #[test]
@@ -354,7 +354,7 @@ mod test_parse_output {
 
         let result = parse_output(&tokens);
         assert_eq!(result.unwrap_err().to_string(),
-                   "Parsing error: Expected :, found 1");
+                   "liquid: Expected `:`, found `1`\n");
     }
 }
 
diff --git a/src/interpreter/context.rs b/src/interpreter/context.rs
@@ -14,7 +14,7 @@ pub fn unexpected_value_error<S: ToString>(expected: &str, actual: Option<S>) ->
 
 pub fn unexpected_value_error_string(expected: &str, actual: Option<String>) -> Error {
     let actual = actual.unwrap_or_else(|| "nothing".to_owned());
-    Error::with_msg(format!("Expected {}, found {}", expected, actual))
+    Error::with_msg(format!("Expected {}, found `{}`", expected, actual))
 }
 
 #[derive(Clone, Debug)]
diff --git a/src/interpreter/output.rs b/src/interpreter/output.rs
@@ -1,4 +1,8 @@
-use error::{Error, Result, ResultLiquidChainExt};
+use std::fmt;
+
+use itertools;
+
+use error::{Error, Result, ResultLiquidChainExt, ResultLiquidExt};
 use value::Value;
 
 use super::Context;
@@ -20,12 +24,30 @@ impl FilterPrototype {
     }
 }
 
+impl fmt::Display for FilterPrototype {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        write!(f,
+               "{}: {}",
+               self.name,
+               itertools::join(&self.arguments, ", "))
+    }
+}
+
 #[derive(Clone, Debug, PartialEq)]
 pub struct Output {
     entry: Argument,
     filters: Vec<FilterPrototype>,
 }
 
+impl fmt::Display for Output {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        write!(f,
+               "{} | {}",
+               self.entry,
+               itertools::join(&self.filters, " | "))
+    }
+}
+
 impl Renderable for Output {
     fn render(&self, context: &mut Context) -> Result<Option<String>> {
         let entry = self.apply_filters(context)?;
@@ -60,7 +82,10 @@ impl Output {
                 .map(|a| a.evaluate(context))
                 .collect();
             let arguments = arguments?;
-            entry = f.filter(&entry, &*arguments).chain("Filter error")?;
+            entry = f.filter(&entry, &*arguments)
+                .chain("Filter error")
+                .context("input", &entry)
+                .context_with(|| ("args".into(), itertools::join(&arguments, ", ")))?;
         }
 
         Ok(entry)
diff --git a/src/tags/assign_tag.rs b/src/tags/assign_tag.rs
@@ -6,16 +6,25 @@ use interpreter::Renderable;
 use compiler::LiquidOptions;
 use compiler::Token;
 use compiler::{parse_output, expect, unexpected_token_error};
+use compiler::ResultLiquidExt;
 
 #[derive(Clone, Debug)]
 struct Assign {
     dst: String,
     src: Output,
 }
 
+impl Assign {
+    fn trace(&self) -> String {
+        format!("{{% assign {} = {}%}}", self.dst, self.src)
+    }
+}
+
 impl Renderable for Assign {
     fn render(&self, context: &mut Context) -> Result<Option<String>> {
-        let value = self.src.apply_filters(context)?;
+        let value = self.src
+            .apply_filters(context)
+            .trace_with(|| self.trace().into())?;
         context.set_global_val(&self.dst, value);
         Ok(None)
     }
diff --git a/src/tags/capture_block.rs b/src/tags/capture_block.rs
@@ -15,13 +15,18 @@ struct Capture {
     template: Template,
 }
 
+impl Capture {
+    fn trace(&self) -> String {
+        format!("{{% capture {} %}}", self.id)
+    }
+}
+
 impl Renderable for Capture {
     fn render(&self, context: &mut Context) -> Result<Option<String>> {
-        let output = match self.template.render(context) {
-            Ok(Some(s)) => s.clone(),
-            Ok(None) => "".to_owned(),
-            Err(x) => return Err(x),
-        };
+        let output = self.template
+            .render(context)
+            .trace_with(|| self.trace().into())?
+            .unwrap_or_else(|| "".to_owned());
 
         context.set_global_val(&self.id, Value::scalar(output));
         Ok(None)
diff --git a/src/tags/case_block.rs b/src/tags/case_block.rs
@@ -32,6 +32,10 @@ impl CaseOption {
         }
         Ok(false)
     }
+
+    fn trace(&self) -> String {
+        format!("{{% when {} %}}", itertools::join(self.args.iter(), " or "))
+    }
 }
 
 #[derive(Debug)]
@@ -41,17 +45,30 @@ struct Case {
     else_block: Option<Template>,
 }
 
+impl Case {
+    fn trace(&self) -> String {
+        format!("{{% case {} %}}", self.target)
+    }
+}
+
 impl Renderable for Case {
     fn render(&self, context: &mut Context) -> Result<Option<String>> {
         let value = self.target.evaluate(context)?;
         for case in &self.cases {
             if case.evaluate(&value, context)? {
-                return case.template.render(context);
+                return case.template
+                    .render(context)
+                    .trace_with(|| case.trace().into())
+                    .trace_with(|| self.trace().into())
+                    .context_with(|| (self.target.to_string().into(), value.to_string()));
             }
         }
 
         if let Some(ref t) = self.else_block {
-            return t.render(context);
+            return t.render(context)
+                .trace_with(|| "{{% else %}}".to_owned().into())
+                .trace_with(|| self.trace().into())
+                .context_with(|| (self.target.to_string().into(), value.to_string()));
         }
 
         Ok(None)
diff --git a/src/tags/cycle_tag.rs b/src/tags/cycle_tag.rs
@@ -1,6 +1,6 @@
 use itertools;
 
-use error::Result;
+use error::{Result, ResultLiquidExt};
 
 use interpreter::Argument;
 use interpreter::Context;
@@ -15,9 +15,18 @@ struct Cycle {
     values: Vec<Argument>,
 }
 
+impl Cycle {
+    fn trace(&self) -> String {
+        format!("{{% cycle {} %}}",
+                itertools::join(self.values.iter(), ", "))
+    }
+}
+
 impl Renderable for Cycle {
     fn render(&self, context: &mut Context) -> Result<Option<String>> {
-        let value = context.cycle_element(&self.name, &self.values)?;
+        let value = context
+            .cycle_element(&self.name, &self.values)
+            .trace_with(|| self.trace().into())?;
         Ok(value.map(|v| v.to_string()))
     }
 }
diff --git a/src/tags/for_block.rs b/src/tags/for_block.rs
@@ -1,3 +1,4 @@
+use std::fmt;
 use std::slice::Iter;
 
 use itertools;
@@ -20,6 +21,31 @@ enum Range {
     Counted(Argument, Argument),
 }
 
+impl Range {
+    pub fn evaluate(&self, context: &Context) -> Result<Vec<Value>> {
+        let range = match *self {
+            Range::Array(ref array_id) => get_array(context, array_id)?,
+
+            Range::Counted(ref start_arg, ref stop_arg) => {
+                let start = int_argument(start_arg, context, "start")?;
+                let stop = int_argument(stop_arg, context, "end")?;
+                (start..stop).map(|x| Value::scalar(x as i32)).collect()
+            }
+        };
+
+        Ok(range)
+    }
+}
+
+impl fmt::Display for Range {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        match *self {
+            Range::Array(ref arr) => write!(f, "{}", arr),
+            Range::Counted(ref start, ref end) => write!(f, "({}..{})", start, end),
+        }
+    }
+}
+
 #[derive(Debug)]
 struct For {
     var_name: String,
@@ -31,6 +57,16 @@ struct For {
     reversed: bool,
 }
 
+impl For {
+    fn trace(&self) -> String {
+        trace_for_tag(&self.var_name,
+                      &self.range,
+                      self.limit,
+                      self.offset,
+                      self.reversed)
+    }
+}
+
 fn get_array(context: &Context, array_id: &Argument) -> Result<Vec<Value>> {
     let array = array_id.evaluate(context)?;
     match array {
@@ -39,49 +75,50 @@ fn get_array(context: &Context, array_id: &Argument) -> Result<Vec<Value>> {
     }
 }
 
-fn int_argument(arg: &Argument, context: &Context) -> Result<isize> {
+fn int_argument(arg: &Argument, context: &Context, arg_name: &str) -> Result<isize> {
     let value = arg.evaluate(context)?;
 
-    let value =
-        value
-            .as_scalar()
-            .and_then(Scalar::to_integer)
-            .ok_or_else(|| unexpected_value_error("whole number", Some(value.type_name())))?;
+    let value = value
+        .as_scalar()
+        .and_then(Scalar::to_integer)
+        .ok_or_else(|| unexpected_value_error("whole number", Some(value.type_name())))
+        .context_with(|| (arg_name.to_string().into(), value.to_string()))?;
 
     Ok(value as isize)
 }
 
-impl Renderable for For {
-    fn render(&self, context: &mut Context) -> Result<Option<String>> {
-        let mut range = match self.range {
-            Range::Array(ref array_id) => get_array(context, array_id)?,
+fn for_slice(range: &mut [Value], limit: Option<usize>, offset: usize, reversed: bool) -> &[Value] {
+    let end = match limit {
+        Some(n) => offset + n,
+        None => range.len(),
+    };
 
-            Range::Counted(ref start_arg, ref stop_arg) => {
-                let start = int_argument(start_arg, context)?;
-                let stop = int_argument(stop_arg, context)?;
-                (start..stop).map(|x| Value::scalar(x as i32)).collect()
-            }
-        };
+    let slice = if end > range.len() {
+        &mut range[offset..]
+    } else {
+        &mut range[offset..end]
+    };
 
-        let end = match self.limit {
-            Some(n) => self.offset + n,
-            None => range.len(),
-        };
+    if reversed {
+        slice.reverse();
+    };
 
-        let slice = if end > range.len() {
-            &mut range[self.offset..]
-        } else {
-            &mut range[self.offset..end]
-        };
+    slice
+}
 
-        if self.reversed {
-            slice.reverse();
-        };
+impl Renderable for For {
+    fn render(&self, context: &mut Context) -> Result<Option<String>> {
+        let mut range = self.range
+            .evaluate(context)
+            .trace_with(|| self.trace().into())?;
+        let range = for_slice(&mut range, self.limit, self.offset, self.reversed);
 
-        match slice.len() {
+        match range.len() {
             0 => {
                 if let Some(ref t) = self.else_template {
                     t.render(context)
+                        .trace_with(|| "{{% else %}}".to_owned().into())
+                        .trace_with(|| self.trace().into())
                 } else {
                     Ok(None)
                 }
@@ -93,7 +130,7 @@ impl Renderable for For {
                     let mut helper_vars = Object::new();
                     helper_vars.insert("length".to_owned(), Value::scalar(range_len as i32));
 
-                    for (i, v) in slice.iter().enumerate() {
+                    for (i, v) in range.iter().enumerate() {
                         helper_vars.insert("index0".to_owned(), Value::scalar(i as i32));
                         helper_vars.insert("index".to_owned(), Value::scalar((i + 1) as i32));
                         helper_vars.insert("rindex0".to_owned(),
@@ -105,7 +142,11 @@ impl Renderable for For {
 
                         scope.set_val("forloop", Value::Object(helper_vars.clone()));
                         scope.set_val(&self.var_name, v.clone());
-                        let inner = try!(self.item_template.render(&mut scope))
+                        let inner = self.item_template
+                            .render(&mut scope)
+                            .trace_with(|| self.trace().into())
+                            .context_with(|| (self.var_name.clone().into(), v.to_string()))
+                            .context("index", &(i + 1))?
                             .unwrap_or_else(String::new);
                         ret = ret + &inner;
 
@@ -159,10 +200,6 @@ fn trace_for_tag(var_name: &str,
     if reversed {
         parameters.push("reversed".to_owned());
     }
-    let range = match *range {
-        Range::Array(ref arr) => arr.to_string(),
-        Range::Counted(ref start, ref end) => format!("({}..{})", start, end),
-    };
     format!("{{% for {} in {} {} %}}",
             var_name,
             range,
diff --git a/src/tags/if_block.rs b/src/tags/if_block.rs
diff --git a/src/tags/include_tag.rs b/src/tags/include_tag.rs

Original file line number	Diff line number	Diff line change
`@@ -51,7 +51,7 @@ pub fn unexpected_token_error<S: ToString>(expected: &str, actual: Option<S>) ->`
`51`	`51`
`52`	`52`	`pub fn unexpected_token_error_string(expected: &str, actual: Option<String>) -> Error {`
`53`	`53`	`let actual = actual.unwrap_or_else(\|\| "nothing".to_owned());`
`54`		`- Error::with_msg(format!("Expected {}, found {}", expected, actual))`
	`54`	+ Error::with_msg(format!("Expected {}, found `{}`", expected, actual))
`55`	`55`	`}`
`56`	`56`
`57`	`57`	`// creates an expression, which wraps everything that gets rendered`
`@@ -230,7 +230,7 @@ pub fn expect<'a, T>(tokens: &mut T, expected: &Token) -> Result<&'a Token>`
`230`	`230`	`{`
`231`	`231`	`match tokens.next() {`
`232`	`232`	`Some(x) if x == expected => Ok(x),`
`233`		`- x => Err(unexpected_token_error(&expected.to_string(), x)),`
	`233`	+ x => Err(unexpected_token_error(&format!("`{}`", expected), x)),
`234`	`234`	`}`
`235`	`235`	`}`
`236`	`236`
`@@ -336,7 +336,7 @@ mod test_parse_output {`
`336`	`336`
`337`	`337`	`let result = parse_output(&tokens);`
`338`	`338`	`assert_eq!(result.unwrap_err().to_string(),`
`339`		`- "Parsing error: Expected an identifier, found 1");`
	`339`	+ "liquid: Expected identifier, found `1`\n");
`340`	`340`	`}`
`341`	`341`
`342`	`342`	`#[test]`
`@@ -345,7 +345,7 @@ mod test_parse_output {`
`345`	`345`
`346`	`346`	`let result = parse_output(&tokens);`
`347`	`347`	`assert_eq!(result.unwrap_err().to_string(),`
`348`		`- "Parsing error: Expected a comma or a pipe, found blabla");`
	`348`	+ "liquid: Expected `,` \| `\|`, found `blabla`\n");
`349`	`349`	`}`
`350`	`350`
`351`	`351`	`#[test]`
`@@ -354,7 +354,7 @@ mod test_parse_output {`
`354`	`354`
`355`	`355`	`let result = parse_output(&tokens);`
`356`	`356`	`assert_eq!(result.unwrap_err().to_string(),`
`357`		`- "Parsing error: Expected :, found 1");`
	`357`	+ "liquid: Expected `:`, found `1`\n");
`358`	`358`	`}`
`359`	`359`	`}`
`360`	`360`
Original file line number	Diff line number	Diff line change
`@@ -14,7 +14,7 @@ pub fn unexpected_value_error<S: ToString>(expected: &str, actual: Option<S>) ->`
`14`	`14`
`15`	`15`	`pub fn unexpected_value_error_string(expected: &str, actual: Option<String>) -> Error {`
`16`	`16`	`let actual = actual.unwrap_or_else(\|\| "nothing".to_owned());`
`17`		`- Error::with_msg(format!("Expected {}, found {}", expected, actual))`
	`17`	+ Error::with_msg(format!("Expected {}, found `{}`", expected, actual))
`18`	`18`	`}`
`19`	`19`
`20`	`20`	`#[derive(Clone, Debug)]`
Original file line number	Diff line number	Diff line change
`@@ -32,6 +32,10 @@ impl CaseOption {`
`32`	`32`	`}`
`33`	`33`	`Ok(false)`
`34`	`34`	`}`
	`35`	`+`
	`36`	`+ fn trace(&self) -> String {`
	`37`	`+ format!("{{% when {} %}}", itertools::join(self.args.iter(), " or "))`
	`38`	`+ }`
`35`	`39`	`}`
`36`	`40`
`37`	`41`	`#[derive(Debug)]`
`@@ -41,17 +45,30 @@ struct Case {`
`41`	`45`	`else_block: Option<Template>,`
`42`	`46`	`}`
`43`	`47`
	`48`	`+impl Case {`
	`49`	`+ fn trace(&self) -> String {`
	`50`	`+ format!("{{% case {} %}}", self.target)`
	`51`	`+ }`
	`52`	`+}`
	`53`	`+`
`44`	`54`	`impl Renderable for Case {`
`45`	`55`	`fn render(&self, context: &mut Context) -> Result<Option<String>> {`
`46`	`56`	`let value = self.target.evaluate(context)?;`
`47`	`57`	`for case in &self.cases {`
`48`	`58`	`if case.evaluate(&value, context)? {`
`49`		`- return case.template.render(context);`
	`59`	`+ return case.template`
	`60`	`+ .render(context)`
	`61`	`+ .trace_with(\|\| case.trace().into())`
	`62`	`+ .trace_with(\|\| self.trace().into())`
	`63`	`+ .context_with(\|\| (self.target.to_string().into(), value.to_string()));`
`50`	`64`	`}`
`51`	`65`	`}`
`52`	`66`
`53`	`67`	`if let Some(ref t) = self.else_block {`
`54`		`- return t.render(context);`
	`68`	`+ return t.render(context)`
	`69`	`+ .trace_with(\|\| "{{% else %}}".to_owned().into())`
	`70`	`+ .trace_with(\|\| self.trace().into())`
	`71`	`+ .context_with(\|\| (self.target.to_string().into(), value.to_string()));`
`55`	`72`	`}`
`56`	`73`
`57`	`74`	`Ok(None)`