Repository: mitsuhiko/minijinja (language: rust)

Issue:
`{% macro %}` hoisting
Hey there,
thanks for your efforts maintaining and sharing this with us.

I'm currently attempting to migrate a huge codebase of Jinja2 compatible (Twig) templates to Minijinja, and macros seem to not be hoisted. 

Consider the following template:

```jinja
{% macro defined_first() %}
  <h1>defined_first</h1>
{% endmacro %}

{% macro defined_second() %}
  {{ defined_first() }}
  <h1>defined_second</h1>
  {{ defined_third() }}
{% endmacro %}

{% macro defined_third() %}
  <h1>defined_third</h1>
{% endmacro %}

{{ defined_second() }}

```

## Actual result – failure
```
Error: invalid operation: value of type undefined is not callable (in template.html:8)
-------------------------------- template.html --------------------------------
   5 | {% macro defined_second() %}
   6 |   {{ defined_first() }}
   7 |   <h1>defined_second</h1>
   8 >   {{ defined_third() }}
     i      ^^^^^^^^^^^^^^^ invalid operation
   9 | {% endmacro %}
  10 | 
  11 | {% macro defined_third() %}
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
```

## Expected result – rendered template (like @ Jinja2)

```html
  <h1>defined_first</h1>

  <h1>defined_second</h1>
  
  <h1>defined_third</h1>
```


Please let me know if this is intentional and isn't going to be changed. 
Otherwise I'd be happy to attempt submitting a PR (in case this isn't a fundamental architectural limitation).


Discussion / hints:
This is an effect of how the macro closure system is currently implemented and clearly a bug. Macros in Jinja2 are not hoisted either, but they pull in the entire enclosing scope, whereas MiniJinja pulls in only the referenced scoped at the time of the macro declaration.

This should be resolved but I need to investigate what sensible semantics would be here. Happy to accept a patch but I will likely also address this myself.

Fix patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 736e0a3d..faf34d23 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -2,6 +2,10 @@
 
 All notable changes to MiniJinja are documented here.
 
+## 0.31.0
+
+- Changed the closure behavior of macros to match the one of Jinja2. (#233)
+
 ## 0.30.7
 
 - Added `testutils` module. (#221)
diff --git a/minijinja/src/compiler/codegen.rs b/minijinja/src/compiler/codegen.rs
index 00fb07f4..f7c3ca39 100644
--- a/minijinja/src/compiler/codegen.rs
+++ b/minijinja/src/compiler/codegen.rs
@@ -377,7 +377,7 @@ impl<'source> CodeGenerator<'source> {
 
     #[cfg(feature = "macros")]
     fn compile_macro_expression(&mut self, macro_decl: &ast::Spanned<ast::Macro<'source>>) {
-        use crate::compiler::instructions::{MACRO_CALLER, MACRO_SELF_REFERENTIAL};
+        use crate::compiler::instructions::MACRO_CALLER;
         use crate::value::ValueRepr;
         self.set_line_from_span(macro_decl.span());
         let instr = self.add(Instruction::Jump(!0));
@@ -398,14 +398,12 @@ impl<'source> CodeGenerator<'source> {
         }
         self.add(Instruction::Return);
         let mut undeclared = crate::compiler::meta::find_macro_closure(macro_decl);
-        let self_reference = undeclared.contains(macro_decl.name);
         let caller_reference = undeclared.remove("caller");
         let macro_instr = self.next_instruction();
         for name in &undeclared {
-            self.add(Instruction::LoadConst(Value::from(*name)));
-            self.add(Instruction::Lookup(name));
+            self.add(Instruction::Enclose(name));
         }
-        self.add(Instruction::BuildMap(undeclared.len()));
+        self.add(Instruction::GetClosure);
         self.add(Instruction::LoadConst(Value::from(ValueRepr::Seq(
             macro_decl
                 .args
@@ -418,9 +416,6 @@ impl<'source> CodeGenerator<'source> {
                 .into(),
         ))));
         let mut flags = 0;
-        if self_reference {
-            flags |= MACRO_SELF_REFERENTIAL;
-        }
         if caller_reference {
             flags |= MACRO_CALLER;
         }
diff --git a/minijinja/src/compiler/instructions.rs b/minijinja/src/compiler/instructions.rs
index 69043811..410cb259 100644
--- a/minijinja/src/compiler/instructions.rs
+++ b/minijinja/src/compiler/instructions.rs
@@ -11,10 +11,6 @@ pub const LOOP_FLAG_WITH_LOOP_VAR: u8 = 1;
 /// This loop is recursive.
 pub const LOOP_FLAG_RECURSIVE: u8 = 2;
 
-/// This macro is self referential.
-#[cfg(feature = "macros")]
-pub const MACRO_SELF_REFERENTIAL: u8 = 1;
-
 /// This macro uses the caller var.
 #[cfg(feature = "macros")]
 pub const MACRO_CALLER: u8 = 2;
@@ -230,6 +226,14 @@ pub enum Instruction<'source> {
     /// True if the value is undefined
     #[cfg(feature = "macros")]
     IsUndefined,
+
+    /// Encloses a variable.
+    #[cfg(feature = "macros")]
+    Enclose(&'source str),
+
+    /// Returns the closure of this context level.
+    #[cfg(feature = "macros")]
+    GetClosure,
 }
 
 #[derive(Copy, Clone)]
diff --git a/minijinja/src/vm/closure_object.rs b/minijinja/src/vm/closure_object.rs
new file mode 100644
index 00000000..998dfe9a
--- /dev/null
+++ b/minijinja/src/vm/closure_object.rs
@@ -0,0 +1,65 @@
+use std::collections::BTreeMap;
+use std::fmt;
+use std::sync::{Arc, Mutex};
+
+use crate::key::{Key, StaticKey};
+use crate::value::{intern, Object, ObjectKind, StructObject, Value};
+
+/// Utility to enclose values for macros.
+///
+/// See `closure` on the [`Frame`] for how it's used.
+#[derive(Debug, Default)]
+pub(crate) struct Closure {
+    values: Mutex<BTreeMap<StaticKey, Value>>,
+}
+
+impl Closure {
+    /// Stores a value by key in the closure.
+    pub fn store(&self, key: &str, value: Value) {
+        self.values
+            .lock()
+            .unwrap()
+            .insert(StaticKey::from(key), value);
+    }
+
+    /// Upset a value into the closure.
+    #[cfg(feature = "macros")]
+    pub fn store_if_missing<F: FnOnce() -> Value>(&self, key: &str, f: F) {
+        let mut values = self.values.lock().unwrap();
+        if !values.contains_key(&Key::Str(key)) {
+            values.insert(key.into(), f());
+        }
+    }
+}
+
+impl fmt::Display for Closure {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        let mut m = f.debug_map();
+        for (key, value) in self.values.lock().unwrap().iter() {
+            m.entry(&key, &value);
+        }
+        m.finish()
+    }
+}
+
+impl Object for Closure {
+    fn kind(&self) -> ObjectKind<'_> {
+        ObjectKind::Struct(self)
+    }
+}
+
+impl StructObject for Closure {
+    fn fields(&self) -> Vec<Arc<String>> {
+        self.values
+            .lock()
+            .unwrap()
+            .keys()
+            .filter_map(|x| x.as_str())
+            .map(intern)
+            .collect()
+    }
+
+    fn get_field(&self, name: &str) -> Option<Value> {
+        self.values.lock().unwrap().get(&Key::Str(name)).cloned()
+    }
+}
diff --git a/minijinja/src/vm/context.rs b/minijinja/src/vm/context.rs
index fb057755..6d606e93 100644
--- a/minijinja/src/vm/context.rs
+++ b/minijinja/src/vm/context.rs
@@ -6,6 +6,7 @@ use std::sync::Arc;
 use crate::environment::Environment;
 use crate::error::{Error, ErrorKind};
 use crate::value::{OwnedValueIterator, Value};
+use crate::vm::closure_object::Closure;
 use crate::vm::loop_object::Loop;
 
 type Locals<'env> = BTreeMap<&'env str, Value>;
@@ -31,6 +32,13 @@ pub(crate) struct Frame<'env> {
     pub(crate) locals: Locals<'env>,
     pub(crate) ctx: Value,
     pub(crate) current_loop: Option<LoopState>,
+
+    // normally a frame does not carry a closure, but it can when a macro is
+    // declared.  Once that happens, all writes to the frames locals are also
+    // duplicated into the closure.  Macros declared on that level, then share
+    // the closure object to enclose the parent values.  This emulates the
+    // behavior of closures in Jinja2.
+    pub(crate) closure: Option<Arc<Closure>>,
 }
 
 impl<'env> Default for Frame<'env> {
@@ -45,6 +53,7 @@ impl<'env> Frame<'env> {
             locals: Locals::new(),
             ctx,
             current_loop: None,
+            closure: None,
         }
     }
 }
@@ -184,7 +193,51 @@ impl<'env> Context<'env> {
 
     /// Stores a variable in the context.
     pub fn store(&mut self, key: &'env str, value: Value) {
-        self.stack.last_mut().unwrap().locals.insert(key, value);
+        let top = self.stack.last_mut().unwrap();
+        if let Some(ref closure) = top.closure {
+            closure.store(key, value.clone());
+        }
+        top.locals.insert(key, value);
+    }
+
+    /// Adds a value to a closure if missing.
+    ///
+    /// All macros declare on a certain level reuse the same closure.  This is done
+    /// to emulate the behavior of how scopes work in Jinja2 in Python.  The
+    /// unfortunate downside is that this has to be done with a `Mutex`.
+    #[cfg(feature = "macros")]
+    pub fn enclose(&mut self, env: &Environment, key: &str) {
+        self.closure()
+            .store_if_missing(key, || self.load(env, key).unwrap_or(Value::UNDEFINED));
+    }
+
+    /// Loads the closure and returns it.
+    #[cfg(feature = "macros")]
+    pub fn closure(&mut self) -> Arc<Closure> {
+        let top = self.stack.last_mut().unwrap();
+        top.closure
+            .get_or_insert_with(|| Arc::new(Closure::default()))
+            .clone()
+    }
+
+    /// Temporarily takes the closure.
+    ///
+    /// This is done because includes are in the same scope as the module that
+    /// triggers the import, but we do not want to allow closures to be modified
+    /// from another file as this would be very confusing.
+    ///
+    /// This means that if you override a variable referenced by a macro after
+    /// including in the parent template, it will not override the value seen by
+    /// the macro.
+    #[cfg(feature = "multi_template")]
+    pub fn take_closure(&mut self) -> Option<Arc<Closure>> {
+        self.stack.last_mut().unwrap().closure.take()
+    }
+
+    /// Puts the closure back.
+    #[cfg(feature = "multi_template")]
+    pub fn reset_closure(&mut self, closure: Option<Arc<Closure>>) {
+        self.stack.last_mut().unwrap().closure = closure;
     }
 
     /// Looks up a variable in the context.
diff --git a/minijinja/src/vm/macro_object.rs b/minijinja/src/vm/macro_object.rs
index b0b829ed..974a1213 100644
--- a/minijinja/src/vm/macro_object.rs
+++ b/minijinja/src/vm/macro_object.rs
@@ -19,7 +19,6 @@ pub(crate) struct MacroData {
     // state under `state.macros`.
     pub macro_ref_id: usize,
     pub closure: Value,
-    pub self_reference: bool,
     pub caller_reference: bool,
 }
 
@@ -85,8 +84,8 @@ impl Object for Macro {
             kwargs_used.insert("caller");
             Some(
                 kwargs
-                    .and_then(|x| x.get(&Key::Str("caller")))
-                    .unwrap_or(&Value::UNDEFINED),
+                    .and_then(|x| x.get(&Key::Str("caller")).cloned())
+                    .unwrap_or(Value::UNDEFINED),
             )
         } else {
             None
@@ -112,27 +111,7 @@ impl Object for Macro {
         // there.  Unfortunately because we only have a &self reference here, we
         // cannot bump our own refcount.  Instead we need to wrap the macro data
         // into an extra level of Arc to avoid unnecessary clones.
-        let closure = if self.data.self_reference || caller.is_some() {
-            if let ValueRepr::Map(ref map, kind) = self.data.closure.0 {
-                let mut map = map.clone();
-                if self.data.self_reference {
-                    Arc::make_mut(&mut map).insert(
-                        Key::String(self.data.name.clone()),
-                        Value::from_object(Macro {
-                            data: self.data.clone(),
-                        }),
-                    );
-                }
-                if let Some(caller) = caller {
-                    Arc::make_mut(&mut map).insert(Key::Str("caller"), caller.clone());
-                }
-                ValueRepr::Map(map, kind).into()
-            } else {
-                unreachable!();
-            }
-        } else {
-            self.data.closure.clone()
-        };
+        let closure = self.data.closure.clone();
 
         // This requires some explanation here.  Because we get the state as &State and
         // not &mut State we are required to create a new state here.  This is unfortunate
@@ -140,7 +119,15 @@ impl Object for Macro {
         // Because macros cannot return anything other than strings (most importantly they)
         // can't return other macros this is however not an issue, as modifications in the
         // macro cannot leak out.
-        ok!(vm.eval_macro(instructions, *offset, closure, &mut out, state, arg_values));
+        ok!(vm.eval_macro(
+            instructions,
+            *offset,
+            closure,
+            caller,
+            &mut out,
+            state,
+            arg_values
+        ));
 
         Ok(if !matches!(state.auto_escape(), AutoEscape::None) {
             Value::from_safe_string(rv)
diff --git a/minijinja/src/vm/mod.rs b/minijinja/src/vm/mod.rs
index a6888ceb..9104d49e 100644
--- a/minijinja/src/vm/mod.rs
+++ b/minijinja/src/vm/mod.rs
@@ -23,6 +23,7 @@ use crate::vm::fuel::FuelTracker;
 
 pub use crate::vm::state::State;
 
+mod closure_object;
 mod context;
 #[cfg(feature = "fuel")]
 mod fuel;
@@ -107,16 +108,21 @@ impl<'env> Vm<'env> {
 
     /// Evaluate a macro in a state.
     #[cfg(feature = "macros")]
+    #[allow(clippy::too_many_arguments)]
     pub fn eval_macro(
         &self,
         instructions: &Instructions<'env>,
         pc: usize,
-        root: Value,
+        closure: Value,
+        caller: Option<Value>,
         out: &mut Output,
         state: &State,
         args: Vec<Value>,
     ) -> Result<Option<Value>, Error> {
-        let mut ctx = Context::new(Frame::new(root));
+        let mut ctx = Context::new(Frame::new(closure));
+        if let Some(caller) = caller {
+            ctx.store("caller", caller);
+        }
         ok!(ctx.incr_depth(state.ctx.depth() + MACRO_RECURSION_COST));
         self.eval_impl(
             &mut State {
@@ -645,6 +651,14 @@ impl<'env> Vm<'env> {
                 }
                 #[cfg(feature = "macros")]
                 Instruction::Return => break,
+                #[cfg(feature = "macros")]
+                Instruction::Enclose(name) => {
+                    state.ctx.enclose(state.env, name);
+                }
+                #[cfg(feature = "macros")]
+                Instruction::GetClosure => {
+                    stack.push(Value::from(state.ctx.closure()));
+                }
             }
             pc += 1;
         }
@@ -689,8 +703,10 @@ impl<'env> Vm<'env> {
             let old_escape = mem::replace(&mut state.auto_escape, tmpl.initial_auto_escape());
             let old_instructions = mem::replace(&mut state.instructions, tmpl.instructions());
             let old_blocks = mem::replace(&mut state.blocks, prepare_blocks(tmpl.blocks()));
+            let old_closure = state.ctx.take_closure();
             ok!(state.ctx.incr_depth(INCLUDE_RECURSION_COST));
             let rv = self.eval_state(state, out);
+            state.ctx.reset_closure(old_closure);
             state.ctx.decr_depth(INCLUDE_RECURSION_COST);
             state.auto_escape = old_escape;
             state.instructions = old_instructions;
@@ -904,7 +920,7 @@ impl<'env> Vm<'env> {
         name: &str,
         flags: u8,
     ) {
-        use crate::compiler::instructions::{MACRO_CALLER, MACRO_SELF_REFERENTIAL};
+        use crate::compiler::instructions::MACRO_CALLER;
 
         let arg_spec = match stack.pop().0 {
             ValueRepr::Seq(args) => args
@@ -925,7 +941,6 @@ impl<'env> Vm<'env> {
                 arg_spec,
                 macro_ref_id,
                 closure,
-                self_reference: (flags & MACRO_SELF_REFERENTIAL) != 0,
                 caller_reference: (flags & MACRO_CALLER) != 0,
             }),
         }));


Test patch:
diff --git a/minijinja/tests/inputs/do_closure.txt b/minijinja/tests/inputs/do_closure.txt
index 20fdf6fe..548e31bd 100644
--- a/minijinja/tests/inputs/do_closure.txt
+++ b/minijinja/tests/inputs/do_closure.txt
@@ -2,5 +2,4 @@
 ---
 {% macro other_macro() %}23{% endmacro %}
 {% macro foo() %}[{% do other_macro() %}]{% endmacro %}
-{% set other_macro = 42 %}
 Should not show output of other_macro: {{ foo() }}
\ No newline at end of file
diff --git a/minijinja/tests/inputs/macro_closure_behavior.txt b/minijinja/tests/inputs/macro_closure_behavior.txt
new file mode 100644
index 00000000..dba47866
--- /dev/null
+++ b/minijinja/tests/inputs/macro_closure_behavior.txt
@@ -0,0 +1,10 @@
+{}
+---
+{%- macro before_closure() %}{{ closure }}{% endmacro %}
+{%- set closure = 1 %}
+{%- macro after_closure() %}{{ closure }}{% endmacro %}
+{%- set closure = 2 %}
+{%- macro after_closure_reset() %}{{ closure }}{% endmacro %}
+{{ before_closure() }}
+{{ after_closure() }}
+{{ after_closure_reset() }}
\ No newline at end of file
diff --git a/minijinja/tests/inputs/macro_hoisting.txt b/minijinja/tests/inputs/macro_hoisting.txt
new file mode 100644
index 00000000..49412f77
--- /dev/null
+++ b/minijinja/tests/inputs/macro_hoisting.txt
@@ -0,0 +1,5 @@
+{}
+---
+{% macro first(param) %}first: {{ second(param )}}{% endmacro %}
+{% macro second(param) %}second: {{ param }}{% endmacro %}
+{{ first(42) }}
\ No newline at end of file
diff --git a/minijinja/tests/inputs/macro_recursive_alias.txt b/minijinja/tests/inputs/macro_recursive_alias.txt
index 2e01b2ad..11fdac9a 100644
--- a/minijinja/tests/inputs/macro_recursive_alias.txt
+++ b/minijinja/tests/inputs/macro_recursive_alias.txt
@@ -28,5 +28,4 @@
     </nest>
 {%- endmacro -%}
 {% set not_nest = nest %}
-{% set nest = "not callable" %}
 {{ not_nest(top_level) }}
diff --git a/minijinja/tests/snapshots/test_templates__vm@do_closure.txt.snap b/minijinja/tests/snapshots/test_templates__vm@do_closure.txt.snap
index 5edc374b..08d8d901 100644
--- a/minijinja/tests/snapshots/test_templates__vm@do_closure.txt.snap
+++ b/minijinja/tests/snapshots/test_templates__vm@do_closure.txt.snap
@@ -1,11 +1,10 @@
 ---
 source: minijinja/tests/test_templates.rs
-description: "{% macro other_macro() %}23{% endmacro %}\n{% macro foo() %}[{% do other_macro() %}]{% endmacro %}\n{% set other_macro = 42 %}\nShould not show output of other_macro: {{ foo() }}"
+description: "{% macro other_macro() %}23{% endmacro %}\n{% macro foo() %}[{% do other_macro() %}]{% endmacro %}\nShould not show output of other_macro: {{ foo() }}"
 info: {}
 input_file: minijinja/tests/inputs/do_closure.txt
 ---
 
 
-
 Should not show output of other_macro: []
 
diff --git a/minijinja/tests/snapshots/test_templates__vm@macro_caller_methods.txt.snap b/minijinja/tests/snapshots/test_templates__vm@macro_caller_methods.txt.snap
index 136158f6..e0f43fcf 100644
--- a/minijinja/tests/snapshots/test_templates__vm@macro_caller_methods.txt.snap
+++ b/minijinja/tests/snapshots/test_templates__vm@macro_caller_methods.txt.snap
@@ -9,7 +9,7 @@ input_file: minijinja/tests/inputs/macro_caller_methods.txt
 
 
 
-  closure: original closure
+  closure: other closure
   name: caller
   args: ["a", "b"]
   caller: false
diff --git a/minijinja/tests/snapshots/test_templates__vm@macro_closure_behavior.txt.snap b/minijinja/tests/snapshots/test_templates__vm@macro_closure_behavior.txt.snap
new file mode 100644
index 00000000..f07d9474
--- /dev/null
+++ b/minijinja/tests/snapshots/test_templates__vm@macro_closure_behavior.txt.snap
@@ -0,0 +1,11 @@
+---
+source: minijinja/tests/test_templates.rs
+description: "{%- macro before_closure() %}{{ closure }}{% endmacro %}\n{%- set closure = 1 %}\n{%- macro after_closure() %}{{ closure }}{% endmacro %}\n{%- set closure = 2 %}\n{%- macro after_closure_reset() %}{{ closure }}{% endmacro %}\n{{ before_closure() }}\n{{ after_closure() }}\n{{ after_closure_reset() }}"
+info: {}
+input_file: minijinja/tests/inputs/macro_closure_behavior.txt
+---
+
+2
+2
+2
+
diff --git a/minijinja/tests/snapshots/test_templates__vm@macro_hoisting.txt.snap b/minijinja/tests/snapshots/test_templates__vm@macro_hoisting.txt.snap
new file mode 100644
index 00000000..912cfd72
--- /dev/null
+++ b/minijinja/tests/snapshots/test_templates__vm@macro_hoisting.txt.snap
@@ -0,0 +1,10 @@
+---
+source: minijinja/tests/test_templates.rs
+description: "{% macro first(param) %}first: {{ second(param )}}{% endmacro %}\n{% macro second(param) %}second: {{ param }}{% endmacro %}\n{{ first(42) }}"
+info: {}
+input_file: minijinja/tests/inputs/macro_hoisting.txt
+---
+
+
+first: second: 42
+
diff --git a/minijinja/tests/snapshots/test_templates__vm@macro_recursive_alias.txt.snap b/minijinja/tests/snapshots/test_templates__vm@macro_recursive_alias.txt.snap
index 0faa1b29..dd8144da 100644
--- a/minijinja/tests/snapshots/test_templates__vm@macro_recursive_alias.txt.snap
+++ b/minijinja/tests/snapshots/test_templates__vm@macro_recursive_alias.txt.snap
@@ -1,6 +1,6 @@
 ---
 source: minijinja/tests/test_templates.rs
-description: "{%- macro nest(children) -%}\n    <nest>\n    {%- for child in children -%}\n        <child>{{ nest(child.children) }}</child>\n    {%- endfor -%}\n    </nest>\n{%- endmacro -%}\n{% set not_nest = nest %}\n{% set nest = \"not callable\" %}\n{{ not_nest(top_level) }}"
+description: "{%- macro nest(children) -%}\n    <nest>\n    {%- for child in children -%}\n        <child>{{ nest(child.children) }}</child>\n    {%- endfor -%}\n    </nest>\n{%- endmacro -%}\n{% set not_nest = nest %}\n{{ not_nest(top_level) }}"
 info:
   top_level:
     - children:
@@ -11,6 +11,5 @@ info:
 input_file: minijinja/tests/inputs/macro_recursive_alias.txt
 ---
 
-
 <nest><child><nest><child><nest></nest></child><child><nest></nest></child></nest></child><child><nest><child><nest></nest></child></nest></child></nest>