Merge pull request #56 from DeterminateSystems/references-without-context

Improve lazy trees backward compatibility

.github/workflows/ci.yml

@@ -89,7 +89,15 @@ jobs:
                 | ".#hydraJobs.tests." + .')
 
   flake_regressions:
-    if: github.event_name == 'merge_group'
+    if: |
+      github.event_name == 'merge_group'
+      || (
+        github.event.pull_request.head.repo.full_name == 'DeterminateSystems/nix-src'
+        && (
+          (github.event.action == 'labeled' && github.event.label.name == 'flake-regression-test')
+          || (github.event.action != 'labeled' && contains(github.event.pull_request.labels.*.name, 'flake-regression-test'))
+        )
+      )
     needs: build_x86_64-linux
     runs-on: namespace-profile-x86-32cpu-64gb
     steps:
@@ -112,7 +120,15 @@ jobs:
       - run: nix build -L --out-link ./new-nix && PATH=$(pwd)/new-nix/bin:$PATH PARALLEL="-P 50%" flake-regressions/eval-all.sh
 
   flake_regressions_lazy:
-    if: github.event_name == 'merge_group'
+    if: |
+      github.event_name == 'merge_group'
+      || (
+        github.event.pull_request.head.repo.full_name == 'DeterminateSystems/nix-src'
+        && (
+          (github.event.action == 'labeled' && github.event.label.name == 'flake-regression-test')
+          || (github.event.action != 'labeled' && contains(github.event.pull_request.labels.*.name, 'flake-regression-test'))
+        )
+      )
     needs: build_x86_64-linux
     runs-on: namespace-profile-x86-32cpu-64gb
     steps:

src/libexpr-test-support/include/nix/expr/tests/value/context.hh

@@ -23,6 +23,11 @@ struct Arbitrary<NixStringContextElem::DrvDeep> {
     static Gen<NixStringContextElem::DrvDeep> arbitrary();
 };
 
+template<>
+struct Arbitrary<NixStringContextElem::Path> {
+    static Gen<NixStringContextElem::Path> arbitrary();
+};
+
 template<>
 struct Arbitrary<NixStringContextElem> {
     static Gen<NixStringContextElem> arbitrary();

src/libexpr-test-support/tests/value/context.cc

@@ -15,6 +15,15 @@ Gen<NixStringContextElem::DrvDeep> Arbitrary<NixStringContextElem::DrvDeep>::arb
     });
 }
 
+Gen<NixStringContextElem::Path> Arbitrary<NixStringContextElem::Path>::arbitrary()
+{
+    return gen::map(gen::arbitrary<StorePath>(), [](StorePath storePath) {
+        return NixStringContextElem::Path{
+            .storePath = storePath,
+        };
+    });
+}
+
 Gen<NixStringContextElem> Arbitrary<NixStringContextElem>::arbitrary()
 {
     return gen::mapcat(
@@ -30,6 +39,9 @@ Gen<NixStringContextElem> Arbitrary<NixStringContextElem>::arbitrary()
             case 2:
                 return gen::map(
                     gen::arbitrary<NixStringContextElem::Built>(), [](NixStringContextElem a) { return a; });
+            case 3:
+                return gen::map(
+                    gen::arbitrary<NixStringContextElem::Path>(), [](NixStringContextElem a) { return a; });
             default:
                 assert(false);
             }

src/libexpr/eval-cache.cc

@@ -618,18 +618,21 @@ string_t AttrCursor::getStringWithContext()
             if (auto s = std::get_if<string_t>(&cachedValue->second)) {
                 bool valid = true;
                 for (auto & c : s->second) {
-                    const StorePath & path = std::visit(overloaded {
-                        [&](const NixStringContextElem::DrvDeep & d) -> const StorePath & {
-                            return d.drvPath;
+                    const StorePath * path = std::visit(overloaded {
+                        [&](const NixStringContextElem::DrvDeep & d) -> const StorePath * {
+                            return &d.drvPath;
                         },
-                        [&](const NixStringContextElem::Built & b) -> const StorePath & {
-                            return b.drvPath->getBaseStorePath();
+                        [&](const NixStringContextElem::Built & b) -> const StorePath * {
+                            return &b.drvPath->getBaseStorePath();
                         },
-                        [&](const NixStringContextElem::Opaque & o) -> const StorePath & {
-                            return o.path;
+                        [&](const NixStringContextElem::Opaque & o) -> const StorePath * {
+                            return &o.path;
+                        },
+                        [&](const NixStringContextElem::Path & p) -> const StorePath * {
+                            return nullptr;
                         },
                     }, c.raw);
-                    if (!root->state.store->isValidPath(path)) {
+                    if (!path || !root->state.store->isValidPath(*path)) {
                         valid = false;
                         break;
                     }

src/libexpr/eval.cc

@@ -952,8 +952,8 @@ void EvalState::mkPos(Value & v, PosIdx p)
             // FIXME: only do this for virtual store paths?
             attrs.alloc(sFile).mkString(path->path.abs(),
                 {
-                    NixStringContextElem::Opaque{
-                        .path = store->toStorePath(path->path.abs()).first
+                    NixStringContextElem::Path{
+                        .storePath = store->toStorePath(path->path.abs()).first
                     }
                 });
         else
@@ -2078,7 +2078,7 @@ void ExprConcatStrings::eval(EvalState & state, Env & env, Value & v)
     else if (firstType == nFloat)
         v.mkFloat(nf);
     else if (firstType == nPath) {
-        if (!context.empty())
+        if (hasContext(context))
             state.error<EvalError>("a string that refers to a store path cannot be appended to a path").atPos(pos).withFrame(env, *this).debugThrow();
         v.mkPath(state.rootPath(CanonPath(str())));
     } else
@@ -2277,7 +2277,10 @@ std::string_view EvalState::forceStringNoCtx(Value & v, const PosIdx pos, std::s
 {
     auto s = forceString(v, pos, errorCtx);
     if (v.context()) {
-        error<EvalError>("the string '%1%' is not allowed to refer to a store path (such as '%2%')", v.string_view(), v.context()[0]).withTrace(pos, errorCtx).debugThrow();
+        NixStringContext context;
+        copyContext(v, context);
+        if (hasContext(context))
+            error<EvalError>("the string '%1%' is not allowed to refer to a store path (such as '%2%')", v.string_view(), v.context()[0]).withTrace(pos, errorCtx).debugThrow();
     }
     return s;
 }
@@ -2336,7 +2339,16 @@ BackedStringView EvalState::coerceToString(
               v.payload.path.path
             : copyToStore
             ? store->printStorePath(copyPathToStore(context, v.path()))
-            : std::string(v.path().path.abs());
+            : ({
+                auto path = v.path();
+                if (path.accessor == rootFS && store->isInStore(path.path.abs())) {
+                    context.insert(
+                        NixStringContextElem::Path{
+                            .storePath = store->toStorePath(path.path.abs()).first
+                        });
+                }
+                std::string(path.path.abs());
+              });
     }
 
     if (v.type() == nAttrs) {
@@ -2499,6 +2511,11 @@ std::pair<SingleDerivedPath, std::string_view> EvalState::coerceToSingleDerivedP
         [&](NixStringContextElem::Built && b) -> SingleDerivedPath {
             return std::move(b);
         },
+        [&](NixStringContextElem::Path && p) -> SingleDerivedPath {
+            error<EvalError>(
+                "string '%s' has no context",
+                s).withTrace(pos, errorCtx).debugThrow();
+        },
     }, ((NixStringContextElem &&) *context.begin()).raw);
     return {
         std::move(derivedPath),

src/libexpr/include/nix/expr/value/context.hh

@@ -54,10 +54,35 @@ struct NixStringContextElem {
      */
     using Built = SingleDerivedPath::Built;
 
+    /**
+     * A store path that will not result in a store reference when
+     * used in a derivation or toFile.
+     *
+     * When you apply `builtins.toString` to a path value representing
+     * a path in the Nix store (as is the case with flake inputs),
+     * historically you got a string without context
+     * (e.g. `/nix/store/...-source`). This is broken, since it allows
+     * you to pass a store path to a derivation/toFile without a
+     * proper store reference. This is especially a problem with lazy
+     * trees, since the store path is a virtual path that doesn't
+     * exist.
+     *
+     * For backwards compatibility, and to warn users about this
+     * unsafe use of `toString`, we keep track of such strings as a
+     * special type of context.
+     */
+    struct Path
+    {
+        StorePath storePath;
+
+        GENERATE_CMP(Path, me->storePath);
+    };
+
     using Raw = std::variant<
         Opaque,
         DrvDeep,
-        Built
+        Built,
+        Path
     >;
 
     Raw raw;
@@ -82,4 +107,10 @@ struct NixStringContextElem {
 
 typedef std::set<NixStringContextElem> NixStringContext;
 
+/**
+ * Returns false if `context` has no elements other than
+ * `NixStringContextElem::Path`.
+ */
+bool hasContext(const NixStringContext & context);
+
 }

src/libexpr/primops.cc

@@ -89,6 +89,9 @@ StringMap EvalState::realiseContext(const NixStringContext & context, StorePathS
                 if (maybePathsOut)
                     maybePathsOut->emplace(d.drvPath);
             },
+            [&](const NixStringContextElem::Path & p) {
+                // FIXME: do something?
+            },
         }, c.raw);
     }
 
@@ -1414,6 +1417,8 @@ static void derivationStrictInternal(
        derivation. */
     StringMap rewrites;
 
+    std::optional<std::string> drvS;
+
     for (auto & c : context) {
         std::visit(overloaded {
             /* Since this allows the builder to gain access to every
@@ -1438,6 +1443,17 @@ static void derivationStrictInternal(
             [&](const NixStringContextElem::Opaque & o) {
                 drv.inputSrcs.insert(state.devirtualize(o.path, &rewrites));
             },
+            [&](const NixStringContextElem::Path & p) {
+                if (!drvS) drvS = drv.unparse(*state.store, true);
+                if (drvS->find(p.storePath.to_string()) != drvS->npos) {
+                    auto devirtualized = state.devirtualize(p.storePath, &rewrites);
+                    warn(
+                        "Using 'builtins.derivation' to create a derivation named '%s' that references the store path '%s' without a proper context. "
+                        "The resulting derivation will not have a correct store reference, so this is unreliable and may stop working in the future.",
+                        drvName,
+                        state.store->printStorePath(devirtualized));
+                }
+            },
         }, c.raw);
     }
 
@@ -2346,10 +2362,21 @@ static void prim_toFile(EvalState & state, const PosIdx pos, Value * * args, Val
     std::string contents(state.forceString(*args[1], context, pos, "while evaluating the second argument passed to builtins.toFile"));
 
     StorePathSet refs;
+    StringMap rewrites;
 
     for (auto c : context) {
         if (auto p = std::get_if<NixStringContextElem::Opaque>(&c.raw))
             refs.insert(p->path);
+        else if (auto p = std::get_if<NixStringContextElem::Path>(&c.raw)) {
+            if (contents.find(p->storePath.to_string()) != contents.npos) {
+                auto devirtualized = state.devirtualize(p->storePath, &rewrites);
+                warn(
+                    "Using 'builtins.toFile' to create a file named '%s' that references the store path '%s' without a proper context. "
+                    "The resulting file will not have a correct store reference, so this is unreliable and may stop working in the future.",
+                    name,
+                    state.store->printStorePath(devirtualized));
+            }
+        }
         else
             state.error<EvalError>(
                 "files created by %1% may not reference derivations, but %2% references %3%",
@@ -2359,6 +2386,8 @@ static void prim_toFile(EvalState & state, const PosIdx pos, Value * * args, Val
             ).atPos(pos).debugThrow();
     }
 
+    contents = rewriteStrings(contents, rewrites);
+
     auto storePath = settings.readOnlyMode
         ? state.store->makeFixedOutputPathFromCA(name, TextInfo {
             .hash = hashString(HashAlgorithm::SHA256, contents),

src/libexpr/primops/context.cc

@@ -7,9 +7,15 @@ namespace nix {
 
 static void prim_unsafeDiscardStringContext(EvalState & state, const PosIdx pos, Value * * args, Value & v)
 {
-    NixStringContext context;
+    NixStringContext context, filtered;
+
     auto s = state.coerceToString(pos, *args[0], context, "while evaluating the argument passed to builtins.unsafeDiscardStringContext");
-    v.mkString(*s);
+
+    for (auto & c : context)
+        if (auto * p = std::get_if<NixStringContextElem::Path>(&c.raw))
+            filtered.insert(*p);
+
+    v.mkString(*s, filtered);
 }
 
 static RegisterPrimOp primop_unsafeDiscardStringContext({
@@ -21,12 +27,19 @@ static RegisterPrimOp primop_unsafeDiscardStringContext({
     .fun = prim_unsafeDiscardStringContext,
 });
 
+bool hasContext(const NixStringContext & context)
+{
+    for (auto & c : context)
+        if (!std::get_if<NixStringContextElem::Path>(&c.raw))
+            return true;
+    return false;
+}
 
 static void prim_hasContext(EvalState & state, const PosIdx pos, Value * * args, Value & v)
 {
     NixStringContext context;
     state.forceString(*args[0], context, pos, "while evaluating the argument passed to builtins.hasContext");
-    v.mkBool(!context.empty());
+    v.mkBool(hasContext(context));
 }
 
 static RegisterPrimOp primop_hasContext({
@@ -103,7 +116,7 @@ static void prim_addDrvOutputDependencies(EvalState & state, const PosIdx pos, V
     NixStringContext context;
     auto s = state.coerceToString(pos, *args[0], context, "while evaluating the argument passed to builtins.addDrvOutputDependencies");
 
-	auto contextSize = context.size();
+    auto contextSize = context.size();
     if (contextSize != 1) {
         state.error<EvalError>(
             "context of string '%s' must have exactly one element, but has %d",
@@ -136,6 +149,11 @@ static void prim_addDrvOutputDependencies(EvalState & state, const PosIdx pos, V
                  above does not make much sense. */
                 return std::move(c);
             },
+            [&](const NixStringContextElem::Path & p) -> NixStringContextElem::DrvDeep {
+                state.error<EvalError>(
+                    "`addDrvOutputDependencies` does not work on a string without context"
+                ).atPos(pos).debugThrow();
+            },
         }, context.begin()->raw) }),
     };
 
@@ -206,6 +224,8 @@ static void prim_getContext(EvalState & state, const PosIdx pos, Value * * args,
             [&](NixStringContextElem::Opaque && o) {
                 contextInfos[std::move(o.path)].path = true;
             },
+            [&](NixStringContextElem::Path && p) {
+            },
         }, ((NixStringContextElem &&) i).raw);
     }
 

src/libexpr/print.cc

@@ -249,7 +249,11 @@ private:
 
     void printString(Value & v)
     {
-        printLiteralString(output, v.string_view(), options.maxStringLength, options.ansiColors);
+        NixStringContext context;
+        copyContext(v, context);
+        std::ostringstream s;
+        printLiteralString(s, v.string_view(), options.maxStringLength, options.ansiColors);
+        output << state.devirtualize(s.str(), context);
     }
 
     void printPath(Value & v)

src/libexpr/value-to-json.cc

@@ -7,9 +7,10 @@
 #include <iomanip>
 #include <nlohmann/json.hpp>
 
-
 namespace nix {
+
 using json = nlohmann::json;
+
 json printValueAsJSON(EvalState & state, bool strict,
     Value & v, const PosIdx pos, NixStringContext & context, bool copyToStore)
 {
@@ -31,9 +32,7 @@ json printValueAsJSON(EvalState & state, bool strict,
 
         case nString:
             copyContext(v, context);
-            // FIXME: only use the context from `v`.
-            // FIXME: make devirtualization configurable?
-            out = state.devirtualize(v.c_str(), context);
+            out = v.c_str();
             break;
 
         case nPath:

src/libexpr/value/context.cc

@@ -57,6 +57,11 @@ NixStringContextElem NixStringContextElem::parse(
             .drvPath = StorePath { s.substr(1) },
         };
     }
+    case '@': {
+        return NixStringContextElem::Path {
+            .storePath = StorePath { s.substr(1) },
+        };
+    }
     default: {
         // Ensure no '!'
         if (s.find("!") != std::string_view::npos) {
@@ -100,6 +105,10 @@ std::string NixStringContextElem::to_string() const
             res += '=';
             res += d.drvPath.to_string();
         },
+        [&](const NixStringContextElem::Path & p) {
+            res += '@';
+            res += p.storePath.to_string();
+        },
     }, raw);
 
     return res;

src/nix/app.cc

@@ -92,6 +92,9 @@ UnresolvedApp InstallableValue::toApp(EvalState & state)
                         .path = o.path,
                     };
                 },
+                [&](const NixStringContextElem::Path & p) -> DerivedPath {
+                    throw Error("'program' attribute of an 'app' output cannot have no context");
+                },
             }, c.raw));
         }
 

src/nix/eval.cc

@@ -122,7 +122,10 @@ struct CmdEval : MixJSON, InstallableValueCommand, MixReadOnlyOption
         }
 
         else if (json) {
-            logger->cout("%s", printValueAsJSON(*state, true, *v, pos, context, false));
+            logger->cout("%s",
+                state->devirtualize(
+                    printValueAsJSON(*state, true, *v, pos, context, false).dump(),
+                    context));
         }
 
         else {