diff --git a/CHANGELOG.md b/CHANGELOG.md index fa71603c..26368142 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/minijinja/src/value/merge_object.rs b/minijinja/src/value/merge_object.rs index db57ea80..058ea320 100644 --- a/minijinja/src/value/merge_object.rs +++ b/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())) } } diff --git a/minijinja/src/value/mod.rs b/minijinja/src/value/mod.rs index 8fc43acf..2b2b32b0 100644 --- a/minijinja/src/value/mod.rs +++ b/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, diff --git a/minijinja/tests/test_value.rs b/minijinja/tests/test_value.rs index e97d0166..eb1ec235 100644 --- a/minijinja/tests/test_value.rs +++ b/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![