refactor(tvix/eval): remove extra Rc<..> around Value::Attrs

The `im::OrdMap` is already small and cheap to copy while sharing
memory, so this is not required anymore.

Only the `KV` variant may have slightly larger content, but in
practice this doesn't seem to make a difference when comparing the two
variants and this one is less complicated.

Change-Id: I64a563b209a2444125653777551373cb2989ca7d
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7677
Reviewed-by: sterni <sternenseemann@systemli.org>
Tested-by: BuildkiteCI

tvix/eval/src/builtins/mod.rs

@@ -21,7 +21,7 @@ use crate::{
     vm::VM,
 };
 
-use crate::{arithmetic_op, unwrap_or_clone_rc};
+use crate::arithmetic_op;
 
 use self::versions::{VersionPart, VersionPartsIter};
 
@@ -1022,8 +1022,8 @@ fn placeholders() -> Vec<Builtin> {
                 // values on the fields that a real derivation would contain.
                 //
                 // Crucially this means we do not yet *validate* the values either.
-                let attrs = unwrap_or_clone_rc(args[0].to_attrs()?);
-                let attrs = attrs.update(NixAttrs::from_iter(
+                let input = args[0].to_attrs()?;
+                let attrs = input.update(NixAttrs::from_iter(
                     [
                         (
                             "outPath",
@@ -1038,7 +1038,7 @@ fn placeholders() -> Vec<Builtin> {
                     .into_iter(),
                 ));
 
-                Ok(Value::Attrs(Rc::new(attrs)))
+                Ok(Value::Attrs(Box::new(attrs)))
             },
         ),
     ]

tvix/eval/src/lib.rs

@@ -37,7 +37,6 @@ mod test_utils;
 mod tests;
 
 use std::path::PathBuf;
-use std::rc::Rc;
 use std::str::FromStr;
 use std::sync::Arc;
 
@@ -63,12 +62,6 @@ pub mod internal {
     pub use crate::vm::VM;
 }
 
-// TODO: use Rc::unwrap_or_clone once it is stabilised.
-// https://doc.rust-lang.org/std/rc/struct.Rc.html#method.unwrap_or_clone
-pub(crate) fn unwrap_or_clone_rc<T: Clone>(rc: Rc<T>) -> T {
-    Rc::try_unwrap(rc).unwrap_or_else(|rc| (*rc).clone())
-}
-
 /// An `Evaluation` represents how a piece of Nix code is evaluated. It can be
 /// instantiated and configured directly, or it can be accessed through the
 /// various simplified helper methods available below.

tvix/eval/src/value/attrs.rs

@@ -173,6 +173,17 @@ impl NixAttrs {
         Self(AttrsRep::Empty)
     }
 
+    /// Compare two attribute sets by pointer equality. Only makes
+    /// sense for some attribute set reprsentations, i.e. returning
+    /// `false` does not mean that the two attribute sets do not have
+    /// equal *content*.
+    pub fn ptr_eq(&self, other: &Self) -> bool {
+        match (&self.0, &other.0) {
+            (AttrsRep::Im(lhs), AttrsRep::Im(rhs)) => lhs.ptr_eq(rhs),
+            _ => false,
+        }
+    }
+
     /// Return an attribute set containing the merge of the two
     /// provided sets. Keys from the `other` set have precedence.
     pub fn update(self, other: Self) -> Self {

tvix/eval/src/value/mod.rs

@@ -39,7 +39,7 @@ pub enum Value {
     Float(f64),
     String(NixString),
     Path(PathBuf),
-    Attrs(Rc<NixAttrs>),
+    Attrs(Box<NixAttrs>),
     List(NixList),
     Closure(Rc<Closure>), // must use Rc<Closure> here in order to get proper pointer equality
     Builtin(Builtin),
@@ -154,7 +154,7 @@ where
 impl Value {
     /// Construct a [`Value::Attrs`] from a [`NixAttrs`].
     pub fn attrs(attrs: NixAttrs) -> Self {
-        Self::Attrs(Rc::new(attrs))
+        Self::Attrs(Box::new(attrs))
     }
 }
 
@@ -303,7 +303,7 @@ impl Value {
     gen_cast!(as_int, i64, "int", Value::Integer(x), *x);
     gen_cast!(as_float, f64, "float", Value::Float(x), *x);
     gen_cast!(to_str, NixString, "string", Value::String(s), s.clone());
-    gen_cast!(to_attrs, Rc<NixAttrs>, "set", Value::Attrs(a), a.clone());
+    gen_cast!(to_attrs, Box<NixAttrs>, "set", Value::Attrs(a), a.clone());
     gen_cast!(to_list, NixList, "list", Value::List(l), l.clone());
     gen_cast!(
         as_closure,

tvix/eval/src/vm.rs

@@ -12,7 +12,6 @@ use crate::{
     observer::RuntimeObserver,
     opcode::{CodeIdx, Count, JumpOffset, OpCode, StackIdx, UpvalueIdx},
     spans::LightSpan,
-    unwrap_or_clone_rc,
     upvalues::Upvalues,
     value::{Builtin, Closure, CoercionKind, Lambda, NixAttrs, NixList, Thunk, Value},
     warnings::{EvalWarning, WarningKind},
@@ -584,7 +583,7 @@ impl<'o> VM<'o> {
                 (Value::List(_), _) => break false,
 
                 (Value::Attrs(a1), Value::Attrs(a2)) => {
-                    if allow_pointer_equality_on_functions_and_thunks && Rc::ptr_eq(&a1, &a2) {
+                    if allow_pointer_equality_on_functions_and_thunks && a1.ptr_eq(&a2) {
                         continue;
                     }
                     allow_pointer_equality_on_functions_and_thunks = true;
@@ -620,8 +619,8 @@ impl<'o> VM<'o> {
                         }
                         _ => {}
                     }
-                    let iter1 = unwrap_or_clone_rc(a1).into_iter_sorted();
-                    let iter2 = unwrap_or_clone_rc(a2).into_iter_sorted();
+                    let iter1 = a1.into_iter_sorted();
+                    let iter2 = a2.into_iter_sorted();
                     if iter1.len() != iter2.len() {
                         break false;
                     }
@@ -745,10 +744,10 @@ impl<'o> VM<'o> {
             OpCode::OpAttrs(Count(count)) => self.run_attrset(count)?,
 
             OpCode::OpAttrsUpdate => {
-                let rhs = unwrap_or_clone_rc(fallible!(self, self.pop().to_attrs()));
-                let lhs = unwrap_or_clone_rc(fallible!(self, self.pop().to_attrs()));
+                let rhs = fallible!(self, self.pop().to_attrs());
+                let lhs = fallible!(self, self.pop().to_attrs());
 
-                self.push(Value::attrs(lhs.update(rhs)))
+                self.push(Value::attrs(lhs.update(*rhs)))
             }
 
             OpCode::OpAttrsSelect => {