Clearer error messages for failed comparisons

Fixes #1231

4 files changed, 38 insertions, 37 deletions

diff --git a/src/eval/array.rs b/src/eval/array.rs
index 6ae5d7cf..d0b63b35 100644
--- a/src/eval/array.rs
+++ b/src/eval/array.rs
@@ -344,17 +344,14 @@ impl Array {
         vec.make_mut().sort_by(|a, b| {
             // Until we get `try` blocks :)
             match (key_of(a.clone()), key_of(b.clone())) {
-                (Ok(a), Ok(b)) => a.partial_cmp(&b).unwrap_or_else(|| {
-                    if result.is_ok() {
-                        result = Err(eco_format!(
-                            "cannot order {} and {}",
-                            a.type_name(),
-                            b.type_name(),
-                        ))
-                        .at(span);
-                    }
-                    Ordering::Equal
-                }),
+                (Ok(a), Ok(b)) => {
+                    typst::eval::ops::compare(&a, &b).unwrap_or_else(|err| {
+                        if result.is_ok() {
+                            result = Err(err).at(span);
+                        }
+                        Ordering::Equal
+                    })
+                }
                 (Err(e), _) | (_, Err(e)) => {
                     if result.is_ok() {
                         result = Err(e);
diff --git a/src/eval/mod.rs b/src/eval/mod.rs
index e80c95c8..a91bea10 100644
--- a/src/eval/mod.rs
+++ b/src/eval/mod.rs
@@ -16,7 +16,7 @@ mod args;
 mod func;
 mod methods;
 mod module;
-mod ops;
+pub mod ops;
 mod scope;
 mod symbol;
 
diff --git a/src/eval/ops.rs b/src/eval/ops.rs
index 48e74c8f..3e397c30 100644
--- a/src/eval/ops.rs
+++ b/src/eval/ops.rs
@@ -1,6 +1,7 @@
 //! Operations on values.
 
 use std::cmp::Ordering;
+use std::fmt::Debug;
 
 use ecow::eco_format;
 
@@ -318,11 +319,8 @@ macro_rules! comparison {
     ($name:ident, $op:tt, $($pat:tt)*) => {
         /// Compute how a value compares with another value.
         pub fn $name(lhs: Value, rhs: Value) -> StrResult<Value> {
-            if let Some(ordering) = compare(&lhs, &rhs) {
-                Ok(Bool(matches!(ordering, $($pat)*)))
-            } else {
-                mismatch!(concat!("cannot apply '", $op, "' to {} and {}"), lhs, rhs);
-            }
+            let ordering = compare(&lhs, &rhs)?;
+            Ok(Bool(matches!(ordering, $($pat)*)))
         }
     };
 }
@@ -371,28 +369,34 @@ pub fn equal(lhs: &Value, rhs: &Value) -> bool {
 }
 
 /// Compare two values.
-pub fn compare(lhs: &Value, rhs: &Value) -> Option<Ordering> {
-    match (lhs, rhs) {
-        (Bool(a), Bool(b)) => a.partial_cmp(b),
-        (Int(a), Int(b)) => a.partial_cmp(b),
-        (Float(a), Float(b)) => a.partial_cmp(b),
-        (Length(a), Length(b)) => a.partial_cmp(b),
-        (Angle(a), Angle(b)) => a.partial_cmp(b),
-        (Ratio(a), Ratio(b)) => a.partial_cmp(b),
-        (Relative(a), Relative(b)) => a.partial_cmp(b),
-        (Fraction(a), Fraction(b)) => a.partial_cmp(b),
-        (Str(a), Str(b)) => a.partial_cmp(b),
+pub fn compare(lhs: &Value, rhs: &Value) -> StrResult<Ordering> {
+    Ok(match (lhs, rhs) {
+        (Bool(a), Bool(b)) => a.cmp(b),
+        (Int(a), Int(b)) => a.cmp(b),
+        (Float(a), Float(b)) => try_cmp_values(a, b)?,
+        (Length(a), Length(b)) => try_cmp_values(a, b)?,
+        (Angle(a), Angle(b)) => a.cmp(b),
+        (Ratio(a), Ratio(b)) => a.cmp(b),
+        (Relative(a), Relative(b)) => try_cmp_values(a, b)?,
+        (Fraction(a), Fraction(b)) => a.cmp(b),
+        (Str(a), Str(b)) => a.cmp(b),
 
         // Some technically different things should be comparable.
-        (&Int(a), &Float(b)) => (a as f64).partial_cmp(&b),
-        (&Float(a), &Int(b)) => a.partial_cmp(&(b as f64)),
-        (&Length(a), &Relative(b)) if b.rel.is_zero() => a.partial_cmp(&b.abs),
-        (&Ratio(a), &Relative(b)) if b.abs.is_zero() => a.partial_cmp(&b.rel),
-        (&Relative(a), &Length(b)) if a.rel.is_zero() => a.abs.partial_cmp(&b),
-        (&Relative(a), &Ratio(b)) if a.abs.is_zero() => a.rel.partial_cmp(&b),
+        (Int(a), Float(b)) => try_cmp_values(&(*a as f64), b)?,
+        (Float(a), Int(b)) => try_cmp_values(a, &(*b as f64))?,
+        (Length(a), Relative(b)) if b.rel.is_zero() => try_cmp_values(a, &b.abs)?,
+        (Ratio(a), Relative(b)) if b.abs.is_zero() => a.cmp(&b.rel),
+        (Relative(a), Length(b)) if a.rel.is_zero() => try_cmp_values(&a.abs, b)?,
+        (Relative(a), Ratio(b)) if a.abs.is_zero() => a.rel.cmp(b),
+
+        _ => mismatch!("cannot compare {} and {}", lhs, rhs),
+    })
+}
 
-        _ => Option::None,
-    }
+/// Try to compare two values.
+fn try_cmp_values<T: PartialOrd + Debug>(a: &T, b: &T) -> StrResult<Ordering> {
+    a.partial_cmp(b)
+        .ok_or_else(|| eco_format!("cannot compare {:?} with {:?}", a, b))
 }
 
 /// Test whether one value is "in" another one.
diff --git a/src/eval/value.rs b/src/eval/value.rs
index 36cda80b..6d947866 100644
--- a/src/eval/value.rs
+++ b/src/eval/value.rs
@@ -207,7 +207,7 @@ impl PartialEq for Value {
 
 impl PartialOrd for Value {
     fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
-        ops::compare(self, other)
+        ops::compare(self, other).ok()
     }
 }