From a942a34469b22491d381a517e86a17f3999a140a Mon Sep 17 00:00:00 2001
From: Robert Hensing <robert@roberthensing.nl>
Date: Thu, 23 May 2024 18:07:33 +0200
Subject: [PATCH] C API: Fix nix_c_primop_wrapper for strict initializers

https://github.com/NixOS/nix/pull/10555 added a check requiring
that output parameters always have an uninitialized Value as argument.

Unfortunately the output parameter of the primop callback received
a thunk instead.

See the comment for implementation considerations.
---
 src/libexpr-c/nix_api_value.cc     | 24 +++++++++++++++++---
 tests/unit/libexpr/nix_api_expr.cc | 35 ++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+), 3 deletions(-)

diff --git a/src/libexpr-c/nix_api_value.cc b/src/libexpr-c/nix_api_value.cc
index 0366e5020..37e0012d1 100644
--- a/src/libexpr-c/nix_api_value.cc
+++ b/src/libexpr-c/nix_api_value.cc
@@ -73,10 +73,28 @@ static void nix_c_primop_wrapper(
     PrimOpFun f, void * userdata, nix::EvalState & state, const nix::PosIdx pos, nix::Value ** args, nix::Value & v)
 {
     nix_c_context ctx;
-    f(userdata, &ctx, (EvalState *) &state, (Value **) args, (Value *) &v);
-    /* TODO: In the future, this should throw different errors depending on the error code */
-    if (ctx.last_err_code != NIX_OK)
+
+    // v currently has a thunk, but the C API initializers require an uninitialized value.
+    //
+    // We can't destroy the thunk, because that makes it impossible to retry,
+    // which is needed for tryEval and for evaluation drivers that evaluate more
+    // than one value (e.g. an attrset with two derivations, both of which
+    // reference v).
+    //
+    // Instead we create a temporary value, and then assign the result to v.
+    // This does not give the primop definition access to the thunk, but that's
+    // ok because we don't see a need for this yet (e.g. inspecting thunks,
+    // or maybe something to make blackholes work better; we don't know).
+    nix::Value vTmp;
+
+    f(userdata, &ctx, (EvalState *) &state, (Value **) args, (Value *) &vTmp);
+
+    if (ctx.last_err_code != NIX_OK) {
+        /* TODO: Throw different errors depending on the error code */
         state.error<nix::EvalError>("Error from builtin function: %s", *ctx.last_err).atPos(pos).debugThrow();
+    }
+
+    v = vTmp;
 }
 
 PrimOp * nix_alloc_primop(
diff --git a/tests/unit/libexpr/nix_api_expr.cc b/tests/unit/libexpr/nix_api_expr.cc
index 0818f1cab..babc8c6a4 100644
--- a/tests/unit/libexpr/nix_api_expr.cc
+++ b/tests/unit/libexpr/nix_api_expr.cc
@@ -191,4 +191,39 @@ TEST_F(nix_api_expr_test, nix_expr_realise_context)
     nix_realised_string_free(r);
 }
 
+const char * SAMPLE_USER_DATA = "whatever";
+
+static void primop_square(void * user_data, nix_c_context * context, EvalState * state, Value ** args, Value * ret)
+{
+    assert(context);
+    assert(state);
+    assert(user_data == SAMPLE_USER_DATA);
+    auto i = nix_get_int(context, args[0]);
+    nix_init_int(context, ret, i * i);
+}
+
+TEST_F(nix_api_expr_test, nix_expr_primop)
+{
+    PrimOp * primop =
+        nix_alloc_primop(ctx, primop_square, 1, "square", nullptr, "square an integer", (void *) SAMPLE_USER_DATA);
+    assert_ctx_ok();
+    Value * primopValue = nix_alloc_value(ctx, state);
+    assert_ctx_ok();
+    nix_init_primop(ctx, primopValue, primop);
+    assert_ctx_ok();
+
+    Value * three = nix_alloc_value(ctx, state);
+    assert_ctx_ok();
+    nix_init_int(ctx, three, 3);
+    assert_ctx_ok();
+
+    Value * result = nix_alloc_value(ctx, state);
+    assert_ctx_ok();
+    nix_value_call(ctx, state, primopValue, three, result);
+    assert_ctx_ok();
+
+    auto r = nix_get_int(ctx, result);
+    ASSERT_EQ(9, r);
+}
+
 } // namespace nixC