refactor(tvix/eval): add VM::call_value helper method

This makes it possible to call a callable value (builtin or
closure/lambda) directly, without unwrapping it first. This is needed
for pretty much all higher-order functions to work correctly.

This is mostly equivalent to the previous code in coerce_to_string for
calling `__toString`, except it expects the argument(s) to already be
placed on the stack.

Note that the span for the `NotCallable` error is not currently
guaranteed to make any sense, will experiment with this.

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

tvix/eval/src/builtins/mod.rs

@@ -12,8 +12,7 @@ use std::{
 
 use crate::{
     errors::ErrorKind,
-    upvalues::UpvalueCarrier,
+    value::{Builtin, CoercionKind, NixAttrs, NixList, NixString, Value},
-    value::{Builtin, Closure, CoercionKind, NixAttrs, NixList, NixString, Value},
     vm::VM,
 };
 
@@ -147,14 +146,13 @@ fn pure_builtins() -> Vec<Builtin> {
         }),
         Builtin::new("map", &[true, true], |args, vm| {
             let list: NixList = args[1].to_list()?;
-            let func: Closure = args[0].to_closure()?;
 
             list.into_iter()
                 .map(|val| {
                     // Leave the argument on the stack before calling the
                     // function.
                     vm.push(val);
-                    vm.call(func.lambda(), func.upvalues().clone(), 1)
+                    vm.call_value(&args[0])
                 })
                 .collect::<Result<Vec<Value>, _>>()
                 .map(|list| Value::List(NixList::from(list)))

tvix/eval/src/tests/tvix_tests/eval-okay-builtins-map.exp

@@ -1 +1 @@
-[ [ 1 2 3 4 5 ] [ 2 4 6 8 10 ] [ 2 4 6 8 10 ] [ 1 2 3 4 5 ] ]
+[ [ 1 2 3 4 5 ] [ 2 4 6 8 10 ] [ 2 4 6 8 10 ] [ 2 4 6 8 10 ] [ 1 2 3 4 5 ] ]

tvix/eval/src/tests/tvix_tests/eval-okay-builtins-map.nix

@@ -11,6 +11,9 @@
     in builtins.map (x: x * n) [ 1 2 3 4 5 ]
   )
 
+  # same, but with a builtin
+  (builtins.map (builtins.mul 2) [ 1 2 3 4 5 ])
+
   # from global scope
   (map (x: x) [ 1 2 3 4 5 ])
 ]

tvix/eval/src/value/mod.rs

@@ -16,7 +16,6 @@ mod thunk;
 
 use crate::errors::ErrorKind;
 use crate::opcode::StackIdx;
-use crate::upvalues::UpvalueCarrier;
 use crate::vm::VM;
 pub use attrs::NixAttrs;
 pub use builtin::Builtin;
@@ -155,22 +154,9 @@ impl Value {
                     (Some(f), _) => {
                         // use a closure here to deal with the thunk borrow we need to do below
                         let call_to_string = |value: &Value, vm: &mut VM| {
-                            // TODO(sterni): calling logic should be extracted into a helper
+                            // Leave self on the stack as an argument to the function call.
-                            let result = match value {
+                            vm.push(self.clone());
-                                Value::Closure(c) => {
+                            let result = vm.call_value(value)?;
-                                    vm.push(self.clone());
-                                    vm.call(c.lambda(), c.upvalues().clone(), 1)
-                                        .map_err(|e| e.kind)
-                                }
-
-                                Value::Builtin(b) => {
-                                    vm.push(self.clone());
-                                    vm.call_builtin(b.clone()).map_err(|e| e.kind)?;
-                                    Ok(vm.pop())
-                                }
-
-                                _ => Err(ErrorKind::NotCallable),
-                            }?;
 
                             match result {
                                 Value::String(s) => Ok(s),

tvix/eval/src/vm.rs

@@ -175,7 +175,28 @@ impl<'o> VM<'o> {
         }
     }
 
-    #[allow(clippy::let_and_return)] // due to disassembler
+    /// Execute the given value in this VM's context, if it is a
+    /// callable.
+    ///
+    /// The stack of the VM must be prepared with all required
+    /// arguments before calling this and the value must have already
+    /// been forced.
+    pub fn call_value(&mut self, callable: &Value) -> EvalResult<Value> {
+        match callable {
+            Value::Closure(c) => self.call(c.lambda(), c.upvalues().clone(), 1),
+
+            Value::Builtin(b) => {
+                self.call_builtin(b.clone())?;
+                Ok(self.pop())
+            }
+
+            Value::Thunk(t) => self.call_value(&t.value()),
+
+            // TODO: this isn't guaranteed to be a useful span, actually
+            _ => Err(self.error(ErrorKind::NotCallable)),
+        }
+    }
+
     /// Execute the given lambda in this VM's context, returning its
     /// value after its stack frame completes.
     pub fn call(
@@ -456,17 +477,7 @@ impl<'o> VM<'o> {
 
                 OpCode::OpCall => {
                     let callable = self.pop();
-                    match callable {
+                    self.call_value(&callable)?;
-                        Value::Closure(closure) => {
-                            let result =
-                                self.call(closure.lambda(), closure.upvalues().clone(), 1)?;
-                            self.push(result)
-                        }
-
-                        Value::Builtin(builtin) => self.call_builtin(builtin)?,
-
-                        _ => return Err(self.error(ErrorKind::NotCallable)),
-                    };
                 }
 
                 OpCode::OpTailCall => {