Repository: mitsuhiko/minijinja (language: 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.




Discussion / hints:

Fix patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index ebab82f4..472e8130 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 dc58351b..37060d51 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 9eade61f..48cafe7e 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 5a379441..55b2c621 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 ec9d32fd..52d26d1c 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 0ebca355..c231dfeb 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()