Merge pull request #9555 from 9999years/positions-in-errors

Pass positions when evaluating

(cherry picked from commit c8458bd731eb1c74159bebe459ea00165e056b65)
Change-Id: I1b4a5d58973be6264ffdb23b4492da200fdb71be

doc/manual/rl-next/source-positions-in-errors.md

@@ -0,0 +1,43 @@
+synopsis: Source locations are printed more consistently in errors
+issues: #561
+prs: #9555
+description: {
+
+Source location information is now included in error messages more
+consistently. Given this code:
+
+```nix
+let
+  attr = {foo = "bar";};
+  key = {};
+in
+  attr.${key}
+```
+
+Previously, Nix would show this unhelpful message when attempting to evaluate
+it:
+
+```
+error:
+       … while evaluating an attribute name
+
+       error: value is a set while a string was expected
+```
+
+Now, the error message displays where the problematic value was found:
+
+```
+error:
+       … while evaluating an attribute name
+
+         at bad.nix:4:11:
+
+            3|   key = {};
+            4| in attr.${key}
+             |           ^
+            5|
+
+       error: value is a set while a string was expected
+```
+
+}

src/libexpr/eval-inline.hh

@@ -103,8 +103,10 @@ void EvalState::forceValue(Value & v, Callable getPos)
             throw;
         }
     }
-    else if (v.isApp())
+    else if (v.isApp()) {
-        callFunction(*v.app.left, *v.app.right, v, noPos);
+        PosIdx pos = getPos();
+        callFunction(*v.app.left, *v.app.right, v, pos);
+    }
     else if (v.isBlackhole())
         error("infinite recursion encountered").atPos(getPos()).template debugThrow<EvalError>();
 }
@@ -121,9 +123,9 @@ template <typename Callable>
 [[gnu::always_inline]]
 inline void EvalState::forceAttrs(Value & v, Callable getPos, std::string_view errorCtx)
 {
-    forceValue(v, noPos);
+    PosIdx pos = getPos();
+    forceValue(v, pos);
     if (v.type() != nAttrs) {
-        PosIdx pos = getPos();
         error("value is %1% while a set was expected", showType(v)).withTrace(pos, errorCtx).debugThrow<TypeError>();
     }
 }
@@ -132,7 +134,7 @@ inline void EvalState::forceAttrs(Value & v, Callable getPos, std::string_view e
 [[gnu::always_inline]]
 inline void EvalState::forceList(Value & v, const PosIdx pos, std::string_view errorCtx)
 {
-    forceValue(v, noPos);
+    forceValue(v, pos);
     if (!v.isList()) {
         error("value is %1% while a list was expected", showType(v)).withTrace(pos, errorCtx).debugThrow<TypeError>();
     }

src/libexpr/eval.cc

@@ -341,7 +341,7 @@ static Symbol getName(const AttrName & name, EvalState & state, Env & env)
     } else {
         Value nameValue;
         name.expr->eval(state, env, nameValue);
-        state.forceStringNoCtx(nameValue, noPos, "while evaluating an attribute name");
+        state.forceStringNoCtx(nameValue, name.expr->getPos(), "while evaluating an attribute name");
         return state.symbols.create(nameValue.string.s);
     }
 }
@@ -1503,7 +1503,7 @@ void ExprOpHasAttr::eval(EvalState & state, Env & env, Value & v)
     e->eval(state, env, vTmp);
 
     for (auto & i : attrPath) {
-        state.forceValue(*vAttrs, noPos);
+        state.forceValue(*vAttrs, getPos());
         Bindings::iterator j;
         auto name = getName(i, state, env);
         if (vAttrs->type() != nAttrs ||
@@ -1672,7 +1672,7 @@ void EvalState::callFunction(Value & fun, size_t nrArgs, Value * * args, Value &
                 if (countCalls) primOpCalls[name]++;
 
                 try {
-                    vCur.primOp->fun(*this, noPos, args, vCur);
+                    vCur.primOp->fun(*this, vCur.determinePos(noPos), args, vCur);
                 } catch (Error & e) {
                     addErrorTrace(e, pos, "while calling the '%1%' builtin", name);
                     throw;
@@ -1720,7 +1720,7 @@ void EvalState::callFunction(Value & fun, size_t nrArgs, Value * * args, Value &
                     // 1. Unify this and above code. Heavily redundant.
                     // 2. Create a fake env (arg1, arg2, etc.) and a fake expr (arg1: arg2: etc: builtins.name arg1 arg2 etc)
                     //    so the debugger allows to inspect the wrong parameters passed to the builtin.
-                    primOp->primOp->fun(*this, noPos, vArgs, vCur);
+                    primOp->primOp->fun(*this, vCur.determinePos(noPos), vArgs, vCur);
                 } catch (Error & e) {
                     addErrorTrace(e, pos, "while calling the '%1%' builtin", name);
                     throw;
@@ -1828,7 +1828,7 @@ https://nixos.org/manual/nix/stable/language/constructs.html#functions.)", symbo
         }
     }
 
-    callFunction(fun, allocValue()->mkAttrs(attrs), res, noPos);
+    callFunction(fun, allocValue()->mkAttrs(attrs), res, pos);
 }
 
 
@@ -1864,7 +1864,7 @@ void ExprAssert::eval(EvalState & state, Env & env, Value & v)
 
 void ExprOpNot::eval(EvalState & state, Env & env, Value & v)
 {
-    v.mkBool(!state.evalBool(env, e, noPos, "in the argument of the not operator")); // XXX: FIXME: !
+    v.mkBool(!state.evalBool(env, e, getPos(), "in the argument of the not operator")); // XXX: FIXME: !
 }
 
 
@@ -2305,7 +2305,7 @@ BackedStringView EvalState::coerceToString(
             std::string result;
             for (auto [n, v2] : enumerate(v.listItems())) {
                 try {
-                    result += *coerceToString(noPos, *v2, context,
+                    result += *coerceToString(pos, *v2, context,
                             "while evaluating one element of the list",
                             coerceMore, copyToStore, canonicalizePath);
                 } catch (Error & e) {
@@ -2428,8 +2428,8 @@ SingleDerivedPath EvalState::coerceToSingleDerivedPath(const PosIdx pos, Value &
 
 bool EvalState::eqValues(Value & v1, Value & v2, const PosIdx pos, std::string_view errorCtx)
 {
-    forceValue(v1, noPos);
+    forceValue(v1, pos);
-    forceValue(v2, noPos);
+    forceValue(v2, pos);
 
     /* !!! Hack to support some old broken code that relies on pointer
        equality tests between sets.  (Specifically, builderDefs calls

src/libexpr/nixexpr.hh

@@ -400,6 +400,7 @@ struct ExprOpNot : Expr
 {
     Expr * e;
     ExprOpNot(Expr * e) : e(e) { };
+    PosIdx getPos() const override { return e->getPos(); }
     COMMON_METHODS
 };
 

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

@@ -0,0 +1,20 @@
+error:
+       … while evaluating the attribute 'puppy."${key}"'
+
+         at /pwd/lang/eval-fail-attr-name-type.nix:3:5:
+
+            2|   attrs = {
+            3|     puppy.doggy = {};
+             |     ^
+            4|   };
+
+       … while evaluating an attribute name
+
+         at /pwd/lang/eval-fail-attr-name-type.nix:7:17:
+
+            6| in
+            7|   attrs.puppy.${key}
+             |                 ^
+            8|
+
+       error: value is an integer while a string was expected

tests/functional/lang/eval-fail-attr-name-type.nix

@@ -0,0 +1,7 @@
+let
+  attrs = {
+    puppy.doggy = {};
+  };
+  key = 1;
+in
+  attrs.puppy.${key}

tests/functional/lang/eval-fail-call-primop.err.exp

@@ -0,0 +1,12 @@
+error:
+       … while calling the 'length' builtin
+
+         at /pwd/lang/eval-fail-call-primop.nix:1:1:
+
+            1| builtins.length 1
+             | ^
+            2|
+
+       … while evaluating the first argument passed to builtins.length
+
+       error: value is an integer while a list was expected

tests/functional/lang/eval-fail-call-primop.nix

@@ -0,0 +1 @@
+builtins.length 1

tests/functional/lang/eval-fail-not-throws.err.exp

@@ -0,0 +1,18 @@
+error:
+       … in the argument of the not operator
+
+         at /pwd/lang/eval-fail-not-throws.nix:1:4:
+
+            1| ! (throw "uh oh!")
+             |    ^
+            2|
+
+       … while calling the 'throw' builtin
+
+         at /pwd/lang/eval-fail-not-throws.nix:1:4:
+
+            1| ! (throw "uh oh!")
+             |    ^
+            2|
+
+       error: uh oh!

tests/functional/lang/eval-fail-not-throws.nix

@@ -0,0 +1 @@
+! (throw "uh oh!")

tests/functional/lang/eval-fail-using-set-as-attr-name.err.exp

@@ -0,0 +1,11 @@
+error:
+       … while evaluating an attribute name
+
+         at /pwd/lang/eval-fail-using-set-as-attr-name.nix:5:10:
+
+            4| in
+            5|   attr.${key}
+             |          ^
+            6|
+
+       error: value is a set while a string was expected

tests/functional/lang/eval-fail-using-set-as-attr-name.nix

@@ -0,0 +1,5 @@
+let
+  attr = {foo = "bar";};
+  key = {};
+in
+  attr.${key}