Repository: djc/askama
Pull request #503 (2021-06-25 19:32:09)

Implement "if let" statement
This PR implements ``{% if let variant = value %}`` conditions.

The variant can be ``None``, ``Some(foo)``, ``MyStruct { x, y }``,
etc., but nested matching is not implemented. Names are matched by
reference.

The syntax is mostly sugar for

```rs
{% match val %}
    {% when variant %}
        ...
    {% when _ -%}
{% endmatch %}
```

but it can be used in ordinary if-else blocks.

Kijewski: > A test case with `{% else %}` would also be nice :)

I added more tests. 

> Other than that, I personally would love some sugar for simplifying "if some then unwrap"

Thank you! Since it's such a prevalent pattern in rust, I thought such a syntactic sugar might be useful to others, too. :)
vallentin: Nice, it didn't register to me, that you could do `if let` on a whole struct with fields. That's pretty cool.

I'll wait until @djc has given it a review :)
djc: I think the goal here is good to have. In order to be able to review this well, however, some work needs to be done. At the very least, mechanic changes to add explicit lifetimes all over the place should be in a separate commit. I'd also prefer to have any refactoring in a separate commit before the new stuff is added.
Kijewski: I wanted to wait for the general consensus, if that feature is wanted before I clean up my commits. I'll do that now. :)
vallentin: I only just gave it a quick scroll through. Besides being explicit, are the added lifetimes required for anything? :)
Kijewski: > I only just gave it a quick scroll through. Besides being explicit, are the added lifetimes required for anything? :)

In the first iteration I only added the explicit lifetimes when the compiler refused to compile the code without it. Well, that was just about everywhere.

Now, while refactoring (actually re-doing) my changes, I did not look where I needed to add the lifetime. Since the changes are internal to Generator they can't break anything. `cargo clean && cargo clippy -- -D clippy::needless_lifetimes` did not report anything.
Kijewski: The lifetimes are not needed anymore. I mixed up expressions and nodes. That's fixed now.
Kijewski: Thanks for the review! I made changes.
djc: We can ignore the test failure (which is due to #504), can you fix up the clippy thing?
Kijewski: cargo fmt is happy now.
djc: But the clippy issue is still there...
Kijewski: Ah, sorry. Fixed: https://github.com/djc/askama/compare/4e9493a..12a6783

Patch:
diff --git a/askama_shared/src/generator.rs b/askama_shared/src/generator.rs
index 0ce7df3..fc94977 100644
--- a/askama_shared/src/generator.rs
+++ b/askama_shared/src/generator.rs
@@ -3,7 +3,8 @@ use crate::filters;
 use crate::heritage::{Context, Heritage};
 use crate::input::{Source, TemplateInput};
 use crate::parser::{
-    parse, Cond, Expr, MatchParameter, MatchParameters, MatchVariant, Node, Target, When, Ws,
+    parse, Cond, CondTest, Expr, MatchParameter, MatchParameters, MatchVariant, Node, Target, When,
+    Ws,
 };
 
 use proc_macro2::Span;
@@ -483,15 +484,28 @@ impl<'a, S: std::hash::BuildHasher> Generator<'a, S> {
                 self.locals.pop();
             }
 
+            self.locals.push();
             let mut arm_size = 0;
-            match *cond {
-                Some(ref expr) => {
-                    if i == 0 {
-                        buf.write("if ");
-                    } else {
-                        buf.dedent()?;
-                        buf.write("} else if ");
+            if let Some(CondTest { target, expr }) = cond {
+                if i == 0 {
+                    buf.write("if ");
+                } else {
+                    buf.dedent()?;
+                    buf.write("} else if ");
+                }
+
+                if let Some((variant, params)) = target {
+                    let mut expr_buf = Buffer::new(0);
+                    self.visit_expr(&mut expr_buf, expr)?;
+                    buf.write("let ");
+                    self.visit_match_variant(buf, variant);
+                    if let Some(params) = params {
+                        self.visit_match_params(buf, params);
                     }
+                    buf.write(" = &(");
+                    buf.write(&expr_buf.buf);
+                    buf.write(")");
+                } else {
                     // The following syntax `*(&(...) as &bool)` is used to
                     // trigger Rust's automatic dereferencing, to coerce
                     // e.g. `&&&&&bool` to `bool`. First `&(...) as &bool`
@@ -502,15 +516,13 @@ impl<'a, S: std::hash::BuildHasher> Generator<'a, S> {
                     buf.write(&expr_code);
                     buf.write(") as &bool)");
                 }
-                None => {
-                    buf.dedent()?;
-                    buf.write("} else");
-                    has_else = true;
-                }
+            } else {
+                buf.dedent()?;
+                buf.write("} else");
+                has_else = true;
             }
 
             buf.writeln(" {")?;
-            self.locals.push();
 
             arm_size += self.handle(ctx, nodes, buf, AstLevel::Nested)?;
             arm_sizes.push(arm_size);
@@ -564,44 +576,7 @@ impl<'a, S: std::hash::BuildHasher> Generator<'a, S> {
                 None => buf.write("_"),
             };
 
-            match params {
-                MatchParameters::Simple(params) => {
-                    if !params.is_empty() {
-                        buf.write("(");
-                        for (i, param) in params.iter().enumerate() {
-                            if let MatchParameter::Name(p) = *param {
-                                self.locals.insert_with_default(p);
-                            }
-                            if i > 0 {
-                                buf.write(", ");
-                            }
-                            self.visit_match_param(buf, param);
-                        }
-                        buf.write(")");
-                    }
-                }
-                MatchParameters::Named(params) => {
-                    buf.write("{");
-                    for (i, param) in params.iter().enumerate() {
-                        if let Some(MatchParameter::Name(p)) = param.1 {
-                            let p = normalize_identifier(p);
-                            self.locals.insert_with_default(p);
-                        } else {
-                            self.locals.insert_with_default(param.0);
-                        }
-
-                        if i > 0 {
-                            buf.write(", ");
-                        }
-                        buf.write(param.0);
-                        if let Some(param) = &param.1 {
-                            buf.write(":");
-                            self.visit_match_param(buf, param);
-                        }
-                    }
-                    buf.write("}");
-                }
-            }
+            self.visit_match_params(buf, params);
             buf.writeln(" => {")?;
 
             arm_size = self.handle(ctx, body, buf, AstLevel::Nested)?;
@@ -1155,6 +1130,47 @@ impl<'a, S: std::hash::BuildHasher> Generator<'a, S> {
         wrapped
     }
 
+    fn visit_match_params(&mut self, buf: &mut Buffer, params: &MatchParameters<'a>) {
+        match params {
+            MatchParameters::Simple(params) => {
+                if !params.is_empty() {
+                    buf.write("(");
+                    for (i, param) in params.iter().enumerate() {
+                        if let MatchParameter::Name(p) = *param {
+                            self.locals.insert_with_default(p);
+                        }
+                        if i > 0 {
+                            buf.write(", ");
+                        }
+                        self.visit_match_param(buf, param);
+                    }
+                    buf.write(")");
+                }
+            }
+            MatchParameters::Named(params) => {
+                buf.write("{");
+                for (i, param) in params.iter().enumerate() {
+                    if let Some(MatchParameter::Name(p)) = param.1 {
+                        let p = normalize_identifier(p);
+                        self.locals.insert_with_default(p);
+                    } else {
+                        self.locals.insert_with_default(param.0);
+                    }
+
+                    if i > 0 {
+                        buf.write(", ");
+                    }
+                    buf.write(param.0);
+                    if let Some(param) = &param.1 {
+                        buf.write(":");
+                        self.visit_match_param(buf, param);
+                    }
+                }
+                buf.write("}");
+            }
+        }
+    }
+
     fn visit_filter(
         &mut self,
         buf: &mut Buffer,
diff --git a/askama_shared/src/lib.rs b/askama_shared/src/lib.rs
index 290ca19..0b1f375 100644
--- a/askama_shared/src/lib.rs
+++ b/askama_shared/src/lib.rs
@@ -200,7 +200,7 @@ struct RawConfig<'d> {
 impl<'d> RawConfig<'d> {
     #[cfg(feature = "config")]
     fn from_toml_str(s: &str) -> std::result::Result<RawConfig<'_>, CompileError> {
-        toml::from_str(&s).map_err(|e| {
+        toml::from_str(s).map_err(|e| {
             CompileError::String(format!("invalid TOML in {}: {}", CONFIG_FILE_NAME, e))
         })
     }
diff --git a/askama_shared/src/parser.rs b/askama_shared/src/parser.rs
index ea13738..3c5a86c 100644
--- a/askama_shared/src/parser.rs
+++ b/askama_shared/src/parser.rs
@@ -18,7 +18,7 @@ pub enum Node<'a> {
     Call(Ws, Option<&'a str>, &'a str, Vec<Expr<'a>>),
     LetDecl(Ws, Target<'a>),
     Let(Ws, Target<'a>, Expr<'a>),
-    Cond(Vec<(Ws, Option<Expr<'a>>, Vec<Node<'a>>)>, Ws),
+    Cond(Vec<Cond<'a>>, Ws),
     Match(Ws, Expr<'a>, Vec<When<'a>>, Ws),
     Loop(Ws, Target<'a>, Expr<'a>, Vec<Node<'a>>, Ws),
     Extends(Expr<'a>),
@@ -144,7 +144,13 @@ pub enum Target<'a> {
 #[derive(Clone, Copy, Debug, PartialEq)]
 pub struct Ws(pub bool, pub bool);
 
-pub type Cond<'a> = (Ws, Option<Expr<'a>>, Vec<Node<'a>>);
+pub type Cond<'a> = (Ws, Option<CondTest<'a>>, Vec<Node<'a>>);
+
+#[derive(Debug, PartialEq)]
+pub struct CondTest<'a> {
+    pub target: Option<(MatchVariant<'a>, Option<MatchParameters<'a>>)>,
+    pub expr: Expr<'a>,
+}
 
 fn ws<F, I, O, E>(mut inner: F) -> impl FnMut(I) -> IResult<I, O, E>
 where
@@ -724,9 +730,25 @@ fn block_call(i: &[u8]) -> IResult<&[u8], Node> {
     ))
 }
 
-fn cond_if(i: &[u8]) -> IResult<&[u8], Expr> {
-    let (i, (_, cond)) = tuple((ws(tag("if")), ws(expr_any)))(i)?;
-    Ok((i, cond))
+fn cond_if(i: &[u8]) -> IResult<&[u8], CondTest> {
+    let mut p = tuple((
+        ws(tag("if")),
+        opt(tuple((
+            ws(alt((tag("let"), tag("set")))),
+            ws(match_variant),
+            opt(alt((match_simple_parameters, match_named_parameters))),
+            ws(tag("=")),
+        ))),
+        ws(expr_any),
+    ));
+    let (i, (_, dest, expr)) = p(i)?;
+    Ok((
+        i,
+        CondTest {
+            target: dest.map(|(_, variant, params, _)| (variant, params)),
+            expr,
+        },
+    ))
 }
 
 fn cond_block<'a>(i: &'a [u8], s: &'a Syntax<'a>) -> IResult<&'a [u8], Cond<'a>> {


Test patch:
diff --git a/testing/templates/if-let-else.html b/testing/templates/if-let-else.html
new file mode 100644
index 0000000..e6b43ca
--- /dev/null
+++ b/testing/templates/if-let-else.html
@@ -0,0 +1,7 @@
+{%- if !cond -%}
+    !cond
+{%- else if let Ok(ok) = value -%}
+    {{ ok }}
+{%- else if let Err(err) = value -%}
+    {{ err }}
+{%- endif -%}
diff --git a/testing/templates/if-let-shadowing.html b/testing/templates/if-let-shadowing.html
new file mode 100644
index 0000000..7f3d50f
--- /dev/null
+++ b/testing/templates/if-let-shadowing.html
@@ -0,0 +1,1 @@
+{% if let Some(text) = text %}{{ text }}{% endif %}
diff --git a/testing/templates/if-let-struct.html b/testing/templates/if-let-struct.html
new file mode 100644
index 0000000..53232f7
--- /dev/null
+++ b/testing/templates/if-let-struct.html
@@ -0,0 +1,1 @@
+{% if let Digits { one, two, three } = digits %}{{ one }} {{ two }} {{ three }}{% endif %}
diff --git a/testing/templates/if-let.html b/testing/templates/if-let.html
new file mode 100644
index 0000000..ff4006f
--- /dev/null
+++ b/testing/templates/if-let.html
@@ -0,0 +1,1 @@
+{% if let Some(some_text) = text %}{{ some_text }}{% endif %}
diff --git a/testing/tests/if_let.rs b/testing/tests/if_let.rs
new file mode 100644
index 0000000..3e4e2b5
--- /dev/null
+++ b/testing/tests/if_let.rs
@@ -0,0 +1,109 @@
+use askama::Template;
+
+#[derive(Template)]
+#[template(path = "if-let.html")]
+struct IfLetTemplate {
+    text: Option<&'static str>,
+}
+
+#[test]
+fn test_if_let() {
+    let s = IfLetTemplate {
+        text: Some("hello"),
+    };
+    assert_eq!(s.render().unwrap(), "hello");
+
+    let t = IfLetTemplate { text: None };
+    assert_eq!(t.render().unwrap(), "");
+}
+
+#[derive(Template)]
+#[template(path = "if-let-shadowing.html")]
+struct IfLetShadowingTemplate {
+    text: Option<&'static str>,
+}
+
+#[test]
+fn test_if_let_shadowing() {
+    let s = IfLetShadowingTemplate {
+        text: Some("hello"),
+    };
+    assert_eq!(s.render().unwrap(), "hello");
+
+    let t = IfLetShadowingTemplate { text: None };
+    assert_eq!(t.render().unwrap(), "");
+}
+
+struct Digits {
+    one: i32,
+    two: i32,
+    three: i32,
+}
+
+#[derive(Template)]
+#[template(path = "if-let-struct.html")]
+struct IfLetStruct {
+    digits: Digits,
+}
+
+#[test]
+fn test_if_let_struct() {
+    let digits = Digits {
+        one: 1,
+        two: 2,
+        three: 3,
+    };
+    let s = IfLetStruct { digits };
+    assert_eq!(s.render().unwrap(), "1 2 3");
+}
+
+#[derive(Template)]
+#[template(path = "if-let-struct.html")]
+struct IfLetStructRef<'a> {
+    digits: &'a Digits,
+}
+
+#[test]
+fn test_if_let_struct_ref() {
+    let digits = Digits {
+        one: 1,
+        two: 2,
+        three: 3,
+    };
+    let s = IfLetStructRef { digits: &digits };
+    assert_eq!(s.render().unwrap(), "1 2 3");
+}
+
+#[derive(Template)]
+#[template(path = "if-let-else.html")]
+struct IfLetElse {
+    cond: bool,
+    value: Result<i32, &'static str>,
+}
+
+#[test]
+fn test_if_let_else() {
+    let s = IfLetElse {
+        cond: false,
+        value: Ok(4711),
+    };
+    assert_eq!(s.render().unwrap(), "!cond");
+
+    let s = IfLetElse {
+        cond: true,
+        value: Ok(4711),
+    };
+    assert_eq!(s.render().unwrap(), "4711");
+
+    let s = IfLetElse {
+        cond: false,
+        value: Err("fail"),
+    };
+    assert_eq!(s.render().unwrap(), "!cond");
+
+    let s = IfLetElse {
+        cond: true,
+        value: Err("fail"),
+    };
+    assert_eq!(s.render().unwrap(), "fail");
+}