encode black holes as tApp values

checking for isBlackhole in the forceValue hot path is rather more
expensive than necessary, and with a little bit of trickery we can move
such handling into the isApp case. small performance benefit, but under
some circumstances we've seen 2% improvement as well.

〉 nix eval --raw --impure --expr 'with import <nixpkgs/nixos> {}; system'

before:

  Time (mean ± σ):      4.429 s ±  0.002 s    [User: 3.929 s, System: 0.500 s]
  Range (min … max):    4.427 s …  4.433 s    10 runs

after:

  Time (mean ± σ):      4.396 s ±  0.002 s    [User: 3.894 s, System: 0.501 s]
  Range (min … max):    4.393 s …  4.399 s    10 runs

src/libexpr/eval-inline.hh

@@ -104,11 +104,16 @@ void EvalState::forceValue(Value & v, Callable getPos)
         }
     }
     else if (v.isApp()) {
-        PosIdx pos = getPos();
+        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[getPos()];
+            throw;
+        }
     }
-    else if (v.isBlackhole())
-        error("infinite recursion encountered").atPos(getPos()).template debugThrow<EvalError>();
 }
 
 

src/libexpr/eval.cc

@@ -162,7 +162,17 @@ void Value::print(const SymbolTable &symbols, std::ostream &str,
         break;
     case tThunk:
     case tApp:
-        str << "<CODE>";
+        if (!isBlackhole()) {
+            str << "<CODE>";
+        } else {
+            // Although we know for sure that it's going to be an infinite recursion
+            // when this value is accessed _in the current context_, it's likely
+            // that the user will misinterpret a simpler «infinite recursion» output
+            // as a definitive statement about the value, while in fact it may be
+            // a valid value after `builtins.trace` and perhaps some other steps
+            // have completed.
+            str << "«potential infinite recursion»";
+        }
         break;
     case tLambda:
         str << "<LAMBDA>";
@@ -179,15 +189,6 @@ void Value::print(const SymbolTable &symbols, std::ostream &str,
     case tFloat:
         str << fpoint;
         break;
-    case tBlackhole:
-        // Although we know for sure that it's going to be an infinite recursion
-        // when this value is accessed _in the current context_, it's likely
-        // that the user will misinterpret a simpler «infinite recursion» output
-        // as a definitive statement about the value, while in fact it may be
-        // a valid value after `builtins.trace` and perhaps some other steps
-        // have completed.
-        str << "«potential infinite recursion»";
-        break;
     default:
         printError("Nix evaluator internal error: Value::print(): invalid value type %1%", internalType);
         abort();
@@ -256,8 +257,7 @@ std::string showType(const Value & v)
             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 tApp: return "a function application";
+        case tApp: return v.isBlackhole() ? "a black hole" : "a function application";
-        case tBlackhole: return "a black hole";
     default:
         return std::string(showType(v.type()));
     }
@@ -1621,15 +1621,17 @@ void EvalState::callFunction(Value & fun, size_t nrArgs, Value * * args, Value &
                 return;
             } else {
                 /* We have all the arguments, so call the primop. */
-                auto name = vCur.primOp->name;
+                auto * fn = vCur.primOp;
 
                 nrPrimOpCalls++;
-                if (countCalls) primOpCalls[name]++;
+                // This will count black holes, but that's ok, because unrecoverable errors are rare.
+                if (countCalls) primOpCalls[fn->name]++;
 
                 try {
-                    vCur.primOp->fun(*this, vCur.determinePos(noPos), args, vCur);
+                    fn->fun(*this, vCur.determinePos(noPos), args, vCur);
                 } catch (Error & e) {
-                    addErrorTrace(e, pos, "while calling the '%1%' builtin", name);
+                    if (!fn->hideInDiagnostics)
+                        addErrorTrace(e, pos, "while calling the '%1%' builtin", fn->name);
                     throw;
                 }
 
@@ -1666,18 +1668,20 @@ void EvalState::callFunction(Value & fun, size_t nrArgs, Value * * args, Value &
                 for (size_t i = 0; i < argsLeft; ++i)
                     vArgs[argsDone + i] = args[i];
 
-                auto name = primOp->primOp->name;
+                auto fn = primOp->primOp;
                 nrPrimOpCalls++;
-                if (countCalls) primOpCalls[name]++;
+                // This will count black holes, but that's ok, because unrecoverable errors are rare.
+                if (countCalls) primOpCalls[fn->name]++;
 
                 try {
                     // TODO:
                     // 1. Unify this and above code. Heavily redundant.
                     // 2. Create a fake env (arg1, arg2, etc.) and a fake expr (arg1: arg2: etc: builtins.name arg1 arg2 etc)
                     //    so the debugger allows to inspect the wrong parameters passed to the builtin.
-                    primOp->primOp->fun(*this, vCur.determinePos(noPos), vArgs, vCur);
+                    fn->fun(*this, vCur.determinePos(noPos), vArgs, vCur);
                 } catch (Error & e) {
-                    addErrorTrace(e, pos, "while calling the '%1%' builtin", name);
+                    if (!fn->hideInDiagnostics)
+                        addErrorTrace(e, pos, "while calling the '%1%' builtin", fn->name);
                     throw;
                 }
 

src/libexpr/eval.hh

@@ -77,6 +77,14 @@ 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().

src/libexpr/nixexpr.hh

@@ -21,6 +21,13 @@ MakeError(TypeError, EvalError);
 MakeError(UndefinedVarError, Error);
 MakeError(MissingArgumentError, EvalError);
 
+class InfiniteRecursionError : public EvalError
+{
+    friend class EvalState;
+public:
+    using EvalError::EvalError;
+};
+
 /**
  * Position objects.
  */

src/libexpr/primops.cc

@@ -4263,6 +4263,29 @@ 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,4 +51,10 @@ 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

@@ -32,7 +32,6 @@ typedef enum {
     tThunk,
     tApp,
     tLambda,
-    tBlackhole,
     tPrimOp,
     tPrimOpApp,
     tExternal,
@@ -151,7 +150,7 @@ public:
     // type() == nThunk
     inline bool isThunk() const { return internalType == tThunk; };
     inline bool isApp() const { return internalType == tApp; };
-    inline bool isBlackhole() const { return internalType == tBlackhole; };
+    inline bool isBlackhole() const;
 
     // type() == nFunction
     inline bool isLambda() const { return internalType == tLambda; };
@@ -248,7 +247,7 @@ public:
             case tLambda: case tPrimOp: case tPrimOpApp: return nFunction;
             case tExternal: return nExternal;
             case tFloat: return nFloat;
-            case tThunk: case tApp: case tBlackhole: return nThunk;
+            case tThunk: case tApp: return nThunk;
         }
         if (invalidIsThunk)
             return nThunk;
@@ -356,11 +355,7 @@ public:
         lambda.fun = f;
     }
 
-    inline void mkBlackhole()
+    inline void mkBlackhole();
-    {
-        internalType = tBlackhole;
-        // Value will be overridden anyways
-    }
 
     void mkPrimOp(PrimOp * p);
 
@@ -447,6 +442,19 @@ public:
 };
 
 
+extern Value prim_blackHole;
+
+inline bool Value::isBlackhole() const
+{
+    return internalType == tApp && app.left == &prim_blackHole;
+}
+
+inline void Value::mkBlackhole()
+{
+    mkApp(&prim_blackHole, &prim_blackHole);
+}
+
+
 #if HAVE_BOEHMGC
 typedef std::vector<Value *, traceable_allocator<Value *>> ValueVector;
 typedef std::map<Symbol, Value *, std::less<Symbol>, traceable_allocator<std::pair<const Symbol, Value *>>> ValueMap;