From 6b3c3c982669e805c9fc06ee74182606497b7bc3 Mon Sep 17 00:00:00 2001 From: Vincent Ambo Date: Fri, 2 Sep 2022 05:40:50 +0300 Subject: refactor(tvix/eval): add macros for generating Value casters The casting methods of `Value` are pretty verbose, and actually incorrect before this commit as they did not account for inner thunk values. To address this, we first attempt to make them correct by introducing a standard macro to generate them and traverse the inner thunk(s) if necessary. This is likely to be a performance hit as it will now involve more cloning of values. We can do multiple things to alleviate this, but should do some measurements first. Change-Id: If315d6e2afe7b69db727df535bc6cbfb89a691aa Reviewed-on: https://cl.tvl.fyi/c/depot/+/6412 Reviewed-by: sterni Tested-by: BuildkiteCI --- tvix/eval/src/builtins/mod.rs | 8 ++-- tvix/eval/src/value/mod.rs | 104 +++++++++++++++++------------------------- tvix/eval/src/vm.rs | 20 ++++---- 3 files changed, 56 insertions(+), 76 deletions(-) diff --git a/tvix/eval/src/builtins/mod.rs b/tvix/eval/src/builtins/mod.rs index 16ac418f6bd8..d0ed834a9e2c 100644 --- a/tvix/eval/src/builtins/mod.rs +++ b/tvix/eval/src/builtins/mod.rs @@ -24,12 +24,12 @@ fn pure_builtins() -> Vec { }), Builtin::new("abort", 1, |mut args, _| { return Err(ErrorKind::Abort( - args.pop().unwrap().to_string()?.as_str().to_owned(), + args.pop().unwrap().to_str()?.as_str().to_owned(), )); }), Builtin::new("catAttrs", 2, |mut args, _| { let list = args.pop().unwrap().to_list()?; - let key = args.pop().unwrap().to_string()?; + let key = args.pop().unwrap().to_str()?; let mut output = vec![]; for set in list.into_iter() { @@ -46,7 +46,7 @@ fn pure_builtins() -> Vec { arithmetic_op!(a, b, /) }), Builtin::new("length", 1, |args, _| { - Ok(Value::Integer(args[0].as_list()?.len() as i64)) + Ok(Value::Integer(args[0].to_list()?.len() as i64)) }), Builtin::new("isAttrs", 1, |args, _| { Ok(Value::Bool(matches!(args[0], Value::Attrs(_)))) @@ -90,7 +90,7 @@ fn pure_builtins() -> Vec { }), Builtin::new("throw", 1, |mut args, _| { return Err(ErrorKind::Throw( - args.pop().unwrap().to_string()?.as_str().to_owned(), + args.pop().unwrap().to_str()?.as_str().to_owned(), )); }), Builtin::new("toString", 1, |args, _| { diff --git a/tvix/eval/src/value/mod.rs b/tvix/eval/src/value/mod.rs index 5cfad2e66ea8..46ad65c5025e 100644 --- a/tvix/eval/src/value/mod.rs +++ b/tvix/eval/src/value/mod.rs @@ -43,11 +43,42 @@ pub enum Value { DeferredUpvalue(StackIdx), } -impl Value { - pub fn is_number(&self) -> bool { - matches!(self, Value::Integer(_) | Value::Float(_)) - } +// Helper macros to generate the to_*/as_* macros while accounting for +// thunks. + +/// Generate an `as_*` method returning a reference to the expected +/// type, or a type error. This only works for types that implement +/// `Copy`, as returning a reference to an inner thunk value is not +/// possible. + +/// Generate an `as_*/to_*` accessor method that returns either the +/// expected type, or a type error. +macro_rules! gen_cast { + ( $name:ident, $type:ty, $expected:expr, $variant:pat, $result:expr ) => { + pub fn $name(&self) -> Result<$type, ErrorKind> { + match self { + $variant => Ok($result), + Value::Thunk(thunk) => Self::$name(&thunk.value()), + other => Err(type_error($expected, &other)), + } + } + }; +} +/// Generate an `is_*` type-checking method. +macro_rules! gen_is { + ( $name:ident, $variant:pat ) => { + pub fn $name(&self) -> bool { + match self { + $variant => true, + Value::Thunk(thunk) => Self::$name(&thunk.value()), + _ => false, + } + } + }; +} + +impl Value { pub fn type_of(&self) -> &'static str { match self { Value::Null => "null", @@ -70,65 +101,14 @@ impl Value { } } - pub fn as_bool(&self) -> Result { - match self { - Value::Bool(b) => Ok(*b), - other => Err(type_error("bool", &other)), - } - } - - pub fn as_attrs(&self) -> Result<&NixAttrs, ErrorKind> { - match self { - Value::Attrs(attrs) => Ok(attrs), - other => Err(type_error("set", &other)), - } - } - - pub fn as_str(&self) -> Result<&str, ErrorKind> { - match self { - Value::String(s) => Ok(s.as_str()), - other => Err(type_error("string", &other)), - } - } - - pub fn as_list(&self) -> Result<&NixList, ErrorKind> { - match self { - Value::List(xs) => Ok(xs), - other => Err(type_error("list", &other)), - } - } + gen_cast!(as_bool, bool, "bool", Value::Bool(b), *b); + gen_cast!(to_str, NixString, "string", Value::String(s), s.clone()); + gen_cast!(to_attrs, Rc, "set", Value::Attrs(a), a.clone()); + gen_cast!(to_list, NixList, "list", Value::List(l), l.clone()); + gen_cast!(to_closure, Closure, "lambda", Value::Closure(c), c.clone()); - pub fn to_string(self) -> Result { - match self { - Value::String(s) => Ok(s), - other => Err(type_error("string", &other)), - } - } - - pub fn to_attrs(self) -> Result, ErrorKind> { - match self { - Value::Attrs(s) => Ok(s), - other => Err(type_error("set", &other)), - } - } - - pub fn to_list(self) -> Result { - match self { - Value::List(l) => Ok(l), - other => Err(type_error("list", &other)), - } - } - - pub fn to_closure(self) -> Result { - match self { - Value::Closure(c) => Ok(c), - other => Err(type_error("lambda", &other)), - } - } - - pub fn is_bool(&self) -> bool { - matches!(self, Value::Bool(_)) - } + gen_is!(is_number, Value::Integer(_) | Value::Float(_)); + gen_is!(is_bool, Value::Bool(_)); } impl Display for Value { diff --git a/tvix/eval/src/vm.rs b/tvix/eval/src/vm.rs index 318fc726abd7..8d616b8d73b8 100644 --- a/tvix/eval/src/vm.rs +++ b/tvix/eval/src/vm.rs @@ -259,7 +259,7 @@ impl VM { } OpCode::OpAttrsSelect => { - let key = fallible!(self, self.pop().to_string()); + let key = fallible!(self, self.pop().to_str()); let attrs = fallible!(self, self.pop().to_attrs()); match attrs.select(key.as_str()) { @@ -274,7 +274,7 @@ impl VM { } OpCode::OpAttrsTrySelect => { - let key = fallible!(self, self.pop().to_string()); + let key = fallible!(self, self.pop().to_str()); let value = match self.pop() { Value::Attrs(attrs) => match attrs.select(key.as_str()) { Some(value) => value.clone(), @@ -288,7 +288,7 @@ impl VM { } OpCode::OpAttrsIsSet => { - let key = fallible!(self, self.pop().to_string()); + let key = fallible!(self, self.pop().to_str()); let result = match self.pop() { Value::Attrs(attrs) => attrs.contains(key.as_str()), @@ -379,13 +379,13 @@ impl VM { } OpCode::OpResolveWith => { - let ident = fallible!(self, self.pop().to_string()); + let ident = fallible!(self, self.pop().to_str()); let value = self.resolve_with(ident.as_str())?; self.push(value) } OpCode::OpResolveWithOrUpvalue(idx) => { - let ident = fallible!(self, self.pop().to_string()); + let ident = fallible!(self, self.pop().to_str()); match self.resolve_with(ident.as_str()) { // Variable found in local `with`-stack. Ok(value) => self.push(value), @@ -529,7 +529,7 @@ impl VM { let mut path = Vec::with_capacity(count); for _ in 0..count { - path.push(fallible!(self, self.pop().to_string())); + path.push(fallible!(self, self.pop().to_str())); } self.push(Value::AttrPath(path)); @@ -553,7 +553,7 @@ impl VM { let mut out = String::new(); for _ in 0..count { - out.push_str(fallible!(self, self.pop().to_string()).as_str()); + out.push_str(fallible!(self, self.pop().to_str()).as_str()); } self.push(Value::String(out.into())); @@ -562,7 +562,7 @@ impl VM { fn resolve_dynamic_upvalue(&mut self, ident_idx: ConstantIdx) -> EvalResult { let chunk = self.chunk(); - let ident = fallible!(self, chunk.constant(ident_idx).as_str()).to_string(); + let ident = fallible!(self, chunk.constant(ident_idx).to_str()); // Peek at the current instruction (note: IP has already // advanced!) to see if it is actually data indicating a @@ -577,7 +577,7 @@ impl VM { _ => None, }; - match self.resolve_with(&ident) { + match self.resolve_with(ident.as_str()) { Ok(v) => Ok(v), Err(Error { @@ -595,7 +595,7 @@ impl VM { /// Resolve a dynamic identifier through the with-stack at runtime. fn resolve_with(&self, ident: &str) -> EvalResult { for idx in self.with_stack.iter().rev() { - let with = fallible!(self, self.stack[*idx].as_attrs()); + let with = fallible!(self, self.stack[*idx].to_attrs()); match with.select(ident) { None => continue, Some(val) => return Ok(val.clone()), -- cgit 1.4.1