guiwf/lix
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

libexpr/eval: Refactor force* and eval*, introduce check*

Browse Source 
Status quo: We have `force$Type` and `eval$Type`, both which first
produce a value and then do a type check. The type checking logic is not
consistently implemented, with lots of code duplication.
This change does:

- Introduce new `check*` functions which unify the logic (the error
handling unfortunately still needs some duplication for now)
- Make both `force*` and `eval*` use the `check*` function for the
actual type checking
- Inline and dismantle the `eval*` functions for being of little use and
little used. This makes the `ExprOp*::eval` implementations for binary
logic operators more verbose, but IMO that's a good thing: The
implementation now needs to be a lot more explicit about the
short-circuiting semantics, something which was previously hidden behind
the short-circuiting semantics of the C++ language, in a way that could
easily be overlooked and lead to confusion, which is something that
happened to me twice in a year.
- Changes `forceAttrs` and `forceList` to include the context in case
`forceValue` fails (compared to only when the type check fails). This
was done for code consistency, because I could not find any reason why
list and attrs had different semantics here than int, float and bool. So
far the visible change is minimal (see the diff on the err.exp), however
this needs vetting for potential performance regressions.

Change-Id: I33e5c706d46850c9e1126293ee01dab85ba07587
This commit is contained in:
piegames
2026-01-03 22:37:20 +01:00
parent d387c9113c
commit ee608784dd
5 changed files with 170 additions and 94 deletions
Download Patch File Download Diff File Expand all files Collapse all files
lix/libexpr/eval-inline.hh
+92 -15
View File
@@ -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<TypeError>(
"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<TypeError>(
"expected %1% but found %2%: %3%",
Uncolored(vType),
showType(v),
ValuePrinter(*this, v, errorPrintOptions)
)
.debugThrow();
}
}
template<typename... Args>
[[gnu::always_inline]]
bool EvalState::checkBool(Value & v, Args &&... errorArgs)
{
checkType(v, nBool, std::forward<Args>(errorArgs)...);
return v.boolean();
}
template<typename... Args>
[[gnu::always_inline]]
NixInt EvalState::checkInt(Value & v, Args &&... errorArgs)
{
checkType(v, nInt, std::forward<Args>(errorArgs)...);
return v.integer();
}
template<typename... Args>
[[gnu::always_inline]]
NixFloat EvalState::checkFloat(Value & v, Args &&... errorArgs)
{
if (v.type() == nInt) {
return v.integer().value;
}
checkType(v, nFloat, std::forward<Args>(errorArgs)...);
return v.fpoint();
}
template<typename... Args>
[[gnu::always_inline]]
void EvalState::checkList(Value & v, Args &&... errorArgs)
{
checkType(v, nList, std::forward<Args>(errorArgs)...);
}
template<typename... Args>
[[gnu::always_inline]]
Bindings * EvalState::checkAttrs(Value & v, Args &&... errorArgs)
{
checkType(v, nAttrs, std::forward<Args>(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<TypeError>(
"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<TypeError>(
"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;
}
}
lix/libexpr/eval.cc
+54 -69
View File
@@ -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<TypeError>( \
"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<AssertionError>("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<TypeError>(
"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<TypeError>(
"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<TypeError>(
"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();
}
lix/libexpr/eval.hh
+14 -9
View File
@@ -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<typename... Args>
bool checkBool(Value & v, Args &&... errorArgs);
template<typename... Args>
NixInt checkInt(Value & v, Args &&... errorArgs);
template<typename... Args>
NixFloat checkFloat(Value & v, Args &&... errorArgs);
template<typename... Args>
void checkList(Value & v, Args &&... errorArgs);
template<typename... Args>
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
tests/functional2/lang/attrs/in-5.nix
+4 -1
View File
@@ -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])
]
tests/functional2/lang/drv-trace-summary/eval-fail.err.exp
+6
View File
@@ -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'