Stratify ExtraPathInfo along Installable hierarchy

Instead of having a bunch of optional fields, have a few subclasses
which can have mandatory fields.

Additionally, the new `getExtraPathInfo`, and `nixpkgsFlakeRef`, are
moved to `InstallableValue`.

I did these things because https://github.com/NixOS/rfcs/pull/134 ; with
these things moved to `InstallableValue`, the base `Installable` no
longer depends on libexpr! This is a major step towards that.

Also, add a bunch of doc comments for sake of the internal API docs.

src/libcmd/command.hh

@@ -1,6 +1,6 @@
 #pragma once
 
-#include "installables.hh"
+#include "installable-value.hh"
 #include "args.hh"
 #include "common-eval-args.hh"
 #include "path.hh"

src/libcmd/installable-attr-path.cc

@@ -87,6 +87,10 @@ DerivedPathsWithInfo InstallableAttrPath::toDerivedPaths()
                 .drvPath = drvPath,
                 .outputs = outputs,
             },
+            .info = make_ref<ExtraPathInfoValue>(ExtraPathInfoValue::Value {
+                /* FIXME: reconsider backwards compatibility above
+                   so we can fill in this info. */
+            }),
         });
 
     return res;

src/libcmd/installable-derived-path.cc

@@ -10,7 +10,10 @@ std::string InstallableDerivedPath::what() const
 
 DerivedPathsWithInfo InstallableDerivedPath::toDerivedPaths()
 {
-    return {{.path = derivedPath, .info = {} }};
+    return {{
+        .path = derivedPath,
+        .info = make_ref<ExtraPathInfo>(),
+    }};
 }
 
 std::optional<StorePath> InstallableDerivedPath::getStorePath()

src/libcmd/installable-flake.cc

@@ -101,7 +101,8 @@ DerivedPathsWithInfo InstallableFlake::toDerivedPaths()
             return {{
                 .path = DerivedPath::Opaque {
                     .path = std::move(storePath),
-                }
+                },
+                .info = make_ref<ExtraPathInfo>(),
             }};
         }
 
@@ -113,7 +114,8 @@ DerivedPathsWithInfo InstallableFlake::toDerivedPaths()
                 return {{
                     .path = DerivedPath::Opaque {
                         .path = std::move(*storePath),
-                    }
+                    },
+                    .info = make_ref<ExtraPathInfo>(),
                 }};
             } else
                 throw Error("flake output attribute '%s' evaluates to the string '%s' which is not a store path", attrPath, s);
@@ -160,13 +162,16 @@ DerivedPathsWithInfo InstallableFlake::toDerivedPaths()
                 },
             }, extendedOutputsSpec.raw()),
         },
-        .info = {
+        .info = make_ref<ExtraPathInfoFlake>(
-            .priority = priority,
+            ExtraPathInfoValue::Value {
-            .originalRef = flakeRef,
+                .priority = priority,
-            .resolvedRef = getLockedFlake()->flake.lockedRef,
+                .attrPath = attrPath,
-            .attrPath = attrPath,
+                .extendedOutputsSpec = extendedOutputsSpec,
-            .extendedOutputsSpec = extendedOutputsSpec,
+            },
-        }
+            ExtraPathInfoFlake::Flake {
+                .originalRef = flakeRef,
+                .resolvedRef = getLockedFlake()->flake.lockedRef,
+            }),
     }};
 }
 
@@ -212,6 +217,7 @@ std::shared_ptr<flake::LockedFlake> InstallableFlake::getLockedFlake() const
 {
     if (!_lockedFlake) {
         flake::LockFlags lockFlagsApplyConfig = lockFlags;
+        // FIXME why this side effect?
         lockFlagsApplyConfig.applyNixConfig = true;
         _lockedFlake = std::make_shared<flake::LockedFlake>(lockFlake(*state, flakeRef, lockFlagsApplyConfig));
     }
@@ -229,7 +235,7 @@ FlakeRef InstallableFlake::nixpkgsFlakeRef() const
         }
     }
 
-    return Installable::nixpkgsFlakeRef();
+    return InstallableValue::nixpkgsFlakeRef();
 }
 
 }

src/libcmd/installable-flake.hh

@@ -4,6 +4,30 @@
 
 namespace nix {
 
+/**
+ * Extra info about a \ref DerivedPath "derived path" that ultimately
+ * come from a Flake.
+ *
+ * Invariant: every ExtraPathInfo gotten from an InstallableFlake should
+ * be possible to downcast to an ExtraPathInfoFlake.
+ */
+struct ExtraPathInfoFlake : ExtraPathInfoValue
+{
+    /**
+     * Extra struct to get around C++ designated initializer limitations
+     */
+    struct Flake {
+        FlakeRef originalRef;
+        FlakeRef resolvedRef;
+    };
+
+    Flake flake;
+
+    ExtraPathInfoFlake(Value && v, Flake && f)
+        : ExtraPathInfoValue(std::move(v)), flake(f)
+    { }
+};
+
 struct InstallableFlake : InstallableValue
 {
     FlakeRef flakeRef;
@@ -33,8 +57,10 @@ struct InstallableFlake : InstallableValue
 
     std::pair<Value *, PosIdx> toValue(EvalState & state) override;
 
-    /* Get a cursor to every attrpath in getActualAttrPaths()
+    /**
-       that exists. However if none exists, throw an exception. */
+     * Get a cursor to every attrpath in getActualAttrPaths() that
+     * exists. However if none exists, throw an exception.
+     */
     std::vector<ref<eval_cache::AttrCursor>>
     getCursors(EvalState & state) override;
 

src/libcmd/installable-value.hh

@@ -1,9 +1,15 @@
 #pragma once
 
 #include "installables.hh"
+#include "flake/flake.hh"
 
 namespace nix {
 
+struct DrvInfo;
+struct SourceExprCommand;
+
+namespace eval_cache { class EvalCache; class AttrCursor; }
+
 struct App
 {
     std::vector<DerivedPath> context;
@@ -17,26 +23,83 @@ struct UnresolvedApp
     App resolve(ref<Store> evalStore, ref<Store> store);
 };
 
+/**
+ * Extra info about a \ref DerivedPath "derived path" that ultimately
+ * come from a Nix language value.
+ *
+ * Invariant: every ExtraPathInfo gotten from an InstallableValue should
+ * be possible to downcast to an ExtraPathInfoValue.
+ */
+struct ExtraPathInfoValue : ExtraPathInfo
+{
+    /**
+     * Extra struct to get around C++ designated initializer limitations
+     */
+    struct Value {
+        /**
+         * An optional priority for use with "build envs". See Package
+         */
+        std::optional<NixInt> priority;
+
+        /**
+         * The attribute path associated with this value. The idea is
+         * that an installable referring to a value typically refers to
+         * a larger value, from which we project a smaller value out
+         * with this.
+         */
+        std::string attrPath;
+
+        /**
+         * \todo merge with DerivedPath's 'outputs' field?
+         */
+        ExtendedOutputsSpec extendedOutputsSpec;
+    };
+
+    Value value;
+
+    ExtraPathInfoValue(Value && v)
+        : value(v)
+    { }
+
+    virtual ~ExtraPathInfoValue() = default;
+};
+
+/**
+ * An Installable which corresponds a Nix langauge value, in addition to
+ * a collection of \ref DerivedPath "derived paths".
+ */
 struct InstallableValue : Installable
 {
     ref<EvalState> state;
 
     InstallableValue(ref<EvalState> state) : state(state) {}
 
+    virtual ~InstallableValue() { }
+
     virtual std::pair<Value *, PosIdx> toValue(EvalState & state) = 0;
 
-    /* Get a cursor to each value this Installable could refer to. However
+    /**
-       if none exists, throw exception instead of returning empty vector. */
+     * Get a cursor to each value this Installable could refer to.
+     * However if none exists, throw exception instead of returning
+     * empty vector.
+     */
     virtual std::vector<ref<eval_cache::AttrCursor>>
     getCursors(EvalState & state);
 
-    /* Get the first and most preferred cursor this Installable could refer
+    /**
-       to, or throw an exception if none exists. */
+     * Get the first and most preferred cursor this Installable could
+     * refer to, or throw an exception if none exists.
+     */
     virtual ref<eval_cache::AttrCursor>
     getCursor(EvalState & state);
 
     UnresolvedApp toApp(EvalState & state);
 
+    virtual FlakeRef nixpkgsFlakeRef() const
+    {
+        return FlakeRef::fromAttrs({{"type","indirect"}, {"id", "nixpkgs"}});
+    }
+
     static InstallableValue & require(Installable & installable);
     static ref<InstallableValue> require(ref<Installable> installable);
 };

src/libcmd/installables.cc

@@ -517,7 +517,7 @@ std::vector<std::pair<ref<Installable>, BuiltPathWithResult>> Installable::build
 
     struct Aux
     {
-        ExtraPathInfo info;
+        ref<ExtraPathInfo> info;
         ref<Installable> installable;
     };
 

src/libcmd/installables.hh

@@ -4,9 +4,7 @@
 #include "path.hh"
 #include "outputs-spec.hh"
 #include "derived-path.hh"
-#include "eval.hh"
 #include "store-api.hh"
-#include "flake/flake.hh"
 #include "build-result.hh"
 
 #include <optional>
@@ -14,83 +12,144 @@
 namespace nix {
 
 struct DrvInfo;
-struct SourceExprCommand;
-
-namespace eval_cache { class EvalCache; class AttrCursor; }
 
 enum class Realise {
-    /* Build the derivation. Postcondition: the
+    /**
-       derivation outputs exist. */
+     * Build the derivation.
+     *
+     * Postcondition: the derivation outputs exist.
+     */
     Outputs,
-    /* Don't build the derivation. Postcondition: the store derivation
+    /**
-       exists. */
+     * Don't build the derivation.
+     *
+     * Postcondition: the store derivation exists.
+     */
     Derivation,
-    /* Evaluate in dry-run mode. Postcondition: nothing. */
+    /**
-    // FIXME: currently unused, but could be revived if we can
+     * Evaluate in dry-run mode.
-    // evaluate derivations in-memory.
+     *
+     * Postcondition: nothing.
+     *
+     * \todo currently unused, but could be revived if we can evaluate
+     * derivations in-memory.
+     */
     Nothing
 };
 
-/* How to handle derivations in commands that operate on store paths. */
+/**
+ * How to handle derivations in commands that operate on store paths.
+ */
 enum class OperateOn {
-    /* Operate on the output path. */
+    /**
+     * Operate on the output path.
+     */
     Output,
-    /* Operate on the .drv path. */
+    /**
+     * Operate on the .drv path.
+     */
     Derivation
 };
 
+/**
+ * Extra info about a DerivedPath
+ *
+ * Yes, this is empty, but that is intended. It will be sub-classed by
+ * the subclasses of Installable to allow those to provide more info.
+ * Certain commands will make use of this info.
+ */
 struct ExtraPathInfo
 {
-    std::optional<NixInt> priority;
+    virtual ~ExtraPathInfo() = default;
-    std::optional<FlakeRef> originalRef;
-    std::optional<FlakeRef> resolvedRef;
-    std::optional<std::string> attrPath;
-    // FIXME: merge with DerivedPath's 'outputs' field?
-    std::optional<ExtendedOutputsSpec> extendedOutputsSpec;
 };
 
-/* A derived path with any additional info that commands might
+/**
-   need from the derivation. */
+ * A DerivedPath with \ref ExtraPathInfo "any additional info" that
+ * commands might need from the derivation.
+ */
 struct DerivedPathWithInfo
 {
     DerivedPath path;
-    ExtraPathInfo info;
+    ref<ExtraPathInfo> info;
 };
 
+/**
+ * Like DerivedPathWithInfo but extending BuiltPath with \ref
+ * ExtraPathInfo "extra info" and also possibly the \ref BuildResult
+ * "result of building".
+ */
 struct BuiltPathWithResult
 {
     BuiltPath path;
-    ExtraPathInfo info;
+    ref<ExtraPathInfo> info;
     std::optional<BuildResult> result;
 };
 
+/**
+ * Shorthand, for less typing and helping us keep the choice of
+ * collection in sync.
+ */
 typedef std::vector<DerivedPathWithInfo> DerivedPathsWithInfo;
 
 struct Installable;
+
+/**
+ * Shorthand, for less typing and helping us keep the choice of
+ * collection in sync.
+ */
 typedef std::vector<ref<Installable>> Installables;
 
+/**
+ * Installables are the main positional arguments for the Nix
+ * Command-line.
+ *
+ * This base class is very flexible, and just assumes and the
+ * Installable refers to a collection of \ref DerivedPath "derived paths" with
+ * \ref ExtraPathInfo "extra info".
+ */
 struct Installable
 {
     virtual ~Installable() { }
 
+    /**
+     * What Installable is this?
+     *
+     * Prints back valid CLI syntax that would result in this same
+     * installable. It doesn't need to be exactly what the user wrote,
+     * just something that means the same thing.
+     */
     virtual std::string what() const = 0;
 
+    /**
+     * Get the collection of \ref DerivedPathWithInfo "derived paths
+     * with info" that this \ref Installable instalallable denotes.
+     *
+     * This is the main method of this class
+     */
     virtual DerivedPathsWithInfo toDerivedPaths() = 0;
 
+    /**
+     * A convenience wrapper of the above for when we expect an
+     * installable to produce a single \ref DerivedPath "derived path"
+     * only.
+     *
+     * If no or multiple \ref DerivedPath "derived paths" are produced,
+     * and error is raised.
+     */
     DerivedPathWithInfo toDerivedPath();
 
-    /* Return a value only if this installable is a store path or a
+    /**
-       symlink to it. */
+     * Return a value only if this installable is a store path or a
+     * symlink to it.
+     *
+     * \todo should we move this to InstallableDerivedPath? It is only
+     * supposed to work there anyways. Can always downcast.
+     */
     virtual std::optional<StorePath> getStorePath()
     {
         return {};
     }
 
-    virtual FlakeRef nixpkgsFlakeRef() const
-    {
-        return FlakeRef::fromAttrs({{"type","indirect"}, {"id", "nixpkgs"}});
-    }
-
     static std::vector<BuiltPathWithResult> build(
         ref<Store> evalStore,
         ref<Store> store,

src/nix/build.cc

@@ -1,4 +1,3 @@
-#include "eval.hh"
 #include "command.hh"
 #include "common-args.hh"
 #include "shared.hh"

src/nix/bundle.cc

@@ -5,6 +5,7 @@
 #include "store-api.hh"
 #include "local-fs-store.hh"
 #include "fs-accessor.hh"
+#include "eval-inline.hh"
 
 using namespace nix;
 

src/nix/develop.cc

@@ -1,6 +1,6 @@
 #include "eval.hh"
-#include "command.hh"
 #include "installable-flake.hh"
+#include "command-installable-value.hh"
 #include "common-args.hh"
 #include "shared.hh"
 #include "store-api.hh"
@@ -252,7 +252,7 @@ static StorePath getDerivationEnvironment(ref<Store> store, ref<Store> evalStore
     throw Error("get-env.sh failed to produce an environment");
 }
 
-struct Common : InstallableCommand, MixProfile
+struct Common : InstallableValueCommand, MixProfile
 {
     std::set<std::string> ignoreVars{
         "BASHOPTS",
@@ -374,7 +374,7 @@ struct Common : InstallableCommand, MixProfile
         return res;
     }
 
-    StorePath getShellOutPath(ref<Store> store, ref<Installable> installable)
+    StorePath getShellOutPath(ref<Store> store, ref<InstallableValue> installable)
     {
         auto path = installable->getStorePath();
         if (path && hasSuffix(path->to_string(), "-env"))
@@ -393,7 +393,7 @@ struct Common : InstallableCommand, MixProfile
     }
 
     std::pair<BuildEnvironment, std::string>
-    getBuildEnvironment(ref<Store> store, ref<Installable> installable)
+    getBuildEnvironment(ref<Store> store, ref<InstallableValue> installable)
     {
         auto shellOutPath = getShellOutPath(store, installable);
 
@@ -481,7 +481,7 @@ struct CmdDevelop : Common, MixEnvironment
           ;
     }
 
-    void run(ref<Store> store, ref<Installable> installable) override
+    void run(ref<Store> store, ref<InstallableValue> installable) override
     {
         auto [buildEnvironment, gcroot] = getBuildEnvironment(store, installable);
 
@@ -605,7 +605,7 @@ struct CmdPrintDevEnv : Common, MixJSON
 
     Category category() override { return catUtility; }
 
-    void run(ref<Store> store, ref<Installable> installable) override
+    void run(ref<Store> store, ref<InstallableValue> installable) override
     {
         auto buildEnvironment = getBuildEnvironment(store, installable).first;
 

src/nix/profile.cc

@@ -254,13 +254,19 @@ struct ProfileManifest
     }
 };
 
-static std::map<Installable *, std::pair<BuiltPaths, ExtraPathInfo>>
+static std::map<Installable *, std::pair<BuiltPaths, ref<ExtraPathInfo>>>
 builtPathsPerInstallable(
     const std::vector<std::pair<ref<Installable>, BuiltPathWithResult>> & builtPaths)
 {
-    std::map<Installable *, std::pair<BuiltPaths, ExtraPathInfo>> res;
+    std::map<Installable *, std::pair<BuiltPaths, ref<ExtraPathInfo>>> res;
     for (auto & [installable, builtPath] : builtPaths) {
-        auto & r = res[&*installable];
+        auto & r = res.insert({
+            &*installable,
+            {
+                {},
+                make_ref<ExtraPathInfo>(),
+            }
+        }).first->second;
         /* Note that there could be conflicting info
            (e.g. meta.priority fields) if the installable returned
            multiple derivations. So pick one arbitrarily. FIXME:
@@ -307,14 +313,16 @@ struct CmdProfileInstall : InstallablesCommand, MixDefaultProfile
         for (auto & installable : installables) {
             ProfileElement element;
 
-            auto & [res, info] = builtPaths[&*installable];
+            auto iter = builtPaths.find(&*installable);
+            if (iter == builtPaths.end()) continue;
+            auto & [res, info] = iter->second;
 
-            if (info.originalRef && info.resolvedRef && info.attrPath && info.extendedOutputsSpec) {
+            if (auto * info2 = dynamic_cast<ExtraPathInfoFlake *>(&*info)) {
                 element.source = ProfileElementSource {
-                    .originalRef = *info.originalRef,
+                    .originalRef = info2->flake.originalRef,
-                    .resolvedRef = *info.resolvedRef,
+                    .resolvedRef = info2->flake.resolvedRef,
-                    .attrPath = *info.attrPath,
+                    .attrPath = info2->value.attrPath,
-                    .outputs = *info.extendedOutputsSpec,
+                    .outputs = info2->value.extendedOutputsSpec,
                 };
             }
 
@@ -323,7 +331,12 @@ struct CmdProfileInstall : InstallablesCommand, MixDefaultProfile
             element.priority =
                 priority
                 ? *priority
-                : info.priority.value_or(defaultPriority);
+                : ({
+                    auto * info2 = dynamic_cast<ExtraPathInfoValue *>(&*info);
+                    info2
+                        ? info2->value.priority.value_or(defaultPriority)
+                        : defaultPriority;
+                });
 
             element.updateStorePaths(getEvalStore(), store, res);
 
@@ -541,19 +554,20 @@ struct CmdProfileUpgrade : virtual SourceExprCommand, MixDefaultProfile, MixProf
 
                 auto derivedPaths = installable->toDerivedPaths();
                 if (derivedPaths.empty()) continue;
-                auto & info = derivedPaths[0].info;
+                auto * infop = dynamic_cast<ExtraPathInfoFlake *>(&*derivedPaths[0].info);
+                // `InstallableFlake` should use `ExtraPathInfoFlake`.
+                assert(infop);
+                auto & info = *infop;
 
-                assert(info.resolvedRef && info.attrPath);
+                if (element.source->resolvedRef == info.flake.resolvedRef) continue;
-
-                if (element.source->resolvedRef == info.resolvedRef) continue;
 
                 printInfo("upgrading '%s' from flake '%s' to '%s'",
-                    element.source->attrPath, element.source->resolvedRef, *info.resolvedRef);
+                    element.source->attrPath, element.source->resolvedRef, info.flake.resolvedRef);
 
                 element.source = ProfileElementSource {
                     .originalRef = installable->flakeRef,
-                    .resolvedRef = *info.resolvedRef,
+                    .resolvedRef = info.flake.resolvedRef,
-                    .attrPath = *info.attrPath,
+                    .attrPath = info.value.attrPath,
                     .outputs = installable->extendedOutputsSpec,
                 };
 
@@ -582,7 +596,10 @@ struct CmdProfileUpgrade : virtual SourceExprCommand, MixDefaultProfile, MixProf
         for (size_t i = 0; i < installables.size(); ++i) {
             auto & installable = installables.at(i);
             auto & element = manifest.elements[indices.at(i)];
-            element.updateStorePaths(getEvalStore(), store, builtPaths[&*installable].first);
+            element.updateStorePaths(
+                getEvalStore(),
+                store,
+                builtPaths.find(&*installable)->second.first);
         }
 
         updateProfile(manifest.build(store));