Repository: mitsuhiko/minijinja (rust)

Issue:
Undefined Behavior Divergence Between &Value and Value
## Description

Currently the way `Value` and `&Value` behave differs with regards to undefined values.  Today `&Value` cannot receive an undefined value in strict mode, but `Value` can.  This is due to these different implementations for `ArgType::from_state_and_value`:

```rust
    #[doc(hidden)]
    fn from_state_and_value(
        state: Option<&'a State>,
        value: Option<&'a Value>,
    ) -> Result<(Self::Output, usize), Error> {
        if value.map_or(false, |x| x.is_undefined())
            && state.map_or(false, |x| {
                matches!(x.undefined_behavior(), UndefinedBehavior::Strict)
            })
        {
            Err(Error::from(ErrorKind::UndefinedError))
        } else {
            Ok((ok!(Self::from_value(value)), 1))
        }
    }

// vs

    fn from_state_and_value(
        _state: Option<&'a State>,
        value: Option<&'a Value>,
    ) -> Result<(Self::Output, usize), Error> {
        Ok((ok!(Self::from_value(value)), 1))
    }
```

This causes problems for instance for the `is_undefined` filter if we want to align this.  The case of "is this value undefined" needs to be supported, but removing that undefined behavior strict case from the conversions would change the behavior of all other filters.  I will need to have some thinking about how to align this.

I ran into this originally after #654 which converted some to the reference style.



Pull request description:
Pass arguments by reference when useful and fix an undefined bug
This is technically a behavior change but the current behavior clearly does not make sense and is in violation with Jinja2.

For the difference in behavior see $656.  After this change, there is no difference between `Value` and `&Value` as parameter other than the borrow difference.

Fixes #656

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index ebab82f..472e813 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -8,6 +8,9 @@ All notable changes to MiniJinja are documented here.
 - Added `truncate` filter to `minijinja-contrib`.  #647
 - Added `wordcount` filter to `minijinja-contrib`.  #649
 - Added `wordwrap` filter to `minijinja-contrib`.  #651
+- Some tests and filters now pass borrowed values for performance reasons
+  and a bug was fixed that caused undefined values in strict undefined
+  mode not to work with tests.  #657
 
 ## 2.5.0
 
diff --git a/minijinja/src/filters.rs b/minijinja/src/filters.rs
index dc58351..37060d5 100644
--- a/minijinja/src/filters.rs
+++ b/minijinja/src/filters.rs
@@ -264,9 +264,9 @@ pub fn safe(v: String) -> Value {
 /// this filter escapes with the format that is native to the format or HTML
 /// otherwise.  This means that if the auto escape setting is set to
 /// `Json` for instance then this filter will serialize to JSON instead.
-pub fn escape(state: &State, v: Value) -> Result<Value, Error> {
+pub fn escape(state: &State, v: &Value) -> Result<Value, Error> {
     if v.is_safe() {
-        return Ok(v);
+        return Ok(v.clone());
     }
 
     // this tries to use the escaping flag of the current scope, then
@@ -284,7 +284,7 @@ pub fn escape(state: &State, v: Value) -> Result<Value, Error> {
         None => String::new(),
     };
     let mut out = Output::with_string(&mut rv);
-    ok!(write_escaped(&mut out, auto_escape, &v));
+    ok!(write_escaped(&mut out, auto_escape, v));
     Ok(Value::from_safe_string(rv))
 }
 
@@ -385,7 +385,7 @@ mod builtins {
     /// <p>Search results: {{ results|length }}
     /// ```
     #[cfg_attr(docsrs, doc(cfg(feature = "builtins")))]
-    pub fn length(v: Value) -> Result<usize, Error> {
+    pub fn length(v: &Value) -> Result<usize, Error> {
         v.len().ok_or_else(|| {
             Error::new(
                 ErrorKind::InvalidOperation,
@@ -420,7 +420,7 @@ mod builtins {
     /// * `by`: set to `"value"` to sort by value. Defaults to `"key"`.
     /// * `reverse`: set to `true` to sort in reverse.
     #[cfg_attr(docsrs, doc(cfg(feature = "builtins")))]
-    pub fn dictsort(v: Value, kwargs: Kwargs) -> Result<Value, Error> {
+    pub fn dictsort(v: &Value, kwargs: Kwargs) -> Result<Value, Error> {
         if v.kind() == ValueKind::Map {
             let mut rv = Vec::with_capacity(v.len().unwrap_or(0));
             let iter = ok!(v.try_iter());
@@ -478,7 +478,7 @@ mod builtins {
     /// </dl>
     /// ```
     #[cfg_attr(docsrs, doc(cfg(feature = "builtins")))]
-    pub fn items(v: Value) -> Result<Value, Error> {
+    pub fn items(v: &Value) -> Result<Value, Error> {
         if v.kind() == ValueKind::Map {
             let mut rv = Vec::with_capacity(v.len().unwrap_or(0));
             let iter = ok!(v.try_iter());
@@ -503,7 +503,7 @@ mod builtins {
     /// {% endfor %}
     /// ```
     #[cfg_attr(docsrs, doc(cfg(feature = "builtins")))]
-    pub fn reverse(v: Value) -> Result<Value, Error> {
+    pub fn reverse(v: &Value) -> Result<Value, Error> {
         v.reverse()
     }
 
@@ -521,7 +521,7 @@ mod builtins {
 
     /// Joins a sequence by a character
     #[cfg_attr(docsrs, doc(cfg(feature = "builtins")))]
-    pub fn join(val: Value, joiner: Option<Cow<'_, str>>) -> Result<String, Error> {
+    pub fn join(val: &Value, joiner: Option<Cow<'_, str>>) -> Result<String, Error> {
         if val.is_undefined() || val.is_none() {
             return Ok(String::new());
         }
@@ -599,11 +599,11 @@ mod builtins {
     /// <p>{{ my_variable|default("my_variable was not defined") }}</p>
     /// ```
     #[cfg_attr(docsrs, doc(cfg(feature = "builtins")))]
-    pub fn default(value: Value, other: Option<Value>) -> Value {
+    pub fn default(value: &Value, other: Option<Value>) -> Value {
         if value.is_undefined() {
             other.unwrap_or_else(|| Value::from(""))
         } else {
-            value
+            value.clone()
         }
     }
 
@@ -640,12 +640,12 @@ mod builtins {
     /// {{ "42"|int == 42 }} -> true
     /// ```
     #[cfg_attr(docsrs, doc(cfg(feature = "builtins")))]
-    pub fn int(value: Value) -> Result<Value, Error> {
+    pub fn int(value: &Value) -> Result<Value, Error> {
         match &value.0 {
             ValueRepr::Undefined | ValueRepr::None => Ok(Value::from(0)),
             ValueRepr::Bool(x) => Ok(Value::from(*x as u64)),
             ValueRepr::U64(_) | ValueRepr::I64(_) | ValueRepr::U128(_) | ValueRepr::I128(_) => {
-                Ok(value)
+                Ok(value.clone())
             }
             ValueRepr::F64(v) => Ok(Value::from(*v as i128)),
             ValueRepr::String(..) | ValueRepr::SmallStr(_) => {
@@ -663,7 +663,7 @@ mod builtins {
                 ErrorKind::InvalidOperation,
                 format!("cannot convert {} to integer", value.kind()),
             )),
-            ValueRepr::Invalid(_) => value.validate(),
+            ValueRepr::Invalid(_) => value.clone().validate(),
         }
     }
 
@@ -673,7 +673,7 @@ mod builtins {
     /// {{ "42.5"|float == 42.5 }} -> true
     /// ```
     #[cfg_attr(docsrs, doc(cfg(feature = "builtins")))]
-    pub fn float(value: Value) -> Result<Value, Error> {
+    pub fn float(value: &Value) -> Result<Value, Error> {
         match &value.0 {
             ValueRepr::Undefined | ValueRepr::None => Ok(Value::from(0.0)),
             ValueRepr::Bool(x) => Ok(Value::from(*x as u64 as f64)),
@@ -683,8 +683,8 @@ mod builtins {
                 .parse::<f64>()
                 .map(Value::from)
                 .map_err(|err| Error::new(ErrorKind::InvalidOperation, err.to_string())),
-            ValueRepr::Invalid(_) => value.validate(),
-            _ => as_f64(&value, true).map(Value::from).ok_or_else(|| {
+            ValueRepr::Invalid(_) => value.clone().validate(),
+            _ => as_f64(value, true).map(Value::from).ok_or_else(|| {
                 Error::new(
                     ErrorKind::InvalidOperation,
                     format!("cannot convert {} to float", value.kind()),
@@ -728,7 +728,7 @@ mod builtins {
     /// {{ value['key'] == value|attr('key') }} -> true
     /// ```
     #[cfg_attr(docsrs, doc(cfg(feature = "builtins")))]
-    pub fn attr(value: Value, key: &Value) -> Result<Value, Error> {
+    pub fn attr(value: &Value, key: &Value) -> Result<Value, Error> {
         value.get_item(key)
     }
 
@@ -769,7 +769,7 @@ mod builtins {
     /// </dl>
     /// ```
     #[cfg_attr(docsrs, doc(cfg(feature = "builtins")))]
-    pub fn first(value: Value) -> Result<Value, Error> {
+    pub fn first(value: &Value) -> Result<Value, Error> {
         if let Some(s) = value.as_str() {
             Ok(s.chars().next().map_or(Value::UNDEFINED, Value::from))
         } else if let Some(mut iter) = value.as_object().and_then(|x| x.try_iter()) {
@@ -895,9 +895,9 @@ mod builtins {
     ///
     /// If the string has been marked as safe, that value is preserved.
     #[cfg_attr(docsrs, doc(cfg(feature = "builtins")))]
-    pub fn string(value: Value) -> Value {
+    pub fn string(value: &Value) -> Value {
         if value.kind() == ValueKind::String {
-            value
+            value.clone()
         } else {
             value.to_string().into()
         }
@@ -912,7 +912,7 @@ mod builtins {
     /// {{ 42|bool }} -> true
     /// ```
     #[cfg_attr(docsrs, doc(cfg(feature = "builtins")))]
-    pub fn bool(value: Value) -> bool {
+    pub fn bool(value: &Value) -> bool {
         value.is_true()
     }
 
@@ -1056,7 +1056,7 @@ mod builtins {
     /// ```
     #[cfg_attr(docsrs, doc(cfg(all(feature = "builtins", feature = "json"))))]
     #[cfg(feature = "json")]
-    pub fn tojson(value: Value, indent: Option<Value>, args: Kwargs) -> Result<Value, Error> {
+    pub fn tojson(value: &Value, indent: Option<Value>, args: Kwargs) -> Result<Value, Error> {
         let indent = match indent {
             Some(indent) => Some(indent),
             None => ok!(args.get("indent")),
@@ -1163,7 +1163,7 @@ mod builtins {
     /// ```
     #[cfg_attr(docsrs, doc(cfg(all(feature = "builtins", feature = "urlencode"))))]
     #[cfg(feature = "urlencode")]
-    pub fn urlencode(value: Value) -> Result<String, Error> {
+    pub fn urlencode(value: &Value) -> Result<String, Error> {
         const SET: &percent_encoding::AsciiSet = &percent_encoding::NON_ALPHANUMERIC
             .remove(b'/')
             .remove(b'.')
@@ -1549,7 +1549,7 @@ mod builtins {
     /// the resulting list will have `["CA", "NY"]`.  This can be disabled by
     /// passing `case_sensitive=True`.
     #[cfg_attr(docsrs, doc(cfg(feature = "builtins")))]
-    pub fn unique(values: Vec<Value>, kwargs: Kwargs) -> Result<Value, Error> {
+    pub fn unique(state: &State, values: Value, kwargs: Kwargs) -> Result<Value, Error> {
         use std::collections::BTreeSet;
 
         let attr = ok!(kwargs.get::<Option<&str>>("attribute"));
@@ -1559,7 +1559,8 @@ mod builtins {
         let mut rv = Vec::new();
         let mut seen = BTreeSet::new();
 
-        for item in values {
+        let iter = ok!(state.undefined_behavior().try_iter(values));
+        for item in iter {
             let value_to_compare = if let Some(attr) = attr {
                 item.get_path_or_default(attr, &Value::UNDEFINED)
             } else {
diff --git a/minijinja/src/value/argtypes.rs b/minijinja/src/value/argtypes.rs
index 9eade61..48cafe7 100644
--- a/minijinja/src/value/argtypes.rs
+++ b/minijinja/src/value/argtypes.rs
@@ -5,7 +5,6 @@ use std::ops::{Deref, DerefMut};
 use std::sync::Arc;
 
 use crate::error::{Error, ErrorKind};
-use crate::utils::UndefinedBehavior;
 use crate::value::{
     DynObject, ObjectRepr, Packed, SmallStr, StringType, Value, ValueKind, ValueMap, ValueRepr,
 };
@@ -158,18 +157,10 @@ pub trait ArgType<'a> {
 
     #[doc(hidden)]
     fn from_state_and_value(
-        state: Option<&'a State>,
+        _state: Option<&'a State>,
         value: Option<&'a Value>,
     ) -> Result<(Self::Output, usize), Error> {
-        if value.map_or(false, |x| x.is_undefined())
-            && state.map_or(false, |x| {
-                matches!(x.undefined_behavior(), UndefinedBehavior::Strict)
-            })
-        {
-            Err(Error::from(ErrorKind::UndefinedError))
-        } else {
-            Ok((ok!(Self::from_value(value)), 1))
-        }
+        Ok((ok!(Self::from_value(value)), 1))
     }
 
     #[doc(hidden)]
@@ -960,13 +951,6 @@ impl TryFrom<Value> for Kwargs {
 impl<'a> ArgType<'a> for Value {
     type Output = Self;
 
-    fn from_state_and_value(
-        _state: Option<&'a State>,
-        value: Option<&'a Value>,
-    ) -> Result<(Self::Output, usize), Error> {
-        Ok((ok!(Self::from_value(value)), 1))
-    }
-
     fn from_value(value: Option<&'a Value>) -> Result<Self, Error> {
         match value {
             Some(value) => Ok(value.clone()),


Test patch:
diff --git a/minijinja/src/tests.rs b/minijinja/src/tests.rs
index 5a37944..55b2c62 100644
--- a/minijinja/src/tests.rs
+++ b/minijinja/src/tests.rs
@@ -202,7 +202,7 @@ impl BoxedTest {
 /// ```jinja
 /// {{ 42 is undefined }} -> false
 /// ```
-pub fn is_undefined(v: Value) -> bool {
+pub fn is_undefined(v: &Value) -> bool {
     v.is_undefined()
 }
 
@@ -211,7 +211,7 @@ pub fn is_undefined(v: Value) -> bool {
 /// ```jinja
 /// {{ 42 is defined }} -> true
 /// ```
-pub fn is_defined(v: Value) -> bool {
+pub fn is_defined(v: &Value) -> bool {
     !v.is_undefined()
 }
 
@@ -220,7 +220,7 @@ pub fn is_defined(v: Value) -> bool {
 /// ```jinja
 /// {{ none is none }} -> true
 /// ```
-pub fn is_none(v: Value) -> bool {
+pub fn is_none(v: &Value) -> bool {
     v.is_none()
 }
 
@@ -231,7 +231,7 @@ pub fn is_none(v: Value) -> bool {
 /// ```
 ///
 /// This filter is also registered with the `escaped` alias.
-pub fn is_safe(v: Value) -> bool {
+pub fn is_safe(v: &Value) -> bool {
     v.is_safe()
 }
 
@@ -250,7 +250,7 @@ mod builtins {
     /// {{ true is boolean }} -> true
     /// ```
     #[cfg_attr(docsrs, doc(cfg(feature = "builtins")))]
-    pub fn is_boolean(v: Value) -> bool {
+    pub fn is_boolean(v: &Value) -> bool {
         v.kind() == ValueKind::Bool
     }
 
@@ -280,8 +280,8 @@ mod builtins {
     /// {{ 42 is divisibleby(2) }} -> true
     /// ```
     #[cfg_attr(docsrs, doc(cfg(feature = "builtins")))]
-    pub fn is_divisibleby(v: Value, other: Value) -> bool {
-        match coerce(&v, &other, false) {
+    pub fn is_divisibleby(v: &Value, other: &Value) -> bool {
+        match coerce(v, other, false) {
             Some(CoerceResult::I128(a, b)) => (a % b) == 0,
             Some(CoerceResult::F64(a, b)) => (a % b) == 0.0,
             _ => false,
@@ -295,7 +295,7 @@ mod builtins {
     /// {{ "42" is number }} -> false
     /// ```
     #[cfg_attr(docsrs, doc(cfg(feature = "builtins")))]
-    pub fn is_number(v: Value) -> bool {
+    pub fn is_number(v: &Value) -> bool {
         matches!(v.kind(), ValueKind::Number)
     }
 
@@ -306,7 +306,7 @@ mod builtins {
     /// {{ 42.0 is integer }} -> false
     /// ```
     #[cfg_attr(docsrs, doc(cfg(feature = "builtins")))]
-    pub fn is_integer(v: Value) -> bool {
+    pub fn is_integer(v: &Value) -> bool {
         v.is_integer()
     }
 
@@ -317,7 +317,7 @@ mod builtins {
     /// {{ 42.0 is float }} -> true
     /// ```
     #[cfg_attr(docsrs, doc(cfg(feature = "builtins")))]
-    pub fn is_float(v: Value) -> bool {
+    pub fn is_float(v: &Value) -> bool {
         matches!(v.0, crate::value::ValueRepr::F64(_))
     }
 
@@ -328,7 +328,7 @@ mod builtins {
     /// {{ 42 is string }} -> false
     /// ```
     #[cfg_attr(docsrs, doc(cfg(feature = "builtins")))]
-    pub fn is_string(v: Value) -> bool {
+    pub fn is_string(v: &Value) -> bool {
         matches!(v.kind(), ValueKind::String)
     }
 
@@ -339,7 +339,7 @@ mod builtins {
     /// {{ 42 is sequence }} -> false
     /// ```
     #[cfg_attr(docsrs, doc(cfg(feature = "builtins")))]
-    pub fn is_sequence(v: Value) -> bool {
+    pub fn is_sequence(v: &Value) -> bool {
         matches!(v.kind(), ValueKind::Seq)
     }
 
@@ -350,7 +350,7 @@ mod builtins {
     /// {{ 42 is iterable }} -> false
     /// ```
     #[cfg_attr(docsrs, doc(cfg(feature = "builtins")))]
-    pub fn is_iterable(v: Value) -> bool {
+    pub fn is_iterable(v: &Value) -> bool {
         v.try_iter().is_ok()
     }
 
@@ -361,7 +361,7 @@ mod builtins {
     /// {{ [1, 2, 3] is mapping }} -> false
     /// ```
     #[cfg_attr(docsrs, doc(cfg(feature = "builtins")))]
-    pub fn is_mapping(v: Value) -> bool {
+    pub fn is_mapping(v: &Value) -> bool {
         matches!(v.kind(), ValueKind::Map)
     }
 
@@ -494,7 +494,7 @@ mod builtins {
     #[cfg_attr(docsrs, doc(cfg(feature = "builtins")))]
     #[cfg(feature = "builtins")]
     pub fn is_in(state: &State, value: &Value, other: &Value) -> Result<bool, Error> {
-        ok!(state.undefined_behavior().assert_iterable(value));
+        ok!(state.undefined_behavior().assert_iterable(other));
         Ok(crate::value::ops::contains(other, value)
             .map(|value| value.is_true())
             .unwrap_or(false))
diff --git a/minijinja/tests/test_environment.rs b/minijinja/tests/test_environment.rs
index ec9d32f..52d26d1 100644
--- a/minijinja/tests/test_environment.rs
+++ b/minijinja/tests/test_environment.rs
@@ -161,7 +161,7 @@ fn test_unknown_method_callback() {
     env.set_unknown_method_callback(|_state, value, method, args| {
         if value.kind() == ValueKind::Map && method == "items" {
             from_args::<()>(args)?;
-            minijinja::filters::items(value.clone())
+            minijinja::filters::items(value)
         } else {
             Err(Error::from(ErrorKind::UnknownMethod))
         }
diff --git a/minijinja/tests/test_undefined.rs b/minijinja/tests/test_undefined.rs
index 0ebca35..c231dfe 100644
--- a/minijinja/tests/test_undefined.rs
+++ b/minijinja/tests/test_undefined.rs
@@ -1,7 +1,7 @@
 #![cfg(feature = "builtins")]
 use std::collections::HashMap;
 
-use minijinja::{context, render, Environment, ErrorKind, State, UndefinedBehavior, Value};
+use minijinja::{context, render, Environment, ErrorKind, State, UndefinedBehavior};
 
 use similar_asserts::assert_eq;
 
@@ -42,12 +42,6 @@ fn test_lenient_undefined() {
 fn test_strict_undefined() {
     let mut env = Environment::new();
     env.set_undefined_behavior(UndefinedBehavior::Strict);
-    env.add_filter("test", |_state: &State, _value: String| -> String {
-        panic!("filter must not be called");
-    });
-    env.add_filter("test2", |_state: &State, _value: Value| -> String {
-        "WAS CALLED".into()
-    });
 
     assert_eq!(
         env.render_str("{{ true.missing_attribute }}", ())
@@ -97,13 +91,6 @@ fn test_strict_undefined() {
             .kind(),
         ErrorKind::InvalidOperation
     );
-    assert_eq!(
-        env.render_str("{{ undefined|test }}", ())
-            .unwrap_err()
-            .kind(),
-        ErrorKind::UndefinedError
-    );
-    assert_eq!(render!(in env, "{{ undefined|test2 }}"), "WAS CALLED");
     assert_eq!(
         env.render_str("{{ 42 in undefined }}", ())
             .unwrap_err()