From d4cb83d58be90666f6433e8264ca6482cd9478c1 Mon Sep 17 00:00:00 2001
From: sterni <sternenseemann@systemli.org>
Date: Fri, 15 Mar 2024 22:46:00 +0100
Subject: [PATCH] fix(tvix/eval): propagate errs from key comparison in
 genericClosure

The accompanying test case shows that we need to bubble up the catchable
error from the equality check if one is created.

Change-Id: Ic9929a57aa7653c8aa5a72d1711cf3264798c731
Reviewed-on: https://cl.tvl.fyi/c/depot/+/11159
Tested-by: BuildkiteCI
Autosubmit: sterni <sternenseemann@systemli.org>
Reviewed-by: tazjin <tazjin@tvl.su>
---
 tvix/eval/src/builtins/mod.rs                 | 22 +++++++++++++------
 ...ins-genericClosure-propagate-catchable.exp |  1 +
 ...ins-genericClosure-propagate-catchable.nix |  1 +
 3 files changed, 17 insertions(+), 7 deletions(-)
 create mode 100644 tvix/eval/src/tests/tvix_tests/eval-okay-builtins-genericClosure-propagate-catchable.exp
 create mode 100644 tvix/eval/src/tests/tvix_tests/eval-okay-builtins-genericClosure-propagate-catchable.nix

diff --git a/tvix/eval/src/builtins/mod.rs b/tvix/eval/src/builtins/mod.rs
index b19ba7009..bbf2eb80c 100644
--- a/tvix/eval/src/builtins/mod.rs
+++ b/tvix/eval/src/builtins/mod.rs
@@ -501,7 +501,13 @@ mod pure_builtins {
             let attrs = val.to_attrs()?;
             let key = attrs.select_required("key")?;
 
-            if !bgc_insert_key(&co, key.clone(), &mut done_keys).await? {
+            let value_missing = bgc_insert_key(&co, key.clone(), &mut done_keys).await?;
+
+            if let Err(cek) = value_missing {
+                return Ok(Value::Catchable(Box::new(cek)));
+            }
+
+            if let Ok(false) = value_missing {
                 continue;
             }
 
@@ -1485,7 +1491,11 @@ mod pure_builtins {
 
 /// Internal helper function for genericClosure, determining whether a
 /// value has been seen before.
-async fn bgc_insert_key(co: &GenCo, key: Value, done: &mut Vec<Value>) -> Result<bool, ErrorKind> {
+async fn bgc_insert_key(
+    co: &GenCo,
+    key: Value,
+    done: &mut Vec<Value>,
+) -> Result<Result<bool, CatchableErrorKind>, ErrorKind> {
     for existing in done.iter() {
         match generators::check_equality(
             co,
@@ -1496,16 +1506,14 @@ async fn bgc_insert_key(co: &GenCo, key: Value, done: &mut Vec<Value>) -> Result
         )
         .await?
         {
-            Ok(true) => return Ok(false),
+            Ok(true) => return Ok(Ok(false)),
             Ok(false) => (),
-            Err(_cek) => {
-                unimplemented!("TODO(amjoseph): not sure what the correct behavior is here")
-            }
+            Err(cek) => return Ok(Err(cek)),
         }
     }
 
     done.push(key);
-    Ok(true)
+    Ok(Ok(true))
 }
 
 /// The set of standard pure builtins in Nix, mostly concerned with
diff --git a/tvix/eval/src/tests/tvix_tests/eval-okay-builtins-genericClosure-propagate-catchable.exp b/tvix/eval/src/tests/tvix_tests/eval-okay-builtins-genericClosure-propagate-catchable.exp
new file mode 100644
index 000000000..6c89e78fc
--- /dev/null
+++ b/tvix/eval/src/tests/tvix_tests/eval-okay-builtins-genericClosure-propagate-catchable.exp
@@ -0,0 +1 @@
+{ success = false; value = false; }
diff --git a/tvix/eval/src/tests/tvix_tests/eval-okay-builtins-genericClosure-propagate-catchable.nix b/tvix/eval/src/tests/tvix_tests/eval-okay-builtins-genericClosure-propagate-catchable.nix
new file mode 100644
index 000000000..1dfc0bb04
--- /dev/null
+++ b/tvix/eval/src/tests/tvix_tests/eval-okay-builtins-genericClosure-propagate-catchable.nix
@@ -0,0 +1 @@
+builtins.tryEval (builtins.genericClosure { operator = (_: [{ key = throw "lol"; }]); startSet = [{ key = "lol"; }]; })