From 278bccc1ea3542610012177171234ad1c3c44dcf Mon Sep 17 00:00:00 2001
From: Griffin Smith <root@gws.fyi>
Date: Sat, 8 Oct 2022 13:33:33 -0400
Subject: [PATCH] refactor(tvix/eval): Encapsulate Value::Attrs construction

Factor out the construction of Value::Attrs (including the Rc) into a
new `attrs` constructor function, to abstract away the presence of the
Rc itself.

Change-Id: I42fd4c3841e1db368db999ddd651277ff995f025
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6892
Autosubmit: grfn <grfn@gws.fyi>
Reviewed-by: sterni <sternenseemann@systemli.org>
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
---
 tvix/eval/src/builtins/mod.rs    | 3 +--
 tvix/eval/src/value/arbitrary.rs | 4 ++--
 tvix/eval/src/value/mod.rs       | 8 ++++++++
 tvix/eval/src/vm.rs              | 4 ++--
 4 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/tvix/eval/src/builtins/mod.rs b/tvix/eval/src/builtins/mod.rs
index b0aab352d..eeb784ce4 100644
--- a/tvix/eval/src/builtins/mod.rs
+++ b/tvix/eval/src/builtins/mod.rs
@@ -6,7 +6,6 @@
 use std::cmp;
 use std::collections::{BTreeMap, HashMap};
 use std::path::PathBuf;
-use std::rc::Rc;
 
 use crate::{
     errors::ErrorKind,
@@ -439,7 +438,7 @@ pub fn global_builtins() -> HashMap<&'static str, Value> {
         }
     }
 
-    globals.insert("builtins", Value::Attrs(Rc::new(builtins)));
+    globals.insert("builtins", Value::attrs(builtins));
 
     globals
 }
diff --git a/tvix/eval/src/value/arbitrary.rs b/tvix/eval/src/value/arbitrary.rs
index 20be3732e..cd7629cfb 100644
--- a/tvix/eval/src/value/arbitrary.rs
+++ b/tvix/eval/src/value/arbitrary.rs
@@ -1,7 +1,7 @@
 //! Support for configurable generation of arbitrary nix values
 
 use proptest::{prelude::*, strategy::BoxedStrategy};
-use std::{ffi::OsString, rc::Rc};
+use std::ffi::OsString;
 
 use super::{NixAttrs, NixList, NixString, Value};
 
@@ -70,7 +70,7 @@ fn non_internal_value() -> impl Strategy<Value = Value> {
                 Default::default(),
                 Parameters::Strategy(inner.clone())
             ))
-            .prop_map(|a| Value::Attrs(Rc::new(a))),
+            .prop_map(Value::attrs),
             any_with::<NixList>((Default::default(), Parameters::Strategy(inner)))
                 .prop_map(Value::List)
         ]
diff --git a/tvix/eval/src/value/mod.rs b/tvix/eval/src/value/mod.rs
index e6a6052c2..dd61ecf0b 100644
--- a/tvix/eval/src/value/mod.rs
+++ b/tvix/eval/src/value/mod.rs
@@ -115,6 +115,14 @@ impl<'a> Deref for ForceResult<'a> {
     }
 }
 
+/// Constructors
+impl Value {
+    /// Construct a [`Value::Attrs`] from a [`NixAttrs`].
+    pub fn attrs(attrs: NixAttrs) -> Self {
+        Self::Attrs(Rc::new(attrs))
+    }
+}
+
 impl Value {
     /// Coerce a `Value` to a string. See `CoercionKind` for a rundown of what
     /// input types are accepted under what circumstances.
diff --git a/tvix/eval/src/vm.rs b/tvix/eval/src/vm.rs
index d616b9ce1..9d28c7be1 100644
--- a/tvix/eval/src/vm.rs
+++ b/tvix/eval/src/vm.rs
@@ -374,7 +374,7 @@ impl<'o> VM<'o> {
                     let rhs = unwrap_or_clone_rc(fallible!(self, self.pop().to_attrs()));
                     let lhs = unwrap_or_clone_rc(fallible!(self, self.pop().to_attrs()));
 
-                    self.push(Value::Attrs(Rc::new(lhs.update(rhs))))
+                    self.push(Value::attrs(lhs.update(rhs)))
                 }
 
                 OpCode::OpAttrsSelect => {
@@ -641,7 +641,7 @@ impl<'o> VM<'o> {
             NixAttrs::construct(count, self.stack.split_off(self.stack.len() - count * 2))
         );
 
-        self.push(Value::Attrs(Rc::new(attrs)));
+        self.push(Value::attrs(attrs));
         Ok(())
     }