Clearer error messages for failed comparisons

Fixes #1231

8 files changed, 57 insertions, 53 deletions

diff --git a/library/src/compute/calc.rs b/library/src/compute/calc.rs
index bd673c54..f485f817 100644
--- a/library/src/compute/calc.rs
+++ b/library/src/compute/calc.rs
@@ -831,18 +831,9 @@ fn minmax(
     };
 
     for Spanned { v, span } in iter {
-        match v.partial_cmp(&extremum) {
-            Some(ordering) => {
-                if ordering == goal {
-                    extremum = v;
-                }
-            }
-            None => bail!(
-                span,
-                "cannot compare {} and {}",
-                extremum.type_name(),
-                v.type_name(),
-            ),
+        let ordering = typst::eval::ops::compare(&v, &extremum).at(span)?;
+        if ordering == goal {
+            extremum = v;
         }
     }
 
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()
     }
 }
 
diff --git a/tests/typ/compiler/array.typ b/tests/typ/compiler/array.typ
index ef6e4b6b..59e9f70b 100644
--- a/tests/typ/compiler/array.typ
+++ b/tests/typ/compiler/array.typ
@@ -244,10 +244,14 @@
 #(1, 2, 0, 3).sorted(key: x => 5 / x)
 
 ---
-// Error: 2-26 cannot order content and content
+// Error: 2-26 cannot compare content and content
 #([Hi], [There]).sorted()
 
 ---
+// Error: 2-26 cannot compare 3em with 2pt
+#(1pt, 2pt, 3em).sorted()
+
+---
 // Error: 2-18 array index out of bounds (index: -4, len: 3) and no default value was specified
 #(1, 2, 3).at(-4)
 
diff --git a/tests/typ/compiler/ops-invalid.typ b/tests/typ/compiler/ops-invalid.typ
index 2af3b767..0ed26a56 100644
--- a/tests/typ/compiler/ops-invalid.typ
+++ b/tests/typ/compiler/ops-invalid.typ
@@ -26,14 +26,18 @@
 #(not ())
 
 ---
-// Error: 3-19 cannot apply '<=' to relative length and ratio
+// Error: 3-19 cannot compare relative length and ratio
 #(30% + 1pt <= 40%)
 
 ---
-// Error: 3-14 cannot apply '<=' to length and length
+// Error: 3-14 cannot compare 1em with 10pt
 #(1em <= 10pt)
 
 ---
+// Error: 3-22 cannot compare 2.2 with NaN
+#(2.2 <= float("nan"))
+
+---
 // Error: 3-12 cannot divide by zero
 #(1.2 / 0.0)
 
diff --git a/tests/typ/compute/calc.typ b/tests/typ/compute/calc.typ
index 9ff7c8bd..3a1e85f3 100644
--- a/tests/typ/compute/calc.typ
+++ b/tests/typ/compute/calc.typ
@@ -191,10 +191,14 @@
 #calc.min()
 
 ---
-// Error: 14-18 cannot compare integer and string
+// Error: 14-18 cannot compare string and integer
 #calc.min(1, "hi")
 
 ---
+// Error: 16-19 cannot compare 1pt with 1em
+#calc.max(1em, 1pt)
+
+---
 // Test the `range` function.
 #test(range(4), (0, 1, 2, 3))
 #test(range(1, 4), (1, 2, 3))