refactor(tvix/eval): add macros for generating Value casters r/4745

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 <sternenseemann@systemli.org>
Tested-by: BuildkiteCI

3 files changed, 56 insertions, 76 deletions

diff --git a/tvix/eval/src/builtins/mod.rs b/tvix/eval/src/builtins/mod.rs
index 16ac418f6b..d0ed834a9e 100644
--- a/tvix/eval/src/builtins/mod.rs
+++ b/tvix/eval/src/builtins/mod.rs
@@ -24,12 +24,12 @@ fn pure_builtins() -> Vec<Builtin> {
         }),
         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<Builtin> {
             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> {
         }),
         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 5cfad2e66e..46ad65c502 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<bool, ErrorKind> {
-        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<NixAttrs>, "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<NixString, ErrorKind> {
-        match self {
-            Value::String(s) => Ok(s),
-            other => Err(type_error("string", &other)),
-        }
-    }
-
-    pub fn to_attrs(self) -> Result<Rc<NixAttrs>, ErrorKind> {
-        match self {
-            Value::Attrs(s) => Ok(s),
-            other => Err(type_error("set", &other)),
-        }
-    }
-
-    pub fn to_list(self) -> Result<NixList, ErrorKind> {
-        match self {
-            Value::List(l) => Ok(l),
-            other => Err(type_error("list", &other)),
-        }
-    }
-
-    pub fn to_closure(self) -> Result<Closure, ErrorKind> {
-        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 318fc726ab..8d616b8d73 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<Value> {
         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<Value> {
         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()),