Repository: mitsuhiko/minijinja (language: rust)

Issue:
Remove State.current_call
This requires a mutable reference to the state which is not always available. It also means that `current_call` is currently only set when the engine calls into values, but not when anything else calls. While this does not happen that frequently today, it does mean that it's not possible today to expose calls in any meaningful way to filters or others.

This problem already exists internally as `map` for instance invokes other filters, but `current_call` is not updated.

I'm not sure if `current_call` is particularly useful today. Maybe it could be removed?  It might be possible to replicate the main uses of this via the `Object` traits for method calls (dynamic dispatch) and by potentially adding a way to hook the invocation of filters and tests.


Discussion / hints:

Fix patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 52072b9b..04c971c3 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -2,6 +2,13 @@
 
 All notable changes to MiniJinja are documented here.
 
+## 1.0.0
+
+- Removed support for `State::current_call`.  This property wasn't too useful
+  and unreliable.  Supporting it properly for nested invocations would require
+  calls to take a mutable state or use interior mutability which did not seem
+  reasonable for this.  (#269)
+
 ## 0.34.0
 
 - Updated `self_cell` and `percent-encoding` dependencies.  (#264)
diff --git a/minijinja-py/src/state.rs b/minijinja-py/src/state.rs
index 382489ba..543cdb9a 100644
--- a/minijinja-py/src/state.rs
+++ b/minijinja-py/src/state.rs
@@ -49,12 +49,6 @@ impl StateRef {
         with_state(|state| Ok(state.current_block().map(|x| x.into())))
     }
 
-    /// Returns the current call
-    #[getter]
-    pub fn get_current_call(&self) -> PyResult<Option<String>> {
-        with_state(|state| Ok(state.current_call().map(|x| x.into())))
-    }
-
     /// Looks up a variable in the context
     #[pyo3(text_signature = "(self, name)")]
     pub fn lookup(&self, name: &str) -> PyResult<Py<PyAny>> {
diff --git a/minijinja/src/vm/mod.rs b/minijinja/src/vm/mod.rs
index ebccd5a7..bb5c2f93 100644
--- a/minijinja/src/vm/mod.rs
+++ b/minijinja/src/vm/mod.rs
@@ -144,7 +144,6 @@ impl<'env> Vm<'env> {
                 env: self.env,
                 ctx,
                 current_block: None,
-                current_call: None,
                 auto_escape: state.auto_escape(),
                 instructions,
                 blocks: BTreeMap::default(),
@@ -514,7 +513,6 @@ impl<'env> Vm<'env> {
                     stack.push(out.end_capture(state.auto_escape));
                 }
                 Instruction::ApplyFilter(name, arg_count, local_id) => {
-                    state.current_call = Some(name);
                     let filter =
                         ctx_ok!(get_or_lookup_local(&mut loaded_filters, *local_id, || {
                             state.env.get_filter(name)
@@ -529,10 +527,8 @@ impl<'env> Vm<'env> {
                     a = ctx_ok!(filter.apply_to(state, args));
                     stack.drop_top(*arg_count);
                     stack.push(a);
-                    state.current_call = Some(name);
                 }
                 Instruction::PerformTest(name, arg_count, local_id) => {
-                    state.current_call = Some(name);
                     let test = ctx_ok!(get_or_lookup_local(&mut loaded_tests, *local_id, || {
                         state.env.get_test(name)
                     })
@@ -543,11 +539,8 @@ impl<'env> Vm<'env> {
                     let rv = ctx_ok!(test.perform(state, args));
                     stack.drop_top(*arg_count);
                     stack.push(Value::from(rv));
-                    state.current_call = None;
                 }
                 Instruction::CallFunction(name, arg_count) => {
-                    state.current_call = Some(name);
-
                     // super is a special function reserved for super-ing into blocks.
                     if *name == "super" {
                         if *arg_count != 0 {
@@ -578,16 +571,12 @@ impl<'env> Vm<'env> {
                             format!("{name} is unknown"),
                         ));
                     }
-
-                    state.current_call = None;
                 }
                 Instruction::CallMethod(name, arg_count) => {
-                    state.current_call = Some(name);
                     let args = stack.slice_top(*arg_count);
                     a = ctx_ok!(args[0].call_method(state, name, &args[1..]));
                     stack.drop_top(*arg_count);
                     stack.push(a);
-                    state.current_call = None;
                 }
                 Instruction::CallObject(arg_count) => {
                     let args = stack.slice_top(*arg_count);
@@ -602,13 +591,9 @@ impl<'env> Vm<'env> {
                     stack.pop();
                 }
                 Instruction::FastSuper => {
-                    // Note that we don't store 'current_call' here since it
-                    // would only be visible (and unused) internally.
                     ctx_ok!(self.perform_super(state, out, false));
                 }
                 Instruction::FastRecurse => {
-                    // Note that we don't store 'current_call' here since it
-                    // would only be visible (and unused) internally.
                     recurse_loop!(false);
                 }
                 // Explanation on the behavior of `LoadBlocks` and rendering of
diff --git a/minijinja/src/vm/state.rs b/minijinja/src/vm/state.rs
index e83b0c02..6170d99b 100644
--- a/minijinja/src/vm/state.rs
+++ b/minijinja/src/vm/state.rs
@@ -28,7 +28,6 @@ pub struct State<'vm, 'env> {
     pub(crate) env: &'env Environment<'env>,
     pub(crate) ctx: Context<'env>,
     pub(crate) current_block: Option<&'env str>,
-    pub(crate) current_call: Option<&'env str>,
     pub(crate) auto_escape: AutoEscape,
     pub(crate) instructions: &'vm Instructions<'env>,
     pub(crate) blocks: BTreeMap<&'env str, BlockStack<'vm, 'env>>,
@@ -45,7 +44,6 @@ impl<'vm, 'env> fmt::Debug for State<'vm, 'env> {
         let mut ds = f.debug_struct("State");
         ds.field("name", &self.instructions.name());
         ds.field("current_block", &self.current_block);
-        ds.field("current_call", &self.current_call);
         ds.field("auto_escape", &self.auto_escape);
         ds.field("ctx", &self.ctx);
         ds.field("env", &self.env);
@@ -66,7 +64,6 @@ impl<'vm, 'env> State<'vm, 'env> {
             env,
             ctx,
             current_block: None,
-            current_call: None,
             auto_escape,
             instructions,
             blocks,
@@ -107,13 +104,6 @@ impl<'vm, 'env> State<'vm, 'env> {
         self.current_block
     }
 
-    /// Returns the name of the item (filter, function, test, method) currently
-    /// being called.
-    #[inline(always)]
-    pub fn current_call(&self) -> Option<&str> {
-        self.current_call
-    }
-
     /// Looks up a variable by name in the context.
     #[inline(always)]
     pub fn lookup(&self, name: &str) -> Option<Value> {


Test patch:
diff --git a/minijinja-py/tests/test_state.py b/minijinja-py/tests/test_state.py
index 75bf135d..3a2f444f 100644
--- a/minijinja-py/tests/test_state.py
+++ b/minijinja-py/tests/test_state.py
@@ -9,7 +9,6 @@ def my_func(state):
         assert state.name == "template-name"
         assert state.auto_escape is None
         assert state.current_block == "foo"
-        assert state.current_call == "my_func"
         assert state.lookup("bar") == 23
         assert state.lookup("aha") is None
         assert state.lookup("my_func") is my_func
@@ -33,7 +32,6 @@ def my_func(state):
         assert state.name == "template-name"
         assert state.auto_escape is None
         assert state.current_block == "foo"
-        assert state.current_call == "my_func"
         assert state.lookup("bar") == 23
         assert state.lookup("aha") is None
         assert state.env is env
@@ -57,7 +55,6 @@ def my_filter(state, value):
         assert state.name == "template-name"
         assert state.auto_escape is None
         assert state.current_block == "foo"
-        assert state.current_call == "myfilter"
         assert state.lookup("bar") == 23
         assert state.lookup("aha") is None
         assert state.env is env
@@ -81,7 +78,6 @@ def my_test(state, value):
         assert state.name == "template-name"
         assert state.auto_escape is None
         assert state.current_block == "foo"
-        assert state.current_call == "mytest"
         assert state.lookup("bar") == 23
         assert state.lookup("aha") is None
         assert state.env is env
diff --git a/minijinja/tests/snapshots/test_templates__vm@debug.txt.snap b/minijinja/tests/snapshots/test_templates__vm@debug.txt.snap
index 2926ae8c..09b7addd 100644
--- a/minijinja/tests/snapshots/test_templates__vm@debug.txt.snap
+++ b/minijinja/tests/snapshots/test_templates__vm@debug.txt.snap
@@ -8,9 +8,6 @@ input_file: minijinja/tests/inputs/debug.txt
 State {
     name: "debug.txt",
     current_block: None,
-    current_call: Some(
-        "debug",
-    ),
     auto_escape: None,
     ctx: {
         "x": 0,
diff --git a/minijinja/tests/test_templates.rs b/minijinja/tests/test_templates.rs
index 1633e39c..c14245cd 100644
--- a/minijinja/tests/test_templates.rs
+++ b/minijinja/tests/test_templates.rs
@@ -304,107 +304,6 @@ fn test_loop_changed() {
     assert_eq!(rv, "12345");
 }
 
-#[test]
-fn test_current_call_state() {
-    use minijinja::value::{Object, Value};
-    use std::fmt;
-
-    #[derive(Debug)]
-    struct MethodAndFunc;
-
-    impl fmt::Display for MethodAndFunc {
-        fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-            write!(f, "{self:?}")
-        }
-    }
-
-    impl Object for MethodAndFunc {
-        fn call_method(&self, state: &State, name: &str, args: &[Value]) -> Result<Value, Error> {
-            assert_eq!(name, state.current_call().unwrap());
-            let args = args
-                .iter()
-                .map(|v| v.to_string())
-                .collect::<Vec<_>>()
-                .join(", ");
-
-            Ok(format!("{}({args})", state.current_call().unwrap()).into())
-        }
-
-        fn call(&self, state: &State, args: &[Value]) -> Result<Value, Error> {
-            let args = args
-                .iter()
-                .map(|v| v.to_string())
-                .collect::<Vec<_>>()
-                .join(", ");
-
-            Ok(format!("{}({args})", state.current_call().unwrap()).into())
-        }
-    }
-
-    fn current_call(state: &State, value: Option<&str>) -> String {
-        format!("{}({})", state.current_call().unwrap(), value.unwrap_or(""))
-    }
-
-    fn check_test(state: &State, value: &str) -> bool {
-        state.current_call() == Some(value)
-    }
-
-    let mut env = Environment::new();
-    env.add_function("fn_call_a", current_call);
-    env.add_function("fn_call_b", current_call);
-    env.add_filter("filter_call", current_call);
-    env.add_test("my_test", check_test);
-    env.add_test("another_test", check_test);
-    env.add_global("object", Value::from_object(MethodAndFunc));
-
-    env.add_template(
-        "test",
-        r#"
-        {{ fn_call_a() }}
-        {{ "foo" | filter_call }}
-        {{ fn_call_a() | filter_call }}
-        {{ fn_call_b() | filter_call }}
-        {{ fn_call_a(fn_call_b()) }}
-        {{ fn_call_a(fn_call_b()) | filter_call }}
-
-        {{ "my_test" is my_test }}
-        {{ "another_test" is my_test }}
-        {{ "another_test" is another_test }}
-
-        {{ object.foo() }}
-        {{ object.bar() }}
-        {{ object.foo(object.bar(object.baz())) }}
-        {{ object(object.bar()) }}
-        {{ object.baz(object()) }}
-    "#,
-    )
-    .unwrap();
-
-    let tmpl = env.get_template("test").unwrap();
-    let rv = tmpl.render(context!()).unwrap();
-    assert_eq!(
-        rv,
-        r#"
-        fn_call_a()
-        filter_call(foo)
-        filter_call(fn_call_a())
-        filter_call(fn_call_b())
-        fn_call_a(fn_call_b())
-        filter_call(fn_call_a(fn_call_b()))
-
-        true
-        false
-        true
-
-        foo()
-        bar()
-        foo(bar(baz()))
-        object(bar())
-        baz(object())
-    "#
-    );
-}
-
 // ideally this would work, but unfortunately the way serde flatten works makes it
 // impossible for us to support with the internal optimizations in the value model.
 // see https://github.com/mitsuhiko/minijinja/issues/222