refactor(3p/nix/libexpr): Make nix::AttrName a std::variant r/1278

nix:AttrName was one of the few classes that relied on the default
constructor of nix::Symbol (which I am trying to remove in a separate
change).

The class essentially represents the name of an attribute in a set,
which is either just a string expression or a dynamically evaluated
expression (e.g. string interpolation).

Previously it would be constructed by only setting one of the fields
and defaulting the other, now it is an explicit std::variant.

Note that there are several code paths where not all eventualities are
handled and this code is bug-for-bug compatible with those, except
that unknown conditions (which should never work) are now throwing
instead of silently doing ... something.

The language tests pass with this change, and the depot derivations
that I tested with evaluated successfully.

Change-Id: Icf1ee60a5f8308f4ab18a82749e00cf37a938a8f
Reviewed-on: https://cl.tvl.fyi/c/depot/+/1138
Reviewed-by: edef <edef@edef.eu>
Reviewed-by: glittershark <grfn@gws.fyi>
Tested-by: BuildkiteCI

7 files changed, 67 insertions, 53 deletions

diff --git a/third_party/nix/src/libexpr/eval.cc b/third_party/nix/src/libexpr/eval.cc
index 6d77928d7d..6df7e66b65 100644
--- a/third_party/nix/src/libexpr/eval.cc
+++ b/third_party/nix/src/libexpr/eval.cc
@@ -6,6 +6,7 @@
 #include <fstream>
 #include <iostream>
 #include <new>
+#include <variant>
 
 #include <absl/strings/match.h>
 #include <gc/gc.h>
@@ -24,6 +25,7 @@
 #include "libutil/hash.hh"
 #include "libutil/json.hh"
 #include "libutil/util.hh"
+#include "libutil/visitor.hh"
 
 namespace nix {
 
@@ -185,13 +187,15 @@ static void* oomHandler(size_t requested) {
 #endif
 
 static Symbol getName(const AttrName& name, EvalState& state, Env& env) {
-  if (name.symbol.set()) {
-    return name.symbol;
-  }
-  Value nameValue;
-  name.expr->eval(state, env, nameValue);
-  state.forceStringNoCtx(nameValue);
-  return state.symbols.Create(nameValue.string.s);
+  return std::visit(
+      util::overloaded{[&](const Symbol& name) -> Symbol { return name; },
+                       [&](Expr* expr) -> Symbol {
+                         Value nameValue;
+                         expr->eval(state, env, nameValue);
+                         state.forceStringNoCtx(nameValue);
+                         return state.symbols.Create(nameValue.string.s);
+                       }},
+      name);
 }
 
 static bool gcInitialised = false;
@@ -890,12 +894,7 @@ static std::string showAttrPath(EvalState& state, Env& env,
     } else {
       first = false;
     }
-    try {
-      out << getName(i, state, env);
-    } catch (Error& e) {
-      assert(!i.symbol.set());
-      out << "\"${" << *i.expr << "}\"";
-    }
+    out << getName(i, state, env);
   }
   return out.str();
 }
diff --git a/third_party/nix/src/libexpr/nixexpr.cc b/third_party/nix/src/libexpr/nixexpr.cc
index 28d8eee7a7..2de37f50e9 100644
--- a/third_party/nix/src/libexpr/nixexpr.cc
+++ b/third_party/nix/src/libexpr/nixexpr.cc
@@ -1,9 +1,11 @@
 #include "libexpr/nixexpr.hh"
 
 #include <cstdlib>
+#include <variant>
 
 #include "libstore/derivations.hh"
 #include "libutil/util.hh"
+#include "libutil/visitor.hh"
 
 namespace nix {
 
@@ -198,17 +200,17 @@ std::ostream& operator<<(std::ostream& str, const Pos& pos) {
 std::string showAttrPath(const AttrPath& attrPath) {
   std::ostringstream out;
   bool first = true;
-  for (auto& i : attrPath) {
+  for (auto& attr : attrPath) {
     if (!first) {
       out << '.';
     } else {
       first = false;
     }
-    if (i.symbol.set()) {
-      out << i.symbol;
-    } else {
-      out << "\"${" << *i.expr << "}\"";
-    }
+
+    std::visit(util::overloaded{
+                   [&](const Symbol& sym) { out << sym; },
+                   [&](const Expr* expr) { out << "\"${" << *expr << "}\""; }},
+               attr);
   }
   return out.str();
 }
@@ -268,8 +270,8 @@ void ExprSelect::bindVars(const StaticEnv& env) {
     def->bindVars(env);
   }
   for (auto& i : attrPath) {
-    if (!i.symbol.set()) {
-      i.expr->bindVars(env);
+    if (auto* expr = std::get_if<Expr*>(&i)) {
+      (*expr)->bindVars(env);
     }
   }
 }
@@ -277,8 +279,8 @@ void ExprSelect::bindVars(const StaticEnv& env) {
 void ExprOpHasAttr::bindVars(const StaticEnv& env) {
   e->bindVars(env);
   for (auto& i : attrPath) {
-    if (!i.symbol.set()) {
-      i.expr->bindVars(env);
+    if (auto* expr = std::get_if<Expr*>(&i)) {
+      (*expr)->bindVars(env);
     }
   }
 }
diff --git a/third_party/nix/src/libexpr/nixexpr.hh b/third_party/nix/src/libexpr/nixexpr.hh
index 0bf5245181..7ec6f7f17f 100644
--- a/third_party/nix/src/libexpr/nixexpr.hh
+++ b/third_party/nix/src/libexpr/nixexpr.hh
@@ -1,6 +1,7 @@
 #pragma once
 
 #include <map>
+#include <variant>
 
 #include "libexpr/symbol-table.hh"
 #include "libexpr/value.hh"
@@ -60,12 +61,7 @@ class EvalState;
 struct StaticEnv;
 
 /* An attribute path is a sequence of attribute names. */
-struct AttrName {
-  Symbol symbol;
-  Expr* expr;
-  AttrName(const Symbol& s) : symbol(s){};
-  AttrName(Expr* e) : expr(e){};
-};
+using AttrName = std::variant<Symbol, Expr*>;
 
 typedef std::vector<AttrName> AttrPath;
 
diff --git a/third_party/nix/src/libexpr/parser.y b/third_party/nix/src/libexpr/parser.y
index fe8759f3c6..eb3f92db78 100644
--- a/third_party/nix/src/libexpr/parser.y
+++ b/third_party/nix/src/libexpr/parser.y
@@ -16,6 +16,7 @@
 #ifndef BISON_HEADER
 #define BISON_HEADER
 
+#include <variant>
 #include "libutil/util.hh"
 #include "libexpr/nixexpr.hh"
 #include "libexpr/eval.hh"
@@ -84,30 +85,39 @@ static void addAttr(ExprAttrs * attrs, AttrPath & attrPath,
     // Checking attrPath validity.
     // ===========================
     for (i = attrPath.begin(); i + 1 < attrPath.end(); i++) {
-        if (i->symbol.set()) {
-            ExprAttrs::AttrDefs::iterator j = attrs->attrs.find(i->symbol);
+        if (const auto* sym = std::get_if<Symbol>(&(*i)); sym && sym->set()) {
+            ExprAttrs::AttrDefs::iterator j = attrs->attrs.find(*sym);
             if (j != attrs->attrs.end()) {
                 if (!j->second.inherited) {
                     ExprAttrs * attrs2 = dynamic_cast<ExprAttrs *>(j->second.e);
                     if (!attrs2) { dupAttr(attrPath, pos, j->second.pos); }
                     attrs = attrs2;
-                } else
+                } else {
                     dupAttr(attrPath, pos, j->second.pos);
+                }
             } else {
-                ExprAttrs * nested = new ExprAttrs;
-                attrs->attrs[i->symbol] = ExprAttrs::AttrDef(nested, pos);
+                ExprAttrs* nested = new ExprAttrs;
+                attrs->attrs[*sym] = ExprAttrs::AttrDef(nested, pos);
                 attrs = nested;
             }
         } else {
-            ExprAttrs *nested = new ExprAttrs;
-            attrs->dynamicAttrs.push_back(ExprAttrs::DynamicAttrDef(i->expr, nested, pos));
-            attrs = nested;
+          // Yes, this code does not handle all conditions
+          // exhaustively. We use std::get to throw if the condition
+          // that isn't covered happens, which is potentially a
+          // behaviour change from the previous default constructed
+          // Symbol. It should alert us about anything untoward going
+          // on here.
+          auto* expr = std::get<Expr*>(*i);
+
+          ExprAttrs *nested = new ExprAttrs;
+          attrs->dynamicAttrs.push_back(ExprAttrs::DynamicAttrDef(expr, nested, pos));
+          attrs = nested;
         }
     }
     // Expr insertion.
     // ==========================
-    if (i->symbol.set()) {
-        ExprAttrs::AttrDefs::iterator j = attrs->attrs.find(i->symbol);
+    if (auto* sym = std::get_if<Symbol>(&(*i)); sym && sym->set()) {
+        ExprAttrs::AttrDefs::iterator j = attrs->attrs.find(*sym);
         if (j != attrs->attrs.end()) {
             // This attr path is already defined. However, if both
             // e and the expr pointed by the attr path are two attribute sets,
@@ -118,8 +128,9 @@ static void addAttr(ExprAttrs * attrs, AttrPath & attrPath,
             if (jAttrs && ae) {
                 for (auto & ad : ae->attrs) {
                     auto j2 = jAttrs->attrs.find(ad.first);
-                    if (j2 != jAttrs->attrs.end()) // Attr already defined in iAttrs, error.
+                    if (j2 != jAttrs->attrs.end()) {// Attr already defined in iAttrs, error.
                         dupAttr(ad.first, j2->second.pos, ad.second.pos);
+                    }
                     jAttrs->attrs[ad.first] = ad.second;
                 }
             } else {
@@ -127,11 +138,13 @@ static void addAttr(ExprAttrs * attrs, AttrPath & attrPath,
             }
         } else {
             // This attr path is not defined. Let's create it.
-            attrs->attrs[i->symbol] = ExprAttrs::AttrDef(e, pos);
-            e->setName(i->symbol);
+            attrs->attrs[*sym] = ExprAttrs::AttrDef(e, pos);
+            e->setName(*sym);
         }
     } else {
-        attrs->dynamicAttrs.push_back(ExprAttrs::DynamicAttrDef(i->expr, e, pos));
+        // Same caveat as the identical line above.
+        auto* expr = std::get<Expr*>(*i);
+        attrs->dynamicAttrs.push_back(ExprAttrs::DynamicAttrDef(expr, e, pos));
     }
 }
 
@@ -444,19 +457,23 @@ binds
   | binds INHERIT attrs ';'
     { $$ = $1;
       for (auto & i : *$3) {
-          if ($$->attrs.find(i.symbol) != $$->attrs.end())
-              dupAttr(i.symbol, makeCurPos(@3, data), $$->attrs[i.symbol].pos);
+          auto sym = std::get<Symbol>(i);
+          if ($$->attrs.find(sym) != $$->attrs.end()) {
+              dupAttr(sym, makeCurPos(@3, data), $$->attrs[sym].pos);
+          }
           Pos pos = makeCurPos(@3, data);
-          $$->attrs[i.symbol] = ExprAttrs::AttrDef(new ExprVar(CUR_POS, i.symbol), pos, true);
+          $$->attrs[sym] = ExprAttrs::AttrDef(new ExprVar(CUR_POS, sym), pos, true);
       }
     }
   | binds INHERIT '(' expr ')' attrs ';'
     { $$ = $1;
       /* !!! Should ensure sharing of the expression in $4. */
       for (auto & i : *$6) {
-          if ($$->attrs.find(i.symbol) != $$->attrs.end())
-              dupAttr(i.symbol, makeCurPos(@6, data), $$->attrs[i.symbol].pos);
-          $$->attrs[i.symbol] = ExprAttrs::AttrDef(new ExprSelect(CUR_POS, $4, i.symbol), makeCurPos(@6, data));
+          auto sym = std::get<Symbol>(i);
+          if ($$->attrs.find(sym) != $$->attrs.end()) {
+            dupAttr(sym, makeCurPos(@6, data), $$->attrs[sym].pos);
+          }
+          $$->attrs[sym] = ExprAttrs::AttrDef(new ExprSelect(CUR_POS, $4, sym), makeCurPos(@6, data));
       }
     }
   | { $$ = new ExprAttrs; }
diff --git a/third_party/nix/src/libexpr/symbol-table.hh b/third_party/nix/src/libexpr/symbol-table.hh
index c4c1163bed..efc2314b75 100644
--- a/third_party/nix/src/libexpr/symbol-table.hh
+++ b/third_party/nix/src/libexpr/symbol-table.hh
@@ -21,7 +21,7 @@ class Symbol {
 
   bool operator<(const Symbol& s2) const { return *s < *s2.s; }
 
-  operator const std::string&() const { return *s; }
+  operator const std::string &() const { return *s; }
 
   bool set() const { return s; }
 
diff --git a/third_party/nix/src/libutil/config.cc b/third_party/nix/src/libutil/config.cc
index 20c70c823f..5d28b7a20f 100644
--- a/third_party/nix/src/libutil/config.cc
+++ b/third_party/nix/src/libutil/config.cc
@@ -102,8 +102,8 @@ void AbstractConfig::applyConfigFile(const Path& path) {
       }
 
       // TODO(tazjin): absl::string_view after path functions are fixed.
-      std::vector<std::string> tokens =
-          absl::StrSplit(line, absl::ByAnyChar(" \t\n\r"), absl::SkipWhitespace());
+      std::vector<std::string> tokens = absl::StrSplit(
+          line, absl::ByAnyChar(" \t\n\r"), absl::SkipWhitespace());
       if (tokens.empty()) {
         continue;
       }
diff --git a/third_party/nix/src/nix-daemon/nix-daemon.cc b/third_party/nix/src/nix-daemon/nix-daemon.cc
index c7967307c1..da3d909f18 100644
--- a/third_party/nix/src/nix-daemon/nix-daemon.cc
+++ b/third_party/nix/src/nix-daemon/nix-daemon.cc
@@ -16,6 +16,7 @@
 #include <unistd.h>
 
 #include "libmain/shared.hh"
+#include "libproto/worker.pb.h"
 #include "libstore/derivations.hh"
 #include "libstore/globals.hh"
 #include "libstore/local-store.hh"
@@ -27,7 +28,6 @@
 #include "libutil/serialise.hh"
 #include "libutil/util.hh"
 #include "nix/legacy.hh"
-#include "libproto/worker.pb.h"
 
 using namespace nix;