Repository: djc/askama
Pull request #510 (2021-07-01 18:52:15)

Fix and optimize visit_target usage
There was a bit of code duplication in generator.rs with respect to the output of targets.

Also there was a bug which caused broken code for

```rs
{% for val in range %}{% set val = "anything" %}{% endfor %}
```

which tried to write to `val` instead of shadowing it.

Kijewski: I addressed you comments.

Actually I'm not sure if I like matching bools. I'm quite new to rust and as a Python / C++ programmer it just feels wrong. You'd get publicly stoned if you shared C++ code that contains `switch (cond) { case true: …`  :)
djc: > Actually I'm not sure if I like matching bools. I'm quite new to rust and as a Python / C++ programmer it just feels wrong.

Can you articulate what you dislike about them?

Patch:
diff --git a/askama_shared/src/generator.rs b/askama_shared/src/generator.rs
index 8457400..281c3b0 100644
--- a/askama_shared/src/generator.rs
+++ b/askama_shared/src/generator.rs
@@ -614,7 +614,7 @@ impl<'a, S: std::hash::BuildHasher> Generator<'a, S> {
 
         let flushed = self.write_buf_writable(buf)?;
         buf.write("for (");
-        self.visit_target(buf, var);
+        self.visit_target(buf, true, var);
         match iter {
             Expr::Range(_, _, _) => buf.writeln(&format!(
                 ", _loop_item) in ::askama::helpers::TemplateLoop::new({}) {{",
@@ -807,24 +807,27 @@ impl<'a, S: std::hash::BuildHasher> Generator<'a, S> {
         self.handle_ws(ws);
         self.write_buf_writable(buf)?;
         buf.write("let ");
-        match *var {
+        self.visit_target(buf, false, var);
+        buf.writeln(";")
+    }
+
+    fn is_shadowing_variable(&self, var: &Target<'a>) -> bool {
+        match var {
             Target::Name(name) => {
                 let name = normalize_identifier(name);
-                self.locals.insert_with_default(name);
-                buf.write(name);
-            }
-            Target::Tuple(ref targets) => {
-                buf.write("(");
-                for name in targets {
-                    let name = normalize_identifier(name);
-                    self.locals.insert_with_default(name);
-                    buf.write(name);
-                    buf.write(",");
+                match self.locals.get(&name) {
+                    // declares a new variable
+                    None => false,
+                    // an initialized variable gets shadowed
+                    Some(meta) if meta.initialized => true,
+                    // initializes a variable that was introduced in a LetDecl before
+                    _ => false,
                 }
-                buf.write(")");
             }
+            Target::Tuple(targets) => targets
+                .iter()
+                .any(|name| self.is_shadowing_variable(&Target::Name(name))),
         }
-        buf.writeln(";")
     }
 
     fn write_let(
@@ -838,45 +841,20 @@ impl<'a, S: std::hash::BuildHasher> Generator<'a, S> {
         let mut expr_buf = Buffer::new(0);
         self.visit_expr(&mut expr_buf, val)?;
 
-        match *var {
-            Target::Name(name) => {
-                let name = normalize_identifier(name);
-                let meta = self.locals.get(&name).cloned();
-
-                let shadowed = matches!(&meta, Some(meta) if meta.initialized);
-                if shadowed {
-                    // Need to flush the buffer if the variable is being shadowed,
-                    // to ensure the old variable is used.
-                    self.write_buf_writable(buf)?;
-                }
-                if shadowed || meta.is_none() {
-                    buf.write("let ");
-                }
-                buf.write(name);
-
-                self.locals.insert(name, LocalMeta::initialized());
-            }
-            Target::Tuple(ref targets) => {
-                let shadowed = targets.iter().any(|name| {
-                    let name = normalize_identifier(name);
-                    matches!(self.locals.get(&name), Some(meta) if meta.initialized)
-                });
-                if shadowed {
-                    // Need to flush the buffer if the variable is being shadowed,
-                    // to ensure the old variable is used.
-                    self.write_buf_writable(buf)?;
-                }
-
-                buf.write("let (");
-                for name in targets {
-                    let name = normalize_identifier(name);
-                    self.locals.insert(name, LocalMeta::initialized());
-                    buf.write(name);
-                    buf.write(",");
-                }
-                buf.write(")");
-            }
+        let shadowed = self.is_shadowing_variable(var);
+        if shadowed {
+            // Need to flush the buffer if the variable is being shadowed,
+            // to ensure the old variable is used.
+            self.write_buf_writable(buf)?;
         }
+        if shadowed
+            || !matches!(var, &Target::Name(_))
+            || matches!(var, Target::Name(name) if self.locals.get(name).is_none())
+        {
+            buf.write("let ");
+        }
+
+        self.visit_target(buf, true, var);
         buf.writeln(&format!(" = {};", &expr_buf.buf))
     }
 
@@ -1533,19 +1511,20 @@ impl<'a, S: std::hash::BuildHasher> Generator<'a, S> {
         DisplayWrap::Unwrapped
     }
 
-    fn visit_target(&mut self, buf: &mut Buffer, target: &'a Target<'_>) {
-        match *target {
+    fn visit_target(&mut self, buf: &mut Buffer, initialized: bool, target: &Target<'a>) {
+        match target {
             Target::Name(name) => {
                 let name = normalize_identifier(name);
-                self.locals.insert_with_default(name);
+                match initialized {
+                    true => self.locals.insert(name, LocalMeta::initialized()),
+                    false => self.locals.insert_with_default(name),
+                }
                 buf.write(name);
             }
-            Target::Tuple(ref targets) => {
+            Target::Tuple(targets) => {
                 buf.write("(");
                 for name in targets {
-                    let name = normalize_identifier(name);
-                    self.locals.insert_with_default(name);
-                    buf.write(name);
+                    self.visit_target(buf, initialized, &Target::Name(name));
                     buf.write(",");
                 }
                 buf.write(")");


Test patch:
diff --git a/testing/tests/loops.rs b/testing/tests/loops.rs
index 3bba428..7b008a2 100644
--- a/testing/tests/loops.rs
+++ b/testing/tests/loops.rs
@@ -214,3 +214,24 @@ fn test_for_vec_attr_range() {
     };
     assert_eq!(t.render().unwrap(), "1 2 3 4 5 6 ");
 }
+
+#[derive(Template)]
+#[template(
+    source = "{% for v in v %}{% let v = v %}{% for v in v.iterable %}{% let v = v %}{{ v }} {% endfor %}{% endfor %}",
+    ext = "txt"
+)]
+struct ForVecAttrSliceShadowingTemplate {
+    v: Vec<ForVecAttrSlice>,
+}
+
+#[test]
+fn test_for_vec_attr_slice_shadowing() {
+    let t = ForVecAttrSliceShadowingTemplate {
+        v: vec![
+            ForVecAttrSlice { iterable: &[1, 2] },
+            ForVecAttrSlice { iterable: &[3, 4] },
+            ForVecAttrSlice { iterable: &[5, 6] },
+        ],
+    };
+    assert_eq!(t.render().unwrap(), "1 2 3 4 5 6 ");
+}