Repository: mitsuhiko/minijinja
Pull request #101 (2022-09-03 22:48:04)

New Function Traits
This is a change to MiniJinja which changes the traits for filters, functions and tests to take values by reference. This in theory should allow borrowing out of the value (#97) but the lack of GATs in Rust does not actually make this possible yet.

This is a breaking change because some of this functionality is exposed to user code (eg: via `Object`).


Patch:
diff --git a/examples/dynamic/src/main.rs b/examples/dynamic/src/main.rs
index aba2afe..0c1b348 100644
--- a/examples/dynamic/src/main.rs
+++ b/examples/dynamic/src/main.rs
@@ -18,7 +18,7 @@ impl fmt::Display for Cycler {
 }
 
 impl Object for Cycler {
-    fn call(&self, _state: &State, args: Vec<Value>) -> Result<Value, Error> {
+    fn call(&self, _state: &State, args: &[Value]) -> Result<Value, Error> {
         let _: () = FunctionArgs::from_values(args)?;
         let idx = self.idx.fetch_add(1, Ordering::Relaxed);
         Ok(self.values[idx % self.values.len()].clone())
@@ -42,7 +42,7 @@ impl fmt::Display for Magic {
 }
 
 impl Object for Magic {
-    fn call_method(&self, _state: &State, name: &str, args: Vec<Value>) -> Result<Value, Error> {
+    fn call_method(&self, _state: &State, name: &str, args: &[Value]) -> Result<Value, Error> {
         if name == "make_class" {
             let (tag,): (String,) = FunctionArgs::from_values(args)?;
             Ok(Value::from(format!("magic-{}", tag)))
diff --git a/minijinja/src/environment.rs b/minijinja/src/environment.rs
index ae49da8..0d3419f 100644
--- a/minijinja/src/environment.rs
+++ b/minijinja/src/environment.rs
@@ -526,10 +526,10 @@ impl<'source> Environment<'source> {
     /// For details about filters have a look at [`filters`].
     pub fn add_filter<F, V, Rv, Args>(&mut self, name: &'source str, f: F)
     where
-        V: ArgType,
+        V: for<'a> ArgType<'a>,
         Rv: Into<Value>,
         F: filters::Filter<V, Rv, Args>,
-        Args: FunctionArgs,
+        Args: for<'a> FunctionArgs<'a>,
     {
         self.filters.insert(name, filters::BoxedFilter::new(f));
     }
@@ -544,9 +544,9 @@ impl<'source> Environment<'source> {
     /// For details about tests have a look at [`tests`].
     pub fn add_test<F, V, Args>(&mut self, name: &'source str, f: F)
     where
-        V: ArgType,
+        V: for<'a> ArgType<'a>,
         F: tests::Test<V, Args>,
-        Args: FunctionArgs,
+        Args: for<'a> FunctionArgs<'a>,
     {
         self.tests.insert(name, tests::BoxedTest::new(f));
     }
@@ -564,7 +564,7 @@ impl<'source> Environment<'source> {
     where
         Rv: Into<Value>,
         F: functions::Function<Rv, Args>,
-        Args: FunctionArgs,
+        Args: for<'a> FunctionArgs<'a>,
     {
         self.add_global(name, functions::BoxedFunction::new(f).to_value());
     }
diff --git a/minijinja/src/filters.rs b/minijinja/src/filters.rs
index 5cd649a..9d92688 100644
--- a/minijinja/src/filters.rs
+++ b/minijinja/src/filters.rs
@@ -51,7 +51,7 @@ use crate::value::{ArgType, FunctionArgs, RcType, Value};
 use crate::vm::State;
 use crate::AutoEscape;
 
-type FilterFunc = dyn Fn(&State, Value, Vec<Value>) -> Result<Value, Error> + Sync + Send + 'static;
+type FilterFunc = dyn Fn(&State, &Value, &[Value]) -> Result<Value, Error> + Sync + Send + 'static;
 
 #[derive(Clone)]
 pub(crate) struct BoxedFilter(RcType<FilterFunc>);
@@ -89,9 +89,9 @@ impl BoxedFilter {
     pub fn new<F, V, Rv, Args>(f: F) -> BoxedFilter
     where
         F: Filter<V, Rv, Args>,
-        V: ArgType,
+        V: for<'a> ArgType<'a>,
         Rv: Into<Value>,
-        Args: FunctionArgs,
+        Args: for<'a> FunctionArgs<'a>,
     {
         BoxedFilter(RcType::new(
             move |state, value, args| -> Result<Value, Error> {
@@ -106,7 +106,7 @@ impl BoxedFilter {
     }
 
     /// Applies the filter to a value and argument.
-    pub fn apply_to(&self, state: &State, value: Value, args: Vec<Value>) -> Result<Value, Error> {
+    pub fn apply_to(&self, state: &State, value: &Value, args: &[Value]) -> Result<Value, Error> {
         (self.0)(state, value, args)
     }
 }
@@ -759,7 +759,7 @@ mod builtins {
         };
         let bx = BoxedFilter::new(test);
         assert_eq!(
-            bx.apply_to(&state, Value::from(23), vec![Value::from(42)])
+            bx.apply_to(&state, &Value::from(23), &[Value::from(42)][..])
                 .unwrap(),
             Value::from(65)
         );
@@ -785,15 +785,15 @@ mod builtins {
         };
         let bx = BoxedFilter::new(add);
         assert_eq!(
-            bx.apply_to(&state, Value::from(23), vec![Value::from(42)])
+            bx.apply_to(&state, &Value::from(23), &[Value::from(42)][..])
                 .unwrap(),
             Value::from(65)
         );
         assert_eq!(
             bx.apply_to(
                 &state,
-                Value::from(23),
-                vec![Value::from(42), Value::UNDEFINED]
+                &Value::from(23),
+                &[Value::from(42), Value::UNDEFINED][..]
             )
             .unwrap(),
             Value::from(65)
@@ -801,8 +801,8 @@ mod builtins {
         assert_eq!(
             bx.apply_to(
                 &state,
-                Value::from(23),
-                vec![Value::from(42), Value::from(1)]
+                &Value::from(23),
+                &[Value::from(42), Value::from(1)][..]
             )
             .unwrap(),
             Value::from(66)
diff --git a/minijinja/src/functions.rs b/minijinja/src/functions.rs
index d53af7b..df6bf07 100644
--- a/minijinja/src/functions.rs
+++ b/minijinja/src/functions.rs
@@ -46,7 +46,7 @@ use crate::error::Error;
 use crate::value::{FunctionArgs, Object, Value};
 use crate::vm::State;
 
-type FuncFunc = dyn Fn(&State, Vec<Value>) -> Result<Value, Error> + Sync + Send + 'static;
+type FuncFunc = dyn Fn(&State, &[Value]) -> Result<Value, Error> + Sync + Send + 'static;
 
 /// A boxed function.
 #[derive(Clone)]
@@ -86,7 +86,7 @@ impl BoxedFunction {
     where
         F: Function<Rv, Args>,
         Rv: Into<Value>,
-        Args: FunctionArgs,
+        Args: for<'a> FunctionArgs<'a>,
     {
         BoxedFunction(
             Arc::new(move |env, args| -> Result<Value, Error> {
@@ -98,7 +98,7 @@ impl BoxedFunction {
     }
 
     /// Invokes the function.
-    pub fn invoke(&self, state: &State, args: Vec<Value>) -> Result<Value, Error> {
+    pub fn invoke(&self, state: &State, args: &[Value]) -> Result<Value, Error> {
         (self.0)(state, args)
     }
 
@@ -129,7 +129,7 @@ impl fmt::Display for BoxedFunction {
 }
 
 impl Object for BoxedFunction {
-    fn call(&self, state: &State, args: Vec<Value>) -> Result<Value, Error> {
+    fn call(&self, state: &State, args: &[Value]) -> Result<Value, Error> {
         self.invoke(state, args)
     }
 }
diff --git a/minijinja/src/value.rs b/minijinja/src/value.rs
index 345eb7e..f2719de 100644
--- a/minijinja/src/value.rs
+++ b/minijinja/src/value.rs
@@ -166,9 +166,9 @@ fn with_internal_serialization<R, F: FnOnce() -> R>(f: F) -> R {
 /// For each argument the conversion is performed via the [`ArgType`]
 /// trait which is implemented for some primitive concrete types as well
 /// as these types wrapped in [`Option`].
-pub trait FunctionArgs: Sized {
+pub trait FunctionArgs<'a>: Sized {
     /// Converts to function arguments from a slice of values.
-    fn from_values(values: Vec<Value>) -> Result<Self, Error>;
+    fn from_values(values: &'a [Value]) -> Result<Self, Error>;
 }
 
 /// A trait implemented by all filter/test argument types.
@@ -177,15 +177,15 @@ pub trait FunctionArgs: Sized {
 /// `Option<Value>` where `Some` means the argument was provided or
 /// `None` if it was not.  This is used to implement optional arguments
 /// to functions.
-pub trait ArgType: Sized {
+pub trait ArgType<'a>: Sized {
     #[doc(hidden)]
-    fn from_value(value: Option<Value>) -> Result<Self, Error>;
+    fn from_value(value: Option<&'a Value>) -> Result<Self, Error>;
 }
 
 macro_rules! tuple_impls {
     ( $( $name:ident )* ) => {
-        impl<$($name: ArgType,)*> FunctionArgs for ($($name,)*) {
-            fn from_values(values: Vec<Value>) -> Result<Self, Error> {
+        impl<'a, $($name: ArgType<'a>,)*> FunctionArgs<'a> for ($($name,)*) {
+            fn from_values(values: &'a [Value]) -> Result<Self, Error> {
                 #![allow(non_snake_case, unused)]
                 let arg_count = 0 $(
                     + { let $name = (); 1 }
@@ -199,7 +199,7 @@ macro_rules! tuple_impls {
                 {
                     let mut idx = 0;
                     $(
-                        let $name = ArgType::from_value(values.get(idx).cloned())?;
+                        let $name = ArgType::from_value(values.get(idx))?;
                         idx += 1;
                     )*
                     Ok(( $($name,)* ))
@@ -706,23 +706,23 @@ macro_rules! primitive_try_from {
             }
         }
 
-        impl ArgType for $ty {
-            fn from_value(value: Option<Value>) -> Result<Self, Error> {
+        impl<'a> ArgType<'a> for $ty {
+            fn from_value(value: Option<&Value>) -> Result<Self, Error> {
                 match value {
-                    Some(value) => TryFrom::try_from(value),
+                    Some(value) => TryFrom::try_from(value.clone()),
                     None => Err(Error::new(ErrorKind::UndefinedError, concat!("missing argument")))
                 }
             }
         }
 
-        impl ArgType for Option<$ty> {
-            fn from_value(value: Option<Value>) -> Result<Self, Error> {
+        impl<'a> ArgType<'a> for Option<$ty> {
+            fn from_value(value: Option<&Value>) -> Result<Self, Error> {
                 match value {
                     Some(value) => {
                         if value.is_undefined() || value.is_none() {
                             Ok(None)
                         } else {
-                            TryFrom::try_from(value).map(Some)
+                            TryFrom::try_from(value.clone()).map(Some)
                         }
                     }
                     None => Ok(None),
@@ -766,40 +766,60 @@ primitive_try_from!(f64, {
     ValueRepr::F64(val) => val,
 });
 
-macro_rules! infallible_conversion {
-    ($ty:ty) => {
-        impl ArgType for $ty {
-            fn from_value(value: Option<Value>) -> Result<Self, Error> {
-                match value {
-                    Some(value) => Ok(value.into()),
-                    None => Err(Error::new(
-                        ErrorKind::UndefinedError,
-                        concat!("missing argument"),
-                    )),
+impl<'a> ArgType<'a> for Value {
+    fn from_value(value: Option<&'a Value>) -> Result<Self, Error> {
+        match value {
+            Some(value) => Ok(value.clone()),
+            None => Err(Error::new(
+                ErrorKind::UndefinedError,
+                concat!("missing argument"),
+            )),
+        }
+    }
+}
+
+impl<'a> ArgType<'a> for Option<Value> {
+    fn from_value(value: Option<&'a Value>) -> Result<Self, Error> {
+        match value {
+            Some(value) => {
+                if value.is_undefined() || value.is_none() {
+                    Ok(None)
+                } else {
+                    Ok(Some(value.clone()))
                 }
             }
+            None => Ok(None),
         }
+    }
+}
 
-        impl ArgType for Option<$ty> {
-            fn from_value(value: Option<Value>) -> Result<Self, Error> {
-                match value {
-                    Some(value) => {
-                        if value.is_undefined() || value.is_none() {
-                            Ok(None)
-                        } else {
-                            Ok(Some(value.clone().into()))
-                        }
-                    }
-                    None => Ok(None),
+impl<'a> ArgType<'a> for String {
+    fn from_value(value: Option<&'a Value>) -> Result<Self, Error> {
+        match value {
+            Some(value) => Ok(value.to_string()),
+            None => Err(Error::new(
+                ErrorKind::UndefinedError,
+                concat!("missing argument"),
+            )),
+        }
+    }
+}
+
+impl<'a> ArgType<'a> for Option<String> {
+    fn from_value(value: Option<&'a Value>) -> Result<Self, Error> {
+        match value {
+            Some(value) => {
+                if value.is_undefined() || value.is_none() {
+                    Ok(None)
+                } else {
+                    Ok(Some(value.to_string()))
                 }
             }
+            None => Ok(None),
         }
-    };
+    }
 }
 
-infallible_conversion!(String);
-infallible_conversion!(Value);
-
 impl From<Value> for String {
     fn from(val: Value) -> Self {
         val.to_string()
@@ -812,15 +832,15 @@ impl From<usize> for Value {
     }
 }
 
-impl<T: ArgType> ArgType for Vec<T> {
-    fn from_value(value: Option<Value>) -> Result<Self, Error> {
+impl<'a, T: ArgType<'a>> ArgType<'a> for Vec<T> {
+    fn from_value(value: Option<&'a Value>) -> Result<Self, Error> {
         match value {
             None => Ok(Vec::new()),
             Some(values) => {
                 let values = values.as_slice()?;
                 let mut rv = Vec::new();
                 for value in values {
-                    rv.push(ArgType::from_value(Some(value.clone()))?);
+                    rv.push(ArgType::from_value(Some(value))?);
                 }
                 Ok(rv)
             }
@@ -1040,7 +1060,7 @@ impl Value {
     }
 
     /// Calls the value directly.
-    pub(crate) fn call(&self, state: &State, args: Vec<Value>) -> Result<Value, Error> {
+    pub(crate) fn call(&self, state: &State, args: &[Value]) -> Result<Value, Error> {
         if let ValueRepr::Dynamic(ref dy) = self.0 {
             dy.call(state, args)
         } else {
@@ -1056,7 +1076,7 @@ impl Value {
         &self,
         state: &State,
         name: &str,
-        args: Vec<Value>,
+        args: &[Value],
     ) -> Result<Value, Error> {
         if let ValueRepr::Dynamic(ref dy) = self.0 {
             dy.call_method(state, name, args)
@@ -1097,19 +1117,6 @@ impl Value {
         }
     }
 
-    pub(crate) fn try_into_vec(self) -> Result<Vec<Value>, Error> {
-        match self.0 {
-            ValueRepr::Seq(v) => Ok(match RcType::try_unwrap(v) {
-                Ok(v) => v,
-                Err(rc) => (*rc).clone(),
-            }),
-            _ => Err(Error::new(
-                ErrorKind::ImpossibleOperation,
-                "cannot convert value into list",
-            )),
-        }
-    }
-
     pub(crate) fn iter_as_str_map(&self) -> impl Iterator<Item = (&str, Value)> {
         match self.0 {
             ValueRepr::Map(ref m) => Box::new(
@@ -1712,7 +1719,7 @@ pub trait Object: fmt::Display + fmt::Debug + Any + Sync + Send {
     ///
     /// It's the responsibility of the implementer to ensure that an
     /// error is generated if an invalid method is invoked.
-    fn call_method(&self, state: &State, name: &str, args: Vec<Value>) -> Result<Value, Error> {
+    fn call_method(&self, state: &State, name: &str, args: &[Value]) -> Result<Value, Error> {
         let _state = state;
         let _args = args;
         Err(Error::new(
@@ -1725,7 +1732,7 @@ pub trait Object: fmt::Display + fmt::Debug + Any + Sync + Send {
     ///
     /// The default implementation just generates an error that the object
     /// cannot be invoked.
-    fn call(&self, state: &State, args: Vec<Value>) -> Result<Value, Error> {
+    fn call(&self, state: &State, args: &[Value]) -> Result<Value, Error> {
         let _state = state;
         let _args = args;
         Err(Error::new(
diff --git a/minijinja/src/vm.rs b/minijinja/src/vm.rs
index 98bb5aa..9101b20 100644
--- a/minijinja/src/vm.rs
+++ b/minijinja/src/vm.rs
@@ -64,21 +64,22 @@ impl Object for LoopState {
         }
     }
 
-    fn call(&self, _state: &State, _args: Vec<Value>) -> Result<Value, Error> {
+    fn call(&self, _state: &State, _args: &[Value]) -> Result<Value, Error> {
         Err(Error::new(
             ErrorKind::ImpossibleOperation,
             "loop cannot be called if reassigned to different variable",
         ))
     }
 
-    fn call_method(&self, _state: &State, name: &str, args: Vec<Value>) -> Result<Value, Error> {
+    fn call_method(&self, _state: &State, name: &str, args: &[Value]) -> Result<Value, Error> {
         #[cfg(feature = "sync")]
         {
             if name == "changed" {
                 let mut last_changed_value = self.last_changed_value.lock().unwrap();
-                let changed = last_changed_value.as_ref() != Some(&args);
+                let value = args.to_owned();
+                let changed = last_changed_value.as_ref() != Some(&value);
                 if changed {
-                    *last_changed_value = Some(args);
+                    *last_changed_value = Some(value);
                     return Ok(Value::from(true));
                 }
                 return Ok(Value::from(false));
@@ -403,8 +404,8 @@ impl<'vm, 'env> State<'vm, 'env> {
     pub(crate) fn apply_filter(
         &self,
         name: &str,
-        value: Value,
-        args: Vec<Value>,
+        value: &Value,
+        args: &[Value],
     ) -> Result<Value, Error> {
         if let Some(filter) = self.env().get_filter(name) {
             filter.apply_to(self, value, args)
@@ -419,8 +420,8 @@ impl<'vm, 'env> State<'vm, 'env> {
     pub(crate) fn perform_test(
         &self,
         name: &str,
-        value: Value,
-        args: Vec<Value>,
+        value: &Value,
+        args: &[Value],
     ) -> Result<bool, Error> {
         if let Some(test) = self.env().get_test(name) {
             test.perform(self, value, args)
@@ -683,12 +684,12 @@ impl<'env> Vm<'env> {
                     stack.push(Value::from(map));
                 }
                 Instruction::BuildList(count) => {
-                    let mut v = Vec::new();
+                    let mut v = Vec::with_capacity(*count);
                     for _ in 0..*count {
                         v.push(stack.pop());
                     }
                     v.reverse();
-                    stack.push(v.into());
+                    stack.push(Value(ValueRepr::Seq(RcType::new(v))));
                 }
                 Instruction::UnpackList(count) => {
                     let top = stack.pop();
@@ -972,17 +973,22 @@ impl<'env> Vm<'env> {
                     end_capture!();
                 }
                 Instruction::ApplyFilter(name) => {
-                    let args = try_ctx!(stack.pop().try_into_vec());
+                    let top = stack.pop();
+                    let args = try_ctx!(top.as_slice());
                     let value = stack.pop();
-                    stack.push(try_ctx!(state.apply_filter(name, value, args)));
+                    stack.push(try_ctx!(state.apply_filter(name, &value, args)));
                 }
                 Instruction::PerformTest(name) => {
-                    let args = try_ctx!(stack.pop().try_into_vec());
+                    let top = stack.pop();
+                    let args = try_ctx!(top.as_slice());
                     let value = stack.pop();
-                    stack.push(Value::from(try_ctx!(state.perform_test(name, value, args))));
+                    stack.push(Value::from(
+                        try_ctx!(state.perform_test(name, &value, args)),
+                    ));
                 }
                 Instruction::CallFunction(function_name) => {
-                    let args = try_ctx!(stack.pop().try_into_vec());
+                    let top = stack.pop();
+                    let args = try_ctx!(top.as_slice());
                     // super is a special function reserved for super-ing into blocks.
                     if *function_name == "super" {
                         if !args.is_empty() {
@@ -1000,7 +1006,7 @@ impl<'env> Vm<'env> {
                                 format!("loop() takes one argument, got {}", args.len())
                             ));
                         }
-                        stack.push(args.into_iter().next().unwrap());
+                        stack.push(args[0].clone());
                         recurse_loop!(true);
                     } else if let Some(func) = state.ctx.load(self.env, function_name) {
                         stack.push(try_ctx!(func.call(state, args)));
@@ -1012,12 +1018,14 @@ impl<'env> Vm<'env> {
                     }
                 }
                 Instruction::CallMethod(name) => {
-                    let args = try_ctx!(stack.pop().try_into_vec());
+                    let top = stack.pop();
+                    let args = try_ctx!(top.as_slice());
                     let obj = stack.pop();
                     stack.push(try_ctx!(obj.call_method(state, name, args)));
                 }
                 Instruction::CallObject => {
-                    let args = try_ctx!(stack.pop().try_into_vec());
+                    let top = stack.pop();
+                    let args = try_ctx!(top.as_slice());
                     let obj = stack.pop();
                     stack.push(try_ctx!(obj.call(state, args)));
                 }


Test patch:
diff --git a/minijinja/src/tests.rs b/minijinja/src/tests.rs
index 66b72ea..9a1f825 100644
--- a/minijinja/src/tests.rs
+++ b/minijinja/src/tests.rs
@@ -51,7 +51,7 @@ use crate::error::Error;
 use crate::value::{ArgType, FunctionArgs, RcType, Value};
 use crate::vm::State;
 
-type TestFunc = dyn Fn(&State, Value, Vec<Value>) -> Result<bool, Error> + Sync + Send + 'static;
+type TestFunc = dyn Fn(&State, &Value, &[Value]) -> Result<bool, Error> + Sync + Send + 'static;
 
 #[derive(Clone)]
 pub(crate) struct BoxedTest(RcType<TestFunc>);
@@ -89,14 +89,15 @@ impl BoxedTest {
     pub fn new<F, V, Args>(f: F) -> BoxedTest
     where
         F: Test<V, Args>,
-        V: ArgType,
-        Args: FunctionArgs,
+        V: for<'a> ArgType<'a>,
+        Args: for<'a> FunctionArgs<'a>,
     {
         BoxedTest(RcType::new(
             move |state, value, args| -> Result<bool, Error> {
+                let value = Some(value);
                 f.perform(
                     state,
-                    ArgType::from_value(Some(value))?,
+                    ArgType::from_value(value)?,
                     FunctionArgs::from_values(args)?,
                 )
             },
@@ -104,7 +105,7 @@ impl BoxedTest {
     }
 
     /// Applies the filter to a value and argument.
-    pub fn perform(&self, state: &State, value: Value, args: Vec<Value>) -> Result<bool, Error> {
+    pub fn perform(&self, state: &State, value: &Value, args: &[Value]) -> Result<bool, Error> {
         (self.0)(state, value, args)
     }
 }
@@ -213,7 +214,7 @@ mod builtins {
         };
         let bx = BoxedTest::new(test);
         assert!(bx
-            .perform(&state, Value::from(23), vec![Value::from(23)])
+            .perform(&state, &Value::from(23), &[Value::from(23)][..])
             .unwrap());
     }
 }