From dab871f1296209edd56d75b01baf5efcbadb6a20 Mon Sep 17 00:00:00 2001 From: Raito Bezarius Date: Tue, 20 May 2025 01:25:19 +0200 Subject: [PATCH] libexpr: refactor string coercion modes Inspired by cl/3191 and https://git.lix.systems/delroth/lix/commit/ae0247cbb4fc739ab013dc87d02e5f3191cf25ab. `coerceToString` takes now an enumeration that lives in `value.hh`, this enumeration is meant to represent increasing subsets of behaviors, e.g. any level above Strict should do what the previous levels do and extra behavior until `ToString`, which transforms many Nix values into an arbitrary string representation, e.g. `null` to `""`. Change-Id: Ief7a4756e8c0660e197623efebeaf07710746ec7 Signed-off-by: Raito Bezarius Co-authored-by: Pierre Bourdon --- lix/libexpr/eval.cc | 24 ++++++++++++------------ lix/libexpr/eval.hh | 7 +++---- lix/libexpr/flake/flake.cc | 2 +- lix/libexpr/primops.cc | 18 +++++++++--------- lix/libexpr/primops/fetchMercurial.cc | 4 ++-- lix/libexpr/primops/fetchTree.cc | 4 ++-- lix/libexpr/value-to-json.cc | 2 +- lix/libexpr/value.hh | 20 +++++++++++++++++++- subprojects/nix-eval-jobs/src/worker.cc | 2 +- 9 files changed, 50 insertions(+), 33 deletions(-) diff --git a/lix/libexpr/eval.cc b/lix/libexpr/eval.cc index 3beec65d3..762e2827d 100644 --- a/lix/libexpr/eval.cc +++ b/lix/libexpr/eval.cc @@ -2033,7 +2033,7 @@ void ExprConcatStrings::eval(EvalState & state, Env & env, Value & v) path */ auto part = state.coerceToString(i_pos, vTmp, context, "while evaluating a path segment", - false, firstType == nString, !first); + StringCoercionMode::Strict, firstType == nString, !first); sSize += part->size(); s.emplace_back(std::move(part)); } @@ -2266,7 +2266,7 @@ bool EvalState::isDerivation(Value & v) std::optional EvalState::tryAttrsToString(const PosIdx pos, Value & v, - NixStringContext & context, bool coerceMore, bool copyToStore) + NixStringContext & context, StringCoercionMode mode, bool copyToStore) { auto i = v.attrs->find(ctx.s.toString); if (i != v.attrs->end()) { @@ -2275,7 +2275,7 @@ std::optional EvalState::tryAttrsToString(const PosIdx pos, Value & callFunction(*i->value, v, v1, i->pos); return coerceToString(pos, v1, context, "while evaluating the result of the `__toString` attribute", - coerceMore, copyToStore).toOwned(); + mode, copyToStore).toOwned(); } catch (EvalError & e) { e.addTrace(ctx.positions[pos], "while converting a set to string"); throw; @@ -2290,7 +2290,7 @@ BackedStringView EvalState::coerceToString( Value & v, NixStringContext & context, std::string_view errorCtx, - bool coerceMore, + StringCoercionMode mode, bool copyToStore, bool canonicalizePath) { @@ -2314,7 +2314,7 @@ BackedStringView EvalState::coerceToString( } if (v.type() == nAttrs) { - auto maybeString = tryAttrsToString(pos, v, context, coerceMore, copyToStore); + auto maybeString = tryAttrsToString(pos, v, context, mode, copyToStore); if (maybeString) return std::move(*maybeString); auto i = v.attrs->find(ctx.s.outPath); @@ -2328,19 +2328,19 @@ BackedStringView EvalState::coerceToString( .debugThrow(); } return coerceToString(pos, *i->value, context, errorCtx, - coerceMore, copyToStore, canonicalizePath); + mode, copyToStore, canonicalizePath); } if (v.type() == nExternal) { try { - return v.external->coerceToString(*this, pos, context, coerceMore, copyToStore); + return v.external->coerceToString(*this, pos, context, mode, copyToStore); } catch (Error & e) { e.addTrace(nullptr, errorCtx); throw; } } - if (coerceMore) { + if (mode >= StringCoercionMode::ToString) { /* Note that `false' is represented as an empty string for shell scripting convenience, just like `null'. */ if (v.type() == nBool && v.boolean) return "1"; @@ -2355,7 +2355,7 @@ BackedStringView EvalState::coerceToString( try { result += *coerceToString(pos, *v2, context, "while evaluating one element of the list", - coerceMore, copyToStore, canonicalizePath); + mode, copyToStore, canonicalizePath); } catch (Error & e) { e.addTrace(ctx.positions[pos], errorCtx); throw; @@ -2412,7 +2412,7 @@ try { SourcePath EvalState::coerceToPath(const PosIdx pos, Value & v, NixStringContext & context, std::string_view errorCtx) { - auto path = coerceToString(pos, v, context, errorCtx, false, false, true).toOwned(); + auto path = coerceToString(pos, v, context, errorCtx, StringCoercionMode::Strict, false, true).toOwned(); if (path == "" || path[0] != '/') ctx.errors.make("string '%1%' doesn't represent an absolute path", path).withTrace(pos, errorCtx).debugThrow(); return CanonPath(path); @@ -2421,7 +2421,7 @@ SourcePath EvalState::coerceToPath(const PosIdx pos, Value & v, NixStringContext StorePath EvalState::coerceToStorePath(const PosIdx pos, Value & v, NixStringContext & context, std::string_view errorCtx) { - auto path = coerceToString(pos, v, context, errorCtx, false, false, true).toOwned(); + auto path = coerceToString(pos, v, context, errorCtx, StringCoercionMode::Strict, false, true).toOwned(); if (auto storePath = ctx.store->maybeParseStorePath(path)) return *storePath; ctx.errors.make("path '%1%' is not in the Nix store", path).withTrace(pos, errorCtx).debugThrow(); @@ -2883,7 +2883,7 @@ try { } -std::string ExternalValueBase::coerceToString(EvalState & state, const PosIdx & pos, NixStringContext & context, bool copyMore, bool copyToStore) const +std::string ExternalValueBase::coerceToString(EvalState & state, const PosIdx & pos, NixStringContext & context, StringCoercionMode mode, bool copyToStore) const { state.ctx.errors.make( "cannot coerce %1% to a string: %2%", showType(), *this diff --git a/lix/libexpr/eval.hh b/lix/libexpr/eval.hh index 3635fbc9a..5cd5820ec 100644 --- a/lix/libexpr/eval.hh +++ b/lix/libexpr/eval.hh @@ -767,19 +767,18 @@ public: bool isDerivation(Value & v); std::optional tryAttrsToString(const PosIdx pos, Value & v, - NixStringContext & context, bool coerceMore = false, bool copyToStore = true); + NixStringContext & context, StringCoercionMode mode = StringCoercionMode::Strict, bool copyToStore = true); /** * String coercion. * * Converts strings, paths and derivations to a - * string. If `coerceMore` is set, also converts nulls, integers, - * booleans and lists to a string. If `copyToStore` is set, + * string. If `copyToStore` is set, * referenced paths are copied to the Nix store as a side effect. */ BackedStringView coerceToString(const PosIdx pos, Value & v, NixStringContext & context, std::string_view errorCtx, - bool coerceMore = false, bool copyToStore = true, + StringCoercionMode mode = StringCoercionMode::Strict, bool copyToStore = true, bool canonicalizePath = true); /** diff --git a/lix/libexpr/flake/flake.cc b/lix/libexpr/flake/flake.cc index ff14b8e00..07661f1c7 100644 --- a/lix/libexpr/flake/flake.cc +++ b/lix/libexpr/flake/flake.cc @@ -298,7 +298,7 @@ static Flake getFlake( NixStringContext emptyContext = {}; flake.config.settings.emplace( state.ctx.symbols[setting.name], - state.coerceToString(setting.pos, *setting.value, emptyContext, "", false, true, true) .toOwned()); + state.coerceToString(setting.pos, *setting.value, emptyContext, "", StringCoercionMode::Strict, true, true) .toOwned()); } else if (setting.value->type() == nInt) flake.config.settings.emplace( diff --git a/lix/libexpr/primops.cc b/lix/libexpr/primops.cc index 993a30d9f..2d2bce27c 100644 --- a/lix/libexpr/primops.cc +++ b/lix/libexpr/primops.cc @@ -315,13 +315,13 @@ void prim_exec(EvalState & state, Value * * args, Value & v) NixStringContext context; auto program = state.coerceToString(noPos, *elems[0], context, "while evaluating the first element of the argument passed to builtins.exec", - false, false).toOwned(); + StringCoercionMode::Strict, false).toOwned(); Strings commandArgs; for (unsigned int i = 1; i < args[0]->listSize(); ++i) { commandArgs.push_back( state.coerceToString(noPos, *elems[i], context, "while evaluating an element of the argument passed to builtins.exec", - false, false).toOwned()); + StringCoercionMode::Strict, false).toOwned()); } try { auto _ = state.realiseContext(context); // FIXME: Handle CA derivations @@ -610,7 +610,7 @@ static void prim_addErrorContext(EvalState & state, Value * * args, Value & v) NixStringContext context; auto message = state.coerceToString(noPos, *args[0], context, "while evaluating the error message passed to builtins.addErrorContext", - false, false).toOwned(); + StringCoercionMode::Strict, false).toOwned(); e.addTrace(nullptr, HintFmt(message)); throw; } @@ -880,7 +880,7 @@ drvName, Bindings * attrs, Value & v) for (auto elem : i->value->listItems()) { auto s = state.coerceToString(noPos, *elem, context, "while evaluating an element of the argument list", - true).toOwned(); + StringCoercionMode::ToString).toOwned(); drv.args.push_back(s); } } @@ -929,7 +929,7 @@ drvName, Bindings * attrs, Value & v) } else { - auto s = state.coerceToString(noPos, *i->value, context, context_below, true).toOwned(); + auto s = state.coerceToString(noPos, *i->value, context, context_below, StringCoercionMode::ToString).toOwned(); drv.env.emplace(key, s); if (i->name == state.ctx.s.builder) drv.builder = std::move(s); else if (i->name == state.ctx.s.system) drv.platform = std::move(s); @@ -1212,7 +1212,7 @@ static void prim_baseNameOf(EvalState & state, Value * * args, Value & v) NixStringContext context; v.mkString(baseNameOf(*state.coerceToString(noPos, *args[0], context, "while evaluating the first argument passed to builtins.baseNameOf", - false, false)), context); + StringCoercionMode::Strict, false)), context); } /* Return the directory of the given path, i.e., everything before the @@ -1228,7 +1228,7 @@ static void prim_dirOf(EvalState & state, Value * * args, Value & v) NixStringContext context; auto path = state.coerceToString(noPos, *args[0], context, "while evaluating the first argument passed to 'builtins.dirOf'", - false, false); + StringCoercionMode::Strict, false); auto dir = dirOf(*path); v.mkString(dir, context); } @@ -1289,7 +1289,7 @@ static void prim_findFile(EvalState & state, Value * * args, Value & v) NixStringContext context; auto path = state.coerceToString(noPos, *i->value, context, "while evaluating the `path` attribute of an element of the list passed to builtins.findFile", - false, false).toOwned(); + StringCoercionMode::Strict, false).toOwned(); try { auto rewrites = state.realiseContext(context); @@ -2453,7 +2453,7 @@ static void prim_toString(EvalState & state, Value * * args, Value & v) NixStringContext context; auto s = state.coerceToString(noPos, *args[0], context, "while evaluating the first argument passed to builtins.toString", - true, false); + StringCoercionMode::ToString, false); v.mkString(*s, context); } diff --git a/lix/libexpr/primops/fetchMercurial.cc b/lix/libexpr/primops/fetchMercurial.cc index f7ee77162..47d4b1eb8 100644 --- a/lix/libexpr/primops/fetchMercurial.cc +++ b/lix/libexpr/primops/fetchMercurial.cc @@ -22,7 +22,7 @@ static void prim_fetchMercurial(EvalState & state, Value * * args, Value & v) if (n == "url") url = state.coerceToString(attr.pos, *attr.value, context, "while evaluating the `url` attribute passed to builtins.fetchMercurial", - false, false).toOwned(); + StringCoercionMode::Strict, false).toOwned(); else if (n == "rev") { // Ugly: unlike fetchGit, here the "rev" attribute can // be both a revision or a branch/tag name. @@ -44,7 +44,7 @@ static void prim_fetchMercurial(EvalState & state, Value * * args, Value & v) } else url = state.coerceToString(noPos, *args[0], context, "while evaluating the first argument passed to builtins.fetchMercurial", - false, false).toOwned(); + StringCoercionMode::Strict, false).toOwned(); // FIXME: git externals probably can be used to bypass the URI // whitelist. Ah well. diff --git a/lix/libexpr/primops/fetchTree.cc b/lix/libexpr/primops/fetchTree.cc index 2b6e21fb9..b793640f2 100644 --- a/lix/libexpr/primops/fetchTree.cc +++ b/lix/libexpr/primops/fetchTree.cc @@ -139,7 +139,7 @@ static void fetchTree( if (attr.name == state.ctx.s.type) continue; state.forceValue(*attr.value, attr.pos); if (attr.value->type() == nPath || attr.value->type() == nString) { - auto s = state.coerceToString(attr.pos, *attr.value, context, "", false, false).toOwned(); + auto s = state.coerceToString(attr.pos, *attr.value, context, "", StringCoercionMode::Strict, false).toOwned(); attrs.emplace(state.ctx.symbols[attr.name], state.ctx.symbols[attr.name] == "url" ? type == "git" @@ -173,7 +173,7 @@ static void fetchTree( } else { auto url = state.coerceToString(pos, *args[0], context, "while evaluating the first argument passed to the fetcher", - false, false).toOwned(); + StringCoercionMode::Strict, false).toOwned(); if (type == "git") { fetchers::Attrs attrs; diff --git a/lix/libexpr/value-to-json.cc b/lix/libexpr/value-to-json.cc index 60efd13bb..faa5e8fe4 100644 --- a/lix/libexpr/value-to-json.cc +++ b/lix/libexpr/value-to-json.cc @@ -46,7 +46,7 @@ JSON printValueAsJSON(EvalState & state, bool strict, break; case nAttrs: { - auto maybeString = state.tryAttrsToString(pos, v, context, false, false); + auto maybeString = state.tryAttrsToString(pos, v, context, StringCoercionMode::Strict, false); if (maybeString) { out = *maybeString; break; diff --git a/lix/libexpr/value.hh b/lix/libexpr/value.hh index c15d5335f..07c8c42f9 100644 --- a/lix/libexpr/value.hh +++ b/lix/libexpr/value.hh @@ -10,6 +10,7 @@ #include "lix/libexpr/gc-alloc.hh" #include "lix/libexpr/symbol-table.hh" #include "lix/libexpr/value/context.hh" +#include "lix/libutil/logging.hh" #include "lix/libutil/source-path.hh" #include "lix/libexpr/print-options.hh" #include "lix/libutil/checked-arithmetic.hh" @@ -59,6 +60,23 @@ typedef enum { nExternal } ValueType; + +/** + * Modes of string coercion. + * + * Determines how permissive the coercion functions are when converting + * values to strings. + * + * - Strict: Only allow coercion of values that are already strings, + * paths, or derivations. + * - ToString: Additionally allow coercion of integers, booleans, null, + * and lists to strings. + */ +enum class StringCoercionMode { + Strict, + ToString, +}; + class Bindings; struct Env; struct Expr; @@ -106,7 +124,7 @@ class ExternalValueBase * Coerce the value to a string. Defaults to uncoercable, i.e. throws an * error. */ - virtual std::string coerceToString(EvalState & state, const PosIdx & pos, NixStringContext & context, bool copyMore, bool copyToStore) const; + virtual std::string coerceToString(EvalState & state, const PosIdx & pos, NixStringContext & context, StringCoercionMode mode, bool copyToStore) const; /** * Compare to another value of the same type. Defaults to uncomparable, diff --git a/subprojects/nix-eval-jobs/src/worker.cc b/subprojects/nix-eval-jobs/src/worker.cc index da8834d31..014407495 100644 --- a/subprojects/nix-eval-jobs/src/worker.cc +++ b/subprojects/nix-eval-jobs/src/worker.cc @@ -94,7 +94,7 @@ readConstituents(const nix::Value *v, nix::box_ptr &state, nix::NixStringContext context; state->coerceToString(a->pos, *a->value, context, "while evaluating the `constituents` attribute", - true, false); + nix::StringCoercionMode::ToString, false); for (auto &c : context) std::visit(nix::overloaded{ [&](const nix::NixStringContextElem::Built &b) {