chore(tvix/eval): remove existing nested key implementation r/4990

This implementation, which only ever worked for non-recursive
attribute sets, is no longer needed and thus removed here.

We have a new implementation of these nested keys coming up instead.

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

4 files changed, 3 insertions, 101 deletions

diff --git a/tvix/eval/src/opcode.rs b/tvix/eval/src/opcode.rs
index 9ddd1953210c..c2eabba1c28e 100644
--- a/tvix/eval/src/opcode.rs
+++ b/tvix/eval/src/opcode.rs
@@ -91,7 +91,6 @@ pub enum OpCode {
     /// Note that this takes the count of *pairs*, not the number of *stack values* - the actual
     /// number of values popped off the stack will be twice the argument to this op
     OpAttrs(Count),
-    OpAttrPath(Count),
     OpAttrsUpdate,
     OpAttrsSelect,
     OpAttrsTrySelect,
diff --git a/tvix/eval/src/value/attrs.rs b/tvix/eval/src/value/attrs.rs
index 5eb258cc9e47..318a8cfa8209 100644
--- a/tvix/eval/src/value/attrs.rs
+++ b/tvix/eval/src/value/attrs.rs
@@ -8,7 +8,6 @@
 use std::collections::btree_map;
 use std::collections::BTreeMap;
 use std::fmt::Display;
-use std::rc::Rc;
 
 use crate::errors::ErrorKind;
 use crate::vm::VM;
@@ -248,23 +247,11 @@ impl NixAttrs {
 
         for _ in 0..count {
             let value = stack_slice.pop().unwrap();
-
-            // It is at this point that nested attribute sets need to
-            // be constructed (if they exist).
-            //
             let key = stack_slice.pop().unwrap();
+
             match key {
                 Value::String(ks) => set_attr(&mut attrs, ks, value)?,
 
-                Value::AttrPath(mut path) => {
-                    set_nested_attr(
-                        &mut attrs,
-                        path.pop().expect("AttrPath is never empty"),
-                        path,
-                        value,
-                    )?;
-                }
-
                 Value::Null => {
                     // This is in fact valid, but leads to the value
                     // being ignored and nothing being set, i.e. `{
@@ -410,66 +397,6 @@ fn set_attr(attrs: &mut NixAttrs, key: NixString, value: Value) -> Result<(), Er
     }
 }
 
-/// Set a nested attribute inside of an attribute set, throwing a
-/// duplicate key error if a non-hashmap entry already exists on the
-/// path.
-///
-/// There is some optimisation potential for this simple implementation
-/// if it becomes a problem.
-fn set_nested_attr(
-    attrs: &mut NixAttrs,
-    key: NixString,
-    mut path: Vec<NixString>,
-    value: Value,
-) -> Result<(), ErrorKind> {
-    // If there is no next key we are at the point where we
-    // should insert the value itself.
-    if path.is_empty() {
-        return set_attr(attrs, key, value);
-    }
-
-    // If there is not we go one step further down, in which case we
-    // need to ensure that there either is no entry, or the existing
-    // entry is a hashmap into which to insert the next value.
-    //
-    // If a value of a different type exists, the user specified a
-    // duplicate key.
-    match attrs.0.map_mut().entry(key) {
-        // Vacant entry -> new attribute set is needed.
-        btree_map::Entry::Vacant(entry) => {
-            let mut map = NixAttrs(AttrsRep::Map(BTreeMap::new()));
-
-            // TODO(tazjin): technically recursing further is not
-            // required, we can create the whole hierarchy here, but
-            // it's noisy.
-            set_nested_attr(&mut map, path.pop().expect("next key exists"), path, value)?;
-
-            entry.insert(Value::Attrs(Rc::new(map)));
-        }
-
-        // Occupied entry: Either error out if there is something
-        // other than attrs, or insert the next value.
-        btree_map::Entry::Occupied(mut entry) => match entry.get_mut() {
-            Value::Attrs(attrs) => {
-                set_nested_attr(
-                    Rc::make_mut(attrs),
-                    path.pop().expect("next key exists"),
-                    path,
-                    value,
-                )?;
-            }
-
-            _ => {
-                return Err(ErrorKind::DuplicateAttrsKey {
-                    key: entry.key().as_str().to_string(),
-                })
-            }
-        },
-    }
-
-    Ok(())
-}
-
 /// Internal helper type to track the iteration status of an iterator
 /// over the name/value representation.
 #[derive(Debug)]
diff --git a/tvix/eval/src/value/mod.rs b/tvix/eval/src/value/mod.rs
index 1ee98c25b2cc..de59691f278f 100644
--- a/tvix/eval/src/value/mod.rs
+++ b/tvix/eval/src/value/mod.rs
@@ -41,7 +41,6 @@ pub enum Value {
     // Internal values that, while they technically exist at runtime,
     // are never returned to or created directly by users.
     Thunk(Thunk),
-    AttrPath(Vec<NixString>),
     AttrNotFound,
     DynamicUpvalueMissing(NixString),
     Blueprint(Rc<Lambda>),
@@ -220,8 +219,7 @@ impl Value {
                 kind,
             }),
 
-            (Value::AttrPath(_), _)
-            | (Value::AttrNotFound, _)
+            (Value::AttrNotFound, _)
             | (Value::DynamicUpvalueMissing(_), _)
             | (Value::Blueprint(_), _)
             | (Value::DeferredUpvalue(_), _) => {
@@ -244,7 +242,6 @@ impl Value {
 
             // Internal types
             Value::Thunk(_)
-            | Value::AttrPath(_)
             | Value::AttrNotFound
             | Value::DynamicUpvalueMissing(_)
             | Value::Blueprint(_)
@@ -338,7 +335,6 @@ impl Display for Value {
             Value::Thunk(t) => t.fmt(f),
 
             // internal types
-            Value::AttrPath(path) => write!(f, "internal[attrpath({})]", path.len()),
             Value::AttrNotFound => f.write_str("internal[not found]"),
             Value::Blueprint(_) => f.write_str("internal[blueprint]"),
             Value::DeferredUpvalue(_) => f.write_str("internal[deferred_upvalue]"),
diff --git a/tvix/eval/src/vm.rs b/tvix/eval/src/vm.rs
index 547772b0fe9b..a71ab14c412c 100644
--- a/tvix/eval/src/vm.rs
+++ b/tvix/eval/src/vm.rs
@@ -308,7 +308,6 @@ impl<'o> VM<'o> {
                 OpCode::OpFalse => self.push(Value::Bool(false)),
 
                 OpCode::OpAttrs(Count(count)) => self.run_attrset(count)?,
-                OpCode::OpAttrPath(Count(count)) => self.run_attr_path(count)?,
 
                 OpCode::OpAttrsUpdate => {
                     let rhs = unwrap_or_clone_rc(fallible!(self, self.pop().to_attrs()));
@@ -588,24 +587,6 @@ impl<'o> VM<'o> {
         }
     }
 
-    /// Construct runtime representation of an attr path (essentially
-    /// just a list of strings).
-    ///
-    /// The difference to the list construction operation is that this
-    /// forces all elements into strings, as attribute set keys are
-    /// required to be strict in Nix.
-    fn run_attr_path(&mut self, count: usize) -> EvalResult<()> {
-        debug_assert!(count > 1, "AttrPath needs at least two fragments");
-        let mut path = Vec::with_capacity(count);
-
-        for _ in 0..count {
-            path.push(fallible!(self, self.pop().to_str()));
-        }
-
-        self.push(Value::AttrPath(path));
-        Ok(())
-    }
-
     fn run_attrset(&mut self, count: usize) -> EvalResult<()> {
         let attrs = fallible!(
             self,
@@ -736,8 +717,7 @@ impl<'o> VM<'o> {
 
             // If any of these internal values are encountered here a
             // critical error has happened (likely a compiler bug).
-            Value::AttrPath(_)
-            | Value::AttrNotFound
+            Value::AttrNotFound
             | Value::DynamicUpvalueMissing(_)
             | Value::Blueprint(_)
             | Value::DeferredUpvalue(_) => {