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.

Co-authored-by: Sergei Zimmerman <sergei@zimmerman.foo>

src/libexpr/primops.cc

@@ -1774,28 +1774,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,136 @@ 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). For `Deferred` outputs:
+ *   - if `fillIn` is true, fill in the output path to make `InputAddressed`
+ *   - if `fillIn` is false, throw an error
+ *   Then validate or fill in the environment variable with the path.
+ *
+ * - with `Deferred` hash kind, validate that the output is either
+ *   `InputAddressed` (error) or `Deferred` (correct).
+ *
+ * For `CAFixed` outputs, validate or fill in the environment variable
+ * with the computed path.
+ *
+ * @tparam fillIn If true, fill in missing output paths and environment
+ * variables. If false, validate that all paths are correct (throws on
+ * mismatch).
+ */
+template<bool fillIn>
+static void processDerivationOutputPaths(Store & store, auto && drv, std::string_view drvName)
+{
+    std::optional<DrvHash> hashesModulo;
+
+    for (auto & [outputName, output] : drv.outputs) {
+        auto envHasRightPath = [&](const StorePath & actual) {
+            if constexpr (fillIn) {
+                auto j = drv.env.find(outputName);
+                /* Fill in mode: fill in missing or empty environment
+                   variables */
+                if (j == drv.env.end())
+                    drv.env.insert(j, {outputName, store.printStorePath(actual)});
+                else if (j->second == "")
+                    j->second = store.printStorePath(actual);
+                /* We now validation will succeed after fill-in, but
+                   just to be extra sure, validate unconditionally */
+            }
+            auto j = drv.env.find(outputName);
+            if (j == drv.env.end())
+                throw Error(
+                    "derivation has missing environment variable '%s', should be '%s' but is not present",
+                    outputName,
+                    store.printStorePath(actual));
+            if (j->second != store.printStorePath(actual))
+                throw Error(
+                    "derivation has incorrect environment variable '%s', should be '%s' but is actually '%s'",
+                    outputName,
+                    store.printStorePath(actual),
+                    j->second);
+        };
+        auto hash = [&]<typename Output>(const Output & outputVariant) {
+            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);
+
+                if constexpr (std::is_same_v<Output, DerivationOutput::InputAddressed>) {
+                    if (outputVariant.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(outputVariant.path),
+                        store.printStorePath(outPath));
+                } else if constexpr (std::is_same_v<Output, DerivationOutput::Deferred>) {
+                    if constexpr (fillIn)
+                        /* Fill in output path for Deferred
+                           outputs */
+                        output = DerivationOutput::InputAddressed{
+                            .path = outPath,
+                        };
+                    else
+                        /* Validation mode: deferred outputs
+                           should have been filled in */
+                        throw Error(
+                            "derivation has incorrect deferred output, should be '%s'", store.printStorePath(outPath));
+                } else {
+                    /* Will never happen, based on where
+                       `hash` is called. */
+                    static_assert(false);
+                }
+                envHasRightPath(outPath);
+                break;
+            }
+            case DrvHash::Kind::Deferred:
+                if constexpr (std::is_same_v<Output, DerivationOutput::InputAddressed>) {
+                    /* Error case, an explicilty wrong path is
+                       always an error. */
+                    throw Error(
+                        "derivation has incorrect output '%s', should be deferred",
+                        store.printStorePath(outputVariant.path));
+                } else if constexpr (std::is_same_v<Output, DerivationOutput::Deferred>) {
+                    /* Correct: Deferred output with Deferred
+                       hash kind. */
+                } else {
+                    /* Will never happen, based on where
+                       `hash` is called. */
+                    static_assert(false);
+                }
+                break;
+            }
+        };
+        std::visit(
+            overloaded{
+                [&](const DerivationOutput::InputAddressed & o) { hash(o); },
+                [&](const DerivationOutput::Deferred & o) { hash(o); },
+                [&](const DerivationOutput::CAFixed & dof) { envHasRightPath(dof.path(store, drvName, outputName)); },
+                [&](const auto &) {
+                    // Nothing to do for other output types
+                },
+            },
+            output.raw);
+    }
+
+    /* Don't need the answer, but do this anyways to assert is proper
+       combination. The code above is more general and naturally allows
+       combinations that are currently prohibited. */
+    drv.type();
+}
+
 void Derivation::checkInvariants(Store & store, const StorePath & drvPath) const
 {
     assert(drvPath.isDerivation());
@@ -1217,65 +1347,41 @@ 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));
-    };
-
-    // 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;
-    for (auto & i : outputs) {
-        std::visit(
-            overloaded{
-                [&](const DerivationOutput::InputAddressed & doia) {
-                    if (!hashesModulo) {
-                        // somewhat expensive so we do lazily
-                        hashesModulo = hashDerivationModulo(store, *this, true);
     }
-                    auto currentOutputHash = get(hashesModulo->hashes, i.first);
+}
-                    if (!currentOutputHash)
+
-                        throw Error(
+void Derivation::checkInvariants(Store & store) const
-                            "derivation '%s' has unexpected output '%s' (local-store / hashesModulo) named '%s'",
+{
-                            store.printStorePath(drvPath),
+    processDerivationOutputPaths<false>(store, *this, name);
-                            store.printStorePath(doia.path),
+}
-                            i.first);
+
-                    StorePath recomputed = store.makeOutputPath(i.first, *currentOutputHash, drvName);
+void Derivation::fillInOutputPaths(Store & store)
-                    if (doia.path != recomputed)
+{
-                        throw Error(
+    processDerivationOutputPaths<true>(store, *this, name);
-                            "derivation '%s' has incorrect output '%s', should be '%s'",
+}
-                            store.printStorePath(drvPath),
+
-                            store.printStorePath(doia.path),
+Derivation Derivation::parseJsonAndValidate(Store & store, const nlohmann::json & json)
-                            store.printStorePath(recomputed));
+{
-                    envHasRightPath(doia.path, i.first);
+    auto drv = static_cast<Derivation>(json);
-                },
+
-                [&](const DerivationOutput::CAFixed & dof) {
+    drv.fillInOutputPaths(store);
-                    auto path = dof.path(store, drvName, i.first);
+
-                    envHasRightPath(path, i.first);
+    try {
-                },
+        drv.checkInvariants(store);
-                [&](const DerivationOutput::CAFloating &) {
+    } catch (Error & e) {
-                    /* Nothing to check */
+        e.addTrace({}, "while checking derivation from JSON with name '%s'", drv.name);
-                },
+        throw;
-                [&](const DerivationOutput::Deferred &) {
-                    /* Nothing to check */
-                },
-                [&](const DerivationOutput::Impure &) {
-                    /* Nothing to check */
-                },
-            },
-            i.second.raw);
     }
+
+    return drv;
 }
 
 const Hash impureOutputHash = hashString(HashAlgorithm::SHA256, "impure");

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

@@ -368,9 +368,48 @@ 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` outputs (whether preexisting
+     *   or newly computed) have the right computed paths. Likewise
+     *   defines (if absent or the empty string) or checks (if
+     *   preexisting and non-empty) environment variables for each
+     *   output with their path.
+     *
+     * - 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 +422,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 example 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/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" ]]