refactor(tvix/libexpr): Remove Bindings::SortedByKeys() r/2028

Since we don't have a Bindings implementation with unstable order this
function is not required, as its callers can just iterate over the
attributes instead.

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

5 files changed, 47 insertions, 65 deletions

diff --git a/third_party/nix/src/libexpr/attr-set.cc b/third_party/nix/src/libexpr/attr-set.cc
index 808ffce176..b1617c981f 100644
--- a/third_party/nix/src/libexpr/attr-set.cc
+++ b/third_party/nix/src/libexpr/attr-set.cc
@@ -32,17 +32,6 @@ size_t Bindings::size() const { return attributes_.size(); }
 
 bool Bindings::empty() { return attributes_.empty(); }
 
-std::vector<const Attr*> Bindings::SortedByKeys() {
-  std::vector<const Attr*> res;
-  res.reserve(attributes_.size());
-
-  for (const auto& [key, value] : attributes_) {
-    res.emplace_back(&value);
-  }
-
-  return res;
-}
-
 Bindings::iterator Bindings::find(const Symbol& name) {
   return attributes_.find(name);
 }
diff --git a/third_party/nix/src/libexpr/attr-set.hh b/third_party/nix/src/libexpr/attr-set.hh
index c02f3930fd..5d77e0907c 100644
--- a/third_party/nix/src/libexpr/attr-set.hh
+++ b/third_party/nix/src/libexpr/attr-set.hh
@@ -59,13 +59,6 @@ class Bindings {
   iterator end();
   const_iterator cend() const;
 
-  // Returns the elements of the attribute set as a vector, sorted
-  // lexicographically by keys.
-  //
-  // This is used primarily for builtins that have guaranteed
-  // ordering, such as `attrNames` or `attrValues`.
-  std::vector<const Attr*> SortedByKeys();
-
   // oh no
   friend class EvalState;
 
diff --git a/third_party/nix/src/libexpr/eval.cc b/third_party/nix/src/libexpr/eval.cc
index f7b745a7b4..682ea64832 100644
--- a/third_party/nix/src/libexpr/eval.cc
+++ b/third_party/nix/src/libexpr/eval.cc
@@ -102,9 +102,9 @@ static void printValue(std::ostream& str, std::set<const Value*>& active,
       break;
     case tAttrs: {
       str << "{ ";
-      for (auto& i : v.attrs->SortedByKeys()) {
-        str << i->name << " = ";
-        printValue(str, active, *i->value);
+      for (const auto& [key, value] : *v.attrs) {
+        str << key << " = ";
+        printValue(str, active, *value.value);
         str << "; ";
       }
       str << "}";
diff --git a/third_party/nix/src/libexpr/get-drvs.cc b/third_party/nix/src/libexpr/get-drvs.cc
index d81fb5dfad..164c1e54f3 100644
--- a/third_party/nix/src/libexpr/get-drvs.cc
+++ b/third_party/nix/src/libexpr/get-drvs.cc
@@ -389,26 +389,26 @@ static void getDerivations(EvalState& state, Value& vIn,
        there are names clashes between derivations, the derivation
        bound to the attribute with the "lower" name should take
        precedence). */
-    for (auto& i : v.attrs->SortedByKeys()) {
-      DLOG(INFO) << "evaluating attribute '" << i->name << "'";
-      if (!std::regex_match(std::string(i->name), attrRegex)) {
+    for (auto& [_, i] : *v.attrs) {
+      DLOG(INFO) << "evaluating attribute '" << i.name << "'";
+      if (!std::regex_match(std::string(i.name), attrRegex)) {
         continue;
       }
-      std::string pathPrefix2 = addToPath(pathPrefix, i->name);
+      std::string pathPrefix2 = addToPath(pathPrefix, i.name);
       if (combineChannels) {
-        getDerivations(state, *i->value, pathPrefix2, autoArgs, drvs, done,
+        getDerivations(state, *i.value, pathPrefix2, autoArgs, drvs, done,
                        ignoreAssertionFailures);
-      } else if (getDerivation(state, *i->value, pathPrefix2, drvs, done,
+      } else if (getDerivation(state, *i.value, pathPrefix2, drvs, done,
                                ignoreAssertionFailures)) {
         /* If the value of this attribute is itself a set,
            should we recurse into it?  => Only if it has a
            `recurseForDerivations = true' attribute. */
-        if (i->value->type == tAttrs) {
-          Bindings::iterator j = i->value->attrs->find(
+        if (i.value->type == tAttrs) {
+          Bindings::iterator j = i.value->attrs->find(
               state.symbols.Create("recurseForDerivations"));
-          if (j != i->value->attrs->end() &&
+          if (j != i.value->attrs->end() &&
               state.forceBool(*j->second.value, *j->second.pos)) {
-            getDerivations(state, *i->value, pathPrefix2, autoArgs, drvs, done,
+            getDerivations(state, *i.value, pathPrefix2, autoArgs, drvs, done,
                            ignoreAssertionFailures);
           }
         }
diff --git a/third_party/nix/src/libexpr/primops.cc b/third_party/nix/src/libexpr/primops.cc
index 0bac7793c7..49c43556fe 100644
--- a/third_party/nix/src/libexpr/primops.cc
+++ b/third_party/nix/src/libexpr/primops.cc
@@ -510,11 +510,11 @@ static void prim_derivationStrict(EvalState& state, const Pos& pos,
   StringSet outputs;
   outputs.insert("out");
 
-  for (auto& i : args[0]->attrs->SortedByKeys()) {
-    if (i->name == state.sIgnoreNulls) {
+  for (auto& [_, i] : *args[0]->attrs) {
+    if (i.name == state.sIgnoreNulls) {
       continue;
     }
-    const std::string& key = i->name;
+    const std::string& key = i.name;
 
     auto handleHashMode = [&](const std::string& s) {
       if (s == "recursive") {
@@ -556,19 +556,19 @@ static void prim_derivationStrict(EvalState& state, const Pos& pos,
 
     try {
       if (ignoreNulls) {
-        state.forceValue(*i->value);
-        if (i->value->type == tNull) {
+        state.forceValue(*i.value);
+        if (i.value->type == tNull) {
           continue;
         }
       }
 
       /* The `args' attribute is special: it supplies the
          command-line arguments to the builder. */
-      if (i->name == state.sArgs) {
-        state.forceList(*i->value, pos);
-        for (unsigned int n = 0; n < i->value->listSize(); ++n) {
-          std::string s = state.coerceToString(
-              posDrvName, *(*i->value->list)[n], context, true);
+      if (i.name == state.sArgs) {
+        state.forceList(*i.value, pos);
+        for (unsigned int n = 0; n < i.value->listSize(); ++n) {
+          std::string s = state.coerceToString(posDrvName, *(*i.value->list)[n],
+                                               context, true);
           drv.args.push_back(s);
         }
       }
@@ -577,48 +577,48 @@ static void prim_derivationStrict(EvalState& state, const Pos& pos,
          the environment. */
       else {
         if (jsonObject) {
-          if (i->name == state.sStructuredAttrs) {
+          if (i.name == state.sStructuredAttrs) {
             continue;
           }
 
           auto placeholder(jsonObject->placeholder(key));
-          printValueAsJSON(state, true, *i->value, placeholder, context);
-
-          if (i->name == state.sBuilder) {
-            drv.builder = state.forceString(*i->value, context, posDrvName);
-          } else if (i->name == state.sSystem) {
-            drv.platform = state.forceStringNoCtx(*i->value, posDrvName);
-          } else if (i->name == state.sOutputHash) {
-            outputHash = state.forceStringNoCtx(*i->value, posDrvName);
-          } else if (i->name == state.sOutputHashAlgo) {
-            outputHashAlgo = state.forceStringNoCtx(*i->value, posDrvName);
-          } else if (i->name == state.sOutputHashMode) {
-            handleHashMode(state.forceStringNoCtx(*i->value, posDrvName));
-          } else if (i->name == state.sOutputs) {
+          printValueAsJSON(state, true, *i.value, placeholder, context);
+
+          if (i.name == state.sBuilder) {
+            drv.builder = state.forceString(*i.value, context, posDrvName);
+          } else if (i.name == state.sSystem) {
+            drv.platform = state.forceStringNoCtx(*i.value, posDrvName);
+          } else if (i.name == state.sOutputHash) {
+            outputHash = state.forceStringNoCtx(*i.value, posDrvName);
+          } else if (i.name == state.sOutputHashAlgo) {
+            outputHashAlgo = state.forceStringNoCtx(*i.value, posDrvName);
+          } else if (i.name == state.sOutputHashMode) {
+            handleHashMode(state.forceStringNoCtx(*i.value, posDrvName));
+          } else if (i.name == state.sOutputs) {
             /* Require ‘outputs’ to be a list of strings. */
-            state.forceList(*i->value, posDrvName);
+            state.forceList(*i.value, posDrvName);
             Strings ss;
-            for (unsigned int n = 0; n < i->value->listSize(); ++n) {
+            for (unsigned int n = 0; n < i.value->listSize(); ++n) {
               ss.emplace_back(
-                  state.forceStringNoCtx(*(*i->value->list)[n], posDrvName));
+                  state.forceStringNoCtx(*(*i.value->list)[n], posDrvName));
             }
             handleOutputs(ss);
           }
 
         } else {
-          auto s = state.coerceToString(posDrvName, *i->value, context, true);
+          auto s = state.coerceToString(posDrvName, *i.value, context, true);
           drv.env.emplace(key, s);
-          if (i->name == state.sBuilder) {
+          if (i.name == state.sBuilder) {
             drv.builder = s;
-          } else if (i->name == state.sSystem) {
+          } else if (i.name == state.sSystem) {
             drv.platform = s;
-          } else if (i->name == state.sOutputHash) {
+          } else if (i.name == state.sOutputHash) {
             outputHash = s;
-          } else if (i->name == state.sOutputHashAlgo) {
+          } else if (i.name == state.sOutputHashAlgo) {
             outputHashAlgo = s;
-          } else if (i->name == state.sOutputHashMode) {
+          } else if (i.name == state.sOutputHashMode) {
             handleHashMode(s);
-          } else if (i->name == state.sOutputs) {
+          } else if (i.name == state.sOutputs) {
             handleOutputs(absl::StrSplit(s, absl::ByAnyChar(" \t\n\r"),
                                          absl::SkipEmpty()));
           }