Repository: djc/askama
Pull request #1044 (2024-05-15 21:45:33)

 Better error message for `{% macro super %} `


Patch:
diff --git a/askama_derive/src/generator.rs b/askama_derive/src/generator.rs
index 1bb419a..decb01c 100644
--- a/askama_derive/src/generator.rs
+++ b/askama_derive/src/generator.rs
@@ -717,7 +717,7 @@ impl<'a> Generator<'a> {
                 break;
             }
         }
-        let current_buf = mem::take(&mut self.buf_writable);
+        let current_buf = mem::take(&mut self.buf_writable.buf);
 
         self.prepare_ws(filter.ws1);
         let mut size_hint = self.handle(ctx, &filter.nodes, buf, AstLevel::Nested)?;
@@ -746,7 +746,7 @@ impl<'a> Generator<'a> {
             }
         };
 
-        mem::drop(mem::replace(&mut self.buf_writable, current_buf));
+        self.buf_writable.buf = current_buf;
 
         let mut filter_buf = Buffer::new(buf.indent);
         let Filter {
@@ -904,13 +904,6 @@ impl<'a> Generator<'a> {
         name: Option<&'a str>,
         outer: Ws,
     ) -> Result<usize, CompileError> {
-        let block_fragment_write = self.input.block == name && self.buf_writable.discard;
-
-        // Allow writing to the buffer if we're in the block fragment
-        if block_fragment_write {
-            self.buf_writable.discard = false;
-        }
-
         // Flush preceding whitespace according to the outer WS spec
         self.flush_ws(outer);
 
@@ -929,6 +922,15 @@ impl<'a> Generator<'a> {
             (None, None) => return Err("cannot call 'super()' outside block".into()),
         };
 
+        self.write_buf_writable(buf)?;
+
+        let block_fragment_write = self.input.block == name && self.buf_writable.discard;
+        // Allow writing to the buffer if we're in the block fragment
+        if block_fragment_write {
+            self.buf_writable.discard = false;
+        }
+        let prev_buf_discard = mem::replace(&mut buf.discard, self.buf_writable.discard);
+
         // Get the block definition from the heritage chain
         let heritage = self
             .heritage
@@ -973,9 +975,9 @@ impl<'a> Generator<'a> {
             // Need to flush the buffer before popping the variable stack
             child.write_buf_writable(buf)?;
         }
+
         child.flush_ws(def.ws2);
-        let buf_writable = mem::take(&mut child.buf_writable);
-        self.buf_writable = buf_writable;
+        self.buf_writable = child.buf_writable;
 
         // Restore original block context and set whitespace suppression for
         // succeeding whitespace according to the outer WS spec
@@ -985,6 +987,7 @@ impl<'a> Generator<'a> {
         if block_fragment_write {
             self.buf_writable.discard = true;
         }
+        buf.discard = prev_buf_discard;
 
         Ok(size_hint)
     }
@@ -1036,7 +1039,7 @@ impl<'a> Generator<'a> {
             .all(|w| matches!(w, Writable::Lit(_)))
         {
             let mut buf_lit = Buffer::new(0);
-            for s in mem::take(&mut self.buf_writable) {
+            for s in mem::take(&mut self.buf_writable.buf) {
                 if let Writable::Lit(s) = s {
                     buf_lit.write(s);
                 };
@@ -1056,7 +1059,7 @@ impl<'a> Generator<'a> {
         let mut buf_format = Buffer::new(0);
         let mut buf_expr = Buffer::new(indent + 1);
 
-        for s in mem::take(&mut self.buf_writable) {
+        for s in mem::take(&mut self.buf_writable.buf) {
             match s {
                 Writable::Lit(s) => {
                     buf_format.write(&s.replace('{', "{{").replace('}', "}}"));
@@ -1826,6 +1829,7 @@ struct Buffer {
     indent: u8,
     // Whether the output buffer is currently at the start of a line
     start: bool,
+    discard: bool,
 }
 
 impl Buffer {
@@ -1834,10 +1838,14 @@ impl Buffer {
             buf: String::new(),
             indent,
             start: true,
+            discard: false,
         }
     }
 
     fn writeln(&mut self, s: &str) -> Result<(), CompileError> {
+        if self.discard {
+            return Ok(());
+        }
         if s == "}" {
             self.dedent()?;
         }
@@ -1853,6 +1861,9 @@ impl Buffer {
     }
 
     fn write(&mut self, s: &str) {
+        if self.discard {
+            return;
+        }
         if self.start {
             for _ in 0..(self.indent * 4) {
                 self.buf.push(' ');
@@ -1991,16 +2002,17 @@ fn is_copyable(expr: &Expr<'_>) -> bool {
 }
 
 fn is_copyable_within_op(expr: &Expr<'_>, within_op: bool) -> bool {
-    use Expr::*;
     match expr {
-        BoolLit(_) | NumLit(_) | StrLit(_) | CharLit(_) => true,
-        Unary(.., expr) => is_copyable_within_op(expr, true),
-        BinOp(_, lhs, rhs) => is_copyable_within_op(lhs, true) && is_copyable_within_op(rhs, true),
-        Range(..) => true,
+        Expr::BoolLit(_) | Expr::NumLit(_) | Expr::StrLit(_) | Expr::CharLit(_) => true,
+        Expr::Unary(.., expr) => is_copyable_within_op(expr, true),
+        Expr::BinOp(_, lhs, rhs) => {
+            is_copyable_within_op(lhs, true) && is_copyable_within_op(rhs, true)
+        }
+        Expr::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.
-        Call(..) | Path(..) => true,
+        Expr::Call(..) | Expr::Path(..) => 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()`
@@ -2131,15 +2143,6 @@ impl<'a> WritableBuffer<'a> {
     }
 }
 
-impl<'a> IntoIterator for WritableBuffer<'a> {
-    type Item = Writable<'a>;
-    type IntoIter = <Vec<Writable<'a>> as IntoIterator>::IntoIter;
-
-    fn into_iter(self) -> Self::IntoIter {
-        self.buf.into_iter()
-    }
-}
-
 impl<'a> Deref for WritableBuffer<'a> {
     type Target = [Writable<'a>];
 
diff --git a/askama_derive/src/input.rs b/askama_derive/src/input.rs
index 989db1d..10b9927 100644
--- a/askama_derive/src/input.rs
+++ b/askama_derive/src/input.rs
@@ -138,9 +138,8 @@ impl TemplateInput<'_> {
                         Ok(())
                     };
 
-                    use Node::*;
                     match n {
-                        Extends(extends) if top => {
+                        Node::Extends(extends) if top => {
                             let extends = self.config.find_template(extends.path, Some(&path))?;
                             let dependency_path = (path.clone(), extends.clone());
                             if path == extends {
@@ -153,48 +152,48 @@ impl TemplateInput<'_> {
                             dependency_graph.push(dependency_path);
                             add_to_check(extends)?;
                         }
-                        Macro(m) if top => {
+                        Node::Macro(m) if top => {
                             nested.push(&m.nodes);
                         }
-                        Import(import) if top => {
+                        Node::Import(import) if top => {
                             let import = self.config.find_template(import.path, Some(&path))?;
                             add_to_check(import)?;
                         }
-                        FilterBlock(f) => {
+                        Node::FilterBlock(f) => {
                             nested.push(&f.nodes);
                         }
-                        Include(include) => {
+                        Node::Include(include) => {
                             let include = self.config.find_template(include.path, Some(&path))?;
                             add_to_check(include)?;
                         }
-                        BlockDef(b) => {
+                        Node::BlockDef(b) => {
                             nested.push(&b.nodes);
                         }
-                        If(i) => {
+                        Node::If(i) => {
                             for cond in &i.branches {
                                 nested.push(&cond.nodes);
                             }
                         }
-                        Loop(l) => {
+                        Node::Loop(l) => {
                             nested.push(&l.body);
                             nested.push(&l.else_nodes);
                         }
-                        Match(m) => {
+                        Node::Match(m) => {
                             for arm in &m.arms {
                                 nested.push(&arm.nodes);
                             }
                         }
-                        Lit(_)
-                        | Comment(_)
-                        | Expr(_, _)
-                        | Call(_)
-                        | Extends(_)
-                        | Let(_)
-                        | Import(_)
-                        | Macro(_)
-                        | Raw(_)
-                        | Continue(_)
-                        | Break(_) => {}
+                        Node::Lit(_)
+                        | Node::Comment(_)
+                        | Node::Expr(_, _)
+                        | Node::Call(_)
+                        | Node::Extends(_)
+                        | Node::Let(_)
+                        | Node::Import(_)
+                        | Node::Macro(_)
+                        | Node::Raw(_)
+                        | Node::Continue(_)
+                        | Node::Break(_) => {}
                     }
                 }
                 top = false;
@@ -392,12 +391,11 @@ impl FromStr for Print {
     type Err = CompileError;
 
     fn from_str(s: &str) -> Result<Print, Self::Err> {
-        use self::Print::*;
         Ok(match s {
-            "all" => All,
-            "ast" => Ast,
-            "code" => Code,
-            "none" => None,
+            "all" => Print::All,
+            "ast" => Print::Ast,
+            "code" => Print::Code,
+            "none" => Print::None,
             v => return Err(format!("invalid value for print option: {v}",).into()),
         })
     }
diff --git a/askama_parser/src/node.rs b/askama_parser/src/node.rs
index 545037a..064ecd7 100644
--- a/askama_parser/src/node.rs
+++ b/askama_parser/src/node.rs
@@ -7,7 +7,7 @@ use nom::character::complete::char;
 use nom::combinator::{
     complete, consumed, cut, eof, map, map_res, not, opt, peek, recognize, value,
 };
-use nom::error::{Error, ErrorKind};
+use nom::error::ErrorKind;
 use nom::error_position;
 use nom::multi::{fold_many0, many0, many1, separated_list0, separated_list1};
 use nom::sequence::{delimited, pair, preceded, terminated, tuple};
@@ -87,7 +87,10 @@ impl<'a> Node<'a> {
         ));
         let (j, (pws, _, nws)) = p(i)?;
         if !s.is_in_loop() {
-            return Err(nom::Err::Failure(error_position!(i, ErrorKind::Tag)));
+            return Err(nom::Err::Failure(ErrorContext {
+                input: i,
+                message: Some(Cow::Borrowed("you can only `break` inside a `for` loop")),
+            }));
         }
         Ok((j, Self::Break(Ws(pws, nws))))
     }
@@ -100,7 +103,10 @@ impl<'a> Node<'a> {
         ));
         let (j, (pws, _, nws)) = p(i)?;
         if !s.is_in_loop() {
-            return Err(nom::Err::Failure(error_position!(i, ErrorKind::Tag)));
+            return Err(nom::Err::Failure(ErrorContext {
+                input: i,
+                message: Some(Cow::Borrowed("you can only `continue` inside a `for` loop")),
+            }));
         }
         Ok((j, Self::Continue(Ws(pws, nws))))
     }
@@ -520,7 +526,13 @@ impl<'a> Macro<'a> {
                 |i| s.tag_block_end(i),
             ))),
         ));
-        let (i, (pws1, _, (name, params, nws1, _))) = start(i)?;
+        let (j, (pws1, _, (name, params, nws1, _))) = start(i)?;
+        if name == "super" {
+            return Err(nom::Err::Failure(ErrorContext {
+                input: i,
+                message: Some(Cow::Borrowed("'super' is not a valid name for a macro")),
+            }));
+        }
 
         let mut end = cut(tuple((
             |i| Node::many(i, s),
@@ -537,15 +549,7 @@ impl<'a> Macro<'a> {
                 )),
             ))),
         )));
-        let (i, (contents, (_, pws2, _, nws2))) = end(i)?;
-
-        if name == "super" {
-            // TODO: yield a a better error message here
-            return Err(ErrorContext::from_err(nom::Err::Failure(Error::new(
-                i,
-                ErrorKind::Fail,
-            ))));
-        }
+        let (i, (contents, (_, pws2, _, nws2))) = end(j)?;
 
         Ok((
             i,


Test patch:
diff --git a/testing/templates/blocks.txt b/testing/templates/blocks.txt
new file mode 100644
index 0000000..2272d82
--- /dev/null
+++ b/testing/templates/blocks.txt
@@ -0,0 +1,11 @@
+{% block index %}
+Section: {{ s1 }}
+{% endblock %}
+
+{% block section -%}
+  [
+  {%- for value in  values -%}
+    {{ value }}
+  {%- endfor -%}
+  ]
+{%- endblock %}
diff --git a/testing/tests/block_fragments.rs b/testing/tests/block_fragments.rs
index b84a511..ae723b6 100644
--- a/testing/tests/block_fragments.rs
+++ b/testing/tests/block_fragments.rs
@@ -11,7 +11,7 @@ struct FragmentSimple<'a> {
 fn test_fragment_simple() {
     let simple = FragmentSimple { name: "world" };
 
-    assert_eq!(simple.render().unwrap(), "\n\n<p>Hello world!</p>\n");
+    assert_eq!(simple.render().unwrap(), "\n<p>Hello world!</p>\n");
 }
 
 #[derive(Template)]
@@ -28,7 +28,7 @@ fn test_fragment_super() {
 
     assert_eq!(
         sup.render().unwrap(),
-        "\n\n<p>Hello world!</p>\n\n<p>Parent body content</p>\n\n"
+        "\n<p>Hello world!</p>\n\n<p>Parent body content</p>\n\n"
     );
 }
 
@@ -43,7 +43,7 @@ fn test_fragment_nested_block() {
 
     assert_eq!(
         nested_block.render().unwrap(),
-        "\n\n<p>I should be here.</p>\n"
+        "\n<p>I should be here.</p>\n"
     );
 }
 
@@ -61,7 +61,7 @@ fn test_fragment_nested_super() {
 
     assert_eq!(
         nested_sup.render().unwrap(),
-        "\n\n<p>Hello world!</p>\n\n[\n<p>Parent body content</p>\n]\n\n"
+        "\n<p>Hello world!</p>\n\n[\n<p>Parent body content</p>\n]\n\n"
     );
 }
 
@@ -79,5 +79,27 @@ fn test_fragment_unused_expression() {
         required: "Required",
     };
 
-    assert_eq!(unused_expr.render().unwrap(), "\n\n<p>Required</p>\n");
+    assert_eq!(unused_expr.render().unwrap(), "\n<p>Required</p>\n");
+}
+
+#[derive(Template)]
+#[template(path = "blocks.txt", block = "index")]
+struct RenderInPlace<'a> {
+    s1: Section<'a>,
+}
+
+#[derive(Template)]
+#[template(path = "blocks.txt", block = "section")]
+struct Section<'a> {
+    values: &'a [&'a str],
+}
+
+#[test]
+fn test_specific_block() {
+    let s1 = Section {
+        values: &["a", "b", "c"],
+    };
+    assert_eq!(s1.render().unwrap(), "[abc]");
+    let t = RenderInPlace { s1 };
+    assert_eq!(t.render().unwrap(), "\nSection: [abc]\n");
 }
diff --git a/testing/tests/inheritance.rs b/testing/tests/inheritance.rs
index 87232b3..926008f 100644
--- a/testing/tests/inheritance.rs
+++ b/testing/tests/inheritance.rs
@@ -329,14 +329,9 @@ fn test_flat_deep() {
 
 #[derive(Template)]
 #[template(path = "let-base.html")]
+#[allow(dead_code)]
 struct LetBase {}
 
-#[test]
-fn test_let_base() {
-    let t = LetBase {};
-    assert_eq!(t.render().unwrap(), "");
-}
-
 #[derive(Template)]
 #[template(path = "let-child.html")]
 struct LetChild {}
diff --git a/testing/tests/simple.rs b/testing/tests/simple.rs
index 4012128..a8ecb76 100644
--- a/testing/tests/simple.rs
+++ b/testing/tests/simple.rs
@@ -345,6 +345,7 @@ struct FunctionTemplate;
 
 impl FunctionTemplate {
     #[allow(clippy::trivially_copy_pass_by_ref)]
+    #[allow(dead_code)]
     fn world3(&self, s: &str, v: u8) -> String {
         format!("world{s}{v}")
     }
diff --git a/testing/tests/ui/break_outside_of_loop.stderr b/testing/tests/ui/break_outside_of_loop.stderr
index 132421a..96aa54b 100644
--- a/testing/tests/ui/break_outside_of_loop.stderr
+++ b/testing/tests/ui/break_outside_of_loop.stderr
@@ -1,4 +1,5 @@
-error: failed to parse template source at row 1, column 9 near:
+error: you can only `break` inside a `for` loop
+       failed to parse template source at row 1, column 9 near:
        "break%}, have a parsing error!"
  --> tests/ui/break_outside_of_loop.rs:3:10
   |
diff --git a/testing/tests/ui/macro-super.rs b/testing/tests/ui/macro-super.rs
new file mode 100644
index 0000000..ec57800
--- /dev/null
+++ b/testing/tests/ui/macro-super.rs
@@ -0,0 +1,8 @@
+use askama::Template;
+
+#[derive(Template)]
+#[template(source = "{%- macro super() -%}{%- endmacro -%}", ext = "html")]
+struct MacroSuper;
+
+fn main() {
+}
diff --git a/testing/tests/ui/macro-super.stderr b/testing/tests/ui/macro-super.stderr
new file mode 100644
index 0000000..2437cdf
--- /dev/null
+++ b/testing/tests/ui/macro-super.stderr
@@ -0,0 +1,9 @@
+error: 'super' is not a valid name for a macro
+       failed to parse template source at row 1, column 2 near:
+       "- macro super() -%}{%- endmacro -%}"
+ --> tests/ui/macro-super.rs:3:10
+  |
+3 | #[derive(Template)]
+  |          ^^^^^^^^
+  |
+  = note: this error originates in the derive macro `Template` (in Nightly builds, run with -Z macro-backtrace for more info)