refactor(tvix): always pass Bindings by ptr, use shared/unique_ptr r/1658

Value now carries a shared_ptr<Bindings>, and all Bindings constructors return a unique_ptr<Bindings>.

The test that wanted to compare two Bindings by putting them into Values has been modified to use the new Equal() method on Bindings (extracted from EvalState).

Change-Id: I8dfb60e65fdabb717e3b3e5d56d5b3fc82f70883
Reviewed-on: https://cl.tvl.fyi/c/depot/+/1744
Tested-by: BuildkiteCI
Reviewed-by: glittershark <grfn@gws.fyi>
Reviewed-by: tazjin <mail@tazj.in>

11 files changed, 77 insertions, 54 deletions

diff --git a/third_party/nix/src/libexpr/attr-path.cc b/third_party/nix/src/libexpr/attr-path.cc
index a267e82b6ebe..86ebeec2fb15 100644
--- a/third_party/nix/src/libexpr/attr-path.cc
+++ b/third_party/nix/src/libexpr/attr-path.cc
@@ -39,7 +39,7 @@ static Strings parseAttrPath(const std::string& s) {
 }
 
 Value* findAlongAttrPath(EvalState& state, const std::string& attrPath,
-                         Bindings& autoArgs, Value& vIn) {
+                         Bindings* autoArgs, Value& vIn) {
   Strings tokens = parseAttrPath(attrPath);
 
   Error attrError =
diff --git a/third_party/nix/src/libexpr/attr-path.hh b/third_party/nix/src/libexpr/attr-path.hh
index a71ceb9a06cd..97170be84098 100644
--- a/third_party/nix/src/libexpr/attr-path.hh
+++ b/third_party/nix/src/libexpr/attr-path.hh
@@ -8,6 +8,6 @@
 namespace nix {
 
 Value* findAlongAttrPath(EvalState& state, const std::string& attrPath,
-                         Bindings& autoArgs, Value& vIn);
+                         Bindings* autoArgs, Value& vIn);
 
 }
diff --git a/third_party/nix/src/libexpr/attr-set.cc b/third_party/nix/src/libexpr/attr-set.cc
index e9e2a2baded0..808ffce176ee 100644
--- a/third_party/nix/src/libexpr/attr-set.cc
+++ b/third_party/nix/src/libexpr/attr-set.cc
@@ -9,8 +9,6 @@
 
 namespace nix {
 
-static Bindings ZERO_BINDINGS;
-
 // This function inherits its name from previous implementations, in
 // which Bindings was backed by an array of elements which was scanned
 // linearly.
@@ -22,8 +20,6 @@ static Bindings ZERO_BINDINGS;
 // This behaviour is mimicked by using .insert(), which will *not*
 // override existing values.
 void Bindings::push_back(const Attr& attr) {
-  assert(this != &ZERO_BINDINGS);
-
   auto [_, inserted] = attributes_.insert({attr.name, attr});
 
   if (!inserted) {
@@ -51,20 +47,48 @@ Bindings::iterator Bindings::find(const Symbol& name) {
   return attributes_.find(name);
 }
 
-Bindings::iterator Bindings::begin() { return attributes_.begin(); }
+bool Bindings::Equal(const Bindings* other, EvalState& state) const {
+  if (this == other) {
+    return true;
+  }
+
+  if (this->attributes_.size() != other->attributes_.size()) {
+    return false;
+  }
+
+  Bindings::const_iterator i;
+  Bindings::const_iterator j;
+  for (i = this->cbegin(), j = other->cbegin(); i != this->cend(); ++i, ++j) {
+    if (i->second.name != j->second.name ||
+        !state.eqValues(*i->second.value, *j->second.value)) {
+      return false;
+    }
+  }
+
+  return true;
+}
 
+Bindings::iterator Bindings::begin() { return attributes_.begin(); }
 Bindings::iterator Bindings::end() { return attributes_.end(); }
 
-Bindings* Bindings::NewGC(size_t capacity) {
+Bindings::const_iterator Bindings::cbegin() const {
+  return attributes_.cbegin();
+}
+
+Bindings::const_iterator Bindings::cend() const { return attributes_.cend(); }
+
+std::unique_ptr<Bindings> Bindings::New(size_t capacity) {
   if (capacity == 0) {
-    return &ZERO_BINDINGS;
+    // TODO(tazjin): A lot of 0-capacity Bindings are allocated.
+    // It would be nice to optimize that.
   }
 
-  return new Bindings;
+  return std::make_unique<Bindings>();
 }
 
-Bindings* Bindings::Merge(const Bindings& lhs, const Bindings& rhs) {
-  auto bindings = NewGC(lhs.size() + rhs.size());
+std::unique_ptr<Bindings> Bindings::Merge(const Bindings& lhs,
+                                          const Bindings& rhs) {
+  auto bindings = New(lhs.size() + rhs.size());
 
   // Values are merged by inserting the entire iterator range of both
   // input sets. The right-hand set (the values of which take
@@ -81,7 +105,7 @@ Bindings* Bindings::Merge(const Bindings& lhs, const Bindings& rhs) {
 void EvalState::mkAttrs(Value& v, size_t capacity) {
   clearValue(v);
   v.type = tAttrs;
-  v.attrs = Bindings::NewGC(capacity);
+  v.attrs = Bindings::New(capacity);
   nrAttrsets++;
   nrAttrsInAttrsets += capacity;
 }
diff --git a/third_party/nix/src/libexpr/attr-set.hh b/third_party/nix/src/libexpr/attr-set.hh
index 6397008517d2..c02f3930fda5 100644
--- a/third_party/nix/src/libexpr/attr-set.hh
+++ b/third_party/nix/src/libexpr/attr-set.hh
@@ -25,15 +25,17 @@ using AttributeMap = absl::btree_map<Symbol, Attr>;
 
 class Bindings {
  public:
-  typedef AttributeMap::iterator iterator;
+  using iterator = AttributeMap::iterator;
+  using const_iterator = AttributeMap::const_iterator;
 
   // Allocate a new attribute set that is visible to the garbage
   // collector.
-  static Bindings* NewGC(size_t capacity = 0);
+  static std::unique_ptr<Bindings> New(size_t capacity = 0);
 
   // Create a new attribute set by merging two others. This is used to
   // implement the `//` operator in Nix.
-  static Bindings* Merge(const Bindings& lhs, const Bindings& rhs);
+  static std::unique_ptr<Bindings> Merge(const Bindings& lhs,
+                                         const Bindings& rhs);
 
   // Return the number of contained elements.
   size_t size() const;
@@ -44,11 +46,18 @@ class Bindings {
   // Insert, but do not replace, values in the attribute set.
   void push_back(const Attr& attr);
 
+  // Are these two attribute sets deeply equal?
+  // Note: Does not special-case derivations. Use state.eqValues() to check
+  // attrsets that may be derivations.
+  bool Equal(const Bindings* other, EvalState& state) const;
+
   // Look up a specific element of the attribute set.
   iterator find(const Symbol& name);
 
   iterator begin();
+  const_iterator cbegin() const;
   iterator end();
+  const_iterator cend() const;
 
   // Returns the elements of the attribute set as a vector, sorted
   // lexicographically by keys.
diff --git a/third_party/nix/src/libexpr/common-eval-args.cc b/third_party/nix/src/libexpr/common-eval-args.cc
index b3e3782732c8..e36457f4f8ff 100644
--- a/third_party/nix/src/libexpr/common-eval-args.cc
+++ b/third_party/nix/src/libexpr/common-eval-args.cc
@@ -32,8 +32,8 @@ MixEvalArgs::MixEvalArgs() {
       .handler([&](const std::string& s) { searchPath.push_back(s); });
 }
 
-Bindings* MixEvalArgs::getAutoArgs(EvalState& state) {
-  Bindings* res = Bindings::NewGC(autoArgs.size());
+std::unique_ptr<Bindings> MixEvalArgs::getAutoArgs(EvalState& state) {
+  auto res = Bindings::New(autoArgs.size());
   for (auto& i : autoArgs) {
     Value* v = state.allocValue();
     if (i.second[0] == 'E') {
diff --git a/third_party/nix/src/libexpr/common-eval-args.hh b/third_party/nix/src/libexpr/common-eval-args.hh
index be7126c039df..9b0a77c6f7cd 100644
--- a/third_party/nix/src/libexpr/common-eval-args.hh
+++ b/third_party/nix/src/libexpr/common-eval-args.hh
@@ -11,7 +11,7 @@ class Bindings;
 struct MixEvalArgs : virtual Args {
   MixEvalArgs();
 
-  Bindings* getAutoArgs(EvalState& state);
+  std::unique_ptr<Bindings> getAutoArgs(EvalState& state);
 
   Strings searchPath;
 
diff --git a/third_party/nix/src/libexpr/eval.cc b/third_party/nix/src/libexpr/eval.cc
index 4ae3661b0b15..569e04e3a712 100644
--- a/third_party/nix/src/libexpr/eval.cc
+++ b/third_party/nix/src/libexpr/eval.cc
@@ -1096,7 +1096,7 @@ void EvalState::callFunction(Value& fun, Value& arg, Value& v, const Pos& pos) {
 // prevents tail-call optimisation.
 void EvalState::incrFunctionCall(ExprLambda* fun) { functionCalls[fun]++; }
 
-void EvalState::autoCallFunction(Bindings& args, Value& fun, Value& res) {
+void EvalState::autoCallFunction(Bindings* args, Value& fun, Value& res) {
   forceValue(fun);
 
   if (fun.type == tAttrs) {
@@ -1118,8 +1118,8 @@ void EvalState::autoCallFunction(Bindings& args, Value& fun, Value& res) {
   mkAttrs(*actualArgs, fun.lambda.fun->formals->formals.size());
 
   for (auto& i : fun.lambda.fun->formals->formals) {
-    Bindings::iterator j = args.find(i.name);
-    if (j != args.end()) {
+    Bindings::iterator j = args->find(i.name);
+    if (j != args->end()) {
       actualArgs->attrs->push_back(j->second);
     } else if (i.def == nullptr) {
       throwTypeError(
@@ -1615,22 +1615,7 @@ bool EvalState::eqValues(Value& v1, Value& v2) {
         }
       }
 
-      if (v1.attrs->size() != v2.attrs->size()) {
-        return false;
-      }
-
-      /* Otherwise, compare the attributes one by one. */
-      Bindings::iterator i;
-      Bindings::iterator j;
-      for (i = v1.attrs->begin(), j = v2.attrs->begin(); i != v1.attrs->end();
-           ++i, ++j) {
-        if (i->second.name != j->second.name ||
-            !eqValues(*i->second.value, *j->second.value)) {
-          return false;
-        }
-      }
-
-      return true;
+      return v1.attrs->Equal(v2.attrs.get(), *this);
     }
 
     /* Functions are incomparable. */
@@ -1811,8 +1796,8 @@ size_t valueSize(const Value& v) {
         sz += doString(v.path);
         break;
       case tAttrs:
-        if (seenBindings.find(v.attrs) == seenBindings.end()) {
-          seenBindings.insert(v.attrs);
+        if (seenBindings.find(v.attrs.get()) == seenBindings.end()) {
+          seenBindings.insert(v.attrs.get());
           sz += sizeof(Bindings);
           for (const auto& i : *v.attrs) {
             sz += doValue(*i.second.value);
diff --git a/third_party/nix/src/libexpr/eval.hh b/third_party/nix/src/libexpr/eval.hh
index f3e38cfb3cb6..aa7af777a1f1 100644
--- a/third_party/nix/src/libexpr/eval.hh
+++ b/third_party/nix/src/libexpr/eval.hh
@@ -247,8 +247,9 @@ class EvalState {
   void callPrimOp(Value& fun, Value& arg, Value& v, const Pos& pos);
 
   /* Automatically call a function for which each argument has a
-     default value or has a binding in the `args' map. */
-  void autoCallFunction(Bindings& args, Value& fun, Value& res);
+     default value or has a binding in the `args' map.  'args' need
+     not live past the end of the call. */
+  void autoCallFunction(Bindings* args, Value& fun, Value& res);
 
   /* Allocation primitives. */
   Value* allocValue();
diff --git a/third_party/nix/src/libexpr/get-drvs.cc b/third_party/nix/src/libexpr/get-drvs.cc
index 02ddae1f886c..d81fb5dfadc8 100644
--- a/third_party/nix/src/libexpr/get-drvs.cc
+++ b/third_party/nix/src/libexpr/get-drvs.cc
@@ -4,6 +4,7 @@
 #include <regex>
 #include <utility>
 
+#include <absl/container/flat_hash_set.h>
 #include <absl/strings/numbers.h>
 #include <glog/logging.h>
 
@@ -13,7 +14,8 @@
 
 namespace nix {
 
-DrvInfo::DrvInfo(EvalState& state, std::string attrPath, Bindings* attrs)
+DrvInfo::DrvInfo(EvalState& state, std::string attrPath,
+                 std::shared_ptr<Bindings> attrs)
     : state(&state), attrs(attrs), attrPath(std::move(attrPath)) {}
 
 DrvInfo::DrvInfo(EvalState& state, const ref<Store>& store,
@@ -161,7 +163,7 @@ std::string DrvInfo::queryOutputName() const {
 
 Bindings* DrvInfo::getMeta() {
   if (meta != nullptr) {
-    return meta;
+    return meta.get();
   }
   if (attrs == nullptr) {
     return nullptr;
@@ -172,7 +174,7 @@ Bindings* DrvInfo::getMeta() {
   }
   state->forceAttrs(*a->second.value, *a->second.pos);
   meta = a->second.value->attrs;
-  return meta;
+  return meta.get();
 }
 
 StringSet DrvInfo::queryMetaNames() {
@@ -292,8 +294,8 @@ bool DrvInfo::queryMetaBool(const std::string& name, bool def) {
 }
 
 void DrvInfo::setMeta(const std::string& name, Value* v) {
-  Bindings* old = getMeta();
-  meta = Bindings::NewGC(old->size() + 1);
+  std::shared_ptr<Bindings> old = meta;
+  meta = std::shared_ptr<Bindings>(Bindings::New(old->size() + 1).release());
   Symbol sym = state->symbols.Create(name);
   if (old != nullptr) {
     for (auto i : *old) {
@@ -308,7 +310,7 @@ void DrvInfo::setMeta(const std::string& name, Value* v) {
 }
 
 /* Cache for already considered attrsets. */
-using Done = std::set<Bindings*>;
+using Done = absl::flat_hash_set<std::shared_ptr<Bindings>>;
 
 /* Evaluate value `v'.  If it evaluates to a set of type `derivation',
    then put information about it in `drvs' (unless it's already in `done').
@@ -364,7 +366,7 @@ static std::string addToPath(const std::string& s1, const std::string& s2) {
 static std::regex attrRegex("[A-Za-z_][A-Za-z0-9-_+]*");
 
 static void getDerivations(EvalState& state, Value& vIn,
-                           const std::string& pathPrefix, Bindings& autoArgs,
+                           const std::string& pathPrefix, Bindings* autoArgs,
                            DrvInfos& drvs, Done& done,
                            bool ignoreAssertionFailures) {
   Value v;
@@ -434,7 +436,7 @@ static void getDerivations(EvalState& state, Value& vIn,
 }
 
 void getDerivations(EvalState& state, Value& v, const std::string& pathPrefix,
-                    Bindings& autoArgs, DrvInfos& drvs,
+                    Bindings* autoArgs, DrvInfos& drvs,
                     bool ignoreAssertionFailures) {
   Done done;
   getDerivations(state, v, pathPrefix, autoArgs, drvs, done,
diff --git a/third_party/nix/src/libexpr/get-drvs.hh b/third_party/nix/src/libexpr/get-drvs.hh
index 608713ed1b6b..b17efe8b3355 100644
--- a/third_party/nix/src/libexpr/get-drvs.hh
+++ b/third_party/nix/src/libexpr/get-drvs.hh
@@ -23,7 +23,8 @@ struct DrvInfo {
 
   bool failed = false;  // set if we get an AssertionError
 
-  Bindings *attrs = nullptr, *meta = nullptr;
+  std::shared_ptr<Bindings> attrs = nullptr;
+  std::shared_ptr<Bindings> meta = nullptr;
 
   Bindings* getMeta();
 
@@ -33,7 +34,8 @@ struct DrvInfo {
   std::string attrPath; /* path towards the derivation */
 
   DrvInfo(EvalState& state) : state(&state){};
-  DrvInfo(EvalState& state, std::string attrPath, Bindings* attrs);
+  DrvInfo(EvalState& state, std::string attrPath,
+          std::shared_ptr<Bindings> attrs);
   DrvInfo(EvalState& state, const ref<Store>& store,
           const std::string& drvPathWithOutputs);
 
@@ -75,7 +77,7 @@ std::optional<DrvInfo> getDerivation(EvalState& state, Value& v,
                                      bool ignoreAssertionFailures);
 
 void getDerivations(EvalState& state, Value& v, const std::string& pathPrefix,
-                    Bindings& autoArgs, DrvInfos& drvs,
+                    Bindings* autoArgs, DrvInfos& drvs,
                     bool ignoreAssertionFailures);
 
 }  // namespace nix
diff --git a/third_party/nix/src/libexpr/value.hh b/third_party/nix/src/libexpr/value.hh
index 85ce9e7a1ba0..82360c566813 100644
--- a/third_party/nix/src/libexpr/value.hh
+++ b/third_party/nix/src/libexpr/value.hh
@@ -95,7 +95,7 @@ struct Value {
     bool boolean;
     NixString string;
     const char* path;
-    Bindings* attrs;
+    std::shared_ptr<Bindings> attrs;
     NixList* list;
     NixThunk thunk;
     NixApp app;  // TODO(tazjin): "app"?