From 780b47193a19ec34d467776b142d115bd0029dff Mon Sep 17 00:00:00 2001 From: Aspen Smith Date: Thu, 1 Feb 2024 16:48:36 -0500 Subject: refactor(tvix/eval): Generalize propagation of catchable values Rather than explicitly checking for Value::Catchable in all builtins, make the #[builtin] proc macro insert this for all strict arguments by default, with support for a #[catch] attribute on the argument to disable this behavior. That attribute hasn't actually been *used* anywhere here, primarily because the tests pass without it, even for those builtins which weren't previously checking for Value::Catchable - if some time passes without this being used I might get rid of support for it entirely. There's also a `try_value` macro in builtins directly for the places where builtins were eg forcing something, then explicitly propagating a catchable value. Change-Id: Ie22037b9d3e305e3bdb682d105fe467bd90d53e9 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10732 Tested-by: BuildkiteCI Reviewed-by: raitobezarius --- tvix/eval/builtin-macros/src/lib.rs | 37 +++++-- tvix/eval/src/builtins/mod.rs | 190 +++--------------------------------- 2 files changed, 44 insertions(+), 183 deletions(-) diff --git a/tvix/eval/builtin-macros/src/lib.rs b/tvix/eval/builtin-macros/src/lib.rs index de73b4576a0c..5cc9807f54fe 100644 --- a/tvix/eval/builtin-macros/src/lib.rs +++ b/tvix/eval/builtin-macros/src/lib.rs @@ -22,6 +22,10 @@ struct BuiltinArgument { /// function is called. strict: bool, + /// Propagate catchable values as values to the function, rather than short-circuit returning + /// them if encountered + catch: bool, + /// Span at which the argument was defined. span: Span, } @@ -205,6 +209,7 @@ pub fn builtins(args: TokenStream, item: TokenStream) -> TokenStream { .map(|arg| { let span = arg.span(); let mut strict = true; + let mut catch = false; let (name, ty) = match arg { FnArg::Receiver(_) => { return Err(quote_spanned!(span => { @@ -219,6 +224,9 @@ pub fn builtins(args: TokenStream, item: TokenStream) -> TokenStream { if id == "lazy" { strict = false; false + } else if id == "catch" { + catch = true; + false } else { true } @@ -233,8 +241,15 @@ pub fn builtins(args: TokenStream, item: TokenStream) -> TokenStream { } }; + if catch && !strict { + return Err(quote_spanned!(span => { + compile_error!("Cannot mix both lazy and catch on the same argument") + })); + } + Ok(BuiltinArgument { strict, + catch, span, name, ty, @@ -267,12 +282,22 @@ pub fn builtins(args: TokenStream, item: TokenStream) -> TokenStream { let ident = &arg.name; if arg.strict { - f.block = Box::new(parse_quote_spanned! {arg.span=> { - let #ident: #ty = tvix_eval::generators::request_force(&co, values.pop() - .expect("Tvix bug: builtin called with incorrect number of arguments")).await; - - #block - }}); + if arg.catch { + f.block = Box::new(parse_quote_spanned! {arg.span=> { + let #ident: #ty = tvix_eval::generators::request_force(&co, values.pop() + .expect("Tvix bug: builtin called with incorrect number of arguments")).await; + #block + }}); + } else { + f.block = Box::new(parse_quote_spanned! {arg.span=> { + let #ident: #ty = tvix_eval::generators::request_force(&co, values.pop() + .expect("Tvix bug: builtin called with incorrect number of arguments")).await; + if #ident.is_catchable() { + return Ok(#ident); + } + #block + }}); + } } else { f.block = Box::new(parse_quote_spanned! {arg.span=> { let #ident: #ty = values.pop() diff --git a/tvix/eval/src/builtins/mod.rs b/tvix/eval/src/builtins/mod.rs index 0374b4226839..de21ccd5b1a7 100644 --- a/tvix/eval/src/builtins/mod.rs +++ b/tvix/eval/src/builtins/mod.rs @@ -91,6 +91,16 @@ mod pure_builtins { use super::*; + macro_rules! try_value { + ($value:expr) => {{ + let val = $value; + if val.is_catchable() { + return Ok(val); + } + val + }}; + } + #[builtin("abort")] async fn builtin_abort(co: GenCo, message: Value) -> Result { // TODO(sterni): coerces to string @@ -108,21 +118,9 @@ mod pure_builtins { #[builtin("all")] async fn builtin_all(co: GenCo, pred: Value, list: Value) -> Result { - if list.is_catchable() { - return Ok(list); - } - - if pred.is_catchable() { - return Ok(pred); - } - for value in list.to_list()?.into_iter() { let pred_result = generators::request_call_with(&co, pred.clone(), [value]).await; - let pred_result = generators::request_force(&co, pred_result).await; - - if pred_result.is_catchable() { - return Ok(pred_result); - } + let pred_result = try_value!(generators::request_force(&co, pred_result).await); if !pred_result.as_bool()? { return Ok(Value::Bool(false)); @@ -134,21 +132,9 @@ mod pure_builtins { #[builtin("any")] async fn builtin_any(co: GenCo, pred: Value, list: Value) -> Result { - if list.is_catchable() { - return Ok(list); - } - - if pred.is_catchable() { - return Ok(pred); - } - for value in list.to_list()?.into_iter() { let pred_result = generators::request_call_with(&co, pred.clone(), [value]).await; - let pred_result = generators::request_force(&co, pred_result).await; - - if pred_result.is_catchable() { - return Ok(pred_result); - } + let pred_result = try_value!(generators::request_force(&co, pred_result).await); if pred_result.as_bool()? { return Ok(Value::Bool(true)); @@ -160,9 +146,6 @@ mod pure_builtins { #[builtin("attrNames")] async fn builtin_attr_names(co: GenCo, set: Value) -> Result { - if set.is_catchable() { - return Ok(set); - } let xs = set.to_attrs()?; let mut output = Vec::with_capacity(xs.len()); @@ -175,10 +158,6 @@ mod pure_builtins { #[builtin("attrValues")] async fn builtin_attr_values(co: GenCo, set: Value) -> Result { - if set.is_catchable() { - return Ok(set); - } - let xs = set.to_attrs()?; let mut output = Vec::with_capacity(xs.len()); @@ -231,14 +210,6 @@ mod pure_builtins { #[builtin("catAttrs")] async fn builtin_cat_attrs(co: GenCo, key: Value, list: Value) -> Result { - if list.is_catchable() { - return Ok(list); - } - - if key.is_catchable() { - return Ok(key); - } - let key = key.to_str()?; let list = list.to_list()?; let mut output = vec![]; @@ -275,10 +246,6 @@ mod pure_builtins { #[builtin("concatLists")] async fn builtin_concat_lists(co: GenCo, lists: Value) -> Result { - if lists.is_catchable() { - return Ok(lists); - } - let mut out = imbl::Vector::new(); for value in lists.to_list()? { @@ -291,14 +258,6 @@ mod pure_builtins { #[builtin("concatMap")] async fn builtin_concat_map(co: GenCo, f: Value, list: Value) -> Result { - if list.is_catchable() { - return Ok(list); - } - - if f.is_catchable() { - return Ok(f); - } - let list = list.to_list()?; let mut res = imbl::Vector::new(); for val in list { @@ -315,14 +274,6 @@ mod pure_builtins { separator: Value, list: Value, ) -> Result { - if list.is_catchable() { - return Ok(list); - } - - if separator.is_catchable() { - return Ok(separator); - } - let mut separator = separator.to_contextful_str()?; let mut context = NixContext::new(); if let Some(sep_context) = separator.context_mut() { @@ -413,10 +364,6 @@ mod pure_builtins { #[builtin("elem")] async fn builtin_elem(co: GenCo, x: Value, xs: Value) -> Result { - if xs.is_catchable() { - return Ok(xs); - } - for val in xs.to_list()? { match generators::check_equality(&co, x.clone(), val, PointerEquality::AllowAll).await? { @@ -430,12 +377,6 @@ mod pure_builtins { #[builtin("elemAt")] async fn builtin_elem_at(co: GenCo, xs: Value, i: Value) -> Result { - if xs.is_catchable() { - return Ok(xs); - } - if i.is_catchable() { - return Ok(i); - } let xs = xs.to_list()?; let i = i.as_int()?; if i < 0 { @@ -450,23 +391,12 @@ mod pure_builtins { #[builtin("filter")] async fn builtin_filter(co: GenCo, pred: Value, list: Value) -> Result { - if pred.is_catchable() { - return Ok(pred); - } - - if list.is_catchable() { - return Ok(list); - } - let list: NixList = list.to_list()?; let mut out = imbl::Vector::new(); for value in list { let result = generators::request_call_with(&co, pred.clone(), [value.clone()]).await; - let verdict = generators::request_force(&co, result).await; - if verdict.is_catchable() { - return Ok(verdict); - } + let verdict = try_value!(generators::request_force(&co, result).await); if verdict.as_bool()? { out.push_back(value); } @@ -487,10 +417,6 @@ mod pure_builtins { #[lazy] nul: Value, list: Value, ) -> Result { - if list.is_catchable() { - return Ok(list); - } - let mut nul = nul; let list = list.to_list()?; for val in list { @@ -509,10 +435,6 @@ mod pure_builtins { #[builtin("functionArgs")] async fn builtin_function_args(co: GenCo, f: Value) -> Result { - if f.is_catchable() { - return Ok(f); - } - let lambda = &f.as_closure()?.lambda(); let formals = if let Some(formals) = &lambda.formals { formals @@ -526,10 +448,6 @@ mod pure_builtins { #[builtin("fromJSON")] async fn builtin_from_json(co: GenCo, json: Value) -> Result { - if json.is_catchable() { - return Ok(json); - } - let json_str = json.to_str()?; serde_json::from_slice(&json_str).map_err(|err| err.into()) @@ -564,10 +482,6 @@ mod pure_builtins { #[builtin("genericClosure")] async fn builtin_generic_closure(co: GenCo, input: Value) -> Result { - if input.is_catchable() { - return Ok(input); - } - let attrs = input.to_attrs()?; // The work set is maintained as a VecDeque because new items @@ -613,10 +527,6 @@ mod pure_builtins { generator: Value, length: Value, ) -> Result { - if length.is_catchable() { - return Ok(length); - } - let mut out = imbl::Vector::::new(); let len = length.as_int()?; // the best span we can get… @@ -636,12 +546,6 @@ mod pure_builtins { #[builtin("getAttr")] async fn builtin_get_attr(co: GenCo, key: Value, set: Value) -> Result { - if key.is_catchable() { - return Ok(key); - } - if set.is_catchable() { - return Ok(set); - } let k = key.to_str()?; let xs = set.to_attrs()?; @@ -655,14 +559,6 @@ mod pure_builtins { #[builtin("groupBy")] async fn builtin_group_by(co: GenCo, f: Value, list: Value) -> Result { - if list.is_catchable() { - return Ok(list); - } - - if f.is_catchable() { - return Ok(f); - } - let mut res: BTreeMap> = BTreeMap::new(); for val in list.to_list()? { let key = generators::request_force( @@ -682,14 +578,6 @@ mod pure_builtins { #[builtin("hasAttr")] async fn builtin_has_attr(co: GenCo, key: Value, set: Value) -> Result { - if set.is_catchable() { - return Ok(set); - } - - if key.is_catchable() { - return Ok(key); - } - let k = key.to_str()?; let xs = set.to_attrs()?; @@ -1007,10 +895,6 @@ mod pure_builtins { #[builtin("listToAttrs")] async fn builtin_list_to_attrs(co: GenCo, list: Value) -> Result { - if list.is_catchable() { - return Ok(list); - } - let list = list.to_list()?; let mut map = BTreeMap::new(); for val in list { @@ -1027,14 +911,6 @@ mod pure_builtins { #[builtin("map")] async fn builtin_map(co: GenCo, f: Value, list: Value) -> Result { - if list.is_catchable() { - return Ok(list); - } - - if f.is_catchable() { - return Ok(f); - } - let mut out = imbl::Vector::::new(); // the best span we can get… @@ -1140,14 +1016,6 @@ mod pure_builtins { #[builtin("partition")] async fn builtin_partition(co: GenCo, pred: Value, list: Value) -> Result { - if list.is_catchable() { - return Ok(list); - } - - if pred.is_catchable() { - return Ok(pred); - } - let mut right: imbl::Vector = Default::default(); let mut wrong: imbl::Vector = Default::default(); @@ -1176,14 +1044,6 @@ mod pure_builtins { attrs: Value, keys: Value, ) -> Result { - if attrs.is_catchable() { - return Ok(attrs); - } - - if keys.is_catchable() { - return Ok(keys); - } - let attrs = attrs.to_attrs()?; let keys = keys .to_list()? @@ -1207,18 +1067,6 @@ mod pure_builtins { to: Value, s: Value, ) -> Result { - if s.is_catchable() { - return Ok(s); - } - - if to.is_catchable() { - return Ok(to); - } - - if from.is_catchable() { - return Ok(from); - } - let from = from.to_list()?; for val in &from { generators::request_force(&co, val.clone()).await; @@ -1372,14 +1220,6 @@ mod pure_builtins { #[builtin("sort")] async fn builtin_sort(co: GenCo, comparator: Value, list: Value) -> Result { - if list.is_catchable() { - return Ok(list); - } - - if comparator.is_catchable() { - return Ok(comparator); - } - let list = list.to_list()?; let sorted = list.sort_by(&co, comparator).await?; Ok(Value::List(sorted)) @@ -1664,10 +1504,6 @@ mod placeholder_builtins { co: GenCo, s: Value, ) -> Result { - if s.is_catchable() { - return Ok(s); - } - let span = generators::request_span(&co).await; let mut v = s .coerce_to_string( -- cgit 1.4.1