From e282175f484b1c40d714bf0ae5e3311a8157ee24 Mon Sep 17 00:00:00 2001 From: Sergei Zimmerman Date: Wed, 24 Sep 2025 01:04:23 +0300 Subject: [PATCH 1/4] libexpr: Split out `MakeBinOpMembers` from `MakeBinOp` --- src/libexpr/include/nix/expr/nixexpr.hh | 63 +++++++++++++------------ 1 file changed, 33 insertions(+), 30 deletions(-) diff --git a/src/libexpr/include/nix/expr/nixexpr.hh b/src/libexpr/include/nix/expr/nixexpr.hh index aa62760d8..2682e623b 100644 --- a/src/libexpr/include/nix/expr/nixexpr.hh +++ b/src/libexpr/include/nix/expr/nixexpr.hh @@ -574,38 +574,41 @@ struct ExprOpNot : Expr COMMON_METHODS }; -#define MakeBinOp(name, s) \ - struct name : Expr \ - { \ - PosIdx pos; \ - Expr *e1, *e2; \ - name(Expr * e1, Expr * e2) \ - : e1(e1) \ - , e2(e2) {}; \ - name(const PosIdx & pos, Expr * e1, Expr * e2) \ - : pos(pos) \ - , e1(e1) \ - , e2(e2) {}; \ - void show(const SymbolTable & symbols, std::ostream & str) const override \ - { \ - str << "("; \ - e1->show(symbols, str); \ - str << " " s " "; \ - e2->show(symbols, str); \ - str << ")"; \ - } \ - void bindVars(EvalState & es, const std::shared_ptr & env) override \ - { \ - e1->bindVars(es, env); \ - e2->bindVars(es, env); \ - } \ - void eval(EvalState & state, Env & env, Value & v) override; \ - PosIdx getPos() const override \ - { \ - return pos; \ - } \ +#define MakeBinOpMembers(name, s) \ + PosIdx pos; \ + Expr *e1, *e2; \ + name(Expr * e1, Expr * e2) \ + : e1(e1) \ + , e2(e2){}; \ + name(const PosIdx & pos, Expr * e1, Expr * e2) \ + : pos(pos) \ + , e1(e1) \ + , e2(e2){}; \ + void show(const SymbolTable & symbols, std::ostream & str) const override \ + { \ + str << "("; \ + e1->show(symbols, str); \ + str << " " s " "; \ + e2->show(symbols, str); \ + str << ")"; \ + } \ + void bindVars(EvalState & es, const std::shared_ptr & env) override \ + { \ + e1->bindVars(es, env); \ + e2->bindVars(es, env); \ + } \ + void eval(EvalState & state, Env & env, Value & v) override; \ + PosIdx getPos() const override \ + { \ + return pos; \ } +#define MakeBinOp(name, s) \ + struct name : Expr \ + { \ + MakeBinOpMembers(name, s) \ + }; + MakeBinOp(ExprOpEq, "=="); MakeBinOp(ExprOpNEq, "!="); MakeBinOp(ExprOpAnd, "&&"); From b7c6cf900f4dba8c4464d5c355ee0559d06afe9b Mon Sep 17 00:00:00 2001 From: Sergei Zimmerman Date: Wed, 24 Sep 2025 01:04:26 +0300 Subject: [PATCH 2/4] libexpr: Explicitly define `ExprOpUpdate` --- src/libexpr/include/nix/expr/nixexpr.hh | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/libexpr/include/nix/expr/nixexpr.hh b/src/libexpr/include/nix/expr/nixexpr.hh index 2682e623b..e04e4f23c 100644 --- a/src/libexpr/include/nix/expr/nixexpr.hh +++ b/src/libexpr/include/nix/expr/nixexpr.hh @@ -614,9 +614,13 @@ MakeBinOp(ExprOpNEq, "!="); MakeBinOp(ExprOpAnd, "&&"); MakeBinOp(ExprOpOr, "||"); MakeBinOp(ExprOpImpl, "->"); -MakeBinOp(ExprOpUpdate, "//"); MakeBinOp(ExprOpConcatLists, "++"); +struct ExprOpUpdate : Expr +{ + MakeBinOpMembers(ExprOpUpdate, "//") +}; + struct ExprConcatStrings : Expr { PosIdx pos; From 9789019a5042e40ad34f52c007f53364fcbbbe9c Mon Sep 17 00:00:00 2001 From: Sergei Zimmerman Date: Wed, 24 Sep 2025 01:04:58 +0300 Subject: [PATCH 3/4] libexpr: Move *StackReservation constants to gc-small-vector.hh There are other places where it's useful to use these constants (notably in eval.hh). --- src/libexpr/include/nix/expr/gc-small-vector.hh | 16 ++++++++++++++++ src/libexpr/include/nix/expr/primops.hh | 16 ---------------- 2 files changed, 16 insertions(+), 16 deletions(-) diff --git a/src/libexpr/include/nix/expr/gc-small-vector.hh b/src/libexpr/include/nix/expr/gc-small-vector.hh index fdd80b2c7..95c028e5a 100644 --- a/src/libexpr/include/nix/expr/gc-small-vector.hh +++ b/src/libexpr/include/nix/expr/gc-small-vector.hh @@ -26,4 +26,20 @@ using SmallValueVector = SmallVector; template using SmallTemporaryValueVector = SmallVector; +/** + * For functions where we do not expect deep recursion, we can use a sizable + * part of the stack a free allocation space. + * + * Note: this is expected to be multiplied by sizeof(Value), or about 24 bytes. + */ +constexpr size_t nonRecursiveStackReservation = 128; + +/** + * Functions that maybe applied to self-similar inputs, such as concatMap on a + * tree, should reserve a smaller part of the stack for allocation. + * + * Note: this is expected to be multiplied by sizeof(Value), or about 24 bytes. + */ +constexpr size_t conservativeStackReservation = 16; + } // namespace nix diff --git a/src/libexpr/include/nix/expr/primops.hh b/src/libexpr/include/nix/expr/primops.hh index 885a53e9a..6407ba84e 100644 --- a/src/libexpr/include/nix/expr/primops.hh +++ b/src/libexpr/include/nix/expr/primops.hh @@ -8,22 +8,6 @@ namespace nix { -/** - * For functions where we do not expect deep recursion, we can use a sizable - * part of the stack a free allocation space. - * - * Note: this is expected to be multiplied by sizeof(Value), or about 24 bytes. - */ -constexpr size_t nonRecursiveStackReservation = 128; - -/** - * Functions that maybe applied to self-similar inputs, such as concatMap on a - * tree, should reserve a smaller part of the stack for allocation. - * - * Note: this is expected to be multiplied by sizeof(Value), or about 24 bytes. - */ -constexpr size_t conservativeStackReservation = 16; - struct RegisterPrimOp { typedef std::vector PrimOps; From 97ce7759d07fc44967e7fb3030fe9cbb8ebc2c92 Mon Sep 17 00:00:00 2001 From: Sergei Zimmerman Date: Wed, 24 Sep 2025 21:47:59 +0300 Subject: [PATCH 4/4] libexpr: Use same naive iterative merging but with `evalForUpdate` --- src/libexpr/eval.cc | 38 ++++++++++++++++--- src/libexpr/include/nix/expr/nixexpr.hh | 22 ++++++++++- .../lang/eval-fail-recursion.err.exp | 4 +- 3 files changed, 55 insertions(+), 9 deletions(-) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 18212940e..87b1e73a5 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -1866,12 +1866,8 @@ void ExprOpImpl::eval(EvalState & state, Env & env, Value & v) || state.evalBool(env, e2, pos, "in the right operand of the IMPL (->) operator")); } -void ExprOpUpdate::eval(EvalState & state, Env & env, Value & v) +void ExprOpUpdate::eval(EvalState & state, Value & v, Value & v1, Value & v2) { - Value v1, v2; - state.evalAttrs(env, e1, v1, pos, "in the left operand of the update (//) operator"); - state.evalAttrs(env, e2, v2, pos, "in the right operand of the update (//) operator"); - state.nrOpUpdates++; const Bindings & bindings1 = *v1.attrs(); @@ -1945,6 +1941,38 @@ void ExprOpUpdate::eval(EvalState & state, Env & env, Value & v) state.nrOpUpdateValuesCopied += v.attrs()->size(); } +void ExprOpUpdate::eval(EvalState & state, Env & env, Value & v) +{ + UpdateQueue q; + evalForUpdate(state, env, q); + + v.mkAttrs(&Bindings::emptyBindings); + for (auto & rhs : std::views::reverse(q)) { + /* Remember that queue is sorted rightmost attrset first. */ + eval(state, /*v=*/v, /*v1=*/v, /*v2=*/rhs); + } +} + +void Expr::evalForUpdate(EvalState & state, Env & env, UpdateQueue & q, std::string_view errorCtx) +{ + Value v; + state.evalAttrs(env, this, v, getPos(), errorCtx); + q.push_back(v); +} + +void ExprOpUpdate::evalForUpdate(EvalState & state, Env & env, UpdateQueue & q) +{ + /* Output rightmost attrset first to the merge queue as the one + with the most priority. */ + e2->evalForUpdate(state, env, q, "in the right operand of the update (//) operator"); + e1->evalForUpdate(state, env, q, "in the left operand of the update (//) operator"); +} + +void ExprOpUpdate::evalForUpdate(EvalState & state, Env & env, UpdateQueue & q, std::string_view errorCtx) +{ + evalForUpdate(state, env, q); +} + void ExprOpConcatLists::eval(EvalState & state, Env & env, Value & v) { Value v1; diff --git a/src/libexpr/include/nix/expr/nixexpr.hh b/src/libexpr/include/nix/expr/nixexpr.hh index e04e4f23c..7721918c3 100644 --- a/src/libexpr/include/nix/expr/nixexpr.hh +++ b/src/libexpr/include/nix/expr/nixexpr.hh @@ -4,6 +4,7 @@ #include #include +#include "nix/expr/gc-small-vector.hh" #include "nix/expr/value.hh" #include "nix/expr/symbol-table.hh" #include "nix/expr/eval-error.hh" @@ -80,6 +81,8 @@ typedef std::vector AttrPath; std::string showAttrPath(const SymbolTable & symbols, const AttrPath & attrPath); +using UpdateQueue = SmallTemporaryValueVector; + /* Abstract syntax of Nix expressions. */ struct Expr @@ -110,6 +113,14 @@ struct Expr * of thunks allocated. */ virtual Value * maybeThunk(EvalState & state, Env & env); + + /** + * Only called when performing an attrset update: `//` or similar. + * Instead of writing to a Value &, this function writes to an UpdateQueue. + * This allows the expression to perform multiple updates in a delayed manner, gathering up all the updates before + * applying them. + */ + virtual void evalForUpdate(EvalState & state, Env & env, UpdateQueue & q, std::string_view errorCtx); virtual void setName(Symbol name); virtual void setDocComment(DocComment docComment) {}; @@ -607,7 +618,7 @@ struct ExprOpNot : Expr struct name : Expr \ { \ MakeBinOpMembers(name, s) \ - }; + } MakeBinOp(ExprOpEq, "=="); MakeBinOp(ExprOpNEq, "!="); @@ -618,7 +629,14 @@ MakeBinOp(ExprOpConcatLists, "++"); struct ExprOpUpdate : Expr { - MakeBinOpMembers(ExprOpUpdate, "//") +private: + /** Special case for merging of two attrsets. */ + void eval(EvalState & state, Value & v, Value & v1, Value & v2); + void evalForUpdate(EvalState & state, Env & env, UpdateQueue & q); + +public: + MakeBinOpMembers(ExprOpUpdate, "//"); + virtual void evalForUpdate(EvalState & state, Env & env, UpdateQueue & q, std::string_view errorCtx) override; }; struct ExprConcatStrings : Expr diff --git a/tests/functional/lang/eval-fail-recursion.err.exp b/tests/functional/lang/eval-fail-recursion.err.exp index 8bfb4e12e..ee41ff46b 100644 --- a/tests/functional/lang/eval-fail-recursion.err.exp +++ b/tests/functional/lang/eval-fail-recursion.err.exp @@ -1,9 +1,9 @@ error: … in the right operand of the update (//) operator - at /pwd/lang/eval-fail-recursion.nix:2:11: + at /pwd/lang/eval-fail-recursion.nix:2:14: 1| let 2| a = { } // a; - | ^ + | ^ 3| in error: infinite recursion encountered