From 4df1a3ca7661ee4aa1f0c626c577f60a487d30f3 Mon Sep 17 00:00:00 2001 From: Sergei Zimmerman Date: Thu, 11 Sep 2025 01:51:48 +0300 Subject: [PATCH 1/3] libexpr: Make emptyBindings a global constant This object is always constant and will never get modified. Having it as a global (constant) static is much easier and unclutters the EvalState. Same idea as in https://git.lix.systems/lix-project/lix/commit/f017f9ddd336e32a5ed1ee835f1c6c7e73a052ae. Co-authored-by: eldritch horrors --- src/libexpr/attr-path.cc | 4 ++-- src/libexpr/attr-set.cc | 4 +++- src/libexpr/eval.cc | 1 - src/libexpr/include/nix/expr/attr-set.hh | 6 ++++++ src/libexpr/include/nix/expr/eval.hh | 2 -- src/libexpr/primops.cc | 4 ++-- src/nix/flake.cc | 2 +- src/nix/nix-env/user-env.cc | 2 +- src/nix/upgrade-nix.cc | 2 +- 9 files changed, 16 insertions(+), 11 deletions(-) diff --git a/src/libexpr/attr-path.cc b/src/libexpr/attr-path.cc index b02b08db4..58705bfa1 100644 --- a/src/libexpr/attr-path.cc +++ b/src/libexpr/attr-path.cc @@ -110,8 +110,8 @@ std::pair findPackageFilename(EvalState & state, Value & v { Value * v2; try { - auto dummyArgs = state.allocBindings(0); - v2 = findAlongAttrPath(state, "meta.position", *dummyArgs, v).first; + auto & dummyArgs = Bindings::emptyBindings; + v2 = findAlongAttrPath(state, "meta.position", dummyArgs, v).first; } catch (Error &) { throw NoPositionInfo("package '%s' has no source location information", what); } diff --git a/src/libexpr/attr-set.cc b/src/libexpr/attr-set.cc index eb44b0dd9..48d4c4d4a 100644 --- a/src/libexpr/attr-set.cc +++ b/src/libexpr/attr-set.cc @@ -5,13 +5,15 @@ namespace nix { +Bindings Bindings::emptyBindings; + /* Allocate a new array of attributes for an attribute set with a specific capacity. The space is implicitly reserved after the Bindings structure. */ Bindings * EvalState::allocBindings(size_t capacity) { if (capacity == 0) - return &emptyBindings; + return &Bindings::emptyBindings; if (capacity > std::numeric_limits::max()) throw Error("attribute set of size %d is too big", capacity); nrAttrsets++; diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index df4e52e5d..b586c3409 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -202,7 +202,6 @@ EvalState::EvalState( , settings{settings} , symbols(StaticEvalSymbols::staticSymbolTable()) , repair(NoRepair) - , emptyBindings(Bindings()) , storeFS(makeMountedSourceAccessor({ {CanonPath::root, makeEmptySourceAccessor()}, /* In the pure eval case, we can simply require diff --git a/src/libexpr/include/nix/expr/attr-set.hh b/src/libexpr/include/nix/expr/attr-set.hh index b5e927a7e..4ab54c8eb 100644 --- a/src/libexpr/include/nix/expr/attr-set.hh +++ b/src/libexpr/include/nix/expr/attr-set.hh @@ -54,6 +54,12 @@ public: typedef uint32_t size_t; PosIdx pos; + /** + * An instance of bindings objects with 0 attributes. + * This object must never be modified. + */ + static Bindings emptyBindings; + private: size_t size_ = 0; Attr attrs[0]; diff --git a/src/libexpr/include/nix/expr/eval.hh b/src/libexpr/include/nix/expr/eval.hh index 5015a009b..0b91645ea 100644 --- a/src/libexpr/include/nix/expr/eval.hh +++ b/src/libexpr/include/nix/expr/eval.hh @@ -313,8 +313,6 @@ public: */ RepairFlag repair; - Bindings emptyBindings; - /** * Empty list constant. */ diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 9ba417c32..2a3eec672 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -3326,14 +3326,14 @@ static void prim_functionArgs(EvalState & state, const PosIdx pos, Value ** args { state.forceValue(*args[0], pos); if (args[0]->isPrimOpApp() || args[0]->isPrimOp()) { - v.mkAttrs(&state.emptyBindings); + v.mkAttrs(&Bindings::emptyBindings); return; } if (!args[0]->isLambda()) state.error("'functionArgs' requires a function").atPos(pos).debugThrow(); if (!args[0]->lambda().fun->hasFormals()) { - v.mkAttrs(&state.emptyBindings); + v.mkAttrs(&Bindings::emptyBindings); return; } diff --git a/src/nix/flake.cc b/src/nix/flake.cc index 8d6387c9d..3b1e2f5e4 100644 --- a/src/nix/flake.cc +++ b/src/nix/flake.cc @@ -522,7 +522,7 @@ struct CmdFlakeCheck : FlakeCommand auto checkNixOSConfiguration = [&](const std::string & attrPath, Value & v, const PosIdx pos) { try { Activity act(*logger, lvlInfo, actUnknown, fmt("checking NixOS configuration '%s'", attrPath)); - Bindings & bindings(*state->allocBindings(0)); + Bindings & bindings = Bindings::emptyBindings; auto vToplevel = findAlongAttrPath(*state, "config.system.build.toplevel", bindings, v).first; state->forceValue(*vToplevel, pos); if (!state->isDerivation(*vToplevel)) diff --git a/src/nix/nix-env/user-env.cc b/src/nix/nix-env/user-env.cc index 4ed93135d..552172825 100644 --- a/src/nix/nix-env/user-env.cc +++ b/src/nix/nix-env/user-env.cc @@ -24,7 +24,7 @@ PackageInfos queryInstalled(EvalState & state, const Path & userEnv) if (pathExists(manifestFile)) { Value v; state.evalFile(state.rootPath(CanonPath(manifestFile)).resolveSymlinks(), v); - Bindings & bindings(*state.allocBindings(0)); + Bindings & bindings = Bindings::emptyBindings; getDerivations(state, v, "", bindings, elems, false); } return elems; diff --git a/src/nix/upgrade-nix.cc b/src/nix/upgrade-nix.cc index 48235a27f..f26613bf8 100644 --- a/src/nix/upgrade-nix.cc +++ b/src/nix/upgrade-nix.cc @@ -162,7 +162,7 @@ struct CmdUpgradeNix : MixDryRun, StoreCommand auto state = std::make_unique(LookupPath{}, store, fetchSettings, evalSettings); auto v = state->allocValue(); state->eval(state->parseExprFromString(res.data, state->rootPath(CanonPath("/no-such-path"))), *v); - Bindings & bindings(*state->allocBindings(0)); + Bindings & bindings = Bindings::emptyBindings; auto v2 = findAlongAttrPath(*state, settings.thisSystem, bindings, *v).first; return store->parseStorePath( From 462b9ac49c14c4751c2f56b79297124427fb71f8 Mon Sep 17 00:00:00 2001 From: Sergei Zimmerman Date: Mon, 8 Sep 2025 01:20:31 +0300 Subject: [PATCH 2/3] libexpr: Make Value::isa and Value::getStorage private methods This was always intended to be the case, but accidentally left in the public interface. --- src/libexpr/include/nix/expr/value.hh | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libexpr/include/nix/expr/value.hh b/src/libexpr/include/nix/expr/value.hh index 82db1a775..0b10b78b5 100644 --- a/src/libexpr/include/nix/expr/value.hh +++ b/src/libexpr/include/nix/expr/value.hh @@ -833,6 +833,7 @@ struct Value : public ValueStorage { friend std::string showType(const Value & v); +private: template bool isa() const noexcept { From 5db4b0699ce880e8a4a2e836dd536834718da7a3 Mon Sep 17 00:00:00 2001 From: Sergei Zimmerman Date: Thu, 11 Sep 2025 01:53:41 +0300 Subject: [PATCH 3/3] libexpr: Make constant Values global constants, move out of EvalState These constant Values have no business being in the EvalState in the first place. The ultimate goal is to get rid of the ugly `getBuiltins` and its relience (in `createBaseEnv`) on these global constants is getting in the way. Same idea as in https://git.lix.systems/lix-project/lix/commit/f017f9ddd336e32a5ed1ee835f1c6c7e73a052ae. Co-authored-by: eldritch horrors --- src/libexpr/eval.cc | 8 ++------ src/libexpr/include/nix/expr/eval.hh | 26 ------------------------ src/libexpr/include/nix/expr/value.hh | 28 ++++++++++++++++++++++++++ src/libexpr/meson.build | 1 + src/libexpr/primops.cc | 12 +++++------ src/libexpr/value.cc | 29 +++++++++++++++++++++++++++ src/nix/nix-env/nix-env.cc | 2 +- 7 files changed, 67 insertions(+), 39 deletions(-) create mode 100644 src/libexpr/value.cc diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index b586c3409..3a53ecf79 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -284,10 +284,6 @@ EvalState::EvalState( static_assert(sizeof(Env) <= 16, "environment must be <= 16 bytes"); - vEmptyList.mkList(buildList(0)); - vNull.mkNull(); - vTrue.mkBool(true); - vFalse.mkBool(false); vStringRegular.mkStringNoCopy("regular"); vStringDirectory.mkStringNoCopy("directory"); vStringSymlink.mkStringNoCopy("symlink"); @@ -894,7 +890,7 @@ ListBuilder::ListBuilder(EvalState & state, size_t size) Value * EvalState::getBool(bool b) { - return b ? &vTrue : &vFalse; + return b ? &Value::vTrue : &Value::vFalse; } unsigned long nrThunks = 0; @@ -1300,7 +1296,7 @@ void ExprList::eval(EvalState & state, Env & env, Value & v) Value * ExprList::maybeThunk(EvalState & state, Env & env) { if (elems.empty()) { - return &state.vEmptyList; + return &Value::vEmptyList; } return Expr::maybeThunk(state, env); } diff --git a/src/libexpr/include/nix/expr/eval.hh b/src/libexpr/include/nix/expr/eval.hh index 0b91645ea..430e334b8 100644 --- a/src/libexpr/include/nix/expr/eval.hh +++ b/src/libexpr/include/nix/expr/eval.hh @@ -313,32 +313,6 @@ public: */ RepairFlag repair; - /** - * Empty list constant. - */ - Value vEmptyList; - - /** - * `null` constant. - * - * This is _not_ a singleton. Pointer equality is _not_ sufficient. - */ - Value vNull; - - /** - * `true` constant. - * - * This is _not_ a singleton. Pointer equality is _not_ sufficient. - */ - Value vTrue; - - /** - * `true` constant. - * - * This is _not_ a singleton. Pointer equality is _not_ sufficient. - */ - Value vFalse; - /** `"regular"` */ Value vStringRegular; /** `"directory"` */ diff --git a/src/libexpr/include/nix/expr/value.hh b/src/libexpr/include/nix/expr/value.hh index 0b10b78b5..c74588a31 100644 --- a/src/libexpr/include/nix/expr/value.hh +++ b/src/libexpr/include/nix/expr/value.hh @@ -833,6 +833,34 @@ struct Value : public ValueStorage { friend std::string showType(const Value & v); + /** + * Empty list constant. + * + * This is _not_ a singleton. Pointer equality is _not_ sufficient. + */ + static Value vEmptyList; + + /** + * `null` constant. + * + * This is _not_ a singleton. Pointer equality is _not_ sufficient. + */ + static Value vNull; + + /** + * `true` constant. + * + * This is _not_ a singleton. Pointer equality is _not_ sufficient. + */ + static Value vTrue; + + /** + * `true` constant. + * + * This is _not_ a singleton. Pointer equality is _not_ sufficient. + */ + static Value vFalse; + private: template bool isa() const noexcept diff --git a/src/libexpr/meson.build b/src/libexpr/meson.build index 00fb82e3c..40d3f390b 100644 --- a/src/libexpr/meson.build +++ b/src/libexpr/meson.build @@ -163,6 +163,7 @@ sources = files( 'search-path.cc', 'value-to-json.cc', 'value-to-xml.cc', + 'value.cc', 'value/context.cc', ) diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 2a3eec672..f099e060e 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -1075,11 +1075,11 @@ static void prim_tryEval(EvalState & state, const PosIdx pos, Value ** args, Val try { state.forceValue(*args[0], pos); attrs.insert(state.s.value, args[0]); - attrs.insert(state.symbols.create("success"), &state.vTrue); + attrs.insert(state.symbols.create("success"), &Value::vTrue); } catch (AssertionError & e) { // `value = false;` is unfortunate but removing it is a breaking change. - attrs.insert(state.s.value, &state.vFalse); - attrs.insert(state.symbols.create("success"), &state.vFalse); + attrs.insert(state.s.value, &Value::vFalse); + attrs.insert(state.symbols.create("success"), &Value::vFalse); } // restore the debugRepl pointer if we saved it earlier. @@ -4613,7 +4613,7 @@ void prim_match(EvalState & state, const PosIdx pos, Value ** args, Value & v) auto list = state.buildList(match.size() - 1); for (const auto & [i, v2] : enumerate(list)) if (!match[i + 1].matched) - v2 = &state.vNull; + v2 = &Value::vNull; else v2 = mkString(state, match[i + 1]); v.mkList(list); @@ -4705,7 +4705,7 @@ void prim_split(EvalState & state, const PosIdx pos, Value ** args, Value & v) auto list2 = state.buildList(slen); for (const auto & [si, v2] : enumerate(list2)) { if (!match[si + 1].matched) - v2 = &state.vNull; + v2 = &Value::vNull; else v2 = mkString(state, match[si + 1]); } @@ -5059,7 +5059,7 @@ void EvalState::createBaseEnv(const EvalSettings & evalSettings) addConstant( "null", - &vNull, + &Value::vNull, { .type = nNull, .doc = R"( diff --git a/src/libexpr/value.cc b/src/libexpr/value.cc new file mode 100644 index 000000000..07d036b0d --- /dev/null +++ b/src/libexpr/value.cc @@ -0,0 +1,29 @@ +#include "nix/expr/value.hh" + +namespace nix { + +Value Value::vEmptyList = []() { + Value res; + res.setStorage(List{.size = 0, .elems = nullptr}); + return res; +}(); + +Value Value::vNull = []() { + Value res; + res.mkNull(); + return res; +}(); + +Value Value::vTrue = []() { + Value res; + res.mkBool(true); + return res; +}(); + +Value Value::vFalse = []() { + Value res; + res.mkBool(false); + return res; +}(); + +} // namespace nix diff --git a/src/nix/nix-env/nix-env.cc b/src/nix/nix-env/nix-env.cc index f165c069c..01c8ccf4b 100644 --- a/src/nix/nix-env/nix-env.cc +++ b/src/nix/nix-env/nix-env.cc @@ -158,7 +158,7 @@ static void loadSourceExpr(EvalState & state, const SourcePath & path, Value & v directory). */ else if (st.type == SourceAccessor::tDirectory) { auto attrs = state.buildBindings(maxAttrs); - attrs.insert(state.symbols.create("_combineChannels"), &state.vEmptyList); + attrs.insert(state.symbols.create("_combineChannels"), &Value::vEmptyList); StringSet seen; getAllExprs(state, path, seen, attrs); v.mkAttrs(attrs);