refactor(tvix/value): ensure internal attrs representation is hidden r/4481

Wraps the attrs representation in an additional newtype struct with a
private field in order to hide the representation from other modules.

This is done in order to avoid accidental leakage of the internals
outside of value::attrs.

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

2 files changed, 53 insertions, 47 deletions

diff --git a/tvix/eval/src/value/attrs.rs b/tvix/eval/src/value/attrs.rs
index e7da6ee621..9204a5bb95 100644
--- a/tvix/eval/src/value/attrs.rs
+++ b/tvix/eval/src/value/attrs.rs
@@ -18,29 +18,55 @@ use super::Value;
 mod tests;
 
 #[derive(Clone, Debug)]
-pub enum NixAttrs {
+enum AttrsRep {
     Empty,
     Map(BTreeMap<NixString, Value>),
     KV { name: Value, value: Value },
 }
 
+impl AttrsRep {
+    /// Retrieve reference to a mutable map inside of an attrs,
+    /// optionally changing the representation if required.
+    fn map_mut(&mut self) -> &mut BTreeMap<NixString, Value> {
+        match self {
+            AttrsRep::Map(m) => m,
+
+            AttrsRep::Empty => {
+                *self = AttrsRep::Map(BTreeMap::new());
+                self.map_mut()
+            }
+
+            AttrsRep::KV { name, value } => {
+                *self = AttrsRep::Map(BTreeMap::from([
+                    ("name".into(), std::mem::replace(name, Value::Blackhole)),
+                    ("value".into(), std::mem::replace(value, Value::Blackhole)),
+                ]));
+                self.map_mut()
+            }
+        }
+    }
+}
+
+#[derive(Clone, Debug)]
+pub struct NixAttrs(AttrsRep);
+
 impl Display for NixAttrs {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         f.write_str("{ ")?;
 
-        match self {
-            NixAttrs::KV { name, value } => {
+        match &self.0 {
+            AttrsRep::KV { name, value } => {
                 f.write_fmt(format_args!("name = {}; ", name))?;
                 f.write_fmt(format_args!("value = {}; ", value))?;
             }
 
-            NixAttrs::Map(map) => {
+            AttrsRep::Map(map) => {
                 for (name, value) in map.iter() {
                     f.write_fmt(format_args!("{} = {}; ", name.ident_str(), value))?;
                 }
             }
 
-            NixAttrs::Empty => { /* no values to print! */ }
+            AttrsRep::Empty => { /* no values to print! */ }
         }
 
         f.write_str("}")
@@ -56,22 +82,22 @@ impl PartialEq for NixAttrs {
 impl NixAttrs {
     // Update one attribute set with the values of the other.
     pub fn update(&self, other: &Self) -> Self {
-        match (self, other) {
+        match (&self.0, &other.0) {
             // Short-circuit on some optimal cases:
-            (NixAttrs::Empty, NixAttrs::Empty) => NixAttrs::Empty,
-            (NixAttrs::Empty, _) => other.clone(),
-            (_, NixAttrs::Empty) => self.clone(),
-            (NixAttrs::KV { .. }, NixAttrs::KV { .. }) => other.clone(),
+            (AttrsRep::Empty, AttrsRep::Empty) => NixAttrs(AttrsRep::Empty),
+            (AttrsRep::Empty, _) => other.clone(),
+            (_, AttrsRep::Empty) => self.clone(),
+            (AttrsRep::KV { .. }, AttrsRep::KV { .. }) => other.clone(),
 
             // Slightly more advanced, but still optimised updates
-            (NixAttrs::Map(m), NixAttrs::KV { name, value }) => {
+            (AttrsRep::Map(m), AttrsRep::KV { name, value }) => {
                 let mut m = m.clone();
                 m.insert(NixString::NAME, name.clone());
                 m.insert(NixString::VALUE, value.clone());
-                NixAttrs::Map(m)
+                NixAttrs(AttrsRep::Map(m))
             }
 
-            (NixAttrs::KV { name, value }, NixAttrs::Map(m)) => {
+            (AttrsRep::KV { name, value }, AttrsRep::Map(m)) => {
                 let mut m = m.clone();
 
                 match m.entry(NixString::NAME) {
@@ -94,36 +120,15 @@ impl NixAttrs {
                     }
                 };
 
-                NixAttrs::Map(m)
+                NixAttrs(AttrsRep::Map(m))
             }
 
             // Plain merge of maps.
-            (NixAttrs::Map(m1), NixAttrs::Map(m2)) => {
+            (AttrsRep::Map(m1), AttrsRep::Map(m2)) => {
                 let mut m1 = m1.clone();
                 let mut m2 = m2.clone();
                 m1.append(&mut m2);
-                NixAttrs::Map(m1)
-            }
-        }
-    }
-
-    /// Retrieve reference to a mutable map inside of an attrs,
-    /// optionally changing the representation if required.
-    fn map_mut(&mut self) -> &mut BTreeMap<NixString, Value> {
-        match self {
-            NixAttrs::Map(m) => m,
-
-            NixAttrs::Empty => {
-                *self = NixAttrs::Map(BTreeMap::new());
-                self.map_mut()
-            }
-
-            NixAttrs::KV { name, value } => {
-                *self = NixAttrs::Map(BTreeMap::from([
-                    ("name".into(), std::mem::replace(name, Value::Blackhole)),
-                    ("value".into(), std::mem::replace(value, Value::Blackhole)),
-                ]));
-                self.map_mut()
+                NixAttrs(AttrsRep::Map(m1))
             }
         }
     }
@@ -140,7 +145,7 @@ impl NixAttrs {
 
         // Optimisation: Empty attribute set
         if count == 0 {
-            return Ok(NixAttrs::Empty);
+            return Ok(NixAttrs(AttrsRep::Empty));
         }
 
         // Optimisation: KV pattern
@@ -151,7 +156,7 @@ impl NixAttrs {
         }
 
         // TODO(tazjin): extend_reserve(count) (rust#72631)
-        let mut attrs = NixAttrs::Map(BTreeMap::new());
+        let mut attrs = NixAttrs(AttrsRep::Map(BTreeMap::new()));
 
         for _ in 0..count {
             let value = stack_slice.pop().unwrap();
@@ -217,16 +222,16 @@ fn attempt_optimise_kv(slice: &mut [Value]) -> Option<NixAttrs> {
         }
     };
 
-    Some(NixAttrs::KV {
+    Some(NixAttrs(AttrsRep::KV {
         name: std::mem::replace(&mut slice[name_idx], Value::Blackhole),
         value: std::mem::replace(&mut slice[value_idx], Value::Blackhole),
-    })
+    }))
 }
 
 // Set an attribute on an in-construction attribute set, while
 // checking against duplicate keys.
 fn set_attr(attrs: &mut NixAttrs, key: NixString, value: Value) -> EvalResult<()> {
-    let attrs = attrs.map_mut();
+    let attrs = attrs.0.map_mut();
     let entry = attrs.entry(key);
 
     match entry {
@@ -261,7 +266,7 @@ fn set_nested_attr(
         return set_attr(attrs, key, value);
     }
 
-    let attrs = attrs.map_mut();
+    let attrs = attrs.0.map_mut();
     let entry = attrs.entry(key);
 
     // If there is not we go one step further down, in which case we
@@ -273,7 +278,7 @@ fn set_nested_attr(
     match entry {
         // Vacant entry -> new attribute set is needed.
         std::collections::btree_map::Entry::Vacant(entry) => {
-            let mut map = NixAttrs::Map(BTreeMap::new());
+            let mut map = NixAttrs(AttrsRep::Map(BTreeMap::new()));
 
             // TODO(tazjin): technically recursing further is not
             // required, we can create the whole hierarchy here, but
diff --git a/tvix/eval/src/value/attrs/tests.rs b/tvix/eval/src/value/attrs/tests.rs
index 9bd8305482..647a358655 100644
--- a/tvix/eval/src/value/attrs/tests.rs
+++ b/tvix/eval/src/value/attrs/tests.rs
@@ -5,7 +5,7 @@ fn test_empty_attrs() {
     let attrs = NixAttrs::construct(0, vec![]).expect("empty attr construction should succeed");
 
     assert!(
-        matches!(attrs, NixAttrs::Empty),
+        matches!(attrs, NixAttrs(AttrsRep::Empty)),
         "empty attribute set should use optimised representation"
     );
 }
@@ -19,7 +19,7 @@ fn test_simple_attrs() {
     .expect("simple attr construction should succeed");
 
     assert!(
-        matches!(attrs, NixAttrs::Map(_)),
+        matches!(attrs, NixAttrs(AttrsRep::Map(_))),
         "simple attribute set should use map representation",
     )
 }
@@ -43,7 +43,8 @@ fn test_kv_attrs() {
     .expect("constructing K/V pair attrs should succeed");
 
     match kv_attrs {
-        NixAttrs::KV { name, value } if name == meaning_val || value == forty_two_val => {}
+        NixAttrs(AttrsRep::KV { name, value }) if name == meaning_val || value == forty_two_val => {
+        }
 
         _ => panic!(
             "K/V attribute set should use optimised representation, but got {:?}",