Repository: rinja-rs/rinja (rust)

Issue:
Variables defined in base template, fail to be accessed in partials included by extended templates
Given the following example:
~~~~
## base.txt
{% let variable = 42 %}
{% block extended %}{% endblock %}
~~~~

~~~~
## extended.txt
{% extends "base.txt" %}
{% block extended %}
WORKS for extended: {{ variable }}
{% include "partial.txt" %}
{% endblock %}
~~~~

~~~~
## partial.txt
FAILS for extended: {{ variable }}
WORKS for includes: {{ variable }}
~~~~

~~~~
## includes.txt
{% let variable = 42 %}
WORKS for includes: {{ variable }}
{% include "partial.txt" %}
~~~~

The `extended.txt` template fails for Rinja with the following error, meanwhile succeeds with Askama (i.e. Rinja regression when migrating from Askama):
~~~~
error[E0609]: no field `variable` on type `&RinjaTestTemplate`
~~~~

Meanwhile, `includes.txt` works for both Askama and Rinja.

----

Even more strangely, if the `partial.txt` is an empty file, thus containing no reference to the variable, just by including it in `extended.txt` triggers the error.

Meanwhile, if the inclusion is missing, `extended.txt` compiles correctly.

----

For some reason the Rinja parser / generator behaves differently for variables defined in a base template, than variables defined in non-inherited templates, but only when that variable is used in partials (i.e. `{% include "partial.txt" %}`).

Thus, at least the scoping rules seem to be applied ambiguously.

Moreover, the current Rinja documentation supports the case, by implying that `{% include ... %}` statements should behave as if they wouldn't be there, and the contents of the included template would be spliced at the place of inclusion:

> The include statement lets you split large or repetitive blocks into separate template files. Included templates get full access to the context in which they're used, including local variables like those from loops:

----

Where would such a use-case be needed:
* one wants to create multiple pages, that each share a common outer layout, thus one creates a `base` template;
* the `base` template is then extended for each `extended` pages;
* the `extended` pages would like to include some common `widget` that are developed as includes (i.e. the `partial.txt` template in this issue);
* the `base` and `widget` "communicate" based on variables, but don't want to involve the `extended` pages, thus the developer defines some variables inside `base` that are to be used in `widget` templates;

At the moment, for the given use-case, I see no workaround that doesn't involve moving and duplicating the variables both in `base` and `extended`.


Pull request description:
Allow variables inheritance in block
Fixes #246.

Patch:
diff --git a/book/update-theme.py b/book/update-theme.py
index 54b4a08b..0a4ad48d 100755
--- a/book/update-theme.py
+++ b/book/update-theme.py
@@ -7,6 +7,7 @@ from io import StringIO
 from json import load
 from os import PathLike
 from pathlib import Path
+from time import sleep
 
 
 INDEX_HBS_DOMAIN = "api.github.com"
@@ -53,12 +54,17 @@ SIDEBAR_END = r"""
 
 
 def update_theme(target: PathLike) -> None:
-    with closing(HTTPSConnection(INDEX_HBS_DOMAIN, INDEX_HBS_PORT)) as conn:
-        conn.request(INDEX_HBS_PROTO, INDEX_HBS_PATH, INDEX_HBS_DATA, INDEX_HBS_HEADERS)
-        res = conn.getresponse()
-        if res.status != 200:
-            raise Exception(f"Status={res.status!r}")
-        data = load(res)
+    for i in reversed(range(3)):
+        with closing(HTTPSConnection(INDEX_HBS_DOMAIN, INDEX_HBS_PORT)) as conn:
+            conn.request(INDEX_HBS_PROTO, INDEX_HBS_PATH, INDEX_HBS_DATA, INDEX_HBS_HEADERS)
+            res = conn.getresponse()
+            if res.status == 200:
+                data = load(res)
+                break
+        if i != 0:
+            sleep(1.0)
+    else:
+        raise Exception(f"Status={res.status!r}")
 
     if data["encoding"] != "base64":
         raise Exception(f'Encoding={data["encoding"]!r}')
diff --git a/rinja_derive/src/generator.rs b/rinja_derive/src/generator.rs
index f512f61a..2a12805e 100644
--- a/rinja_derive/src/generator.rs
+++ b/rinja_derive/src/generator.rs
@@ -3,7 +3,7 @@ use std::collections::hash_map::{Entry, HashMap};
 use std::ops::Deref;
 use std::path::Path;
 use std::sync::Arc;
-use std::{cmp, hash, mem, str};
+use std::{mem, str};
 
 use parser::node::{
     Call, Comment, Cond, CondTest, FilterBlock, If, Include, Let, Lit, Loop, Macro, Match,
@@ -25,7 +25,7 @@ use crate::{BUILT_IN_FILTERS, CRATE, CompileError, FileInfo, MsgValidEscapers};
 pub(crate) fn template_to_string(
     input: &TemplateInput<'_>,
     contexts: &HashMap<&Arc<Path>, Context<'_>, FxBuildHasher>,
-    heritage: Option<&Heritage<'_>>,
+    heritage: Option<&Heritage<'_, '_>>,
 ) -> Result<String, CompileError> {
     let mut buf = Buffer::new();
     template_into_buffer(input, contexts, heritage, &mut buf, false)?;
@@ -35,7 +35,7 @@ pub(crate) fn template_to_string(
 pub(crate) fn template_into_buffer(
     input: &TemplateInput<'_>,
     contexts: &HashMap<&Arc<Path>, Context<'_>, FxBuildHasher>,
-    heritage: Option<&Heritage<'_>>,
+    heritage: Option<&Heritage<'_, '_>>,
     buf: &mut Buffer,
     only_template: bool,
 ) -> Result<(), CompileError> {
@@ -57,15 +57,15 @@ pub(crate) fn template_into_buffer(
     result
 }
 
-struct Generator<'a> {
+struct Generator<'a, 'h> {
     // The template input state: original struct AST and attributes
     input: &'a TemplateInput<'a>,
     // All contexts, keyed by the package-relative template path
     contexts: &'a HashMap<&'a Arc<Path>, Context<'a>, FxBuildHasher>,
     // The heritage contains references to blocks and their ancestry
-    heritage: Option<&'a Heritage<'a>>,
+    heritage: Option<&'h Heritage<'a, 'h>>,
     // Variables accessible directly from the current scope (not redirected to context)
-    locals: MapChain<'a, Cow<'a, str>, LocalMeta>,
+    locals: MapChain<'a>,
     // Suffix whitespace from the previous literal. Will be flushed to the
     // output buffer unless suppressed by whitespace suppression on the next
     // non-literal.
@@ -81,16 +81,16 @@ struct Generator<'a> {
     is_in_filter_block: usize,
 }
 
-impl<'a> Generator<'a> {
-    fn new<'n>(
-        input: &'n TemplateInput<'_>,
-        contexts: &'n HashMap<&'n Arc<Path>, Context<'n>, FxBuildHasher>,
-        heritage: Option<&'n Heritage<'_>>,
-        locals: MapChain<'n, Cow<'n, str>, LocalMeta>,
+impl<'a, 'h> Generator<'a, 'h> {
+    fn new(
+        input: &'a TemplateInput<'a>,
+        contexts: &'a HashMap<&'a Arc<Path>, Context<'a>, FxBuildHasher>,
+        heritage: Option<&'h Heritage<'a, 'h>>,
+        locals: MapChain<'a>,
         buf_writable_discard: bool,
         is_in_filter_block: usize,
-    ) -> Generator<'n> {
-        Generator {
+    ) -> Self {
+        Self {
             input,
             contexts,
             heritage,
@@ -131,12 +131,45 @@ impl<'a> Generator<'a> {
         Ok(())
     }
 
-    fn push_locals<T, F: FnOnce(&mut Self) -> Result<T, CompileError>>(
+    fn push_locals<T, F>(&mut self, callback: F) -> Result<T, CompileError>
+    where
+        F: FnOnce(&mut Self) -> Result<T, CompileError>,
+    {
+        self.locals.scopes.push(HashMap::default());
+        let res = callback(self);
+        self.locals.scopes.pop().unwrap();
+        res
+    }
+
+    fn with_child<'b, T, F>(
         &mut self,
+        heritage: Option<&'b Heritage<'a, 'b>>,
         callback: F,
-    ) -> Result<T, CompileError> {
+    ) -> Result<T, CompileError>
+    where
+        F: FnOnce(&mut Generator<'a, 'b>) -> Result<T, CompileError>,
+    {
         self.locals.scopes.push(HashMap::default());
-        let res = callback(self);
+
+        let buf_writable = mem::take(&mut self.buf_writable);
+        let locals = mem::replace(&mut self.locals, MapChain::new_empty());
+
+        let mut child = Generator::new(
+            self.input,
+            self.contexts,
+            heritage,
+            locals,
+            self.buf_writable.discard,
+            self.is_in_filter_block,
+        );
+        child.buf_writable = buf_writable;
+        let res = callback(&mut child);
+        Generator {
+            locals: self.locals,
+            buf_writable: self.buf_writable,
+            ..
+        } = child;
+
         self.locals.scopes.pop().unwrap();
         res
     }
@@ -304,8 +337,7 @@ impl<'a> Generator<'a> {
     }
 
     fn is_var_defined(&self, var_name: &str) -> bool {
-        self.locals.get(&var_name.into()).is_some()
-            || self.input.fields.iter().any(|f| f == var_name)
+        self.locals.get(var_name).is_some() || self.input.fields.iter().any(|f| f == var_name)
     }
 
     fn evaluate_condition(
@@ -966,17 +998,14 @@ impl<'a> Generator<'a> {
             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,
-            self.buf_writable.discard,
-            self.is_in_filter_block,
-        );
-        let mut size_hint = child.handle(handle_ctx, handle_ctx.nodes, buf, AstLevel::Top)?;
-        size_hint += child.write_buf_writable(handle_ctx, buf)?;
+
+        let size_hint = self.with_child(heritage.as_ref(), |child| {
+            let mut size_hint = 0;
+            size_hint += child.handle(handle_ctx, handle_ctx.nodes, buf, AstLevel::Top)?;
+            size_hint += child.write_buf_writable(handle_ctx, buf)?;
+            Ok(size_hint)
+        })?;
+
         self.prepare_ws(i.ws);
 
         Ok(size_hint)
@@ -991,7 +1020,7 @@ impl<'a> Generator<'a> {
         match var {
             Target::Name(name) => {
                 let name = normalize_identifier(name);
-                match self.locals.get(&Cow::Borrowed(name)) {
+                match self.locals.get(name) {
                     // declares a new variable
                     None => Ok(false),
                     // an initialized variable gets shadowed
@@ -1062,7 +1091,7 @@ impl<'a> Generator<'a> {
         }
         if shadowed
             || !matches!(l.var, Target::Name(_))
-            || matches!(&l.var, Target::Name(name) if self.locals.get(&Cow::Borrowed(name)).is_none())
+            || matches!(&l.var, Target::Name(name) if self.locals.get(name).is_none())
         {
             buf.write("let ");
         }
@@ -1152,30 +1181,21 @@ impl<'a> Generator<'a> {
                 .or_insert_with(|| import.clone());
         }
 
-        let mut child = Self::new(
-            self.input,
-            self.contexts,
-            Some(heritage),
-            // Variables are NOT inherited from the parent scope.
-            MapChain::default(),
-            self.buf_writable.discard,
-            self.is_in_filter_block,
-        );
-        child.buf_writable = mem::take(&mut self.buf_writable);
-
-        // Handle inner whitespace suppression spec and process block nodes
-        child.prepare_ws(def.ws1);
+        let size_hint = self.with_child(Some(heritage), |child| {
+            // Handle inner whitespace suppression spec and process block nodes
+            child.prepare_ws(def.ws1);
 
-        child.super_block = Some(cur);
-        let size_hint = child.handle(&child_ctx, &def.nodes, buf, AstLevel::Block)?;
+            child.super_block = Some(cur);
+            let size_hint = child.handle(&child_ctx, &def.nodes, buf, AstLevel::Block)?;
 
-        if !child.locals.is_current_empty() {
-            // Need to flush the buffer before popping the variable stack
-            child.write_buf_writable(ctx, buf)?;
-        }
+            if !child.locals.is_current_empty() {
+                // Need to flush the buffer before popping the variable stack
+                child.write_buf_writable(ctx, buf)?;
+            }
 
-        child.flush_ws(def.ws2);
-        self.buf_writable = child.buf_writable;
+            child.flush_ws(def.ws2);
+            Ok(size_hint)
+        })?;
 
         // Restore original block context and set whitespace suppression for
         // succeeding whitespace according to the outer WS spec
@@ -2631,7 +2651,7 @@ enum EvaluatedResult {
 }
 
 impl<'a> Conds<'a> {
-    fn compute_branches(generator: &Generator<'a>, i: &'a If<'a>) -> Self {
+    fn compute_branches(generator: &Generator<'a, '_>, i: &'a If<'a>) -> Self {
         let mut conds = Vec::with_capacity(i.branches.len());
         let mut ws_before = None;
         let mut ws_after = None;
@@ -2754,40 +2774,26 @@ impl LocalMeta {
     }
 }
 
-#[derive(Debug, Clone)]
-struct MapChain<'a, K, V>
-where
-    K: cmp::Eq + hash::Hash,
-{
-    parent: Option<&'a MapChain<'a, K, V>>,
-    scopes: Vec<HashMap<K, V, FxBuildHasher>>,
+struct MapChain<'a> {
+    scopes: Vec<HashMap<Cow<'a, str>, LocalMeta, FxBuildHasher>>,
 }
 
-impl<'a, K: 'a, V: 'a> MapChain<'a, K, V>
-where
-    K: cmp::Eq + hash::Hash,
-{
-    fn with_parent<'p>(parent: &'p MapChain<'_, K, V>) -> MapChain<'p, K, V> {
-        MapChain {
-            parent: Some(parent),
-            scopes: vec![HashMap::default()],
-        }
+impl<'a> MapChain<'a> {
+    fn new_empty() -> Self {
+        Self { scopes: vec![] }
     }
 
     /// Iterates the scopes in reverse and returns `Some(LocalMeta)`
     /// from the first scope where `key` exists.
-    fn get(&self, key: &K) -> Option<&V> {
-        let mut scopes = self.scopes.iter().rev();
-        scopes
-            .find_map(|set| set.get(key))
-            .or_else(|| self.parent.and_then(|set| set.get(key)))
+    fn get<'b>(&'b self, key: &str) -> Option<&'b LocalMeta> {
+        self.scopes.iter().rev().find_map(|set| set.get(key))
     }
 
     fn is_current_empty(&self) -> bool {
         self.scopes.last().unwrap().is_empty()
     }
 
-    fn insert(&mut self, key: K, val: V) {
+    fn insert(&mut self, key: Cow<'a, str>, val: LocalMeta) {
         self.scopes.last_mut().unwrap().insert(key, val);
 
         // Note that if `insert` returns `Some` then it implies
@@ -2797,15 +2803,10 @@ where
         // compile error "identifier `a` used more than once".
     }
 
-    fn insert_with_default(&mut self, key: K)
-    where
-        V: Default,
-    {
-        self.insert(key, V::default());
+    fn insert_with_default(&mut self, key: Cow<'a, str>) {
+        self.insert(key, LocalMeta::default());
     }
-}
 
-impl MapChain<'_, Cow<'_, str>, LocalMeta> {
     fn resolve(&self, name: &str) -> Option<String> {
         let name = normalize_identifier(name);
         self.get(&Cow::Borrowed(name)).map(|meta| match &meta.refs {
@@ -2820,10 +2821,9 @@ impl MapChain<'_, Cow<'_, str>, LocalMeta> {
     }
 }
 
-impl<'a, K: Eq + hash::Hash, V> Default for MapChain<'a, K, V> {
+impl Default for MapChain<'_> {
     fn default() -> Self {
         Self {
-            parent: None,
             scopes: vec![HashMap::default()],
         }
     }
diff --git a/rinja_derive/src/heritage.rs b/rinja_derive/src/heritage.rs
index f21f7b11..be65ff1b 100644
--- a/rinja_derive/src/heritage.rs
+++ b/rinja_derive/src/heritage.rs
@@ -9,34 +9,35 @@ use rustc_hash::FxBuildHasher;
 use crate::config::Config;
 use crate::{CompileError, FileInfo};
 
-pub(crate) struct Heritage<'a> {
-    pub(crate) root: &'a Context<'a>,
-    pub(crate) blocks: BlockAncestry<'a>,
+pub(crate) struct Heritage<'a, 'h> {
+    pub(crate) root: &'h Context<'a>,
+    pub(crate) blocks: BlockAncestry<'a, 'h>,
 }
 
-impl Heritage<'_> {
-    pub(crate) fn new<'n>(
-        mut ctx: &'n Context<'n>,
-        contexts: &'n HashMap<&'n Arc<Path>, Context<'n>, FxBuildHasher>,
-    ) -> Heritage<'n> {
-        let mut blocks: BlockAncestry<'n> = ctx
+impl<'a, 'h> Heritage<'a, 'h> {
+    pub(crate) fn new(
+        mut root: &'h Context<'a>,
+        contexts: &'a HashMap<&'a Arc<Path>, Context<'a>, FxBuildHasher>,
+    ) -> Self {
+        let mut blocks: BlockAncestry<'a, 'h> = root
             .blocks
             .iter()
-            .map(|(name, def)| (*name, vec![(ctx, *def)]))
+            .map(|(name, def)| (*name, vec![(root, *def)]))
             .collect();
 
-        while let Some(path) = &ctx.extends {
-            ctx = &contexts[path];
-            for (name, def) in &ctx.blocks {
-                blocks.entry(name).or_default().push((ctx, def));
+        while let Some(path) = &root.extends {
+            root = &contexts[path];
+            for (name, def) in &root.blocks {
+                blocks.entry(name).or_default().push((root, def));
             }
         }
 
-        Heritage { root: ctx, blocks }
+        Self { root, blocks }
     }
 }
 
-type BlockAncestry<'a> = HashMap<&'a str, Vec<(&'a Context<'a>, &'a BlockDef<'a>)>, FxBuildHasher>;
+type BlockAncestry<'a, 'h> =
+    HashMap<&'a str, Vec<(&'h Context<'a>, &'a BlockDef<'a>)>, FxBuildHasher>;
 
 #[derive(Clone)]
 pub(crate) struct Context<'a> {


Test patch:
diff --git a/testing/templates/base-decl.txt b/testing/templates/base-decl.txt
new file mode 100644
index 00000000..1ede5296
--- /dev/null
+++ b/testing/templates/base-decl.txt
@@ -0,0 +1,2 @@
+{%- let variable = 42 -%}
+{%- block extended %}{% endblock -%}
diff --git a/testing/templates/use-var.txt b/testing/templates/use-var.txt
new file mode 100644
index 00000000..ced40d69
--- /dev/null
+++ b/testing/templates/use-var.txt
@@ -0,0 +1,1 @@
+{{ variable }}
diff --git a/testing/tests/block_fragments.rs b/testing/tests/block_fragments.rs
index 15234664..9e1c7ee2 100644
--- a/testing/tests/block_fragments.rs
+++ b/testing/tests/block_fragments.rs
@@ -145,3 +145,21 @@ fn test_fragment_include() {
     let fragment_include = FragmentInclude { s: "world" };
     assert_eq!(fragment_include.render().unwrap(), "\nINCLUDED: world\n");
 }
+
+// This test ensures that parent variables are inherited in the block.
+// This is a regression test for <https://github.com/rinja-rs/rinja/issues/246>.
+#[test]
+fn test_variable_inheritance_in_block() {
+    #[derive(Template)]
+    #[template(
+        source = r#"{% extends "base-decl.txt" %}
+{%- block extended -%}
+--> {{ variable }}
+{% include "use-var.txt" -%}
+{% endblock %}"#,
+        ext = "txt"
+    )]
+    struct Y;
+
+    assert_eq!(Y.render().unwrap(), "--> 42\n42");
+}