From d54aeb1f2055bdac05cb62ab30b6c63dc86a78e0 Mon Sep 17 00:00:00 2001 From: Vincent Ambo Date: Sat, 24 Sep 2022 16:00:32 +0300 Subject: chore(tvix/eval): remove existing nested key implementation 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 Tested-by: BuildkiteCI --- tvix/eval/src/opcode.rs | 1 - tvix/eval/src/value/attrs.rs | 75 +------------------------------------------- tvix/eval/src/value/mod.rs | 6 +--- tvix/eval/src/vm.rs | 22 +------------ 4 files changed, 3 insertions(+), 101 deletions(-) (limited to 'tvix') 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, - 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), AttrNotFound, DynamicUpvalueMissing(NixString), Blueprint(Rc), @@ -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(_) => { -- cgit 1.4.1