From 84230a38be2b9438ea98075bc87739e809bb3c92 Mon Sep 17 00:00:00 2001 From: Ryan Bottriell Date: Sat, 8 Apr 2023 13:30:19 -0700 Subject: [PATCH 1/4] Fix error in parser error ownership Signed-off-by: Ryan Bottriell --- crates/core/src/parser/parser.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/core/src/parser/parser.rs b/crates/core/src/parser/parser.rs index 92335d8cb..bc79509e5 100644 --- a/crates/core/src/parser/parser.rs +++ b/crates/core/src/parser/parser.rs @@ -773,7 +773,7 @@ impl<'a> TagTokenIter<'a> { ::pest::error::ErrorVariant::CustomError { message: error_msg.to_string(), }, - self.position, + self.position.to_owned(), ); convert_pest_error(pest_error) } From d9b72fe54dba03f5a07612239ebfdd7c94415e9f Mon Sep 17 00:00:00 2001 From: Ryan Bottriell Date: Sat, 8 Apr 2023 14:30:57 -0700 Subject: [PATCH 2/4] Add tests for sub path with default filter Signed-off-by: Ryan Bottriell --- tests/filters.rs | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/tests/filters.rs b/tests/filters.rs index 4c9cfc398..efc2feaf4 100644 --- a/tests/filters.rs +++ b/tests/filters.rs @@ -539,3 +539,32 @@ fn test_compact() { let output = template.render(&globals).unwrap(); assert_eq!(output, "A C".to_string()); } + +#[test] +fn indexed_variable_default() { + let text = "{{ does.not.exist | default: 'default' }}"; + let globals = liquid::object!({}); + + let template = liquid::ParserBuilder::with_stdlib() + .build() + .unwrap() + .parse(text) + .unwrap(); + let output = template.render(&globals).unwrap(); + assert_eq!(output, "default".to_string()); +} + +#[test] +fn indexed_variable_no_default() { + let text = "{{ does.not.exist }}"; + let globals = liquid::object!({}); + + let template = liquid::ParserBuilder::with_stdlib() + .build() + .unwrap() + .parse(text) + .unwrap(); + template + .render(&globals) + .expect_err("should fail when the filter ends with a missing value"); +} From 59f081d720f5d10ffc0b0ed4029d2b8560421c4e Mon Sep 17 00:00:00 2001 From: Ryan Bottriell Date: Sat, 8 Apr 2023 13:31:29 -0700 Subject: [PATCH 3/4] Update filter chain to only reject missing values at the end of the chain Signed-off-by: Ryan Bottriell --- crates/core/src/parser/filter_chain.rs | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/crates/core/src/parser/filter_chain.rs b/crates/core/src/parser/filter_chain.rs index 09ab818f6..38561261c 100644 --- a/crates/core/src/parser/filter_chain.rs +++ b/crates/core/src/parser/filter_chain.rs @@ -7,6 +7,7 @@ use crate::model::{ValueCow, ValueView}; use crate::runtime::Expression; use crate::runtime::Renderable; use crate::runtime::Runtime; +use crate::Value; /// A `Value` expression. #[derive(Debug)] @@ -24,22 +25,22 @@ impl FilterChain { /// Process `Value` expression within `runtime`'s stack. pub fn evaluate<'s>(&'s self, runtime: &'s dyn Runtime) -> Result> { // take either the provided value or the value from the provided variable - let mut entry = self.entry.evaluate(runtime)?; + let mut entry = self.entry.evaluate(runtime); // apply all specified filters for filter in &self.filters { - entry = ValueCow::Owned( - filter - .evaluate(entry.as_view(), runtime) - .trace("Filter error") - .context_key("filter") - .value_with(|| format!("{}", filter).into()) - .context_key("input") - .value_with(|| format!("{}", entry.source()).into())?, - ); + let value = entry.unwrap_or_else(|_| ValueCow::Owned(Value::Nil)); + entry = filter + .evaluate(value.as_view(), runtime) + .trace("Filter error") + .context_key("filter") + .value_with(|| format!("{}", filter).into()) + .context_key("input") + .value_with(|| format!("{}", value.source()).into()) + .map(ValueCow::Owned); } - Ok(entry) + entry } } From d258842cbe2f9a463e566be24a0eb9a21fb7c739 Mon Sep 17 00:00:00 2001 From: Ryan Bottriell Date: Sat, 8 Apr 2023 14:12:20 -0700 Subject: [PATCH 4/4] Update error type with a kind that can be matched Signed-off-by: Ryan Bottriell --- crates/core/src/error/error.rs | 53 ++++++++++++++++++++++++-- crates/core/src/lib.rs | 2 +- crates/core/src/model/find.rs | 2 +- crates/core/src/parser/filter_chain.rs | 23 +++++++---- crates/core/src/runtime/runtime.rs | 4 +- crates/core/src/runtime/stack.rs | 12 ++---- 6 files changed, 71 insertions(+), 25 deletions(-) diff --git a/crates/core/src/error/error.rs b/crates/core/src/error/error.rs index c376d5696..417e20e2d 100644 --- a/crates/core/src/error/error.rs +++ b/crates/core/src/error/error.rs @@ -16,26 +16,50 @@ pub struct Error { inner: Box, } +impl Error { + /// Identifies the underlying kind for this error + pub fn kind(&self) -> &ErrorKind { + &self.inner.kind + } +} + // Guts of `Error` here to keep `Error`'s memory size small to avoid bloating the size of // `Result` in the success case and spilling over from register-based returns to stack-based // returns. There are already enough memory allocations below, one more // shouldn't hurt. #[derive(Debug, Clone)] struct InnerError { - msg: crate::model::KString, + kind: ErrorKind, user_backtrace: Vec, cause: Option, } impl Error { - /// Create a new compiler error with the given message + /// Create an error that identifies the provided variable as unknown + pub fn unknown_variable>(name: S) -> Self { + Self::with_kind(ErrorKind::UnknownVariable).context("requested variable", name) + } + + /// Create a new error of the given kind + pub fn with_kind(kind: ErrorKind) -> Self { + let error = InnerError { + kind, + user_backtrace: vec![Trace::empty()], + cause: None, + }; + Self { + inner: Box::new(error), + } + } + + /// Create a new custom error with the given message pub fn with_msg>(msg: S) -> Self { Self::with_msg_cow(msg.into()) } fn with_msg_cow(msg: crate::model::KString) -> Self { let error = InnerError { - msg, + kind: ErrorKind::Custom(msg), user_backtrace: vec![Trace::empty()], cause: None, }; @@ -108,7 +132,7 @@ const ERROR_DESCRIPTION: &str = "liquid"; impl fmt::Display for Error { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - writeln!(f, "{}: {}", ERROR_DESCRIPTION, self.inner.msg)?; + writeln!(f, "{}: {}", ERROR_DESCRIPTION, self.inner.kind)?; for trace in &self.inner.user_backtrace { if let Some(trace) = trace.get_trace() { writeln!(f, "from: {}", trace)?; @@ -129,3 +153,24 @@ impl error::Error for Error { self.inner.cause.as_ref().and_then(|e| e.source()) } } + +/// The type of an error. +#[derive(Debug, Clone)] +pub enum ErrorKind { + /// A variable was being indexed but the desired index did not exist + UnknownIndex, + /// A referenced variable did not exist + UnknownVariable, + /// A custom error with no discernible kind + Custom(crate::model::KString), +} + +impl std::fmt::Display for ErrorKind { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + ErrorKind::UnknownIndex => f.write_str("Unknown index"), + ErrorKind::UnknownVariable => f.write_str("Unknown variable"), + ErrorKind::Custom(s) => s.fmt(f), + } + } +} diff --git a/crates/core/src/lib.rs b/crates/core/src/lib.rs index 1d4f66d4e..de9b7b9e1 100644 --- a/crates/core/src/lib.rs +++ b/crates/core/src/lib.rs @@ -13,7 +13,7 @@ pub mod parser; pub mod partials; pub mod runtime; -pub use error::{Error, Result}; +pub use error::{Error, ErrorKind, Result}; #[cfg(feature = "derive")] #[doc(hidden)] pub use liquid_derive::{ diff --git a/crates/core/src/model/find.rs b/crates/core/src/model/find.rs index fb8051c29..27fbd3c17 100644 --- a/crates/core/src/model/find.rs +++ b/crates/core/src/model/find.rs @@ -219,7 +219,7 @@ pub fn find<'o>(value: &'o dyn ValueView, path: &[ScalarCow<'_>]) -> Result(&'s self, runtime: &'s dyn Runtime) -> Result> { // take either the provided value or the value from the provided variable - let mut entry = self.entry.evaluate(runtime); + let mut entry = match self.entry.evaluate(runtime) { + Ok(v) => v, + Err(err) if self.filters.is_empty() => return Err(err), + Err(err) => match err.kind() { + // a missing value is not an error if there are filters + // that come next to process it. They will decide if this + // is appropriate or not (eg: the default filter) + ErrorKind::UnknownIndex | ErrorKind::UnknownVariable => ValueCow::Owned(Value::Nil), + _ => return Err(err), + }, + }; // apply all specified filters for filter in &self.filters { - let value = entry.unwrap_or_else(|_| ValueCow::Owned(Value::Nil)); entry = filter - .evaluate(value.as_view(), runtime) + .evaluate(entry.as_view(), runtime) .trace("Filter error") .context_key("filter") .value_with(|| format!("{}", filter).into()) .context_key("input") - .value_with(|| format!("{}", value.source()).into()) - .map(ValueCow::Owned); + .value_with(|| format!("{}", entry.source()).into()) + .map(ValueCow::Owned)?; } - entry + Ok(entry) } } diff --git a/crates/core/src/runtime/runtime.rs b/crates/core/src/runtime/runtime.rs index 15fb5694e..feaa0d993 100644 --- a/crates/core/src/runtime/runtime.rs +++ b/crates/core/src/runtime/runtime.rs @@ -244,9 +244,7 @@ impl<'g> Runtime for RuntimeCore<'g> { fn get(&self, path: &[ScalarCow<'_>]) -> Result> { let key = path.first().cloned().unwrap_or_else(|| Scalar::new("nil")); - Error::with_msg("Unknown variable") - .context("requested variable", key.to_kstr()) - .into_err() + Error::unknown_variable(key.to_kstr()).into_err() } fn set_global( diff --git a/crates/core/src/runtime/stack.rs b/crates/core/src/runtime/stack.rs index e04601159..9bb3a3761 100644 --- a/crates/core/src/runtime/stack.rs +++ b/crates/core/src/runtime/stack.rs @@ -56,9 +56,7 @@ impl super::Runtime for StackFrame { } fn get(&self, path: &[ScalarCow<'_>]) -> Result> { - let key = path.first().ok_or_else(|| { - Error::with_msg("Unknown variable").context("requested variable", "nil") - })?; + let key = path.first().ok_or_else(|| Error::unknown_variable("nil"))?; let key = key.to_kstr(); let data = &self.data; if data.contains_key(key.as_str()) { @@ -130,9 +128,7 @@ impl super::Runtime for GlobalFrame

{ } fn get(&self, path: &[ScalarCow<'_>]) -> Result> { - let key = path.first().ok_or_else(|| { - Error::with_msg("Unknown variable").context("requested variable", "nil") - })?; + let key = path.first().ok_or_else(|| Error::unknown_variable("nil"))?; let key = key.to_kstr(); let data = self.data.borrow(); if data.contains_key(key.as_str()) { @@ -205,9 +201,7 @@ impl super::Runtime for IndexFrame

{ } fn get(&self, path: &[ScalarCow<'_>]) -> Result> { - let key = path.first().ok_or_else(|| { - Error::with_msg("Unknown variable").context("requested variable", "nil") - })?; + let key = path.first().ok_or_else(|| Error::unknown_variable("nil"))?; let key = key.to_kstr(); let data = self.data.borrow(); if data.contains_key(key.as_str()) {