diff --git a/lix/libexpr/eval-inline.hh b/lix/libexpr/eval-inline.hh index 3d414928d..9f62556ff 100644 --- a/lix/libexpr/eval-inline.hh +++ b/lix/libexpr/eval-inline.hh @@ -100,6 +100,85 @@ Env & EvalMemory::allocEnv(size_t size) return *env; } +/* The overloaded versions of `checkType` exist because of non-unified error handling + * The variant which takes an Expression is required because of debug frames (`withFrames`). + * Ideally, at some point in the future, we'd implement debug frames that are not tied to the expression and + * env and then unify both `checkType` functions into one. Then the argument forwarding overloading hack done + * for the other functions below will be removable again. + */ +[[gnu::always_inline]] +void EvalState::checkType(Value & v, ValueType vType, Env & env, Expr & e) +{ + if (v.type() != vType) { + ctx.errors + .make( + "expected %1% but found %2%: %3%", + Uncolored(vType), + showType(v), + ValuePrinter(*this, v, errorPrintOptions) + ) + .atPos(e.getPos()) + .withFrame(env, e) + .debugThrow(); + } +} + +[[gnu::always_inline]] +void EvalState::checkType(Value & v, ValueType vType) +{ + if (v.type() != vType) { + ctx.errors + .make( + "expected %1% but found %2%: %3%", + Uncolored(vType), + showType(v), + ValuePrinter(*this, v, errorPrintOptions) + ) + .debugThrow(); + } +} + +template +[[gnu::always_inline]] +bool EvalState::checkBool(Value & v, Args &&... errorArgs) +{ + checkType(v, nBool, std::forward(errorArgs)...); + return v.boolean(); +} + +template +[[gnu::always_inline]] +NixInt EvalState::checkInt(Value & v, Args &&... errorArgs) +{ + checkType(v, nInt, std::forward(errorArgs)...); + return v.integer(); +} + +template +[[gnu::always_inline]] +NixFloat EvalState::checkFloat(Value & v, Args &&... errorArgs) +{ + if (v.type() == nInt) { + return v.integer().value; + } + checkType(v, nFloat, std::forward(errorArgs)...); + return v.fpoint(); +} + +template +[[gnu::always_inline]] +void EvalState::checkList(Value & v, Args &&... errorArgs) +{ + checkType(v, nList, std::forward(errorArgs)...); +} + +template +[[gnu::always_inline]] +Bindings * EvalState::checkAttrs(Value & v, Args &&... errorArgs) +{ + checkType(v, nAttrs, std::forward(errorArgs)...); + return v.attrs(); +} [[gnu::always_inline]] void EvalState::forceValue(Value & v, const PosIdx pos) @@ -138,15 +217,14 @@ void EvalState::forceValue(Value & v, const PosIdx pos) } [[gnu::always_inline]] -inline void EvalState::forceAttrs(Value & v, const PosIdx pos, std::string_view errorCtx) +inline Bindings * EvalState::forceAttrs(Value & v, const PosIdx pos, std::string_view errorCtx) { - forceValue(v, pos); - if (v.type() != nAttrs) { - ctx.errors.make( - "expected a set but found %1%: %2%", - showType(v), - ValuePrinter(*this, v, errorPrintOptions) - ).withTrace(pos, errorCtx).debugThrow(); + try { + forceValue(v, pos); + return checkAttrs(v); + } catch (Error & e) { + e.addTrace(ctx.positions[pos], errorCtx); + throw; } } @@ -154,13 +232,12 @@ inline void EvalState::forceAttrs(Value & v, const PosIdx pos, std::string_view [[gnu::always_inline]] inline void EvalState::forceList(Value & v, const PosIdx pos, std::string_view errorCtx) { - forceValue(v, pos); - if (!v.isList()) { - ctx.errors.make( - "expected a list but found %1%: %2%", - showType(v), - ValuePrinter(*this, v, errorPrintOptions) - ).withTrace(pos, errorCtx).debugThrow(); + try { + forceValue(v, pos); + checkList(v); + } catch (Error & e) { + e.addTrace(ctx.positions[pos], errorCtx); + throw; } } diff --git a/lix/libexpr/eval.cc b/lix/libexpr/eval.cc index 330497e92..e6cffd61c 100644 --- a/lix/libexpr/eval.cc +++ b/lix/libexpr/eval.cc @@ -1056,37 +1056,6 @@ void EvalState::eval(Expr & e, Value & v) e.eval(*this, ctx.builtins.env, v); } -#define checkType(typeName, stringName) \ - if (v.type() != (typeName)) \ - ctx.errors.make( \ - "expected a %1% but found %2%: %3%", \ - Uncolored(stringName), \ - showType(v), \ - ValuePrinter(*this, v, errorPrintOptions) \ - ).atPos(e.getPos()).withFrame(env, e).debugThrow(); - -inline bool EvalState::evalBool(Env & env, Expr & e) -{ - Value v; - e.eval(*this, env, v); - checkType(nBool, "Boolean"); - return v.boolean(); -} - - -inline void EvalState::evalAttrs(Env & env, Expr & e, Value & v) -{ - e.eval(*this, env, v); - checkType(nAttrs, "set"); -} - -inline void EvalState::evalList(Env & env, Expr & e, Value & v) -{ - e.eval(*this, env, v); - checkType(nList, "list"); -} - - void Expr::eval(EvalState & state, Env & env, Value & v) { abort(); @@ -1870,13 +1839,17 @@ void ExprWith::eval(EvalState & state, Env & env, Value & v) void ExprIf::eval(EvalState & state, Env & env, Value & v) { - (state.evalBool(env, *cond) ? *then : *else_).eval(state, env, v); + Value vCond; + cond->eval(state, env, vCond); + (state.checkBool(vCond, env, *cond) ? *then : *else_).eval(state, env, v); } void ExprAssert::eval(EvalState & state, Env & env, Value & v) { - if (!state.evalBool(env, *cond)) { + Value vCond; + cond->eval(state, env, vCond); + if (!state.checkBool(vCond, env, *cond)) { state.ctx.errors.make("assertion failed") .atPos(pos) .withFrame(env, *this) @@ -1888,7 +1861,9 @@ void ExprAssert::eval(EvalState & state, Env & env, Value & v) void ExprOpNot::eval(EvalState & state, Env & env, Value & v) { - v.mkBool(!state.evalBool(env, *e)); + Value vInner; + e->eval(state, env, vInner); + v.mkBool(!state.checkBool(vInner, env, *e)); } @@ -1910,27 +1885,57 @@ void ExprOpNEq::eval(EvalState & state, Env & env, Value & v) void ExprOpAnd::eval(EvalState & state, Env & env, Value & v) { - v.mkBool(state.evalBool(env, *e1) && state.evalBool(env, *e2)); + Value v1; + e1->eval(state, env, v1); + /* Explicitly short-circuit */ + if (!state.checkBool(v1, env, *e1)) { + v.mkBool(false); + return; + } + Value v2; + e2->eval(state, env, v2); + v.mkBool(state.checkBool(v2, env, *e2)); } void ExprOpOr::eval(EvalState & state, Env & env, Value & v) { - v.mkBool(state.evalBool(env, *e1) || state.evalBool(env, *e2)); + Value v1; + e1->eval(state, env, v1); + /* Explicitly short-circuit */ + if (state.checkBool(v1, env, *e1)) { + v.mkBool(true); + return; + } + Value v2; + e2->eval(state, env, v2); + v.mkBool(state.checkBool(v2, env, *e2)); } void ExprOpImpl::eval(EvalState & state, Env & env, Value & v) { - v.mkBool(!state.evalBool(env, *e1) || state.evalBool(env, *e2)); + Value v1; + e1->eval(state, env, v1); + /* Explicitly short-circuit (ex falso quodlibet) */ + if (!state.checkBool(v1, env, *e1)) { + v.mkBool(true); + return; + } + Value v2; + e2->eval(state, env, v2); + v.mkBool(state.checkBool(v2, env, *e2)); } void ExprOpUpdate::eval(EvalState & state, Env & env, Value & v) { - Value v1, v2; - state.evalAttrs(env, *e1, v1); - state.evalAttrs(env, *e2, v2); + Value v1; + e1->eval(state, env, v1); + state.checkAttrs(v1, env, *e1); + Value v2; + e2->eval(state, env, v2); + state.checkAttrs(v2, env, *e2); state.ctx.stats.nrOpUpdates++; @@ -1970,8 +1975,12 @@ void ExprOpConcatLists::eval(EvalState & state, Env & env, Value & v) /* We don't call into `concatLists` as that loses the position information of the expressions. */ - Value v1; state.evalList(env, *e1, v1); - Value v2; state.evalList(env, *e2, v2); + Value v1; + e1->eval(state, env, v1); + state.checkList(v1, env, *e1); + Value v2; + e2->eval(state, env, v2); + state.checkList(v2, env, *e2); size_t l1 = v1.listSize(), l2 = v2.listSize(), len = l1 + l2; @@ -2211,19 +2220,11 @@ NixInt EvalState::forceInt(Value & v, const PosIdx pos, std::string_view errorCt { try { forceValue(v, pos); - if (v.type() != nInt) - ctx.errors.make( - "expected an integer but found %1%: %2%", - showType(v), - ValuePrinter(*this, v, errorPrintOptions) - ).atPos(pos).debugThrow(); - return v.integer(); + return checkInt(v); } catch (Error & e) { e.addTrace(ctx.positions[pos], errorCtx); throw; } - - return v.integer(); } @@ -2231,15 +2232,7 @@ NixFloat EvalState::forceFloat(Value & v, const PosIdx pos, std::string_view err { try { forceValue(v, pos); - if (v.type() == nInt) - return v.integer().value; - else if (v.type() != nFloat) - ctx.errors.make( - "expected a float but found %1%: %2%", - showType(v), - ValuePrinter(*this, v, errorPrintOptions) - ).atPos(pos).debugThrow(); - return v.fpoint(); + return checkFloat(v); } catch (Error & e) { e.addTrace(ctx.positions[pos], errorCtx); throw; @@ -2251,19 +2244,11 @@ bool EvalState::forceBool(Value & v, const PosIdx pos, std::string_view errorCtx { try { forceValue(v, pos); - if (v.type() != nBool) - ctx.errors.make( - "expected a Boolean but found %1%: %2%", - showType(v), - ValuePrinter(*this, v, errorPrintOptions) - ).atPos(pos).debugThrow(); - return v.boolean(); + return checkBool(v); } catch (Error & e) { e.addTrace(ctx.positions[pos], errorCtx); throw; } - - return v.boolean(); } diff --git a/lix/libexpr/eval.hh b/lix/libexpr/eval.hh index b18b936bd..bacef0991 100644 --- a/lix/libexpr/eval.hh +++ b/lix/libexpr/eval.hh @@ -654,14 +654,6 @@ public: */ void eval(Expr & e, Value & v); - /** - * Evaluation the expression, then verify that it has the expected - * type. - */ - inline bool evalBool(Env & env, Expr & e); - inline void evalAttrs(Env & env, Expr & e, Value & v); - inline void evalList(Env & env, Expr & e, Value & v); - /** * If `v` is a thunk, enter it and overwrite `v` with the result * of the evaluation of the thunk. If `v` is a delayed function @@ -685,7 +677,7 @@ public: NixFloat forceFloat(Value & v, const PosIdx pos, std::string_view errorCtx); bool forceBool(Value & v, const PosIdx pos, std::string_view errorCtx); - void forceAttrs(Value & v, const PosIdx pos, std::string_view errorCtx); + inline Bindings * forceAttrs(Value & v, const PosIdx pos, std::string_view errorCtx); inline void forceList(Value & v, const PosIdx pos, std::string_view errorCtx); /** * @param v either lambda or primop @@ -695,6 +687,19 @@ public: std::string_view forceString(Value & v, NixStringContext & context, const PosIdx pos, std::string_view errorCtx); std::string_view forceStringNoCtx(Value & v, const PosIdx pos, std::string_view errorCtx); + inline void checkType(Value & v, ValueType vType, Env & env, Expr & e); + inline void checkType(Value & v, ValueType vType); + template + bool checkBool(Value & v, Args &&... errorArgs); + template + NixInt checkInt(Value & v, Args &&... errorArgs); + template + NixFloat checkFloat(Value & v, Args &&... errorArgs); + template + void checkList(Value & v, Args &&... errorArgs); + template + Bindings * checkAttrs(Value & v, Args &&... errorArgs); + /** * Realise the given context, and return a mapping from the placeholders * used to construct the associated value to their final store path diff --git a/tests/functional2/lang/attrs/in-5.nix b/tests/functional2/lang/attrs/in-5.nix index a4584cd3b..5d108bc7f 100644 --- a/tests/functional2/lang/attrs/in-5.nix +++ b/tests/functional2/lang/attrs/in-5.nix @@ -16,6 +16,9 @@ in as.x.y.bla or bs.f-o-o.bar or "xyzzy" as.x.y.bla or bs.bar.foo or "xyzzy" (123).bla or null.foo or "xyzzy" - # Backwards compatibility test. + # Backwards compatibility test for `fun or` being handled as intended. + # n.b. this code contains a type error, because the nul value should be false instead of []. + # but the code expands to `true || (false || (false || [])))`, so as long as at least one value in the list is true + # it short-circuits and never runs into the type error (fold or [] [true false false]) ] diff --git a/tests/functional2/lang/drv-trace-summary/eval-fail.err.exp b/tests/functional2/lang/drv-trace-summary/eval-fail.err.exp index 3147281fa..535a7a9fd 100644 --- a/tests/functional2/lang/drv-trace-summary/eval-fail.err.exp +++ b/tests/functional2/lang/drv-trace-summary/eval-fail.err.exp @@ -1,6 +1,8 @@ error: … while calling the 'getAttr' builtin at «internal»:1:500: + … while evaluating the second argument passed to builtins.getAttr + … while calling the 'derivationStrict' builtin at «internal»:1:208: … while evaluating derivation 'package-you-care-about' @@ -22,6 +24,8 @@ error: … while calling the 'getAttr' builtin at «internal»:1:500: + … while evaluating the second argument passed to builtins.getAttr + … while calling the 'derivationStrict' builtin at «internal»:1:208: … while evaluating derivation 'direct-dependency' @@ -41,6 +45,8 @@ error: … while calling the 'getAttr' builtin at «internal»:1:500: + … while evaluating the second argument passed to builtins.getAttr + … while calling the 'derivationStrict' builtin at «internal»:1:208: … while evaluating derivation 'transitive-dependency'