Repository: mitsuhiko/minijinja (language: rust)

Issue:
UndefinedBehavior::Strict and undefined key in mapping
## Description

I can reproduce my problem with `minijinja-cli`, but it happens in my own code.

## Reproduction steps

1. with input `input.json`:
```
{ "m": {} }
```
2. and template `template.j2`:
```
{% if m.key is defined %}
  {{ m.key }}
{% endif %}
```
3. and running `minijinja-cli --strict template.j2 input.json` I get
```
template error: undefined value (in [...]/template.j2:1)
----------------------------------- test.j2 -----------------------------------
   1 > {%- if m.key is defined %}
     i        ^^^^^ undefined value
   2 | {{ m.key }}
   3 | {%- endif %}
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Referenced variables: {
    m: {},
}
-------------------------------------------------------------------------------
```

I would expect that the `is_defined` and `is_undefined` test would work.  I also did not find a test like `has_key` which would also solve that problem.

Is that behavior expected and intended?

## minijinja version
`minijinja-cli 1.0.20`


Discussion / hints:
This is currently intentional but not ideal as it does not match the Jinja2 behavior for strict undefineds.  However you can solve this issue today:

```jinja2
{% if "key" in m %}
  {{ m.key }}
{% endif %}
```
Ah, thank you.  I missed the `is_in` test.  Do you prefer to keep this issue open?
Yes. This likely should be changed.

Fix patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 89825428..463acbff 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -56,6 +56,8 @@ For upgrade instructions read the [UPDATING](UPDATING.md) guide.
   was raised to 100000. #493
 - `Value::from` is now implemented for `Error` as public API to create invalid values.
   Previously this behavior was hidden internally in the serde support. #495
+- `UndefinedBehavior::Strict` now acts more delayed.  This means that now `value.key is defined`
+  will no longer error when `value.key` is `undefined`. #500
 
 ## 1.0.20
 
diff --git a/minijinja/src/utils.rs b/minijinja/src/utils.rs
index 4dcd4b96..4efd8725 100644
--- a/minijinja/src/utils.rs
+++ b/minijinja/src/utils.rs
@@ -147,15 +147,27 @@ impl UndefinedBehavior {
     /// looking up a missing attribute on a defined value.
     pub(crate) fn handle_undefined(self, parent_was_undefined: bool) -> Result<Value, Error> {
         match (self, parent_was_undefined) {
-            (UndefinedBehavior::Lenient, false) | (UndefinedBehavior::Chainable, _) => {
-                Ok(Value::UNDEFINED)
-            }
-            (UndefinedBehavior::Lenient, true) | (UndefinedBehavior::Strict, _) => {
+            (UndefinedBehavior::Lenient, false)
+            | (UndefinedBehavior::Strict, false)
+            | (UndefinedBehavior::Chainable, _) => Ok(Value::UNDEFINED),
+            (UndefinedBehavior::Lenient, true) | (UndefinedBehavior::Strict, true) => {
                 Err(Error::from(ErrorKind::UndefinedError))
             }
         }
     }
 
+    /// Utility method to check if something is true.
+    ///
+    /// This fails only for strict undefined values.
+    #[inline]
+    pub(crate) fn is_true(self, value: &Value) -> Result<bool, Error> {
+        if matches!(self, UndefinedBehavior::Strict) && value.is_undefined() {
+            Err(Error::from(ErrorKind::UndefinedError))
+        } else {
+            Ok(value.is_true())
+        }
+    }
+
     /// Tries to iterate over a value while handling the undefined value.
     ///
     /// If the value is undefined, then iteration fails if the behavior is set to strict,
diff --git a/minijinja/src/vm/mod.rs b/minijinja/src/vm/mod.rs
index 24af90f0..807faecd 100644
--- a/minijinja/src/vm/mod.rs
+++ b/minijinja/src/vm/mod.rs
@@ -235,8 +235,7 @@ impl<'env> Vm<'env> {
 
             // if we only have two arguments that we pull from the stack, we
             // can assign them to a and b.  This slightly reduces the amount of
-            // code bloat generated here.  Do the same for a potential error
-            // that needs processing.
+            // code bloat generated here.
             let a;
             let b;
             let mut err;
@@ -483,13 +482,13 @@ impl<'env> Vm<'env> {
                 }
                 Instruction::JumpIfFalse(jump_target) => {
                     a = stack.pop();
-                    if !a.is_true() {
+                    if !ok!(undefined_behavior.is_true(&a)) {
                         pc = *jump_target;
                         continue;
                     }
                 }
                 Instruction::JumpIfFalseOrPop(jump_target) => {
-                    if !stack.peek().is_true() {
+                    if !ok!(undefined_behavior.is_true(stack.peek())) {
                         pc = *jump_target;
                         continue;
                     } else {
@@ -497,7 +496,7 @@ impl<'env> Vm<'env> {
                     }
                 }
                 Instruction::JumpIfTrueOrPop(jump_target) => {
-                    if stack.peek().is_true() {
+                    if ok!(undefined_behavior.is_true(stack.peek())) {
                         pc = *jump_target;
                         continue;
                     } else {


Test patch:
diff --git a/minijinja/src/tests.rs b/minijinja/src/tests.rs
index 2aeb1e5a..5ec1d88c 100644
--- a/minijinja/src/tests.rs
+++ b/minijinja/src/tests.rs
@@ -474,10 +474,11 @@ mod builtins {
     /// This is useful when combined with [`select`](crate::filters::select).
     #[cfg_attr(docsrs, doc(cfg(feature = "builtins")))]
     #[cfg(feature = "builtins")]
-    pub fn is_in(value: &Value, other: &Value) -> bool {
-        crate::value::ops::contains(other, value)
+    pub fn is_in(state: &State, value: &Value, other: &Value) -> Result<bool, Error> {
+        ok!(state.undefined_behavior().assert_iterable(value));
+        Ok(crate::value::ops::contains(other, value)
             .map(|value| value.is_true())
-            .unwrap_or(false)
+            .unwrap_or(false))
     }
 
     /// Checks if a value is `true`.
diff --git a/minijinja/tests/test_undefined.rs b/minijinja/tests/test_undefined.rs
index 0fb70332..0ebca355 100644
--- a/minijinja/tests/test_undefined.rs
+++ b/minijinja/tests/test_undefined.rs
@@ -1,5 +1,7 @@
 #![cfg(feature = "builtins")]
-use minijinja::{render, Environment, ErrorKind, State, UndefinedBehavior};
+use std::collections::HashMap;
+
+use minijinja::{context, render, Environment, ErrorKind, State, UndefinedBehavior, Value};
 
 use similar_asserts::assert_eq;
 
@@ -24,8 +26,13 @@ fn test_lenient_undefined() {
         render!(in env, "<{% for x in undefined %}...{% endfor %}>"),
         "<>"
     );
+    assert_eq!(render!(in env, "{{ 'foo' is in(undefined) }}"), "false");
     assert_eq!(render!(in env, "<{{ undefined }}>"), "<>");
     assert_eq!(render!(in env, "{{ undefined is undefined }}"), "true");
+    assert_eq!(
+        render!(in env, "{{ x.foo is undefined }}", x => HashMap::<String, String>::new()),
+        "true"
+    );
     assert_eq!(render!(in env, "{{ undefined|list }}"), "[]");
     assert_eq!(render!(in env, "<{{ undefined|test }}>"), "<>");
     assert_eq!(render!(in env, "{{ 42 in undefined }}"), "false");
@@ -38,6 +45,9 @@ fn test_strict_undefined() {
     env.add_filter("test", |_state: &State, _value: String| -> String {
         panic!("filter must not be called");
     });
+    env.add_filter("test2", |_state: &State, _value: Value| -> String {
+        "WAS CALLED".into()
+    });
 
     assert_eq!(
         env.render_str("{{ true.missing_attribute }}", ())
@@ -57,11 +67,30 @@ fn test_strict_undefined() {
             .kind(),
         ErrorKind::UndefinedError
     );
+    assert_eq!(
+        env.render_str("{{ 'foo' is in(undefined) }}", ())
+            .unwrap_err()
+            .kind(),
+        ErrorKind::UndefinedError
+    );
     assert_eq!(
         env.render_str("<{{ undefined }}>", ()).unwrap_err().kind(),
         ErrorKind::UndefinedError
     );
     assert_eq!(render!(in env, "{{ undefined is undefined }}"), "true");
+    assert_eq!(
+        render!(in env, "{{ x.foo is undefined }}", x => HashMap::<String, String>::new()),
+        "true"
+    );
+    assert_eq!(
+        env.render_str(
+            "{% if x.foo %}...{% endif %}",
+            context! { x => HashMap::<String, String>::new() }
+        )
+        .unwrap_err()
+        .kind(),
+        ErrorKind::UndefinedError
+    );
     assert_eq!(
         env.render_str("{{ undefined|list }}", ())
             .unwrap_err()
@@ -74,6 +103,7 @@ fn test_strict_undefined() {
             .kind(),
         ErrorKind::UndefinedError
     );
+    assert_eq!(render!(in env, "{{ undefined|test2 }}"), "WAS CALLED");
     assert_eq!(
         env.render_str("{{ 42 in undefined }}", ())
             .unwrap_err()
@@ -98,6 +128,11 @@ fn test_chainable_undefined() {
         render!(in env, "<{% for x in undefined %}...{% endfor %}>"),
         "<>"
     );
+    assert_eq!(
+        render!(in env, "{{ x.foo is undefined }}", x => HashMap::<String, String>::new()),
+        "true"
+    );
+    assert_eq!(render!(in env, "{{ 'foo' is in(undefined) }}"), "false");
     assert_eq!(render!(in env, "<{{ undefined }}>"), "<>");
     assert_eq!(render!(in env, "{{ undefined is undefined }}"), "true");
     assert_eq!(render!(in env, "{{ undefined|list }}"), "[]");