Merge pull request #13983 from xokdvium/bindings-fixes

libexpr: Make Bindings::iterator a proper strong type instead of pointer

src/libexpr-tests/primops.cc

@@ -195,18 +195,18 @@ TEST_F(PrimOpTest, unsafeGetAttrPos)
     auto v = eval(expr);
     ASSERT_THAT(v, IsAttrsOfSize(3));
 
-    auto file = v.attrs()->find(createSymbol("file"));
+    auto file = v.attrs()->get(createSymbol("file"));
     ASSERT_NE(file, nullptr);
     ASSERT_THAT(*file->value, IsString());
     auto s = baseNameOf(file->value->string_view());
     ASSERT_EQ(s, "foo.nix");
 
-    auto line = v.attrs()->find(createSymbol("line"));
+    auto line = v.attrs()->get(createSymbol("line"));
     ASSERT_NE(line, nullptr);
     state.forceValue(*line->value, noPos);
     ASSERT_THAT(*line->value, IsIntEq(4));
 
-    auto column = v.attrs()->find(createSymbol("column"));
+    auto column = v.attrs()->get(createSymbol("column"));
     ASSERT_NE(column, nullptr);
     state.forceValue(*column->value, noPos);
     ASSERT_THAT(*column->value, IsIntEq(3));
@@ -246,7 +246,7 @@ TEST_F(PrimOpTest, removeAttrsRetains)
 {
     auto v = eval("builtins.removeAttrs { x = 1; y = 2; } [\"x\"]");
     ASSERT_THAT(v, IsAttrsOfSize(1));
-    ASSERT_NE(v.attrs()->find(createSymbol("y")), nullptr);
+    ASSERT_NE(v.attrs()->get(createSymbol("y")), nullptr);
 }
 
 TEST_F(PrimOpTest, listToAttrsEmptyList)
@@ -266,7 +266,7 @@ TEST_F(PrimOpTest, listToAttrs)
 {
     auto v = eval("builtins.listToAttrs [ { name = \"key\"; value = 123; } ]");
     ASSERT_THAT(v, IsAttrsOfSize(1));
-    auto key = v.attrs()->find(createSymbol("key"));
+    auto key = v.attrs()->get(createSymbol("key"));
     ASSERT_NE(key, nullptr);
     ASSERT_THAT(*key->value, IsIntEq(123));
 }
@@ -275,7 +275,7 @@ TEST_F(PrimOpTest, intersectAttrs)
 {
     auto v = eval("builtins.intersectAttrs { a = 1; b = 2; } { b = 3; c = 4; }");
     ASSERT_THAT(v, IsAttrsOfSize(1));
-    auto b = v.attrs()->find(createSymbol("b"));
+    auto b = v.attrs()->get(createSymbol("b"));
     ASSERT_NE(b, nullptr);
     ASSERT_THAT(*b->value, IsIntEq(3));
 }
@@ -293,11 +293,11 @@ TEST_F(PrimOpTest, functionArgs)
     auto v = eval("builtins.functionArgs ({ x, y ? 123}: 1)");
     ASSERT_THAT(v, IsAttrsOfSize(2));
 
-    auto x = v.attrs()->find(createSymbol("x"));
+    auto x = v.attrs()->get(createSymbol("x"));
     ASSERT_NE(x, nullptr);
     ASSERT_THAT(*x->value, IsFalse());
 
-    auto y = v.attrs()->find(createSymbol("y"));
+    auto y = v.attrs()->get(createSymbol("y"));
     ASSERT_NE(y, nullptr);
     ASSERT_THAT(*y->value, IsTrue());
 }
@@ -307,13 +307,13 @@ TEST_F(PrimOpTest, mapAttrs)
     auto v = eval("builtins.mapAttrs (name: value: value * 10) { a = 1; b = 2; }");
     ASSERT_THAT(v, IsAttrsOfSize(2));
 
-    auto a = v.attrs()->find(createSymbol("a"));
+    auto a = v.attrs()->get(createSymbol("a"));
     ASSERT_NE(a, nullptr);
     ASSERT_THAT(*a->value, IsThunk());
     state.forceValue(*a->value, noPos);
     ASSERT_THAT(*a->value, IsIntEq(10));
 
-    auto b = v.attrs()->find(createSymbol("b"));
+    auto b = v.attrs()->get(createSymbol("b"));
     ASSERT_NE(b, nullptr);
     ASSERT_THAT(*b->value, IsThunk());
     state.forceValue(*b->value, noPos);
@@ -839,11 +839,11 @@ TEST_P(ParseDrvNamePrimOpTest, parseDrvName)
     auto v = eval(expr);
     ASSERT_THAT(v, IsAttrsOfSize(2));
 
-    auto name = v.attrs()->find(createSymbol("name"));
+    auto name = v.attrs()->get(createSymbol("name"));
     ASSERT_TRUE(name);
     ASSERT_THAT(*name->value, IsStringEq(expectedName));
 
-    auto version = v.attrs()->find(createSymbol("version"));
+    auto version = v.attrs()->get(createSymbol("version"));
     ASSERT_TRUE(version);
     ASSERT_THAT(*version->value, IsStringEq(expectedVersion));
 }

src/libexpr-tests/trivial.cc

@@ -75,11 +75,11 @@ TEST_F(TrivialExpressionTest, updateAttrs)
 {
     auto v = eval("{ a = 1; } // { b = 2; a = 3; }");
     ASSERT_THAT(v, IsAttrsOfSize(2));
-    auto a = v.attrs()->find(createSymbol("a"));
+    auto a = v.attrs()->get(createSymbol("a"));
     ASSERT_NE(a, nullptr);
     ASSERT_THAT(*a->value, IsIntEq(3));
 
-    auto b = v.attrs()->find(createSymbol("b"));
+    auto b = v.attrs()->get(createSymbol("b"));
     ASSERT_NE(b, nullptr);
     ASSERT_THAT(*b->value, IsIntEq(2));
 }
@@ -176,7 +176,7 @@ TEST_P(AttrSetMergeTrvialExpressionTest, attrsetMergeLazy)
     auto v = eval(expr);
     ASSERT_THAT(v, IsAttrsOfSize(1));
 
-    auto a = v.attrs()->find(createSymbol("a"));
+    auto a = v.attrs()->get(createSymbol("a"));
     ASSERT_NE(a, nullptr);
 
     ASSERT_THAT(*a->value, IsThunk());
@@ -184,11 +184,11 @@ TEST_P(AttrSetMergeTrvialExpressionTest, attrsetMergeLazy)
 
     ASSERT_THAT(*a->value, IsAttrsOfSize(2));
 
-    auto b = a->value->attrs()->find(createSymbol("b"));
+    auto b = a->value->attrs()->get(createSymbol("b"));
     ASSERT_NE(b, nullptr);
     ASSERT_THAT(*b->value, IsIntEq(1));
 
-    auto c = a->value->attrs()->find(createSymbol("c"));
+    auto c = a->value->attrs()->get(createSymbol("c"));
     ASSERT_NE(c, nullptr);
     ASSERT_THAT(*c->value, IsIntEq(2));
 }
@@ -330,7 +330,7 @@ TEST_F(TrivialExpressionTest, bindOr)
 {
     auto v = eval("{ or = 1; }");
     ASSERT_THAT(v, IsAttrsOfSize(1));
-    auto b = v.attrs()->find(createSymbol("or"));
+    auto b = v.attrs()->get(createSymbol("or"));
     ASSERT_NE(b, nullptr);
     ASSERT_THAT(*b->value, IsIntEq(1));
 }

src/libexpr/attr-set.cc

@@ -35,8 +35,7 @@ Value & BindingsBuilder::alloc(std::string_view name, PosIdx pos)
 
 void Bindings::sort()
 {
-    if (size_)
+    std::sort(attrs, attrs + size_);
-        std::sort(begin(), end());
 }
 
 Value & Value::mkAttrs(BindingsBuilder & bindings)

src/libexpr/include/nix/expr/attr-set.hh

@@ -6,6 +6,7 @@
 
 #include <algorithm>
 #include <functional>
+#include <concepts>
 
 namespace nix {
 
@@ -81,9 +82,55 @@ public:
         return !size_;
     }
 
-    typedef Attr * iterator;
+    class iterator
+    {
+    public:
+        using value_type = Attr;
+        using pointer = const value_type *;
+        using reference = const value_type &;
+        using difference_type = std::ptrdiff_t;
+        using iterator_category = std::forward_iterator_tag;
 
-    typedef const Attr * const_iterator;
+        friend class Bindings;
+
+    private:
+        pointer ptr = nullptr;
+
+        explicit iterator(pointer ptr)
+            : ptr(ptr)
+        {
+        }
+
+    public:
+        iterator() = default;
+
+        reference operator*() const
+        {
+            return *ptr;
+        }
+
+        const value_type * operator->() const
+        {
+            return ptr;
+        }
+
+        iterator & operator++()
+        {
+            ++ptr;
+            return *this;
+        }
+
+        iterator operator++(int)
+        {
+            pointer tmp = ptr;
+            ++*this;
+            return iterator(tmp);
+        }
+
+        bool operator==(const iterator & rhs) const = default;
+    };
+
+    using const_iterator = iterator;
 
     void push_back(const Attr & attr)
     {
@@ -93,39 +140,33 @@ public:
     const_iterator find(Symbol name) const
     {
         Attr key(name, 0);
-        const_iterator i = std::lower_bound(begin(), end(), key);
+        auto first = attrs;
-        if (i != end() && i->name == name)
+        auto last = attrs + size_;
-            return i;
+        const Attr * i = std::lower_bound(first, last, key);
+        if (i != last && i->name == name)
+            return const_iterator{i};
         return end();
     }
 
     const Attr * get(Symbol name) const
     {
         Attr key(name, 0);
-        const_iterator i = std::lower_bound(begin(), end(), key);
+        auto first = attrs;
-        if (i != end() && i->name == name)
+        auto last = attrs + size_;
-            return &*i;
+        const Attr * i = std::lower_bound(first, last, key);
+        if (i != last && i->name == name)
+            return i;
         return nullptr;
     }
 
-    iterator begin()
-    {
-        return &attrs[0];
-    }
-
-    iterator end()
-    {
-        return &attrs[size_];
-    }
-
     const_iterator begin() const
     {
-        return &attrs[0];
+        return const_iterator(attrs);
     }
 
     const_iterator end() const
     {
-        return &attrs[size_];
+        return const_iterator(attrs + size_);
     }
 
     Attr & operator[](size_t pos)
@@ -159,6 +200,9 @@ public:
     friend class EvalState;
 };
 
+static_assert(std::forward_iterator<Bindings::iterator>);
+static_assert(std::ranges::forward_range<Bindings>);
+
 /**
  * A wrapper around Bindings that ensures that its always in sorted
  * order at the end. The only way to consume a BindingsBuilder is to