From 3550a32b25068aa8b1670ef7fd2af2b821f35dc1 Mon Sep 17 00:00:00 2001 From: Maximilian Bosch Date: Fri, 8 Jan 2021 22:09:35 +0100 Subject: [PATCH 1/5] primops/derivation: use position of currently evaluated attribute * The position of the `name`-attribute appears in the trace. * If e.g. `meta` has no `outPath`-attribute, a `cannot coerce set to string` error will be thrown where `pos` points to `name =` which is highly misleading. --- src/libexpr/primops.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 428adf4c2..f9eefddd0 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -953,7 +953,7 @@ static void prim_derivationStrict(EvalState & state, const Pos & pos, Value * * } } else { - auto s = state.coerceToString(posDrvName, *i->value, context, true); + auto s = state.coerceToString(*i->pos, *i->value, context, true); drv.env.emplace(key, s); if (i->name == state.sBuilder) drv.builder = s; else if (i->name == state.sSystem) drv.platform = s; From 7c76964daa0d1ca07fd609f5eb28b51afd1246b7 Mon Sep 17 00:00:00 2001 From: Maximilian Bosch Date: Fri, 8 Jan 2021 22:27:00 +0100 Subject: [PATCH 2/5] 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. */ From f60473a07752fa409cec2c3ed818af99327a571c Mon Sep 17 00:00:00 2001 From: Maximilian Bosch Date: Sun, 11 Apr 2021 12:14:05 +0200 Subject: [PATCH 3/5] libexpr/primops: Move attr name extraction into its own function This now takes care of providing positioning for both the faulting value and the faulting function call in case of an error. --- src/libexpr/primops.cc | 86 ++++++++++++++++++++++++++---------------- 1 file changed, 54 insertions(+), 32 deletions(-) diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index c7bbfc050..7f3c298ba 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -547,18 +547,42 @@ typedef list ValueList; #endif +static Bindings::iterator extractAttrNameFromPrimopCall( + EvalState &state, + string funcName, + string attrName, + Bindings *attrSet, + const Pos &pos +) { + Bindings::iterator value = attrSet->find(state.symbols.create(attrName)); + if (value == attrSet->end()) { + auto e = TypeError({ + .msg = hintfmt("attribute '%s' missing for call to '%s'", attrName, funcName), + .errPos = *attrSet->pos, + }); + + // Adding another trace for the function name to make it clear + // which call received wrong arguments. + e.addTrace(pos, hintfmt("while invoking '%s'", funcName)); + throw e; + } + + return value; +} + static void prim_genericClosure(EvalState & state, const Pos & pos, Value * * args, Value & v) { state.forceAttrs(*args[0], pos); /* Get the start set. */ - Bindings::iterator startSet = - args[0]->attrs->find(state.symbols.create("startSet")); - if (startSet == args[0]->attrs->end()) - throw EvalError({ - .msg = hintfmt("attribute 'startSet' required"), - .errPos = pos - }); + Bindings::iterator startSet = extractAttrNameFromPrimopCall( + state, + "genericClosure", + "startSet", + args[0]->attrs, + pos + ); + state.forceList(*startSet->value, pos); ValueList workSet; @@ -566,13 +590,14 @@ static void prim_genericClosure(EvalState & state, const Pos & pos, Value * * ar workSet.push_back(startSet->value->listElems()[n]); /* Get the operator. */ - Bindings::iterator op = - args[0]->attrs->find(state.symbols.create("operator")); - if (op == args[0]->attrs->end()) - throw EvalError({ - .msg = hintfmt("attribute 'operator' required"), - .errPos = pos - }); + Bindings::iterator op = extractAttrNameFromPrimopCall( + state, + "genericClosure", + "operator", + args[0]->attrs, + pos + ); + state.forceValue(*op->value, pos); /* Construct the closure by applying the operator to element of @@ -2148,28 +2173,25 @@ static void prim_listToAttrs(EvalState & state, const Pos & pos, Value * * args, Value & v2(*args[0]->listElems()[i]); state.forceAttrs(v2, pos); - Bindings::iterator j = v2.attrs->find(state.sName); - if (j == v2.attrs->end()) { - auto e = TypeError({ - .msg = hintfmt("'name' attribute missing for 'listToAttrs'"), - .errPos = *v2.attrs->pos - }); - e.addTrace(pos, hintfmt("while invoking '%s'", "listToAttrs")); - throw e; - } + Bindings::iterator j = extractAttrNameFromPrimopCall( + state, + "listToAttrs", + state.sName, + v2.attrs, + pos + ); + 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()) { - auto e = TypeError({ - .msg = hintfmt("'value' attribute missing for 'listToAttrs'"), - .errPos = *v2.attrs->pos - }); - e.addTrace(pos, hintfmt("while invoking '%s'", "listToAttrs")); - throw e; - } + Bindings::iterator j2 = extractAttrNameFromPrimopCall( + state, + "listToAttrs", + state.sValue, + v2.attrs, + pos + ); v.attrs->push_back(Attr(sym, j2->value, j2->pos)); } } From ad7b47dc85c4c5fc01543bcd237bd050b50d9f4e Mon Sep 17 00:00:00 2001 From: Maximilian Bosch Date: Sun, 11 Apr 2021 13:55:07 +0200 Subject: [PATCH 4/5] primops/libexpr: use new attr-call extractor everywhere; use function's pos if attr-set pos == noPos --- src/libexpr/primops.cc | 70 ++++++++++++++++++++++++++---------------- 1 file changed, 44 insertions(+), 26 deletions(-) diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 7f3c298ba..e03f811b8 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -556,15 +556,29 @@ static Bindings::iterator extractAttrNameFromPrimopCall( ) { Bindings::iterator value = attrSet->find(state.symbols.create(attrName)); if (value == attrSet->end()) { - auto e = TypeError({ - .msg = hintfmt("attribute '%s' missing for call to '%s'", attrName, funcName), - .errPos = *attrSet->pos, - }); + hintformat errorMsg = hintfmt( + "attribute '%s' missing for call to '%s'", + attrName, + funcName + ); - // Adding another trace for the function name to make it clear - // which call received wrong arguments. - e.addTrace(pos, hintfmt("while invoking '%s'", funcName)); - throw e; + Pos aPos = *attrSet->pos; + if (aPos == noPos) { + throw TypeError({ + .msg = errorMsg, + .errPos = pos, + }); + } else { + auto e = TypeError({ + .msg = errorMsg, + .errPos = aPos, + }); + + // Adding another trace for the function name to make it clear + // which call received wrong arguments. + e.addTrace(pos, hintfmt("while invoking '%s'", funcName)); + throw e; + } } return value; @@ -841,12 +855,14 @@ static void prim_derivationStrict(EvalState & state, const Pos & pos, Value * * state.forceAttrs(*args[0], pos); /* Figure out the name first (for stack backtraces). */ - Bindings::iterator attr = args[0]->attrs->find(state.sName); - if (attr == args[0]->attrs->end()) - throw EvalError({ - .msg = hintfmt("required attribute 'name' missing"), - .errPos = pos - }); + Bindings::iterator attr = extractAttrNameFromPrimopCall( + state, + "derivationStrict", + state.sName, + args[0]->attrs, + pos + ); + string drvName; Pos & posDrvName(*attr->pos); try { @@ -1394,12 +1410,13 @@ static void prim_findFile(EvalState & state, const Pos & pos, Value * * args, Va if (i != v2.attrs->end()) prefix = state.forceStringNoCtx(*i->value, pos); - i = v2.attrs->find(state.symbols.create("path")); - if (i == v2.attrs->end()) - throw EvalError({ - .msg = hintfmt("attribute 'path' missing"), - .errPos = pos - }); + i = extractAttrNameFromPrimopCall( + state, + "findFile", + "path", + v2.attrs, + pos + ); PathSet context; string path = state.coerceToString(pos, *i->value, context, false, false); @@ -2041,12 +2058,13 @@ void prim_getAttr(EvalState & state, const Pos & pos, Value * * args, Value & v) string attr = state.forceStringNoCtx(*args[0], pos); state.forceAttrs(*args[1], pos); // !!! Should we create a symbol here or just do a lookup? - Bindings::iterator i = args[1]->attrs->find(state.symbols.create(attr)); - if (i == args[1]->attrs->end()) - throw EvalError({ - .msg = hintfmt("attribute '%1%' missing", attr), - .errPos = pos - }); + Bindings::iterator i = extractAttrNameFromPrimopCall( + state, + "getAttr", + attr, + args[1]->attrs, + pos + ); // !!! add to stack trace? if (state.countCalls && i->pos) state.attrSelects[*i->pos]++; state.forceValue(*i->value, pos); From 864ef0e93dfd58b147d46c340305acf2f9c6b314 Mon Sep 17 00:00:00 2001 From: Maximilian Bosch Date: Tue, 13 Apr 2021 23:08:59 +0200 Subject: [PATCH 5/5] libexpr/primops: review --- src/libexpr/primops.cc | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index e03f811b8..ff2f302ed 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -547,13 +547,13 @@ typedef list ValueList; #endif -static Bindings::iterator extractAttrNameFromPrimopCall( - EvalState &state, - string funcName, - string attrName, - Bindings *attrSet, - const Pos &pos -) { +static Bindings::iterator getAttr( + EvalState & state, + string funcName, + string attrName, + Bindings * attrSet, + const Pos & pos) +{ Bindings::iterator value = attrSet->find(state.symbols.create(attrName)); if (value == attrSet->end()) { hintformat errorMsg = hintfmt( @@ -589,7 +589,7 @@ static void prim_genericClosure(EvalState & state, const Pos & pos, Value * * ar state.forceAttrs(*args[0], pos); /* Get the start set. */ - Bindings::iterator startSet = extractAttrNameFromPrimopCall( + Bindings::iterator startSet = getAttr( state, "genericClosure", "startSet", @@ -604,7 +604,7 @@ static void prim_genericClosure(EvalState & state, const Pos & pos, Value * * ar workSet.push_back(startSet->value->listElems()[n]); /* Get the operator. */ - Bindings::iterator op = extractAttrNameFromPrimopCall( + Bindings::iterator op = getAttr( state, "genericClosure", "operator", @@ -855,7 +855,7 @@ static void prim_derivationStrict(EvalState & state, const Pos & pos, Value * * state.forceAttrs(*args[0], pos); /* Figure out the name first (for stack backtraces). */ - Bindings::iterator attr = extractAttrNameFromPrimopCall( + Bindings::iterator attr = getAttr( state, "derivationStrict", state.sName, @@ -1410,7 +1410,7 @@ static void prim_findFile(EvalState & state, const Pos & pos, Value * * args, Va if (i != v2.attrs->end()) prefix = state.forceStringNoCtx(*i->value, pos); - i = extractAttrNameFromPrimopCall( + i = getAttr( state, "findFile", "path", @@ -2058,7 +2058,7 @@ void prim_getAttr(EvalState & state, const Pos & pos, Value * * args, Value & v) string attr = state.forceStringNoCtx(*args[0], pos); state.forceAttrs(*args[1], pos); // !!! Should we create a symbol here or just do a lookup? - Bindings::iterator i = extractAttrNameFromPrimopCall( + Bindings::iterator i = getAttr( state, "getAttr", attr, @@ -2191,7 +2191,7 @@ static void prim_listToAttrs(EvalState & state, const Pos & pos, Value * * args, Value & v2(*args[0]->listElems()[i]); state.forceAttrs(v2, pos); - Bindings::iterator j = extractAttrNameFromPrimopCall( + Bindings::iterator j = getAttr( state, "listToAttrs", state.sName, @@ -2203,7 +2203,7 @@ static void prim_listToAttrs(EvalState & state, const Pos & pos, Value * * args, Symbol sym = state.symbols.create(name); if (seen.insert(sym).second) { - Bindings::iterator j2 = extractAttrNameFromPrimopCall( + Bindings::iterator j2 = getAttr( state, "listToAttrs", state.sValue,