From 8a40f75c2d0cd03e3c3f680f4bd062f0611f2ab8 Mon Sep 17 00:00:00 2001
From: Adam Joseph <adam@westernsemico.com>
Date: Fri, 8 Dec 2023 23:25:21 -0800
Subject: fix(tvix/eval): never use partial_cmp() (partial fix b/338)

This is part of a fix for b/338.

We should never use PartialOrd::partial_cmp().

All Nix types except floats are obviously totally-ordered.  In
addition, it turns out that because Nix treats division by zero
rather than producing a NaN, and because it does not support
"negative zero", even floats are in fact totally ordered in Nix.

Therefore, every call to PartialOrd::partial_cmp() in tvix is an
error.  We have to *implement* this function, but we should never
call it on built-in types.

Moreover, nix_cmp_ordering() currently returns an Option<Ordering>.
I'm not sure what was going on there, since it's impossible for it
to return None.  This commit fixes it to return simply Ordering
rather than Option<Ordering>.

Change-Id: If5c084164cf19cfb38c5a15554c0422faa5f895d
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10218
Autosubmit: Adam Joseph <adam@westernsemico.com>
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
---
 tvix/eval/src/builtins/mod.rs |  2 +-
 tvix/eval/src/value/mod.rs    | 29 +++++++++++------------------
 tvix/eval/src/vm/macros.rs    |  8 ++++----
 3 files changed, 16 insertions(+), 23 deletions(-)

(limited to 'tvix/eval')

diff --git a/tvix/eval/src/builtins/mod.rs b/tvix/eval/src/builtins/mod.rs
index ead916377f..35b7549ffe 100644
--- a/tvix/eval/src/builtins/mod.rs
+++ b/tvix/eval/src/builtins/mod.rs
@@ -612,7 +612,7 @@ mod pure_builtins {
     async fn builtin_less_than(co: GenCo, x: Value, y: Value) -> Result<Value, ErrorKind> {
         Ok(Value::Bool(matches!(
             x.nix_cmp_ordering(y, co).await?,
-            Some(Ordering::Less)
+            Ordering::Less
         )))
     }
 
diff --git a/tvix/eval/src/value/mod.rs b/tvix/eval/src/value/mod.rs
index f4ed7733ee..c0bdc5fc5e 100644
--- a/tvix/eval/src/value/mod.rs
+++ b/tvix/eval/src/value/mod.rs
@@ -600,18 +600,9 @@ impl Value {
     // TODO(amjoseph): de-asyncify this (when called directly by the VM)
     /// Compare `self` against other using (fallible) Nix ordering semantics.
     ///
-    /// Note that as this returns an `Option<Ordering>` it can not directly be
-    /// used as a generator function in the VM. The exact use depends on the
-    /// callsite, as the meaning is interpreted in different ways e.g. based on
-    /// the comparison operator used.
-    ///
     /// The function is intended to be used from within other generator
     /// functions or `gen!` blocks.
-    pub async fn nix_cmp_ordering(
-        self,
-        other: Self,
-        co: GenCo,
-    ) -> Result<Option<Ordering>, ErrorKind> {
+    pub async fn nix_cmp_ordering(self, other: Self, co: GenCo) -> Result<Ordering, ErrorKind> {
         Self::nix_cmp_ordering_(self, other, co).await
     }
 
@@ -619,19 +610,21 @@ impl Value {
         mut myself: Self,
         mut other: Self,
         co: GenCo,
-    ) -> Result<Option<Ordering>, ErrorKind> {
+    ) -> Result<Ordering, ErrorKind> {
         'outer: loop {
             match (myself, other) {
                 // same types
-                (Value::Integer(i1), Value::Integer(i2)) => return Ok(i1.partial_cmp(&i2)),
-                (Value::Float(f1), Value::Float(f2)) => return Ok(f1.partial_cmp(&f2)),
-                (Value::String(s1), Value::String(s2)) => return Ok(s1.partial_cmp(&s2)),
+                // Nix does not support NaN or "negative infinity",
+                // so its floats are in fact totally ordered.
+                (Value::Integer(i1), Value::Integer(i2)) => return Ok(i1.cmp(&i2)),
+                (Value::Float(f1), Value::Float(f2)) => return Ok(f1.total_cmp(&f2)),
+                (Value::String(s1), Value::String(s2)) => return Ok(s1.cmp(&s2)),
                 (Value::List(l1), Value::List(l2)) => {
                     for i in 0.. {
                         if i == l2.len() {
-                            return Ok(Some(Ordering::Greater));
+                            return Ok(Ordering::Greater);
                         } else if i == l1.len() {
-                            return Ok(Some(Ordering::Less));
+                            return Ok(Ordering::Less);
                         } else if !generators::check_equality(
                             &co,
                             l1[i].clone(),
@@ -651,8 +644,8 @@ impl Value {
                 }
 
                 // different types
-                (Value::Integer(i1), Value::Float(f2)) => return Ok((i1 as f64).partial_cmp(&f2)),
-                (Value::Float(f1), Value::Integer(i2)) => return Ok(f1.partial_cmp(&(i2 as f64))),
+                (Value::Integer(i1), Value::Float(f2)) => return Ok((i1 as f64).total_cmp(&f2)),
+                (Value::Float(f1), Value::Integer(i2)) => return Ok(f1.total_cmp(&(i2 as f64))),
 
                 // unsupported types
                 (lhs, rhs) => {
diff --git a/tvix/eval/src/vm/macros.rs b/tvix/eval/src/vm/macros.rs
index 969b508077..8a536ee466 100644
--- a/tvix/eval/src/vm/macros.rs
+++ b/tvix/eval/src/vm/macros.rs
@@ -53,18 +53,18 @@ macro_rules! cmp_op {
     }};
 
     (@order < $ordering:expr) => {
-        $ordering == Some(Ordering::Less)
+        $ordering == Ordering::Less
     };
 
     (@order > $ordering:expr) => {
-        $ordering == Some(Ordering::Greater)
+        $ordering == Ordering::Greater
     };
 
     (@order <= $ordering:expr) => {
-        !matches!($ordering, None | Some(Ordering::Greater))
+        matches!($ordering, Ordering::Equal | Ordering::Less)
     };
 
     (@order >= $ordering:expr) => {
-        !matches!($ordering, None | Some(Ordering::Less))
+        matches!($ordering, Ordering::Equal | Ordering::Greater)
     };
 }
-- 
cgit 1.4.1