From 8f2004d360dde108f90d2b49b0609bd43b7b6d7d Mon Sep 17 00:00:00 2001 From: Vincent Ambo Date: Wed, 21 Sep 2022 00:32:49 +0300 Subject: [PATCH] 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 --- tvix/eval/src/builtins/mod.rs | 6 ++-- .../tvix_tests/eval-okay-builtins-map.exp | 2 +- .../tvix_tests/eval-okay-builtins-map.nix | 3 ++ tvix/eval/src/value/mod.rs | 20 ++--------- tvix/eval/src/vm.rs | 35 ++++++++++++------- 5 files changed, 32 insertions(+), 34 deletions(-) diff --git a/tvix/eval/src/builtins/mod.rs b/tvix/eval/src/builtins/mod.rs index 598c8aa08..1ea23d49e 100644 --- a/tvix/eval/src/builtins/mod.rs +++ b/tvix/eval/src/builtins/mod.rs @@ -12,8 +12,7 @@ use std::{ use crate::{ errors::ErrorKind, - upvalues::UpvalueCarrier, - value::{Builtin, Closure, CoercionKind, NixAttrs, NixList, NixString, Value}, + value::{Builtin, CoercionKind, NixAttrs, NixList, NixString, Value}, vm::VM, }; @@ -147,14 +146,13 @@ fn pure_builtins() -> Vec { }), 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::, _>>() .map(|list| Value::List(NixList::from(list))) diff --git a/tvix/eval/src/tests/tvix_tests/eval-okay-builtins-map.exp b/tvix/eval/src/tests/tvix_tests/eval-okay-builtins-map.exp index e1ff70800..6cf530403 100644 --- a/tvix/eval/src/tests/tvix_tests/eval-okay-builtins-map.exp +++ b/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 ] ] diff --git a/tvix/eval/src/tests/tvix_tests/eval-okay-builtins-map.nix b/tvix/eval/src/tests/tvix_tests/eval-okay-builtins-map.nix index 6ff42d089..71b351fd5 100644 --- a/tvix/eval/src/tests/tvix_tests/eval-okay-builtins-map.nix +++ b/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 ]) ] diff --git a/tvix/eval/src/value/mod.rs b/tvix/eval/src/value/mod.rs index fb02dfc36..1ee98c25b 100644 --- a/tvix/eval/src/value/mod.rs +++ b/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 - let result = match value { - Value::Closure(c) => { - 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), - }?; + // Leave self on the stack as an argument to the function call. + vm.push(self.clone()); + let result = vm.call_value(value)?; match result { Value::String(s) => Ok(s), diff --git a/tvix/eval/src/vm.rs b/tvix/eval/src/vm.rs index 909e219bc..69ffc7d5c 100644 --- a/tvix/eval/src/vm.rs +++ b/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 { + 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 { - 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)), - }; + self.call_value(&callable)?; } OpCode::OpTailCall => {