Repository: mitsuhiko/minijinja (language: rust)

Issue:
`Object::len()` and `Object::is_empty()` conflict with inherent `len()`, `is_empty()` 
Calling `self.len()` inside of any method implementation (in particular, `enumerate()`) calls the trait method, which is unlikely to be what's desired. The only way to prevent this is to rename `len()` and `is_empty()` to something else.

I would suggest `length()` or `enumerator_len()` instead of `len()`. For `is_empty()`, maybe `has_no_elements()` or `is_void()` or `is_vacant()`?


Discussion / hints:

Fix patch:
diff --git a/minijinja/src/functions.rs b/minijinja/src/functions.rs
index 19416af2..2bbbfd9e 100644
--- a/minijinja/src/functions.rs
+++ b/minijinja/src/functions.rs
@@ -342,7 +342,7 @@ mod builtins {
             },
         };
 
-        if !update_with.values.is_empty() {
+        if update_with.values.is_true() {
             rv.extend(
                 update_with
                     .values
diff --git a/minijinja/src/value/mod.rs b/minijinja/src/value/mod.rs
index d4255a14..7623f142 100644
--- a/minijinja/src/value/mod.rs
+++ b/minijinja/src/value/mod.rs
@@ -783,7 +783,11 @@ impl Value {
         Kwargs::extract(self).is_some()
     }
 
-    /// Is this value true?
+    /// Is this value considered true?
+    ///
+    /// The engine inherits the same behavior as Jinja2 when it comes to
+    /// considering objects true.  Empty objects are generally not considered
+    /// true.  For custom objects this is customized by [`Object::is_true`].
     pub fn is_true(&self) -> bool {
         match self.0 {
             ValueRepr::Bool(val) => val,
@@ -795,7 +799,7 @@ impl Value {
             ValueRepr::String(ref x, _) => !x.is_empty(),
             ValueRepr::Bytes(ref x) => !x.is_empty(),
             ValueRepr::None | ValueRepr::Undefined | ValueRepr::Invalid(_) => false,
-            ValueRepr::Object(ref x) => !x.is_empty(),
+            ValueRepr::Object(ref x) => x.is_true(),
         }
     }
 
@@ -869,7 +873,7 @@ impl Value {
     pub fn len(&self) -> Option<usize> {
         match self.0 {
             ValueRepr::String(ref s, _) => Some(s.chars().count()),
-            ValueRepr::Object(ref dy) => dy.len(),
+            ValueRepr::Object(ref dy) => dy.enumerator_len(),
             _ => None,
         }
     }
@@ -1113,7 +1117,7 @@ impl Value {
             ValueRepr::Object(ref dy) => match dy.repr() {
                 ObjectRepr::Map | ObjectRepr::Plain | ObjectRepr::Iterable => dy.get_value(key),
                 ObjectRepr::Seq => {
-                    let idx = index(key, || dy.len()).map(Value::from);
+                    let idx = index(key, || dy.enumerator_len()).map(Value::from);
                     dy.get_value(idx.as_ref().unwrap_or(key))
                 }
             },
@@ -1266,7 +1270,7 @@ impl Serialize for Value {
                 ObjectRepr::Plain => serializer.serialize_str(&o.to_string()),
                 ObjectRepr::Seq | ObjectRepr::Iterable => {
                     use serde::ser::SerializeSeq;
-                    let mut seq = ok!(serializer.serialize_seq(o.len()));
+                    let mut seq = ok!(serializer.serialize_seq(o.enumerator_len()));
                     if let Some(iter) = o.try_iter() {
                         for item in iter {
                             ok!(seq.serialize_element(&item));
diff --git a/minijinja/src/value/object.rs b/minijinja/src/value/object.rs
index 90904ddd..152ed2e8 100644
--- a/minijinja/src/value/object.rs
+++ b/minijinja/src/value/object.rs
@@ -187,23 +187,23 @@ pub trait Object: fmt::Debug + Send + Sync {
         }
     }
 
-    /// Returns the length of the object.
+    /// Returns the length of the enumerator.
     ///
     /// By default the length is taken by calling [`enumerate`](Self::enumerate) and
     /// inspecting the [`Enumerator`].  This means that in order to determine
     /// the length, an iteration is started.  If you this is a problem for your
     /// uses, you can manually implement this.  This might for instance be
     /// needed if your type can only be iterated over once.
-    fn len(self: &Arc<Self>) -> Option<usize> {
+    fn enumerator_len(self: &Arc<Self>) -> Option<usize> {
         self.enumerate().query_len()
     }
 
-    /// Returns `true` if this object is considered empty.
+    /// Returns `true` if this object is considered true for if conditions.
     ///
-    /// The default implementation checks if the [`len`](Self::len) of the
-    /// object is `Some(0)` which is the recommended behavior for objects.
-    fn is_empty(self: &Arc<Self>) -> bool {
-        self.len() == Some(0)
+    /// The default implementation checks if the [`enumerator_len`](Self::enumerator_len)
+    /// is not `Some(0)` which is the recommended behavior for objects.
+    fn is_true(self: &Arc<Self>) -> bool {
+        self.enumerator_len() != Some(0)
     }
 
     /// The engine calls this to invoke the object itself.
@@ -269,7 +269,7 @@ pub trait Object: fmt::Debug + Send + Sync {
             // for either sequences or iterables, a length is needed, otherwise we
             // don't want to risk iteration during printing and fall back to the
             // debug print.
-            ObjectRepr::Seq | ObjectRepr::Iterable if self.len().is_some() => {
+            ObjectRepr::Seq | ObjectRepr::Iterable if self.enumerator_len().is_some() => {
                 let mut dbg = f.debug_list();
                 for value in self.try_iter().into_iter().flatten() {
                     dbg.entry(&Dbg(&value));
@@ -613,9 +613,9 @@ type_erase! {
 
         fn enumerate(&self) -> Enumerator;
 
-        fn is_empty(&self) -> bool;
+        fn is_true(&self) -> bool;
 
-        fn len(&self) -> Option<usize>;
+        fn enumerator_len(&self) -> Option<usize>;
 
         fn call(
             &self,
@@ -689,7 +689,7 @@ impl<T: Into<Value> + Clone + Send + Sync + fmt::Debug> Object for Vec<T> {
     }
 
     fn enumerate(self: &Arc<Self>) -> Enumerator {
-        Enumerator::Seq(Vec::len(self))
+        Enumerator::Seq(self.len())
     }
 }
 
diff --git a/minijinja/src/value/ops.rs b/minijinja/src/value/ops.rs
index 1eefb6d5..efe08f04 100644
--- a/minijinja/src/value/ops.rs
+++ b/minijinja/src/value/ops.rs
@@ -115,7 +115,7 @@ pub fn slice(value: Value, start: Value, stop: Value, step: Value) -> Result<Val
         ValueRepr::Undefined | ValueRepr::None => Ok(Value::from(Vec::<Value>::new())),
         ValueRepr::Object(obj) if matches!(obj.repr(), ObjectRepr::Seq | ObjectRepr::Iterable) => {
             Ok(Value::make_object_iterable(obj, move |obj| {
-                let len = obj.len().unwrap_or_default();
+                let len = obj.enumerator_len().unwrap_or_default();
                 let (start, len) = get_offset_and_len(start, stop, || len);
                 // The manual match here is important that we do not mess up the size_hint
                 if let Some(iter) = obj.try_iter() {


Test patch:
diff --git a/minijinja/tests/test_value.rs b/minijinja/tests/test_value.rs
index 9ea2abe0..c4b4a908 100644
--- a/minijinja/tests/test_value.rs
+++ b/minijinja/tests/test_value.rs
@@ -261,7 +261,7 @@ fn test_builtin_seq_objects() {
 fn test_value_object_interface() {
     let val = Value::from_object(vec![1u32, 2, 3, 4]);
     let obj = val.as_object().unwrap();
-    assert_eq!(obj.len(), Some(4));
+    assert_eq!(obj.enumerator_len(), Some(4));
     assert_eq!(obj.to_string(), "[1, 2, 3, 4]");
 }