From 7c76964daa0d1ca07fd609f5eb28b51afd1246b7 Mon Sep 17 00:00:00 2001 From: Maximilian Bosch Date: Fri, 8 Jan 2021 22:27:00 +0100 Subject: [PATCH] libexpr: misc improvements for proper error position When working on some more complex Nix code, there are sometimes rather unhelpful or misleading error messages, especially if coerce-errors are thrown. This patch is a first steps towards improving that. I'm happy to file more changes after that, but I'd like to gather some feedback first. To summarize, this patch does the following things: * Attrsets (a.k.a. `Bindings` in `libexpr`) now have a `Pos`. This is helpful e.g. to identify which attribute-set in `listToAttrs` is invalid. * The `Value`-struct has a new method named `determinePos` which tries to guess the position of a value and falls back to a default if that's not possible. This can be used to provide better messages if a coercion fails. * The new `determinePos`-API is used by `builtins.concatMap` now. With that change, Nix shows the exact position in the error where a wrong value was returned by the lambda. To make sure it's still obvious that `concatMap` is the problem, another stack-frame was added. * The changes described above can be added to every other `primop`, but first I'd like to get some feedback about the overall approach. --- src/libexpr/attr-set.hh | 1 + src/libexpr/eval.cc | 11 +++++++++++ src/libexpr/nixexpr.hh | 4 +++- src/libexpr/parser.y | 2 +- src/libexpr/primops.cc | 31 +++++++++++++++++++++---------- src/libexpr/value.hh | 2 ++ 6 files changed, 39 insertions(+), 12 deletions(-) diff --git a/src/libexpr/attr-set.hh b/src/libexpr/attr-set.hh index 6d68e5df3..1da8d91df 100644 --- a/src/libexpr/attr-set.hh +++ b/src/libexpr/attr-set.hh @@ -35,6 +35,7 @@ class Bindings { public: typedef uint32_t size_t; + Pos *pos; private: size_t size_, capacity_; diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 3afe2e47b..7fd44c2cf 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -201,6 +201,15 @@ string showType(const Value & v) } } +Pos Value::determinePos(const Pos &pos) const +{ + switch (internalType) { + case tAttrs: return *attrs->pos; + case tLambda: return lambda.fun->pos; + case tApp: return app.left->determinePos(pos); + default: return pos; + } +} bool Value::isTrivial() const { @@ -1060,6 +1069,8 @@ void ExprAttrs::eval(EvalState & state, Env & env, Value & v) v.attrs->push_back(Attr(nameSym, i.valueExpr->maybeThunk(state, *dynamicEnv), &i.pos)); v.attrs->sort(); // FIXME: inefficient } + + v.attrs->pos = &pos; } diff --git a/src/libexpr/nixexpr.hh b/src/libexpr/nixexpr.hh index 8df8055b3..51a14cd59 100644 --- a/src/libexpr/nixexpr.hh +++ b/src/libexpr/nixexpr.hh @@ -180,6 +180,7 @@ struct ExprOpHasAttr : Expr struct ExprAttrs : Expr { bool recursive; + Pos pos; struct AttrDef { bool inherited; Expr * e; @@ -199,7 +200,8 @@ struct ExprAttrs : Expr }; typedef std::vector DynamicAttrDefs; DynamicAttrDefs dynamicAttrs; - ExprAttrs() : recursive(false) { }; + ExprAttrs(const Pos &pos) : recursive(false), pos(pos) { }; + ExprAttrs() : recursive(false), pos(noPos) { }; COMMON_METHODS }; diff --git a/src/libexpr/parser.y b/src/libexpr/parser.y index 49d995bb9..f948dde47 100644 --- a/src/libexpr/parser.y +++ b/src/libexpr/parser.y @@ -478,7 +478,7 @@ binds $$->attrs[i.symbol] = ExprAttrs::AttrDef(new ExprSelect(CUR_POS, $4, i.symbol), makeCurPos(@6, data)); } } - | { $$ = new ExprAttrs; } + | { $$ = new ExprAttrs(makeCurPos(@0, data)); } ; attrs diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index f9eefddd0..c7bbfc050 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -2149,21 +2149,27 @@ static void prim_listToAttrs(EvalState & state, const Pos & pos, Value * * args, state.forceAttrs(v2, pos); Bindings::iterator j = v2.attrs->find(state.sName); - if (j == v2.attrs->end()) - throw TypeError({ - .msg = hintfmt("'name' attribute missing in a call to 'listToAttrs'"), - .errPos = pos + if (j == v2.attrs->end()) { + auto e = TypeError({ + .msg = hintfmt("'name' attribute missing for 'listToAttrs'"), + .errPos = *v2.attrs->pos }); - string name = state.forceStringNoCtx(*j->value, pos); + e.addTrace(pos, hintfmt("while invoking '%s'", "listToAttrs")); + throw e; + } + string name = state.forceStringNoCtx(*j->value, *j->pos); Symbol sym = state.symbols.create(name); if (seen.insert(sym).second) { Bindings::iterator j2 = v2.attrs->find(state.symbols.create(state.sValue)); - if (j2 == v2.attrs->end()) - throw TypeError({ - .msg = hintfmt("'value' attribute missing in a call to 'listToAttrs'"), - .errPos = pos + if (j2 == v2.attrs->end()) { + auto e = TypeError({ + .msg = hintfmt("'value' attribute missing for 'listToAttrs'"), + .errPos = *v2.attrs->pos }); + e.addTrace(pos, hintfmt("while invoking '%s'", "listToAttrs")); + throw e; + } v.attrs->push_back(Attr(sym, j2->value, j2->pos)); } } @@ -2804,7 +2810,12 @@ static void prim_concatMap(EvalState & state, const Pos & pos, Value * * args, V for (unsigned int n = 0; n < nrLists; ++n) { Value * vElem = args[1]->listElems()[n]; state.callFunction(*args[0], *vElem, lists[n], pos); - state.forceList(lists[n], pos); + try { + state.forceList(lists[n], lists[n].determinePos(args[0]->determinePos(pos))); + } catch (TypeError &e) { + e.addTrace(pos, hintfmt("while invoking '%s'", "concatMap")); + throw e; + } len += lists[n].listSize(); } diff --git a/src/libexpr/value.hh b/src/libexpr/value.hh index b317c1898..a1f131f9e 100644 --- a/src/libexpr/value.hh +++ b/src/libexpr/value.hh @@ -341,6 +341,8 @@ public: return internalType == tList1 ? 1 : internalType == tList2 ? 2 : bigList.size; } + Pos determinePos(const Pos &pos) const; + /* Check whether forcing this value requires a trivial amount of computation. In particular, function applications are non-trivial. */