From 3f7fe9ceed67e85b3ee0cdfb0ac00493be983396 Mon Sep 17 00:00:00 2001 From: Armin Ronacher Date: Wed, 16 Nov 2022 12:41:55 +0100 Subject: [PATCH 01/16] Refactor objects to support sequences --- examples/load-lazy/src/main.rs | 10 +- minijinja/src/filters.rs | 4 +- minijinja/src/value/argtypes.rs | 20 ++- minijinja/src/value/mod.rs | 151 +++++++++++++++++----- minijinja/src/value/object.rs | 210 ++++++++++++++++++++++++++----- minijinja/src/value/ops.rs | 4 +- minijinja/src/vm/loop_object.rs | 76 +++++------ minijinja/src/vm/macro_object.rs | 46 ++++--- minijinja/src/vm/mod.rs | 2 +- minijinja/tests/test_value.rs | 58 ++++++++- 10 files changed, 453 insertions(+), 128 deletions(-) diff --git a/examples/load-lazy/src/main.rs b/examples/load-lazy/src/main.rs index c4066fa3..69b6f66e 100644 --- a/examples/load-lazy/src/main.rs +++ b/examples/load-lazy/src/main.rs @@ -4,6 +4,8 @@ use std::fmt; use std::fs; use std::sync::Mutex; +use minijinja::value::AsStruct; +use minijinja::value::ObjectBehavior; use minijinja::value::{Object, Value}; use minijinja::Environment; @@ -19,13 +21,19 @@ impl fmt::Display for Site { } impl Object for Site { + fn behavior(&self) -> ObjectBehavior<'_> { + ObjectBehavior::Struct(self) + } +} + +impl AsStruct for Site { /// This loads a file on attribute access. Note that attribute access /// can neither access the state nor return failures as such it can at /// max turn into an undefined object. /// /// If that is necessary, use `call_method()` instead which is able to /// both access interpreter state and fail. - fn get_attr(&self, name: &str) -> Option { + fn get(&self, name: &str) -> Option { let mut cache = self.cache.lock().unwrap(); if let Some(rv) = cache.get(name) { return Some(rv.clone()); diff --git a/minijinja/src/filters.rs b/minijinja/src/filters.rs index 47c362c8..b81ed90b 100644 --- a/minijinja/src/filters.rs +++ b/minijinja/src/filters.rs @@ -406,7 +406,7 @@ mod builtins { Ok(Value::from(s.chars().rev().collect::())) } else if matches!(v.kind(), ValueKind::Seq) { Ok(Value::from( - ok!(v.as_slice()).iter().rev().cloned().collect::>(), + ok!(v.as_cow_slice()).iter().rev().cloned().collect::>(), )) } else { Err(Error::new( @@ -448,7 +448,7 @@ mod builtins { Ok(rv) } else if matches!(val.kind(), ValueKind::Seq) { let mut rv = String::new(); - for item in ok!(val.as_slice()) { + for item in &ok!(val.as_cow_slice())[..] { if !rv.is_empty() { rv.push_str(joiner); } diff --git a/minijinja/src/value/argtypes.rs b/minijinja/src/value/argtypes.rs index 67842e34..354cad5e 100644 --- a/minijinja/src/value/argtypes.rs +++ b/minijinja/src/value/argtypes.rs @@ -111,6 +111,9 @@ where /// Byte slices will borrow out of values carrying bytes or strings. In the latter /// case the utf-8 bytes are returned. /// +/// Similarly it's not possible to borrow dynamic slices ([`AsSlice`](crate::value::AsSlice)) +/// into `&[Value]`. +/// /// ## Notes on State /// /// When `&State` is used, it does not consume a passed parameter. This means that @@ -463,13 +466,25 @@ impl<'a> ArgType<'a> for &Value { } } +impl<'a> ArgType<'a> for Cow<'_, [Value]> { + type Output = Cow<'a, [Value]>; + + #[inline(always)] + fn from_value(value: Option<&'a Value>) -> Result, Error> { + match value { + Some(value) => Ok(ok!(value.as_cow_slice())), + None => Err(Error::from(ErrorKind::MissingArgument)), + } + } +} + impl<'a> ArgType<'a> for &[Value] { type Output = &'a [Value]; #[inline(always)] fn from_value(value: Option<&'a Value>) -> Result<&'a [Value], Error> { match value { - Some(value) => Ok(ok!(value.as_slice())), + Some(value) => value.as_slice(), None => Err(Error::from(ErrorKind::MissingArgument)), } } @@ -573,12 +588,13 @@ impl From for Value { } impl<'a, T: ArgType<'a, Output = T>> ArgType<'a> for Vec { - type Output = Self; + type Output = Vec; fn from_value(value: Option<&'a Value>) -> Result { match value { None => Ok(Vec::new()), Some(values) => { + // TODO: can we somehow use values.as_cow_slice? let values = ok!(values.as_slice()); let mut rv = Vec::new(); for value in values { diff --git a/minijinja/src/value/mod.rs b/minijinja/src/value/mod.rs index 656d12f9..0ba73cbf 100644 --- a/minijinja/src/value/mod.rs +++ b/minijinja/src/value/mod.rs @@ -120,7 +120,7 @@ use crate::value::serialize::ValueSerializer; use crate::vm::State; pub use crate::value::argtypes::{from_args, ArgType, FunctionArgs, FunctionResult, Rest}; -pub use crate::value::object::Object; +pub use crate::value::object::{AsSeq, AsStruct, Object, ObjectBehavior}; mod argtypes; #[cfg(feature = "deserialization")] @@ -523,7 +523,13 @@ impl Value { ValueRepr::Bytes(_) => ValueKind::Bytes, ValueRepr::U128(_) => ValueKind::Number, ValueRepr::Seq(_) => ValueKind::Seq, - ValueRepr::Map(..) | ValueRepr::Dynamic(_) => ValueKind::Map, + ValueRepr::Map(..) => ValueKind::Map, + ValueRepr::Dynamic(ref dy) => match dy.behavior() { + // XXX: basic objects should probably not report as map + ObjectBehavior::Basic => ValueKind::Map, + ObjectBehavior::Seq(_) => ValueKind::Seq, + ObjectBehavior::Struct(_) => ValueKind::Map, + }, } } @@ -547,7 +553,11 @@ impl Value { ValueRepr::None | ValueRepr::Undefined => false, ValueRepr::Seq(ref x) => !x.is_empty(), ValueRepr::Map(ref x, _) => !x.is_empty(), - ValueRepr::Dynamic(_) => true, + ValueRepr::Dynamic(ref x) => match x.behavior() { + ObjectBehavior::Basic => true, + ObjectBehavior::Seq(s) => !s.is_empty(), + ObjectBehavior::Struct(s) => !s.is_empty(), + }, } } @@ -585,6 +595,9 @@ impl Value { /// If the value is a sequence it's returned as slice. /// + /// Note that [`Object`]s can impersonate sequences but they are not + /// slices themselves. As a result these will return an error. + /// /// ``` /// # use minijinja::value::Value; /// let seq = Value::from(vec![1u32, 2, 3, 4]); @@ -593,13 +606,41 @@ impl Value { /// ``` pub fn as_slice(&self) -> Result<&[Value], Error> { match self.0 { - ValueRepr::Undefined | ValueRepr::None => Ok(&[][..]), - ValueRepr::Seq(ref v) => Ok(&v[..]), - _ => Err(Error::new( - ErrorKind::InvalidOperation, - format!("value of type {} is not a sequence", self.kind()), - )), + ValueRepr::Undefined | ValueRepr::None => return Ok(&[][..]), + ValueRepr::Seq(ref v) => return Ok(&v[..]), + ValueRepr::Dynamic(ref dy) if matches!(dy.behavior(), ObjectBehavior::Seq(_)) => { + return Err(Error::new( + ErrorKind::InvalidOperation, + "dynamic sequence value cannot be borrowed as slice", + )); + } + _ => {} + } + Err(Error::new( + ErrorKind::InvalidOperation, + format!("value of type {} is not a sequence", self.kind()), + )) + } + + /// If the value is a sequence it's returned as slice. + /// + /// ``` + /// # use minijinja::value::Value; + /// let seq = Value::from(vec![1u32, 2, 3, 4]); + /// let slice = seq.as_slice().unwrap(); + /// assert_eq!(slice.len(), 4); + /// ``` + pub(crate) fn as_cow_slice(&self) -> Result, Error> { + if let ValueRepr::Dynamic(ref dy) = self.0 { + if let ObjectBehavior::Seq(seq) = dy.behavior() { + let mut items = vec![]; + for idx in 0..seq.len() { + items.push(seq.get(idx).unwrap_or(Value::UNDEFINED)); + } + return Ok(Cow::Owned(items)); + } } + self.as_slice().map(Cow::Borrowed) } /// Returns the length of the contained value. @@ -616,7 +657,11 @@ impl Value { ValueRepr::String(ref s, _) => Some(s.chars().count()), ValueRepr::Map(ref items, _) => Some(items.len()), ValueRepr::Seq(ref items) => Some(items.len()), - ValueRepr::Dynamic(ref dy) => Some(dy.attributes().count()), + ValueRepr::Dynamic(ref dy) => match dy.behavior() { + ObjectBehavior::Basic => None, + ObjectBehavior::Seq(s) => Some(s.len()), + ObjectBehavior::Struct(s) => Some(s.len()), + }, _ => None, } } @@ -643,7 +688,10 @@ impl Value { let lookup_key = Key::Str(key); items.get(&lookup_key).cloned() } - ValueRepr::Dynamic(ref dy) => dy.get_attr(key), + ValueRepr::Dynamic(ref dy) => match dy.behavior() { + ObjectBehavior::Basic | ObjectBehavior::Seq(_) => None, + ObjectBehavior::Struct(s) => s.get(key), + }, ValueRepr::Undefined => { return Err(Error::from(ErrorKind::UndefinedError)); } @@ -775,10 +823,13 @@ impl Value { return items.get(idx).cloned(); } } - ValueRepr::Dynamic(ref dy) => match key { - Key::String(ref key) => return dy.get_attr(key), - Key::Str(key) => return dy.get_attr(key), - _ => {} + ValueRepr::Dynamic(ref dy) => match dy.behavior() { + ObjectBehavior::Basic | ObjectBehavior::Seq(_) => {} + ObjectBehavior::Struct(s) => match key { + Key::String(ref key) => return s.get(key), + Key::Str(key) => return s.get(key), + _ => {} + }, }, _ => {} } @@ -853,10 +904,15 @@ impl Value { m.iter() .filter_map(|(k, v)| k.as_str().map(move |k| (k, v.clone()))), ) as Box>, - ValueRepr::Dynamic(ref obj) => Box::new( - obj.attributes() - .filter_map(move |attr| Some((attr, some!(obj.get_attr(attr))))), - ) as Box>, + ValueRepr::Dynamic(ref obj) => match obj.behavior() { + ObjectBehavior::Basic | ObjectBehavior::Seq(_) => { + Box::new(None.into_iter()) as Box> + } + ObjectBehavior::Struct(s) => Box::new( + s.fields() + .filter_map(move |attr| Some((attr, some!(s.get(attr))))), + ) as Box>, + }, _ => Box::new(None.into_iter()) as Box>, } } @@ -879,9 +935,23 @@ impl Value { items.len(), ), ValueRepr::Dynamic(ref obj) => { - let attrs = obj.attributes().map(Value::from).collect::>(); - let attr_count = attrs.len(); - (ValueIteratorState::Seq(0, Arc::new(attrs)), attr_count) + match obj.behavior() { + ObjectBehavior::Basic => (ValueIteratorState::Empty, 0), + ObjectBehavior::Seq(s) => { + // TODO: lazy iteration? + let mut items = vec![]; + for idx in 0..s.len() { + items.push(s.get(idx).unwrap_or(Value::UNDEFINED)); + } + (ValueIteratorState::Seq(0, Arc::new(items)), s.len()) + } + ObjectBehavior::Struct(s) => { + // TODO: lazy iteration? + let attrs = s.fields().map(Value::from).collect::>(); + let attr_count = s.len(); + (ValueIteratorState::Seq(0, Arc::new(attrs)), attr_count) + } + } } _ => { return Err(Error::new( @@ -930,15 +1000,26 @@ impl Serialize for Value { } map.end() } - ValueRepr::Dynamic(ref n) => { - use serde::ser::SerializeMap; - let mut s = ok!(serializer.serialize_map(None)); - for k in n.attributes() { - let v = n.get_attr(k).unwrap_or(Value::UNDEFINED); - ok!(s.serialize_entry(&k, &v)); + ValueRepr::Dynamic(ref dy) => match dy.behavior() { + ObjectBehavior::Basic => serializer.serialize_str(&dy.to_string()), + ObjectBehavior::Seq(s) => { + use serde::ser::SerializeSeq; + let mut seq = ok!(serializer.serialize_seq(Some(s.len()))); + for idx in 0..s.len() { + ok!(seq.serialize_element(&s.get(idx).unwrap_or(Value::UNDEFINED))); + } + seq.end() } - s.end() - } + ObjectBehavior::Struct(s) => { + use serde::ser::SerializeMap; + let mut map = ok!(serializer.serialize_map(None)); + for k in s.fields() { + let v = s.get(k).unwrap_or(Value::UNDEFINED); + ok!(map.serialize_entry(&k, &v)); + } + map.end() + } + }, } } } @@ -1038,14 +1119,20 @@ fn test_dynamic_object_roundtrip() { } impl Object for X { - fn get_attr(&self, name: &str) -> Option { + fn behavior(&self) -> ObjectBehavior<'_> { + ObjectBehavior::Struct(self) + } + } + + impl crate::value::object::AsStruct for X { + fn get(&self, name: &str) -> Option { match name { "value" => Some(Value::from(self.0.load(atomic::Ordering::Relaxed))), _ => None, } } - fn attributes(&self) -> Box + '_> { + fn fields(&self) -> Box + '_> { Box::new(["value"].into_iter()) } } diff --git a/minijinja/src/value/object.rs b/minijinja/src/value/object.rs index 1ed7fc22..3eb3b3c2 100644 --- a/minijinja/src/value/object.rs +++ b/minijinja/src/value/object.rs @@ -22,33 +22,23 @@ use crate::vm::State; /// Objects need to implement [`Display`](std::fmt::Display) which is used by /// the engine to convert the object into a string if needed. Additionally /// [`Debug`](std::fmt::Debug) is required as well. +/// +/// To customize the behavior of the object one needs to implement +/// [`behavior`](Self::behavior). This defines for instance if an object is a +/// sequence or a map. +/// +/// For examples of how to implement objects refer to [`AsSeq`] and +/// [`AsStruct`]. pub trait Object: fmt::Display + fmt::Debug + Any + Sync + Send { - /// Invoked by the engine to get the attribute of an object. - /// - /// Where possible it's a good idea for this to align with the return value - /// of [`attributes`](Self::attributes) but it's not necessary. + /// Describes the behavior of the object. /// - /// If an attribute does not exist, `None` shall be returned. - /// - /// A note should be made here on side effects: unlike calling objects or - /// calling methods on objects, accessing attributes is not supposed to - /// have side effects. Neither does this API get access to the interpreter - /// [`State`] nor is there a channel to send out failures as only an option - /// can be returned. If you do plan on doing something in attribute access - /// that is fallible, instead use a method call. - fn get_attr(&self, name: &str) -> Option { - let _name = name; - None - } - - /// An enumeration of attributes that are known to exist on this object. + /// If not implemented behavior for an object is [`ObjectBehavior::Basic`] + /// which just means that it's stringifyable and potentially can be + /// called or has methods. /// - /// The default implementation returns an empty iterator. If it's not possible - /// to implement this, it's fine for the implementation to be omitted. The - /// enumeration here is used by the `for` loop to iterate over the attributes - /// on the value. - fn attributes(&self) -> Box + '_> { - Box::new(None.into_iter()) + /// For more information see [`ObjectBehavior`]. + fn behavior(&self) -> ObjectBehavior<'_> { + ObjectBehavior::Basic } /// Called when the engine tries to call a method on the object. @@ -85,12 +75,8 @@ pub trait Object: fmt::Display + fmt::Debug + Any + Sync + Send { } impl Object for std::sync::Arc { - fn get_attr(&self, name: &str) -> Option { - T::get_attr(self, name) - } - - fn attributes(&self) -> Box + '_> { - T::attributes(self) + fn behavior(&self) -> ObjectBehavior<'_> { + T::behavior(self) } fn call_method(&self, state: &State, name: &str, args: &[Value]) -> Result { @@ -101,3 +87,167 @@ impl Object for std::sync::Arc { T::call(self, state, args) } } + +/// Returns the object's behavior. +/// +/// When the engine works with a dynamic object it will typically try to +/// determine the behavior by invoking [`Object::behavior`]. More behaviors +/// can be added in the future so this enum is non-exhaustive. +/// +/// Today object's can have the behavior of structs and sequences but this +/// might expand in the future. It does mean that not all types of values can +/// be represented by objects. +#[non_exhaustive] +pub enum ObjectBehavior<'a> { + /// This object is a basic object. + /// + /// Such an object has no attributes but it might be callable and it + /// can be stringified. When serialized it's serialized in it's + /// stringified form. + Basic, + /// This object is a sequence. + Seq(&'a dyn AsSeq), + /// This object is a struct. + /// + /// Structs are maps with string keys. + Struct(&'a dyn AsStruct), +} + +/// Views an [`Object`] as sequence of values. +/// +/// # Example +/// +/// The following example shows how to implement a dynamic object which +/// represents a sequence of three items: +/// +/// ``` +/// use std::fmt; +/// use minijinja::value::{Value, Object, ObjectBehavior, AsSeq}; +/// +/// #[derive(Debug, Clone)] +/// struct Point(f32, f32, f32); +/// +/// impl fmt::Display for Point { +/// fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { +/// write!(f, "({}, {}, {})", self.0, self.1, self.2) +/// } +/// } +/// +/// impl Object for Point { +/// fn behavior(&self) -> ObjectBehavior<'_> { +/// ObjectBehavior::Seq(self) +/// } +/// } +/// +/// impl AsSeq for Point { +/// fn get(&self, idx: usize) -> Option { +/// match idx { +/// 0 => Some(Value::from(self.0)), +/// 1 => Some(Value::from(self.1)), +/// 2 => Some(Value::from(self.2)), +/// _ => None, +/// } +/// } +/// +/// fn len(&self) -> usize { +/// 3 +/// } +/// } +/// +/// let value = Value::from_object(Point(1.0, 2.5, 3.0)); +/// ``` +pub trait AsSeq { + /// Looks up an item by index. + fn get(&self, idx: usize) -> Option; + + /// Returns the number of items in the sequence. + fn len(&self) -> usize; + + /// Checks if the struct is empty. + /// + /// The default implementation checks if the length is 0. + fn is_empty(&self) -> bool { + self.len() == 0 + } +} + +/// Views an [`Object`] as a struct. +/// +/// # Example +/// +/// The following example shows how to implement a dynamic object which +/// represents a struct: +/// +/// ``` +/// use std::fmt; +/// use minijinja::value::{Value, Object, ObjectBehavior, AsStruct}; +/// +/// #[derive(Debug, Clone)] +/// struct Point(f32, f32, f32); +/// +/// impl fmt::Display for Point { +/// fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { +/// write!(f, "({}, {}, {})", self.0, self.1, self.2) +/// } +/// } +/// +/// impl Object for Point { +/// fn behavior(&self) -> ObjectBehavior<'_> { +/// ObjectBehavior::Struct(self) +/// } +/// } +/// +/// impl AsStruct for Point { +/// fn get(&self, name: &str) -> Option { +/// match name { +/// "x" => Some(Value::from(self.0)), +/// "y" => Some(Value::from(self.1)), +/// "z" => Some(Value::from(self.2)), +/// _ => None, +/// } +/// } +/// +/// fn fields(&self) -> Box + '_> { +/// Box::new(["x", "y", "z"].into_iter()) +/// } +/// } +/// +/// let value = Value::from_object(Point(1.0, 2.5, 3.0)); +/// ``` +pub trait AsStruct { + /// Invoked by the engine to get a field of a struct. + /// + /// Where possible it's a good idea for this to align with the return value + /// of [`fields`](Self::fields) but it's not necessary. + /// + /// If an field does not exist, `None` shall be returned. + /// + /// A note should be made here on side effects: unlike calling objects or + /// calling methods on objects, accessing fields is not supposed to + /// have side effects. Neither does this API get access to the interpreter + /// [`State`] nor is there a channel to send out failures as only an option + /// can be returned. If you do plan on doing something in field access + /// that is fallible, instead use a method call. + fn get(&self, idx: &str) -> Option; + + /// Iterates over the fields. + /// + /// The default implementation returns an empty iterator. + fn fields(&self) -> Box + '_> { + Box::new(None.into_iter()) + } + + /// Returns the number of fields in the struct. + /// + /// The default implementation returns the number of fields. + fn len(&self) -> usize { + self.fields().count() + } + + /// Checks if the struct is empty. + /// + /// The default implementation checks if the length is 0. + fn is_empty(&self) -> bool { + self.len() == 0 + } +} diff --git a/minijinja/src/value/ops.rs b/minijinja/src/value/ops.rs index 30599dc3..17813540 100644 --- a/minijinja/src/value/ops.rs +++ b/minijinja/src/value/ops.rs @@ -108,7 +108,9 @@ pub fn slice(value: Value, start: Value, stop: Value, step: Value) -> Result) -> fmt::Result { let mut s = f.debug_struct("Loop"); - for attr in self.attributes() { - s.field(attr, &self.get_attr(attr).unwrap()); + for attr in self.fields() { + s.field(attr, &self.get(attr).unwrap()); } s.finish() } } impl Object for Loop { - fn attributes(&self) -> Box + '_> { - Box::new( - [ - "index0", - "index", - "length", - "revindex", - "revindex0", - "first", - "last", - "depth", - "depth0", - ] - .into_iter(), - ) - } - - fn get_attr(&self, name: &str) -> Option { - let idx = self.idx.load(Ordering::Relaxed) as u64; - let len = self.len as u64; - match name { - "index0" => Some(Value::from(idx)), - "index" => Some(Value::from(idx + 1)), - "length" => Some(Value::from(len)), - "revindex" => Some(Value::from(len.saturating_sub(idx))), - "revindex0" => Some(Value::from(len.saturating_sub(idx).saturating_sub(1))), - "first" => Some(Value::from(idx == 0)), - "last" => Some(Value::from(len == 0 || idx == len - 1)), - "depth" => Some(Value::from(self.depth + 1)), - "depth0" => Some(Value::from(self.depth)), - _ => None, - } + fn behavior(&self) -> ObjectBehavior<'_> { + ObjectBehavior::Struct(self) } fn call(&self, _state: &State, _args: &[Value]) -> Result { @@ -91,6 +61,42 @@ impl Object for Loop { } } +impl AsStruct for Loop { + fn fields(&self) -> Box + '_> { + Box::new( + [ + "index0", + "index", + "length", + "revindex", + "revindex0", + "first", + "last", + "depth", + "depth0", + ] + .into_iter(), + ) + } + + fn get(&self, name: &str) -> Option { + let idx = self.idx.load(Ordering::Relaxed) as u64; + let len = self.len as u64; + match name { + "index0" => Some(Value::from(idx)), + "index" => Some(Value::from(idx + 1)), + "length" => Some(Value::from(len)), + "revindex" => Some(Value::from(len.saturating_sub(idx))), + "revindex0" => Some(Value::from(len.saturating_sub(idx).saturating_sub(1))), + "first" => Some(Value::from(idx == 0)), + "last" => Some(Value::from(len == 0 || idx == len - 1)), + "depth" => Some(Value::from(self.depth + 1)), + "depth0" => Some(Value::from(self.depth)), + _ => None, + } + } +} + impl fmt::Display for Loop { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { write!( diff --git a/minijinja/src/vm/macro_object.rs b/minijinja/src/vm/macro_object.rs index 0735464a..9838aad1 100644 --- a/minijinja/src/vm/macro_object.rs +++ b/minijinja/src/vm/macro_object.rs @@ -6,7 +6,7 @@ use crate::error::{Error, ErrorKind}; use crate::key::Key; use crate::output::Output; use crate::utils::AutoEscape; -use crate::value::{MapType, Object, StringType, Value, ValueRepr}; +use crate::value::{AsStruct, MapType, Object, ObjectBehavior, StringType, Value, ValueRepr}; use crate::vm::state::State; use crate::vm::Vm; @@ -41,25 +41,8 @@ impl fmt::Display for Macro { } impl Object for Macro { - fn attributes(&self) -> Box + '_> { - Box::new(["name", "arguments"].into_iter()) - } - - fn get_attr(&self, name: &str) -> Option { - match name { - "name" => Some(Value(ValueRepr::String( - self.data.name.clone(), - StringType::Normal, - ))), - "arguments" => Some(Value::from( - self.data - .arg_spec - .iter() - .map(|x| Value(ValueRepr::String(x.clone(), StringType::Normal))) - .collect::>(), - )), - _ => None, - } + fn behavior(&self) -> ObjectBehavior<'_> { + ObjectBehavior::Struct(self) } fn call(&self, state: &State, args: &[Value]) -> Result { @@ -150,3 +133,26 @@ impl Object for Macro { }) } } + +impl AsStruct for Macro { + fn fields(&self) -> Box + '_> { + Box::new(["name", "arguments"].into_iter()) + } + + fn get(&self, name: &str) -> Option { + match name { + "name" => Some(Value(ValueRepr::String( + self.data.name.clone(), + StringType::Normal, + ))), + "arguments" => Some(Value::from( + self.data + .arg_spec + .iter() + .map(|x| Value(ValueRepr::String(x.clone(), StringType::Normal))) + .collect::>(), + )), + _ => None, + } + } +} diff --git a/minijinja/src/vm/mod.rs b/minijinja/src/vm/mod.rs index c4e8cb6a..c72f0b34 100644 --- a/minijinja/src/vm/mod.rs +++ b/minijinja/src/vm/mod.rs @@ -816,7 +816,7 @@ impl<'env> Vm<'env> { let top = stack.pop(); let v = ok!(top - .as_slice() + .as_cow_slice() .map_err(|e| Error::new(ErrorKind::CannotUnpack, "not a sequence").with_source(e))); if v.len() != *count { return Err(Error::new( diff --git a/minijinja/tests/test_value.rs b/minijinja/tests/test_value.rs index 048b32aa..0b95b639 100644 --- a/minijinja/tests/test_value.rs +++ b/minijinja/tests/test_value.rs @@ -2,7 +2,7 @@ use std::cmp::Ordering; use std::fmt; use insta::assert_snapshot; -use minijinja::value::{Object, Value}; +use minijinja::value::{AsSeq, AsStruct, Object, ObjectBehavior, Value}; use minijinja::ErrorKind; #[test] @@ -92,7 +92,7 @@ fn test_value_by_index() { } #[test] -fn test_object_iteration() { +fn test_map_object_iteration() { #[derive(Debug, Clone)] struct Point(i32, i32, i32); @@ -103,7 +103,13 @@ fn test_object_iteration() { } impl Object for Point { - fn get_attr(&self, name: &str) -> Option { + fn behavior(&self) -> ObjectBehavior<'_> { + ObjectBehavior::Struct(self) + } + } + + impl AsStruct for Point { + fn get(&self, name: &str) -> Option { match name { "x" => Some(Value::from(self.0)), "y" => Some(Value::from(self.1)), @@ -112,7 +118,7 @@ fn test_object_iteration() { } } - fn attributes(&self) -> Box + '_> { + fn fields(&self) -> Box + '_> { Box::new(["x", "y", "z"].into_iter()) } } @@ -128,3 +134,47 @@ fn test_object_iteration() { z: 3 "###); } + +#[test] +fn test_seq_object_iteration() { + #[derive(Debug, Clone)] + struct Point(i32, i32, i32); + + impl fmt::Display for Point { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "{}, {}, {}", self.0, self.1, self.2) + } + } + + impl Object for Point { + fn behavior(&self) -> ObjectBehavior<'_> { + ObjectBehavior::Seq(self) + } + } + + impl AsSeq for Point { + fn get(&self, index: usize) -> Option { + match index { + 0 => Some(Value::from(self.0)), + 1 => Some(Value::from(self.1)), + 2 => Some(Value::from(self.2)), + _ => None, + } + } + + fn len(&self) -> usize { + 3 + } + } + + let point = Point(1, 2, 3); + let rv = minijinja::render!( + "{% for value in point %}{{ loop.index0 }}: {{ value }}\n{% endfor %}", + point => Value::from_object(point) + ); + assert_snapshot!(rv, @r###" + 0: 1 + 1: 2 + 2: 3 + "###); +} From 18544cf349287ef9bc76a04f9975435b2954e779 Mon Sep 17 00:00:00 2001 From: Armin Ronacher Date: Wed, 16 Nov 2022 14:01:12 +0100 Subject: [PATCH 02/16] fmt --- minijinja/src/filters.rs | 6 +++++- minijinja/src/vm/mod.rs | 9 +++++---- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/minijinja/src/filters.rs b/minijinja/src/filters.rs index b81ed90b..0ddd112d 100644 --- a/minijinja/src/filters.rs +++ b/minijinja/src/filters.rs @@ -406,7 +406,11 @@ mod builtins { Ok(Value::from(s.chars().rev().collect::())) } else if matches!(v.kind(), ValueKind::Seq) { Ok(Value::from( - ok!(v.as_cow_slice()).iter().rev().cloned().collect::>(), + ok!(v.as_cow_slice()) + .iter() + .rev() + .cloned() + .collect::>(), )) } else { Err(Error::new( diff --git a/minijinja/src/vm/mod.rs b/minijinja/src/vm/mod.rs index c72f0b34..caf037e5 100644 --- a/minijinja/src/vm/mod.rs +++ b/minijinja/src/vm/mod.rs @@ -814,10 +814,11 @@ impl<'env> Vm<'env> { fn unpack_list(&self, stack: &mut Stack, count: &usize) -> Result<(), Error> { let top = stack.pop(); - let v = - ok!(top - .as_cow_slice() - .map_err(|e| Error::new(ErrorKind::CannotUnpack, "not a sequence").with_source(e))); + let v = ok!(top.as_cow_slice().map_err(|e| Error::new( + ErrorKind::CannotUnpack, + "not a sequence" + ) + .with_source(e))); if v.len() != *count { return Err(Error::new( ErrorKind::CannotUnpack, From afe9d0eecd5cec06645e8452176364a4dc7c1e7b Mon Sep 17 00:00:00 2001 From: Armin Ronacher Date: Wed, 16 Nov 2022 20:30:28 +0100 Subject: [PATCH 03/16] More efficient slicing for newfangled sequences --- minijinja/src/value/ops.rs | 63 +++++++++++++++++++++++++------------- 1 file changed, 41 insertions(+), 22 deletions(-) diff --git a/minijinja/src/value/ops.rs b/minijinja/src/value/ops.rs index 17813540..de4a1be8 100644 --- a/minijinja/src/value/ops.rs +++ b/minijinja/src/value/ops.rs @@ -2,7 +2,7 @@ use std::convert::{TryFrom, TryInto}; use std::fmt::Write; use crate::error::{Error, ErrorKind}; -use crate::value::{Arc, Value, ValueKind, ValueRepr}; +use crate::value::{Arc, ObjectBehavior, Value, ValueKind, ValueRepr}; pub enum CoerceResult { I128(i128, i128), @@ -97,29 +97,48 @@ pub fn slice(value: Value, start: Value, stop: Value, step: Value) -> Result(), - )); + match value.0 { + ValueRepr::String(ref s, _) => { + let (start, len) = get_offset_and_len(start, stop, || s.chars().count()); + return Ok(Value::from( + s.chars() + .skip(start) + .take(len) + .step_by(step) + .collect::(), + )); + } + ValueRepr::Undefined | ValueRepr::None => return Ok(Value::from(Vec::::new())), + ValueRepr::Seq(ref s) => { + let (start, len) = get_offset_and_len(start, stop, || s.len()); + return Ok(Value::from( + s.iter() + .skip(start) + .take(len) + .step_by(step) + .cloned() + .collect::>(), + )); + } + ValueRepr::Dynamic(ref dy) => { + if let ObjectBehavior::Seq(s) = dy.behavior() { + let (start, len) = get_offset_and_len(start, stop, || s.len()); + return Ok(Value::from( + (0..s.len()) + .skip(start) + .take(len) + .step_by(step) + .map(|idx| s.get(idx).unwrap_or(Value::UNDEFINED)) + .collect::>(), + )); + } + } + _ => {} } - // TODO: this converts the entire value into a slice before throwing away - // values which is wasteful. - let slice = ok!(value.as_cow_slice()); - let (start, len) = get_offset_and_len(start, stop, || slice.len()); - Ok(Value::from( - slice - .iter() - .skip(start) - .take(len) - .step_by(step) - .cloned() - .collect::>(), + Err(Error::new( + ErrorKind::InvalidOperation, + format!("value of type {} cannot be sliced", value.kind()), )) } From 3595f261880b61fb61fb026ca2f8fcce93031334 Mon Sep 17 00:00:00 2001 From: Armin Ronacher Date: Fri, 18 Nov 2022 11:46:30 +0100 Subject: [PATCH 04/16] Rename behavior to kind --- examples/dynamic/src/main.rs | 2 +- examples/load-lazy/src/main.rs | 10 ++--- minijinja/src/error.rs | 3 ++ minijinja/src/value/argtypes.rs | 4 +- minijinja/src/value/mod.rs | 70 ++++++++++++++++---------------- minijinja/src/value/object.rs | 70 +++++++++++++++++--------------- minijinja/src/value/ops.rs | 4 +- minijinja/src/vm/loop_object.rs | 10 ++--- minijinja/src/vm/macro_object.rs | 8 ++-- minijinja/tests/test_value.rs | 14 +++---- 10 files changed, 102 insertions(+), 93 deletions(-) diff --git a/examples/dynamic/src/main.rs b/examples/dynamic/src/main.rs index c0689363..9c1431ba 100644 --- a/examples/dynamic/src/main.rs +++ b/examples/dynamic/src/main.rs @@ -50,7 +50,7 @@ impl Object for Magic { Ok(Value::from(format!("magic-{}", tag))) } else { Err(Error::new( - minijinja::ErrorKind::InvalidOperation, + minijinja::ErrorKind::UnknownMethod, format!("object has no method named {}", name), )) } diff --git a/examples/load-lazy/src/main.rs b/examples/load-lazy/src/main.rs index 69b6f66e..ed46659c 100644 --- a/examples/load-lazy/src/main.rs +++ b/examples/load-lazy/src/main.rs @@ -4,8 +4,8 @@ use std::fmt; use std::fs; use std::sync::Mutex; -use minijinja::value::AsStruct; -use minijinja::value::ObjectBehavior; +use minijinja::value::ObjectKind; +use minijinja::value::StructObject; use minijinja::value::{Object, Value}; use minijinja::Environment; @@ -21,12 +21,12 @@ impl fmt::Display for Site { } impl Object for Site { - fn behavior(&self) -> ObjectBehavior<'_> { - ObjectBehavior::Struct(self) + fn kind(&self) -> ObjectKind<'_> { + ObjectKind::Struct(self) } } -impl AsStruct for Site { +impl StructObject for Site { /// This loads a file on attribute access. Note that attribute access /// can neither access the state nor return failures as such it can at /// max turn into an undefined object. diff --git a/minijinja/src/error.rs b/minijinja/src/error.rs index b6e0f82b..18883d70 100644 --- a/minijinja/src/error.rs +++ b/minijinja/src/error.rs @@ -118,6 +118,8 @@ pub enum ErrorKind { UnknownTest, /// A function is unknown UnknownFunction, + /// Un unknown method was called + UnknownMethod, /// A bad escape sequence in a string was encountered. BadEscape, /// An operation on an undefined value was attempted. @@ -147,6 +149,7 @@ impl ErrorKind { ErrorKind::UnknownFilter => "unknown filter", ErrorKind::UnknownFunction => "unknown function", ErrorKind::UnknownTest => "unknown test", + ErrorKind::UnknownMethod => "unknown method", ErrorKind::BadEscape => "bad string escape", ErrorKind::UndefinedError => "undefined value", ErrorKind::BadSerialization => "could not serialize to internal format", diff --git a/minijinja/src/value/argtypes.rs b/minijinja/src/value/argtypes.rs index 354cad5e..09a12908 100644 --- a/minijinja/src/value/argtypes.rs +++ b/minijinja/src/value/argtypes.rs @@ -111,8 +111,8 @@ where /// Byte slices will borrow out of values carrying bytes or strings. In the latter /// case the utf-8 bytes are returned. /// -/// Similarly it's not possible to borrow dynamic slices ([`AsSlice`](crate::value::AsSlice)) -/// into `&[Value]`. +/// Similarly it's not possible to borrow dynamic slices +/// ([`SeqObject`](crate::value::SeqObject)) into `&[Value]`. /// /// ## Notes on State /// diff --git a/minijinja/src/value/mod.rs b/minijinja/src/value/mod.rs index 0ba73cbf..79d6a9c9 100644 --- a/minijinja/src/value/mod.rs +++ b/minijinja/src/value/mod.rs @@ -120,7 +120,7 @@ use crate::value::serialize::ValueSerializer; use crate::vm::State; pub use crate::value::argtypes::{from_args, ArgType, FunctionArgs, FunctionResult, Rest}; -pub use crate::value::object::{AsSeq, AsStruct, Object, ObjectBehavior}; +pub use crate::value::object::{Object, ObjectKind, SeqObject, StructObject}; mod argtypes; #[cfg(feature = "deserialization")] @@ -524,11 +524,11 @@ impl Value { ValueRepr::U128(_) => ValueKind::Number, ValueRepr::Seq(_) => ValueKind::Seq, ValueRepr::Map(..) => ValueKind::Map, - ValueRepr::Dynamic(ref dy) => match dy.behavior() { + ValueRepr::Dynamic(ref dy) => match dy.kind() { // XXX: basic objects should probably not report as map - ObjectBehavior::Basic => ValueKind::Map, - ObjectBehavior::Seq(_) => ValueKind::Seq, - ObjectBehavior::Struct(_) => ValueKind::Map, + ObjectKind::Basic => ValueKind::Map, + ObjectKind::Seq(_) => ValueKind::Seq, + ObjectKind::Struct(_) => ValueKind::Map, }, } } @@ -553,10 +553,10 @@ impl Value { ValueRepr::None | ValueRepr::Undefined => false, ValueRepr::Seq(ref x) => !x.is_empty(), ValueRepr::Map(ref x, _) => !x.is_empty(), - ValueRepr::Dynamic(ref x) => match x.behavior() { - ObjectBehavior::Basic => true, - ObjectBehavior::Seq(s) => !s.is_empty(), - ObjectBehavior::Struct(s) => !s.is_empty(), + ValueRepr::Dynamic(ref x) => match x.kind() { + ObjectKind::Basic => true, + ObjectKind::Seq(s) => !s.is_empty(), + ObjectKind::Struct(s) => !s.is_empty(), }, } } @@ -608,7 +608,7 @@ impl Value { match self.0 { ValueRepr::Undefined | ValueRepr::None => return Ok(&[][..]), ValueRepr::Seq(ref v) => return Ok(&v[..]), - ValueRepr::Dynamic(ref dy) if matches!(dy.behavior(), ObjectBehavior::Seq(_)) => { + ValueRepr::Dynamic(ref dy) if matches!(dy.kind(), ObjectKind::Seq(_)) => { return Err(Error::new( ErrorKind::InvalidOperation, "dynamic sequence value cannot be borrowed as slice", @@ -632,7 +632,7 @@ impl Value { /// ``` pub(crate) fn as_cow_slice(&self) -> Result, Error> { if let ValueRepr::Dynamic(ref dy) = self.0 { - if let ObjectBehavior::Seq(seq) = dy.behavior() { + if let ObjectKind::Seq(seq) = dy.kind() { let mut items = vec![]; for idx in 0..seq.len() { items.push(seq.get(idx).unwrap_or(Value::UNDEFINED)); @@ -657,10 +657,10 @@ impl Value { ValueRepr::String(ref s, _) => Some(s.chars().count()), ValueRepr::Map(ref items, _) => Some(items.len()), ValueRepr::Seq(ref items) => Some(items.len()), - ValueRepr::Dynamic(ref dy) => match dy.behavior() { - ObjectBehavior::Basic => None, - ObjectBehavior::Seq(s) => Some(s.len()), - ObjectBehavior::Struct(s) => Some(s.len()), + ValueRepr::Dynamic(ref dy) => match dy.kind() { + ObjectKind::Basic => None, + ObjectKind::Seq(s) => Some(s.len()), + ObjectKind::Struct(s) => Some(s.len()), }, _ => None, } @@ -688,9 +688,9 @@ impl Value { let lookup_key = Key::Str(key); items.get(&lookup_key).cloned() } - ValueRepr::Dynamic(ref dy) => match dy.behavior() { - ObjectBehavior::Basic | ObjectBehavior::Seq(_) => None, - ObjectBehavior::Struct(s) => s.get(key), + ValueRepr::Dynamic(ref dy) => match dy.kind() { + ObjectKind::Basic | ObjectKind::Seq(_) => None, + ObjectKind::Struct(s) => s.get(key), }, ValueRepr::Undefined => { return Err(Error::from(ErrorKind::UndefinedError)); @@ -823,9 +823,9 @@ impl Value { return items.get(idx).cloned(); } } - ValueRepr::Dynamic(ref dy) => match dy.behavior() { - ObjectBehavior::Basic | ObjectBehavior::Seq(_) => {} - ObjectBehavior::Struct(s) => match key { + ValueRepr::Dynamic(ref dy) => match dy.kind() { + ObjectKind::Basic | ObjectKind::Seq(_) => {} + ObjectKind::Struct(s) => match key { Key::String(ref key) => return s.get(key), Key::Str(key) => return s.get(key), _ => {} @@ -904,11 +904,11 @@ impl Value { m.iter() .filter_map(|(k, v)| k.as_str().map(move |k| (k, v.clone()))), ) as Box>, - ValueRepr::Dynamic(ref obj) => match obj.behavior() { - ObjectBehavior::Basic | ObjectBehavior::Seq(_) => { + ValueRepr::Dynamic(ref obj) => match obj.kind() { + ObjectKind::Basic | ObjectKind::Seq(_) => { Box::new(None.into_iter()) as Box> } - ObjectBehavior::Struct(s) => Box::new( + ObjectKind::Struct(s) => Box::new( s.fields() .filter_map(move |attr| Some((attr, some!(s.get(attr))))), ) as Box>, @@ -935,9 +935,9 @@ impl Value { items.len(), ), ValueRepr::Dynamic(ref obj) => { - match obj.behavior() { - ObjectBehavior::Basic => (ValueIteratorState::Empty, 0), - ObjectBehavior::Seq(s) => { + match obj.kind() { + ObjectKind::Basic => (ValueIteratorState::Empty, 0), + ObjectKind::Seq(s) => { // TODO: lazy iteration? let mut items = vec![]; for idx in 0..s.len() { @@ -945,7 +945,7 @@ impl Value { } (ValueIteratorState::Seq(0, Arc::new(items)), s.len()) } - ObjectBehavior::Struct(s) => { + ObjectKind::Struct(s) => { // TODO: lazy iteration? let attrs = s.fields().map(Value::from).collect::>(); let attr_count = s.len(); @@ -1000,9 +1000,9 @@ impl Serialize for Value { } map.end() } - ValueRepr::Dynamic(ref dy) => match dy.behavior() { - ObjectBehavior::Basic => serializer.serialize_str(&dy.to_string()), - ObjectBehavior::Seq(s) => { + ValueRepr::Dynamic(ref dy) => match dy.kind() { + ObjectKind::Basic => serializer.serialize_str(&dy.to_string()), + ObjectKind::Seq(s) => { use serde::ser::SerializeSeq; let mut seq = ok!(serializer.serialize_seq(Some(s.len()))); for idx in 0..s.len() { @@ -1010,7 +1010,7 @@ impl Serialize for Value { } seq.end() } - ObjectBehavior::Struct(s) => { + ObjectKind::Struct(s) => { use serde::ser::SerializeMap; let mut map = ok!(serializer.serialize_map(None)); for k in s.fields() { @@ -1119,12 +1119,12 @@ fn test_dynamic_object_roundtrip() { } impl Object for X { - fn behavior(&self) -> ObjectBehavior<'_> { - ObjectBehavior::Struct(self) + fn kind(&self) -> ObjectKind<'_> { + ObjectKind::Struct(self) } } - impl crate::value::object::AsStruct for X { + impl crate::value::object::StructObject for X { fn get(&self, name: &str) -> Option { match name { "value" => Some(Value::from(self.0.load(atomic::Ordering::Relaxed))), diff --git a/minijinja/src/value/object.rs b/minijinja/src/value/object.rs index 3eb3b3c2..b1af1e14 100644 --- a/minijinja/src/value/object.rs +++ b/minijinja/src/value/object.rs @@ -23,22 +23,22 @@ use crate::vm::State; /// the engine to convert the object into a string if needed. Additionally /// [`Debug`](std::fmt::Debug) is required as well. /// -/// To customize the behavior of the object one needs to implement -/// [`behavior`](Self::behavior). This defines for instance if an object is a -/// sequence or a map. +/// The exact runtime characteristics of the object are influenced by the +/// [`kind`](Self::kind) of the object. By default an object can just be +/// stringified and methods can be called. /// -/// For examples of how to implement objects refer to [`AsSeq`] and -/// [`AsStruct`]. +/// For examples of how to implement objects refer to [`SeqObject`] and +/// [`StructObject`]. pub trait Object: fmt::Display + fmt::Debug + Any + Sync + Send { - /// Describes the behavior of the object. + /// Describes the kind of an object. /// - /// If not implemented behavior for an object is [`ObjectBehavior::Basic`] + /// If not implemented behavior for an object is [`ObjectKind::Basic`] /// which just means that it's stringifyable and potentially can be /// called or has methods. /// - /// For more information see [`ObjectBehavior`]. - fn behavior(&self) -> ObjectBehavior<'_> { - ObjectBehavior::Basic + /// For more information see [`ObjectKind`]. + fn kind(&self) -> ObjectKind<'_> { + ObjectKind::Basic } /// Called when the engine tries to call a method on the object. @@ -52,7 +52,7 @@ pub trait Object: fmt::Display + fmt::Debug + Any + Sync + Send { let _state = state; let _args = args; Err(Error::new( - ErrorKind::InvalidOperation, + ErrorKind::UnknownMethod, format!("object has no method named {}", name), )) } @@ -75,8 +75,8 @@ pub trait Object: fmt::Display + fmt::Debug + Any + Sync + Send { } impl Object for std::sync::Arc { - fn behavior(&self) -> ObjectBehavior<'_> { - T::behavior(self) + fn kind(&self) -> ObjectKind<'_> { + T::kind(self) } fn call_method(&self, state: &State, name: &str, args: &[Value]) -> Result { @@ -88,29 +88,35 @@ impl Object for std::sync::Arc { } } -/// Returns the object's behavior. +/// A kind defines the object's behavior. /// -/// When the engine works with a dynamic object it will typically try to -/// determine the behavior by invoking [`Object::behavior`]. More behaviors -/// can be added in the future so this enum is non-exhaustive. +/// When a dynamic [`Object`] is implemented, it can be of one of the kinds +/// here. The default behavior will be a [`Basic`](Self::Basic) object which +/// doesn't do much other than that it can be printed. For an object to turn +/// into a [struct](Self::Struct) or [sequence](Self::Seq) the necessary kind +/// has to be returned with a pointer to itself. /// /// Today object's can have the behavior of structs and sequences but this /// might expand in the future. It does mean that not all types of values can /// be represented by objects. #[non_exhaustive] -pub enum ObjectBehavior<'a> { +pub enum ObjectKind<'a> { /// This object is a basic object. /// /// Such an object has no attributes but it might be callable and it /// can be stringified. When serialized it's serialized in it's /// stringified form. Basic, + /// This object is a sequence. - Seq(&'a dyn AsSeq), - /// This object is a struct. /// - /// Structs are maps with string keys. - Struct(&'a dyn AsStruct), + /// Requires that the object implements [`SeqObject`]. + Seq(&'a dyn SeqObject), + + /// This object is a struct (map with string keys). + /// + /// Requires that the object implements [`StructObject`]. + Struct(&'a dyn StructObject), } /// Views an [`Object`] as sequence of values. @@ -122,7 +128,7 @@ pub enum ObjectBehavior<'a> { /// /// ``` /// use std::fmt; -/// use minijinja::value::{Value, Object, ObjectBehavior, AsSeq}; +/// use minijinja::value::{Value, Object, ObjectKind, SeqObject}; /// /// #[derive(Debug, Clone)] /// struct Point(f32, f32, f32); @@ -134,12 +140,12 @@ pub enum ObjectBehavior<'a> { /// } /// /// impl Object for Point { -/// fn behavior(&self) -> ObjectBehavior<'_> { -/// ObjectBehavior::Seq(self) +/// fn kind(&self) -> ObjectKind<'_> { +/// ObjectKind::Seq(self) /// } /// } /// -/// impl AsSeq for Point { +/// impl SeqObject for Point { /// fn get(&self, idx: usize) -> Option { /// match idx { /// 0 => Some(Value::from(self.0)), @@ -156,7 +162,7 @@ pub enum ObjectBehavior<'a> { /// /// let value = Value::from_object(Point(1.0, 2.5, 3.0)); /// ``` -pub trait AsSeq { +pub trait SeqObject { /// Looks up an item by index. fn get(&self, idx: usize) -> Option; @@ -180,7 +186,7 @@ pub trait AsSeq { /// /// ``` /// use std::fmt; -/// use minijinja::value::{Value, Object, ObjectBehavior, AsStruct}; +/// use minijinja::value::{Value, Object, ObjectKind, StructObject}; /// /// #[derive(Debug, Clone)] /// struct Point(f32, f32, f32); @@ -192,12 +198,12 @@ pub trait AsSeq { /// } /// /// impl Object for Point { -/// fn behavior(&self) -> ObjectBehavior<'_> { -/// ObjectBehavior::Struct(self) +/// fn kind(&self) -> ObjectKind<'_> { +/// ObjectKind::Struct(self) /// } /// } /// -/// impl AsStruct for Point { +/// impl StructObject for Point { /// fn get(&self, name: &str) -> Option { /// match name { /// "x" => Some(Value::from(self.0)), @@ -214,7 +220,7 @@ pub trait AsSeq { /// /// let value = Value::from_object(Point(1.0, 2.5, 3.0)); /// ``` -pub trait AsStruct { +pub trait StructObject { /// Invoked by the engine to get a field of a struct. /// /// Where possible it's a good idea for this to align with the return value diff --git a/minijinja/src/value/ops.rs b/minijinja/src/value/ops.rs index de4a1be8..b72a4a93 100644 --- a/minijinja/src/value/ops.rs +++ b/minijinja/src/value/ops.rs @@ -2,7 +2,7 @@ use std::convert::{TryFrom, TryInto}; use std::fmt::Write; use crate::error::{Error, ErrorKind}; -use crate::value::{Arc, ObjectBehavior, Value, ValueKind, ValueRepr}; +use crate::value::{Arc, ObjectKind, Value, ValueKind, ValueRepr}; pub enum CoerceResult { I128(i128, i128), @@ -121,7 +121,7 @@ pub fn slice(value: Value, start: Value, stop: Value, step: Value) -> Result { - if let ObjectBehavior::Seq(s) = dy.behavior() { + if let ObjectKind::Seq(s) = dy.kind() { let (start, len) = get_offset_and_len(start, stop, || s.len()); return Ok(Value::from( (0..s.len()) diff --git a/minijinja/src/vm/loop_object.rs b/minijinja/src/vm/loop_object.rs index 2ae4001d..1c4ecebc 100644 --- a/minijinja/src/vm/loop_object.rs +++ b/minijinja/src/vm/loop_object.rs @@ -3,7 +3,7 @@ use std::sync::atomic::{AtomicUsize, Ordering}; use std::sync::Mutex; use crate::error::{Error, ErrorKind}; -use crate::value::{AsStruct, Object, ObjectBehavior, Value}; +use crate::value::{Object, ObjectKind, StructObject, Value}; use crate::vm::state::State; pub(crate) struct Loop { @@ -24,8 +24,8 @@ impl fmt::Debug for Loop { } impl Object for Loop { - fn behavior(&self) -> ObjectBehavior<'_> { - ObjectBehavior::Struct(self) + fn kind(&self) -> ObjectKind<'_> { + ObjectKind::Struct(self) } fn call(&self, _state: &State, _args: &[Value]) -> Result { @@ -54,14 +54,14 @@ impl Object for Loop { } } else { Err(Error::new( - ErrorKind::InvalidOperation, + ErrorKind::UnknownMethod, format!("loop object has no method named {}", name), )) } } } -impl AsStruct for Loop { +impl StructObject for Loop { fn fields(&self) -> Box + '_> { Box::new( [ diff --git a/minijinja/src/vm/macro_object.rs b/minijinja/src/vm/macro_object.rs index 9838aad1..a41ff1a7 100644 --- a/minijinja/src/vm/macro_object.rs +++ b/minijinja/src/vm/macro_object.rs @@ -6,7 +6,7 @@ use crate::error::{Error, ErrorKind}; use crate::key::Key; use crate::output::Output; use crate::utils::AutoEscape; -use crate::value::{AsStruct, MapType, Object, ObjectBehavior, StringType, Value, ValueRepr}; +use crate::value::{MapType, Object, ObjectKind, StringType, StructObject, Value, ValueRepr}; use crate::vm::state::State; use crate::vm::Vm; @@ -41,8 +41,8 @@ impl fmt::Display for Macro { } impl Object for Macro { - fn behavior(&self) -> ObjectBehavior<'_> { - ObjectBehavior::Struct(self) + fn kind(&self) -> ObjectKind<'_> { + ObjectKind::Struct(self) } fn call(&self, state: &State, args: &[Value]) -> Result { @@ -134,7 +134,7 @@ impl Object for Macro { } } -impl AsStruct for Macro { +impl StructObject for Macro { fn fields(&self) -> Box + '_> { Box::new(["name", "arguments"].into_iter()) } diff --git a/minijinja/tests/test_value.rs b/minijinja/tests/test_value.rs index 0b95b639..31794c10 100644 --- a/minijinja/tests/test_value.rs +++ b/minijinja/tests/test_value.rs @@ -2,7 +2,7 @@ use std::cmp::Ordering; use std::fmt; use insta::assert_snapshot; -use minijinja::value::{AsSeq, AsStruct, Object, ObjectBehavior, Value}; +use minijinja::value::{Object, ObjectKind, SeqObject, StructObject, Value}; use minijinja::ErrorKind; #[test] @@ -103,12 +103,12 @@ fn test_map_object_iteration() { } impl Object for Point { - fn behavior(&self) -> ObjectBehavior<'_> { - ObjectBehavior::Struct(self) + fn kind(&self) -> ObjectKind<'_> { + ObjectKind::Struct(self) } } - impl AsStruct for Point { + impl StructObject for Point { fn get(&self, name: &str) -> Option { match name { "x" => Some(Value::from(self.0)), @@ -147,12 +147,12 @@ fn test_seq_object_iteration() { } impl Object for Point { - fn behavior(&self) -> ObjectBehavior<'_> { - ObjectBehavior::Seq(self) + fn kind(&self) -> ObjectKind<'_> { + ObjectKind::Seq(self) } } - impl AsSeq for Point { + impl SeqObject for Point { fn get(&self, index: usize) -> Option { match index { 0 => Some(Value::from(self.0)), From f20ef10103afa53a3eea248285a9d0d406ea72f7 Mon Sep 17 00:00:00 2001 From: Armin Ronacher Date: Fri, 18 Nov 2022 11:55:07 +0100 Subject: [PATCH 05/16] Implement get_item_opt for dynamic sequences --- minijinja/src/value/mod.rs | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/minijinja/src/value/mod.rs b/minijinja/src/value/mod.rs index 79d6a9c9..cc91e10e 100644 --- a/minijinja/src/value/mod.rs +++ b/minijinja/src/value/mod.rs @@ -824,7 +824,18 @@ impl Value { } } ValueRepr::Dynamic(ref dy) => match dy.kind() { - ObjectKind::Basic | ObjectKind::Seq(_) => {} + ObjectKind::Basic => {} + ObjectKind::Seq(s) => { + if let Key::I64(idx) = key { + let idx = some!(isize::try_from(idx).ok()); + let idx = if idx < 0 { + some!(s.len().checked_sub(-idx as usize)) + } else { + idx as usize + }; + return s.get(idx); + } + } ObjectKind::Struct(s) => match key { Key::String(ref key) => return s.get(key), Key::Str(key) => return s.get(key), From e0d650e470ee8a074b38302dc59d2fb45f7fba93 Mon Sep 17 00:00:00 2001 From: Armin Ronacher Date: Fri, 18 Nov 2022 13:47:41 +0100 Subject: [PATCH 06/16] Added tests for indexing of dynamic objects --- minijinja/tests/test_value.rs | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/minijinja/tests/test_value.rs b/minijinja/tests/test_value.rs index 31794c10..52221c99 100644 --- a/minijinja/tests/test_value.rs +++ b/minijinja/tests/test_value.rs @@ -92,7 +92,7 @@ fn test_value_by_index() { } #[test] -fn test_map_object_iteration() { +fn test_map_object_iteration_and_indexing() { #[derive(Debug, Clone)] struct Point(i32, i32, i32); @@ -123,20 +123,25 @@ fn test_map_object_iteration() { } } - let point = Point(1, 2, 3); let rv = minijinja::render!( "{% for key in point %}{{ key }}: {{ point[key] }}\n{% endfor %}", - point => Value::from_object(point) + point => Value::from_object(Point(1, 2, 3)) ); assert_snapshot!(rv, @r###" x: 1 y: 2 z: 3 "###); + + let rv = minijinja::render!( + "{{ [point.x, point.z, point.missing_attribute] }}", + point => Value::from_object(Point(1, 2, 3)) + ); + assert_snapshot!(rv, @r###"[1, 3, Undefined]"###); } #[test] -fn test_seq_object_iteration() { +fn test_seq_object_iteration_and_indexing() { #[derive(Debug, Clone)] struct Point(i32, i32, i32); @@ -167,14 +172,19 @@ fn test_seq_object_iteration() { } } - let point = Point(1, 2, 3); let rv = minijinja::render!( "{% for value in point %}{{ loop.index0 }}: {{ value }}\n{% endfor %}", - point => Value::from_object(point) + point => Value::from_object(Point(1, 2, 3)) ); assert_snapshot!(rv, @r###" 0: 1 1: 2 2: 3 "###); + + let rv = minijinja::render!( + "{{ [point[0], point[2], point[42]] }}", + point => Value::from_object(Point(1, 2, 3)) + ); + assert_snapshot!(rv, @r###"[1, 3, Undefined]"###); } From 54eaa1d3a704909dc8ac122afbb753bb4ac05c53 Mon Sep 17 00:00:00 2001 From: Armin Ronacher Date: Fri, 18 Nov 2022 13:51:53 +0100 Subject: [PATCH 07/16] Added lazy iteration for dynamic sequences --- minijinja/src/value/mod.rs | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/minijinja/src/value/mod.rs b/minijinja/src/value/mod.rs index cc91e10e..f276aa2c 100644 --- a/minijinja/src/value/mod.rs +++ b/minijinja/src/value/mod.rs @@ -948,16 +948,11 @@ impl Value { ValueRepr::Dynamic(ref obj) => { match obj.kind() { ObjectKind::Basic => (ValueIteratorState::Empty, 0), - ObjectKind::Seq(s) => { - // TODO: lazy iteration? - let mut items = vec![]; - for idx in 0..s.len() { - items.push(s.get(idx).unwrap_or(Value::UNDEFINED)); - } - (ValueIteratorState::Seq(0, Arc::new(items)), s.len()) - } + ObjectKind::Seq(s) => (ValueIteratorState::DynSeq(0, Arc::clone(obj)), s.len()), ObjectKind::Struct(s) => { - // TODO: lazy iteration? + // the assumption is that structs don't have excessive field counts + // and that most iterations go over all fields, so creating a + // temporary vector here is acceptable. let attrs = s.fields().map(Value::from).collect::>(); let attr_count = s.len(); (ValueIteratorState::Seq(0, Arc::new(attrs)), attr_count) @@ -1081,6 +1076,7 @@ impl fmt::Debug for OwnedValueIterator { enum ValueIteratorState { Empty, Seq(usize, Arc>), + DynSeq(usize, Arc), #[cfg(not(feature = "preserve_order"))] Map(Option, Arc), #[cfg(feature = "preserve_order")] @@ -1098,6 +1094,16 @@ impl ValueIteratorState { x }) .cloned(), + ValueIteratorState::DynSeq(idx, obj) => { + if let ObjectKind::Seq(seq) = obj.kind() { + seq.get(*idx).map(|x| { + *idx += 1; + x + }) + } else { + unreachable!() + } + } #[cfg(feature = "preserve_order")] ValueIteratorState::Map(idx, map) => map.get_index(*idx).map(|x| { *idx += 1; From b26dcf7866cb1a72f76c943871d5dbe25116daf1 Mon Sep 17 00:00:00 2001 From: Armin Ronacher Date: Fri, 18 Nov 2022 13:53:17 +0100 Subject: [PATCH 08/16] Document behavior of SeqObject::get for missing items --- minijinja/src/value/object.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/minijinja/src/value/object.rs b/minijinja/src/value/object.rs index b1af1e14..efa55c62 100644 --- a/minijinja/src/value/object.rs +++ b/minijinja/src/value/object.rs @@ -164,6 +164,10 @@ pub enum ObjectKind<'a> { /// ``` pub trait SeqObject { /// Looks up an item by index. + /// + /// Sequences should provide a value for all items in the range of `0..len` + /// but the engine will assume that items within the range are `Undefined` + /// if `None` is returned. fn get(&self, idx: usize) -> Option; /// Returns the number of items in the sequence. From 90771087827746459bd05ce6136f7a9b65a191a7 Mon Sep 17 00:00:00 2001 From: Armin Ronacher Date: Fri, 18 Nov 2022 22:13:26 +0100 Subject: [PATCH 09/16] Fixed various issues with the new seq implementation --- examples/load-lazy/src/main.rs | 2 +- minijinja/src/filters.rs | 67 ++++---- minijinja/src/value/argtypes.rs | 12 -- minijinja/src/value/mod.rs | 143 ++++++++---------- minijinja/src/value/object.rs | 50 +++--- minijinja/src/value/ops.rs | 87 +++++------ minijinja/src/vm/loop_object.rs | 4 +- minijinja/src/vm/macro_object.rs | 2 +- minijinja/src/vm/mod.rs | 40 ++--- ...test_templates__vm@err_bad_filter.txt.snap | 4 +- ..._templates__vm@loop_bad_unpacking.txt.snap | 6 - ...plates__vm@loop_over_non_iterable.txt.snap | 4 +- minijinja/tests/test_value.rs | 22 +-- 13 files changed, 205 insertions(+), 238 deletions(-) diff --git a/examples/load-lazy/src/main.rs b/examples/load-lazy/src/main.rs index ed46659c..94e4b827 100644 --- a/examples/load-lazy/src/main.rs +++ b/examples/load-lazy/src/main.rs @@ -33,7 +33,7 @@ impl StructObject for Site { /// /// If that is necessary, use `call_method()` instead which is able to /// both access interpreter state and fail. - fn get(&self, name: &str) -> Option { + fn get_field(&self, name: &str) -> Option { let mut cache = self.cache.lock().unwrap(); if let Some(rv) = cache.get(name) { return Some(rv.clone()); diff --git a/minijinja/src/filters.rs b/minijinja/src/filters.rs index 0ddd112d..308f5be3 100644 --- a/minijinja/src/filters.rs +++ b/minijinja/src/filters.rs @@ -250,7 +250,7 @@ mod builtins { use super::*; use crate::error::ErrorKind; - use crate::value::{ValueKind, ValueRepr}; + use crate::value::ValueRepr; use std::borrow::Cow; use std::fmt::Write; use std::mem; @@ -404,12 +404,11 @@ mod builtins { pub fn reverse(v: Value) -> Result { if let Some(s) = v.as_str() { Ok(Value::from(s.chars().rev().collect::())) - } else if matches!(v.kind(), ValueKind::Seq) { + } else if let Some(seq) = v.as_seq() { Ok(Value::from( - ok!(v.as_cow_slice()) - .iter() + (0..seq.seq_len()) .rev() - .cloned() + .map(|idx| seq.get_item(idx).unwrap_or(Value::UNDEFINED)) .collect::>(), )) } else { @@ -450,9 +449,10 @@ mod builtins { rv.push(c); } Ok(rv) - } else if matches!(val.kind(), ValueKind::Seq) { + } else if let Some(seq) = val.as_seq() { let mut rv = String::new(); - for item in &ok!(val.as_cow_slice())[..] { + for idx in 0..seq.seq_len() { + let item = seq.get_item(idx).unwrap_or(Value::UNDEFINED); if !rv.is_empty() { rv.push_str(joiner); } @@ -541,13 +541,15 @@ mod builtins { /// ``` #[cfg_attr(docsrs, doc(cfg(feature = "builtins")))] pub fn first(value: Value) -> Result { - match value.0 { - ValueRepr::String(s, _) => Ok(s.chars().next().map_or(Value::UNDEFINED, Value::from)), - ValueRepr::Seq(ref s) => Ok(s.first().cloned().unwrap_or(Value::UNDEFINED)), - _ => Err(Error::new( + if let Some(s) = value.as_str() { + Ok(s.chars().next().map_or(Value::UNDEFINED, Value::from)) + } else if let Some(s) = value.as_seq() { + Ok(s.get_item(0).unwrap_or(Value::UNDEFINED)) + } else { + Err(Error::new( ErrorKind::InvalidOperation, "cannot get first item from value", - )), + )) } } @@ -568,15 +570,21 @@ mod builtins { /// ``` #[cfg_attr(docsrs, doc(cfg(feature = "builtins")))] pub fn last(value: Value) -> Result { - match value.0 { - ValueRepr::String(s, _) => { - Ok(s.chars().rev().next().map_or(Value::UNDEFINED, Value::from)) + if let Some(s) = value.as_str() { + Ok(s.chars().rev().next().map_or(Value::UNDEFINED, Value::from)) + } else if let Some(seq) = value.as_seq() { + let len = seq.seq_len(); + Ok(if len == 0 { + None + } else { + seq.get_item(len - 1) } - ValueRepr::Seq(ref s) => Ok(s.last().cloned().unwrap_or(Value::UNDEFINED)), - _ => Err(Error::new( + .unwrap_or(Value::UNDEFINED)) + } else { + Err(Error::new( ErrorKind::InvalidOperation, "cannot get last item from value", - )), + )) } } @@ -588,21 +596,14 @@ mod builtins { /// an empty list is returned. #[cfg_attr(docsrs, doc(cfg(feature = "builtins")))] pub fn list(value: Value) -> Result { - match &value.0 { - ValueRepr::Undefined => Ok(Value::from(Vec::::new())), - ValueRepr::String(ref s, _) => { - Ok(Value::from(s.chars().map(Value::from).collect::>())) - } - ValueRepr::Seq(_) => Ok(value.clone()), - ValueRepr::Map(ref m, _) => Ok(Value::from( - m.iter() - .map(|x| Value::from(x.0.clone())) - .collect::>(), - )), - _ => Err(Error::new( - ErrorKind::InvalidOperation, - "cannot convert value to list", - )), + if let Some(s) = value.as_str() { + Ok(Value::from(s.chars().map(Value::from).collect::>())) + } else { + let iter = ok!(value.try_iter().map_err(|err| { + Error::new(ErrorKind::InvalidOperation, "cannot convert value to list") + .with_source(err) + })); + Ok(Value::from(iter.collect::>())) } } diff --git a/minijinja/src/value/argtypes.rs b/minijinja/src/value/argtypes.rs index 09a12908..ba61c144 100644 --- a/minijinja/src/value/argtypes.rs +++ b/minijinja/src/value/argtypes.rs @@ -466,18 +466,6 @@ impl<'a> ArgType<'a> for &Value { } } -impl<'a> ArgType<'a> for Cow<'_, [Value]> { - type Output = Cow<'a, [Value]>; - - #[inline(always)] - fn from_value(value: Option<&'a Value>) -> Result, Error> { - match value { - Some(value) => Ok(ok!(value.as_cow_slice())), - None => Err(Error::from(ErrorKind::MissingArgument)), - } - } -} - impl<'a> ArgType<'a> for &[Value] { type Output = &'a [Value]; diff --git a/minijinja/src/value/mod.rs b/minijinja/src/value/mod.rs index f276aa2c..7ec2b6bc 100644 --- a/minijinja/src/value/mod.rs +++ b/minijinja/src/value/mod.rs @@ -555,8 +555,8 @@ impl Value { ValueRepr::Map(ref x, _) => !x.is_empty(), ValueRepr::Dynamic(ref x) => match x.kind() { ObjectKind::Basic => true, - ObjectKind::Seq(s) => !s.is_empty(), - ObjectKind::Struct(s) => !s.is_empty(), + ObjectKind::Seq(s) => s.seq_len() != 0, + ObjectKind::Struct(s) => s.struct_size() != 0, }, } } @@ -593,18 +593,22 @@ impl Value { } } + /// If the value is a sequence it's returned as sequence object. + pub fn as_seq(&self) -> Option<&dyn SeqObject> { + match self.0 { + ValueRepr::Seq(ref v) => return Some(&**v as &dyn SeqObject), + ValueRepr::Dynamic(ref dy) => { + if let ObjectKind::Seq(seq) = dy.kind() { + return Some(seq); + } + } + _ => {} + } + None + } + /// If the value is a sequence it's returned as slice. - /// - /// Note that [`Object`]s can impersonate sequences but they are not - /// slices themselves. As a result these will return an error. - /// - /// ``` - /// # use minijinja::value::Value; - /// let seq = Value::from(vec![1u32, 2, 3, 4]); - /// let slice = seq.as_slice().unwrap(); - /// assert_eq!(slice.len(), 4); - /// ``` - pub fn as_slice(&self) -> Result<&[Value], Error> { + pub(crate) fn as_slice(&self) -> Result<&[Value], Error> { match self.0 { ValueRepr::Undefined | ValueRepr::None => return Ok(&[][..]), ValueRepr::Seq(ref v) => return Ok(&v[..]), @@ -622,27 +626,6 @@ impl Value { )) } - /// If the value is a sequence it's returned as slice. - /// - /// ``` - /// # use minijinja::value::Value; - /// let seq = Value::from(vec![1u32, 2, 3, 4]); - /// let slice = seq.as_slice().unwrap(); - /// assert_eq!(slice.len(), 4); - /// ``` - pub(crate) fn as_cow_slice(&self) -> Result, Error> { - if let ValueRepr::Dynamic(ref dy) = self.0 { - if let ObjectKind::Seq(seq) = dy.kind() { - let mut items = vec![]; - for idx in 0..seq.len() { - items.push(seq.get(idx).unwrap_or(Value::UNDEFINED)); - } - return Ok(Cow::Owned(items)); - } - } - self.as_slice().map(Cow::Borrowed) - } - /// Returns the length of the contained value. /// /// Values without a length will return `None`. @@ -659,8 +642,8 @@ impl Value { ValueRepr::Seq(ref items) => Some(items.len()), ValueRepr::Dynamic(ref dy) => match dy.kind() { ObjectKind::Basic => None, - ObjectKind::Seq(s) => Some(s.len()), - ObjectKind::Struct(s) => Some(s.len()), + ObjectKind::Seq(s) => Some(s.seq_len()), + ObjectKind::Struct(s) => Some(s.struct_size()), }, _ => None, } @@ -690,7 +673,7 @@ impl Value { } ValueRepr::Dynamic(ref dy) => match dy.kind() { ObjectKind::Basic | ObjectKind::Seq(_) => None, - ObjectKind::Struct(s) => s.get(key), + ObjectKind::Struct(s) => s.get_field(key), }, ValueRepr::Undefined => { return Err(Error::from(ErrorKind::UndefinedError)); @@ -810,41 +793,32 @@ impl Value { fn get_item_opt(&self, key: &Value) -> Option { let key = some!(Key::from_borrowed_value(key).ok()); - match self.0 { + let seq = match self.0 { ValueRepr::Map(ref items, _) => return items.get(&key).cloned(), - ValueRepr::Seq(ref items) => { - if let Key::I64(idx) = key { - let idx = some!(isize::try_from(idx).ok()); - let idx = if idx < 0 { - some!(items.len().checked_sub(-idx as usize)) - } else { - idx as usize - }; - return items.get(idx).cloned(); - } - } + ValueRepr::Seq(ref items) => &**items as &dyn SeqObject, ValueRepr::Dynamic(ref dy) => match dy.kind() { - ObjectKind::Basic => {} - ObjectKind::Seq(s) => { - if let Key::I64(idx) = key { - let idx = some!(isize::try_from(idx).ok()); - let idx = if idx < 0 { - some!(s.len().checked_sub(-idx as usize)) - } else { - idx as usize - }; - return s.get(idx); - } - } + ObjectKind::Basic => return None, + ObjectKind::Seq(s) => s, ObjectKind::Struct(s) => match key { - Key::String(ref key) => return s.get(key), - Key::Str(key) => return s.get(key), - _ => {} + Key::String(ref key) => return s.get_field(key), + Key::Str(key) => return s.get_field(key), + _ => return None, }, }, - _ => {} + _ => return None, + }; + + if let Key::I64(idx) = key { + let idx = some!(isize::try_from(idx).ok()); + let idx = if idx < 0 { + some!(seq.seq_len().checked_sub(-idx as usize)) + } else { + idx as usize + }; + seq.get_item(idx) + } else { + None } - None } /// Calls the value directly. @@ -921,7 +895,7 @@ impl Value { } ObjectKind::Struct(s) => Box::new( s.fields() - .filter_map(move |attr| Some((attr, some!(s.get(attr))))), + .filter_map(move |attr| Some((attr, some!(s.get_field(attr))))), ) as Box>, }, _ => Box::new(None.into_iter()) as Box>, @@ -948,13 +922,15 @@ impl Value { ValueRepr::Dynamic(ref obj) => { match obj.kind() { ObjectKind::Basic => (ValueIteratorState::Empty, 0), - ObjectKind::Seq(s) => (ValueIteratorState::DynSeq(0, Arc::clone(obj)), s.len()), + ObjectKind::Seq(s) => { + (ValueIteratorState::DynSeq(0, Arc::clone(obj)), s.seq_len()) + } ObjectKind::Struct(s) => { // the assumption is that structs don't have excessive field counts // and that most iterations go over all fields, so creating a // temporary vector here is acceptable. let attrs = s.fields().map(Value::from).collect::>(); - let attr_count = s.len(); + let attr_count = s.struct_size(); (ValueIteratorState::Seq(0, Arc::new(attrs)), attr_count) } } @@ -962,7 +938,7 @@ impl Value { _ => { return Err(Error::new( ErrorKind::InvalidOperation, - "object is not iterable", + format!("{} is not iterable", self.kind()), )) } }; @@ -1010,9 +986,9 @@ impl Serialize for Value { ObjectKind::Basic => serializer.serialize_str(&dy.to_string()), ObjectKind::Seq(s) => { use serde::ser::SerializeSeq; - let mut seq = ok!(serializer.serialize_seq(Some(s.len()))); - for idx in 0..s.len() { - ok!(seq.serialize_element(&s.get(idx).unwrap_or(Value::UNDEFINED))); + let mut seq = ok!(serializer.serialize_seq(Some(s.seq_len()))); + for idx in 0..s.seq_len() { + ok!(seq.serialize_element(&s.get_item(idx).unwrap_or(Value::UNDEFINED))); } seq.end() } @@ -1020,7 +996,7 @@ impl Serialize for Value { use serde::ser::SerializeMap; let mut map = ok!(serializer.serialize_map(None)); for k in s.fields() { - let v = s.get(k).unwrap_or(Value::UNDEFINED); + let v = s.get_field(k).unwrap_or(Value::UNDEFINED); ok!(map.serialize_entry(&k, &v)); } map.end() @@ -1096,7 +1072,7 @@ impl ValueIteratorState { .cloned(), ValueIteratorState::DynSeq(idx, obj) => { if let ObjectKind::Seq(seq) = obj.kind() { - seq.get(*idx).map(|x| { + seq.get_item(*idx).map(|x| { *idx += 1; x }) @@ -1142,7 +1118,7 @@ fn test_dynamic_object_roundtrip() { } impl crate::value::object::StructObject for X { - fn get(&self, name: &str) -> Option { + fn get_field(&self, name: &str) -> Option { match name { "value" => Some(Value::from(self.0.load(atomic::Ordering::Relaxed))), _ => None, @@ -1169,3 +1145,18 @@ fn test_dynamic_object_roundtrip() { fn test_sizes() { assert_eq!(std::mem::size_of::(), 24); } + +#[test] +fn test_value_as_slice() { + let val = Value::from(vec![1u32, 2, 3]); + assert_eq!( + val.as_slice().unwrap(), + &[Value::from(1), Value::from(2), Value::from(3)] + ); + assert_eq!(Value::UNDEFINED.as_slice().unwrap(), &[]); + assert_eq!(Value::from(()).as_slice().unwrap(), &[]); + assert_eq!( + Value::from("foo").as_slice().unwrap_err().kind(), + ErrorKind::InvalidOperation + ); +} diff --git a/minijinja/src/value/object.rs b/minijinja/src/value/object.rs index efa55c62..2320f342 100644 --- a/minijinja/src/value/object.rs +++ b/minijinja/src/value/object.rs @@ -146,7 +146,7 @@ pub enum ObjectKind<'a> { /// } /// /// impl SeqObject for Point { -/// fn get(&self, idx: usize) -> Option { +/// fn get_item(&self, idx: usize) -> Option { /// match idx { /// 0 => Some(Value::from(self.0)), /// 1 => Some(Value::from(self.1)), @@ -155,7 +155,7 @@ pub enum ObjectKind<'a> { /// } /// } /// -/// fn len(&self) -> usize { +/// fn seq_len(&self) -> usize { /// 3 /// } /// } @@ -165,19 +165,36 @@ pub enum ObjectKind<'a> { pub trait SeqObject { /// Looks up an item by index. /// - /// Sequences should provide a value for all items in the range of `0..len` + /// Sequences should provide a value for all items in the range of `0..seq_len` /// but the engine will assume that items within the range are `Undefined` /// if `None` is returned. - fn get(&self, idx: usize) -> Option; + fn get_item(&self, idx: usize) -> Option; /// Returns the number of items in the sequence. - fn len(&self) -> usize; + fn seq_len(&self) -> usize; +} - /// Checks if the struct is empty. - /// - /// The default implementation checks if the length is 0. - fn is_empty(&self) -> bool { - self.len() == 0 +impl<'a> SeqObject for &'a [Value] { + #[inline(always)] + fn get_item(&self, idx: usize) -> Option { + self.get(idx).cloned().map(Into::into) + } + + #[inline(always)] + fn seq_len(&self) -> usize { + self.len() + } +} + +impl SeqObject for Vec { + #[inline(always)] + fn get_item(&self, idx: usize) -> Option { + self.get(idx).cloned().map(Into::into) + } + + #[inline(always)] + fn seq_len(&self) -> usize { + self.len() } } @@ -208,7 +225,7 @@ pub trait SeqObject { /// } /// /// impl StructObject for Point { -/// fn get(&self, name: &str) -> Option { +/// fn get_field(&self, name: &str) -> Option { /// match name { /// "x" => Some(Value::from(self.0)), /// "y" => Some(Value::from(self.1)), @@ -238,7 +255,7 @@ pub trait StructObject { /// [`State`] nor is there a channel to send out failures as only an option /// can be returned. If you do plan on doing something in field access /// that is fallible, instead use a method call. - fn get(&self, idx: &str) -> Option; + fn get_field(&self, idx: &str) -> Option; /// Iterates over the fields. /// @@ -250,14 +267,7 @@ pub trait StructObject { /// Returns the number of fields in the struct. /// /// The default implementation returns the number of fields. - fn len(&self) -> usize { + fn struct_size(&self) -> usize { self.fields().count() } - - /// Checks if the struct is empty. - /// - /// The default implementation checks if the length is 0. - fn is_empty(&self) -> bool { - self.len() == 0 - } } diff --git a/minijinja/src/value/ops.rs b/minijinja/src/value/ops.rs index b72a4a93..6bf56209 100644 --- a/minijinja/src/value/ops.rs +++ b/minijinja/src/value/ops.rs @@ -2,7 +2,7 @@ use std::convert::{TryFrom, TryInto}; use std::fmt::Write; use crate::error::{Error, ErrorKind}; -use crate::value::{Arc, ObjectKind, Value, ValueKind, ValueRepr}; +use crate::value::{Arc, ObjectKind, SeqObject, Value, ValueKind, ValueRepr}; pub enum CoerceResult { I128(i128, i128), @@ -97,7 +97,7 @@ pub fn slice(value: Value, start: Value, stop: Value, step: Value) -> Result { let (start, len) = get_offset_and_len(start, stop, || s.chars().count()); return Ok(Value::from( @@ -109,37 +109,34 @@ pub fn slice(value: Value, start: Value, stop: Value, step: Value) -> Result return Ok(Value::from(Vec::::new())), - ValueRepr::Seq(ref s) => { - let (start, len) = get_offset_and_len(start, stop, || s.len()); - return Ok(Value::from( - s.iter() + ValueRepr::Seq(ref s) => Some(&**s as &dyn SeqObject), + ValueRepr::Dynamic(ref dy) => { + if let ObjectKind::Seq(seq) = dy.kind() { + Some(seq) + } else { + None + } + } + _ => None, + }; + + match maybe_seq { + Some(seq) => { + let (start, len) = get_offset_and_len(start, stop, || seq.seq_len()); + Ok(Value::from( + (0..seq.seq_len()) .skip(start) .take(len) .step_by(step) - .cloned() + .map(|idx| seq.get_item(idx).unwrap_or(Value::UNDEFINED)) .collect::>(), - )); - } - ValueRepr::Dynamic(ref dy) => { - if let ObjectKind::Seq(s) = dy.kind() { - let (start, len) = get_offset_and_len(start, stop, || s.len()); - return Ok(Value::from( - (0..s.len()) - .skip(start) - .take(len) - .step_by(step) - .map(|idx| s.get(idx).unwrap_or(Value::UNDEFINED)) - .collect::>(), - )); - } + )) } - _ => {} + None => Err(Error::new( + ErrorKind::InvalidOperation, + format!("value of type {} cannot be sliced", value.kind()), + )), } - - Err(Error::new( - ErrorKind::InvalidOperation, - format!("value of type {} cannot be sliced", value.kind()), - )) } fn int_as_value(val: i128) -> Value { @@ -268,27 +265,27 @@ pub fn string_concat(mut left: Value, right: &Value) -> Value { /// Implements a containment operation on values. pub fn contains(container: &Value, value: &Value) -> Result { - match container.0 { - ValueRepr::Seq(ref values) => Ok(Value::from(values.contains(value))), - ValueRepr::Map(ref map, _) => { - let key = match value.clone().try_into_key() { - Ok(key) => key, - Err(_) => return Ok(Value::from(false)), - }; - return Ok(Value::from(map.get(&key).is_some())); - } - ValueRepr::String(ref s, _) => { - return Ok(Value::from(if let Some(s2) = value.as_str() { - s.contains(s2) - } else { - s.contains(&value.to_string()) - })); + let rv = if let Some(s) = container.as_str() { + if let Some(s2) = value.as_str() { + s.contains(s2) + } else { + s.contains(&value.to_string()) } - _ => Err(Error::new( + } else if let Some(seq) = container.as_seq() { + (0..seq.seq_len()).any(|idx| seq.get_item(idx).as_ref() == Some(value)) + } else if let ValueRepr::Map(ref map, _) = container.0 { + let key = match value.clone().try_into_key() { + Ok(key) => key, + Err(_) => return Ok(Value::from(false)), + }; + map.get(&key).is_some() + } else { + return Err(Error::new( ErrorKind::InvalidOperation, "cannot perform a containment check on this value", - )), - } + )); + }; + Ok(Value::from(rv)) } #[test] diff --git a/minijinja/src/vm/loop_object.rs b/minijinja/src/vm/loop_object.rs index 1c4ecebc..7e9c70ef 100644 --- a/minijinja/src/vm/loop_object.rs +++ b/minijinja/src/vm/loop_object.rs @@ -17,7 +17,7 @@ impl fmt::Debug for Loop { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { let mut s = f.debug_struct("Loop"); for attr in self.fields() { - s.field(attr, &self.get(attr).unwrap()); + s.field(attr, &self.get_field(attr).unwrap()); } s.finish() } @@ -79,7 +79,7 @@ impl StructObject for Loop { ) } - fn get(&self, name: &str) -> Option { + fn get_field(&self, name: &str) -> Option { let idx = self.idx.load(Ordering::Relaxed) as u64; let len = self.len as u64; match name { diff --git a/minijinja/src/vm/macro_object.rs b/minijinja/src/vm/macro_object.rs index a41ff1a7..46607225 100644 --- a/minijinja/src/vm/macro_object.rs +++ b/minijinja/src/vm/macro_object.rs @@ -139,7 +139,7 @@ impl StructObject for Macro { Box::new(["name", "arguments"].into_iter()) } - fn get(&self, name: &str) -> Option { + fn get_field(&self, name: &str) -> Option { match name { "name" => Some(Value(ValueRepr::String( self.data.name.clone(), diff --git a/minijinja/src/vm/mod.rs b/minijinja/src/vm/mod.rs index caf037e5..c3b1db04 100644 --- a/minijinja/src/vm/mod.rs +++ b/minijinja/src/vm/mod.rs @@ -283,6 +283,7 @@ impl<'env> Vm<'env> { } Instruction::ListAppend => { a = stack.pop(); + // this intentionally only works with actual sequences if let ValueRepr::Seq(mut v) = stack.pop().0 { Arc::make_mut(&mut v).push(a); stack.push(Value(ValueRepr::Seq(v))) @@ -600,14 +601,17 @@ impl<'env> Vm<'env> { out: &mut Output, ignore_missing: bool, ) -> Result<(), Error> { - let choices = if let ValueRepr::Seq(ref choices) = name.0 { - &choices[..] - } else { - std::slice::from_ref(&name) - }; + use crate::value::SeqObject; + + let single_name_slice = std::slice::from_ref(&name); + let choices = name + .as_seq() + .unwrap_or(&single_name_slice as &dyn SeqObject); + let mut templates_tried = vec![]; - for name in choices { - let name = ok!(name.as_str().ok_or_else(|| { + for idx in 0..choices.seq_len() { + let choice = choices.get_item(idx).unwrap_or(Value::UNDEFINED); + let name = ok!(choice.as_str().ok_or_else(|| { Error::new( ErrorKind::InvalidOperation, "template name was not a string", @@ -617,7 +621,7 @@ impl<'env> Vm<'env> { Ok(tmpl) => tmpl, Err(err) => { if err.kind() == ErrorKind::TemplateNotFound { - templates_tried.push(name); + templates_tried.push(choice); } else { return Err(err); } @@ -652,8 +656,8 @@ impl<'env> Vm<'env> { ) } else { format!( - "tried to include one of multiple templates, none of which existed {:?}", - templates_tried + "tried to include one of multiple templates, none of which existed {}", + Value::from(templates_tried) ) }, )) @@ -814,23 +818,21 @@ impl<'env> Vm<'env> { fn unpack_list(&self, stack: &mut Stack, count: &usize) -> Result<(), Error> { let top = stack.pop(); - let v = ok!(top.as_cow_slice().map_err(|e| Error::new( - ErrorKind::CannotUnpack, - "not a sequence" - ) - .with_source(e))); - if v.len() != *count { + let seq = ok!(top + .as_seq() + .ok_or_else(|| Error::new(ErrorKind::CannotUnpack, "not a sequence"))); + if seq.seq_len() != *count { return Err(Error::new( ErrorKind::CannotUnpack, format!( "sequence of wrong length (expected {}, got {})", *count, - v.len() + seq.seq_len() ), )); } - for value in v.iter().rev() { - stack.push(value.clone()); + for idx in (0..seq.seq_len()).rev() { + stack.push(seq.get_item(idx).unwrap_or(Value::UNDEFINED)); } Ok(()) } diff --git a/minijinja/tests/snapshots/test_templates__vm@err_bad_filter.txt.snap b/minijinja/tests/snapshots/test_templates__vm@err_bad_filter.txt.snap index f93502ea..033c5824 100644 --- a/minijinja/tests/snapshots/test_templates__vm@err_bad_filter.txt.snap +++ b/minijinja/tests/snapshots/test_templates__vm@err_bad_filter.txt.snap @@ -8,12 +8,12 @@ input_file: minijinja/tests/inputs/err_bad_filter.txt Error { kind: InvalidOperation, - detail: "object is not iterable", + detail: "number is not iterable", name: "err_bad_filter.txt", line: 1, } -invalid operation: object is not iterable (in err_bad_filter.txt:1) +invalid operation: number is not iterable (in err_bad_filter.txt:1) ----------------------------- err_bad_filter.txt ------------------------------ 1 > {% for item in 42|slice(4) %} i ^^^^^^^^ invalid operation diff --git a/minijinja/tests/snapshots/test_templates__vm@loop_bad_unpacking.txt.snap b/minijinja/tests/snapshots/test_templates__vm@loop_bad_unpacking.txt.snap index a3aba674..f6eb726a 100644 --- a/minijinja/tests/snapshots/test_templates__vm@loop_bad_unpacking.txt.snap +++ b/minijinja/tests/snapshots/test_templates__vm@loop_bad_unpacking.txt.snap @@ -15,10 +15,6 @@ Error { detail: "not a sequence", name: "loop_bad_unpacking.txt", line: 2, - source: Error { - kind: InvalidOperation, - detail: "value of type number is not a sequence", - }, } cannot unpack: not a sequence (in loop_bad_unpacking.txt:2) @@ -50,5 +46,3 @@ Referenced variables: { } ------------------------------------------------------------------------------- -caused by: invalid operation: value of type number is not a sequence - diff --git a/minijinja/tests/snapshots/test_templates__vm@loop_over_non_iterable.txt.snap b/minijinja/tests/snapshots/test_templates__vm@loop_over_non_iterable.txt.snap index 902a1c62..f42cd053 100644 --- a/minijinja/tests/snapshots/test_templates__vm@loop_over_non_iterable.txt.snap +++ b/minijinja/tests/snapshots/test_templates__vm@loop_over_non_iterable.txt.snap @@ -9,12 +9,12 @@ input_file: minijinja/tests/inputs/loop_over_non_iterable.txt Error { kind: InvalidOperation, - detail: "object is not iterable", + detail: "number is not iterable", name: "loop_over_non_iterable.txt", line: 1, } -invalid operation: object is not iterable (in loop_over_non_iterable.txt:1) +invalid operation: number is not iterable (in loop_over_non_iterable.txt:1) ------------------------- loop_over_non_iterable.txt -------------------------- 1 > [{% for item in seq %}{% endfor %}] ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ diff --git a/minijinja/tests/test_value.rs b/minijinja/tests/test_value.rs index 52221c99..b19c396f 100644 --- a/minijinja/tests/test_value.rs +++ b/minijinja/tests/test_value.rs @@ -3,7 +3,6 @@ use std::fmt; use insta::assert_snapshot; use minijinja::value::{Object, ObjectKind, SeqObject, StructObject, Value}; -use minijinja::ErrorKind; #[test] fn test_sort() { @@ -63,21 +62,6 @@ fn test_float_to_string() { assert_eq!(Value::from(42.0f32).to_string(), "42.0"); } -#[test] -fn test_value_as_slice() { - let val = Value::from(vec![1u32, 2, 3]); - assert_eq!( - val.as_slice().unwrap(), - &[Value::from(1), Value::from(2), Value::from(3)] - ); - assert_eq!(Value::UNDEFINED.as_slice().unwrap(), &[]); - assert_eq!(Value::from(()).as_slice().unwrap(), &[]); - assert_eq!( - Value::from("foo").as_slice().unwrap_err().kind(), - ErrorKind::InvalidOperation - ); -} - #[test] fn test_value_as_bytes() { assert_eq!(Value::from("foo").as_bytes(), Some(&b"foo"[..])); @@ -109,7 +93,7 @@ fn test_map_object_iteration_and_indexing() { } impl StructObject for Point { - fn get(&self, name: &str) -> Option { + fn get_field(&self, name: &str) -> Option { match name { "x" => Some(Value::from(self.0)), "y" => Some(Value::from(self.1)), @@ -158,7 +142,7 @@ fn test_seq_object_iteration_and_indexing() { } impl SeqObject for Point { - fn get(&self, index: usize) -> Option { + fn get_item(&self, index: usize) -> Option { match index { 0 => Some(Value::from(self.0)), 1 => Some(Value::from(self.1)), @@ -167,7 +151,7 @@ fn test_seq_object_iteration_and_indexing() { } } - fn len(&self) -> usize { + fn seq_len(&self) -> usize { 3 } } From dd01f5860d4cdba8caef904173f85bae70b73ccc Mon Sep 17 00:00:00 2001 From: Armin Ronacher Date: Fri, 18 Nov 2022 22:16:53 +0100 Subject: [PATCH 10/16] Add Value::as_struct --- minijinja/src/value/mod.rs | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/minijinja/src/value/mod.rs b/minijinja/src/value/mod.rs index 7ec2b6bc..b9301c6f 100644 --- a/minijinja/src/value/mod.rs +++ b/minijinja/src/value/mod.rs @@ -593,7 +593,7 @@ impl Value { } } - /// If the value is a sequence it's returned as sequence object. + /// If the value is a sequence it's returned as [`SeqObject`]. pub fn as_seq(&self) -> Option<&dyn SeqObject> { match self.0 { ValueRepr::Seq(ref v) => return Some(&**v as &dyn SeqObject), @@ -607,6 +607,16 @@ impl Value { None } + /// If the value is a struct, return it as [`StructObject`]. + pub fn as_struct(&self) -> Option<&dyn StructObject> { + if let ValueRepr::Dynamic(ref dy) = self.0 { + if let ObjectKind::Struct(s) = dy.kind() { + return Some(s); + } + } + None + } + /// If the value is a sequence it's returned as slice. pub(crate) fn as_slice(&self) -> Result<&[Value], Error> { match self.0 { From 5b4e8b857fa5824444beb77c328e8152248cda51 Mon Sep 17 00:00:00 2001 From: Armin Ronacher Date: Fri, 18 Nov 2022 23:36:40 +0100 Subject: [PATCH 11/16] Add an iterator to SeqObject --- minijinja/src/filters.rs | 18 +++----------- minijinja/src/value/argtypes.rs | 1 - minijinja/src/value/mod.rs | 6 ++--- minijinja/src/value/object.rs | 42 +++++++++++++++++++++++++++++++++ minijinja/src/value/ops.rs | 5 ++-- minijinja/src/vm/mod.rs | 7 +++--- 6 files changed, 53 insertions(+), 26 deletions(-) diff --git a/minijinja/src/filters.rs b/minijinja/src/filters.rs index 308f5be3..c45ae6c1 100644 --- a/minijinja/src/filters.rs +++ b/minijinja/src/filters.rs @@ -405,12 +405,7 @@ mod builtins { if let Some(s) = v.as_str() { Ok(Value::from(s.chars().rev().collect::())) } else if let Some(seq) = v.as_seq() { - Ok(Value::from( - (0..seq.seq_len()) - .rev() - .map(|idx| seq.get_item(idx).unwrap_or(Value::UNDEFINED)) - .collect::>(), - )) + Ok(Value::from(seq.iter().rev().collect::>())) } else { Err(Error::new( ErrorKind::InvalidOperation, @@ -451,8 +446,7 @@ mod builtins { Ok(rv) } else if let Some(seq) = val.as_seq() { let mut rv = String::new(); - for idx in 0..seq.seq_len() { - let item = seq.get_item(idx).unwrap_or(Value::UNDEFINED); + for item in seq.iter() { if !rv.is_empty() { rv.push_str(joiner); } @@ -573,13 +567,7 @@ mod builtins { if let Some(s) = value.as_str() { Ok(s.chars().rev().next().map_or(Value::UNDEFINED, Value::from)) } else if let Some(seq) = value.as_seq() { - let len = seq.seq_len(); - Ok(if len == 0 { - None - } else { - seq.get_item(len - 1) - } - .unwrap_or(Value::UNDEFINED)) + Ok(seq.iter().last().unwrap_or(Value::UNDEFINED)) } else { Err(Error::new( ErrorKind::InvalidOperation, diff --git a/minijinja/src/value/argtypes.rs b/minijinja/src/value/argtypes.rs index ba61c144..55446929 100644 --- a/minijinja/src/value/argtypes.rs +++ b/minijinja/src/value/argtypes.rs @@ -582,7 +582,6 @@ impl<'a, T: ArgType<'a, Output = T>> ArgType<'a> for Vec { match value { None => Ok(Vec::new()), Some(values) => { - // TODO: can we somehow use values.as_cow_slice? let values = ok!(values.as_slice()); let mut rv = Vec::new(); for value in values { diff --git a/minijinja/src/value/mod.rs b/minijinja/src/value/mod.rs index b9301c6f..cfcccab7 100644 --- a/minijinja/src/value/mod.rs +++ b/minijinja/src/value/mod.rs @@ -120,7 +120,7 @@ use crate::value::serialize::ValueSerializer; use crate::vm::State; pub use crate::value::argtypes::{from_args, ArgType, FunctionArgs, FunctionResult, Rest}; -pub use crate::value::object::{Object, ObjectKind, SeqObject, StructObject}; +pub use crate::value::object::{Object, ObjectKind, SeqIter, SeqObject, StructObject}; mod argtypes; #[cfg(feature = "deserialization")] @@ -997,8 +997,8 @@ impl Serialize for Value { ObjectKind::Seq(s) => { use serde::ser::SerializeSeq; let mut seq = ok!(serializer.serialize_seq(Some(s.seq_len()))); - for idx in 0..s.seq_len() { - ok!(seq.serialize_element(&s.get_item(idx).unwrap_or(Value::UNDEFINED))); + for item in s.iter() { + ok!(seq.serialize_element(&item)); } seq.end() } diff --git a/minijinja/src/value/object.rs b/minijinja/src/value/object.rs index 2320f342..24639ede 100644 --- a/minijinja/src/value/object.rs +++ b/minijinja/src/value/object.rs @@ -1,5 +1,6 @@ use std::any::Any; use std::fmt; +use std::ops::Range; use crate::error::{Error, ErrorKind}; use crate::value::Value; @@ -174,6 +175,16 @@ pub trait SeqObject { fn seq_len(&self) -> usize; } +impl dyn SeqObject + '_ { + /// Convenient iterator over a [`SeqObject`]. + pub fn iter(&self) -> SeqIter<'_> { + SeqIter { + seq: self, + range: 0..self.seq_len(), + } + } +} + impl<'a> SeqObject for &'a [Value] { #[inline(always)] fn get_item(&self, idx: usize) -> Option { @@ -198,6 +209,37 @@ impl SeqObject for Vec { } } +/// Iterates over [`SeqObject`] +pub struct SeqIter<'a> { + seq: &'a dyn SeqObject, + range: Range, +} + +impl<'a> Iterator for SeqIter<'a> { + type Item = Value; + + fn next(&mut self) -> Option { + self.range + .next() + .map(|idx| self.seq.get_item(idx).unwrap_or(Value::UNDEFINED)) + } + + fn size_hint(&self) -> (usize, Option) { + let len = self.seq.seq_len(); + (len, Some(len)) + } +} + +impl<'a> DoubleEndedIterator for SeqIter<'a> { + fn next_back(&mut self) -> Option { + self.range + .next_back() + .map(|idx| self.seq.get_item(idx).unwrap_or(Value::UNDEFINED)) + } +} + +impl<'a> ExactSizeIterator for SeqIter<'a> {} + /// Views an [`Object`] as a struct. /// /// # Example diff --git a/minijinja/src/value/ops.rs b/minijinja/src/value/ops.rs index 6bf56209..828dffef 100644 --- a/minijinja/src/value/ops.rs +++ b/minijinja/src/value/ops.rs @@ -124,11 +124,10 @@ pub fn slice(value: Value, start: Value, stop: Value, step: Value) -> Result { let (start, len) = get_offset_and_len(start, stop, || seq.seq_len()); Ok(Value::from( - (0..seq.seq_len()) + seq.iter() .skip(start) .take(len) .step_by(step) - .map(|idx| seq.get_item(idx).unwrap_or(Value::UNDEFINED)) .collect::>(), )) } @@ -272,7 +271,7 @@ pub fn contains(container: &Value, value: &Value) -> Result { s.contains(&value.to_string()) } } else if let Some(seq) = container.as_seq() { - (0..seq.seq_len()).any(|idx| seq.get_item(idx).as_ref() == Some(value)) + seq.iter().any(|item| &item == value) } else if let ValueRepr::Map(ref map, _) = container.0 { let key = match value.clone().try_into_key() { Ok(key) => key, diff --git a/minijinja/src/vm/mod.rs b/minijinja/src/vm/mod.rs index c3b1db04..862721a5 100644 --- a/minijinja/src/vm/mod.rs +++ b/minijinja/src/vm/mod.rs @@ -609,8 +609,7 @@ impl<'env> Vm<'env> { .unwrap_or(&single_name_slice as &dyn SeqObject); let mut templates_tried = vec![]; - for idx in 0..choices.seq_len() { - let choice = choices.get_item(idx).unwrap_or(Value::UNDEFINED); + for choice in choices.iter() { let name = ok!(choice.as_str().ok_or_else(|| { Error::new( ErrorKind::InvalidOperation, @@ -831,8 +830,8 @@ impl<'env> Vm<'env> { ), )); } - for idx in (0..seq.seq_len()).rev() { - stack.push(seq.get_item(idx).unwrap_or(Value::UNDEFINED)); + for item in seq.iter().rev() { + stack.push(item); } Ok(()) } From bb105834a6d8535b3e6d5a664ceb749f600d0995 Mon Sep 17 00:00:00 2001 From: Armin Ronacher Date: Fri, 18 Nov 2022 23:37:32 +0100 Subject: [PATCH 12/16] SeqIter -> SeqObjectIter --- minijinja/src/value/mod.rs | 2 +- minijinja/src/value/object.rs | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/minijinja/src/value/mod.rs b/minijinja/src/value/mod.rs index cfcccab7..36ec9cbb 100644 --- a/minijinja/src/value/mod.rs +++ b/minijinja/src/value/mod.rs @@ -120,7 +120,7 @@ use crate::value::serialize::ValueSerializer; use crate::vm::State; pub use crate::value::argtypes::{from_args, ArgType, FunctionArgs, FunctionResult, Rest}; -pub use crate::value::object::{Object, ObjectKind, SeqIter, SeqObject, StructObject}; +pub use crate::value::object::{Object, ObjectKind, SeqObject, SeqObjectIter, StructObject}; mod argtypes; #[cfg(feature = "deserialization")] diff --git a/minijinja/src/value/object.rs b/minijinja/src/value/object.rs index 24639ede..51fbf9d3 100644 --- a/minijinja/src/value/object.rs +++ b/minijinja/src/value/object.rs @@ -177,8 +177,8 @@ pub trait SeqObject { impl dyn SeqObject + '_ { /// Convenient iterator over a [`SeqObject`]. - pub fn iter(&self) -> SeqIter<'_> { - SeqIter { + pub fn iter(&self) -> SeqObjectIter<'_> { + SeqObjectIter { seq: self, range: 0..self.seq_len(), } @@ -210,12 +210,12 @@ impl SeqObject for Vec { } /// Iterates over [`SeqObject`] -pub struct SeqIter<'a> { +pub struct SeqObjectIter<'a> { seq: &'a dyn SeqObject, range: Range, } -impl<'a> Iterator for SeqIter<'a> { +impl<'a> Iterator for SeqObjectIter<'a> { type Item = Value; fn next(&mut self) -> Option { @@ -230,7 +230,7 @@ impl<'a> Iterator for SeqIter<'a> { } } -impl<'a> DoubleEndedIterator for SeqIter<'a> { +impl<'a> DoubleEndedIterator for SeqObjectIter<'a> { fn next_back(&mut self) -> Option { self.range .next_back() @@ -238,7 +238,7 @@ impl<'a> DoubleEndedIterator for SeqIter<'a> { } } -impl<'a> ExactSizeIterator for SeqIter<'a> {} +impl<'a> ExactSizeIterator for SeqObjectIter<'a> {} /// Views an [`Object`] as a struct. /// From d7553d87a4e882abcdf9cdcd1d63bd3aa201c94e Mon Sep 17 00:00:00 2001 From: Armin Ronacher Date: Fri, 18 Nov 2022 23:39:07 +0100 Subject: [PATCH 13/16] Fix size hint on iterator --- minijinja/src/value/object.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/minijinja/src/value/object.rs b/minijinja/src/value/object.rs index 51fbf9d3..fc4bc476 100644 --- a/minijinja/src/value/object.rs +++ b/minijinja/src/value/object.rs @@ -218,19 +218,21 @@ pub struct SeqObjectIter<'a> { impl<'a> Iterator for SeqObjectIter<'a> { type Item = Value; + #[inline(always)] fn next(&mut self) -> Option { self.range .next() .map(|idx| self.seq.get_item(idx).unwrap_or(Value::UNDEFINED)) } + #[inline(always)] fn size_hint(&self) -> (usize, Option) { - let len = self.seq.seq_len(); - (len, Some(len)) + self.range.size_hint() } } impl<'a> DoubleEndedIterator for SeqObjectIter<'a> { + #[inline(always)] fn next_back(&mut self) -> Option { self.range .next_back() From f913ef30ad58a1b3f3969a9e26736e7ebf4efdd0 Mon Sep 17 00:00:00 2001 From: Armin Ronacher Date: Sat, 19 Nov 2022 10:10:59 +0100 Subject: [PATCH 14/16] Better names for counts on maps and sequences --- minijinja/src/value/mod.rs | 21 +++++++++++---------- minijinja/src/value/object.rs | 14 +++++++------- minijinja/src/value/ops.rs | 2 +- minijinja/src/vm/mod.rs | 4 ++-- minijinja/tests/test_value.rs | 2 +- 5 files changed, 22 insertions(+), 21 deletions(-) diff --git a/minijinja/src/value/mod.rs b/minijinja/src/value/mod.rs index 36ec9cbb..865874d8 100644 --- a/minijinja/src/value/mod.rs +++ b/minijinja/src/value/mod.rs @@ -555,8 +555,8 @@ impl Value { ValueRepr::Map(ref x, _) => !x.is_empty(), ValueRepr::Dynamic(ref x) => match x.kind() { ObjectKind::Basic => true, - ObjectKind::Seq(s) => s.seq_len() != 0, - ObjectKind::Struct(s) => s.struct_size() != 0, + ObjectKind::Seq(s) => s.item_count() != 0, + ObjectKind::Struct(s) => s.field_count() != 0, }, } } @@ -652,8 +652,8 @@ impl Value { ValueRepr::Seq(ref items) => Some(items.len()), ValueRepr::Dynamic(ref dy) => match dy.kind() { ObjectKind::Basic => None, - ObjectKind::Seq(s) => Some(s.seq_len()), - ObjectKind::Struct(s) => Some(s.struct_size()), + ObjectKind::Seq(s) => Some(s.item_count()), + ObjectKind::Struct(s) => Some(s.field_count()), }, _ => None, } @@ -821,7 +821,7 @@ impl Value { if let Key::I64(idx) = key { let idx = some!(isize::try_from(idx).ok()); let idx = if idx < 0 { - some!(seq.seq_len().checked_sub(-idx as usize)) + some!(seq.item_count().checked_sub(-idx as usize)) } else { idx as usize }; @@ -932,15 +932,16 @@ impl Value { ValueRepr::Dynamic(ref obj) => { match obj.kind() { ObjectKind::Basic => (ValueIteratorState::Empty, 0), - ObjectKind::Seq(s) => { - (ValueIteratorState::DynSeq(0, Arc::clone(obj)), s.seq_len()) - } + ObjectKind::Seq(s) => ( + ValueIteratorState::DynSeq(0, Arc::clone(obj)), + s.item_count(), + ), ObjectKind::Struct(s) => { // the assumption is that structs don't have excessive field counts // and that most iterations go over all fields, so creating a // temporary vector here is acceptable. let attrs = s.fields().map(Value::from).collect::>(); - let attr_count = s.struct_size(); + let attr_count = s.field_count(); (ValueIteratorState::Seq(0, Arc::new(attrs)), attr_count) } } @@ -996,7 +997,7 @@ impl Serialize for Value { ObjectKind::Basic => serializer.serialize_str(&dy.to_string()), ObjectKind::Seq(s) => { use serde::ser::SerializeSeq; - let mut seq = ok!(serializer.serialize_seq(Some(s.seq_len()))); + let mut seq = ok!(serializer.serialize_seq(Some(s.item_count()))); for item in s.iter() { ok!(seq.serialize_element(&item)); } diff --git a/minijinja/src/value/object.rs b/minijinja/src/value/object.rs index fc4bc476..1a788887 100644 --- a/minijinja/src/value/object.rs +++ b/minijinja/src/value/object.rs @@ -156,7 +156,7 @@ pub enum ObjectKind<'a> { /// } /// } /// -/// fn seq_len(&self) -> usize { +/// fn item_count(&self) -> usize { /// 3 /// } /// } @@ -166,13 +166,13 @@ pub enum ObjectKind<'a> { pub trait SeqObject { /// Looks up an item by index. /// - /// Sequences should provide a value for all items in the range of `0..seq_len` + /// Sequences should provide a value for all items in the range of `0..item_count` /// but the engine will assume that items within the range are `Undefined` /// if `None` is returned. fn get_item(&self, idx: usize) -> Option; /// Returns the number of items in the sequence. - fn seq_len(&self) -> usize; + fn item_count(&self) -> usize; } impl dyn SeqObject + '_ { @@ -180,7 +180,7 @@ impl dyn SeqObject + '_ { pub fn iter(&self) -> SeqObjectIter<'_> { SeqObjectIter { seq: self, - range: 0..self.seq_len(), + range: 0..self.item_count(), } } } @@ -192,7 +192,7 @@ impl<'a> SeqObject for &'a [Value] { } #[inline(always)] - fn seq_len(&self) -> usize { + fn item_count(&self) -> usize { self.len() } } @@ -204,7 +204,7 @@ impl SeqObject for Vec { } #[inline(always)] - fn seq_len(&self) -> usize { + fn item_count(&self) -> usize { self.len() } } @@ -311,7 +311,7 @@ pub trait StructObject { /// Returns the number of fields in the struct. /// /// The default implementation returns the number of fields. - fn struct_size(&self) -> usize { + fn field_count(&self) -> usize { self.fields().count() } } diff --git a/minijinja/src/value/ops.rs b/minijinja/src/value/ops.rs index 828dffef..fa320969 100644 --- a/minijinja/src/value/ops.rs +++ b/minijinja/src/value/ops.rs @@ -122,7 +122,7 @@ pub fn slice(value: Value, start: Value, stop: Value, step: Value) -> Result { - let (start, len) = get_offset_and_len(start, stop, || seq.seq_len()); + let (start, len) = get_offset_and_len(start, stop, || seq.item_count()); Ok(Value::from( seq.iter() .skip(start) diff --git a/minijinja/src/vm/mod.rs b/minijinja/src/vm/mod.rs index 862721a5..fd7e3c26 100644 --- a/minijinja/src/vm/mod.rs +++ b/minijinja/src/vm/mod.rs @@ -820,13 +820,13 @@ impl<'env> Vm<'env> { let seq = ok!(top .as_seq() .ok_or_else(|| Error::new(ErrorKind::CannotUnpack, "not a sequence"))); - if seq.seq_len() != *count { + if seq.item_count() != *count { return Err(Error::new( ErrorKind::CannotUnpack, format!( "sequence of wrong length (expected {}, got {})", *count, - seq.seq_len() + seq.item_count() ), )); } diff --git a/minijinja/tests/test_value.rs b/minijinja/tests/test_value.rs index b19c396f..78f95c95 100644 --- a/minijinja/tests/test_value.rs +++ b/minijinja/tests/test_value.rs @@ -151,7 +151,7 @@ fn test_seq_object_iteration_and_indexing() { } } - fn seq_len(&self) -> usize { + fn item_count(&self) -> usize { 3 } } From 0e56a7207af0f3938060f4e68fdb5fe4d4291c26 Mon Sep 17 00:00:00 2001 From: Armin Ronacher Date: Sat, 19 Nov 2022 11:00:06 +0100 Subject: [PATCH 15/16] Added simplified object creation API for sequences and structs --- examples/dynamic/src/main.rs | 19 +++- examples/dynamic/src/template.html | 6 +- minijinja/src/value/mod.rs | 51 ++++++--- minijinja/src/value/object.rs | 164 ++++++++++++++++++++++++++--- 4 files changed, 211 insertions(+), 29 deletions(-) diff --git a/examples/dynamic/src/main.rs b/examples/dynamic/src/main.rs index 9c1431ba..253e6adb 100644 --- a/examples/dynamic/src/main.rs +++ b/examples/dynamic/src/main.rs @@ -2,7 +2,7 @@ use std::fmt; use std::sync::atomic::{AtomicUsize, Ordering}; -use minijinja::value::{from_args, Object, Value}; +use minijinja::value::{from_args, Object, SeqObject, Value}; use minijinja::{Environment, Error, State}; #[derive(Debug)] @@ -57,10 +57,27 @@ impl Object for Magic { } } +struct SimpleDynamicSeq; + +impl SeqObject for SimpleDynamicSeq { + fn get_item(&self, idx: usize) -> Option { + if idx < 3 { + Some(Value::from(idx * 2)) + } else { + None + } + } + + fn item_count(&self) -> usize { + 3 + } +} + fn main() { let mut env = Environment::new(); env.add_function("cycler", make_cycler); env.add_global("magic", Value::from_object(Magic)); + env.add_global("seq", Value::from_seq_object(SimpleDynamicSeq)); env.add_template("template.html", include_str!("template.html")) .unwrap(); diff --git a/examples/dynamic/src/template.html b/examples/dynamic/src/template.html index 0e702f9d..9b33b1ab 100644 --- a/examples/dynamic/src/template.html +++ b/examples/dynamic/src/template.html @@ -4,4 +4,8 @@
  • {{ char }}
    • {%- endfor %} -{%- endwith %} \ No newline at end of file +{%- endwith %} + +{% for item in seq %} + [{{ item }}] +{% endfor %} \ No newline at end of file diff --git a/minijinja/src/value/mod.rs b/minijinja/src/value/mod.rs index 865874d8..74afad43 100644 --- a/minijinja/src/value/mod.rs +++ b/minijinja/src/value/mod.rs @@ -70,12 +70,14 @@ //! # Dynamic Objects //! //! Values can also hold "dynamic" objects. These are objects which implement the -//! [`Object`] trait. These can be used to implement dynamic functionality such as -//! stateful values and more. Dynamic objects are internally also used to implement -//! the special `loop` variable or macros. +//! [`Object`] trait and optionally [`SeqObject`] or [`StructObject`] These can +//! be used to implement dynamic functionality such as stateful values and more. +//! Dynamic objects are internally also used to implement the special `loop` +//! variable or macros. //! -//! To create a dynamic `Value` object, use [`Value::from_object()`] or the -//! `From>` implementations for `Value`: +//! To create a dynamic `Value` object, use [`Value::from_object`], +//! [`Value::from_seq_object`], [`Value::from_struct_object`] or the `From>` implementations for `Value`: //! //! ```rust //! # use std::sync::Arc; @@ -116,6 +118,7 @@ use crate::error::{Error, ErrorKind}; use crate::functions; use crate::key::{Key, StaticKey}; use crate::utils::OnDrop; +use crate::value::object::{SimpleSeqObject, SimpleStructObject}; use crate::value::serialize::ValueSerializer; use crate::vm::State; @@ -490,6 +493,28 @@ impl Value { Value::from(Arc::new(value) as Arc) } + /// Creates a value from an owned [`SeqObject`]. + /// + /// This is a simplified API for creating dynamic sequences + /// without having to implement the entire [`Object`] protocol. + /// + /// **Note:** objects created this way cannot be downcasted via + /// [`downcast_object_ref`](Self::downcast_object_ref). + pub fn from_seq_object(value: T) -> Value { + Value::from_object(SimpleSeqObject(value)) + } + + /// Creates a value from an owned [`StructObject`]. + /// + /// This is a simplified API for creating dynamic structs + /// without having to implement the entire [`Object`] protocol. + /// + /// **Note:** objects created this way cannot be downcasted via + /// [`downcast_object_ref`](Self::downcast_object_ref). + pub fn from_struct_object(value: T) -> Value { + Value::from_object(SimpleStructObject(value)) + } + /// Creates a callable value from a function. /// /// ``` @@ -526,7 +551,7 @@ impl Value { ValueRepr::Map(..) => ValueKind::Map, ValueRepr::Dynamic(ref dy) => match dy.kind() { // XXX: basic objects should probably not report as map - ObjectKind::Basic => ValueKind::Map, + ObjectKind::Plain => ValueKind::Map, ObjectKind::Seq(_) => ValueKind::Seq, ObjectKind::Struct(_) => ValueKind::Map, }, @@ -554,7 +579,7 @@ impl Value { ValueRepr::Seq(ref x) => !x.is_empty(), ValueRepr::Map(ref x, _) => !x.is_empty(), ValueRepr::Dynamic(ref x) => match x.kind() { - ObjectKind::Basic => true, + ObjectKind::Plain => true, ObjectKind::Seq(s) => s.item_count() != 0, ObjectKind::Struct(s) => s.field_count() != 0, }, @@ -651,7 +676,7 @@ impl Value { ValueRepr::Map(ref items, _) => Some(items.len()), ValueRepr::Seq(ref items) => Some(items.len()), ValueRepr::Dynamic(ref dy) => match dy.kind() { - ObjectKind::Basic => None, + ObjectKind::Plain => None, ObjectKind::Seq(s) => Some(s.item_count()), ObjectKind::Struct(s) => Some(s.field_count()), }, @@ -682,7 +707,7 @@ impl Value { items.get(&lookup_key).cloned() } ValueRepr::Dynamic(ref dy) => match dy.kind() { - ObjectKind::Basic | ObjectKind::Seq(_) => None, + ObjectKind::Plain | ObjectKind::Seq(_) => None, ObjectKind::Struct(s) => s.get_field(key), }, ValueRepr::Undefined => { @@ -807,7 +832,7 @@ impl Value { ValueRepr::Map(ref items, _) => return items.get(&key).cloned(), ValueRepr::Seq(ref items) => &**items as &dyn SeqObject, ValueRepr::Dynamic(ref dy) => match dy.kind() { - ObjectKind::Basic => return None, + ObjectKind::Plain => return None, ObjectKind::Seq(s) => s, ObjectKind::Struct(s) => match key { Key::String(ref key) => return s.get_field(key), @@ -900,7 +925,7 @@ impl Value { .filter_map(|(k, v)| k.as_str().map(move |k| (k, v.clone()))), ) as Box>, ValueRepr::Dynamic(ref obj) => match obj.kind() { - ObjectKind::Basic | ObjectKind::Seq(_) => { + ObjectKind::Plain | ObjectKind::Seq(_) => { Box::new(None.into_iter()) as Box> } ObjectKind::Struct(s) => Box::new( @@ -931,7 +956,7 @@ impl Value { ), ValueRepr::Dynamic(ref obj) => { match obj.kind() { - ObjectKind::Basic => (ValueIteratorState::Empty, 0), + ObjectKind::Plain => (ValueIteratorState::Empty, 0), ObjectKind::Seq(s) => ( ValueIteratorState::DynSeq(0, Arc::clone(obj)), s.item_count(), @@ -994,7 +1019,7 @@ impl Serialize for Value { map.end() } ValueRepr::Dynamic(ref dy) => match dy.kind() { - ObjectKind::Basic => serializer.serialize_str(&dy.to_string()), + ObjectKind::Plain => serializer.serialize_str(&dy.to_string()), ObjectKind::Seq(s) => { use serde::ser::SerializeSeq; let mut seq = ok!(serializer.serialize_seq(Some(s.item_count()))); diff --git a/minijinja/src/value/object.rs b/minijinja/src/value/object.rs index 1a788887..272e28ee 100644 --- a/minijinja/src/value/object.rs +++ b/minijinja/src/value/object.rs @@ -33,13 +33,13 @@ use crate::vm::State; pub trait Object: fmt::Display + fmt::Debug + Any + Sync + Send { /// Describes the kind of an object. /// - /// If not implemented behavior for an object is [`ObjectKind::Basic`] + /// If not implemented behavior for an object is [`ObjectKind::Plain`] /// which just means that it's stringifyable and potentially can be /// called or has methods. /// /// For more information see [`ObjectKind`]. fn kind(&self) -> ObjectKind<'_> { - ObjectKind::Basic + ObjectKind::Plain } /// Called when the engine tries to call a method on the object. @@ -92,7 +92,7 @@ impl Object for std::sync::Arc { /// A kind defines the object's behavior. /// /// When a dynamic [`Object`] is implemented, it can be of one of the kinds -/// here. The default behavior will be a [`Basic`](Self::Basic) object which +/// here. The default behavior will be a [`Plain`](Self::Plain) object which /// doesn't do much other than that it can be printed. For an object to turn /// into a [struct](Self::Struct) or [sequence](Self::Seq) the necessary kind /// has to be returned with a pointer to itself. @@ -102,12 +102,12 @@ impl Object for std::sync::Arc { /// be represented by objects. #[non_exhaustive] pub enum ObjectKind<'a> { - /// This object is a basic object. + /// This object is a plain object. /// /// Such an object has no attributes but it might be callable and it /// can be stringified. When serialized it's serialized in it's /// stringified form. - Basic, + Plain, /// This object is a sequence. /// @@ -120,12 +120,48 @@ pub enum ObjectKind<'a> { Struct(&'a dyn StructObject), } -/// Views an [`Object`] as sequence of values. +/// Provides the behavior of an [`Object`] holding sequence of values. /// -/// # Example +/// An object holding a sequence of values (tuple, list etc.) can be +/// represented by this trait. /// -/// The following example shows how to implement a dynamic object which -/// represents a sequence of three items: +/// # Simplified Example +/// +/// For sequences which do not need any special method behavior, the [`Value`] +/// type is capable of automatically constructing a wrapper [`Object`] by using +/// [`Value::from_seq_object`]. In that case only [`SeqObject`] needs to be +/// implemented and the value will provide default implementations for +/// stringification and debug printing. +/// +/// ``` +/// use minijinja::value::{Value, SeqObject}; +/// +/// struct Point(f32, f32, f32); +/// +/// impl SeqObject for Point { +/// fn get_item(&self, idx: usize) -> Option { +/// match idx { +/// 0 => Some(Value::from(self.0)), +/// 1 => Some(Value::from(self.1)), +/// 2 => Some(Value::from(self.2)), +/// _ => None, +/// } +/// } +/// +/// fn item_count(&self) -> usize { +/// 3 +/// } +/// } +/// +/// let value = Value::from_seq_object(Point(1.0, 2.5, 3.0)); +/// ``` +/// +/// # Full Example +/// +/// This example shows how one can use [`SeqObject`] in conjunction +/// with a fully customized [`Object`]. Note that in this case not +/// only [`Object`] needs to be implemented, but also [`Debug`] and +/// [`Display`](std::fmt::Display) no longer come for free. /// /// ``` /// use std::fmt; @@ -163,7 +199,7 @@ pub enum ObjectKind<'a> { /// /// let value = Value::from_object(Point(1.0, 2.5, 3.0)); /// ``` -pub trait SeqObject { +pub trait SeqObject: Send + Sync { /// Looks up an item by index. /// /// Sequences should provide a value for all items in the range of `0..item_count` @@ -242,12 +278,48 @@ impl<'a> DoubleEndedIterator for SeqObjectIter<'a> { impl<'a> ExactSizeIterator for SeqObjectIter<'a> {} -/// Views an [`Object`] as a struct. +/// Provides the behavior of an [`Object`] holding a struct. +/// +/// An basic object with the shape and behavior of a struct (that means a +/// map with string keys) can be represented by this trait. +/// +/// # Simplified Example +/// +/// For structs which do not need any special method behavior or methods, the +/// [`Value`] type is capable of automatically constructing a wrapper [`Object`] +/// by using [`Value::from_struct_object`]. In that case only [`StructObject`] +/// needs to be implemented and the value will provide default implementations +/// for stringification and debug printing. +/// +/// ``` +/// use minijinja::value::{Value, StructObject}; +/// +/// struct Point(f32, f32, f32); +/// +/// impl StructObject for Point { +/// fn get_field(&self, name: &str) -> Option { +/// match name { +/// "x" => Some(Value::from(self.0)), +/// "y" => Some(Value::from(self.1)), +/// "z" => Some(Value::from(self.2)), +/// _ => None, +/// } +/// } /// -/// # Example +/// fn fields(&self) -> Box + '_> { +/// Box::new(["x", "y", "z"].into_iter()) +/// } +/// } +/// +/// let value = Value::from_struct_object(Point(1.0, 2.5, 3.0)); +/// ``` +/// +/// # Full Example /// /// The following example shows how to implement a dynamic object which -/// represents a struct: +/// represents a struct. Note that in this case not only [`Object`] needs to be +/// implemented, but also [`Debug`] and [`Display`](std::fmt::Display) no longer +/// come for free. /// /// ``` /// use std::fmt; @@ -285,7 +357,7 @@ impl<'a> ExactSizeIterator for SeqObjectIter<'a> {} /// /// let value = Value::from_object(Point(1.0, 2.5, 3.0)); /// ``` -pub trait StructObject { +pub trait StructObject: Send + Sync { /// Invoked by the engine to get a field of a struct. /// /// Where possible it's a good idea for this to align with the return value @@ -315,3 +387,67 @@ pub trait StructObject { self.fields().count() } } + +#[repr(transparent)] +pub struct SimpleSeqObject(pub T); + +impl fmt::Display for SimpleSeqObject { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + ok!(write!(f, "[")); + for (idx, val) in (&self.0 as &dyn SeqObject).iter().enumerate() { + if idx > 0 { + ok!(write!(f, ", ")); + } + ok!(write!(f, "{:?}", val)); + } + write!(f, "]") + } +} + +impl fmt::Debug for SimpleSeqObject { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_list() + .entries((&self.0 as &dyn SeqObject).iter()) + .finish() + } +} + +impl Object for SimpleSeqObject { + fn kind(&self) -> ObjectKind<'_> { + ObjectKind::Seq(&self.0) + } +} + +#[repr(transparent)] +pub struct SimpleStructObject(pub T); + +impl fmt::Display for SimpleStructObject { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + ok!(write!(f, "[")); + for (idx, field) in self.0.fields().enumerate() { + if idx > 0 { + ok!(write!(f, ", ")); + } + let val = self.0.get_field(field).unwrap_or(Value::UNDEFINED); + ok!(write!(f, "{:?}: {:?}", field, val)); + } + write!(f, "]") + } +} + +impl fmt::Debug for SimpleStructObject { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + let mut m = f.debug_map(); + for field in self.0.fields() { + let value = self.0.get_field(field).unwrap_or(Value::UNDEFINED); + m.entry(&field, &value); + } + m.finish() + } +} + +impl Object for SimpleStructObject { + fn kind(&self) -> ObjectKind<'_> { + ObjectKind::Struct(&self.0) + } +} From 53c412bab6a506a87562ffab2673b92fc900635e Mon Sep 17 00:00:00 2001 From: Armin Ronacher Date: Sat, 19 Nov 2022 13:09:49 +0100 Subject: [PATCH 16/16] Restore vector arg conversions through new internal API --- minijinja/src/filters.rs | 30 +++++++++ minijinja/src/value/argtypes.rs | 109 +++++++++++++++++++++++--------- minijinja/src/value/mod.rs | 34 ---------- 3 files changed, 108 insertions(+), 65 deletions(-) diff --git a/minijinja/src/filters.rs b/minijinja/src/filters.rs index c45ae6c1..4c2f7235 100644 --- a/minijinja/src/filters.rs +++ b/minijinja/src/filters.rs @@ -869,6 +869,36 @@ mod builtins { ); }); } + + #[test] + fn test_values_in_vec() { + fn upper(value: &str) -> String { + value.to_uppercase() + } + + fn sum(value: Vec) -> i64 { + value.into_iter().sum::() + } + + let upper = BoxedFilter::new(upper); + let sum = BoxedFilter::new(sum); + + let env = crate::Environment::new(); + State::with_dummy(&env, |state| { + assert_eq!( + upper + .apply_to(state, &[Value::from("Hello World!")]) + .unwrap(), + Value::from("HELLO WORLD!") + ); + + assert_eq!( + sum.apply_to(state, &[Value::from(vec![Value::from(1), Value::from(2)])]) + .unwrap(), + Value::from(3) + ); + }); + } } #[cfg(feature = "builtins")] diff --git a/minijinja/src/value/argtypes.rs b/minijinja/src/value/argtypes.rs index 55446929..08b99c05 100644 --- a/minijinja/src/value/argtypes.rs +++ b/minijinja/src/value/argtypes.rs @@ -5,7 +5,9 @@ use std::ops::{Deref, DerefMut}; use crate::error::{Error, ErrorKind}; use crate::key::{Key, StaticKey}; -use crate::value::{Arc, MapType, Object, Packed, StringType, Value, ValueKind, ValueRepr}; +use crate::value::{ + Arc, MapType, Object, Packed, SeqObject, StringType, Value, ValueKind, ValueRepr, +}; use crate::vm::State; /// A utility trait that represents the return value of functions and filters. @@ -90,10 +92,11 @@ where /// * signed integers: [`i8`], [`i16`], [`i32`], [`i64`], [`i128`] /// * floats: [`f32`], [`f64`] /// * bool: [`bool`] -/// * string: [`String`], [`&str`], `Cow<'_, str>` ([`char`]) +/// * string: [`String`], [`&str`], `Cow<'_, str>`, [`char`] /// * bytes: [`&[u8]`][`slice`] /// * values: [`Value`], `&Value` -/// * vectors: [`Vec`], `&[Value]` +/// * vectors: [`Vec`] +/// * sequences: [`&dyn SeqObject`](crate::value::SeqObject) /// /// The type is also implemented for optional values (`Option`) which is used /// to encode optional parameters to filters, functions or tests. Additionally @@ -111,8 +114,9 @@ where /// Byte slices will borrow out of values carrying bytes or strings. In the latter /// case the utf-8 bytes are returned. /// -/// Similarly it's not possible to borrow dynamic slices -/// ([`SeqObject`](crate::value::SeqObject)) into `&[Value]`. +/// There are also further restrictions imposed on borrowing in some situations. +/// For instance you cannot implicitly borrow out of sequences which means that +/// for instance `Vec<&str>` is not a legal argument. /// /// ## Notes on State /// @@ -126,6 +130,14 @@ pub trait ArgType<'a> { #[doc(hidden)] fn from_value(value: Option<&'a Value>) -> Result; + #[doc(hidden)] + fn from_value_owned(_value: Value) -> Result { + Err(Error::new( + ErrorKind::InvalidOperation, + "type conversion is not legal in this situation (implicit borrow)", + )) + } + #[doc(hidden)] fn from_state_and_value( _state: Option<&'a State>, @@ -354,6 +366,10 @@ macro_rules! primitive_try_from { None => Err(Error::from(ErrorKind::MissingArgument)) } } + + fn from_value_owned(value: Value) -> Result { + TryFrom::try_from(value) + } } } } @@ -425,6 +441,20 @@ impl<'a> ArgType<'a> for &[u8] { } } +impl<'a> ArgType<'a> for &dyn SeqObject { + type Output = &'a dyn SeqObject; + + #[inline(always)] + fn from_value(value: Option<&'a Value>) -> Result { + match value { + Some(value) => value + .as_seq() + .ok_or_else(|| Error::new(ErrorKind::InvalidOperation, "value is not a sequence")), + None => Err(Error::from(ErrorKind::MissingArgument)), + } + } +} + impl<'a, T: ArgType<'a>> ArgType<'a> for Option { type Output = Option; @@ -440,6 +470,14 @@ impl<'a, T: ArgType<'a>> ArgType<'a> for Option { None => Ok(None), } } + + fn from_value_owned(value: Value) -> Result { + if value.is_undefined() || value.is_none() { + Ok(None) + } else { + T::from_value_owned(value).map(Some) + } + } } impl<'a> ArgType<'a> for Cow<'_, str> { @@ -466,18 +504,6 @@ impl<'a> ArgType<'a> for &Value { } } -impl<'a> ArgType<'a> for &[Value] { - type Output = &'a [Value]; - - #[inline(always)] - fn from_value(value: Option<&'a Value>) -> Result<&'a [Value], Error> { - match value { - Some(value) => value.as_slice(), - None => Err(Error::from(ErrorKind::MissingArgument)), - } - } -} - /// Utility type to capture remaining arguments. /// /// In some cases you might want to have a variadic function. In that case @@ -550,6 +576,10 @@ impl<'a> ArgType<'a> for Value { None => Err(Error::from(ErrorKind::MissingArgument)), } } + + fn from_value_owned(value: Value) -> Result { + Ok(value) + } } impl<'a> ArgType<'a> for String { @@ -561,17 +591,9 @@ impl<'a> ArgType<'a> for String { None => Err(Error::from(ErrorKind::MissingArgument)), } } -} - -impl From for String { - fn from(val: Value) -> Self { - val.to_string() - } -} -impl From for Value { - fn from(val: usize) -> Self { - Value::from(val as u64) + fn from_value_owned(value: Value) -> Result { + Ok(value.to_string()) } } @@ -581,14 +603,39 @@ impl<'a, T: ArgType<'a, Output = T>> ArgType<'a> for Vec { fn from_value(value: Option<&'a Value>) -> Result { match value { None => Ok(Vec::new()), - Some(values) => { - let values = ok!(values.as_slice()); + Some(value) => { + let seq = ok!(value + .as_seq() + .ok_or_else(|| { Error::new(ErrorKind::InvalidOperation, "not a sequence") })); let mut rv = Vec::new(); - for value in values { - rv.push(ok!(T::from_value(Some(value)))); + for value in seq.iter() { + rv.push(ok!(T::from_value_owned(value))); } Ok(rv) } } } + + fn from_value_owned(value: Value) -> Result { + let seq = ok!(value + .as_seq() + .ok_or_else(|| { Error::new(ErrorKind::InvalidOperation, "not a sequence") })); + let mut rv = Vec::new(); + for value in seq.iter() { + rv.push(ok!(T::from_value_owned(value))); + } + Ok(rv) + } +} + +impl From for String { + fn from(val: Value) -> Self { + val.to_string() + } +} + +impl From for Value { + fn from(val: usize) -> Self { + Value::from(val as u64) + } } diff --git a/minijinja/src/value/mod.rs b/minijinja/src/value/mod.rs index 74afad43..f6b0e6b4 100644 --- a/minijinja/src/value/mod.rs +++ b/minijinja/src/value/mod.rs @@ -642,25 +642,6 @@ impl Value { None } - /// If the value is a sequence it's returned as slice. - pub(crate) fn as_slice(&self) -> Result<&[Value], Error> { - match self.0 { - ValueRepr::Undefined | ValueRepr::None => return Ok(&[][..]), - ValueRepr::Seq(ref v) => return Ok(&v[..]), - ValueRepr::Dynamic(ref dy) if matches!(dy.kind(), ObjectKind::Seq(_)) => { - return Err(Error::new( - ErrorKind::InvalidOperation, - "dynamic sequence value cannot be borrowed as slice", - )); - } - _ => {} - } - Err(Error::new( - ErrorKind::InvalidOperation, - format!("value of type {} is not a sequence", self.kind()), - )) - } - /// Returns the length of the contained value. /// /// Values without a length will return `None`. @@ -1181,18 +1162,3 @@ fn test_dynamic_object_roundtrip() { fn test_sizes() { assert_eq!(std::mem::size_of::(), 24); } - -#[test] -fn test_value_as_slice() { - let val = Value::from(vec![1u32, 2, 3]); - assert_eq!( - val.as_slice().unwrap(), - &[Value::from(1), Value::from(2), Value::from(3)] - ); - assert_eq!(Value::UNDEFINED.as_slice().unwrap(), &[]); - assert_eq!(Value::from(()).as_slice().unwrap(), &[]); - assert_eq!( - Value::from("foo").as_slice().unwrap_err().kind(), - ErrorKind::InvalidOperation - ); -}