lix-project/lix
20
58
Fork 31
Code Issues 355 Code Review (Gerrit) Projects 3 Releases Wiki Activity

libexpr: Rework error messages on ExprSelct::eval

Browse source 
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
This commit is contained in:
piegames 2024-11-13 15:01:49 +01:00
parent 172515bd8b
commit a2ae14bfd8
8 changed files with 31 additions and 71 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

14
doc/manual/rl-next/error-message-improvements.md
View file

@ -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.

46
lix/libexpr/eval.cc
View file

@ -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<AttrName> 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<EvalError>("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];

6
lix/libexpr/print.cc
View file

@ -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;
}
}

22
lix/libexpr/print.hh
View file

@ -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<EvalState>, 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);
};
}

2
tests/functional/lang/eval-fail-attr-name-type.err.exp
View file

@ -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 = {};

2
tests/functional/lang/eval-fail-recursion.err.exp
View file

@ -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
| ^

4
tests/functional/lang/eval-fail-remove.err.exp
View file

@ -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?

6
tests/functional/lang/eval-fail-select-err.err.exp
View file

@ -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