DerivationOutput: Remove outputKnown state

Now that `DerivationGoal::checkPathValidity` is legible, we can see that
it only sets `outputKnown`, and doesn't read it. Likewise, with
co-routines, we don't have tiny scopes that make local variables
difficult. Between these two things, we can simply have
`checkPathValidity` return what it finds, rather than mutate some state,
and update everyting to use local variables.

The same transformation could probably be done to the other derivation
goal types (which currently, unfortunately, contain their own
`checkPathValidity`s, though they are diverging, and we hope and believe
that they continue to diverge).

src/libstore/build/derivation-goal.cc

@@ -34,7 +34,6 @@ DerivationGoal::DerivationGoal(
     , drvPath(drvPath)
     , wantedOutput(wantedOutput)
     , outputHash{Hash::dummy} // will be updated
-    , outputKnown{}
     , buildMode(buildMode)
 {
     this->drv = std::make_unique<Derivation>(drv);
@@ -98,7 +97,7 @@ Goal::Co DerivationGoal::haveDerivation()
             /* In checking mode, the builder will not register any outputs.
                So we want to make sure the ones that we wanted to check are
                properly there. */
-            buildResult.builtOutputs = assertPathValidity();
+            buildResult.builtOutputs = {{wantedOutput, assertPathValidity()}};
         }
 
         for (auto it = buildResult.builtOutputs.begin(); it != buildResult.builtOutputs.end();) {
@@ -125,19 +124,8 @@ Goal::Co DerivationGoal::haveDerivation()
         if (impure)
             experimentalFeatureSettings.require(Xp::ImpureDerivations);
 
-        auto outputHashes = staticOutputHashes(worker.evalStore, *drv);
+        if (auto * mOutputHash = get(staticOutputHashes(worker.evalStore, *drv), wantedOutput)) {
-        if (auto * mOutputHash = get(outputHashes, wantedOutput)) {
             outputHash = *mOutputHash;
-
-            /* TODO we might want to also allow randomizing the paths
-               for regular CA derivations, e.g. for sake of checking
-               determinism. */
-            if (impure) {
-                outputKnown = InitialOutputStatus{
-                    .path = StorePath::random(outputPathName(drv->name, wantedOutput)),
-                    .status = PathStatus::Absent,
-                };
-            }
         }
 
         if (impure) {
@@ -147,14 +135,12 @@ Goal::Co DerivationGoal::haveDerivation()
         }
     }
 
-    {
+    /* Check what outputs paths are not already valid. */
-        /* Check what outputs paths are not already valid. */
+    auto checkResult = checkPathValidity();
-        auto [allValid, validOutputs] = checkPathValidity();
 
-        /* If they are all valid, then we're done. */
+    /* If they are all valid, then we're done. */
-        if (allValid && buildMode == bmNormal) {
+    if (checkResult && checkResult->second == PathStatus::Valid && buildMode == bmNormal) {
-            co_return done(BuildResult::AlreadyValid, std::move(validOutputs));
+        co_return done(BuildResult::AlreadyValid, {{wantedOutput, checkResult->first}});
-        }
     }
 
     Goals waitees;
@@ -163,13 +149,13 @@ Goal::Co DerivationGoal::haveDerivation()
        through substitutes.  If that doesn't work, we'll build
        them. */
     if (settings.useSubstitutes && drvOptions.substitutesAllowed()) {
-        if (!outputKnown)
+        if (!checkResult)
             waitees.insert(upcast_goal(worker.makeDrvOutputSubstitutionGoal(
                 DrvOutput{outputHash, wantedOutput}, buildMode == bmRepair ? Repair : NoRepair)));
         else {
             auto * cap = getDerivationCA(*drv);
             waitees.insert(upcast_goal(worker.makePathSubstitutionGoal(
-                outputKnown->path,
+                checkResult->first.outPath,
                 buildMode == bmRepair ? Repair : NoRepair,
                 cap ? std::optional{*cap} : std::nullopt)));
         }
@@ -192,10 +178,12 @@ Goal::Co DerivationGoal::haveDerivation()
 
     nrFailed = nrNoSubstituters = 0;
 
-    auto [allValid, validOutputs] = checkPathValidity();
+    checkResult = checkPathValidity();
+
+    bool allValid = checkResult && checkResult->second == PathStatus::Valid;
 
     if (buildMode == bmNormal && allValid) {
-        co_return done(BuildResult::Substituted, std::move(validOutputs));
+        co_return done(BuildResult::Substituted, {{wantedOutput, checkResult->first}});
     }
     if (buildMode == bmRepair && allValid) {
         co_return repairClosure();
@@ -300,15 +288,13 @@ Goal::Co DerivationGoal::repairClosure()
                 "some paths in the output closure of derivation '%s' could not be repaired",
                 worker.store.printStorePath(drvPath));
     }
-    co_return done(BuildResult::AlreadyValid, assertPathValidity());
+    co_return done(BuildResult::AlreadyValid, {{wantedOutput, assertPathValidity()}});
 }
 
-std::pair<bool, SingleDrvOutputs> DerivationGoal::checkPathValidity()
+std::optional<std::pair<Realisation, PathStatus>> DerivationGoal::checkPathValidity()
 {
     if (drv->type().isImpure())
-        return {false, {}};
+        return std::nullopt;
-
-    SingleDrvOutputs validOutputs;
 
     auto drvOutput = DrvOutput{outputHash, wantedOutput};
 
@@ -335,35 +321,29 @@ std::pair<bool, SingleDrvOutputs> DerivationGoal::checkPathValidity()
     if (mRealisation) {
         auto & outputPath = mRealisation->outPath;
         bool checkHash = buildMode == bmRepair;
-        outputKnown = {
+        PathStatus status = !worker.store.isValidPath(outputPath)               ? PathStatus::Absent
-            .path = outputPath,
+                            : !checkHash || worker.pathContentsGood(outputPath) ? PathStatus::Valid
-            .status = !worker.store.isValidPath(outputPath)               ? PathStatus::Absent
+                                                                                : PathStatus::Corrupt;
-                      : !checkHash || worker.pathContentsGood(outputPath) ? PathStatus::Valid
-                                                                          : PathStatus::Corrupt,
-        };
 
-        if (outputKnown->isValid()) {
+        if (experimentalFeatureSettings.isEnabled(Xp::CaDerivations) && status == PathStatus::Valid) {
-            if (experimentalFeatureSettings.isEnabled(Xp::CaDerivations)) {
+            // We know the output because it's a static output of the
-                // We know the output because it's a static output of the
+            // derivation, and the output path is valid, but we don't have
-                // derivation, and the output path is valid, but we don't have
+            // its realisation stored (probably because it has been built
-                // its realisation stored (probably because it has been built
+            // without the `ca-derivations` experimental flag).
-                // without the `ca-derivations` experimental flag).
+            worker.store.registerDrvOutput(*mRealisation);
-                worker.store.registerDrvOutput(*mRealisation);
-            }
-
-            validOutputs.emplace(wantedOutput, *mRealisation);
         }
-    }
 
-    return {outputKnown && outputKnown->isValid(), validOutputs};
+        return {{*mRealisation, status}};
+    } else
+        return std::nullopt;
 }
 
-SingleDrvOutputs DerivationGoal::assertPathValidity()
+Realisation DerivationGoal::assertPathValidity()
 {
-    auto [allValid, validOutputs] = checkPathValidity();
+    auto checkResult = checkPathValidity();
-    if (!allValid)
+    if (!(checkResult && checkResult->second == PathStatus::Valid))
         throw Error("some outputs are unexpectedly invalid");
-    return validOutputs;
+    return checkResult->first;
 }
 
 Goal::Done DerivationGoal::done(BuildResult::Status status, SingleDrvOutputs builtOutputs, std::optional<Error> ex)

src/libstore/include/nix/store/build/derivation-goal.hh

@@ -75,7 +75,6 @@ private:
      */
 
     Hash outputHash;
-    std::optional<InitialOutputStatus> outputKnown;
 
     BuildMode buildMode;
 
@@ -87,18 +86,19 @@ private:
     Co haveDerivation();
 
     /**
-     * Update 'initialOutput' to determine the current status of the
+     * Return `std::nullopt` if the output is unknown, e.g. un unbuilt
-     * outputs of the derivation. Also returns a Boolean denoting
+     * floating content-addressing derivation. Otherwise, returns a pair
-     * whether all outputs are valid and non-corrupt, and a
+     * of a `Realisation`, containing among other things the store path
-     * 'SingleDrvOutputs' structure containing the valid outputs.
+     * of the wanted output, and a `PathStatus` with the
+     * current status of that output.
      */
-    std::pair<bool, SingleDrvOutputs> checkPathValidity();
+    std::optional<std::pair<Realisation, PathStatus>> checkPathValidity();
 
     /**
      * Aborts if any output is not valid or corrupt, and otherwise
-     * returns a 'SingleDrvOutputs' structure containing all outputs.
+     * returns a 'Realisation' for the wanted output.
      */
-    SingleDrvOutputs assertPathValidity();
+    Realisation assertPathValidity();
 
     Co repairClosure();