Fixing NixOS#7479

Types converted:

- `NixStringContextElem`
- `OutputsSpec`
- `ExtendedOutputsSpec`
- `DerivationOutput`
- `DerivationType`

Existing ones mostly conforming the pattern cleaned up:

- `ContentAddressMethod`
- `ContentAddressWithReferences`

The `DerivationGoal::derivationType` field had a bogus initialization,
now caught, so I made it `std::optional`. I think NixOS#8829 can make it
non-optional again because it will ensure we always have the derivation
when we construct a `DerivationGoal`.

See that issue (NixOS#7479) for details on the general goal.

`git grep 'Raw::Raw'` indicates the two types I didn't yet convert
`DerivedPath` and `BuiltPath` (and their `Single` variants) . This is
because @roberth and I (can't find issue right now...) plan on reworking
them somewhat, so I didn't want to churn them more just yet.

Co-authored-by: Eelco Dolstra <edolstra@gmail.com>

src/libcmd/installable-attr-path.cc

@@ -80,7 +80,7 @@ DerivedPathsWithInfo InstallableAttrPath::toDerivedPaths()
  [&](const ExtendedOutputsSpec::Explicit & e) -> OutputsSpec {
  return e;
  },
- }, extendedOutputsSpec.raw());
+ }, extendedOutputsSpec.raw);
 
  auto [iter, didInsert] = byDrvPath.emplace(*drvPath, newOutputs);
 
@@ -96,6 +96,7 @@ DerivedPathsWithInfo InstallableAttrPath::toDerivedPaths()
  .outputs = outputs,
  },
  .info = make_ref<ExtraPathInfoValue>(ExtraPathInfoValue::Value {
+ .extendedOutputsSpec = outputs,
  /* 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,7 +141,7 @@ DerivedPathsWithInfo InstallableFlake::toDerivedPaths()
  [&](const ExtendedOutputsSpec::Explicit & e) -> OutputsSpec {
  return e;
  },
- }, extendedOutputsSpec.raw()),
+ }, extendedOutputsSpec.raw),
  },
  .info = make_ref<ExtraPathInfoFlake>(
  ExtraPathInfoValue::Value {

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/eval-cache.cc

@@ -604,7 +604,7 @@ string_t AttrCursor::getStringWithContext()
  [&](const NixStringContextElem::Opaque & o) -> const StorePath & {
  return o.path;
  },
- }, c.raw());
+ }, c.raw);
  if (!root->state.store->isValidPath(path)) {
  valid = false;
  break;

src/libexpr/eval.cc

@@ -2364,7 +2364,7 @@ std::pair<SingleDerivedPath, std::string_view> EvalState::coerceToSingleDerivedP
  [&](NixStringContextElem::Built && b) -> SingleDerivedPath {
  return std::move(b);
  },
- }, ((NixStringContextElem &&) *context.begin()).raw());
+ }, ((NixStringContextElem &&) *context.begin()).raw);
  return {
  std::move(derivedPath),
  std::move(s),

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/libexpr/primops.cc

@@ -69,7 +69,7 @@ StringMap EvalState::realiseContext(const NixStringContext & context)
  res.insert_or_assign(ctxS, ctxS);
  ensureValid(d.drvPath);
  },
- }, c.raw());
+ }, c.raw);
  }
 
  if (drvs.empty()) return {};
@@ -1265,7 +1265,7 @@ drvName, Bindings * attrs, Value & v)
  [&](const NixStringContextElem::Opaque & o) {
  drv.inputSrcs.insert(o.path);
  },
- }, c.raw());
+ }, c.raw);
  }
 
  /* Do we have all required attributes? */
@@ -1334,13 +1334,13 @@ drvName, Bindings * attrs, Value & v)
  if (isImpure)
  drv.outputs.insert_or_assign(i,
  DerivationOutput::Impure {
- .method = method.raw,
+ .method = method,
  .hashType = ht,
  });
  else
  drv.outputs.insert_or_assign(i,
  DerivationOutput::CAFloating {
- .method = method.raw,
+ .method = method,
  .hashType = ht,
  });
  }
@@ -1373,15 +1373,15 @@ drvName, Bindings * attrs, Value & v)
  drv.env[i] = state.store->printStorePath(outPath);
  drv.outputs.insert_or_assign(
  i,
- DerivationOutputInputAddressed {
+ DerivationOutput::InputAddressed {
  .path = std::move(outPath),
  });
  }
  break;
  ;
  case DrvHash::Kind::Deferred:
  for (auto & i : outputs) {
- drv.outputs.insert_or_assign(i, DerivationOutputDeferred {});
+ drv.outputs.insert_or_assign(i, DerivationOutput::Deferred {});
  }
  }
  }
@@ -2054,7 +2054,7 @@ static void prim_toFile(EvalState & state, const PosIdx pos, Value * * args, Val
  StorePathSet refs;
 
  for (auto c : context) {
- if (auto p = std::get_if<NixStringContextElem::Opaque>(&c))
+ if (auto p = std::get_if<NixStringContextElem::Opaque>(&c.raw))
  refs.insert(p->path);
  else
  state.debugThrowLastTrace(EvalError({

src/libexpr/primops/context.cc

@@ -51,13 +51,13 @@ static void prim_unsafeDiscardOutputDependency(EvalState & state, const PosIdx p
 
  NixStringContext context2;
  for (auto && c : context) {
- if (auto * ptr = std::get_if<NixStringContextElem::DrvDeep>(&c)) {
+ if (auto * ptr = std::get_if<NixStringContextElem::DrvDeep>(&c.raw)) {
  context2.emplace(NixStringContextElem::Opaque {
  .path = ptr->drvPath
  });
  } else {
  /* Can reuse original item */
- context2.emplace(std::move(c));
+ context2.emplace(std::move(c).raw);
  }
  }
 
@@ -114,7 +114,7 @@ static void prim_getContext(EvalState & state, const PosIdx pos, Value * * args,
  [&](NixStringContextElem::Opaque && o) {
  contextInfos[std::move(o.path)].path = true;
  },
- }, ((NixStringContextElem &&) i).raw());
+ }, ((NixStringContextElem &&) i).raw);
  }
 
  auto attrs = state.buildBindings(contextInfos.size());

src/libexpr/tests/value/context.cc

@@ -47,7 +47,7 @@ TEST(NixStringContextElemTest, slash_invalid) {
 TEST(NixStringContextElemTest, opaque) {
  std::string_view opaque = "g1w7hy3qg1w7hy3qg1w7hy3qg1w7hy3q-x";
  auto elem = NixStringContextElem::parse(opaque);
- auto * p = std::get_if<NixStringContextElem::Opaque>(&elem);
+ auto * p = std::get_if<NixStringContextElem::Opaque>(&elem.raw);
  ASSERT_TRUE(p);
  ASSERT_EQ(p->path, StorePath { opaque });
  ASSERT_EQ(elem.to_string(), opaque);
@@ -60,7 +60,7 @@ TEST(NixStringContextElemTest, opaque) {
 TEST(NixStringContextElemTest, drvDeep) {
  std::string_view drvDeep = "=g1w7hy3qg1w7hy3qg1w7hy3qg1w7hy3q-x.drv";
  auto elem = NixStringContextElem::parse(drvDeep);
- auto * p = std::get_if<NixStringContextElem::DrvDeep>(&elem);
+ auto * p = std::get_if<NixStringContextElem::DrvDeep>(&elem.raw);
  ASSERT_TRUE(p);
  ASSERT_EQ(p->drvPath, StorePath { drvDeep.substr(1) });
  ASSERT_EQ(elem.to_string(), drvDeep);
@@ -73,7 +73,7 @@ TEST(NixStringContextElemTest, drvDeep) {
 TEST(NixStringContextElemTest, built_opaque) {
  std::string_view built = "!foo!g1w7hy3qg1w7hy3qg1w7hy3qg1w7hy3q-x.drv";
  auto elem = NixStringContextElem::parse(built);
- auto * p = std::get_if<NixStringContextElem::Built>(&elem);
+ auto * p = std::get_if<NixStringContextElem::Built>(&elem.raw);
  ASSERT_TRUE(p);
  ASSERT_EQ(p->output, "foo");
  ASSERT_EQ(*p->drvPath, ((SingleDerivedPath) SingleDerivedPath::Opaque {
@@ -96,7 +96,7 @@ TEST(NixStringContextElemTest, built_built) {
 
  std::string_view built = "!foo!bar!g1w7hy3qg1w7hy3qg1w7hy3qg1w7hy3q-x.drv";
  auto elem = NixStringContextElem::parse(built, mockXpSettings);
- auto * p = std::get_if<NixStringContextElem::Built>(&elem);
+ auto * p = std::get_if<NixStringContextElem::Built>(&elem.raw);
  ASSERT_TRUE(p);
  ASSERT_EQ(p->output, "foo");
  auto * drvPath = std::get_if<SingleDerivedPath::Built>(&*p->drvPath);

src/libexpr/value/context.cc

@@ -99,7 +99,7 @@ std::string NixStringContextElem::to_string() const
  res += '=';
  res += d.drvPath.to_string();
  },
- }, raw());
+ }, raw);
 
  return res;
 }

src/libexpr/value/context.hh

@@ -4,8 +4,7 @@
 #include "util.hh"
 #include "comparator.hh"
 #include "derived-path.hh"
-
-#include <variant>
+#include "variant-wrapper.hh"
 
 #include <nlohmann/json_fwd.hpp>
 
@@ -26,58 +25,47 @@ public:
  }
 };
 
-/**
- * Plain opaque path to some store object.
- *
- * Encoded as just the path: ‘<path>’.
- */
-typedef SingleDerivedPath::Opaque NixStringContextElem_Opaque;
-
-/**
- * Path to a derivation and its entire build closure.
- *
- * The path doesn't just refer to derivation itself and its closure, but
- * also all outputs of all derivations in that closure (including the
- * root derivation).
- *
- * Encoded in the form ‘=<drvPath>’.
- */
-struct NixStringContextElem_DrvDeep {
- StorePath drvPath;
-
- GENERATE_CMP(NixStringContextElem_DrvDeep, me->drvPath);
-};
+struct NixStringContextElem {
+ /**
+ * Plain opaque path to some store object.
+ *
+ * Encoded as just the path: ‘<path>’.
+ */
+ using Opaque = SingleDerivedPath::Opaque;
 
-/**
- * Derivation output.
- *
- * Encoded in the form ‘!<output>!<drvPath>’.
- */
-typedef SingleDerivedPath::Built NixStringContextElem_Built;
-
-using _NixStringContextElem_Raw = std::variant<
- NixStringContextElem_Opaque,
- NixStringContextElem_DrvDeep,
- NixStringContextElem_Built
->;
-
-struct NixStringContextElem : _NixStringContextElem_Raw {
- using Raw = _NixStringContextElem_Raw;
- using Raw::Raw;
-
- using Opaque = NixStringContextElem_Opaque;
- using DrvDeep = NixStringContextElem_DrvDeep;
- using Built = NixStringContextElem_Built;
-
- inline const Raw & raw() const & {
- return static_cast<const Raw &>(*this);
- }
- inline Raw & raw() & {
- return static_cast<Raw &>(*this);
- }
- inline Raw && raw() && {
- return static_cast<Raw &&>(*this);
- }
+ /**
+ * Path to a derivation and its entire build closure.
+ *
+ * The path doesn't just refer to derivation itself and its closure, but
+ * also all outputs of all derivations in that closure (including the
+ * root derivation).
+ *
+ * Encoded in the form ‘=<drvPath>’.
+ */
+ struct DrvDeep {
+ StorePath drvPath;
+
+ GENERATE_CMP(DrvDeep, me->drvPath);
+ };
+
+ /**
+ * Derivation output.
+ *
+ * Encoded in the form ‘!<output>!<drvPath>’.
+ */
+ using Built = SingleDerivedPath::Built;
+
+ using Raw = std::variant<
+ Opaque,
+ DrvDeep,
+ Built
+ >;
+
+ Raw raw;
+
+ GENERATE_CMP(NixStringContextElem, me->raw);
+
+ MAKE_WRAPPER_CONSTRUCTOR(NixStringContextElem);
 
  /**
  * Decode a context string, one of:

src/libstore/build/derivation-goal.cc

@@ -521,7 +521,7 @@ void DerivationGoal::inputsRealised()
  [&](const DerivationType::Impure &) {
  return true;
  }
- }, drvType.raw());
+ }, drvType.raw);
 
  if (resolveDrv && !fullDrv.inputDrvs.empty()) {
  experimentalFeatureSettings.require(Xp::CaDerivations);
@@ -996,10 +996,11 @@ void DerivationGoal::buildDone()
  }
 
  else {
+ assert(derivationType);
  st =
  dynamic_cast<NotDeterministic*>(&e) ? BuildResult::NotDeterministic :
  statusOk(status) ? BuildResult::OutputRejected :
- !derivationType.isSandboxed() || diskFull ? BuildResult::TransientFailure :
+ !derivationType->isSandboxed() || diskFull ? BuildResult::TransientFailure :
  BuildResult::PermanentFailure;
  }
 
@@ -1358,7 +1359,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/derivation-goal.hh

@@ -184,7 +184,7 @@ struct DerivationGoal : public Goal
  /**
  * The sort of derivation we are building.
  */
- DerivationType derivationType;
+ std::optional<DerivationType> derivationType;
 
  typedef void (DerivationGoal::*GoalState)();
  GoalState state;