From 841cb2ce54b83286f6068cc2d87c63a6966d5a00 Mon Sep 17 00:00:00 2001 From: Armin Ronacher <armin.ronacher@active-4.com> Date: Mon, 16 Sep 2024 19:17:16 +0200 Subject: [PATCH 1/2] Changed Ord to have a more consistent order --- minijinja/src/value/mod.rs | 56 ++++++++------ .../test_templates__vm@coerce.txt.snap | 4 +- .../test_templates__vm@filters.txt.snap | 4 +- minijinja/tests/test_value.rs | 74 ++++++++++++++++++- 4 files changed, 106 insertions(+), 32 deletions(-) diff --git a/minijinja/src/value/mod.rs b/minijinja/src/value/mod.rs index ce361b5c..4fdac445 100644 --- a/minijinja/src/value/mod.rs +++ b/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))) + } } } diff --git a/minijinja/tests/snapshots/test_templates__vm@coerce.txt.snap b/minijinja/tests/snapshots/test_templates__vm@coerce.txt.snap index e6a3fb22..42f124a0 100644 --- a/minijinja/tests/snapshots/test_templates__vm@coerce.txt.snap +++ b/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 - diff --git a/minijinja/tests/snapshots/test_templates__vm@filters.txt.snap b/minijinja/tests/snapshots/test_templates__vm@filters.txt.snap index 14a61668..e29d1c66 100644 --- a/minijinja/tests/snapshots/test_templates__vm@filters.txt.snap +++ b/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"} diff --git a/minijinja/tests/test_value.rs b/minijinja/tests/test_value.rs index df36f695..e97d0166 100644 --- a/minijinja/tests/test_value.rs +++ b/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>, + ] + "###); +} From 6a0e7bf515be3a2b79b17e3224a4a61f3fa8c44c Mon Sep 17 00:00:00 2001 From: Armin Ronacher <armin.ronacher@active-4.com> Date: Mon, 16 Sep 2024 19:24:23 +0200 Subject: [PATCH 2/2] Update changelog --- CHANGELOG.md | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d3462437..32e5d07e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,12 +2,9 @@ All notable changes to MiniJinja are documented here. -## 2.3.1 - -- Fixes some compiler warnings in Rust 1.81. #575 - ## 2.3.0 +- Fixes some compiler warnings in Rust 1.81. #575 - Fixes incorrect ordering of maps when the keys of those maps were not in consistent order. #569 - Implemented the missing `groupby` filter. #570 @@ -15,6 +12,8 @@ All notable changes to MiniJinja are documented here. Jinja2 and supports an optional flag to make it case sensitive. It also now lets one check individual attributes instead of values. #571 +- Changed sort order of `Ord` to avoid accidentally non total order + that could cause panics on Rust 1.81. #579 ## 2.2.0