From a2ae14bfd880e3d3056e355c6ceca82fa61f7245 Mon Sep 17 00:00:00 2001 From: piegames Date: Wed, 13 Nov 2024 15:01:49 +0100 Subject: [PATCH] libexpr: Rework error messages on ExprSelct::eval Calls to `show` have been removed. To counter the loss of information, the error positions have been improved and now correctly point to the current selector instead of the entire select expression. Change-Id: I4771fe874af1ac15828a9863550cd4369a8f0e94 --- .../rl-next/error-message-improvements.md | 14 +++++- lix/libexpr/eval.cc | 46 +++++-------------- lix/libexpr/print.cc | 6 --- lix/libexpr/print.hh | 22 --------- .../lang/eval-fail-attr-name-type.err.exp | 2 +- .../lang/eval-fail-recursion.err.exp | 2 +- .../functional/lang/eval-fail-remove.err.exp | 4 +- .../lang/eval-fail-select-err.err.exp | 6 +-- 8 files changed, 31 insertions(+), 71 deletions(-) diff --git a/doc/manual/rl-next/error-message-improvements.md b/doc/manual/rl-next/error-message-improvements.md index 4fb6f5c4d..7e1585708 100644 --- a/doc/manual/rl-next/error-message-improvements.md +++ b/doc/manual/rl-next/error-message-improvements.md @@ -1,9 +1,21 @@ --- synopsis: Small error message improvements issues: [] -cls: [2185] +cls: [2185, 2187] category: Improvements credits: [piegames] --- +When an attribute selection fails, the error message now correctly points to the attribute in the chain that failed instead of at the beginning of the entire chain. +```diff + error: attribute 'x' missing +- at /pwd/lang/eval-fail-remove.nix:4:3: ++ at /pwd/lang/eval-fail-remove.nix:4:29: + 3| in + 4| (removeAttrs attrs ["x"]).x +- | ^ ++ | ^ + 5| +``` + Failed asserts don't print the failed assertion expression anymore in the error message. That code was buggy and the information was redundant anyways, given that the error position already more accurately shows what exactly failed. diff --git a/lix/libexpr/eval.cc b/lix/libexpr/eval.cc index d0a1a569c..6f52566a3 100644 --- a/lix/libexpr/eval.cc +++ b/lix/libexpr/eval.cc @@ -1235,9 +1235,7 @@ static std::string showAttrPath(EvalState & state, Env & env, const AttrPath & a out << state.ctx.symbols[getName(i, state, env)]; } catch (Error & e) { assert(!i.symbol); - out << "\"${"; - i.expr->show(state.ctx.symbols, out); - out << "}\""; + out << "\"${...}\""; } } return out.str(); @@ -1252,6 +1250,8 @@ void ExprSelect::eval(EvalState & state, Env & env, Value & v) Value * vCurrent = &vFirst; // Position for the current attrset Value in this select chain. PosIdx posCurrent; + // Position for the current selector in this select chain. + PosIdx posCurrentSyntax; try { e->eval(state, env, vFirst); @@ -1259,8 +1259,7 @@ void ExprSelect::eval(EvalState & state, Env & env, Value & v) assert(this->e != nullptr); e.addTrace( state.ctx.positions[getPos()], - "while evaluating '%s' to select '%s' on it", - ExprPrinter(state, *this->e), + "while evaluating an expression to select '%s' on it", showAttrPath(state.ctx.symbols, this->attrPath) ); throw; @@ -1282,36 +1281,12 @@ void ExprSelect::eval(EvalState & state, Env & env, Value & v) Symbol const name = getName(currentAttrName, state, env); - // For formatting errors, which should be done only when needed. - auto partsSoFar = [&]() -> std::string { - std::stringstream ss; - // We start with the base thing this ExprSelect is selecting on. - assert(this->e != nullptr); - this->e->show(state.ctx.symbols, ss); - - // Then grab each part of the attr path up to this one. - assert(partIdx < attrPath.size()); - std::span const parts( - attrPath.begin(), - attrPath.begin() + partIdx - ); - - // And convert them to strings and join them. - for (auto const & part : parts) { - auto const partName = getName(part, state, env); - ss << "." << state.ctx.symbols[partName]; - } - - return ss.str(); - }; - try { state.forceValue(*vCurrent, pos); } catch (Error & e) { e.addTrace( - state.ctx.positions[getPos()], - "while evaluating '%s' to select '%s' on it", - partsSoFar(), + state.ctx.positions[currentAttrName.pos], + "while evaluating an expression to select '%s' on it", state.ctx.symbols[name] ); throw; @@ -1332,8 +1307,8 @@ void ExprSelect::eval(EvalState & state, Env & env, Value & v) showType(*vCurrent), ValuePrinter(state, *vCurrent, errorPrintOptions) ).addTrace( - pos, - HintFmt("while selecting '%s' on '%s'", state.ctx.symbols[name], partsSoFar()) + currentAttrName.pos, + HintFmt("while selecting '%s'", state.ctx.symbols[name]) ).debugThrow(); } @@ -1355,7 +1330,7 @@ void ExprSelect::eval(EvalState & state, Env & env, Value & v) } auto suggestions = Suggestions::bestMatches(allAttrNames, state.ctx.symbols[name]); state.ctx.errors.make("attribute '%s' missing", state.ctx.symbols[name]) - .atPos(pos) + .atPos(currentAttrName.pos) .withSuggestions(suggestions) .withFrame(env, *this) .debugThrow(); @@ -1365,10 +1340,11 @@ void ExprSelect::eval(EvalState & state, Env & env, Value & v) // Set our currently operated-on attrset to this one, and keep going. vCurrent = attrIt->value; posCurrent = attrIt->pos; + posCurrentSyntax = currentAttrName.pos; if (state.ctx.stats.countCalls) state.ctx.stats.attrSelects[posCurrent]++; } - state.forceValue(*vCurrent, (posCurrent ? posCurrent : this->pos)); + state.forceValue(*vCurrent, (posCurrent ? posCurrent : posCurrentSyntax)); } catch (Error & e) { auto pos2r = state.ctx.positions[posCurrent]; diff --git a/lix/libexpr/print.cc b/lix/libexpr/print.cc index 6083f4a40..05bf74863 100644 --- a/lix/libexpr/print.cc +++ b/lix/libexpr/print.cc @@ -581,10 +581,4 @@ fmt_internal::HintFmt & fmt_internal::HintFmt::operator%(const ValuePrinter & va return *this; } -std::ostream & operator<<(std::ostream & output, ExprPrinter const & printer) -{ - printer.expr.show(printer.state.ctx.symbols, output); - return output; -} - } diff --git a/lix/libexpr/print.hh b/lix/libexpr/print.hh index b2e9d8ecd..5f389dcd8 100644 --- a/lix/libexpr/print.hh +++ b/lix/libexpr/print.hh @@ -81,26 +81,4 @@ std::ostream & operator<<(std::ostream & output, const ValuePrinter & printer); template<> fmt_internal::HintFmt & fmt_internal::HintFmt::operator%(const ValuePrinter & value); -/** - * A partially-applied form of Expr::show(), which can be formatted using `<<` - * without allocating an intermediate string. - * This class should not outlive the eval state or it will UAF. - * FIXME: This should take `nix::ref`s, to avoid that, but our eval methods all have - * EvalState &, not ref, and constructing a new shared_ptr to data that - * already has a shared_ptr is a much bigger footgun. In the current architecture of - * libexpr, using an ExprPrinter after an EvalState has been destroyed would be - * pretty hard. - */ -class ExprPrinter -{ - /** The eval state used to get symbols. */ - EvalState const & state; - /** The expression to print. */ - Expr const & expr; - -public: - ExprPrinter(EvalState const & state, Expr const & expr) : state(state), expr(expr) { } - friend std::ostream & operator << (std::ostream & output, ExprPrinter const & printer); -}; - } diff --git a/tests/functional/lang/eval-fail-attr-name-type.err.exp b/tests/functional/lang/eval-fail-attr-name-type.err.exp index 6848a35ed..b68d77794 100644 --- a/tests/functional/lang/eval-fail-attr-name-type.err.exp +++ b/tests/functional/lang/eval-fail-attr-name-type.err.exp @@ -1,5 +1,5 @@ error: - … while evaluating the attribute 'puppy."${key}"' + … while evaluating the attribute 'puppy."${...}"' at /pwd/lang/eval-fail-attr-name-type.nix:3:5: 2| attrs = { 3| puppy.doggy = {}; diff --git a/tests/functional/lang/eval-fail-recursion.err.exp b/tests/functional/lang/eval-fail-recursion.err.exp index f0057b2d5..3231a2df9 100644 --- a/tests/functional/lang/eval-fail-recursion.err.exp +++ b/tests/functional/lang/eval-fail-recursion.err.exp @@ -1,5 +1,5 @@ error: - … while evaluating 'a' to select 'foo' on it + … while evaluating an expression to select 'foo' on it at /pwd/lang/eval-fail-recursion.nix:1:21: 1| let a = {} // a; in a.foo | ^ diff --git a/tests/functional/lang/eval-fail-remove.err.exp b/tests/functional/lang/eval-fail-remove.err.exp index 0ae9c1256..ab99c2b92 100644 --- a/tests/functional/lang/eval-fail-remove.err.exp +++ b/tests/functional/lang/eval-fail-remove.err.exp @@ -1,7 +1,7 @@ error: attribute 'x' missing - at /pwd/lang/eval-fail-remove.nix:4:3: + at /pwd/lang/eval-fail-remove.nix:4:29: 3| in 4| (removeAttrs attrs ["x"]).x - | ^ + | ^ 5| Did you mean y? diff --git a/tests/functional/lang/eval-fail-select-err.err.exp b/tests/functional/lang/eval-fail-select-err.err.exp index 70ee50737..50bcbe51d 100644 --- a/tests/functional/lang/eval-fail-select-err.err.exp +++ b/tests/functional/lang/eval-fail-select-err.err.exp @@ -6,11 +6,11 @@ error: | ^ 3| in somepkg.src.meta - … while evaluating 'somepkg.src' to select 'meta' on it - at /pwd/lang/eval-fail-select-err.nix:3:4: + … while evaluating an expression to select 'meta' on it + at /pwd/lang/eval-fail-select-err.nix:3:16: 2| somepkg.src = throw "invalid foo bar"; 3| in somepkg.src.meta - | ^ + | ^ 4| … caused by explicit throw