Changed Ord to have a more consistent order

minijinja/src/value/mod.rs

@@ -540,7 +540,11 @@ fn f64_total_cmp(left: f64, right: f64) -> Ordering {
 
 impl Ord for Value {
     fn cmp(&self, other: &Self) -> Ordering {
-        let value_ordering = match (&self.0, &other.0) {
+        let kind_ordering = self.kind().cmp(&other.kind());
+        if matches!(kind_ordering, Ordering::Less | Ordering::Greater) {
+            return kind_ordering;
+        }
+        match (&self.0, &other.0) {
             (ValueRepr::None, ValueRepr::None) => Ordering::Equal,
             (ValueRepr::Undefined, ValueRepr::Undefined) => Ordering::Equal,
             (ValueRepr::String(ref a, _), ValueRepr::String(ref b, _)) => a.cmp(b),
@@ -550,34 +554,38 @@ impl Ord for Value {
                 Some(ops::CoerceResult::F64(a, b)) => f64_total_cmp(a, b),
                 Some(ops::CoerceResult::I128(a, b)) => a.cmp(&b),
                 Some(ops::CoerceResult::Str(a, b)) => a.cmp(b),
-                None => match (self.kind(), other.kind()) {
-                    (ValueKind::Seq, ValueKind::Seq) => match (self.try_iter(), other.try_iter()) {
-                        (Ok(a), Ok(b)) => a.cmp(b),
-                        _ => self.len().cmp(&other.len()),
-                    },
-                    (ValueKind::Map, ValueKind::Map) => {
-                        if let (Some(a), Some(b)) = (self.as_object(), other.as_object()) {
-                            if a.is_same_object(b) {
-                                Ordering::Equal
-                            } else {
-                                // 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),
-                                    _ => self.len().cmp(&other.len()),
+                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()) {
+                                    (Some(a), Some(b)) => a.cmp(b),
+                                    _ => unreachable!(),
+                                },
+                                (_, _) => unreachable!(),
                             }
-                        } else {
-                            unreachable!();
                         }
+                    } else {
+                        unreachable!()
                     }
-                    _ => Ordering::Equal,
-                },
+                }
             },
-        };
-        value_ordering.then((self.kind() as usize).cmp(&(other.kind() as usize)))
+        }
     }
 }
 

minijinja/tests/snapshots/test_templates__vm@coerce.txt.snap

@@ -2,6 +2,7 @@
 source: minijinja/tests/test_templates.rs
 description: "{{ \"World\"[0] == \"W\" }}\n{{ \"W\" == \"W\" }}\n{{ 1.0 == 1 }}\n{{ 1 != 2 }}\n{{ none == none }}\n{{ none != undefined }}\n{{ undefined == undefined }}\n{{ true == true }}\n{{ 1 == true }}\n{{ 0 == false }}\n{{ 1 != 0 }}\n{{ \"a\" < \"b\" }}\n{{ \"a\"[0] < \"b\" }}\n{{ false < true }}\n{{ 0 < true }}\n{{ [0, 0] == [0, 0] }}\n{{ [\"a\"] == [\"a\"[0]] }}"
 info: {}
+input_file: minijinja/tests/inputs/coerce.txt
 ---
 true
 true
@@ -17,7 +18,6 @@ true
 true
 true
 true
+false
 true
 true
-true
-

minijinja/tests/snapshots/test_templates__vm@filters.txt.snap

@@ -70,8 +70,8 @@ sort: [1, 2, 4, 9, 111]
 sort-reverse: [111, 9, 4, 2, 1]
 sort-case-insensitive: ["a", "B", "C", "z"]
 sort-case-sensitive: ["B", "C", "a", "z"]
-sort-case-insensitive-mixed: [false, 0, true, 1, "false", "False", "true", "True"]
-sort-case-sensitive-mixed: [false, 0, true, 1, "False", "True", "false", "true"]
+sort-case-insensitive-mixed: [false, true, 0, 1, "false", "False", "true", "True"]
+sort-case-sensitive-mixed: [false, true, 0, 1, "False", "True", "false", "true"]
 sort-attribute [{"name": "a"}, {"name": "b"}]
 d: true
 json: {"a":"b","c":"d"}

minijinja/tests/test_value.rs

@@ -1,11 +1,11 @@
 use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet, LinkedList, VecDeque};
 use std::sync::Arc;
 
-use insta::assert_snapshot;
+use insta::{assert_debug_snapshot, assert_snapshot};
 use similar_asserts::assert_eq;
 
 use minijinja::value::{DynObject, Enumerator, Kwargs, Object, ObjectRepr, Rest, Value};
-use minijinja::{args, render, Environment, Error};
+use minijinja::{args, render, Environment, Error, ErrorKind};
 
 #[test]
 fn test_sort() {
@@ -69,13 +69,13 @@ fn test_sort_different_types() {
     [
         undefined,
         none,
+        false,
+        true,
         -inf,
         -100,
         -75.0,
         -50.0,
-        false,
         0,
-        true,
         1,
         30,
         80,
@@ -1059,3 +1059,69 @@ fn test_float_eq() {
     let xb = Value::from(i64::MAX as f64);
     assert_ne!(xa, xb);
 }
+
+#[test]
+fn test_sorting() {
+    let mut values = vec![
+        Value::from(-f64::INFINITY),
+        Value::from(1.0),
+        Value::from(f64::NAN),
+        Value::from(f64::INFINITY),
+        Value::from(42.0),
+        Value::from(41),
+        Value::from(128),
+        Value::from(-2),
+        Value::from(-5.0),
+        Value::from(32i32),
+        Value::from(true),
+        Value::from(false),
+        Value::from(vec![1, 2, 3]),
+        Value::from(vec![1, 2, 3, 4]),
+        Value::from(vec![1]),
+        Value::from("whatever"),
+        Value::from("floats"),
+        Value::from("the"),
+        Value::from("boat"),
+        Value::UNDEFINED,
+        Value::from(()),
+        Value::from(Error::new(ErrorKind::InvalidOperation, "shit hit the fan")),
+    ];
+    values.sort();
+    assert_debug_snapshot!(&values, @r###"
+    [
+        undefined,
+        none,
+        false,
+        true,
+        -inf,
+        -5.0,
+        -2,
+        1.0,
+        32,
+        41,
+        42.0,
+        128,
+        inf,
+        NaN,
+        "boat",
+        "floats",
+        "the",
+        "whatever",
+        [
+            1,
+        ],
+        [
+            1,
+            2,
+            3,
+        ],
+        [
+            1,
+            2,
+            3,
+            4,
+        ],
+        <invalid value: invalid operation: shit hit the fan>,
+    ]
+    "###);
+}