Repository: mitsuhiko/minijinja
Pull request #707 (2025-02-07 14:41:19)

Allow objects to influence how they are compared
This adds a new `Object::custom_cmp` method to allow limited custom comparisons.  This will allow python objects to be compared against matching types.  This will *not* allow comparisons between objects of different types.  It also requires total order so the python bindings will panic if a sort is attempted of sequences that do not allow total order.

Refs #703


Patch:
diff --git a/.vscode/settings.json b/.vscode/settings.json
index cbb3341..c0b3e8a 100644
--- a/.vscode/settings.json
+++ b/.vscode/settings.json
@@ -1,5 +1,6 @@
 {
     "rust-analyzer.cargo.allFeatures": true,
     "rust-analyzer.checkOnSave.command": "clippy",
-    "python.formatting.provider": "black"
+    "python.formatting.provider": "black",
+    "python.REPL.enableREPLSmartSend": false
 }
\ No newline at end of file
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 23e9bc3..3e09d83 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -14,6 +14,9 @@ All notable changes to MiniJinja are documented here.
   `Environnent.undeclared_variables_in_str` to Python binding.  #699
 - Enable `loop_controls` for Python in-line with the CLI.  #704
 - Fixed a panic when comparing plain objects.  #705
+- Added `Object::custom_cmp` to allow objects to influence how they
+  compare against themselves.  This also fixes Python objects in the
+  Python binding not to compare correctly.  #707
 
 ## 2.8.0
 
diff --git a/minijinja-py/src/typeconv.rs b/minijinja-py/src/typeconv.rs
index 112d385..9e54083 100644
--- a/minijinja-py/src/typeconv.rs
+++ b/minijinja-py/src/typeconv.rs
@@ -1,8 +1,9 @@
+use std::cmp::Ordering;
 use std::collections::BTreeMap;
 use std::fmt;
 use std::sync::{Arc, Mutex};
 
-use minijinja::value::{Enumerator, Object, ObjectRepr, Value, ValueKind};
+use minijinja::value::{DynObject, Enumerator, Object, ObjectRepr, Value, ValueKind};
 use minijinja::{AutoEscape, Error, State};
 
 use once_cell::sync::OnceCell;
@@ -123,6 +124,17 @@ impl Object for DynamicObject {
         })
     }
 
+    fn custom_cmp(self: &Arc<Self>, other: &DynObject) -> Option<Ordering> {
+        // Attention: this can violate the requirements of custom_cmp,
+        // namely that it implements a total order.
+        Python::with_gil(|py| {
+            let self_inner = self.inner.bind(py);
+            let other = other.downcast_ref::<DynamicObject>()?;
+            let other_inner = other.inner.bind(py);
+            self_inner.compare(other_inner).ok()
+        })
+    }
+
     fn enumerate(self: &Arc<Self>) -> Enumerator {
         Python::with_gil(|py| {
             let inner = self.inner.bind(py);
diff --git a/minijinja/src/value/mod.rs b/minijinja/src/value/mod.rs
index b7e8ec0..93c9812 100644
--- a/minijinja/src/value/mod.rs
+++ b/minijinja/src/value/mod.rs
@@ -507,6 +507,10 @@ impl PartialEq for Value {
                     if let (Some(a), Some(b)) = (self.as_object(), other.as_object()) {
                         if a.is_same_object(b) {
                             return true;
+                        } else if a.is_same_object_type(b) {
+                            if let Some(rv) = a.custom_cmp(b) {
+                                return rv == Ordering::Equal;
+                            }
                         }
                         match (a.repr(), b.repr()) {
                             (ObjectRepr::Map, ObjectRepr::Map) => {
@@ -604,38 +608,43 @@ impl Ord for Value {
                 Some(ops::CoerceResult::I128(a, b)) => a.cmp(&b),
                 Some(ops::CoerceResult::Str(a, b)) => a.cmp(b),
                 None => {
-                    if let (Some(a), Some(b)) = (self.as_object(), other.as_object()) {
-                        if a.is_same_object(b) {
-                            Ordering::Equal
-                        } else {
-                            match (a.repr(), b.repr()) {
-                                (ObjectRepr::Map, ObjectRepr::Map) => {
-                                    // This is not really correct.  Because the keys can be in arbitrary
-                                    // order this could just sort really weirdly as a result.  However
-                                    // we don't want to pay the cost of actually sorting the keys for
-                                    // ordering so we just accept this for now.
-                                    match (a.try_iter_pairs(), b.try_iter_pairs()) {
-                                        (Some(a), Some(b)) => a.cmp(b),
-                                        _ => unreachable!(),
-                                    }
-                                }
-                                (
-                                    ObjectRepr::Seq | ObjectRepr::Iterable,
-                                    ObjectRepr::Seq | ObjectRepr::Iterable,
-                                ) => match (a.try_iter(), b.try_iter()) {
+                    let a = self.as_object().unwrap();
+                    let b = other.as_object().unwrap();
+
+                    if a.is_same_object(b) {
+                        Ordering::Equal
+                    } else {
+                        // if there is a custom comparison, run it.
+                        if a.is_same_object_type(b) {
+                            if let Some(rv) = a.custom_cmp(b) {
+                                return rv;
+                            }
+                        }
+                        match (a.repr(), b.repr()) {
+                            (ObjectRepr::Map, ObjectRepr::Map) => {
+                                // This is not really correct.  Because the keys can be in arbitrary
+                                // order this could just sort really weirdly as a result.  However
+                                // we don't want to pay the cost of actually sorting the keys for
+                                // ordering so we just accept this for now.
+                                match (a.try_iter_pairs(), b.try_iter_pairs()) {
                                     (Some(a), Some(b)) => a.cmp(b),
                                     _ => unreachable!(),
-                                },
-                                // terrible fallback for plain objects
-                                (ObjectRepr::Plain, ObjectRepr::Plain) => {
-                                    a.to_string().cmp(&b.to_string())
                                 }
-                                // should not happen
-                                (_, _) => unreachable!(),
                             }
+                            (
+                                ObjectRepr::Seq | ObjectRepr::Iterable,
+                                ObjectRepr::Seq | ObjectRepr::Iterable,
+                            ) => match (a.try_iter(), b.try_iter()) {
+                                (Some(a), Some(b)) => a.cmp(b),
+                                _ => unreachable!(),
+                            },
+                            // terrible fallback for plain objects
+                            (ObjectRepr::Plain, ObjectRepr::Plain) => {
+                                a.to_string().cmp(&b.to_string())
+                            }
+                            // should not happen
+                            (_, _) => unreachable!(),
                         }
-                    } else {
-                        unreachable!()
                     }
                 }
             },
diff --git a/minijinja/src/value/object.rs b/minijinja/src/value/object.rs
index 1bcc654..70f41dd 100644
--- a/minijinja/src/value/object.rs
+++ b/minijinja/src/value/object.rs
@@ -1,4 +1,6 @@
+use std::any::Any;
 use std::borrow::Cow;
+use std::cmp::Ordering;
 use std::collections::BTreeMap;
 use std::fmt;
 use std::hash::Hash;
@@ -243,6 +245,35 @@ pub trait Object: fmt::Debug + Send + Sync {
         Err(Error::from(ErrorKind::UnknownMethod))
     }
 
+    /// Custom comparison of this object against another object of the same type.
+    ///
+    /// This must return either `None` or `Some(Ordering)`.  When implemented this
+    /// must guarantee a total ordering as otherwise sort functions will crash.
+    /// This will only compare against other objects of the same type, not
+    /// anything else.  Objects of different types are given an absolute
+    /// ordering outside the scope of this method.
+    ///
+    /// The requirement is that an implementer downcasts the other [`DynObject`]
+    /// to itself, and it that cannot be accomplished `None` must be returned.
+    ///
+    /// ```rust
+    /// # use std::sync::Arc;
+    /// # use std::cmp::Ordering;
+    /// # use minijinja::value::{DynObject, Object};
+    /// # #[derive(Debug)]
+    /// # struct Thing { num: u32 };
+    /// impl Object for Thing {
+    ///     fn custom_cmp(self: &Arc<Self>, other: &DynObject) -> Option<Ordering> {
+    ///         let other = other.downcast_ref::<Self>()?;
+    ///         Some(self.num.cmp(&other.num))
+    ///     }
+    /// }
+    /// ```
+    fn custom_cmp(self: &Arc<Self>, other: &DynObject) -> Option<Ordering> {
+        let _ = other;
+        None
+    }
+
     /// Formats the object for stringification.
     ///
     /// The default implementation is specific to the behavior of
@@ -610,6 +641,8 @@ type_erase! {
             args: &[Value]
         ) -> Result<Value, Error>;
 
+        fn custom_cmp(&self, other: &DynObject) -> Option<Ordering>;
+
         fn render(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result;
 
         impl fmt::Debug {
@@ -628,6 +661,11 @@ impl DynObject {
     pub(crate) fn is_same_object(&self, other: &DynObject) -> bool {
         self.ptr == other.ptr && self.vtable == other.vtable
     }
+
+    /// Checks if the two dyn objects are of the same type.
+    pub(crate) fn is_same_object_type(&self, other: &DynObject) -> bool {
+        self.type_id() == other.type_id()
+    }
 }
 
 impl Hash for DynObject {


Test patch:
diff --git a/minijinja-py/tests/test_basic.py b/minijinja-py/tests/test_basic.py
index 780beb7..b0c4500 100644
--- a/minijinja-py/tests/test_basic.py
+++ b/minijinja-py/tests/test_basic.py
@@ -2,6 +2,7 @@ import binascii
 import pytest
 import posixpath
 import types
+from functools import total_ordering
 
 from _pytest.unraisableexception import catch_unraisable_exception
 from minijinja import (
@@ -387,3 +388,28 @@ def test_loop_controls():
     {% endfor %}
     """)
     assert rv.split() == ["2"]
+
+
+def test_pass_through_sort():
+    @total_ordering
+    class X(object):
+        def __init__(self, value):
+            self.value = value
+
+        def __eq__(self, other):
+            if type(self) is not type(other):
+                return NotImplemented
+            return self.value == other.value
+
+        def __lt__(self, other):
+            if type(self) is not type(other):
+                return NotImplemented
+            return self.value < other.value
+
+        def __str__(self):
+            return str(self.value)
+
+    values = [X(4), X(23), X(42), X(-1)]
+    env = Environment()
+    rv = env.render_str("{{ values|sort|join(',') }}", values=values)
+    assert rv == "-1,4,23,42"
diff --git a/minijinja/tests/test_value.rs b/minijinja/tests/test_value.rs
index a2291f8..f97a58f 100644
--- a/minijinja/tests/test_value.rs
+++ b/minijinja/tests/test_value.rs
@@ -1,3 +1,4 @@
+use std::cmp::Ordering;
 use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet, LinkedList, VecDeque};
 use std::fmt;
 use std::sync::Arc;
@@ -1248,3 +1249,37 @@ fn test_plain_object_compare() {
     assert!(v1 != Value::from(vec![1, 2, 3]));
     assert!(v1 > Value::from(vec![1, 2, 3]));
 }
+
+#[test]
+fn test_custom_object_compare() {
+    #[derive(Debug)]
+    struct X(i32);
+
+    impl Object for X {
+        fn repr(self: &Arc<Self>) -> ObjectRepr {
+            ObjectRepr::Plain
+        }
+
+        fn custom_cmp(self: &Arc<Self>, other: &DynObject) -> Option<Ordering> {
+            let other = other.downcast_ref::<Self>()?;
+            Some(self.0.cmp(&other.0))
+        }
+
+        fn render(self: &Arc<Self>, f: &mut fmt::Formatter<'_>) -> fmt::Result
+        where
+            Self: Sized + 'static,
+        {
+            write!(f, "{}", self.0)
+        }
+    }
+
+    let nums = (0..5)
+        .map(X)
+        .map(Value::from_object)
+        .rev()
+        .collect::<Vec<_>>();
+    let seq = Value::from_object(nums);
+
+    let rv = render!("{{ seq|sort|join('|') }}", seq);
+    assert_eq!(rv, "0|1|2|3|4");
+}