use singleton expr to generate black hole errors

this also reduces forceValue code size and removes the need for
hideInDiagnostics. coopting thunk forcing like this has the additional
benefit of clarifying how these errors can happen in the first place.

src/libexpr/eval-inline.hh

@@ -93,20 +93,12 @@ void EvalState::forceValue(Value & v, const PosIdx pos)
             expr->eval(*this, *env, v);
         } catch (...) {
             v.mkThunk(env, expr);
+            tryFixupBlackHolePos(v, pos);
             throw;
         }
     }
-    else if (v.isApp()) {
+    else if (v.isApp())
-        try {
+        callFunction(*v.app.left, *v.app.right, v, pos);
-            callFunction(*v.app.left, *v.app.right, v, noPos);
-        } catch (InfiniteRecursionError & e) {
-            // only one black hole can *throw* in any given eval stack so we need not
-            // check whether the position is set already.
-            if (v.isBlackhole())
-                e.err.errPos = positions[pos];
-            throw;
-        }
-    }
 }
 
 

src/libexpr/eval.cc

@@ -256,8 +256,8 @@ std::string showType(const Value & v)
         case tPrimOpApp:
             return fmt("the partially applied built-in function '%s'", std::string(getPrimOp(v)->primOp->name));
         case tExternal: return v.external->showType();
-        case tThunk: return "a thunk";
+        case tThunk: return v.isBlackhole() ? "a black hole" : "a thunk";
-        case tApp: return v.isBlackhole() ? "a black hole" : "a function application";
+        case tApp: return "a function application";
     default:
         return std::string(showType(v.type()));
     }
@@ -1624,14 +1624,12 @@ void EvalState::callFunction(Value & fun, size_t nrArgs, Value * * args, Value &
                 auto * fn = vCur.primOp;
 
                 nrPrimOpCalls++;
-                // This will count black holes, but that's ok, because unrecoverable errors are rare.
                 if (countCalls) primOpCalls[fn->name]++;
 
                 try {
                     fn->fun(*this, vCur.determinePos(noPos), args, vCur);
                 } catch (Error & e) {
-                    if (!fn->hideInDiagnostics)
+                    addErrorTrace(e, pos, "while calling the '%1%' builtin", fn->name);
-                        addErrorTrace(e, pos, "while calling the '%1%' builtin", fn->name);
                     throw;
                 }
 
@@ -1670,7 +1668,6 @@ void EvalState::callFunction(Value & fun, size_t nrArgs, Value * * args, Value &
 
                 auto fn = primOp->primOp;
                 nrPrimOpCalls++;
-                // This will count black holes, but that's ok, because unrecoverable errors are rare.
                 if (countCalls) primOpCalls[fn->name]++;
 
                 try {
@@ -1680,8 +1677,7 @@ void EvalState::callFunction(Value & fun, size_t nrArgs, Value * * args, Value &
                     //    so the debugger allows to inspect the wrong parameters passed to the builtin.
                     fn->fun(*this, vCur.determinePos(noPos), vArgs, vCur);
                 } catch (Error & e) {
-                    if (!fn->hideInDiagnostics)
+                    addErrorTrace(e, pos, "while calling the '%1%' builtin", fn->name);
-                        addErrorTrace(e, pos, "while calling the '%1%' builtin", fn->name);
                     throw;
                 }
 
@@ -2035,6 +2031,29 @@ void ExprPos::eval(EvalState & state, Env & env, Value & v)
 }
 
 
+void ExprBlackHole::eval(EvalState & state, Env & env, Value & v)
+{
+    state.error("infinite recursion encountered")
+        .debugThrow<InfiniteRecursionError>();
+}
+
+// always force this to be separate, otherwise forceValue may inline it and take
+// a massive perf hit
+[[gnu::noinline]]
+void EvalState::tryFixupBlackHolePos(Value & v, PosIdx pos)
+{
+    if (!v.isBlackhole())
+        return;
+    auto e = std::current_exception();
+    try {
+        std::rethrow_exception(e);
+    } catch (InfiniteRecursionError & e) {
+        e.err.errPos = positions[pos];
+    } catch (...) {
+    }
+}
+
+
 void EvalState::forceValueDeep(Value & v)
 {
     std::set<const Value *> seen;

src/libexpr/eval.hh

@@ -77,14 +77,6 @@ struct PrimOp
      */
     std::optional<ExperimentalFeature> experimentalFeature;
 
-    /**
-     * Whether to hide this primop in diagnostics.
-     *
-     * Used to hide the fact that black holes are primop applications from
-     * stack traces.
-     */
-    bool hideInDiagnostics;
-
     /**
      * Validity check to be performed by functions that introduce primops,
      * such as RegisterPrimOp() and Value::mkPrimOp().
@@ -473,6 +465,8 @@ public:
      */
     inline void forceValue(Value & v, const PosIdx pos);
 
+    void tryFixupBlackHolePos(Value & v, PosIdx pos);
+
     /**
      * Force a value, then recursively force list elements and
      * attributes.

src/libexpr/nixexpr.cc

@@ -9,6 +9,8 @@
 
 namespace nix {
 
+ExprBlackHole eBlackHole;
+
 struct PosAdapter : AbstractPos
 {
     Pos::Origin origin;

src/libexpr/nixexpr.hh

@@ -462,6 +462,16 @@ struct ExprPos : Expr
     COMMON_METHODS
 };
 
+/* only used to mark thunks as black holes. */
+struct ExprBlackHole : Expr
+{
+    void show(const SymbolTable & symbols, std::ostream & str) const override {}
+    void eval(EvalState & state, Env & env, Value & v) override;
+    void bindVars(EvalState & es, const std::shared_ptr<const StaticEnv> & env) override {}
+};
+
+extern ExprBlackHole eBlackHole;
+
 
 /* Static environments are used to map variable names onto (level,
    displacement) pairs used to obtain the value of the variable at

src/libexpr/primops.cc

@@ -4263,29 +4263,6 @@ static RegisterPrimOp primop_splitVersion({
 });
 
 
-static void prim_blackHoleFn(EvalState & state, const PosIdx pos, Value * * args, Value & v)
-{
-    state.error("infinite recursion encountered")
-        .debugThrow<InfiniteRecursionError>();
-}
-
-static PrimOp primop_blackHole = {
-    .name = "«blackHole»",
-    .args = {},
-    .fun = prim_blackHoleFn,
-    .hideInDiagnostics = true,
-};
-
-static Value makeBlackHole()
-{
-    Value v;
-    v.mkPrimOp(&primop_blackHole);
-    return v;
-}
-
-Value prim_blackHole = makeBlackHole();
-
-
 /*************************************************************
  * Primop registration
  *************************************************************/

src/libexpr/primops.hh

@@ -51,10 +51,4 @@ void prim_importNative(EvalState & state, const PosIdx pos, Value * * args, Valu
  */
 void prim_exec(EvalState & state, const PosIdx pos, Value * * args, Value & v);
 
-/**
- *  Placeholder value for black holes, used to represent black holes as
- *  applications of this value to the evaluated thunks.
- */
-extern Value prim_blackHole;
-
 }

src/libexpr/value.hh

@@ -61,6 +61,7 @@ class Bindings;
 struct Env;
 struct Expr;
 struct ExprLambda;
+struct ExprBlackHole;
 struct PrimOp;
 class Symbol;
 class PosIdx;
@@ -442,16 +443,17 @@ public:
 };
 
 
-extern Value prim_blackHole;
+extern ExprBlackHole eBlackHole;
 
-inline bool Value::isBlackhole() const
+bool Value::isBlackhole() const
 {
-    return internalType == tApp && app.left == &prim_blackHole;
+    return internalType == tThunk && thunk.expr == (Expr*) &eBlackHole;
 }
 
-inline void Value::mkBlackhole()
+void Value::mkBlackhole()
 {
-    mkApp(&prim_blackHole, &prim_blackHole);
+    internalType = tThunk;
+    thunk.expr = (Expr*) &eBlackHole;
 }