Use `std::set<StringContextElem>` not `PathSet` for string contexts

Motivation

`PathSet` is not correct because string contexts have other forms
(`Built` and `DrvDeep`) that are not rendered as plain store paths.
Instead of wrongly using `PathSet`, or "stringly typed" using
`StringSet`, use `std::std<StringContextElem>`.

-----

In support of this change, `NixStringContext` is now defined as
`std::std<StringContextElem>` not `std:vector<StringContextElem>`. The
old definition was just used by a `getContext` method which was only
used by the eval cache. It can be deleted altogether since the types are
now unified and the preexisting `copyContext` function already suffices.

Summarizing the previous paragraph:

Old:

  - `value/context.hh`: `NixStringContext = std::vector<StringContextElem>`
  - `value.hh`: `NixStringContext Value::getContext(...)`
  - `value.hh`: `copyContext(...)`

New:

  - `value/context.hh`: `NixStringContext = std::set<StringContextElem>`
  - `value.hh`: `copyContext(...)`
----

The string representation of string context elements no longer contains
the store dir. The diff of `src/libexpr/tests/value/context.cc` should
make clear what the new representation is, so we recommend reviewing
that file first. This was done for two reasons:

Less API churn:

`Value::mkString` and friends did not take a `Store` before. But if
`NixStringContextElem::{parse, to_string}` *do* take a store (as they
did before), then we cannot have the `Value` functions use them (in
order to work with the fully-structured `NixStringContext`) without
adding that argument.

That would have been a lot of churn of threading the store, and this
diff is already large enough, so the easier and less invasive thing to
do was simply make the element `parse` and `to_string` functions not
take the `Store` reference, and the easiest way to do that was to simply
drop the store dir.

Space usage:

Dropping the `/nix/store/` (or similar) from the internal representation
will safe space in the heap of the Nix programming being interpreted. If
the heap contains many strings with non-trivial contexts, the saving
could add up to something significant.

----

The eval cache version is bumped.

The eval cache serialization uses `NixStringContextElem::{parse,
to_string}`, and since those functions are changed per the above, that
means the on-disk representation is also changed.

This is simply done by changing the name of the used for the eval cache
from `eval-cache-v4` to eval-cache-v5`.

----

To avoid some duplication `EvalCache::mkPathString` is added to abstract
over the simple case of turning a store path to a string with just that
string in the context.

Context

This PR picks up where #7543 left off. That one introduced the fully
structured `NixStringContextElem` data type, but kept `PathSet context`
as an awkward middle ground between internal `char[][]` interpreter heap
string contexts and `NixStringContext` fully parsed string contexts.

The infelicity of `PathSet context` was specifically called out during
Nix team group review, but it was agreeing that fixing it could be left
as future work. This is that future work.

A possible follow-up step would be to get rid of the `char[][]`
evaluator heap representation, too, but it is not yet clear how to do
that. To use `NixStringContextElem` there we would need to get the STL
containers to GC pointers in the GC build, and I am not sure how to do
that.

----

PR #7543 effectively is writing the inverse of a `mkPathString`,
`mkOutputString`, and one more such function for the `DrvDeep` case. I
would like that PR to have property tests ensuring it is actually the
inverse as expected.

This PR sets things up nicely so that reworking that PR to be in that
more elegant and better tested way is possible.

Co-authored-by: Théophane Hufschmitt <7226587+thufschmitt@users.noreply.github.com>

1 files changed, 54 insertions, 41 deletions

diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc
index 510f674eb..d11b56f8b 100644
--- a/src/libexpr/primops.cc
+++ b/src/libexpr/primops.cc
@@ -38,17 +38,16 @@ namespace nix {
 InvalidPathError::InvalidPathError(const Path & path) :
     EvalError("path '%s' is not valid", path), path(path) {}
 
-StringMap EvalState::realiseContext(const PathSet & context)
+StringMap EvalState::realiseContext(const NixStringContext & context)
 {
     std::vector<DerivedPath::Built> drvs;
     StringMap res;
 
-    for (auto & c_ : context) {
+    for (auto & c : context) {
         auto ensureValid = [&](const StorePath & p) {
             if (!store->isValidPath(p))
                 debugThrowLastTrace(InvalidPathError(store->printStorePath(p)));
         };
-        auto c = NixStringContextElem::parse(*store, c_);
         std::visit(overloaded {
             [&](const NixStringContextElem::Built & b) {
                 drvs.push_back(DerivedPath::Built {
@@ -112,7 +111,7 @@ struct RealisePathFlags {
 
 static Path realisePath(EvalState & state, const PosIdx pos, Value & v, const RealisePathFlags flags = {})
 {
-    PathSet context;
+    NixStringContext context;
 
     auto path = state.coerceToPath(noPos, v, context, "while realising the context of a path");
 
@@ -158,7 +157,12 @@ static void mkOutputString(
             /* FIXME: we need to depend on the basic derivation, not
                derivation */
             : downstreamPlaceholder(*state.store, drvPath, o.first),
-        {"!" + o.first + "!" + state.store->printStorePath(drvPath)});
+        NixStringContext {
+            NixStringContextElem::Built {
+                .drvPath = drvPath,
+                .output = o.first,
+            }
+        });
 }
 
 /* Load and evaluate an expression from path specified by the
@@ -181,7 +185,9 @@ static void import(EvalState & state, const PosIdx pos, Value & vPath, Value * v
         auto storePath = *optStorePath;
         Derivation drv = state.store->readDerivation(storePath);
         auto attrs = state.buildBindings(3 + drv.outputs.size());
-        attrs.alloc(state.sDrvPath).mkString(path, {"=" + path});
+        attrs.alloc(state.sDrvPath).mkString(path, {
+            NixStringContextElem::DrvDeep { .drvPath = storePath },
+        });
         attrs.alloc(state.sName).mkString(drv.env["name"]);
         auto & outputsVal = attrs.alloc(state.sOutputs);
         state.mkList(outputsVal, drv.outputs.size());
@@ -358,7 +364,7 @@ void prim_exec(EvalState & state, const PosIdx pos, Value * * args, Value & v)
     auto count = args[0]->listSize();
     if (count == 0)
         state.error("at least one argument to 'exec' required").atPos(pos).debugThrow<EvalError>();
-    PathSet context;
+    NixStringContext context;
     auto program = state.coerceToString(pos, *elems[0], context,
             "while evaluating the first element of the argument passed to builtins.exec",
             false, false).toOwned();
@@ -768,7 +774,7 @@ static RegisterPrimOp primop_abort({
     )",
     .fun = [](EvalState & state, const PosIdx pos, Value * * args, Value & v)
     {
-        PathSet context;
+        NixStringContext context;
         auto s = state.coerceToString(pos, *args[0], context,
                 "while evaluating the error message passed to builtins.abort").toOwned();
         state.debugThrowLastTrace(Abort("evaluation aborted with the following error message: '%1%'", s));
@@ -787,7 +793,7 @@ static RegisterPrimOp primop_throw({
     )",
     .fun = [](EvalState & state, const PosIdx pos, Value * * args, Value & v)
     {
-      PathSet context;
+      NixStringContext context;
       auto s = state.coerceToString(pos, *args[0], context,
               "while evaluating the error message passed to builtin.throw").toOwned();
       state.debugThrowLastTrace(ThrownError(s));
@@ -800,7 +806,7 @@ static void prim_addErrorContext(EvalState & state, const PosIdx pos, Value * *
         state.forceValue(*args[1], pos);
         v = *args[1];
     } catch (Error & e) {
-        PathSet context;
+        NixStringContext context;
         auto message = state.coerceToString(pos, *args[0], context,
                 "while evaluating the error message passed to builtins.addErrorContext",
                 false, false).toOwned();
@@ -1086,7 +1092,7 @@ drvName, Bindings * attrs, Value & v)
     Derivation drv;
     drv.name = drvName;
 
-    PathSet context;
+    NixStringContext context;
 
     bool contentAddressed = false;
     bool isImpure = false;
@@ -1232,8 +1238,7 @@ drvName, Bindings * attrs, Value & v)
     /* Everything in the context of the strings in the derivation
        attributes should be added as dependencies of the resulting
        derivation. */
-    for (auto & c_ : context) {
-        auto c = NixStringContextElem::parse(*state.store, c_);
+    for (auto & c : context) {
         std::visit(overloaded {
             /* Since this allows the builder to gain access to every
                path in the dependency graph of the derivation (including
@@ -1392,7 +1397,9 @@ drvName, Bindings * attrs, Value & v)
     }
 
     auto result = state.buildBindings(1 + drv.outputs.size());
-    result.alloc(state.sDrvPath).mkString(drvPathS, {"=" + drvPathS});
+    result.alloc(state.sDrvPath).mkString(drvPathS, {
+        NixStringContextElem::DrvDeep { .drvPath = drvPath },
+    });
     for (auto & i : drv.outputs)
         mkOutputString(state, result, drvPath, drv, i);
 
@@ -1437,7 +1444,7 @@ static RegisterPrimOp primop_placeholder({
 /* Convert the argument to a path.  !!! obsolete? */
 static void prim_toPath(EvalState & state, const PosIdx pos, Value * * args, Value & v)
 {
-    PathSet context;
+    NixStringContext context;
     Path path = state.coerceToPath(pos, *args[0], context, "while evaluating the first argument passed to builtins.toPath");
     v.mkString(canonPath(path), context);
 }
@@ -1468,7 +1475,7 @@ static void prim_storePath(EvalState & state, const PosIdx pos, Value * * args,
             .errPos = state.positions[pos]
         }));
 
-    PathSet context;
+    NixStringContext context;
     Path path = state.checkSourcePath(state.coerceToPath(pos, *args[0], context, "while evaluating the first argument passed to builtins.storePath"));
     /* Resolve symlinks in ‘path’, unless ‘path’ itself is a symlink
        directly in the store.  The latter condition is necessary so
@@ -1482,7 +1489,7 @@ static void prim_storePath(EvalState & state, const PosIdx pos, Value * * args,
     auto path2 = state.store->toStorePath(path).first;
     if (!settings.readOnlyMode)
         state.store->ensurePath(path2);
-    context.insert(state.store->printStorePath(path2));
+    context.insert(NixStringContextElem::Opaque { .path = path2 });
     v.mkString(path, context);
 }
 
@@ -1538,7 +1545,7 @@ static RegisterPrimOp primop_pathExists({
    following the last slash. */
 static void prim_baseNameOf(EvalState & state, const PosIdx pos, Value * * args, Value & v)
 {
-    PathSet context;
+    NixStringContext context;
     v.mkString(baseNameOf(*state.coerceToString(pos, *args[0], context,
             "while evaluating the first argument passed to builtins.baseNameOf",
             false, false)), context);
@@ -1560,7 +1567,7 @@ static RegisterPrimOp primop_baseNameOf({
    of the argument. */
 static void prim_dirOf(EvalState & state, const PosIdx pos, Value * * args, Value & v)
 {
-    PathSet context;
+    NixStringContext context;
     auto path = state.coerceToString(pos, *args[0], context,
             "while evaluating the first argument passed to builtins.dirOf",
             false, false);
@@ -1597,7 +1604,12 @@ static void prim_readFile(EvalState & state, const PosIdx pos, Value * * args, V
         refsSink << s;
         refs = refsSink.getResultPaths();
     }
-    auto context = state.store->printStorePathSet(refs);
+    NixStringContext context;
+    for (auto && p : std::move(refs)) {
+        context.insert(NixStringContextElem::Opaque {
+            .path = std::move((StorePath &&)p),
+        });
+    }
     v.mkString(s, context);
 }
 
@@ -1628,7 +1640,7 @@ static void prim_findFile(EvalState & state, const PosIdx pos, Value * * args, V
 
         i = getAttr(state, state.sPath, v2->attrs, "in an element of the __nixPath");
 
-        PathSet context;
+        NixStringContext context;
         auto path = state.coerceToString(pos, *i->value, context,
                 "while evaluating the `path` attribute of an element of the list passed to builtins.findFile",
                 false, false).toOwned();
@@ -1787,7 +1799,7 @@ static RegisterPrimOp primop_readDir({
 static void prim_toXML(EvalState & state, const PosIdx pos, Value * * args, Value & v)
 {
     std::ostringstream out;
-    PathSet context;
+    NixStringContext context;
     printValueAsXML(state, true, false, *args[0], out, context, pos);
     v.mkString(out.str(), context);
 }
@@ -1895,7 +1907,7 @@ static RegisterPrimOp primop_toXML({
 static void prim_toJSON(EvalState & state, const PosIdx pos, Value * * args, Value & v)
 {
     std::ostringstream out;
-    PathSet context;
+    NixStringContext context;
     printValueAsJSON(state, true, *args[0], pos, out, context);
     v.mkString(out.str(), context);
 }
@@ -1945,22 +1957,23 @@ static RegisterPrimOp primop_fromJSON({
    as an input by derivations. */
 static void prim_toFile(EvalState & state, const PosIdx pos, Value * * args, Value & v)
 {
-    PathSet context;
+    NixStringContext context;
     std::string name(state.forceStringNoCtx(*args[0], pos, "while evaluating the first argument passed to builtins.toFile"));
     std::string contents(state.forceString(*args[1], context, pos, "while evaluating the second argument passed to builtins.toFile"));
 
     StorePathSet refs;
 
-    for (auto path : context) {
-        if (path.at(0) != '/')
+    for (auto c : context) {
+        if (auto p = std::get_if<NixStringContextElem::Opaque>(&c))
+            refs.insert(p->path);
+        else
             state.debugThrowLastTrace(EvalError({
                 .msg = hintfmt(
                     "in 'toFile': the file named '%1%' must not contain a reference "
                     "to a derivation but contains (%2%)",
-                    name, path),
+                    name, c.to_string()),
                 .errPos = state.positions[pos]
             }));
-        refs.insert(state.store->parseStorePath(path));
     }
 
     auto storePath = settings.readOnlyMode
@@ -2061,7 +2074,7 @@ static void addPath(
     FileIngestionMethod method,
     const std::optional<Hash> expectedHash,
     Value & v,
-    const PathSet & context)
+    const NixStringContext & context)
 {
     try {
         // FIXME: handle CA derivation outputs (where path needs to
@@ -2135,7 +2148,7 @@ static void addPath(
 
 static void prim_filterSource(EvalState & state, const PosIdx pos, Value * * args, Value & v)
 {
-    PathSet context;
+    NixStringContext context;
     Path path = state.coerceToPath(pos, *args[1], context, "while evaluating the second argument (the path to filter) passed to builtins.filterSource");
     state.forceFunction(*args[0], pos, "while evaluating the first argument passed to builtins.filterSource");
     addPath(state, pos, std::string(baseNameOf(path)), path, args[0], FileIngestionMethod::Recursive, std::nullopt, v, context);
@@ -2204,7 +2217,7 @@ static void prim_path(EvalState & state, const PosIdx pos, Value * * args, Value
     Value * filterFun = nullptr;
     auto method = FileIngestionMethod::Recursive;
     std::optional<Hash> expectedHash;
-    PathSet context;
+    NixStringContext context;
 
     for (auto & attr : *args[0]->attrs) {
         auto n = state.symbols[attr.name];
@@ -3538,7 +3551,7 @@ static RegisterPrimOp primop_lessThan({
    `"/nix/store/whatever..."'. */
 static void prim_toString(EvalState & state, const PosIdx pos, Value * * args, Value & v)
 {
-    PathSet context;
+    NixStringContext context;
     auto s = state.coerceToString(pos, *args[0], context,
             "while evaluating the first argument passed to builtins.toString",
             true, false);
@@ -3577,7 +3590,7 @@ static void prim_substring(EvalState & state, const PosIdx pos, Value * * args,
 {
     int start = state.forceInt(*args[0], pos, "while evaluating the first argument (the start offset) passed to builtins.substring");
     int len = state.forceInt(*args[1], pos, "while evaluating the second argument (the substring length) passed to builtins.substring");
-    PathSet context;
+    NixStringContext context;
     auto s = state.coerceToString(pos, *args[2], context, "while evaluating the third argument (the string) passed to builtins.substring");
 
     if (start < 0)
@@ -3611,7 +3624,7 @@ static RegisterPrimOp primop_substring({
 
 static void prim_stringLength(EvalState & state, const PosIdx pos, Value * * args, Value & v)
 {
-    PathSet context;
+    NixStringContext context;
     auto s = state.coerceToString(pos, *args[0], context, "while evaluating the argument passed to builtins.stringLength");
     v.mkInt(s->size());
 }
@@ -3637,7 +3650,7 @@ static void prim_hashString(EvalState & state, const PosIdx pos, Value * * args,
             .errPos = state.positions[pos]
         }));
 
-    PathSet context; // discarded
+    NixStringContext context; // discarded
     auto s = state.forceString(*args[1], context, pos, "while evaluating the second argument passed to builtins.hashString");
 
     v.mkString(hashString(*ht, s).to_string(Base16, false));
@@ -3683,7 +3696,7 @@ void prim_match(EvalState & state, const PosIdx pos, Value * * args, Value & v)
 
         auto regex = state.regexCache->get(re);
 
-        PathSet context;
+        NixStringContext context;
         const auto str = state.forceString(*args[1], context, pos, "while evaluating the second argument passed to builtins.match");
 
         std::cmatch match;
@@ -3763,7 +3776,7 @@ void prim_split(EvalState & state, const PosIdx pos, Value * * args, Value & v)
 
         auto regex = state.regexCache->get(re);
 
-        PathSet context;
+        NixStringContext context;
         const auto str = state.forceString(*args[1], context, pos, "while evaluating the second argument passed to builtins.split");
 
         auto begin = std::cregex_iterator(str.begin(), str.end(), regex);
@@ -3860,7 +3873,7 @@ static RegisterPrimOp primop_split({
 
 static void prim_concatStringsSep(EvalState & state, const PosIdx pos, Value * * args, Value & v)
 {
-    PathSet context;
+    NixStringContext context;
 
     auto sep = state.forceString(*args[0], context, pos, "while evaluating the first argument (the separator string) passed to builtins.concatStringsSep");
     state.forceList(*args[1], pos, "while evaluating the second argument (the list of strings to concat) passed to builtins.concatStringsSep");
@@ -3900,15 +3913,15 @@ static void prim_replaceStrings(EvalState & state, const PosIdx pos, Value * * a
     for (auto elem : args[0]->listItems())
         from.emplace_back(state.forceString(*elem, pos, "while evaluating one of the strings to replace passed to builtins.replaceStrings"));
 
-    std::vector<std::pair<std::string, PathSet>> to;
+    std::vector<std::pair<std::string, NixStringContext>> to;
     to.reserve(args[1]->listSize());
     for (auto elem : args[1]->listItems()) {
-        PathSet ctx;
+        NixStringContext ctx;
         auto s = state.forceString(*elem, ctx, pos, "while evaluating one of the replacement strings passed to builtins.replaceStrings");
         to.emplace_back(s, std::move(ctx));
     }
 
-    PathSet context;
+    NixStringContext context;
     auto s = state.forceString(*args[2], context, pos, "while evaluating the third argument passed to builtins.replaceStrings");
 
     std::string res;