refactor(3p/nix/libexpr): Remove the nix::Symbol default constructor r/1280

Having a default constructor for this causes a variety of annoying
situations across the codebase in which this is initialised to an
unexpected value, leading to constant guarding against those
conditions.

It turns out there's actually no intrinsic reason that this default
constructor needs to exist. The biggest one was addressed in CL/1138
and this commit cleans up the remaining bits.

Change-Id: I4a847f50bc90e72f028598196592a7d8730a4e01
Reviewed-on: https://cl.tvl.fyi/c/depot/+/1139
Reviewed-by: isomer <isomer@tvl.fyi>
Tested-by: BuildkiteCI

10 files changed, 50 insertions, 37 deletions

diff --git a/third_party/nix/src/libexpr/attr-set.cc b/third_party/nix/src/libexpr/attr-set.cc
index 8b34be6fd917..42ebe629b24a 100644
--- a/third_party/nix/src/libexpr/attr-set.cc
+++ b/third_party/nix/src/libexpr/attr-set.cc
@@ -59,7 +59,7 @@ Bindings::iterator Bindings::end() { return attributes_.end(); }
 
 void Bindings::merge(const Bindings& other) {
   for (auto& [key, value] : other.attributes_) {
-    this->attributes_[key] = value;
+    this->attributes_.insert_or_assign(key, value);
   }
 }
 
diff --git a/third_party/nix/src/libexpr/attr-set.hh b/third_party/nix/src/libexpr/attr-set.hh
index 31e5101d85a2..d4823cb19712 100644
--- a/third_party/nix/src/libexpr/attr-set.hh
+++ b/third_party/nix/src/libexpr/attr-set.hh
@@ -20,7 +20,6 @@ struct Attr {
   Pos* pos;      // TODO(tazjin): Who owns this?
   Attr(Symbol name, Value* value, Pos* pos = &noPos)
       : name(name), value(value), pos(pos){};
-  Attr() : pos(&noPos){};
 };
 
 // Convenience alias for the backing map, with the garbage-collecting
diff --git a/third_party/nix/src/libexpr/eval.cc b/third_party/nix/src/libexpr/eval.cc
index 6df7e66b6542..fb62130697ca 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 <optional>
 #include <variant>
 
 #include <absl/strings/match.h>
@@ -320,6 +321,7 @@ EvalState::EvalState(const Strings& _searchPath, const ref<Store>& store)
       sOutputHash(symbols.Create("outputHash")),
       sOutputHashAlgo(symbols.Create("outputHashAlgo")),
       sOutputHashMode(symbols.Create("outputHashMode")),
+      sDerivationNix(std::nullopt),
       repair(NoRepair),
       store(store),
       baseEnv(allocEnv(128)),
@@ -664,9 +666,9 @@ static inline void mkThunk(Value& v, Env& env, Expr* expr) {
 void EvalState::mkThunk_(Value& v, Expr* expr) { mkThunk(v, baseEnv, expr); }
 
 void EvalState::mkPos(Value& v, Pos* pos) {
-  if ((pos != nullptr) && pos->file.set()) {
+  if ((pos != nullptr) && pos->file.has_value() && pos->file.value().set()) {
     mkAttrs(v, 3);
-    mkString(*allocAttr(v, sFile), pos->file);
+    mkString(*allocAttr(v, sFile), pos->file.value());
     mkInt(*allocAttr(v, sLine), pos->line);
     mkInt(*allocAttr(v, sColumn), pos->column);
   } else {
@@ -847,7 +849,6 @@ void ExprAttrs::eval(EvalState& state, Env& env, Value& value) {
                      nameSym, i.pos, *j->second.pos);
     }
 
-    i.valueExpr->setName(nameSym);
     value.attrs->push_back(
         Attr(nameSym, i.valueExpr->maybeThunk(state, *dynamicEnv), &i.pos));
   }
@@ -936,7 +937,13 @@ void ExprSelect::eval(EvalState& state, Env& env, Value& v) {
     state.forceValue(*vAttrs, (pos2 != nullptr ? *pos2 : this->pos));
 
   } catch (Error& e) {
-    if ((pos2 != nullptr) && pos2->file != state.sDerivationNix) {
+    // This code relies on 'sDerivationNix' being correcty mutated at
+    // some prior point (it would previously otherwise have been a
+    // nullptr).
+    //
+    // We haven't seen this fail, so for now the contained value is
+    // just accessed at the risk of potentially crashing.
+    if ((pos2 != nullptr) && pos2->file != state.sDerivationNix.value()) {
       addErrorPrefix(e, "while evaluating the attribute '%1%' at %2%:\n",
                      showAttrPath(state, env, attrPath), *pos2);
     }
@@ -1792,8 +1799,8 @@ void EvalState::printStats() {
         auto list = topObj.list("functions");
         for (auto& i : functionCalls) {
           auto obj = list.object();
-          if (i.first->name.set()) {
-            obj.attr("name", (const std::string&)i.first->name);
+          if (i.first->name.has_value()) {
+            obj.attr("name", (const std::string&)i.first->name.value());
           } else {
             obj.attr("name", nullptr);
           }
diff --git a/third_party/nix/src/libexpr/eval.hh b/third_party/nix/src/libexpr/eval.hh
index 9e4e500b4745..11cc295b230d 100644
--- a/third_party/nix/src/libexpr/eval.hh
+++ b/third_party/nix/src/libexpr/eval.hh
@@ -62,10 +62,13 @@ class EvalState {
   SymbolTable symbols;
 
   const Symbol sWith, sOutPath, sDrvPath, sType, sMeta, sName, sValue, sSystem,
-      sOverrides, sOutputs, sOutputName, sIgnoreNulls, sFile, sLine, sColumn,
-      sFunctor, sToString, sRight, sWrong, sStructuredAttrs, sBuilder, sArgs,
-      sOutputHash, sOutputHashAlgo, sOutputHashMode;
-  Symbol sDerivationNix;
+      sOutputs, sOutputName, sIgnoreNulls, sFile, sLine, sColumn, sFunctor,
+      sToString, sRight, sWrong, sStructuredAttrs, sBuilder, sArgs, sOutputHash,
+      sOutputHashAlgo, sOutputHashMode;
+
+  // Symbol representing the path to the built-in 'derivation.nix'
+  // file, set during primops initialisation.
+  std::optional<Symbol> sDerivationNix;
 
   /* If set, force copying files to the Nix store even if they
      already exist there. */
diff --git a/third_party/nix/src/libexpr/nixexpr.cc b/third_party/nix/src/libexpr/nixexpr.cc
index 2de37f50e9b9..94e7d335cff0 100644
--- a/third_party/nix/src/libexpr/nixexpr.cc
+++ b/third_party/nix/src/libexpr/nixexpr.cc
@@ -187,11 +187,11 @@ void ExprConcatStrings::show(std::ostream& str) const {
 void ExprPos::show(std::ostream& str) const { str << "__curPos"; }
 
 std::ostream& operator<<(std::ostream& str, const Pos& pos) {
-  if (!pos) {
+  if (!pos || !pos.file.has_value()) {
     str << "undefined position";
   } else {
     str << (format(ANSI_BOLD "%1%" ANSI_NORMAL ":%2%:%3%") %
-            (std::string)pos.file % pos.line % pos.column)
+            (std::string)pos.file.value() % pos.line % pos.column)
                .str();
   }
   return str;
@@ -401,17 +401,12 @@ void ExprConcatStrings::bindVars(const StaticEnv& env) {
 void ExprPos::bindVars(const StaticEnv& env) {}
 
 /* Storing function names. */
-
-void Expr::setName(Symbol& name) {}
-
-void ExprLambda::setName(Symbol& name) {
-  this->name = name;
-  body->setName(name);
-}
+void ExprLambda::setName(Symbol& name) { this->name = name; }
 
 std::string ExprLambda::showNamePos() const {
   return (format("%1% at %2%") %
-          (name.set() ? "'" + (std::string)name + "'" : "anonymous function") %
+          (name.has_value() ? "'" + (std::string)name.value() + "'"
+                            : "anonymous function") %
           pos)
       .str();
 }
diff --git a/third_party/nix/src/libexpr/nixexpr.hh b/third_party/nix/src/libexpr/nixexpr.hh
index 7ec6f7f17f39..3bf9e4a1a77a 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 <optional>
 #include <variant>
 
 #include "libexpr/symbol-table.hh"
@@ -21,20 +22,28 @@ MakeError(RestrictedPathError, Error);
 /* Position objects. */
 
 struct Pos {
-  Symbol file;
+  std::optional<Symbol> file;
   unsigned int line, column;
-  Pos() : line(0), column(0){};
-  Pos(const Symbol& file, unsigned int line, unsigned int column)
+  Pos(const std::optional<Symbol>& file, unsigned int line, unsigned int column)
       : file(file), line(line), column(column){};
+
+  // TODO(tazjin): remove this - empty pos is never useful
+  Pos() : file(std::nullopt), line(0), column(0){};
+
   operator bool() const { return line != 0; }
+
   bool operator<(const Pos& p2) const {
+    if (!file.has_value()) {
+      return true;
+    }
+
     if (!line) {
       return p2.line;
     }
     if (!p2.line) {
       return false;
     }
-    int d = ((std::string)file).compare((std::string)p2.file);
+    int d = ((std::string)file.value()).compare((std::string)p2.file.value());
     if (d < 0) {
       return true;
     }
@@ -75,7 +84,6 @@ struct Expr {
   virtual void bindVars(const StaticEnv& env);
   virtual void eval(EvalState& state, Env& env, Value& v);
   virtual Value* maybeThunk(EvalState& state, Env& env);
-  virtual void setName(Symbol& name);
 };
 
 std::ostream& operator<<(std::ostream& str, const Expr& e);
@@ -219,8 +227,9 @@ struct Formals {
 };
 
 struct ExprLambda : Expr {
+ public:
   Pos pos;
-  Symbol name;
+  std::optional<Symbol> name;
   Symbol arg;
   bool matchAttrs;
   Formals* formals;
@@ -233,10 +242,11 @@ struct ExprLambda : Expr {
         formals(formals),
         body(body) {
     if (!arg.empty() && formals &&
-        formals->argNames.find(arg) != formals->argNames.end())
+        formals->argNames.find(arg) != formals->argNames.end()) {
       throw ParseError(
           format("duplicate formal function argument '%1%' at %2%") % arg %
           pos);
+    }
   };
   void setName(Symbol& name);
   std::string showNamePos() const;
diff --git a/third_party/nix/src/libexpr/parser.y b/third_party/nix/src/libexpr/parser.y
index eb3f92db7844..2ea84fea66b9 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 <optional>
 #include <variant>
 #include "libutil/util.hh"
 #include "libexpr/nixexpr.hh"
@@ -28,9 +29,9 @@ namespace nix {
     {
         EvalState & state;
         SymbolTable & symbols;
-        Expr * result;
+        Expr* result;
         Path basePath;
-        Symbol path;
+        std::optional<Symbol> path;
         std::string error;
         Symbol sLetBody;
         ParseData(EvalState & state)
@@ -139,7 +140,6 @@ static void addAttr(ExprAttrs * attrs, AttrPath & attrPath,
         } else {
             // This attr path is not defined. Let's create it.
             attrs->attrs[*sym] = ExprAttrs::AttrDef(e, pos);
-            e->setName(*sym);
         }
     } else {
         // Same caveat as the identical line above.
@@ -502,8 +502,9 @@ attrpath
       if (str) {
           $$->push_back(AttrName(str->s));
           delete str;
-      } else
+      } else {
           $$->push_back(AttrName($3));
+      }
     }
   | attr { $$ = new std::vector<AttrName>; $$->push_back(AttrName(data->symbols.Create($1))); }
   | string_attr
diff --git a/third_party/nix/src/libexpr/primops.cc b/third_party/nix/src/libexpr/primops.cc
index b238da762d22..d96af55f5caf 100644
--- a/third_party/nix/src/libexpr/primops.cc
+++ b/third_party/nix/src/libexpr/primops.cc
@@ -234,7 +234,7 @@ void prim_exec(EvalState& state, const Pos& pos, Value** args, Value& v) {
   auto output = runProgram(program, true, commandArgs);
   Expr* parsed;
   try {
-    parsed = state.parseExprFromString(output, pos.file);
+    parsed = state.parseExprFromString(output, pos.file.value());
   } catch (Error& e) {
     e.addPrefix(format("While parsing the output from '%1%', at %2%\n") %
                 program % pos);
diff --git a/third_party/nix/src/libexpr/symbol-table.hh b/third_party/nix/src/libexpr/symbol-table.hh
index efc2314b75e8..dcb44d32f623 100644
--- a/third_party/nix/src/libexpr/symbol-table.hh
+++ b/third_party/nix/src/libexpr/symbol-table.hh
@@ -13,8 +13,6 @@ class Symbol {
   friend class SymbolTable;
 
  public:
-  Symbol() : s(0){};
-
   bool operator==(const Symbol& s2) const { return s == s2.s; }
 
   bool operator!=(const Symbol& s2) const { return s != s2.s; }
diff --git a/third_party/nix/src/libexpr/value-to-xml.cc b/third_party/nix/src/libexpr/value-to-xml.cc
index 2a46ac84be61..ea1a85a89719 100644
--- a/third_party/nix/src/libexpr/value-to-xml.cc
+++ b/third_party/nix/src/libexpr/value-to-xml.cc
@@ -20,7 +20,7 @@ static void printValueAsXML(EvalState& state, bool strict, bool location,
                             PathSet& drvsSeen);
 
 static void posToXML(XMLAttrs& xmlAttrs, const Pos& pos) {
-  xmlAttrs["path"] = pos.file;
+  xmlAttrs["path"] = pos.file.value();
   xmlAttrs["line"] = (format("%1%") % pos.line).str();
   xmlAttrs["column"] = (format("%1%") % pos.column).str();
 }