Fixing NixOS#7479: OutputsSpec and ExtendedOutputsSpec

See that issue for details.

src/libcmd/installable-attr-path.cc

@@ -80,9 +80,9 @@ DerivedPathsWithInfo InstallableAttrPath::toDerivedPaths()
             [&](const ExtendedOutputsSpec::Explicit & e) -> OutputsSpec {
                 return e;
             },
-        }, extendedOutputsSpec.raw());
+        }, extendedOutputsSpec.raw);
 
-        auto [iter, didInsert] = byDrvPath.emplace(*drvPath, newOutputs);
+        auto [iter, didInsert] = byDrvPath.emplace(*drvPath, newOutputs.raw);
 
         if (!didInsert)
             iter->second = iter->second.union_(newOutputs);
@@ -93,9 +93,10 @@ DerivedPathsWithInfo InstallableAttrPath::toDerivedPaths()
         res.push_back({
             .path = DerivedPath::Built {
                 .drvPath = makeConstantStorePathRef(drvPath),
-                .outputs = outputs,
+                .outputs = outputs.raw,
             },
             .info = make_ref<ExtraPathInfoValue>(ExtraPathInfoValue::Value {
+                .extendedOutputsSpec = outputs.raw,
                 /* FIXME: reconsider backwards compatibility above
                    so we can fill in this info. */
             }),
@@ -114,7 +115,7 @@ InstallableAttrPath InstallableAttrPath::parse(
     return {
         state, cmd, v,
         prefix == "." ? "" : std::string { prefix },
-        extendedOutputsSpec
+        std::move(extendedOutputsSpec),
     };
 }
 

src/libcmd/installable-derived-path.cc

@@ -55,7 +55,7 @@ InstallableDerivedPath InstallableDerivedPath::parse(
                 .outputs = outputSpec,
             };
         },
-    }, extendedOutputsSpec.raw());
+    }, extendedOutputsSpec.raw);
     return InstallableDerivedPath {
         store,
         std::move(derivedPath),

src/libcmd/installable-flake.cc

@@ -141,13 +141,13 @@ DerivedPathsWithInfo InstallableFlake::toDerivedPaths()
                 [&](const ExtendedOutputsSpec::Explicit & e) -> OutputsSpec {
                     return e;
                 },
-            }, extendedOutputsSpec.raw()),
+            }, extendedOutputsSpec.raw),
         },
         .info = make_ref<ExtraPathInfoFlake>(
             ExtraPathInfoValue::Value {
                 .priority = priority,
                 .attrPath = attrPath,
-                .extendedOutputsSpec = extendedOutputsSpec,
+                .extendedOutputsSpec = extendedOutputsSpec.raw,
             },
             ExtraPathInfoFlake::Flake {
                 .originalRef = flakeRef,

src/libcmd/installables.cc

@@ -459,7 +459,7 @@ Installables SourceExprCommand::parseInstallables(
             result.push_back(
                 make_ref<InstallableAttrPath>(
                     InstallableAttrPath::parse(
-                        state, *this, vFile, prefix, extendedOutputsSpec)));
+                        state, *this, vFile, std::move(prefix), std::move(extendedOutputsSpec))));
         }
 
     } else {
@@ -475,7 +475,7 @@ Installables SourceExprCommand::parseInstallables(
             if (prefix.find('/') != std::string::npos) {
                 try {
                     result.push_back(make_ref<InstallableDerivedPath>(
-                        InstallableDerivedPath::parse(store, prefix, extendedOutputsSpec)));
+                        InstallableDerivedPath::parse(store, prefix, extendedOutputsSpec.raw)));
                     continue;
                 } catch (BadStorePath &) {
                 } catch (...) {
@@ -491,7 +491,7 @@ Installables SourceExprCommand::parseInstallables(
                         getEvalState(),
                         std::move(flakeRef),
                         fragment,
-                        extendedOutputsSpec,
+                        std::move(extendedOutputsSpec),
                         getDefaultFlakeAttrPaths(),
                         getDefaultFlakeAttrPathPrefixes(),
                         lockFlags));

src/libexpr/flake/flakeref.cc

@@ -246,7 +246,7 @@ std::tuple<FlakeRef, std::string, ExtendedOutputsSpec> parseFlakeRefWithFragment
 {
     auto [prefix, extendedOutputsSpec] = ExtendedOutputsSpec::parse(url);
     auto [flakeRef, fragment] = parseFlakeRefWithFragment(std::string { prefix }, baseDir, allowMissing, isFlake);
-    return {std::move(flakeRef), fragment, extendedOutputsSpec};
+    return {std::move(flakeRef), fragment, std::move(extendedOutputsSpec)};
 }
 
 }

src/libstore/build/derivation-goal.cc

@@ -1358,7 +1358,7 @@ std::pair<bool, SingleDrvOutputs> DerivationGoal::checkPathValidity()
         [&](const OutputsSpec::Names & names) {
             return static_cast<StringSet>(names);
         },
-    }, wantedOutputs.raw());
+    }, wantedOutputs.raw);
     SingleDrvOutputs validOutputs;
 
     for (auto & i : queryPartialDerivationOutputMap()) {

src/libstore/build/worker.cc

@@ -268,7 +268,10 @@ void Worker::run(const Goals & _topGoals)
     for (auto & i : _topGoals) {
         topGoals.insert(i);
         if (auto goal = dynamic_cast<DerivationGoal *>(i.get())) {
-            topPaths.push_back(DerivedPath::Built{makeConstantStorePathRef(goal->drvPath), goal->wantedOutputs});
+            topPaths.push_back(DerivedPath::Built {
+                .drvPath = makeConstantStorePathRef(goal->drvPath),
+                .outputs = goal->wantedOutputs.raw,
+            });
         } else if (auto goal = dynamic_cast<PathSubstitutionGoal *>(i.get())) {
             topPaths.push_back(DerivedPath::Opaque{goal->storePath});
         }

src/libstore/misc.cc

@@ -370,7 +370,7 @@ OutputPathMap resolveDerivedPath(Store & store, const DerivedPath::Built & bfd,
             }
             return outputsOpt;
         },
-    }, bfd.outputs.raw());
+    }, bfd.outputs.raw);
 
     OutputPathMap outputs;
     for (auto & [outputName, outputPathOpt] : outputsOpt) {
@@ -418,7 +418,7 @@ OutputPathMap resolveDerivedPath(Store & store, const DerivedPath::Built & bfd)
         [&](const OutputsSpec::Names & names) {
             return static_cast<StringSet>(names);
         },
-    }, bfd.outputs.raw());
+    }, bfd.outputs.raw);
     for (auto iter = outputMap.begin(); iter != outputMap.end();) {
         auto & outputName = iter->first;
         if (bfd.outputs.contains(outputName)) {
@@ -431,7 +431,7 @@ OutputPathMap resolveDerivedPath(Store & store, const DerivedPath::Built & bfd)
     if (!outputsLeft.empty())
         throw Error("derivation '%s' does not have an outputs %s",
             store.printStorePath(drvPath),
-            concatStringsSep(", ", quoteStrings(std::get<OutputsSpec::Names>(bfd.outputs))));
+            concatStringsSep(", ", quoteStrings(std::get<OutputsSpec::Names>(bfd.outputs.raw))));
     return outputMap;
 }
 

src/libstore/outputs-spec.cc

@@ -17,7 +17,7 @@ bool OutputsSpec::contains(const std::string & outputName) const
         [&](const OutputsSpec::Names & outputNames) {
             return outputNames.count(outputName) > 0;
         },
-    }, raw());
+    }, raw);
 }
 
 static std::string outputSpecRegexStr =
@@ -49,7 +49,7 @@ OutputsSpec OutputsSpec::parse(std::string_view s)
     std::optional spec = parseOpt(s);
     if (!spec)
         throw Error("invalid outputs specifier '%s'", s);
-    return *spec;
+    return std::move(*spec);
 }
 
 
@@ -85,7 +85,7 @@ std::string OutputsSpec::to_string() const
         [&](const OutputsSpec::Names & outputNames) -> std::string {
             return concatStringsSep(",", outputNames);
         },
-    }, raw());
+    }, raw);
 }
 
 
@@ -98,7 +98,7 @@ std::string ExtendedOutputsSpec::to_string() const
         [&](const ExtendedOutputsSpec::Explicit & outputSpec) -> std::string {
             return "^" + outputSpec.to_string();
         },
-    }, raw());
+    }, raw);
 }
 
 
@@ -118,9 +118,9 @@ OutputsSpec OutputsSpec::union_(const OutputsSpec & that) const
                     ret.insert(thoseNames.begin(), thoseNames.end());
                     return ret;
                 },
-            }, that.raw());
+            }, that.raw);
         },
-    }, raw());
+    }, raw);
 }
 
 
@@ -142,9 +142,9 @@ bool OutputsSpec::isSubsetOf(const OutputsSpec & that) const
                             ret = false;
                     return ret;
                 },
-            }, raw());
+            }, raw);
         },
-    }, that.raw());
+    }, that.raw);
 }
 
 }
@@ -169,7 +169,7 @@ void adl_serializer<OutputsSpec>::to_json(json & json, OutputsSpec t) {
         [&](const OutputsSpec::Names & names) {
             json = names;
         },
-    }, t.raw());
+    }, t.raw);
 }
 
 
@@ -189,7 +189,7 @@ void adl_serializer<ExtendedOutputsSpec>::to_json(json & json, ExtendedOutputsSp
         [&](const ExtendedOutputsSpec::Explicit & e) {
             adl_serializer<OutputsSpec>::to_json(json, e);
         },
-    }, t.raw());
+    }, t.raw);
 }
 
 }

src/libstore/outputs-spec.hh

@@ -6,62 +6,58 @@
 #include <set>
 #include <variant>
 
+#include "comparator.hh"
 #include "json-impls.hh"
 
 namespace nix {
 
-/**
- * A non-empty set of outputs, specified by name
- */
-struct OutputNames : std::set<std::string> {
-    using std::set<std::string>::set;
+struct OutputsSpec {
+    /**
+     * A non-empty set of outputs, specified by name
+     */
+    struct Names : std::set<std::string> {
+        using std::set<std::string>::set;
+
+        /* These need to be "inherited manually" */
+
+        Names(const std::set<std::string> & s)
+            : std::set<std::string>(s)
+        { assert(!empty()); }
 
-    /* These need to be "inherited manually" */
+        /**
+         * Needs to be "inherited manually"
+         */
+        Names(std::set<std::string> && s)
+            : std::set<std::string>(s)
+        { assert(!empty()); }
 
-    OutputNames(const std::set<std::string> & s)
-        : std::set<std::string>(s)
-    { assert(!empty()); }
+        /* This set should always be non-empty, so we delete this
+           constructor in order make creating empty ones by mistake harder.
+           */
+        Names() = delete;
+    };
 
     /**
-     * Needs to be "inherited manually"
+     * The set of all outputs, without needing to name them explicitly
      */
-    OutputNames(std::set<std::string> && s)
-        : std::set<std::string>(s)
-    { assert(!empty()); }
-
-    /* This set should always be non-empty, so we delete this
-       constructor in order make creating empty ones by mistake harder.
-       */
-    OutputNames() = delete;
-};
+    struct All : std::monostate { };
+
+    typedef std::variant<All, Names> Raw;
 
-/**
- * The set of all outputs, without needing to name them explicitly
- */
-struct AllOutputs : std::monostate { };
+    Raw raw;
 
-typedef std::variant<AllOutputs, OutputNames> _OutputsSpecRaw;
+    GENERATE_CMP(OutputsSpec, me->raw);
 
-struct OutputsSpec : _OutputsSpecRaw {
-    using Raw = _OutputsSpecRaw;
-    using Raw::Raw;
+    /* The moral equivalent of `using Raw::Raw;` */
+    OutputsSpec(auto &&... arg)
+        : raw(std::forward<decltype(arg)>(arg)...)
+    { }
 
     /**
      * Force choosing a variant
      */
     OutputsSpec() = delete;
 
-    using Names = OutputNames;
-    using All = AllOutputs;
-
-    inline const Raw & raw() const {
-        return static_cast<const Raw &>(*this);
-    }
-
-    inline Raw & raw() {
-        return static_cast<Raw &>(*this);
-    }
-
     bool contains(const std::string & output) const;
 
     /**
@@ -84,20 +80,25 @@ struct OutputsSpec : _OutputsSpecRaw {
     std::string to_string() const;
 };
 
-struct DefaultOutputs : std::monostate { };
+struct ExtendedOutputsSpec {
+    struct Default : std::monostate { };
+    using Explicit = OutputsSpec;
+
+    typedef std::variant<Default, Explicit> Raw;
 
-typedef std::variant<DefaultOutputs, OutputsSpec> _ExtendedOutputsSpecRaw;
+    Raw raw;
 
-struct ExtendedOutputsSpec : _ExtendedOutputsSpecRaw {
-    using Raw = _ExtendedOutputsSpecRaw;
-    using Raw::Raw;
+    GENERATE_CMP(ExtendedOutputsSpec, me->raw);
 
-    using Default = DefaultOutputs;
-    using Explicit = OutputsSpec;
+    /* The moral equivalent of `using Raw::Raw;` */
+    ExtendedOutputsSpec(auto &&... arg)
+        : raw(std::forward<decltype(arg)>(arg)...)
+    { }
 
-    inline const Raw & raw() const {
-        return static_cast<const Raw &>(*this);
-    }
+    /**
+     * Force choosing a variant
+     */
+    ExtendedOutputsSpec() = delete;
 
     /**
      * Parse a string of the form 'prefix^output1,...outputN' or

src/libstore/path-with-outputs.cc

@@ -63,7 +63,7 @@ StorePathWithOutputs::ParseResult StorePathWithOutputs::tryFromDerivedPath(const
                             [&](const OutputsSpec::Names & outputs) {
                                 return static_cast<StringSet>(outputs);
                             },
-                        }, bfd.outputs.raw()),
+                        }, bfd.outputs.raw),
                     };
                 },
                 [&](const SingleDerivedPath::Built &) -> StorePathWithOutputs::ParseResult {

src/nix/bundle.cc

@@ -80,7 +80,7 @@ struct CmdBundle : InstallableValueCommand
         auto [bundlerFlakeRef, bundlerName, extendedOutputsSpec] = parseFlakeRefWithFragmentAndExtendedOutputsSpec(bundler, absPath("."));
         const flake::LockFlags lockFlags{ .writeLockFile = false };
         InstallableFlake bundler{this,
-            evalState, std::move(bundlerFlakeRef), bundlerName, extendedOutputsSpec,
+            evalState, std::move(bundlerFlakeRef), bundlerName, std::move(extendedOutputsSpec),
             {"bundlers." + settings.thisSystem.get() + ".default",
              "defaultBundler." + settings.thisSystem.get()
             },

src/nix/develop.cc

@@ -547,7 +547,7 @@ struct CmdDevelop : Common, MixEnvironment
                 state,
                 std::move(nixpkgs),
                 "bashInteractive",
-                DefaultOutputs(),
+                ExtendedOutputsSpec::Default(),
                 Strings{},
                 Strings{"legacyPackages." + settings.thisSystem.get() + "."},
                 nixpkgsLockFlags);

src/nix/flake.cc

@@ -778,7 +778,7 @@ struct CmdFlakeInitCommon : virtual Args, EvalCommand
         auto [templateFlakeRef, templateName] = parseFlakeRefWithFragment(templateUrl, absPath("."));
 
         auto installable = InstallableFlake(nullptr,
-            evalState, std::move(templateFlakeRef), templateName, DefaultOutputs(),
+            evalState, std::move(templateFlakeRef), templateName, ExtendedOutputsSpec::Default(),
             defaultTemplateAttrPaths,
             defaultTemplateAttrPathsPrefixes,
             lockFlags);