Repository: djc/askama
Pull request #453 (2021-02-22 03:24:50)

Fixed path parser to account for single identifier type names
- Fixed `parser::path()` to account for `None` and `Some`. In short, a single identifier is considered a path, if it contains at least 1 uppercase and lowercase character
- Added parser tests for variables and paths
- Added generator test for `{{ Self::foo(None) }} {{ Self::foo(Some(2)) }}`

Reference #451


vallentin: While fixing this I was thinking about #433. Instead of having the following check, then we could e.g. change `Expr::Var(&'a str)` into `Expr::Var(&'a str, bool)`. The `bool` would then represent whether if it's a constant or not, and in short enforce that `self.` is not prepended.

https://github.com/djc/askama/blob/7ee368e57a419bf05826522df695ddd62d17684d/askama_shared/src/generator.rs#L1726

Note I didn't really look into it much, but I feel like using `bool` would be the smallest change. We could also refactor `Expr::Var` into `Expr::Field` and `Expr::Const`. Again, I didn't look into it, but the initial thought is that it would be a bigger change than adding a `bool`. Though I prefer the latter, as it is less ambiguous.

What do you think? Honestly, whether it's none, the former or the latter. It doesn't really matter, I'll refactor if you like the idea.


Patch:
diff --git a/askama_shared/src/parser.rs b/askama_shared/src/parser.rs
index 80f766a..df3ee8f 100644
--- a/askama_shared/src/parser.rs
+++ b/askama_shared/src/parser.rs
@@ -66,10 +66,11 @@ impl Expr<'_> {
             BinOp(_, lhs, rhs) => {
                 lhs.is_copyable_within_op(true) && rhs.is_copyable_within_op(true)
             }
+            Range(..) => true,
             // The result of a call likely doesn't need to be borrowed,
             // as in that case the call is more likely to return a
             // reference in the first place then.
-            VarCall(..) | PathCall(..) | MethodCall(..) => true,
+            VarCall(..) | Path(..) | PathCall(..) | MethodCall(..) => true,
             // If the `expr` is within a `Unary` or `BinOp` then
             // an assumption can be made that the operand is copy.
             // If not, then the value is moved and adding `.clone()`
@@ -355,12 +356,28 @@ fn expr_var_call(i: &[u8]) -> IResult<&[u8], Expr> {
 fn path(i: &[u8]) -> IResult<&[u8], Vec<&str>> {
     let root = opt(value("", ws(tag("::"))));
     let tail = separated_list1(ws(tag("::")), identifier);
-    let (i, (root, start, _, rest)) = tuple((root, identifier, ws(tag("::")), tail))(i)?;
-    let mut path = Vec::new();
-    path.extend(root);
-    path.push(start);
-    path.extend(rest);
-    Ok((i, path))
+
+    match tuple((root, identifier, ws(tag("::")), tail))(i) {
+        Ok((i, (root, start, _, rest))) => {
+            let mut path = Vec::new();
+            path.extend(root);
+            path.push(start);
+            path.extend(rest);
+            Ok((i, path))
+        }
+        Err(err) => {
+            if let Ok((i, name)) = identifier(i) {
+                // If the returned identifier contains both a lowercase and uppercase
+                // character, then we assume it's a type name, e.g. `Some`.
+                if name.contains(char::is_uppercase) && name.contains(char::is_lowercase) {
+                    return Ok((i, vec![name]));
+                }
+            }
+
+            // If `identifier()` fails then just return the original error
+            Err(err)
+        }
+    }
 }
 
 fn expr_path(i: &[u8]) -> IResult<&[u8], Expr> {
@@ -1209,6 +1226,60 @@ mod tests {
         );
     }
 
+    #[test]
+    fn test_parse_var() {
+        let s = Syntax::default();
+
+        assert_eq!(
+            super::parse("{{ foo }}", &s).unwrap(),
+            vec![Node::Expr(Ws(false, false), Expr::Var("foo"))],
+        );
+        assert_eq!(
+            super::parse("{{ FOO }}", &s).unwrap(),
+            vec![Node::Expr(Ws(false, false), Expr::Var("FOO"))],
+        );
+
+        assert_eq!(
+            super::parse("{{ none }}", &s).unwrap(),
+            vec![Node::Expr(Ws(false, false), Expr::Var("none"))],
+        );
+        assert_eq!(
+            super::parse("{{ NONE }}", &s).unwrap(),
+            vec![Node::Expr(Ws(false, false), Expr::Var("NONE"))],
+        );
+    }
+
+    #[test]
+    fn test_parse_path() {
+        let s = Syntax::default();
+
+        assert_eq!(
+            super::parse("{{ None }}", &s).unwrap(),
+            vec![Node::Expr(Ws(false, false), Expr::Path(vec!["None"]))],
+        );
+        assert_eq!(
+            super::parse("{{ Some(123) }}", &s).unwrap(),
+            vec![Node::Expr(
+                Ws(false, false),
+                Expr::PathCall(vec!["Some"], vec![Expr::NumLit("123")],),
+            )],
+        );
+        assert_eq!(
+            super::parse("{{ Ok(123) }}", &s).unwrap(),
+            vec![Node::Expr(
+                Ws(false, false),
+                Expr::PathCall(vec!["Ok"], vec![Expr::NumLit("123")],),
+            )],
+        );
+        assert_eq!(
+            super::parse("{{ Err(123) }}", &s).unwrap(),
+            vec![Node::Expr(
+                Ws(false, false),
+                Expr::PathCall(vec!["Err"], vec![Expr::NumLit("123")],),
+            )],
+        );
+    }
+
     #[test]
     fn test_parse_var_call() {
         assert_eq!(
@@ -1222,8 +1293,25 @@ mod tests {
 
     #[test]
     fn test_parse_path_call() {
+        let s = Syntax::default();
+
+        assert_eq!(
+            super::parse("{{ Option::None }}", &s).unwrap(),
+            vec![Node::Expr(
+                Ws(false, false),
+                Expr::Path(vec!["Option", "None"])
+            )],
+        );
+        assert_eq!(
+            super::parse("{{ Option::Some(123) }}", &s).unwrap(),
+            vec![Node::Expr(
+                Ws(false, false),
+                Expr::PathCall(vec!["Option", "Some"], vec![Expr::NumLit("123")],),
+            )],
+        );
+
         assert_eq!(
-            super::parse("{{ self::function(\"123\", 3) }}", &Syntax::default()).unwrap(),
+            super::parse("{{ self::function(\"123\", 3) }}", &s).unwrap(),
             vec![Node::Expr(
                 Ws(false, false),
                 Expr::PathCall(


Test patch:
diff --git a/testing/tests/loops.rs b/testing/tests/loops.rs
index 809f75e..b324ae3 100644
--- a/testing/tests/loops.rs
+++ b/testing/tests/loops.rs
@@ -70,11 +70,11 @@ fn test_for_range() {
 
 #[derive(Template)]
 #[template(source = "{% for i in [1, 2, 3] %}{{ i }}{% endfor %}", ext = "txt")]
-struct ForArrayTemplate {}
+struct ForArrayTemplate;
 
 #[test]
 fn test_for_array() {
-    let t = ForArrayTemplate {};
+    let t = ForArrayTemplate;
     assert_eq!(t.render().unwrap(), "123");
 }
 
@@ -83,11 +83,11 @@ fn test_for_array() {
     source = "{% for i in [1, 2, 3].iter() %}{{ i }}{% endfor %}",
     ext = "txt"
 )]
-struct ForMethodCallTemplate {}
+struct ForMethodCallTemplate;
 
 #[test]
 fn test_for_method_call() {
-    let t = ForMethodCallTemplate {};
+    let t = ForMethodCallTemplate;
     assert_eq!(t.render().unwrap(), "123");
 }
 
@@ -108,10 +108,26 @@ fn test_for_path_call() {
     source = "{% for i in [1, 2, 3, 4, 5][3..] %}{{ i }}{% endfor %}",
     ext = "txt"
 )]
-struct ForIndexTemplate {}
+struct ForIndexTemplate;
 
 #[test]
 fn test_for_index() {
-    let t = ForIndexTemplate {};
+    let t = ForIndexTemplate;
     assert_eq!(t.render().unwrap(), "45");
 }
+
+#[derive(Template)]
+#[template(
+    source = "{% for (i, j) in (0..10).zip(10..20).zip(30..40) %}{{ i.0 }} {{ i.1 }} {{ j }} {% endfor %}",
+    ext = "txt"
+)]
+struct ForZipRangesTemplate;
+
+#[test]
+fn test_for_zip_ranges() {
+    let t = ForZipRangesTemplate;
+    assert_eq!(
+        t.render().unwrap(),
+        "0 10 30 1 11 31 2 12 32 3 13 33 4 14 34 5 15 35 6 16 36 7 17 37 8 18 38 9 19 39 "
+    );
+}
diff --git a/testing/tests/simple.rs b/testing/tests/simple.rs
index e7fca2c..b6dd31a 100644
--- a/testing/tests/simple.rs
+++ b/testing/tests/simple.rs
@@ -231,6 +231,22 @@ fn test_option() {
     assert_eq!(none.render().unwrap(), "none");
 }
 
+#[derive(Template)]
+#[template(source = "{{ Self::foo(None) }} {{ Self::foo(Some(1)) }}", ext = "txt")]
+struct OptionNoneSomeTemplate;
+
+impl OptionNoneSomeTemplate {
+    fn foo(x: Option<i32>) -> i32 {
+        x.unwrap_or_default()
+    }
+}
+
+#[test]
+fn test_option_none_some() {
+    let t = OptionNoneSomeTemplate;
+    assert_eq!(t.render().unwrap(), "0 1");
+}
+
 #[derive(Template)]
 #[template(path = "generics.html")]
 struct GenericsTemplate<T, U = u8>