Fix a regression in PartialEq/Eq in 2.3.0 (#584)

CHANGELOG.md

@@ -2,6 +2,11 @@
 
 All notable changes to MiniJinja are documented here.
 
+## 2.3.1
+
+- Fixes a regresion in `PartialEq` / `Eq` in `Value` caused by changes
+  in 2.3.0.  #584
+
 ## 2.3.0
 
 - Fixes some compiler warnings in Rust 1.81.  #575

minijinja/src/value/merge_object.rs

@@ -1,7 +1,7 @@
 use std::collections::BTreeSet;
 use std::sync::Arc;
 
-use crate::value::{Enumerator, Object, ObjectExt, Value};
+use crate::value::{Enumerator, Object, Value};
 
 /// Utility struct used by [`context!`](crate::context) to merge
 /// multiple values.
@@ -17,22 +17,14 @@ impl Object for MergeObject {
     }
 
     fn enumerate(self: &Arc<Self>) -> Enumerator {
-        self.mapped_enumerator(|this| {
-            let mut seen = BTreeSet::new();
-            Box::new(
-                this.0
-                    .iter()
-                    .flat_map(|v| v.try_iter().ok())
-                    .flatten()
-                    .filter_map(move |v| {
-                        if seen.contains(&v) {
-                            None
-                        } else {
-                            seen.insert(v.clone());
-                            Some(v)
-                        }
-                    }),
-            )
-        })
+        // we collect here the whole internal object once on iteration so that
+        // we have an enumerator with a known length.
+        let items = self
+            .0
+            .iter()
+            .flat_map(|v| v.try_iter().ok())
+            .flatten()
+            .collect::<BTreeSet<_>>();
+        Enumerator::Iter(Box::new(items.into_iter()))
     }
 }

minijinja/src/value/mod.rs

@@ -493,12 +493,34 @@ impl PartialEq for Value {
                         }
                         match (a.repr(), b.repr()) {
                             (ObjectRepr::Map, ObjectRepr::Map) => {
-                                if a.enumerator_len() != b.enumerator_len() {
+                                // only if we have known lengths can we compare the enumerators
+                                // ahead of time.  This function has a fallback for when a
+                                // map has an unknown length.  That's generally a bad idea, but
+                                // it makes sense supporting regardless as silent failures are
+                                // not a lot of fun.
+                                let mut need_length_fallback = true;
+                                if let (Some(a_len), Some(b_len)) =
+                                    (a.enumerator_len(), b.enumerator_len())
+                                {
+                                    if a_len != b_len {
+                                        return false;
+                                    }
+                                    need_length_fallback = false;
+                                }
+                                let mut a_count = 0;
+                                if !a.try_iter_pairs().map_or(false, |mut ak| {
+                                    ak.all(|(k, v1)| {
+                                        a_count += 1;
+                                        b.get_value(&k).map_or(false, |v2| v1 == v2)
+                                    })
+                                }) {
                                     return false;
                                 }
-                                a.try_iter_pairs().map_or(false, |mut ak| {
-                                    ak.all(|(k, v1)| b.get_value(&k).map_or(false, |v2| v1 == v2))
-                                })
+                                if !need_length_fallback {
+                                    true
+                                } else {
+                                    a_count == b.try_iter().map_or(0, |x| x.count())
+                                }
                             }
                             (
                                 ObjectRepr::Seq | ObjectRepr::Iterable,

minijinja/tests/test_value.rs

@@ -5,7 +5,7 @@ use insta::{assert_debug_snapshot, assert_snapshot};
 use similar_asserts::assert_eq;
 
 use minijinja::value::{DynObject, Enumerator, Kwargs, Object, ObjectRepr, Rest, Value};
-use minijinja::{args, render, Environment, Error, ErrorKind};
+use minijinja::{args, context, render, Environment, Error, ErrorKind};
 
 #[test]
 fn test_sort() {
@@ -1060,6 +1060,68 @@ fn test_float_eq() {
     assert_ne!(xa, xb);
 }
 
+#[test]
+fn test_eq_regression() {
+    // merged objects used to not have a length.  let's make sure that they have
+    let vars = context! {};
+    let new_vars = context! {..vars.clone()};
+    assert_eq!(vars.len(), Some(0));
+    assert_eq!(new_vars.len(), Some(0));
+    assert_eq!(&vars, &new_vars);
+
+    // we also want to make sure that objects with unknown lengths are properly checked.
+    #[derive(Debug)]
+    struct MadMap;
+
+    impl Object for MadMap {
+        fn get_value(self: &Arc<Self>, key: &Value) -> Option<Value> {
+            match key.as_str()? {
+                "a" => Some(Value::from(1)),
+                "b" => Some(Value::from(2)),
+                _ => None,
+            }
+        }
+
+        fn enumerate(self: &Arc<Self>) -> Enumerator {
+            let mut idx = 0;
+            Enumerator::Iter(Box::new(std::iter::from_fn(move || {
+                let new_idx = {
+                    idx += 1;
+                    idx
+                };
+                match new_idx {
+                    1 => Some(Value::from("a")),
+                    2 => Some(Value::from("b")),
+                    _ => None,
+                }
+            })))
+        }
+    }
+
+    let normal_map = context! {
+        a => 1,
+        b => 2
+    };
+    let mad_map = Value::from_object(MadMap);
+    assert_eq!(mad_map.len(), None);
+    assert_eq!(mad_map, normal_map);
+    assert_eq!(normal_map, mad_map);
+    assert_ne!(
+        mad_map,
+        context! {
+            a => 1,
+            b => 2,
+            c => 3,
+        }
+    );
+    assert_ne!(
+        mad_map,
+        context! {
+            a => 1,
+        }
+    );
+}
+
 #[test]
 fn test_sorting() {
     let mut values = vec![