Dedup some derivation initialization logic, and test

`nix derivation add`, and its C API counterpart, now works a bit closer
to `builtins.derivation` in that they don't require the user to fill-in
input addressed paths correctly ahead of time.

The logic for this is carefully deduplicated, between all 3 entry
points, and also between the existing `checkInvariants` function. There
are some more functional tests, and there are also many more unit tests.

src/libexpr/primops.cc

@@ -1768,28 +1768,7 @@ static void derivationStrictInternal(EvalState & state, std::string_view drvName
             drv.outputs.insert_or_assign(i, DerivationOutput::Deferred{});
         }
 
-        auto hashModulo = hashDerivationModulo(*state.store, Derivation(drv), true);
+        drv.fillInOutputPaths(*state.store);
-        switch (hashModulo.kind) {
-        case DrvHash::Kind::Regular:
-            for (auto & i : outputs) {
-                auto h = get(hashModulo.hashes, i);
-                if (!h)
-                    state.error<AssertionError>("derivation produced no hash for output '%s'", i).atPos(v).debugThrow();
-                auto outPath = state.store->makeOutputPath(i, *h, drvName);
-                drv.env[i] = state.store->printStorePath(outPath);
-                drv.outputs.insert_or_assign(
-                    i,
-                    DerivationOutput::InputAddressed{
-                        .path = std::move(outPath),
-                    });
-            }
-            break;
-            ;
-        case DrvHash::Kind::Deferred:
-            for (auto & i : outputs) {
-                drv.outputs.insert_or_assign(i, DerivationOutput::Deferred{});
-            }
-        }
     }
 
     /* Write the resulting term into the Nix store directory. */

src/libstore-c/nix_api_store.cc

@@ -223,13 +223,7 @@ nix_derivation * nix_derivation_from_json(nix_c_context * context, Store * store
     if (context)
         context->last_err_code = NIX_OK;
     try {
-        auto drv = static_cast<nix::Derivation>(nlohmann::json::parse(json));
+        return new nix_derivation{nix::Derivation::parseJsonAndValidate(*store->ptr, nlohmann::json::parse(json))};
-
-        auto drvPath = nix::writeDerivation(*store->ptr, drv, nix::NoRepair, /* read only */ true);
-
-        drv.checkInvariants(*store->ptr, drvPath);
-
-        return new nix_derivation{drv};
     }
     NIXC_CATCH_ERRS_NULL
 }

src/libstore-tests/data/derivation/invariants/bad-depends-on-drv-pre.json

@@ -0,0 +1,27 @@
+{
+  "args": [],
+  "builder": "/bin/sh",
+  "env": {
+    "__doc": "InputAddressed throws when should be deferred",
+    "out": ""
+  },
+  "inputs": {
+    "drvs": {
+      "lg4c4b8r9hlczwprl6kgnzfd9mc1xmkk-dependency.drv": {
+        "dynamicOutputs": {},
+        "outputs": [
+          "out"
+        ]
+      }
+    },
+    "srcs": []
+  },
+  "name": "depends-on-drv",
+  "outputs": {
+    "out": {
+      "path": "c015dhfh5l0lp6wxyvdn7bmwhbbr6hr9-wrong-name"
+    }
+  },
+  "system": "x86_64-linux",
+  "version": 4
+}

src/libstore-tests/data/derivation/invariants/bad-env-var.json

@@ -0,0 +1,18 @@
+{
+  "args": [],
+  "builder": "/bin/sh",
+  "env": {
+    "__doc": "Wrong env var value throws error",
+    "out": "/nix/store/c015dhfh5l0lp6wxyvdn7bmwhbbr6hr9-wrong-name"
+  },
+  "inputs": {
+    "drvs": {},
+    "srcs": []
+  },
+  "name": "bad-env-var",
+  "outputs": {
+    "out": {}
+  },
+  "system": "x86_64-linux",
+  "version": 4
+}

src/libstore-tests/data/derivation/invariants/bad-path.json

@@ -0,0 +1,20 @@
+{
+  "args": [],
+  "builder": "/bin/sh",
+  "env": {
+    "__doc": "Wrong InputAddressed path throws error",
+    "out": "/nix/store/c015dhfh5l0lp6wxyvdn7bmwhbbr6hr9-wrong-name"
+  },
+  "inputs": {
+    "drvs": {},
+    "srcs": []
+  },
+  "name": "bad-path",
+  "outputs": {
+    "out": {
+      "path": "c015dhfh5l0lp6wxyvdn7bmwhbbr6hr9-wrong-name"
+    }
+  },
+  "system": "x86_64-linux",
+  "version": 4
+}

src/libstore-tests/data/derivation/invariants/depends-on-drv-pre.json

@@ -0,0 +1,25 @@
+{
+  "args": [],
+  "builder": "/bin/sh",
+  "env": {
+    "__doc": "Deferred stays deferred with CA dependencies",
+    "out": ""
+  },
+  "inputs": {
+    "drvs": {
+      "lg4c4b8r9hlczwprl6kgnzfd9mc1xmkk-dependency.drv": {
+        "dynamicOutputs": {},
+        "outputs": [
+          "out"
+        ]
+      }
+    },
+    "srcs": []
+  },
+  "name": "depends-on-drv",
+  "outputs": {
+    "out": {}
+  },
+  "system": "x86_64-linux",
+  "version": 4
+}

src/libstore-tests/data/derivation/invariants/filled-in-deferred-empty-env-var-post.json

@@ -0,0 +1,20 @@
+{
+  "args": [],
+  "builder": "/bin/sh",
+  "env": {
+    "__doc": "Fill in deferred output with empty env var",
+    "out": "/nix/store/bilpz1nq8qi9r3bzsp72n34yjgqg43ws-filled-in-deferred-empty-env-var"
+  },
+  "inputs": {
+    "drvs": {},
+    "srcs": []
+  },
+  "name": "filled-in-deferred-empty-env-var",
+  "outputs": {
+    "out": {
+      "path": "bilpz1nq8qi9r3bzsp72n34yjgqg43ws-filled-in-deferred-empty-env-var"
+    }
+  },
+  "system": "x86_64-linux",
+  "version": 4
+}

src/libstore-tests/data/derivation/invariants/filled-in-deferred-empty-env-var-pre.json

@@ -0,0 +1,18 @@
+{
+  "args": [],
+  "builder": "/bin/sh",
+  "env": {
+    "__doc": "Fill in deferred output with empty env var",
+    "out": ""
+  },
+  "inputs": {
+    "drvs": {},
+    "srcs": []
+  },
+  "name": "filled-in-deferred-empty-env-var",
+  "outputs": {
+    "out": {}
+  },
+  "system": "x86_64-linux",
+  "version": 4
+}

src/libstore-tests/data/derivation/invariants/filled-in-deferred-no-env-var-post.json

@@ -0,0 +1,20 @@
+{
+  "args": [],
+  "builder": "/bin/sh",
+  "env": {
+    "__doc": "Fill in deferred with missing env var",
+    "out": "/nix/store/wpk9qrgg77fyswhailap0gicgw98izx9-filled-in-deferred-no-env-var"
+  },
+  "inputs": {
+    "drvs": {},
+    "srcs": []
+  },
+  "name": "filled-in-deferred-no-env-var",
+  "outputs": {
+    "out": {
+      "path": "wpk9qrgg77fyswhailap0gicgw98izx9-filled-in-deferred-no-env-var"
+    }
+  },
+  "system": "x86_64-linux",
+  "version": 4
+}

src/libstore-tests/data/derivation/invariants/filled-in-deferred-no-env-var-pre.json

@@ -0,0 +1,17 @@
+{
+  "args": [],
+  "builder": "/bin/sh",
+  "env": {
+    "__doc": "Fill in deferred with missing env var"
+  },
+  "inputs": {
+    "drvs": {},
+    "srcs": []
+  },
+  "name": "filled-in-deferred-no-env-var",
+  "outputs": {
+    "out": {}
+  },
+  "system": "x86_64-linux",
+  "version": 4
+}

src/libstore-tests/data/derivation/invariants/filled-in-idempotent.json

@@ -0,0 +1,20 @@
+{
+  "args": [],
+  "builder": "/bin/sh",
+  "env": {
+    "__doc": "Correct path stays unchanged",
+    "out": "/nix/store/w4bk7hpyxzgy2gx8fsa8f952435pll3i-filled-in-already"
+  },
+  "inputs": {
+    "drvs": {},
+    "srcs": []
+  },
+  "name": "filled-in-already",
+  "outputs": {
+    "out": {
+      "path": "w4bk7hpyxzgy2gx8fsa8f952435pll3i-filled-in-already"
+    }
+  },
+  "system": "x86_64-linux",
+  "version": 4
+}

src/libstore-tests/derivation/external-formats.cc

@@ -1,57 +1,14 @@
 #include <nlohmann/json.hpp>
 #include <gtest/gtest.h>
 
-#include "nix/util/experimental-features.hh"
 #include "nix/store/derivations.hh"
-
+#include "derivation/test-support.hh"
-#include "nix/store/tests/libstore.hh"
 #include "nix/util/tests/json-characterization.hh"
 
 namespace nix {
 
 using nlohmann::json;
 
-class DerivationTest : public virtual CharacterizationTest, public LibStoreTest
-{
-    std::filesystem::path unitTestData = getUnitTestData() / "derivation";
-
-public:
-    std::filesystem::path goldenMaster(std::string_view testStem) const override
-    {
-        return unitTestData / testStem;
-    }
-
-    /**
-     * We set these in tests rather than the regular globals so we don't have
-     * to worry about race conditions if the tests run concurrently.
-     */
-    ExperimentalFeatureSettings mockXpSettings;
-};
-
-class CaDerivationTest : public DerivationTest
-{
-    void SetUp() override
-    {
-        mockXpSettings.set("experimental-features", "ca-derivations");
-    }
-};
-
-class DynDerivationTest : public DerivationTest
-{
-    void SetUp() override
-    {
-        mockXpSettings.set("experimental-features", "dynamic-derivations ca-derivations");
-    }
-};
-
-class ImpureDerivationTest : public DerivationTest
-{
-    void SetUp() override
-    {
-        mockXpSettings.set("experimental-features", "impure-derivations");
-    }
-};
-
 TEST_F(DerivationTest, BadATerm_version)
 {
     ASSERT_THROW(

src/libstore-tests/derivation/invariants.cc

@@ -0,0 +1,264 @@
+#include <gtest/gtest.h>
+#include <nlohmann/json.hpp>
+
+#include "nix/store/derivations.hh"
+#include "nix/store/tests/libstore.hh"
+#include "nix/store/dummy-store-impl.hh"
+#include "nix/util/tests/json-characterization.hh"
+
+#include "derivation/test-support.hh"
+
+namespace nix {
+
+class FillInOutputPathsTest : public LibStoreTest, public JsonCharacterizationTest<Derivation>
+{
+    std::filesystem::path unitTestData = getUnitTestData() / "derivation" / "invariants";
+
+protected:
+    FillInOutputPathsTest()
+        : LibStoreTest([]() {
+            auto config = make_ref<DummyStoreConfig>(DummyStoreConfig::Params{});
+            config->readOnly = false;
+            return config->openDummyStore();
+        }())
+    {
+    }
+
+    /**
+     * Create a CA floating output derivation and write it to the store.
+     * This is useful for creating dependencies that will cause downstream
+     * derivations to remain deferred.
+     */
+    StorePath makeCAFloatingDependency(std::string_view name)
+    {
+        Derivation depDrv;
+        depDrv.name = name;
+        depDrv.platform = "x86_64-linux";
+        depDrv.builder = "/bin/sh";
+        depDrv.outputs = {
+            {
+                "out",
+                // will ensure that downstream is deferred
+                DerivationOutput{DerivationOutput::CAFloating{
+                    .method = ContentAddressMethod::Raw::NixArchive,
+                    .hashAlgo = HashAlgorithm::SHA256,
+                }},
+            },
+        };
+        depDrv.env = {{"out", ""}};
+
+        // Fill in the dependency derivation's output paths
+        depDrv.fillInOutputPaths(*store);
+
+        // Write the dependency to the store
+        return writeDerivation(*store, depDrv, NoRepair);
+    }
+
+public:
+    std::filesystem::path goldenMaster(std::string_view testStem) const override
+    {
+        return unitTestData / testStem;
+    }
+};
+
+TEST_F(FillInOutputPathsTest, fillsDeferredOutputs_emptyStringEnvVar)
+{
+    using nlohmann::json;
+
+    // Before: Derivation with deferred output
+    Derivation drv;
+    drv.name = "filled-in-deferred-empty-env-var";
+    drv.platform = "x86_64-linux";
+    drv.builder = "/bin/sh";
+    drv.outputs = {
+        {"out", DerivationOutput{DerivationOutput::Deferred{}}},
+    };
+    drv.env = {{"__doc", "Fill in deferred output with empty env var"}, {"out", ""}};
+
+    // Serialize before state
+    checkpointJson("filled-in-deferred-empty-env-var-pre", drv);
+
+    drv.fillInOutputPaths(*store);
+
+    // Serialize after state
+    checkpointJson("filled-in-deferred-empty-env-var-post", drv);
+
+    // After: Should have been converted to InputAddressed
+    auto * outputP = std::get_if<DerivationOutput::InputAddressed>(&drv.outputs.at("out").raw);
+    ASSERT_TRUE(outputP);
+    auto & output = *outputP;
+
+    // Environment variable should be filled in
+    EXPECT_EQ(drv.env.at("out"), store->printStorePath(output.path));
+}
+
+TEST_F(FillInOutputPathsTest, fillsDeferredOutputs_empty_string_var)
+{
+    using nlohmann::json;
+
+    // Before: Derivation with deferred output
+    Derivation drv;
+    drv.name = "filled-in-deferred-no-env-var";
+    drv.platform = "x86_64-linux";
+    drv.builder = "/bin/sh";
+    drv.outputs = {
+        {"out", DerivationOutput{DerivationOutput::Deferred{}}},
+    };
+    drv.env = {
+        {"__doc", "Fill in deferred with missing env var"},
+    };
+
+    // Serialize before state
+    checkpointJson("filled-in-deferred-no-env-var-pre", drv);
+
+    drv.fillInOutputPaths(*store);
+
+    // Serialize after state
+    checkpointJson("filled-in-deferred-no-env-var-post", drv);
+
+    // After: Should have been converted to InputAddressed
+    auto * outputP = std::get_if<DerivationOutput::InputAddressed>(&drv.outputs.at("out").raw);
+    ASSERT_TRUE(outputP);
+    auto & output = *outputP;
+
+    // Environment variable should be filled in
+    EXPECT_EQ(drv.env.at("out"), store->printStorePath(output.path));
+}
+
+TEST_F(FillInOutputPathsTest, preservesInputAddressedOutputs)
+{
+    auto expectedPath = StorePath{"w4bk7hpyxzgy2gx8fsa8f952435pll3i-filled-in-already"};
+
+    Derivation drv;
+    drv.name = "filled-in-already";
+    drv.platform = "x86_64-linux";
+    drv.builder = "/bin/sh";
+    drv.outputs = {
+        {"out", DerivationOutput{DerivationOutput::InputAddressed{.path = expectedPath}}},
+    };
+    drv.env = {
+        {"__doc", "Correct path stays unchanged"},
+        {"out", store->printStorePath(expectedPath)},
+    };
+
+    // Serialize before state
+    checkpointJson("filled-in-idempotent", drv);
+
+    auto drvBefore = drv;
+
+    drv.fillInOutputPaths(*store);
+
+    // Should still be no change
+    EXPECT_EQ(drv, drvBefore);
+}
+
+TEST_F(FillInOutputPathsTest, throwsOnIncorrectInputAddressedPath)
+{
+    auto wrongPath = StorePath{"c015dhfh5l0lp6wxyvdn7bmwhbbr6hr9-wrong-name"};
+
+    Derivation drv;
+    drv.name = "bad-path";
+    drv.platform = "x86_64-linux";
+    drv.builder = "/bin/sh";
+    drv.outputs = {
+        {"out", DerivationOutput{DerivationOutput::InputAddressed{.path = wrongPath}}},
+    };
+    drv.env = {
+        {"__doc", "Wrong InputAddressed path throws error"},
+        {"out", store->printStorePath(wrongPath)},
+    };
+
+    // Serialize before state
+    checkpointJson("bad-path", drv);
+
+    ASSERT_THROW(drv.fillInOutputPaths(*store), Error);
+}
+
+TEST_F(FillInOutputPathsTest, throwsOnIncorrectEnvVar)
+{
+    auto wrongPath = StorePath{"c015dhfh5l0lp6wxyvdn7bmwhbbr6hr9-wrong-name"};
+
+    Derivation drv;
+    drv.name = "bad-env-var";
+    drv.platform = "x86_64-linux";
+    drv.builder = "/bin/sh";
+    drv.outputs = {
+        {"out", DerivationOutput{DerivationOutput::Deferred{}}},
+    };
+    drv.env = {
+        {"__doc", "Wrong env var value throws error"},
+        {"out", store->printStorePath(wrongPath)},
+    };
+
+    // Serialize before state
+    checkpointJson("bad-env-var", drv);
+
+    ASSERT_THROW(drv.fillInOutputPaths(*store), Error);
+}
+
+TEST_F(FillInOutputPathsTest, preservesDeferredWithInputDrvs)
+{
+    using nlohmann::json;
+
+    // Create a CA floating dependency derivation
+    auto depDrvPath = makeCAFloatingDependency("dependency");
+
+    // Create a derivation that depends on the dependency
+    Derivation drv;
+    drv.name = "depends-on-drv";
+    drv.platform = "x86_64-linux";
+    drv.builder = "/bin/sh";
+    drv.outputs = {
+        {"out", DerivationOutput{DerivationOutput::Deferred{}}},
+    };
+    drv.env = {
+        {"__doc", "Deferred stays deferred with CA dependencies"},
+        {"out", ""},
+    };
+    // Add the real input derivation dependency
+    drv.inputDrvs = {.map = {{depDrvPath, {.value = {"out"}}}}};
+
+    // Serialize before state
+    checkpointJson("depends-on-drv-pre", drv);
+
+    auto drvBefore = drv;
+
+    // Apply fillInOutputPaths
+    drv.fillInOutputPaths(*store);
+
+    // Derivation should be unchanged
+    EXPECT_EQ(drv, drvBefore);
+}
+
+TEST_F(FillInOutputPathsTest, throwsOnPatWhenShouldBeDeffered)
+{
+    using nlohmann::json;
+
+    // Create a CA floating dependency derivation
+    auto depDrvPath = makeCAFloatingDependency("dependency");
+
+    auto wrongPath = StorePath{"c015dhfh5l0lp6wxyvdn7bmwhbbr6hr9-wrong-name"};
+
+    // Create a derivation that depends on the dependency
+    Derivation drv;
+    drv.name = "depends-on-drv";
+    drv.platform = "x86_64-linux";
+    drv.builder = "/bin/sh";
+    drv.outputs = {
+        {"out", DerivationOutput{DerivationOutput::InputAddressed{.path = wrongPath}}},
+    };
+    drv.env = {
+        {"__doc", "InputAddressed throws when should be deferred"},
+        {"out", ""},
+    };
+    // Add the real input derivation dependency
+    drv.inputDrvs = {.map = {{depDrvPath, {.value = {"out"}}}}};
+
+    // Serialize before state
+    checkpointJson("bad-depends-on-drv-pre", drv);
+
+    // Apply fillInOutputPaths
+    ASSERT_THROW(drv.fillInOutputPaths(*store), Error);
+}
+
+} // namespace nix

src/libstore-tests/derivation/test-support.hh

@@ -0,0 +1,52 @@
+#pragma once
+
+#include <gtest/gtest.h>
+
+#include "nix/util/experimental-features.hh"
+#include "nix/store/tests/libstore.hh"
+#include "nix/util/tests/characterization.hh"
+
+namespace nix {
+
+class DerivationTest : public virtual CharacterizationTest, public LibStoreTest
+{
+    std::filesystem::path unitTestData = getUnitTestData() / "derivation";
+
+public:
+    std::filesystem::path goldenMaster(std::string_view testStem) const override
+    {
+        return unitTestData / testStem;
+    }
+
+    /**
+     * We set these in tests rather than the regular globals so we don't have
+     * to worry about race conditions if the tests run concurrently.
+     */
+    ExperimentalFeatureSettings mockXpSettings;
+};
+
+class CaDerivationTest : public DerivationTest
+{
+    void SetUp() override
+    {
+        mockXpSettings.set("experimental-features", "ca-derivations");
+    }
+};
+
+class DynDerivationTest : public DerivationTest
+{
+    void SetUp() override
+    {
+        mockXpSettings.set("experimental-features", "dynamic-derivations ca-derivations");
+    }
+};
+
+class ImpureDerivationTest : public DerivationTest
+{
+    void SetUp() override
+    {
+        mockXpSettings.set("experimental-features", "impure-derivations");
+    }
+};
+
+} // namespace nix

src/libstore-tests/meson.build

@@ -58,7 +58,8 @@ sources = files(
   'common-protocol.cc',
   'content-address.cc',
   'derivation-advanced-attrs.cc',
-  'derivation.cc',
+  'derivation/external-formats.cc',
+  'derivation/invariants.cc',
   'derived-path.cc',
   'downstream-placeholder.cc',
   'dummy-store.cc',

src/libstore/derivations.cc

@@ -1210,6 +1210,100 @@ std::optional<BasicDerivation> Derivation::tryResolve(
     return resolved;
 }
 
+/**
+ * Process `InputAddressed`, `Deferred`, and `CAFixed` outputs.
+ *
+ * For `InputAddressed` outputs or `Deferred` outputs:
+ *
+ * - with `Regular` hash kind, validate `InputAddressed` outputs have the
+ *   correct path (throws if mismatch), invoke `deferredCallback` for
+ *   `Deferred` outputs with the computed output path, then invoke
+ *   `envCallback` with the path.
+ *
+ * - with `Deferred` hash kind, validate that the output is either
+ *   `InputAddressed` (error) or `Deferred` (correct).
+ *
+ * For `CAFixed` outputs, invoke `envCallback` with the computed path.
+ */
+static void processForOutputPaths(
+    Store & store, auto && drv, std::string_view drvName, auto && deferredCallback, auto && envCallback)
+{
+    std::optional<DrvHash> hashesModulo;
+
+    for (auto & [outputName, output] : drv.outputs) {
+        auto envHasRightPath = [&](const StorePath & actual) {
+            envCallback(outputName, drv.env.find(outputName), actual);
+        };
+        auto hash = [&] {
+            if (!hashesModulo) {
+                // somewhat expensive so we do lazily
+                hashesModulo = hashDerivationModulo(store, drv, true);
+            }
+            switch (hashesModulo->kind) {
+            case DrvHash::Kind::Regular: {
+                auto h = get(hashesModulo->hashes, outputName);
+                if (!h)
+                    throw Error("derivation produced no hash for output '%s'", outputName);
+                auto outPath = store.makeOutputPath(outputName, *h, drvName);
+                std::visit(
+                    overloaded{
+                        [&](const DerivationOutput::InputAddressed & doia) {
+                            if (doia.path == outPath) {
+                                return; // Correct case
+                            }
+                            // Error case, an explicilty wrong path is always an error.
+                            throw Error(
+                                "derivation has incorrect output '%s', should be '%s'",
+                                store.printStorePath(doia.path),
+                                store.printStorePath(outPath));
+                        },
+                        [&](const DerivationOutput::Deferred &) {
+                            // The callback will either fill in the right path or throw an error
+                            deferredCallback(outputName, output, outPath);
+                        },
+                        [&](const auto &) {
+                            // should never happen, based on where `hash` is called.
+                            unreachable();
+                        },
+                    },
+                    output.raw);
+                envHasRightPath(outPath);
+                break;
+            }
+            case DrvHash::Kind::Deferred:
+                std::visit(
+                    overloaded{
+                        [&](const DerivationOutput::InputAddressed & doia) {
+                            // Error case, an explicilty wrong path is always an error.
+                            throw Error(
+                                "derivation has incorrect output '%s', should be deferred",
+                                store.printStorePath(doia.path));
+                        },
+                        [&](const DerivationOutput::Deferred &) {
+                            // Correct: Deferred output with Deferred hash kind
+                        },
+                        [&](const auto &) {
+                            // should never happen, based on where `hash` is called.
+                            unreachable();
+                        },
+                    },
+                    output.raw);
+                break;
+            }
+        };
+        std::visit(
+            overloaded{
+                [&](const DerivationOutput::InputAddressed &) { hash(); },
+                [&](const DerivationOutput::Deferred &) { hash(); },
+                [&](const DerivationOutput::CAFixed & dof) { envHasRightPath(dof.path(store, drvName, outputName)); },
+                [&](const auto &) {
+                    // Nothing to do for other output types
+                },
+            },
+            output.raw);
+    }
+}
+
 void Derivation::checkInvariants(Store & store, const StorePath & drvPath) const
 {
     assert(drvPath.isDerivation());
@@ -1217,65 +1311,96 @@ void Derivation::checkInvariants(Store & store, const StorePath & drvPath) const
     drvName = drvName.substr(0, drvName.size() - drvExtension.size());
 
     if (drvName != name) {
-        throw Error("Derivation '%s' has name '%s' which does not match its path", store.printStorePath(drvPath), name);
+        throw Error("derivation '%s' has name '%s' which does not match its path", store.printStorePath(drvPath), name);
     }
 
-    auto envHasRightPath = [&](const StorePath & actual, const std::string & varName) {
+    try {
-        auto j = env.find(varName);
+        checkInvariants(store);
-        if (j == env.end() || store.parseStorePath(j->second) != actual)
+    } catch (Error & e) {
-            throw Error(
+        e.addTrace({}, "while checking derivation '%s'", store.printStorePath(drvPath));
-                "derivation '%s' has incorrect environment variable '%s', should be '%s'",
+        throw;
-                store.printStorePath(drvPath),
+    }
-                varName,
+}
-                store.printStorePath(actual));
-    };
 
+void Derivation::checkInvariants(Store & store) const
+{
     // Don't need the answer, but do this anyways to assert is proper
     // combination. The code below is more general and naturally allows
     // combinations that are currently prohibited.
     type();
 
-    std::optional<DrvHash> hashesModulo;
+    processForOutputPaths(
-    for (auto & i : outputs) {
+        store,
-        std::visit(
+        *this,
-            overloaded{
+        name,
-                [&](const DerivationOutput::InputAddressed & doia) {
+        [&](const std::string & outputName, const DerivationOutput & output, const StorePath & outPath) {
-                    if (!hashesModulo) {
+            throw Error("derivation has incorrect deferred output, should be '%s'", store.printStorePath(outPath));
-                        // somewhat expensive so we do lazily
+        },
-                        hashesModulo = hashDerivationModulo(store, *this, true);
+        [&](const std::string & outputName, decltype(env)::const_iterator j, const StorePath & actual) {
-                    }
+            if (j == env.end())
-                    auto currentOutputHash = get(hashesModulo->hashes, i.first);
-                    if (!currentOutputHash)
                 throw Error(
-                            "derivation '%s' has unexpected output '%s' (local-store / hashesModulo) named '%s'",
+                    "derivation has missing environment variable '%s', should be '%s' but is not present",
-                            store.printStorePath(drvPath),
+                    outputName,
-                            store.printStorePath(doia.path),
+                    store.printStorePath(actual));
-                            i.first);
+            if (j->second != store.printStorePath(actual))
-                    StorePath recomputed = store.makeOutputPath(i.first, *currentOutputHash, drvName);
-                    if (doia.path != recomputed)
                 throw Error(
-                            "derivation '%s' has incorrect output '%s', should be '%s'",
+                    "derivation has incorrect environment variable '%s', should be '%s' but is actually '%s'",
-                            store.printStorePath(drvPath),
+                    outputName,
-                            store.printStorePath(doia.path),
+                    store.printStorePath(actual),
-                            store.printStorePath(recomputed));
+                    j->second);
-                    envHasRightPath(doia.path, i.first);
+        });
+}
+
+void Derivation::fillInOutputPaths(Store & store)
+{
+    processForOutputPaths(
+        store,
+        *this,
+        name,
+        [&](const std::string & outputName, DerivationOutput & output, StorePath outPath) {
+            // Fill in output path for Deferred outputs
+            output = DerivationOutput::InputAddressed{
+                .path = std::move(outPath),
+            };
         },
-                [&](const DerivationOutput::CAFixed & dof) {
+        [&](const std::string & outputName, decltype(env)::iterator j, const StorePath & actual) {
-                    auto path = dof.path(store, drvName, i.first);
+            if (j == env.end()) {
-                    envHasRightPath(path, i.first);
+                // fill it in
-                },
+                env.insert(j, {outputName, store.printStorePath(actual)});
-                [&](const DerivationOutput::CAFloating &) {
+                return;
-                    /* Nothing to check */
-                },
-                [&](const DerivationOutput::Deferred &) {
-                    /* Nothing to check */
-                },
-                [&](const DerivationOutput::Impure &) {
-                    /* Nothing to check */
-                },
-            },
-            i.second.raw);
             }
+            auto & value = j->second;
+            if (value == "") {
+                // fill it in
+                j->second = store.printStorePath(actual);
+                return;
+            }
+            if (j->second == store.printStorePath(actual)) {
+                // do nothing, already filled in
+                return;
+            }
+            throw Error(
+                "derivation has incorrect environment variable '%s', should be '%s' but is actually '%s'",
+                outputName,
+                store.printStorePath(actual),
+                value);
+        });
+}
+
+Derivation Derivation::parseJsonAndValidate(Store & store, const nlohmann::json & json)
+{
+    auto drv = static_cast<Derivation>(json);
+
+    drv.fillInOutputPaths(store);
+
+    try {
+        drv.checkInvariants(store);
+    } catch (Error & e) {
+        e.addTrace({}, "while checking derivation from JSON with name '%s'", drv.name);
+        throw;
+    }
+
+    return drv;
 }
 
 const Hash impureOutputHash = hashString(HashAlgorithm::SHA256, "impure");

src/libstore/include/nix/store/derivations.hh

@@ -368,9 +368,46 @@ struct Derivation : BasicDerivation
      * This is mainly a matter of checking the outputs, where our C++
      * representation supports all sorts of combinations we do not yet
      * allow.
+     *
+     * This overload does not validate the derivation name or add path
+     * context to errors. Use this when you don't have a `StorePath` or
+     * when you want to handle error context yourself.
+     *
+     * @param store The store to use for validation
+     */
+    void checkInvariants(Store & store) const;
+
+    /**
+     * This overload does everything the base `checkInvariants` does,
+     * but also validates that the derivation name matches the path, and
+     * improves any error messages that occur using the derivation path.
+     *
+     * @param store The store to use for validation
+     * @param drvPath The path to this derivation
      */
     void checkInvariants(Store & store, const StorePath & drvPath) const;
 
+    /**
+     * Fill in output paths as needed.
+     *
+     * For input-addressed derivations (ready or deferred), it computes
+     * the derivation hash modulo and based on the result:
+     *
+     * - If `Regular`: converts `Deferred` outputs to `InputAddressed`, and
+     *   ensures all `InputAddressed` (exist or just et) have he right
+     *   computed paths. Also updates the environment variables with
+     *   output paths.
+     *
+     * - If `Deferred`: converts `InputAddressed` to `Deferred`.
+     *
+     * Also for fixed-output content-addressed derivations, likewise
+     * updates output paths in env vars.
+     *
+     * @param store The store to use for path computation
+     * @param drvName The derivation name (without .drv extension)
+     */
+    void fillInOutputPaths(Store & store);
+
     Derivation() = default;
 
     Derivation(const BasicDerivation & bd)
@@ -383,6 +420,29 @@ struct Derivation : BasicDerivation
     {
     }
 
+    /**
+     * Parse a derivation from JSON, and also perform various
+     * conveniences such as:
+     *
+     * 1. Filling in output paths in as needed/required.
+     *
+     * 2. Checking invariants in general.
+     *
+     * In the future it might also do things like:
+     *
+     * - assist with the migration from older JSON formats.
+     *
+     * - (a somewhat exmaple of the above) initialize
+     *   `DerivationOptions` from their traditional encoding inside the
+     *   `env` and `structuredAttrs`.
+     *
+     * @param store The store to use for path computation and validation
+     * @param json The JSON representation of the derivation
+     * @return A validated derivation with output paths filled in
+     * @throws Error if parsing fails, output paths can't be computed, or validation fails
+     */
+    static Derivation parseJsonAndValidate(Store & store, const nlohmann::json & json);
+
     bool operator==(const Derivation &) const = default;
     // TODO libc++ 16 (used by darwin) missing `std::map::operator <=>`, can't do yet.
     // auto operator <=> (const Derivation &) const = default;

src/libutil-test-support/include/nix/util/tests/characterization.hh

@@ -31,16 +31,14 @@ static inline bool testAccept()
 /**
  * Mixin class for writing characterization tests
  */
-class CharacterizationTest : public virtual ::testing::Test
+struct CharacterizationTest : virtual ::testing::Test
 {
-protected:
     /**
      * While the "golden master" for this characterization test is
      * located. It should not be shared with any other test.
      */
     virtual std::filesystem::path goldenMaster(PathView testStem) const = 0;
 
-public:
     /**
      * Golden test for reading
      *

src/libutil-test-support/include/nix/util/tests/json-characterization.hh

@@ -39,6 +39,30 @@ void writeJsonTest(CharacterizationTest & test, PathView testStem, const T & val
         [](const auto & file, const auto & got) { return writeFile(file, got.dump(2) + "\n"); });
 }
 
+/**
+ * Golden test in the middle of something
+ */
+template<typename T>
+void checkpointJson(CharacterizationTest & test, PathView testStem, const T & got)
+{
+    using namespace nlohmann;
+
+    auto file = test.goldenMaster(Path{testStem} + ".json");
+
+    json gotJson = static_cast<json>(got);
+
+    if (testAccept()) {
+        std::filesystem::create_directories(file.parent_path());
+        writeFile(file, gotJson.dump(2) + "\n");
+        ADD_FAILURE() << "Updating golden master " << file;
+    } else {
+        json expectedJson = json::parse(readFile(file));
+        ASSERT_EQ(gotJson, expectedJson);
+        T expected = adl_serializer<T>::from_json(expectedJson);
+        ASSERT_EQ(got, expected);
+    }
+}
+
 /**
  * Mixin class for writing characterization tests for `nlohmann::json`
  * conversions for a given type.
@@ -67,6 +91,11 @@ struct JsonCharacterizationTest : virtual CharacterizationTest
     {
         nix::writeJsonTest(*this, testStem, value);
     }
+
+    void checkpointJson(PathView testStem, const T & value)
+    {
+        nix::checkpointJson(*this, testStem, value);
+    }
 };
 
 } // namespace nix

src/nix/derivation-add.cc

@@ -33,12 +33,10 @@ struct CmdAddDerivation : MixDryRun, StoreCommand
     {
         auto json = nlohmann::json::parse(drainFD(STDIN_FILENO));
 
-        auto drv = static_cast<Derivation>(json);
+        auto drv = Derivation::parseJsonAndValidate(*store, json);
 
         auto drvPath = writeDerivation(*store, drv, NoRepair, /* read only */ dryRun);
 
-        drv.checkInvariants(*store, drvPath);
-
         writeDerivation(*store, drv, NoRepair, dryRun);
 
         logger->cout("%s", store->printStorePath(drvPath));

tests/functional/derivation-json.sh

@@ -4,11 +4,20 @@ source common.sh
 
 drvPath=$(nix-instantiate simple.nix)
 
-nix derivation show "$drvPath" | jq .[] > "$TEST_HOME"/simple.json
+nix derivation show "$drvPath" | jq '.[]' > "$TEST_HOME/simple.json"
-
-drvPath2=$(nix derivation add < "$TEST_HOME"/simple.json)
 
+# Round tripping to JSON works
+drvPath2=$(nix derivation add < "$TEST_HOME/simple.json")
 [[ "$drvPath" = "$drvPath2" ]]
 
+# Derivaiton is input addressed, all outputs have a path
+jq -e '.outputs | .[] | has("path")' < "$TEST_HOME/simple.json"
+
 # Input addressed derivations cannot be renamed.
-jq '.name = "foo"' < "$TEST_HOME"/simple.json | expectStderr 1 nix derivation add | grepQuiet "has incorrect output"
+jq '.name = "foo"' < "$TEST_HOME/simple.json" | expectStderr 1 nix derivation add | grepQuiet "has incorrect output"
+
+# If we remove the input addressed to make it a deferred derivation, we
+# still get the same result because Nix will see that need not be
+# deferred and fill in the right input address for us.
+drvPath3=$(jq '.outputs |= map_values(del(.path))' < "$TEST_HOME/simple.json" | nix derivation add)
+[[ "$drvPath" = "$drvPath3" ]]