fix(tvix/eval): remove impl PartialEq for Value r/5236

It isn't possible to implement PartialEq properly for Value, because
any sensible implementation needs to force() thunks, which cannot be
done without a `&mut VM`.

The existing derive(PartialEq) has false negatives, which caused the
bug which cl/7142 fixed.  Fortunately that bug was easy to find, but
a silent false negative deep within the bowels of nixpkgs could be a
real nightmare to hunt down.

Let's just remove the PartialEq impl for Value, and the other
derive(PartialEq)'s that depend on it.

Signed-off-by: Adam Joseph <adam@westernsemico.com>
Change-Id: Iacd3726fefc7fc1edadcd7e9b586e04cf8466775
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7144
Reviewed-by: kanepyork <rikingcoding@gmail.com>
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI

8 files changed, 31 insertions, 24 deletions

diff --git a/tvix/eval/src/chunk.rs b/tvix/eval/src/chunk.rs
index 9f2fdf54dc..e9be0d2a63 100644
--- a/tvix/eval/src/chunk.rs
+++ b/tvix/eval/src/chunk.rs
@@ -28,7 +28,7 @@ struct SourceSpan {
 /// A chunk is a representation of a sequence of bytecode
 /// instructions, associated constants and additional metadata as
 /// emitted by the compiler.
-#[derive(Debug, Default, PartialEq)]
+#[derive(Debug, Default)]
 pub struct Chunk {
     pub code: Vec<OpCode>,
     pub constants: Vec<Value>,
diff --git a/tvix/eval/src/upvalues.rs b/tvix/eval/src/upvalues.rs
index 450ea130ff..687d6850cc 100644
--- a/tvix/eval/src/upvalues.rs
+++ b/tvix/eval/src/upvalues.rs
@@ -24,7 +24,7 @@ use crate::{opcode::UpvalueIdx, Value};
 ///   `let`, `name:` or `{name}:`
 /// - Dynamic identifiers, which are not bound in any enclosing
 ///   scope
-#[derive(Clone, Debug, PartialEq)]
+#[derive(Clone, Debug)]
 pub struct Upvalues {
     /// The upvalues of static identifiers.  Each static identifier
     /// is assigned an integer identifier at compile time, which is
diff --git a/tvix/eval/src/value/attrs.rs b/tvix/eval/src/value/attrs.rs
index ca4d17178a..59e477db4a 100644
--- a/tvix/eval/src/value/attrs.rs
+++ b/tvix/eval/src/value/attrs.rs
@@ -19,7 +19,7 @@ use super::Value;
 #[cfg(test)]
 mod tests;
 
-#[derive(Clone, Debug, PartialEq)]
+#[derive(Clone, Debug)]
 enum AttrsRep {
     Empty,
     Map(BTreeMap<NixString, Value>),
@@ -79,7 +79,7 @@ impl AttrsRep {
 }
 
 #[repr(transparent)]
-#[derive(Clone, Debug, PartialEq)]
+#[derive(Clone, Debug)]
 pub struct NixAttrs(AttrsRep);
 
 impl TotalDisplay for NixAttrs {
diff --git a/tvix/eval/src/value/attrs/tests.rs b/tvix/eval/src/value/attrs/tests.rs
index 637114c89b..65d3c8d7ca 100644
--- a/tvix/eval/src/value/attrs/tests.rs
+++ b/tvix/eval/src/value/attrs/tests.rs
@@ -80,8 +80,9 @@ fn test_kv_attrs() {
     .expect("constructing K/V pair attrs should succeed");
 
     match kv_attrs {
-        NixAttrs(AttrsRep::KV { name, value }) if name == meaning_val || value == forty_two_val => {
-        }
+        NixAttrs(AttrsRep::KV { name, value })
+            if name.to_str().unwrap() == meaning_val.to_str().unwrap()
+                || value.to_str().unwrap() == forty_two_val.to_str().unwrap() => {}
 
         _ => panic!(
             "K/V attribute set should use optimised representation, but got {:?}",
@@ -93,7 +94,7 @@ fn test_kv_attrs() {
 #[test]
 fn test_empty_attrs_iter() {
     let attrs = NixAttrs::construct(0, vec![]).unwrap();
-    assert_eq!(attrs.iter().next(), None);
+    assert!(attrs.iter().next().is_none());
 }
 
 #[test]
@@ -114,13 +115,18 @@ fn test_kv_attrs_iter() {
     )
     .expect("constructing K/V pair attrs should succeed");
 
-    assert_eq!(
-        kv_attrs.iter().collect::<Vec<_>>(),
-        vec![
-            (NixString::NAME_REF, &meaning_val),
-            (NixString::VALUE_REF, &forty_two_val)
-        ]
-    );
+    let mut iter = kv_attrs
+        .iter()
+        .collect::<Vec<_>>()
+        .into_iter()
+        .map(|(k, v)| (k, v));
+    let (k, v) = iter.next().unwrap();
+    assert!(k == NixString::NAME_REF);
+    assert!(v.to_str().unwrap() == meaning_val.to_str().unwrap());
+    let (k, v) = iter.next().unwrap();
+    assert!(k == NixString::VALUE_REF);
+    assert!(v.as_int().unwrap() == forty_two_val.as_int().unwrap());
+    assert!(iter.next().is_none());
 }
 
 #[test]
@@ -131,8 +137,9 @@ fn test_map_attrs_iter() {
     )
     .expect("simple attr construction should succeed");
 
-    assert_eq!(
-        attrs.iter().collect::<Vec<_>>(),
-        vec![(&NixString::from("key"), &Value::String("value".into()))],
-    );
+    let mut iter = attrs.iter().collect::<Vec<_>>().into_iter();
+    let (k, v) = iter.next().unwrap();
+    assert!(k == &NixString::from("key"));
+    assert!(v.to_str().unwrap().as_str() == "value");
+    assert!(iter.next().is_none());
 }
diff --git a/tvix/eval/src/value/function.rs b/tvix/eval/src/value/function.rs
index f282928df9..e543b1aafd 100644
--- a/tvix/eval/src/value/function.rs
+++ b/tvix/eval/src/value/function.rs
@@ -39,7 +39,7 @@ impl Formals {
 /// OpThunkSuspended referencing it.  At runtime `Lambda` is usually wrapped
 /// in `Rc` to avoid copying the `Chunk` it holds (which can be
 /// quite large).
-#[derive(Debug, Default, PartialEq)]
+#[derive(Debug, Default)]
 pub struct Lambda {
     pub(crate) chunk: Chunk,
 
@@ -62,7 +62,7 @@ impl Lambda {
     }
 }
 
-#[derive(Clone, Debug, PartialEq)]
+#[derive(Clone, Debug)]
 pub struct Closure {
     pub lambda: Rc<Lambda>,
     pub upvalues: Upvalues,
diff --git a/tvix/eval/src/value/list.rs b/tvix/eval/src/value/list.rs
index da8f70caac..085c85346b 100644
--- a/tvix/eval/src/value/list.rs
+++ b/tvix/eval/src/value/list.rs
@@ -9,7 +9,7 @@ use super::TotalDisplay;
 use super::Value;
 
 #[repr(transparent)]
-#[derive(Clone, Debug, PartialEq)]
+#[derive(Clone, Debug)]
 pub struct NixList(Vec<Value>);
 
 impl TotalDisplay for NixList {
diff --git a/tvix/eval/src/value/mod.rs b/tvix/eval/src/value/mod.rs
index ff9dccb0b3..db72081e8d 100644
--- a/tvix/eval/src/value/mod.rs
+++ b/tvix/eval/src/value/mod.rs
@@ -31,7 +31,7 @@ pub use thunk::Thunk;
 use self::thunk::ThunkSet;
 
 #[warn(variant_size_differences)]
-#[derive(Clone, Debug, PartialEq)]
+#[derive(Clone, Debug)]
 pub enum Value {
     Null,
     Bool(bool),
diff --git a/tvix/eval/src/value/thunk.rs b/tvix/eval/src/value/thunk.rs
index bcf8e2fc87..549534b04e 100644
--- a/tvix/eval/src/value/thunk.rs
+++ b/tvix/eval/src/value/thunk.rs
@@ -41,7 +41,7 @@ use super::{Lambda, TotalDisplay};
 /// Upvalues must be finalised before leaving the initial state
 /// (Suspended or RecursiveClosure).  The [`value()`] function may
 /// not be called until the thunk is in the final state (Evaluated).
-#[derive(Clone, Debug, PartialEq)]
+#[derive(Clone, Debug)]
 enum ThunkRepr {
     /// Thunk is closed over some values, suspended and awaiting
     /// execution.
@@ -63,7 +63,7 @@ enum ThunkRepr {
 /// evaluation due to self-reference or lazy semantics (or both).
 /// Every reference cycle involving `Value`s will contain at least
 /// one `Thunk`.
-#[derive(Clone, Debug, PartialEq)]
+#[derive(Clone, Debug)]
 pub struct Thunk(Rc<RefCell<ThunkRepr>>);
 
 impl Thunk {