Merge pull request #10305 from hercules-ci/addErrorContext-is-great

Always print addErrorContext

src/libexpr/eval.cc

@@ -1665,7 +1665,8 @@ void EvalState::callFunction(Value & fun, size_t nrArgs, Value * * args, Value &
                 try {
                     fn->fun(*this, vCur.determinePos(noPos), args, vCur);
                 } catch (Error & e) {
-                    addErrorTrace(e, pos, "while calling the '%1%' builtin", fn->name);
+                    if (fn->addTrace)
+                        addErrorTrace(e, pos, "while calling the '%1%' builtin", fn->name);
                     throw;
                 }
 
@@ -1713,7 +1714,8 @@ 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) {
-                    addErrorTrace(e, pos, "while calling the '%1%' builtin", fn->name);
+                    if (fn->addTrace)
+                        addErrorTrace(e, pos, "while calling the '%1%' builtin", fn->name);
                     throw;
                 }
 

src/libexpr/eval.hh

@@ -69,6 +69,13 @@ struct PrimOp
      */
     const char * doc = nullptr;
 
+    /**
+     * Add a trace item, `while calling the '<name>' builtin`
+     *
+     * This is used to remove the redundant item for `builtins.addErrorContext`.
+     */
+    bool addTrace = true;
+
     /**
      * Implementation of the primop.
      */

src/libexpr/primops.cc

@@ -826,7 +826,7 @@ static void prim_addErrorContext(EvalState & state, const PosIdx pos, Value * *
         auto message = state.coerceToString(pos, *args[0], context,
                 "while evaluating the error message passed to builtins.addErrorContext",
                 false, false).toOwned();
-        e.addTrace(nullptr, HintFmt(message));
+        e.addTrace(nullptr, HintFmt(message), TracePrint::Always);
         throw;
     }
 }
@@ -834,6 +834,8 @@ static void prim_addErrorContext(EvalState & state, const PosIdx pos, Value * *
 static RegisterPrimOp primop_addErrorContext(PrimOp {
     .name = "__addErrorContext",
     .arity = 2,
+    // The normal trace item is redundant
+    .addTrace = false,
     .fun = prim_addErrorContext,
 });
 

src/libutil/error.cc

@@ -11,9 +11,9 @@
 
 namespace nix {
 
-void BaseError::addTrace(std::shared_ptr<Pos> && e, HintFmt hint)
+void BaseError::addTrace(std::shared_ptr<Pos> && e, HintFmt hint, TracePrint print)
 {
-    err.traces.push_front(Trace { .pos = std::move(e), .hint = hint });
+    err.traces.push_front(Trace { .pos = std::move(e), .hint = hint, .print = print });
 }
 
 void throwExceptionSelfCheck(){
@@ -163,7 +163,7 @@ static bool printPosMaybe(std::ostream & oss, std::string_view indent, const std
     return hasPos;
 }
 
-void printTrace(
+static void printTrace(
     std::ostream & output,
     const std::string_view & indent,
     size_t & count,
@@ -379,29 +379,39 @@ std::ostream & showErrorInfo(std::ostream & out, const ErrorInfo & einfo, bool s
         // A consecutive sequence of stack traces that are all in `tracesSeen`.
         std::vector<Trace> skippedTraces;
         size_t count = 0;
+        bool truncate = false;
 
         for (const auto & trace : einfo.traces) {
             if (trace.hint.str().empty()) continue;
 
             if (!showTrace && count > 3) {
-                oss << "\n" << ANSI_WARNING "(stack trace truncated; use '--show-trace' to show the full trace)" ANSI_NORMAL << "\n";
-                break;
+                truncate = true;
             }
 
-            if (tracesSeen.count(trace)) {
-                skippedTraces.push_back(trace);
-                continue;
+            if (!truncate || trace.print == TracePrint::Always) {
+
+                if (tracesSeen.count(trace)) {
+                    skippedTraces.push_back(trace);
+                    continue;
+                }
+
+                tracesSeen.insert(trace);
+
+                printSkippedTracesMaybe(oss, ellipsisIndent, count, skippedTraces, tracesSeen);
+
+                count++;
+
+                printTrace(oss, ellipsisIndent, count, trace);
             }
-            tracesSeen.insert(trace);
-
-            printSkippedTracesMaybe(oss, ellipsisIndent, count, skippedTraces, tracesSeen);
-
-            count++;
-
-            printTrace(oss, ellipsisIndent, count, trace);
         }
 
+
         printSkippedTracesMaybe(oss, ellipsisIndent, count, skippedTraces, tracesSeen);
+
+        if (truncate) {
+            oss << "\n" << ANSI_WARNING "(stack trace truncated; use '--show-trace' to show the full, detailed trace)" ANSI_NORMAL << "\n";
+        }
+
         oss << "\n" << prefix;
     }
 

src/libutil/error.hh

@@ -61,9 +61,22 @@ void printCodeLines(std::ostream & out,
     const Pos & errPos,
     const LinesOfCode & loc);
 
+/**
+ * When a stack frame is printed.
+ */
+enum struct TracePrint {
+    /**
+     * The default behavior; always printed when `--show-trace` is set.
+     */
+    Default,
+    /** Always printed. Produced by `builtins.addErrorContext`. */
+    Always,
+};
+
 struct Trace {
     std::shared_ptr<Pos> pos;
     HintFmt hint;
+    TracePrint print = TracePrint::Default;
 };
 
 inline bool operator<(const Trace& lhs, const Trace& rhs);
@@ -161,7 +174,7 @@ public:
         addTrace(std::move(e), HintFmt(std::string(fs), args...));
     }
 
-    void addTrace(std::shared_ptr<Pos> && e, HintFmt hint);
+    void addTrace(std::shared_ptr<Pos> && e, HintFmt hint, TracePrint print = TracePrint::Default);
 
     bool hasTrace() const { return !err.traces.empty(); }
 

tests/functional/lang.sh

@@ -68,8 +68,16 @@ done
 for i in lang/eval-fail-*.nix; do
     echo "evaluating $i (should fail)";
     i=$(basename "$i" .nix)
+    flags="$(
+        if [[ -e "lang/$i.flags" ]]; then
+            sed -e 's/#.*//' < "lang/$i.flags"
+        else
+            # note that show-trace is also set by init.sh
+            echo "--eval --strict --show-trace"
+        fi
+    )"
     if
-        expectStderr 1 nix-instantiate --eval --strict --show-trace "lang/$i.nix" \
+        expectStderr 1 nix-instantiate $flags "lang/$i.nix" \
             | sed "s!$(pwd)!/pwd!g" > "lang/$i.err"
     then
         diffAndAccept "$i" err err.exp

tests/functional/lang/eval-fail-addErrorContext-example.err.exp

@@ -0,0 +1,24 @@
+error:
+       … while counting down; n = 10
+
+       … while counting down; n = 9
+
+       … while counting down; n = 8
+
+       … while counting down; n = 7
+
+       … while counting down; n = 6
+
+       … while counting down; n = 5
+
+       … while counting down; n = 4
+
+       … while counting down; n = 3
+
+       … while counting down; n = 2
+
+       … while counting down; n = 1
+
+       (stack trace truncated; use '--show-trace' to show the full, detailed trace)
+
+       error: kaboom

tests/functional/lang/eval-fail-addErrorContext-example.flags

@@ -0,0 +1 @@
+--eval --strict --no-show-trace

tests/functional/lang/eval-fail-addErrorContext-example.nix

@@ -0,0 +1,9 @@
+let
+  countDown = n:
+    if n == 0
+    then throw "kaboom"
+    else
+      builtins.addErrorContext
+        "while counting down; n = ${toString n}"
+        ("x" + countDown (n - 1));
+in countDown 10