Merge pull request #233 from epage/value

Refactor API
cobalt-org · Nov 20, 2018 · 969b6c8 · 969b6c8
2 parents 32c605b + 662b9c6
commit 969b6c8
Show file tree

Hide file tree

Showing 19 changed files with 573 additions and 288 deletions.
diff --git a/.travis.yml b/.travis.yml
@@ -19,11 +19,11 @@ matrix:
     script:
     - cargo check --tests
   - env: CLIPPY
-    rust: nightly-2018-07-17
+    rust: nightly-2018-11-10
     install:
       - rustup component add clippy-preview
     script:
-      - cargo clippy --all-features -- -D clippy
+      - cargo clippy --all-features
 
 env:
   global:

diff --git a/benches/liquid.rs b/benches/liquid.rs
@@ -30,10 +30,10 @@ static VARIABLE_ONLY: &'static str = "{{product.name}}";
 static VARIABLE_ONLY_OBJECT: &'static str = "
 username: bob
 product:
-  - name: Moto G
-  - manufacturer: Motorola
-  - summary: A phone
-  - price: 100
+  name: Moto G
+  manufacturer: Motorola
+  summary: A phone
+  price: 100
 ";
 
 #[bench]
@@ -67,7 +67,7 @@ static ITERATE: &'static str = "<html>
       <li class=\"champion\">
       <b>{{team.name}}</b>: {{team.score}}
       </li>
-    {{/each}}
+    {% endfor %}
     </ul>
   </body>
 </html>";

diff --git a/liquid-compiler/src/parser.rs b/liquid-compiler/src/parser.rs
@@ -4,7 +4,6 @@
 //! but should be ignored for simple usage.
 
 use std::collections::HashSet;
-use std::iter::FromIterator;
 use std::slice::Iter;
 
 use liquid_interpreter::Expression;
@@ -308,7 +307,7 @@ pub fn split_block<'a>(
 ) -> (&'a [Element], Option<BlockSplit<'a>>) {
     // construct a fast-lookup cache of the delimiters, as we're going to be
     // consulting the delimiter list a *lot*.
-    let delims: HashSet<&str> = HashSet::from_iter(delimiters.iter().map(|x| *x));
+    let delims: HashSet<&str> = delimiters.iter().cloned().collect();
     let mut stack: Vec<String> = Vec::new();
 
     for (i, t) in tokens.iter().enumerate() {

diff --git a/liquid-compiler/src/token.rs b/liquid-compiler/src/token.rs
@@ -2,6 +2,7 @@ use std::fmt;
 
 use liquid_interpreter::Expression;
 use liquid_interpreter::Variable;
+use liquid_value::Scalar;
 use liquid_value::Value;
 
 use super::error::Result;
@@ -79,11 +80,11 @@ impl Token {
             Token::StringLiteral(ref s) => Ok(Expression::with_literal(s.to_owned())),
             Token::BooleanLiteral(b) => Ok(Expression::with_literal(b)),
             Token::Identifier(ref id) => {
-                let mut var = Variable::default();
-                var.extend(
-                    id.split('.')
-                        .map(|s| Expression::with_literal(s.to_owned())),
+                let mut path = id.split('.').map(|s| Scalar::new(s.to_owned()));
+                let mut var = Variable::with_literal(
+                    path.next().expect("there should always be at least one"),
                 );
+                var.extend(path);
                 Ok(Expression::Variable(var))
             }
             ref x => Err(unexpected_token_error(
@@ -132,7 +133,7 @@ mod test {
         let ctx = Context::new();
         let t = Token::StringLiteral("hello".to_owned());
         assert_eq!(
-            t.to_arg().unwrap().evaluate(&ctx).unwrap(),
+            *t.to_arg().unwrap().evaluate(&ctx).unwrap(),
             Value::scalar("hello")
         );
     }
@@ -141,7 +142,7 @@ mod test {
     fn evaluate_handles_number_literals() {
         let ctx = Context::new();
         assert_eq!(
-            Token::FloatLiteral(42f64)
+            *Token::FloatLiteral(42f64)
                 .to_arg()
                 .unwrap()
                 .evaluate(&ctx)
@@ -151,7 +152,7 @@ mod test {
 
         let ctx = Context::new();
         assert_eq!(
-            Token::IntegerLiteral(42i32)
+            *Token::IntegerLiteral(42i32)
                 .to_arg()
                 .unwrap()
                 .evaluate(&ctx)
@@ -164,7 +165,7 @@ mod test {
     fn evaluate_handles_boolean_literals() {
         let ctx = Context::new();
         assert_eq!(
-            Token::BooleanLiteral(true)
+            *Token::BooleanLiteral(true)
                 .to_arg()
                 .unwrap()
                 .evaluate(&ctx)
@@ -173,7 +174,7 @@ mod test {
         );
 
         assert_eq!(
-            Token::BooleanLiteral(false)
+            *Token::BooleanLiteral(false)
                 .to_arg()
                 .unwrap()
                 .evaluate(&ctx)
@@ -187,7 +188,7 @@ mod test {
         let mut ctx = Context::new();
         ctx.stack_mut().set_global("var0", Value::scalar(42f64));
         assert_eq!(
-            Token::Identifier("var0".to_owned())
+            *Token::Identifier("var0".to_owned())
                 .to_arg()
                 .unwrap()
                 .evaluate(&ctx)

diff --git a/liquid-interpreter/src/context.rs b/liquid-interpreter/src/context.rs
@@ -3,7 +3,8 @@ use std::collections::HashMap;
 use std::sync;
 
 use error::{Error, Result};
-use value::{Object, Path, Value};
+use itertools;
+use value::{Object, PathRef, Scalar, Value};
 
 use super::Expression;
 use super::Globals;
@@ -83,7 +84,11 @@ where
 
 impl<'a, 'g> CycleState<'a, 'g> {
     /// See `cycle` tag.
-    pub fn cycle_element(&mut self, name: &str, values: &[Expression]) -> Result<Value> {
+    pub fn cycle_element<'c>(
+        &'c mut self,
+        name: &str,
+        values: &'c [Expression],
+    ) -> Result<&'c Value> {
         let index = self.context.cycles.cycle_index(name, values.len());
         if index >= values.len() {
             return Err(Error::with_msg(
@@ -156,45 +161,74 @@ impl<'g> Stack<'g> {
     ///
     /// This method will panic if popping the topmost frame results in an
     /// empty stack. Given that a context is created with a top-level stack
-    /// frame already in place, empyting the stack should never happen in a
+    /// frame already in place, emptying the stack should never happen in a
     /// well-formed program.
     fn pop_frame(&mut self) {
         if self.stack.pop().is_none() {
-            panic!("Pop leaves empty stack")
+            panic!("Unbalanced push/pop, leaving the stack empty.")
         };
     }
 
     /// Recursively index into the stack.
-    pub fn get(&self, path: &Path) -> Result<&Value> {
-        let mut indexes = path.iter();
-        let key = indexes
-            .next()
-            .ok_or_else(|| Error::with_msg("No variable provided"))?;
+    pub fn try_get(&self, path: PathRef) -> Option<&Value> {
+        let frame = self.find_path_frame(path)?;
+
+        frame.try_get_variable(path)
+    }
+
+    /// Recursively index into the stack.
+    pub fn get(&self, path: PathRef) -> Result<&Value> {
+        let frame = self.find_path_frame(path).ok_or_else(|| {
+            let key = path
+                .iter()
+                .next()
+                .cloned()
+                .unwrap_or_else(|| Scalar::new("nil"));
+            let globals = itertools::join(self.globals().iter(), ", ");
+            Error::with_msg("Unknown variable")
+                .context("requested variable", key.to_str().into_owned())
+                .context("available variables", globals)
+        })?;
+
+        frame.get_variable(path)
+    }
+
+    fn globals(&self) -> Vec<&str> {
+        let mut globals = self.globals.map(|g| g.globals()).unwrap_or_default();
+        for frame in self.stack.iter() {
+            globals.extend(frame.globals());
+        }
+        globals.sort();
+        globals.dedup();
+        globals
+    }
+
+    fn find_path_frame<'a>(&'a self, path: PathRef) -> Option<&'a Globals> {
+        let key = path.iter().next()?;
         let key = key.to_str();
-        let value = self.get_root(key.as_ref())?;
-
-        indexes.fold(Ok(value), |value, index| {
-            let value = value?;
-            let child = value.get(index);
-            let child = child.ok_or_else(|| {
-                Error::with_msg("Unknown index")
-                    .context("variable", format!("{}", path))
-                    .context("index", format!("{}", index))
-            })?;
-            Ok(child)
-        })
+        self.find_frame(key.as_ref())
     }
 
-    fn get_root<'a>(&'a self, name: &str) -> Result<&'a Value> {
+    fn find_frame<'a>(&'a self, name: &str) -> Option<&'a Globals> {
         for frame in self.stack.iter().rev() {
-            if let Some(rval) = frame.get(name) {
-                return Ok(rval);
+            if frame.contains_global(name) {
+                return Some(frame);
             }
         }
-        self.globals
-            .ok_or_else(|| Error::with_msg("Unknown variable").context("variable", name.to_owned()))
-            .and_then(|g| g.get(name))
-            .or_else(|err| self.get_index(name).ok_or_else(|| err))
+
+        if self
+            .globals
+            .map(|g| g.contains_global(name))
+            .unwrap_or(false)
+        {
+            return self.globals;
+        }
+
+        if self.indexes.contains_global(name) {
+            return Some(&self.indexes);
+        }
+
+        None
     }
 
     /// Used by increment and decrement tags
@@ -391,22 +425,19 @@ mod test {
     use value::Scalar;
 
     #[test]
-    fn stack_get_root() {
+    fn stack_find_frame() {
         let mut ctx = Context::new();
         ctx.stack_mut().set_global("number", Value::scalar(42f64));
-        assert_eq!(
-            ctx.stack().get_root("number").unwrap(),
-            &Value::scalar(42f64)
-        );
+        assert!(ctx.stack().find_frame("number").is_some(),);
     }
 
     #[test]
-    fn stack_get_root_failure() {
+    fn stack_find_frame_failure() {
         let mut ctx = Context::new();
         let mut post = Object::new();
         post.insert("number".into(), Value::scalar(42f64));
         ctx.stack_mut().set_global("post", Value::Object(post));
-        assert!(ctx.stack().get_root("post.number").is_err());
+        assert!(ctx.stack().find_frame("post.number").is_none());
     }
 
     #[test]
@@ -415,29 +446,30 @@ mod test {
         let mut post = Object::new();
         post.insert("number".into(), Value::scalar(42f64));
         ctx.stack_mut().set_global("post", Value::Object(post));
-        let indexes = vec![Scalar::new("post"), Scalar::new("number")]
-            .into_iter()
-            .collect();
+        let indexes = [Scalar::new("post"), Scalar::new("number")];
         assert_eq!(ctx.stack().get(&indexes).unwrap(), &Value::scalar(42f64));
     }
 
     #[test]
     fn scoped_variables() {
+        let test_path = [Scalar::new("test")];
+        let global_path = [Scalar::new("global")];
+
         let mut ctx = Context::new();
         ctx.stack_mut().set_global("test", Value::scalar(42f64));
-        assert_eq!(ctx.stack().get_root("test").unwrap(), &Value::scalar(42f64));
+        assert_eq!(ctx.stack().get(&test_path).unwrap(), &Value::scalar(42f64));
 
         ctx.run_in_scope(|new_scope| {
             // assert that values are chained to the parent scope
             assert_eq!(
-                new_scope.stack().get_root("test").unwrap(),
+                new_scope.stack().get(&test_path).unwrap(),
                 &Value::scalar(42f64)
             );
 
             // set a new local value, and assert that it overrides the previous value
             new_scope.stack_mut().set("test", Value::scalar(3.14f64));
             assert_eq!(
-                new_scope.stack().get_root("test").unwrap(),
+                new_scope.stack().get(&test_path).unwrap(),
                 &Value::scalar(3.14f64)
             );
 
@@ -448,9 +480,9 @@ mod test {
         });
 
         // assert that the value has reverted to the old one
-        assert_eq!(ctx.stack().get_root("test").unwrap(), &Value::scalar(42f64));
+        assert_eq!(ctx.stack().get(&test_path).unwrap(), &Value::scalar(42f64));
         assert_eq!(
-            ctx.stack().get_root("global").unwrap(),
+            ctx.stack().get(&global_path).unwrap(),
             &Value::scalar("some value")
         );
     }

diff --git a/liquid-interpreter/src/expression.rs b/liquid-interpreter/src/expression.rs
@@ -39,12 +39,24 @@ impl Expression {
     }
 
     /// Convert to a `Value`.
-    pub fn evaluate(&self, context: &Context) -> Result<Value> {
+    pub fn try_evaluate<'c>(&'c self, context: &'c Context) -> Option<&'c Value> {
         let val = match *self {
-            Expression::Literal(ref x) => x.clone(),
+            Expression::Literal(ref x) => &x,
+            Expression::Variable(ref x) => {
+                let path = x.try_evaluate(context)?;
+                context.stack().try_get(&path)?
+            }
+        };
+        Some(val)
+    }
+
+    /// Convert to a `Value`.
+    pub fn evaluate<'c>(&'c self, context: &'c Context) -> Result<&'c Value> {
+        let val = match *self {
+            Expression::Literal(ref x) => x,
             Expression::Variable(ref x) => {
                 let path = x.evaluate(context)?;
-                context.stack().get(&path)?.clone()
+                context.stack().get(&path)?
             }
         };
         Ok(val)

diff --git a/liquid-interpreter/src/filter_chain.rs b/liquid-interpreter/src/filter_chain.rs
@@ -54,7 +54,7 @@ impl FilterChain {
     /// Process `Value` expression within `context`'s stack.
     pub fn evaluate(&self, context: &Context) -> Result<Value> {
         // take either the provided value or the value from the provided variable
-        let mut entry = self.entry.evaluate(context)?;
+        let mut entry = self.entry.evaluate(context)?.to_owned();
 
         // apply all specified filters
         for filter in &self.filters {
@@ -65,7 +65,7 @@ impl FilterChain {
             let arguments: Result<Vec<Value>> = filter
                 .arguments
                 .iter()
-                .map(|a| a.evaluate(context))
+                .map(|a| Ok(a.evaluate(context)?.to_owned()))
                 .collect();
             let arguments = arguments?;
             entry = f