Repository: djc/askama
Pull request #915 (2023-11-23 11:03:43)

Allow `extends`, `macro` and `import` inside an included template
This makes `include` work [more like in Jinja2 ](https://jinja.palletsprojects.com/en/3.1.x/templates/#include), and closes (#572).
This also improves caching of included templates by storing them inside the `contexts` map.

djc: What is the use case? Why do you want this?
PizzasBear: This allows me to use macros fully inside an included template without having to create another macro file that is associated with the included template and importing it using the same name everywhere I include this template.
GuillaumeGomez: Nice! I'm working on something similar: allowing imports and macro declarations everywhere. So big :+1: for this from me. :)

Do you have some numbers for the impact on compilation performance by any chance?
PizzasBear: No, I didn't measure the performance impact. Though I suspect it wouldn't get much worse because this caches included templates better, but it does clone the `Context` struct for every include.
GuillaumeGomez: Let's see if there is a change already and if there one big enough, we can start optimizing (if @djc is ok with this feature of course).
PizzasBear: I don't know how to measure the performance impact, can you explain me?
GuillaumeGomez: Very ad-hoc way to do it (if nothing comes out from this, I think it's mostly ok): write a file with like 10 templates of different sizes. Then run `cargo build --timings` with and without this PR a few times and compare the median time for both.
PizzasBear: With big files full of `if` statements there doesn't seem to be any difference from `main`.  And including a bunch of big files full of `if`s doesn't seem to show any differences either.
GuillaumeGomez: Can you provide us the code you used to test so we can also confirm please?
PizzasBear: I used the following to generate the templates.
```python
from datetime import datetime


def generate_big_if(num: int, level: int = 0) -> str:
    indent = "    " * level
    if num <= 1:
        return f"{indent}hello {datetime.now()}\n"
    right = num // 2
    left = num - right
    return (
        indent
        + "{% if true %}\n"
        + generate_big_if(left, level + 1)
        + indent
        + "{% else %}\n"
        + generate_big_if(right, level + 1)
        + indent
        + "{% endif %}\n"
    )


def main():
    for i in range(9):
        with open(f"templates/{i}.html", "w+") as f:
            f.write(generate_big_if(int(1.75 ** (i + 3))))


if __name__ == "__main__":
    main()
```
And had this file as well:
```jinja
{% include "1.html" %}
{% include "2.html" %}
{% include "3.html" %}
{% include "4.html" %}
{% include "5.html" %}
{% include "6.html" %}
{% include "7.html" %}
{% include "8.html" %}
{% include "1.html" %}
{% include "2.html" %}
{% include "3.html" %}
{% include "4.html" %}
{% include "5.html" %}
{% include "6.html" %}
{% include "7.html" %}
{% include "8.html" %}
```

And compiled:
```rust
use askama::Template;

#[derive(Template)]
#[template(path = "0.html")]
struct T0;

#[derive(Template)]
#[template(path = "1.html")]
struct T1;

#[derive(Template)]
#[template(path = "2.html")]
struct T2;

#[derive(Template)]
#[template(path = "3.html")]
struct T3;

#[derive(Template)]
#[template(path = "4.html")]
struct T4;

#[derive(Template)]
#[template(path = "5.html")]
struct T5;

#[derive(Template)]
#[template(path = "6.html")]
struct T6;

#[derive(Template)]
#[template(path = "7.html")]
struct T7;

#[derive(Template)]
#[template(path = "8.html")]
struct T8;

#[derive(Template)]
#[template(path = "9.html")]
struct T9;

fn main() {
    println!("Hello, world!");
}
```
GuillaumeGomez: Thanks! Confirming the impact on my side.
GuillaumeGomez: Does it look good to you @djc?
GuillaumeGomez: Ping @djc.
djc: Please squash that change into the originating commit.
PizzasBear: I squashed it

Patch:
diff --git a/askama_derive/src/generator.rs b/askama_derive/src/generator.rs
index c1a8ebe..22f996f 100644
--- a/askama_derive/src/generator.rs
+++ b/askama_derive/src/generator.rs
@@ -1,8 +1,8 @@
 use std::collections::hash_map::{Entry, HashMap};
-use std::path::{Path, PathBuf};
+use std::path::Path;
 use std::{cmp, hash, mem, str};
 
-use crate::config::{get_template_source, WhitespaceHandling};
+use crate::config::WhitespaceHandling;
 use crate::heritage::{Context, Heritage};
 use crate::input::{Source, TemplateInput};
 use crate::CompileError;
@@ -10,7 +10,7 @@ use crate::CompileError;
 use parser::node::{
     Call, Comment, CondTest, If, Include, Let, Lit, Loop, Match, Target, Whitespace, Ws,
 };
-use parser::{Expr, Node, Parsed};
+use parser::{Expr, Node};
 use quote::quote;
 
 pub(crate) struct Generator<'a> {
@@ -20,8 +20,6 @@ pub(crate) struct Generator<'a> {
     contexts: &'a HashMap<&'a Path, Context<'a>>,
     // The heritage contains references to blocks and their ancestry
     heritage: Option<&'a Heritage<'a>>,
-    // Cache ASTs for included templates
-    includes: HashMap<PathBuf, Parsed>,
     // Variables accessible directly from the current scope (not redirected to context)
     locals: MapChain<'a, &'a str, LocalMeta>,
     // Suffix whitespace from the previous literal. Will be flushed to the
@@ -50,7 +48,6 @@ impl<'a> Generator<'a> {
             input,
             contexts,
             heritage,
-            includes: HashMap::default(),
             locals,
             next_ws: None,
             skip_ws: WhitespaceHandling::Preserve,
@@ -846,24 +843,35 @@ impl<'a> Generator<'a> {
             )?;
         }
 
-        // Since nodes must not outlive the Generator, we instantiate a nested `Generator` here to
-        // handle the include's nodes. Unfortunately we can't easily share the `includes` cache.
+        // We clone the context of the child in order to preserve their macros and imports.
+        // But also add all the imports and macros from this template that don't override the
+        // child's ones to preserve this template's context.
+        let child_ctx = &mut self.contexts[path.as_path()].clone();
+        for (name, mac) in &ctx.macros {
+            child_ctx.macros.entry(name).or_insert(mac);
+        }
+        for (name, import) in &ctx.imports {
+            child_ctx
+                .imports
+                .entry(name)
+                .or_insert_with(|| import.clone());
+        }
 
-        let locals = MapChain::with_parent(&self.locals);
-        let mut child = Self::new(self.input, self.contexts, self.heritage, locals);
-
-        let nodes = match self.contexts.get(path.as_path()) {
-            Some(ctx) => ctx.nodes,
-            None => match self.includes.entry(path) {
-                Entry::Occupied(entry) => entry.into_mut().nodes(),
-                Entry::Vacant(entry) => {
-                    let src = get_template_source(entry.key())?;
-                    entry.insert(Parsed::new(src, self.input.syntax)?).nodes()
-                }
-            },
+        // Create a new generator for the child, and call it like in `impl_template` as if it were
+        // a full template, while preserving the context.
+        let heritage = if !child_ctx.blocks.is_empty() || child_ctx.extends.is_some() {
+            Some(Heritage::new(child_ctx, self.contexts))
+        } else {
+            None
         };
 
-        let mut size_hint = child.handle(ctx, nodes, buf, AstLevel::Nested)?;
+        let handle_ctx = match &heritage {
+            Some(heritage) => heritage.root,
+            None => child_ctx,
+        };
+        let locals = MapChain::with_parent(&self.locals);
+        let mut child = Self::new(self.input, self.contexts, heritage.as_ref(), locals);
+        let mut size_hint = child.handle(handle_ctx, handle_ctx.nodes, buf, AstLevel::Top)?;
         size_hint += child.write_buf_writable(buf)?;
         self.prepare_ws(i.ws);
 
diff --git a/askama_derive/src/heritage.rs b/askama_derive/src/heritage.rs
index d75d0a5..bded66b 100644
--- a/askama_derive/src/heritage.rs
+++ b/askama_derive/src/heritage.rs
@@ -35,6 +35,7 @@ impl Heritage<'_> {
 
 type BlockAncestry<'a> = HashMap<&'a str, Vec<(&'a Context<'a>, &'a BlockDef<'a>)>>;
 
+#[derive(Clone)]
 pub(crate) struct Context<'a> {
     pub(crate) nodes: &'a [Node<'a>],
     pub(crate) extends: Option<PathBuf>,
diff --git a/askama_derive/src/input.rs b/askama_derive/src/input.rs
index d10fd7f..215d060 100644
--- a/askama_derive/src/input.rs
+++ b/askama_derive/src/input.rs
@@ -113,32 +113,82 @@ impl TemplateInput<'_> {
         let mut check = vec![(self.path.clone(), source)];
         while let Some((path, source)) = check.pop() {
             let parsed = Parsed::new(source, self.syntax)?;
-            for n in parsed.nodes() {
-                match n {
-                    Node::Extends(extends) => {
-                        let extends = self.config.find_template(extends.path, Some(&path))?;
-                        let dependency_path = (path.clone(), extends.clone());
-                        if dependency_graph.contains(&dependency_path) {
-                            return Err(format!(
-                                "cyclic dependency in graph {:#?}",
-                                dependency_graph
-                                    .iter()
-                                    .map(|e| format!("{:#?} --> {:#?}", e.0, e.1))
-                                    .collect::<Vec<String>>()
-                            )
-                            .into());
+
+            let mut top = true;
+            let mut nested = vec![parsed.nodes()];
+            while let Some(nodes) = nested.pop() {
+                for n in nodes {
+                    let mut add_to_check = |path: PathBuf| -> Result<(), CompileError> {
+                        if !map.contains_key(&path) {
+                            // Add a dummy entry to `map` in order to prevent adding `path`
+                            // multiple times to `check`.
+                            map.insert(path.clone(), Parsed::default());
+                            let source = get_template_source(&path)?;
+                            check.push((path, source));
                         }
-                        dependency_graph.push(dependency_path);
-                        let source = get_template_source(&extends)?;
-                        check.push((extends, source));
-                    }
-                    Node::Import(import) => {
-                        let import = self.config.find_template(import.path, Some(&path))?;
-                        let source = get_template_source(&import)?;
-                        check.push((import, source));
+                        Ok(())
+                    };
+
+                    use Node::*;
+                    match n {
+                        Extends(extends) if top => {
+                            let extends = self.config.find_template(extends.path, Some(&path))?;
+                            let dependency_path = (path.clone(), extends.clone());
+                            if dependency_graph.contains(&dependency_path) {
+                                return Err(format!(
+                                    "cyclic dependency in graph {:#?}",
+                                    dependency_graph
+                                        .iter()
+                                        .map(|e| format!("{:#?} --> {:#?}", e.0, e.1))
+                                        .collect::<Vec<String>>()
+                                )
+                                .into());
+                            }
+                            dependency_graph.push(dependency_path);
+                            add_to_check(extends)?;
+                        }
+                        Macro(m) if top => {
+                            nested.push(&m.nodes);
+                        }
+                        Import(import) if top => {
+                            let import = self.config.find_template(import.path, Some(&path))?;
+                            add_to_check(import)?;
+                        }
+                        Include(include) => {
+                            let include = self.config.find_template(include.path, Some(&path))?;
+                            add_to_check(include)?;
+                        }
+                        BlockDef(b) => {
+                            nested.push(&b.nodes);
+                        }
+                        If(i) => {
+                            for cond in &i.branches {
+                                nested.push(&cond.nodes);
+                            }
+                        }
+                        Loop(l) => {
+                            nested.push(&l.body);
+                            nested.push(&l.else_nodes);
+                        }
+                        Match(m) => {
+                            for arm in &m.arms {
+                                nested.push(&arm.nodes);
+                            }
+                        }
+                        Lit(_)
+                        | Comment(_)
+                        | Expr(_, _)
+                        | Call(_)
+                        | Extends(_)
+                        | Let(_)
+                        | Import(_)
+                        | Macro(_)
+                        | Raw(_)
+                        | Continue(_)
+                        | Break(_) => {}
                     }
-                    _ => {}
                 }
+                top = false;
             }
             map.insert(path, parsed);
         }
diff --git a/askama_parser/src/lib.rs b/askama_parser/src/lib.rs
index 4d27d70..e6f133c 100644
--- a/askama_parser/src/lib.rs
+++ b/askama_parser/src/lib.rs
@@ -28,6 +28,7 @@ mod _parsed {
     use super::node::Node;
     use super::{Ast, ParseError, Syntax};
 
+    #[derive(Default)]
     pub struct Parsed {
         // `source` must outlive `ast`, so `ast` must be declared before `source`
         ast: Ast<'static>,
@@ -68,7 +69,7 @@ mod _parsed {
 
 pub use _parsed::Parsed;
 
-#[derive(Debug)]
+#[derive(Debug, Default)]
 pub struct Ast<'a> {
     nodes: Vec<Node<'a>>,
 }


Test patch:
diff --git a/testing/templates/include-extends-base.html b/testing/templates/include-extends-base.html
new file mode 100644
index 0000000..7a54ca0
--- /dev/null
+++ b/testing/templates/include-extends-base.html
@@ -0,0 +1,6 @@
+<div>
+    <p>Below me is the header</p>
+    {% block header %}{% endblock %}
+    <p>Above me is the header</p>
+</div>
+Hello, {{ name }}!
diff --git a/testing/templates/include-extends-included.html b/testing/templates/include-extends-included.html
new file mode 100644
index 0000000..03b7553
--- /dev/null
+++ b/testing/templates/include-extends-included.html
@@ -0,0 +1,2 @@
+{% extends "include-extends-base.html" %}
+{% block header %}foo{% endblock %}
diff --git a/testing/templates/include-extends.html b/testing/templates/include-extends.html
new file mode 100644
index 0000000..371c133
--- /dev/null
+++ b/testing/templates/include-extends.html
@@ -0,0 +1,4 @@
+<div>
+    <h1>Welcome</h1>
+    {% include "include-extends-included.html" %}
+</div>
diff --git a/testing/templates/include-macro.html b/testing/templates/include-macro.html
new file mode 100644
index 0000000..e29789d
--- /dev/null
+++ b/testing/templates/include-macro.html
@@ -0,0 +1,4 @@
+{% macro m(name) -%}
+    Hello, {{ name }}!
+{%- endmacro -%}
+{% include "included-macro.html" %}
diff --git a/testing/templates/included-macro.html b/testing/templates/included-macro.html
new file mode 100644
index 0000000..efbae18
--- /dev/null
+++ b/testing/templates/included-macro.html
@@ -0,0 +1,6 @@
+{% macro m2(name) -%}
+    Howdy, {{ name }}!
+{%- endmacro -%}
+
+{% call m(name) %}
+{% call m2(name2) %}
diff --git a/testing/tests/include.rs b/testing/tests/include.rs
index f461a7b..c11d96f 100644
--- a/testing/tests/include.rs
+++ b/testing/tests/include.rs
@@ -12,3 +12,44 @@ fn test_include() {
     let s = IncludeTemplate { strs: &strs };
     assert_eq!(s.render().unwrap(), "\n  INCLUDED: foo\n  INCLUDED: bar")
 }
+
+#[derive(Template)]
+#[template(path = "include-extends.html")]
+struct IncludeExtendsTemplate<'a> {
+    name: &'a str,
+}
+
+#[test]
+fn test_include_extends() {
+    let template = IncludeExtendsTemplate { name: "Alice" };
+
+    assert_eq!(
+        template.render().unwrap(),
+        "<div>\n    \
+         <h1>Welcome</h1>\n    \
+         <div>\n    \
+         <p>Below me is the header</p>\n    \
+         foo\n    \
+         <p>Above me is the header</p>\n\
+         </div>\n\
+         Hello, Alice!\n\
+         </div>"
+    );
+}
+
+#[derive(Template)]
+#[template(path = "include-macro.html")]
+struct IncludeMacroTemplate<'a> {
+    name: &'a str,
+    name2: &'a str,
+}
+
+#[test]
+fn test_include_macro() {
+    let template = IncludeMacroTemplate {
+        name: "Alice",
+        name2: "Bob",
+    };
+
+    assert_eq!(template.render().unwrap(), "Hello, Alice!\nHowdy, Bob!");
+}