From 06494742062e77036827dfc7c91dea507b44447f Mon Sep 17 00:00:00 2001 From: Adam Joseph Date: Mon, 31 Oct 2022 17:13:38 -0700 Subject: fix(tvix/eval): remove impl PartialEq for Value 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 Change-Id: Iacd3726fefc7fc1edadcd7e9b586e04cf8466775 Reviewed-on: https://cl.tvl.fyi/c/depot/+/7144 Reviewed-by: kanepyork Reviewed-by: tazjin Tested-by: BuildkiteCI --- tvix/eval/src/chunk.rs | 2 +- tvix/eval/src/upvalues.rs | 2 +- tvix/eval/src/value/attrs.rs | 4 ++-- tvix/eval/src/value/attrs/tests.rs | 35 +++++++++++++++++++++-------------- tvix/eval/src/value/function.rs | 4 ++-- tvix/eval/src/value/list.rs | 2 +- tvix/eval/src/value/mod.rs | 2 +- tvix/eval/src/value/thunk.rs | 4 ++-- 8 files changed, 31 insertions(+), 24 deletions(-) diff --git a/tvix/eval/src/chunk.rs b/tvix/eval/src/chunk.rs index 9f2fdf54dc82..e9be0d2a633e 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, pub constants: Vec, diff --git a/tvix/eval/src/upvalues.rs b/tvix/eval/src/upvalues.rs index 450ea130ff24..687d6850ccfa 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 ca4d17178a5e..59e477db4acf 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), @@ -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 637114c89bc7..65d3c8d7ca0e 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![ - (NixString::NAME_REF, &meaning_val), - (NixString::VALUE_REF, &forty_two_val) - ] - ); + let mut iter = kv_attrs + .iter() + .collect::>() + .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![(&NixString::from("key"), &Value::String("value".into()))], - ); + let mut iter = attrs.iter().collect::>().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 f282928df9f9..e543b1aafd44 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, pub upvalues: Upvalues, diff --git a/tvix/eval/src/value/list.rs b/tvix/eval/src/value/list.rs index da8f70caacaf..085c85346b65 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); impl TotalDisplay for NixList { diff --git a/tvix/eval/src/value/mod.rs b/tvix/eval/src/value/mod.rs index ff9dccb0b3c2..db72081e8dcf 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 bcf8e2fc8756..549534b04ec7 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>); impl Thunk { -- cgit 1.4.1