Merge pull request #10066 from 9999years/print-all-frames

Do not skip any stack frames when `--show-trace` is given
NixOS · Feb 23, 2024 · 0b47783 · 0b47783
2 parents accae60 + fe6408b
commit 0b47783
Show file tree

Hide file tree

Showing 10 changed files with 44 additions and 34 deletions.
diff --git a/src/libexpr/eval-error.cc b/src/libexpr/eval-error.cc
@@ -28,15 +28,7 @@ template<class T>
 EvalErrorBuilder<T> & EvalErrorBuilder<T>::withTrace(PosIdx pos, const std::string_view text)
 {
     error.err.traces.push_front(
-        Trace{.pos = error.state.positions[pos], .hint = HintFmt(std::string(text)), .frame = false});
-    return *this;
-}
-
-template<class T>
-EvalErrorBuilder<T> & EvalErrorBuilder<T>::withFrameTrace(PosIdx pos, const std::string_view text)
-{
-    error.err.traces.push_front(
-        Trace{.pos = error.state.positions[pos], .hint = HintFmt(std::string(text)), .frame = true});
+        Trace{.pos = error.state.positions[pos], .hint = HintFmt(std::string(text))});
     return *this;
 }
 
@@ -63,9 +55,9 @@ EvalErrorBuilder<T> & EvalErrorBuilder<T>::withFrame(const Env & env, const Expr
 }
 
 template<class T>
-EvalErrorBuilder<T> & EvalErrorBuilder<T>::addTrace(PosIdx pos, HintFmt hint, bool frame)
+EvalErrorBuilder<T> & EvalErrorBuilder<T>::addTrace(PosIdx pos, HintFmt hint)
 {
-    error.addTrace(error.state.positions[pos], hint, frame);
+    error.addTrace(error.state.positions[pos], hint);
     return *this;
 }
 

diff --git a/src/libexpr/eval-error.hh b/src/libexpr/eval-error.hh
@@ -89,7 +89,7 @@ public:
 
     [[nodiscard, gnu::noinline]] EvalErrorBuilder<T> & withFrame(const Env & e, const Expr & ex);
 
-    [[nodiscard, gnu::noinline]] EvalErrorBuilder<T> & addTrace(PosIdx pos, HintFmt hint, bool frame = false);
+    [[nodiscard, gnu::noinline]] EvalErrorBuilder<T> & addTrace(PosIdx pos, HintFmt hint);
 
     template<typename... Args>
     [[nodiscard, gnu::noinline]] EvalErrorBuilder<T> &

diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc
@@ -806,14 +806,16 @@ void EvalState::runDebugRepl(const Error * error, const Env & env, const Expr &
     }
 }
 
-void EvalState::addErrorTrace(Error & e, const char * s, const std::string & s2) const
+template<typename... Args>
+void EvalState::addErrorTrace(Error & e, const Args & ... formatArgs) const
 {
-    e.addTrace(nullptr, s, s2);
+    e.addTrace(nullptr, HintFmt(formatArgs...));
 }
 
-void EvalState::addErrorTrace(Error & e, const PosIdx pos, const char * s, const std::string & s2, bool frame) const
+template<typename... Args>
+void EvalState::addErrorTrace(Error & e, const PosIdx pos, const Args & ... formatArgs) const
 {
-    e.addTrace(positions[pos], HintFmt(s, s2), frame);
+    e.addTrace(positions[pos], HintFmt(formatArgs...));
 }
 
 template<typename... Args>
@@ -1587,9 +1589,8 @@ void EvalState::callFunction(Value & fun, size_t nrArgs, Value * * args, Value &
                         "while calling %s",
                         lambda.name
                         ? concatStrings("'", symbols[lambda.name], "'")
-                        : "anonymous lambda",
-                        true);
-                    if (pos) addErrorTrace(e, pos, "from call site%s", "", true);
+                        : "anonymous lambda");
+                    if (pos) addErrorTrace(e, pos, "from call site");
                 }
                 throw;
             }

diff --git a/src/libexpr/eval.hh b/src/libexpr/eval.hh
@@ -432,10 +432,12 @@ public:
     std::string_view forceString(Value & v, NixStringContext & context, const PosIdx pos, std::string_view errorCtx);
     std::string_view forceStringNoCtx(Value & v, const PosIdx pos, std::string_view errorCtx);
 
+    template<typename... Args>
     [[gnu::noinline]]
-    void addErrorTrace(Error & e, const char * s, const std::string & s2) const;
+    void addErrorTrace(Error & e, const Args & ... formatArgs) const;
+    template<typename... Args>
     [[gnu::noinline]]
-    void addErrorTrace(Error & e, const PosIdx pos, const char * s, const std::string & s2, bool frame = false) const;
+    void addErrorTrace(Error & e, const PosIdx pos, const Args & ... formatArgs) const;
 
 public:
     /**

diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc
@@ -811,7 +811,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), true);
+        e.addTrace(nullptr, HintFmt(message));
         throw;
     }
 }
@@ -1075,7 +1075,7 @@ static void prim_derivationStrict(EvalState & state, const PosIdx pos, Value * *
         e.addTrace(nullptr, HintFmt(
                 "while evaluating derivation '%s'\n"
                 "  whose name attribute is located at %s",
-                drvName, pos), true);
+                drvName, pos));
         throw;
     }
 }
@@ -1233,8 +1233,7 @@ drvName, Bindings * attrs, Value & v)
 
         } catch (Error & e) {
             e.addTrace(state.positions[i->pos],
-                HintFmt("while evaluating attribute '%1%' of derivation '%2%'", key, drvName),
-                true);
+                HintFmt("while evaluating attribute '%1%' of derivation '%2%'", key, drvName));
             throw;
         }
     }

diff --git a/src/libutil/error.cc b/src/libutil/error.cc
@@ -11,9 +11,9 @@
 
 namespace nix {
 
-void BaseError::addTrace(std::shared_ptr<Pos> && e, HintFmt hint, bool frame)
+void BaseError::addTrace(std::shared_ptr<Pos> && e, HintFmt hint)
 {
-    err.traces.push_front(Trace { .pos = std::move(e), .hint = hint, .frame = frame });
+    err.traces.push_front(Trace { .pos = std::move(e), .hint = hint });
 }
 
 void throwExceptionSelfCheck(){
@@ -61,8 +61,7 @@ inline bool operator<(const Trace& lhs, const Trace& rhs)
     // This formats a freshly formatted hint string and then throws it away, which
     // shouldn't be much of a problem because it only runs when pos is equal, and this function is
     // used for trace printing, which is infrequent.
-    return std::forward_as_tuple(lhs.hint.str(), lhs.frame)
-        < std::forward_as_tuple(rhs.hint.str(), rhs.frame);
+    return lhs.hint.str() < rhs.hint.str();
 }
 inline bool operator> (const Trace& lhs, const Trace& rhs) { return rhs < lhs; }
 inline bool operator<=(const Trace& lhs, const Trace& rhs) { return !(lhs > rhs); }
@@ -373,7 +372,6 @@ std::ostream & showErrorInfo(std::ostream & out, const ErrorInfo & einfo, bool s
     // prepended to each element of the trace
     auto ellipsisIndent = "  ";
 
-    bool frameOnly = false;
     if (!einfo.traces.empty()) {
         // Stack traces seen since we last printed a chunk of `duplicate frames
         // omitted`.
@@ -384,7 +382,6 @@ std::ostream & showErrorInfo(std::ostream & out, const ErrorInfo & einfo, bool s
 
         for (const auto & trace : einfo.traces) {
             if (trace.hint.str().empty()) continue;
-            if (frameOnly && !trace.frame) continue;
 
             if (!showTrace && count > 3) {
                 oss << "\n" << ANSI_WARNING "(stack trace truncated; use '--show-trace' to show the full trace)" ANSI_NORMAL << "\n";
@@ -400,7 +397,6 @@ std::ostream & showErrorInfo(std::ostream & out, const ErrorInfo & einfo, bool s
             printSkippedTracesMaybe(oss, ellipsisIndent, count, skippedTraces, tracesSeen);
 
             count++;
-            frameOnly = trace.frame;
 
             printTrace(oss, ellipsisIndent, count, trace);
         }

diff --git a/src/libutil/error.hh b/src/libutil/error.hh
@@ -64,7 +64,6 @@ void printCodeLines(std::ostream & out,
 struct Trace {
     std::shared_ptr<Pos> pos;
     HintFmt hint;
-    bool frame;
 };
 
 inline bool operator<(const Trace& lhs, const Trace& rhs);
@@ -162,7 +161,7 @@ public:
         addTrace(std::move(e), HintFmt(std::string(fs), args...));
     }
 
-    void addTrace(std::shared_ptr<Pos> && e, HintFmt hint, bool frame = false);
+    void addTrace(std::shared_ptr<Pos> && e, HintFmt hint);
 
     bool hasTrace() const { return !err.traces.empty(); }
 

diff --git a/tests/functional/lang/eval-fail-duplicate-traces.err.exp b/tests/functional/lang/eval-fail-duplicate-traces.err.exp
@@ -41,4 +41,11 @@ error:
              |                ^
             5|     if n > 0
 
+       … while calling the 'throw' builtin
+         at /pwd/lang/eval-fail-duplicate-traces.nix:7:10:
+            6|     then throwAfter (n - 1)
+            7|     else throw "Uh oh!";
+             |          ^
+            8| in
+
        error: Uh oh!
diff --git a/tests/functional/lang/eval-fail-foldlStrict-strict-op-application.err.exp b/tests/functional/lang/eval-fail-foldlStrict-strict-op-application.err.exp
@@ -27,4 +27,11 @@ error:
              |      ^
             6|
 
+       … while calling the 'throw' builtin
+         at /pwd/lang/eval-fail-foldlStrict-strict-op-application.nix:5:9:
+            4|   null
+            5|   [ (_: throw "Not the final value, but is still forced!") (_: 23) ]
+             |         ^
+            6|
+
        error: Not the final value, but is still forced!
diff --git a/tests/functional/lang/eval-fail-mutual-recursion.err.exp b/tests/functional/lang/eval-fail-mutual-recursion.err.exp
@@ -54,4 +54,11 @@ error:
 
        (21 duplicate frames omitted)
 
+       … while calling the 'throw' builtin
+         at /pwd/lang/eval-fail-mutual-recursion.nix:34:10:
+           33|     then throwAfterB true 10
+           34|     else throw "Uh oh!";
+             |          ^
+           35| in
+
        error: Uh oh!