From 040874e4db904ecbca3964b6d22d35c423969729 Mon Sep 17 00:00:00 2001 From: Rebecca Turner Date: Thu, 22 Feb 2024 17:14:33 -0800 Subject: [PATCH 1/4] Print all stack frames --- src/libutil/error.cc | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/libutil/error.cc b/src/libutil/error.cc index 4a9efc0b5..d2a3d2114 100644 --- a/src/libutil/error.cc +++ b/src/libutil/error.cc @@ -373,7 +373,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 +383,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 +398,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); } From f05c13ecc2345cb8c668289369b066b0520b919b Mon Sep 17 00:00:00 2001 From: Rebecca Turner Date: Thu, 22 Feb 2024 17:14:55 -0800 Subject: [PATCH 2/4] Remove the concept of "skipped frames" --- src/libexpr/eval-error.cc | 14 +++----------- src/libexpr/eval-error.hh | 2 +- src/libexpr/eval.cc | 9 ++++----- src/libexpr/eval.hh | 2 +- src/libexpr/primops.cc | 7 +++---- src/libutil/error.cc | 7 +++---- src/libutil/error.hh | 3 +-- 7 files changed, 16 insertions(+), 28 deletions(-) diff --git a/src/libexpr/eval-error.cc b/src/libexpr/eval-error.cc index f4cdeec5c..8db03610b 100644 --- a/src/libexpr/eval-error.cc +++ b/src/libexpr/eval-error.cc @@ -28,15 +28,7 @@ template EvalErrorBuilder & EvalErrorBuilder::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 -EvalErrorBuilder & EvalErrorBuilder::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 & EvalErrorBuilder::withFrame(const Env & env, const Expr } template -EvalErrorBuilder & EvalErrorBuilder::addTrace(PosIdx pos, HintFmt hint, bool frame) +EvalErrorBuilder & EvalErrorBuilder::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 index 392902ad2..7e0cbe982 100644 --- a/src/libexpr/eval-error.hh +++ b/src/libexpr/eval-error.hh @@ -89,7 +89,7 @@ public: [[nodiscard, gnu::noinline]] EvalErrorBuilder & withFrame(const Env & e, const Expr & ex); - [[nodiscard, gnu::noinline]] EvalErrorBuilder & addTrace(PosIdx pos, HintFmt hint, bool frame = false); + [[nodiscard, gnu::noinline]] EvalErrorBuilder & addTrace(PosIdx pos, HintFmt hint); template [[nodiscard, gnu::noinline]] EvalErrorBuilder & diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 205d40b83..54b1125ce 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -811,9 +811,9 @@ void EvalState::addErrorTrace(Error & e, const char * s, const std::string & s2) e.addTrace(nullptr, s, s2); } -void EvalState::addErrorTrace(Error & e, const PosIdx pos, const char * s, const std::string & s2, bool frame) const +void EvalState::addErrorTrace(Error & e, const PosIdx pos, const char * s, const std::string & s2) const { - e.addTrace(positions[pos], HintFmt(s, s2), frame); + e.addTrace(positions[pos], HintFmt(s, s2)); } template @@ -1587,9 +1587,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%s", ""); } throw; } diff --git a/src/libexpr/eval.hh b/src/libexpr/eval.hh index 42fe0d3e4..80b583eb1 100644 --- a/src/libexpr/eval.hh +++ b/src/libexpr/eval.hh @@ -435,7 +435,7 @@ public: [[gnu::noinline]] void addErrorTrace(Error & e, const char * s, const std::string & s2) const; [[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 char * s, const std::string & s2) const; public: /** diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 42cfa4917..835afba82 100644 --- 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 index d2a3d2114..d1e864a1a 100644 --- a/src/libutil/error.cc +++ b/src/libutil/error.cc @@ -11,9 +11,9 @@ namespace nix { -void BaseError::addTrace(std::shared_ptr && e, HintFmt hint, bool frame) +void BaseError::addTrace(std::shared_ptr && 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); } diff --git a/src/libutil/error.hh b/src/libutil/error.hh index 2e5de5d32..89f5ad021 100644 --- 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; 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 && e, HintFmt hint, bool frame = false); + void addTrace(std::shared_ptr && e, HintFmt hint); bool hasTrace() const { return !err.traces.empty(); } From 91e89628fdfe7b08e0f61b8531edd31833330e04 Mon Sep 17 00:00:00 2001 From: Rebecca Turner Date: Thu, 22 Feb 2024 17:18:27 -0800 Subject: [PATCH 3/4] Make `addErrorTrace` variadic --- src/libexpr/eval.cc | 12 +++++++----- src/libexpr/eval.hh | 6 ++++-- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 54b1125ce..c4e163b08 100644 --- 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 +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) const +template +void EvalState::addErrorTrace(Error & e, const PosIdx pos, const Args & ... formatArgs) const { - e.addTrace(positions[pos], HintFmt(s, s2)); + e.addTrace(positions[pos], HintFmt(formatArgs...)); } template @@ -1588,7 +1590,7 @@ void EvalState::callFunction(Value & fun, size_t nrArgs, Value * * args, Value & lambda.name ? concatStrings("'", symbols[lambda.name], "'") : "anonymous lambda"); - if (pos) addErrorTrace(e, pos, "from call site%s", ""); + if (pos) addErrorTrace(e, pos, "from call site"); } throw; } diff --git a/src/libexpr/eval.hh b/src/libexpr/eval.hh index 80b583eb1..01abd4eb1 100644 --- 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 [[gnu::noinline]] - void addErrorTrace(Error & e, const char * s, const std::string & s2) const; + void addErrorTrace(Error & e, const Args & ... formatArgs) const; + template [[gnu::noinline]] - void addErrorTrace(Error & e, const PosIdx pos, const char * s, const std::string & s2) const; + void addErrorTrace(Error & e, const PosIdx pos, const Args & ... formatArgs) const; public: /** From fe6408b5df4a2a4c2342a02bc9f94abf4ca88a85 Mon Sep 17 00:00:00 2001 From: Rebecca Turner Date: Thu, 22 Feb 2024 17:58:37 -0800 Subject: [PATCH 4/4] Update snapshots --- tests/functional/lang/eval-fail-duplicate-traces.err.exp | 7 +++++++ .../eval-fail-foldlStrict-strict-op-application.err.exp | 7 +++++++ tests/functional/lang/eval-fail-mutual-recursion.err.exp | 7 +++++++ 3 files changed, 21 insertions(+) diff --git a/tests/functional/lang/eval-fail-duplicate-traces.err.exp b/tests/functional/lang/eval-fail-duplicate-traces.err.exp index 32ad9b376..cedaebd3b 100644 --- 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 index 7cb08af8a..4903bc82d 100644 --- 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 index dc2e11766..c034afcd5 100644 --- 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!