Repository: mitsuhiko/minijinja
Pull request #485 (2024-04-09 16:30:08)

Better and more intentional Object implementations
Refs #418


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 668a77c..fdf657e 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -53,6 +53,10 @@ 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`.
 
+## 1.0.18
+
+- Fixed an endless loop in `undeclared_variables`.  #486
+
 ## 1.0.17
 
 - Added support for `Option<Into<Value>>` as return value from
diff --git a/minijinja/src/compiler/meta.rs b/minijinja/src/compiler/meta.rs
index 4aa8296..edd6aa0 100644
--- a/minijinja/src/compiler/meta.rs
+++ b/minijinja/src/compiler/meta.rs
@@ -131,6 +131,8 @@ fn tracker_visit_expr<'a>(expr: &ast::Expr<'a>, state: &mut AssignmentTracker<'a
                                 }
                                 state.assign_nested(rv);
                                 return;
+                            } else {
+                                break;
                             }
                         }
                         ast::Expr::GetAttr(expr) => {
diff --git a/minijinja/src/value/argtypes.rs b/minijinja/src/value/argtypes.rs
index 69e62d5..e7821ba 100644
--- a/minijinja/src/value/argtypes.rs
+++ b/minijinja/src/value/argtypes.rs
@@ -1,6 +1,6 @@
 use std::borrow::Cow;
 use std::cell::RefCell;
-use std::collections::{BTreeMap, HashMap, HashSet};
+use std::collections::HashSet;
 use std::ops::{Deref, DerefMut};
 use std::sync::Arc;
 
@@ -330,26 +330,6 @@ impl<K: Into<Value>, V: Into<Value>> FromIterator<(K, V)> for Value {
     }
 }
 
-impl<K: Into<Value>, V: Into<Value>> From<BTreeMap<K, V>> for Value {
-    fn from(val: BTreeMap<K, V>) -> Self {
-        val.into_iter().map(|(k, v)| (k.into(), v.into())).collect()
-    }
-}
-
-impl<K: Into<Value>, V: Into<Value>> From<HashMap<K, V>> for Value {
-    fn from(val: HashMap<K, V>) -> Self {
-        val.into_iter().map(|(k, v)| (k.into(), v.into())).collect()
-    }
-}
-
-impl<T: Into<Value>> From<Vec<T>> for Value {
-    fn from(val: Vec<T>) -> Self {
-        let values = val.into_iter().map(|v| v.into()).collect::<Vec<_>>();
-
-        Value::from_object(values)
-    }
-}
-
 macro_rules! value_from {
     ($src:ty, $dst:ident) => {
         impl From<$src> for Value {
@@ -1040,6 +1020,12 @@ impl From<usize> for Value {
     }
 }
 
+impl From<isize> for Value {
+    fn from(val: isize) -> Self {
+        Value::from(val as i64)
+    }
+}
+
 impl<I: Into<Value>> From<Option<I>> for Value {
     fn from(value: Option<I>) -> Self {
         match value {
diff --git a/minijinja/src/value/mod.rs b/minijinja/src/value/mod.rs
index 694e0c6..9c150bd 100644
--- a/minijinja/src/value/mod.rs
+++ b/minijinja/src/value/mod.rs
@@ -18,9 +18,16 @@
 //! let int_value = Value::from(42);
 //! let none_value = Value::from(());
 //! let true_value = Value::from(true);
+//! let map = Value::from({
+//!     let mut m = std::collections::HashMap::new();
+//!     m.insert("foo", 1);
+//!     m.insert("bar", 2);
+//!     m
+//! });
 //! ```
 //!
-//! Or via the [`FromIterator`] trait which can create sequences or maps:
+//! Or via the [`FromIterator`] trait which can create sequences or maps.  When
+//! given a tuple it creates maps, otherwise it makes a sequence.
 //!
 //! ```
 //! # use minijinja::value::Value;
@@ -54,6 +61,26 @@
 //! have a rust equivalent.  It can be created via the [`UNDEFINED`](Value::UNDEFINED)
 //! constant.
 //!
+//! # Collections
+//!
+//! The standard library's collection types such as
+//! [`HashMap`](std::collections::HashMap), [`Vec`] and various others from the
+//! collections module are implemented are objects.  There is a cavet here which is
+//! that maps can only have string or [`Value`] as key.  The values in the collections
+//! are lazily converted into value when accessed or iterated over.   These types can
+//! be constructed either from [`Value::from`] or [`Value::from_object`].  Because the
+//! types are boxed unchanged, you can also downcast them.
+//!
+//! ```rust
+//! # use minijinja::Value;
+//! let vec = Value::from(vec![1i32, 2, 3, 4]);
+//! let vec_ref = vec.downcast_object_ref::<Vec<i32>>().unwrap();
+//! assert_eq!(vec_ref, &vec![1, 2, 3, 4]);
+//! ```
+//!
+//! **Caveat:** for convenience reasons maps with `&str` keys can be stored.  The keys
+//! however are converted into `Arc<str>`.
+//!
 //! # Serde Conversions
 //!
 //! MiniJinja will usually however create values via an indirection via [`serde`] when
diff --git a/minijinja/src/value/object.rs b/minijinja/src/value/object.rs
index 152ed2e..6105fc7 100644
--- a/minijinja/src/value/object.rs
+++ b/minijinja/src/value/object.rs
@@ -1,11 +1,11 @@
-use std::borrow::Borrow;
-use std::collections::HashMap;
+use std::borrow::Cow;
+use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet, LinkedList, VecDeque};
 use std::fmt;
 use std::hash::Hash;
 use std::sync::Arc;
 
 use crate::error::{Error, ErrorKind};
-use crate::value::{intern, Value, ValueMap, ValueRepr};
+use crate::value::{intern, Value, ValueRepr};
 use crate::vm::State;
 
 /// A trait that represents a dynamic object.
@@ -679,51 +679,181 @@ impl Enumerator {
     }
 }
 
-impl<T: Into<Value> + Clone + Send + Sync + fmt::Debug> Object for Vec<T> {
-    fn repr(self: &Arc<Self>) -> ObjectRepr {
-        ObjectRepr::Seq
-    }
+macro_rules! impl_value_vec {
+    ($vec_type:ident) => {
+        impl<T> Object for $vec_type<T>
+        where
+            T: Into<Value> + Clone + Send + Sync + fmt::Debug + 'static,
+        {
+            fn repr(self: &Arc<Self>) -> ObjectRepr {
+                ObjectRepr::Seq
+            }
 
-    fn get_value(self: &Arc<Self>, key: &Value) -> Option<Value> {
-        self.get(some!(key.as_usize())).cloned().map(|v| v.into())
-    }
+            fn get_value(self: &Arc<Self>, key: &Value) -> Option<Value> {
+                self.get(some!(key.as_usize())).cloned().map(|v| v.into())
+            }
 
-    fn enumerate(self: &Arc<Self>) -> Enumerator {
-        Enumerator::Seq(self.len())
-    }
+            fn enumerate(self: &Arc<Self>) -> Enumerator {
+                Enumerator::Seq(self.len())
+            }
+        }
+
+        impl<T> From<$vec_type<T>> for Value
+        where
+            T: Into<Value> + Clone + Send + Sync + fmt::Debug + 'static,
+        {
+            fn from(val: $vec_type<T>) -> Self {
+                Value::from_object(val)
+            }
+        }
+    };
 }
 
-impl Object for ValueMap {
-    fn get_value(self: &Arc<Self>, key: &Value) -> Option<Value> {
-        self.get(key).cloned()
-    }
+macro_rules! impl_value_iterable {
+    ($iterable_type:ident, $enumerator:ident) => {
+        impl<T> Object for $iterable_type<T>
+        where
+            T: Into<Value> + Clone + Send + Sync + fmt::Debug + 'static,
+        {
+            fn repr(self: &Arc<Self>) -> ObjectRepr {
+                ObjectRepr::Iterable
+            }
 
-    fn enumerate(self: &Arc<Self>) -> Enumerator {
-        self.mapped_rev_enumerator(|this| Box::new(this.keys().cloned()))
-    }
+            fn enumerate(self: &Arc<Self>) -> Enumerator {
+                self.clone()
+                    .$enumerator(|this| Box::new(this.iter().map(|x| x.clone().into())))
+            }
+        }
+
+        impl<T> From<$iterable_type<T>> for Value
+        where
+            T: Into<Value> + Clone + Send + Sync + fmt::Debug + 'static,
+        {
+            fn from(val: $iterable_type<T>) -> Self {
+                Value::from_object(val)
+            }
+        }
+    };
 }
 
-impl<K, V> Object for HashMap<K, V>
-where
-    K: Borrow<str>
-        + AsRef<str>
-        + PartialEq
-        + Eq
-        + Hash
-        + Clone
-        + Send
-        + Sync
-        + fmt::Debug
-        + 'static,
-    V: Into<Value> + Clone + Send + Sync + fmt::Debug + 'static,
-{
-    fn get_value(self: &Arc<Self>, key: &Value) -> Option<Value> {
-        self.get(some!(key.as_str())).cloned().map(|v| v.into())
-    }
+macro_rules! impl_str_map_helper {
+    ($map_type:ident, $key_type:ty, $enumerator:ident) => {
+        impl<V> Object for $map_type<$key_type, V>
+        where
+            V: Into<Value> + Clone + Send + Sync + fmt::Debug + 'static,
+        {
+            fn get_value(self: &Arc<Self>, key: &Value) -> Option<Value> {
+                self.get(some!(key.as_str())).cloned().map(|v| v.into())
+            }
 
-    fn enumerate(self: &Arc<Self>) -> Enumerator {
-        self.mapped_enumerator(|this| {
-            Box::new(this.keys().map(|k| intern(k.as_ref())).map(Value::from))
-        })
-    }
+            fn enumerate(self: &Arc<Self>) -> Enumerator {
+                self.$enumerator(|this| {
+                    Box::new(this.keys().map(|k| intern(k.as_ref())).map(Value::from))
+                })
+            }
+        }
+    };
+}
+
+macro_rules! impl_str_map {
+    ($map_type:ident, $enumerator:ident) => {
+        impl_str_map_helper!($map_type, String, $enumerator);
+        impl_str_map_helper!($map_type, Arc<str>, $enumerator);
+
+        impl<V> From<$map_type<String, V>> for Value
+        where
+            V: Into<Value> + Send + Sync + Clone + fmt::Debug + 'static,
+        {
+            fn from(val: $map_type<String, V>) -> Self {
+                Value::from_object(val)
+            }
+        }
+
+        impl<V> From<$map_type<Arc<str>, V>> for Value
+        where
+            V: Into<Value> + Send + Sync + Clone + fmt::Debug + 'static,
+        {
+            fn from(val: $map_type<Arc<str>, V>) -> Self {
+                Value::from_object(val)
+            }
+        }
+
+        impl<'a, V> From<$map_type<&'a str, V>> for Value
+        where
+            V: Into<Value> + Send + Sync + Clone + fmt::Debug + 'static,
+        {
+            fn from(val: $map_type<&'a str, V>) -> Self {
+                Value::from(
+                    val.into_iter()
+                        .map(|(k, v)| (intern(k), v))
+                        .collect::<$map_type<Arc<str>, V>>(),
+                )
+            }
+        }
+
+        impl<'a, V> From<$map_type<Cow<'a, str>, V>> for Value
+        where
+            V: Into<Value> + Send + Sync + Clone + fmt::Debug + 'static,
+        {
+            fn from(val: $map_type<Cow<'a, str>, V>) -> Self {
+                Value::from(
+                    val.into_iter()
+                        .map(|(k, v)| {
+                            (
+                                match k {
+                                    Cow::Borrowed(s) => intern(s),
+                                    Cow::Owned(s) => Arc::<str>::from(s),
+                                },
+                                v,
+                            )
+                        })
+                        .collect::<$map_type<Arc<str>, V>>(),
+                )
+            }
+        }
+    };
+}
+
+macro_rules! impl_value_map {
+    ($map_type:ident, $enumerator:ident) => {
+        impl<V> Object for $map_type<Value, V>
+        where
+            V: Into<Value> + Clone + Send + Sync + fmt::Debug + 'static,
+        {
+            fn get_value(self: &Arc<Self>, key: &Value) -> Option<Value> {
+                self.get(key).cloned().map(|v| v.into())
+            }
+
+            fn enumerate(self: &Arc<Self>) -> Enumerator {
+                self.$enumerator(|this| Box::new(this.keys().cloned()))
+            }
+        }
+
+        impl<V> From<$map_type<Value, V>> for Value
+        where
+            V: Into<Value> + Send + Sync + Clone + fmt::Debug + 'static,
+        {
+            fn from(val: $map_type<Value, V>) -> Self {
+                Value::from_object(val)
+            }
+        }
+    };
+}
+
+impl_value_vec!(Vec);
+impl_value_vec!(VecDeque);
+impl_value_iterable!(LinkedList, mapped_rev_enumerator);
+impl_value_iterable!(HashSet, mapped_enumerator);
+impl_value_iterable!(BTreeSet, mapped_rev_enumerator);
+impl_str_map!(HashMap, mapped_enumerator);
+impl_str_map!(BTreeMap, mapped_rev_enumerator);
+impl_value_map!(HashMap, mapped_enumerator);
+impl_value_map!(BTreeMap, mapped_rev_enumerator);
+
+#[cfg(feature = "preserve_order")]
+mod preserve_order_impls {
+    use super::*;
+    use indexmap::IndexMap;
+
+    impl_value_map!(IndexMap, mapped_rev_enumerator);
 }
diff --git a/minijinja/src/value/serialize.rs b/minijinja/src/value/serialize.rs
index 1f44cc3..70039df 100644
--- a/minijinja/src/value/serialize.rs
+++ b/minijinja/src/value/serialize.rs
@@ -432,7 +432,7 @@ impl ser::SerializeStructVariant for SerializeStructVariant {
 
     fn end(self) -> Result<Value, InvalidValue> {
         let mut rv = BTreeMap::new();
-        rv.insert(self.variant, Value::from_object(self.map));
+        rv.insert(self.variant.to_string(), Value::from_object(self.map));
         Ok(rv.into())
     }
 }


Test patch:
diff --git a/minijinja/tests/test_templates.rs b/minijinja/tests/test_templates.rs
index 709377f..f678ae1 100644
--- a/minijinja/tests/test_templates.rs
+++ b/minijinja/tests/test_templates.rs
@@ -447,6 +447,31 @@ fn test_undeclared_variables() {
     );
 }
 
+#[test]
+fn test_undeclared_variables_bug() {
+    // see https://github.com/mitsuhiko/minijinja/issues/484
+    let mut env = Environment::new();
+    env.add_template(
+        "demo",
+        r#"
+        {% for item in content.products.items() %}
+          {{ foo.bar }}
+          {{ item.name }}  // this used to cause a loop
+        {% endfor %}
+    "#,
+    )
+    .unwrap();
+    let tmpl = env.get_template("demo").unwrap();
+    let undeclared = tmpl.undeclared_variables(true);
+    assert_eq!(
+        undeclared,
+        ["foo.bar", "content.products.items"]
+            .into_iter()
+            .map(|x| x.to_string())
+            .collect()
+    );
+}
+
 #[test]
 fn test_block_fragments() {
     let mut env = Environment::new();
diff --git a/minijinja/tests/test_value.rs b/minijinja/tests/test_value.rs
index 257a60d..13e2ac1 100644
--- a/minijinja/tests/test_value.rs
+++ b/minijinja/tests/test_value.rs
@@ -1,3 +1,4 @@
+use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet, LinkedList, VecDeque};
 use std::sync::Arc;
 
 use insta::assert_snapshot;
@@ -774,3 +775,184 @@ fn test_reverse() {
         @"invalid operation: cannot reverse values of type number"
     );
 }
+
+#[test]
+fn test_object_vec() {
+    let value = Value::from(vec![1i32, 2, 3, 4]);
+    assert_eq!(
+        value.downcast_object_ref::<Vec<i32>>(),
+        Some(&vec![1, 2, 3, 4])
+    );
+    assert_eq!(
+        value.get_item_by_index(0).ok().and_then(|x| x.as_i64()),
+        Some(1)
+    );
+    let iter = value.try_iter().unwrap();
+    assert_eq!(iter.size_hint(), (4, Some(4)));
+    assert_eq!(
+        iter.map(|x| x.as_i64().unwrap()).collect::<Vec<_>>(),
+        vec![1, 2, 3, 4]
+    );
+    assert_eq!(value.to_string(), "[1, 2, 3, 4]");
+}
+
+#[test]
+fn test_object_vec_deque() {
+    let value = Value::from(VecDeque::from([1i32, 2, 3, 4]));
+    assert_eq!(
+        value.downcast_object_ref::<VecDeque<i32>>(),
+        Some(&VecDeque::from([1, 2, 3, 4]))
+    );
+    assert_eq!(
+        value.get_item_by_index(0).ok().and_then(|x| x.as_i64()),
+        Some(1)
+    );
+    let iter = value.try_iter().unwrap();
+    assert_eq!(iter.size_hint(), (4, Some(4)));
+    assert_eq!(
+        iter.map(|x| x.as_i64().unwrap()).collect::<Vec<_>>(),
+        vec![1, 2, 3, 4]
+    );
+    assert_eq!(value.to_string(), "[1, 2, 3, 4]");
+}
+
+#[test]
+fn test_object_linked_list() {
+    let value = Value::from(LinkedList::from([1i32, 2, 3, 4]));
+    assert_eq!(
+        value.downcast_object_ref::<LinkedList<i32>>(),
+        Some(&LinkedList::from([1, 2, 3, 4]))
+    );
+    let iter = value.try_iter().unwrap();
+    assert_eq!(iter.size_hint(), (4, Some(4)));
+    assert_eq!(
+        iter.map(|x| x.as_i64().unwrap()).collect::<Vec<_>>(),
+        vec![1, 2, 3, 4]
+    );
+    assert_eq!(value.to_string(), "[1, 2, 3, 4]");
+}
+
+#[test]
+fn test_object_hash_set() {
+    let value = Value::from(HashSet::from([1i32, 2, 3, 4]));
+    assert_eq!(
+        value.downcast_object_ref::<HashSet<i32>>(),
+        Some(&HashSet::from([1, 2, 3, 4]))
+    );
+    let iter = value.try_iter().unwrap();
+    let mut items = iter.map(|x| x.as_i64().unwrap()).collect::<Vec<_>>();
+    items.sort();
+    assert_eq!(items, vec![1, 2, 3, 4]);
+}
+
+#[test]
+fn test_object_btree_set() {
+    let value = Value::from(BTreeSet::from([1i32, 2, 3, 4]));
+    assert_eq!(
+        value.downcast_object_ref::<BTreeSet<i32>>(),
+        Some(&BTreeSet::from([1, 2, 3, 4]))
+    );
+    let iter = value.try_iter().unwrap();
+    assert_eq!(iter.size_hint(), (4, Some(4)));
+    assert_eq!(
+        iter.map(|x| x.as_i64().unwrap()).collect::<Vec<_>>(),
+        vec![1, 2, 3, 4]
+    );
+    assert_eq!(value.to_string(), "[1, 2, 3, 4]");
+}
+
+#[test]
+fn test_object_hash_map() {
+    let value = Value::from(HashMap::from_iter([("foo", 1i32), ("bar", 2)]));
+    assert_eq!(
+        value.downcast_object_ref::<HashMap<Arc<str>, i32>>(),
+        Some(&HashMap::from_iter([
+            (Arc::from("foo".to_string()), 1),
+            (Arc::from("bar".to_string()), 2),
+        ]))
+    );
+    let iter = value.try_iter().unwrap();
+    assert_eq!(iter.size_hint(), (2, Some(2)));
+
+    let value = Value::from(HashMap::from_iter([
+        ("foo".to_string(), 1i32),
+        ("bar".to_string(), 2),
+    ]));
+    assert_eq!(value.get_attr("foo").ok().and_then(|x| x.as_i64()), Some(1));
+    assert_eq!(
+        value.downcast_object_ref::<HashMap<String, i32>>(),
+        Some(&HashMap::from_iter([
+            ("foo".to_string(), 1),
+            ("bar".to_string(), 2),
+        ]))
+    );
+
+    let value = Value::from(HashMap::from_iter([("foo", 1i32)]));
+    assert_eq!(
+        value
+            .try_iter()
+            .unwrap()
+            .map(|x| x.as_str().unwrap().to_string())
+            .collect::<Vec<_>>(),
+        vec!["foo"]
+    );
+    assert_eq!(value.to_string(), "{\"foo\": 1}");
+
+    let value = Value::from(HashMap::from_iter([(Value::from(true), 1i32)]));
+    assert_eq!(
+        value
+            .get_item(&Value::from(true))
+            .ok()
+            .and_then(|x| x.as_i64()),
+        Some(1)
+    );
+    assert_eq!(value.to_string(), "{true: 1}");
+}
+
+#[test]
+fn test_object_btree_map() {
+    let value = Value::from(BTreeMap::from_iter([("foo", 1i32), ("bar", 2)]));
+    assert_eq!(
+        value.downcast_object_ref::<BTreeMap<Arc<str>, i32>>(),
+        Some(&BTreeMap::from_iter([
+            (Arc::from("foo".to_string()), 1),
+            (Arc::from("bar".to_string()), 2),
+        ]))
+    );
+    let iter = value.try_iter().unwrap();
+    assert_eq!(iter.size_hint(), (2, Some(2)));
+
+    let value = Value::from(BTreeMap::from_iter([
+        ("foo".to_string(), 1i32),
+        ("bar".to_string(), 2),
+    ]));
+    assert_eq!(value.get_attr("foo").ok().and_then(|x| x.as_i64()), Some(1));
+    assert_eq!(
+        value.downcast_object_ref::<BTreeMap<String, i32>>(),
+        Some(&BTreeMap::from_iter([
+            ("foo".to_string(), 1),
+            ("bar".to_string(), 2),
+        ]))
+    );
+
+    let value = Value::from(BTreeMap::from_iter([("foo", 1i32)]));
+    assert_eq!(
+        value
+            .try_iter()
+            .unwrap()
+            .map(|x| x.as_str().unwrap().to_string())
+            .collect::<Vec<_>>(),
+        vec!["foo"]
+    );
+    assert_eq!(value.to_string(), "{\"foo\": 1}");
+
+    let value = Value::from(BTreeMap::from_iter([(Value::from(true), 1i32)]));
+    assert_eq!(
+        value
+            .get_item(&Value::from(true))
+            .ok()
+            .and_then(|x| x.as_i64()),
+        Some(1)
+    );
+    assert_eq!(value.to_string(), "{true: 1}");
+}