Repository: mitsuhiko/minijinja (language: rust)

Issue:
Fallible Iteration
Now that iterators are a thing, some operations would be nice to make lazy. However unfortunately some of those operations can fail but the iteration interface really only makes infallible iteration possible.  An example of this are filters such as `|select`.  Today these can fail, but that's not an issue since they buffer into a vector.


Discussion / hints:

Fix patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 18324f0e..1dad5d6e 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -53,6 +53,8 @@ For upgrade instructions read the [UPDATING](UPDATING.md) guide.
 - The CLI now enables unicode support by default.
 - `Value::from_serializable` is now `Value::from_serialize`.
 - Ranges are now iterables and no longer sequences. #493
+- `Value::from` is now implemented for `Error` as public API to create invalid values.
+  Previously this behavior was hidden internally in the serde support. #495
 
 ## 1.0.20
 
diff --git a/minijinja/src/error.rs b/minijinja/src/error.rs
index 0394cc53..fc185068 100644
--- a/minijinja/src/error.rs
+++ b/minijinja/src/error.rs
@@ -1,5 +1,6 @@
 use std::borrow::Cow;
 use std::fmt;
+use std::sync::Arc;
 
 use crate::compiler::tokens::Span;
 
@@ -41,15 +42,16 @@ pub struct Error {
 }
 
 /// The internal error data
+#[derive(Clone)]
 struct ErrorRepr {
     kind: ErrorKind,
     detail: Option<Cow<'static, str>>,
     name: Option<String>,
     lineno: usize,
     span: Option<Span>,
-    source: Option<Box<dyn std::error::Error + Send + Sync>>,
+    source: Option<Arc<dyn std::error::Error + Send + Sync>>,
     #[cfg(feature = "debug")]
-    debug_info: Option<crate::debug::DebugInfo>,
+    debug_info: Option<Arc<crate::debug::DebugInfo>>,
 }
 
 impl fmt::Debug for Error {
@@ -86,7 +88,7 @@ impl fmt::Debug for Error {
 }
 
 /// An enum describing the error kind.
-#[derive(Debug, Copy, Clone, PartialEq, Eq)]
+#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)]
 #[non_exhaustive]
 pub enum ErrorKind {
     /// A non primitive value was encountered where one was expected.
@@ -215,6 +217,12 @@ impl Error {
         }
     }
 
+    pub(crate) fn internal_clone(&self) -> Error {
+        Error {
+            repr: self.repr.clone(),
+        }
+    }
+
     pub(crate) fn set_filename_and_line(&mut self, filename: &str, lineno: usize) {
         self.repr.name = Some(filename.into());
         self.repr.lineno = lineno;
@@ -235,7 +243,7 @@ impl Error {
 
     /// Attaches another error as source to this error.
     pub fn with_source<E: std::error::Error + Send + Sync + 'static>(mut self, source: E) -> Self {
-        self.repr.source = Some(Box::new(source));
+        self.repr.source = Some(Arc::new(source));
         self
     }
 
@@ -350,13 +358,13 @@ impl Error {
     /// ([`Environment::set_debug`](crate::Environment::set_debug)).
     #[cfg(feature = "debug")]
     pub(crate) fn debug_info(&self) -> Option<&crate::debug::DebugInfo> {
-        self.repr.debug_info.as_ref()
+        self.repr.debug_info.as_deref()
     }
 
     #[cfg(feature = "debug")]
     #[cfg_attr(docsrs, doc(cfg(feature = "debug")))]
     pub(crate) fn attach_debug_info(&mut self, value: crate::debug::DebugInfo) {
-        self.repr.debug_info = Some(value);
+        self.repr.debug_info = Some(Arc::new(value));
     }
 }
 
@@ -395,10 +403,10 @@ pub fn attach_basic_debug_info<T>(rv: Result<T, Error>, source: &str) -> Result<
         match rv {
             Ok(rv) => Ok(rv),
             Err(mut err) => {
-                err.repr.debug_info = Some(crate::debug::DebugInfo {
+                err.repr.debug_info = Some(Arc::new(crate::debug::DebugInfo {
                     template_source: Some(source.to_string()),
                     ..Default::default()
-                });
+                }));
                 Err(err)
             }
         }
diff --git a/minijinja/src/filters.rs b/minijinja/src/filters.rs
index 8985ff06..839ee9d7 100644
--- a/minijinja/src/filters.rs
+++ b/minijinja/src/filters.rs
@@ -633,10 +633,7 @@ mod builtins {
                 ErrorKind::InvalidOperation,
                 format!("cannot convert {} to integer", value.kind()),
             )),
-            ValueRepr::Invalid(ref x) => Err(Error::new(
-                ErrorKind::InvalidOperation,
-                format!("invalid value: {}", x),
-            )),
+            ValueRepr::Invalid(_) => value.validate(),
         }
     }
 
@@ -656,10 +653,7 @@ mod builtins {
                 .parse::<f64>()
                 .map(Value::from)
                 .map_err(|err| Error::new(ErrorKind::InvalidOperation, err.to_string())),
-            ValueRepr::Invalid(ref x) => Err(Error::new(
-                ErrorKind::InvalidOperation,
-                format!("invalid value: {}", x),
-            )),
+            ValueRepr::Invalid(_) => value.validate(),
             _ => as_f64(&value).map(Value::from).ok_or_else(|| {
                 Error::new(
                     ErrorKind::InvalidOperation,
diff --git a/minijinja/src/lib.rs b/minijinja/src/lib.rs
index df2ae095..0968a913 100644
--- a/minijinja/src/lib.rs
+++ b/minijinja/src/lib.rs
@@ -158,7 +158,7 @@
 //!   - `deserialization`: when removed this disables deserialization support for
 //!     the [`Value`] type, removes the `ViaDeserialize` type and the error type
 //!     no longer implements `serde::de::Error`.
-//!   - `std_collections`: if this feature is removed some [`Object`](create::value::Object)
+//!   - `std_collections`: if this feature is removed some [`Object`](crate::value::Object)
 //!     implementations for standard library collections are removed.  Only the
 //!     ones needed for the engine to function itself are retained.
 //!
diff --git a/minijinja/src/value/mod.rs b/minijinja/src/value/mod.rs
index 7a76ce6b..91e980a8 100644
--- a/minijinja/src/value/mod.rs
+++ b/minijinja/src/value/mod.rs
@@ -152,6 +152,32 @@ let vec = Vec::<i32>::deserialize(value).unwrap();
 //! let value = Value::from_object(Foo);
 //! let value = Value::from_dyn_object(Arc::new(Foo));
 //! ```
+//!
+//! # Invalid Values
+//!
+//! MiniJinja knows the concept of an "invalid value".  These are rare in practice
+//! and should not be used, but they are needed in some situations.  An invalid value
+//! looks like a value but working with that value in the context of the engine will
+//! fail in most situations.  In principle an invalid value is a value that holds an
+//! error internally.  It's created with [`From`]:
+//!
+//! ```
+//! use minijinja::{Value, Error, ErrorKind};
+//! let error = Error::new(ErrorKind::InvalidOperation, "failed to generate an item");
+//! let invalid_value = Value::from(error);
+//! ```
+//!
+//! Invalid values are typically encountered in the following situations:
+//!
+//! - serialization fails with an error: this is the case when a value is crated
+//!   via [`Value::from_serialize`] and the underlying [`Serialize`] implementation
+//!   fails with an error.
+//! - fallible iteration: there might be situations where an iterator cannot indicate
+//!   failure ahead of iteration and must abort.  In that case the only option an
+//!   iterator in MiniJinja has is to create an invalid value.
+//!
+//! It's generally recommende to ignore the existence of invalid objects and let them
+//! fail naturally as they are encountered.
 
 // this module is based on the content module in insta which in turn is based
 // on the content module in serde::private::ser.
@@ -296,7 +322,12 @@ pub enum ValueKind {
     Iterable,
     /// A plain object without specific behavior.
     Plain,
-    /// This value is invalid.  This can happen when a serialization error occurred.
+    /// This value is invalid (holds an error).
+    ///
+    /// This can happen when a serialization error occurred or the engine
+    /// encountered a failure in a place where an error can otherwise not
+    /// be produced.  Interacting with such values in the context of the
+    /// template evaluation process will attempt to propagate the error.
     Invalid,
 }
 
@@ -384,7 +415,7 @@ pub(crate) enum ValueRepr {
     I64(i64),
     F64(f64),
     None,
-    Invalid(Arc<str>),
+    Invalid(Arc<Error>),
     U128(Packed<u128>),
     I128(Packed<i128>),
     String(Arc<str>, StringType),
@@ -420,7 +451,7 @@ impl Hash for Value {
             ValueRepr::String(ref s, _) => s.hash(state),
             ValueRepr::SmallStr(s) => s.as_str().hash(state),
             ValueRepr::Bool(b) => b.hash(state),
-            ValueRepr::Invalid(s) => s.hash(state),
+            ValueRepr::Invalid(ref e) => (e.kind(), e.detail()).hash(state),
             ValueRepr::Bytes(b) => b.hash(state),
             ValueRepr::Object(d) => d.hash(state),
             ValueRepr::U64(_)
@@ -627,6 +658,22 @@ impl Value {
         transform(value)
     }
 
+    /// Extracts a contained error.
+    ///
+    /// An invalid value carres an error internally and will reveal that error
+    /// at a later point when iteracted with.  This is used to carry
+    /// serialization errors or failures that happen when the engine otherwise
+    /// assumes an infallible operation such as iteration.
+    pub(crate) fn validate(self) -> Result<Value, Error> {
+        if let ValueRepr::Invalid(err) = self.0 {
+            // Today the API implies tghat errors are `Clone`, but we don't want to expose
+            // this as a functionality (yet?).
+            Err(Arc::try_unwrap(err).unwrap_or_else(|arc| (*arc).internal_clone()))
+        } else {
+            Ok(self)
+        }
+    }
+
     /// Creates a value from a safe string.
     ///
     /// A safe string is one that will bypass auto escaping.  For instance if you
@@ -1169,11 +1216,13 @@ impl Value {
                     Some(Value::make_iterable(move || {
                         if let Some(iter) = iter.lock().unwrap().take() {
                             Box::new(iter) as Box<dyn Iterator<Item = Value> + Send + Sync>
-                        } else if let Ok(iter) = for_restart.reverse().and_then(|x| x.try_iter()) {
-                            Box::new(iter) as Box<dyn Iterator<Item = Value> + Send + Sync>
                         } else {
-                            Box::new(None.into_iter())
-                                as Box<dyn Iterator<Item = Value> + Send + Sync>
+                            match for_restart.reverse().and_then(|x| x.try_iter()) {
+                                Ok(iterable) => Box::new(iterable)
+                                    as Box<dyn Iterator<Item = Value> + Send + Sync>,
+                                Err(err) => Box::new(Some(Value::from(err)).into_iter())
+                                    as Box<dyn Iterator<Item = Value> + Send + Sync>,
+                            }
                         }
                     }))
                 }
@@ -1475,6 +1524,12 @@ enum ValueIterImpl {
     Dyn(Box<dyn Iterator<Item = Value> + Send + Sync>),
 }
 
+impl From<Error> for Value {
+    fn from(value: Error) -> Self {
+        Value(ValueRepr::Invalid(Arc::new(value)))
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
diff --git a/minijinja/src/value/object.rs b/minijinja/src/value/object.rs
index 45c3fe5b..b83227aa 100644
--- a/minijinja/src/value/object.rs
+++ b/minijinja/src/value/object.rs
@@ -111,6 +111,11 @@ use crate::vm::State;
 /// let value = Value::from_object(Range10);
 /// ```
 ///
+/// Iteration is encouraged to fail immediately (object is not iterable) or not at
+/// all.  However this is not always possible, but the iteration interface itself
+/// does not support fallible iteration.  It is however possible to accomplish the
+/// same thing by creating an [invalid value](index.html#invalid-values).
+///
 /// # Map As Context
 ///
 /// Map can also be used as template rendering context.  This has a lot of
diff --git a/minijinja/src/value/serialize.rs b/minijinja/src/value/serialize.rs
index 70039dfd..f29e97e8 100644
--- a/minijinja/src/value/serialize.rs
+++ b/minijinja/src/value/serialize.rs
@@ -3,6 +3,7 @@ use std::fmt;
 
 use serde::{ser, Serialize, Serializer};
 
+use crate::error::{Error, ErrorKind};
 use crate::utils::untrusted_size_hint;
 use crate::value::{
     value_map_with_capacity, Arc, Packed, Value, ValueMap, ValueRepr, VALUE_HANDLES,
@@ -10,7 +11,7 @@ use crate::value::{
 };
 
 #[derive(Debug)]
-pub struct InvalidValue(Arc<str>);
+pub struct InvalidValue(String);
 
 impl std::error::Error for InvalidValue {}
 
@@ -25,7 +26,7 @@ impl serde::ser::Error for InvalidValue {
     where
         T: fmt::Display,
     {
-        InvalidValue(Arc::from(msg.to_string()))
+        InvalidValue(msg.to_string())
     }
 }
 
@@ -36,7 +37,7 @@ impl serde::ser::Error for InvalidValue {
 pub fn transform<T: Serialize>(value: T) -> Value {
     match value.serialize(ValueSerializer) {
         Ok(rv) => rv,
-        Err(invalid) => ValueRepr::Invalid(invalid.0).into(),
+        Err(invalid) => Value::from(Error::new(ErrorKind::BadSerialization, invalid.0)),
     }
 }
 
diff --git a/minijinja/src/vm/context.rs b/minijinja/src/vm/context.rs
index 555bbd5c..1fbfcd84 100644
--- a/minijinja/src/vm/context.rs
+++ b/minijinja/src/vm/context.rs
@@ -5,7 +5,7 @@ use std::sync::Arc;
 
 use crate::environment::Environment;
 use crate::error::{Error, ErrorKind};
-use crate::value::{Value, ValueIter, ValueRepr};
+use crate::value::{Value, ValueIter};
 use crate::vm::loop_object::Loop;
 
 #[cfg(feature = "macros")]
@@ -58,11 +58,7 @@ impl<'env> Frame<'env> {
 
     /// Creates a new frame with the given context and validates the value is not invalid
     pub fn new_checked(root: Value) -> Result<Frame<'env>, Error> {
-        if let ValueRepr::Invalid(ref err) = root.0 {
-            Err(Error::new(ErrorKind::BadSerialization, err.to_string()))
-        } else {
-            Ok(Frame::new(root))
-        }
+        Ok(Frame::new(ok!(root.validate())))
     }
 }
 
diff --git a/minijinja/src/vm/mod.rs b/minijinja/src/vm/mod.rs
index 89753e2e..24af90f0 100644
--- a/minijinja/src/vm/mod.rs
+++ b/minijinja/src/vm/mod.rs
@@ -11,7 +11,7 @@ use crate::error::{Error, ErrorKind};
 use crate::output::{CaptureMode, Output};
 use crate::utils::{untrusted_size_hint, AutoEscape, UndefinedBehavior};
 use crate::value::namespace_object::Namespace;
-use crate::value::{ops, value_map_with_capacity, value_optimization, Kwargs, Value, ValueRepr};
+use crate::value::{ops, value_map_with_capacity, value_optimization, Kwargs, Value};
 use crate::vm::context::{Frame, LoopState, Stack};
 use crate::vm::loop_object::Loop;
 use crate::vm::state::BlockStack;
@@ -277,10 +277,10 @@ impl<'env> Vm<'env> {
             macro_rules! assert_valid {
                 ($expr:expr) => {{
                     let val = $expr;
-                    if let ValueRepr::Invalid(ref err) = val.0 {
-                        bail!(Error::new(ErrorKind::BadSerialization, err.to_string()));
+                    match val.validate() {
+                        Ok(val) => val,
+                        Err(err) => bail!(err),
                     }
-                    val
                 }};
             }
 


Test patch:
diff --git a/minijinja/tests/test_deserialization.rs b/minijinja/tests/test_deserialization.rs
index 9aa2202b..936a4fe9 100644
--- a/minijinja/tests/test_deserialization.rs
+++ b/minijinja/tests/test_deserialization.rs
@@ -121,8 +121,14 @@ fn test_invalid() {
     }
 
     let v = Value::from_serialize(X);
-    assert_eq!(v.to_string(), "<invalid value: meh>");
+    assert_eq!(
+        v.to_string(),
+        "<invalid value: could not serialize to value: meh>"
+    );
 
     let err = bool::deserialize(v).unwrap_err();
-    assert_eq!(err.to_string(), "cannot deserialize: meh");
+    assert_eq!(
+        err.to_string(),
+        "cannot deserialize: could not serialize to value: meh"
+    );
 }
diff --git a/minijinja/tests/test_templates.rs b/minijinja/tests/test_templates.rs
index f678ae12..8e6fefed 100644
--- a/minijinja/tests/test_templates.rs
+++ b/minijinja/tests/test_templates.rs
@@ -10,8 +10,8 @@ use std::sync::Arc;
 use std::{env, fs};
 
 use insta::assert_snapshot;
-use minijinja::value::{Enumerator, Object, Value};
-use minijinja::{context, render, Environment, Error, State};
+use minijinja::value::{Enumerator, Object, ObjectRepr, Value};
+use minijinja::{context, render, Environment, Error, ErrorKind, State};
 
 use similar_asserts::assert_eq;
 
@@ -370,7 +370,6 @@ fn test_flattening_sub_item_bad_lookup() {
 }
 
 #[test]
-#[should_panic = "can only flatten structs and maps"]
 fn test_flattening_sub_item_bad_attr() {
     let bad = Bad {
         a: 42,
@@ -381,8 +380,14 @@ fn test_flattening_sub_item_bad_attr() {
     let env = Environment::new();
 
     // resolving an invalid value will fail, even in an attribute lookup
-    env.render_str("{% if good.bad %}...{% endif %}", ctx)
-        .unwrap();
+    let err = env
+        .render_str("{% if good.bad %}...{% endif %}", ctx)
+        .unwrap_err();
+    assert_eq!(err.kind(), ErrorKind::BadSerialization);
+    assert_eq!(
+        err.detail(),
+        Some("can only flatten structs and maps (got an enum)")
+    );
 }
 
 #[test]
@@ -399,7 +404,7 @@ fn test_flattening_sub_item_shielded_print() {
     let value = env.render_str("{{ good }}", ctx).unwrap();
     assert_eq!(
         value,
-        r#"{"bad": <invalid value: can only flatten structs and maps (got an enum)>}"#
+        r#"{"bad": <invalid value: could not serialize to value: can only flatten structs and maps (got an enum)>}"#
     );
 }
 
@@ -565,3 +570,38 @@ fn test_functions() {
         @"23"
     );
 }
+
+#[test]
+fn test_invalid_value_iteration() {
+    let env = Environment::new();
+    let mut out = Vec::<u8>::new();
+
+    #[derive(Debug)]
+    struct FailingIteration;
+
+    impl Object for FailingIteration {
+        fn repr(self: &Arc<Self>) -> ObjectRepr {
+            ObjectRepr::Iterable
+        }
+
+        fn enumerate(self: &Arc<Self>) -> Enumerator {
+            Enumerator::Iter(Box::new((0..4).map(Value::from).chain(Some(Value::from(
+                Error::new(ErrorKind::InvalidOperation, "failed during iteration"),
+            )))))
+        }
+    }
+
+    let t = env
+        .template_from_str("{% for item in iter %}[{{ item }}]{% endfor %}")
+        .unwrap();
+    let err = t
+        .render_to_write(
+            context! { iter => Value::from_object(FailingIteration) },
+            &mut out,
+        )
+        .unwrap_err();
+    let out = String::from_utf8_lossy(&out);
+    assert_eq!(out, "[0][1][2][3]");
+    assert_eq!(err.kind(), ErrorKind::InvalidOperation);
+    assert_eq!(err.detail(), Some("failed during iteration"));
+}