Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow extends, macro and import inside an included template #915

Merged
merged 4 commits into from
Jan 17, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 29 additions & 21 deletions askama_derive/src/generator.rs
Original file line number Diff line number Diff line change
@@ -1,16 +1,16 @@
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;

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> {
Expand All @@ -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
Expand Down Expand Up @@ -50,7 +48,6 @@ impl<'a> Generator<'a> {
input,
contexts,
heritage,
includes: HashMap::default(),
locals,
next_ws: None,
skip_ws: WhitespaceHandling::Preserve,
Expand Down Expand Up @@ -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.
djc marked this conversation as resolved.
Show resolved Hide resolved
// 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();
djc marked this conversation as resolved.
Show resolved Hide resolved
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);

Expand Down
1 change: 1 addition & 0 deletions askama_derive/src/heritage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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>,
Expand Down
96 changes: 73 additions & 23 deletions askama_derive/src/input.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
djc marked this conversation as resolved.
Show resolved Hide resolved
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());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand the strategy of proactively inserting a fake entry into map here. Why is that necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parsed variable has to be owned and not mutably borrowed from map because we are going to insert new entries into it. Inserting a dummy entry prevents re-adding the same file multiple times to check before we process it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth adding a code comment to explain it then. ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I add it to the commit: "Improve performance of find_used_templates", or make a separate one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the comment

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

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::*;
djc marked this conversation as resolved.
Show resolved Hide resolved
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);
}
Expand Down
3 changes: 2 additions & 1 deletion askama_parser/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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>,
Expand Down Expand Up @@ -68,7 +69,7 @@ mod _parsed {

pub use _parsed::Parsed;

#[derive(Debug)]
#[derive(Debug, Default)]
pub struct Ast<'a> {
nodes: Vec<Node<'a>>,
}
Expand Down
6 changes: 6 additions & 0 deletions testing/templates/include-extends-base.html
Original file line number Diff line number Diff line change
@@ -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 }}!
2 changes: 2 additions & 0 deletions testing/templates/include-extends-included.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
{% extends "include-extends-base.html" %}
{% block header %}foo{% endblock %}
4 changes: 4 additions & 0 deletions testing/templates/include-extends.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
<div>
<h1>Welcome</h1>
{% include "include-extends-included.html" %}
</div>
4 changes: 4 additions & 0 deletions testing/templates/include-macro.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{% macro m(name) -%}
Hello, {{ name }}!
{%- endmacro -%}
{% include "included-macro.html" %}
6 changes: 6 additions & 0 deletions testing/templates/included-macro.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{% macro m2(name) -%}
Howdy, {{ name }}!
{%- endmacro -%}

{% call m(name) %}
{% call m2(name2) %}
41 changes: 41 additions & 0 deletions testing/tests/include.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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!");
}