From e75501da3ecf2b4081bd17a9d22f008178671fc0 Mon Sep 17 00:00:00 2001 From: Sergei Zimmerman Date: Sat, 13 Sep 2025 23:21:24 +0300 Subject: [PATCH 1/2] libexpr: Remove non-const iterators of Bindings --- src/libexpr/attr-set.cc | 3 +-- src/libexpr/include/nix/expr/attr-set.hh | 12 ------------ 2 files changed, 1 insertion(+), 14 deletions(-) diff --git a/src/libexpr/attr-set.cc b/src/libexpr/attr-set.cc index 48d4c4d4a..88474c36f 100644 --- a/src/libexpr/attr-set.cc +++ b/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(begin(), end()); + std::sort(attrs, attrs + size_); } Value & Value::mkAttrs(BindingsBuilder & bindings) diff --git a/src/libexpr/include/nix/expr/attr-set.hh b/src/libexpr/include/nix/expr/attr-set.hh index 4ab54c8eb..5bf266e54 100644 --- a/src/libexpr/include/nix/expr/attr-set.hh +++ b/src/libexpr/include/nix/expr/attr-set.hh @@ -81,8 +81,6 @@ public: return !size_; } - typedef Attr * iterator; - typedef const Attr * const_iterator; void push_back(const Attr & attr) @@ -108,16 +106,6 @@ public: return nullptr; } - iterator begin() - { - return &attrs[0]; - } - - iterator end() - { - return &attrs[size_]; - } - const_iterator begin() const { return &attrs[0]; From ddabd94f82787bd4f47fff70818d16b0a0dbbfc0 Mon Sep 17 00:00:00 2001 From: Sergei Zimmerman Date: Sun, 14 Sep 2025 22:52:37 +0300 Subject: [PATCH 2/2] libexpr: Make Bindings::iterator a proper strong type instead of pointer As evident from the number of tests that were holding this API completely wrong (the end() iterator returned from find() is NEVER nullptr) we should not have this footgun. A proper strong type guarantees that this confusion will not happen again. Also this will be helpful down the road when Bindings becomes something smarter than an array of Attr. --- src/libexpr-tests/primops.cc | 24 ++++---- src/libexpr-tests/trivial.cc | 12 ++-- src/libexpr/include/nix/expr/attr-set.hh | 74 +++++++++++++++++++++--- 3 files changed, 83 insertions(+), 27 deletions(-) diff --git a/src/libexpr-tests/primops.cc b/src/libexpr-tests/primops.cc index f3f7de8d9..aa4ef5e21 100644 --- a/src/libexpr-tests/primops.cc +++ b/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)); } diff --git a/src/libexpr-tests/trivial.cc b/src/libexpr-tests/trivial.cc index 02433234e..a287ce4d1 100644 --- a/src/libexpr-tests/trivial.cc +++ b/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)); } diff --git a/src/libexpr/include/nix/expr/attr-set.hh b/src/libexpr/include/nix/expr/attr-set.hh index 5bf266e54..132be163d 100644 --- a/src/libexpr/include/nix/expr/attr-set.hh +++ b/src/libexpr/include/nix/expr/attr-set.hh @@ -6,6 +6,7 @@ #include #include +#include namespace nix { @@ -81,7 +82,55 @@ public: return !size_; } - typedef const Attr * const_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; + + 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) { @@ -91,29 +140,33 @@ public: const_iterator find(Symbol name) const { Attr key(name, 0); - const_iterator i = std::lower_bound(begin(), end(), key); - if (i != end() && i->name == name) - return i; + auto first = attrs; + auto last = attrs + size_; + 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); - if (i != end() && i->name == name) - return &*i; + auto first = attrs; + auto last = attrs + size_; + const Attr * i = std::lower_bound(first, last, key); + if (i != last && i->name == name) + return i; return nullptr; } 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) @@ -147,6 +200,9 @@ public: friend class EvalState; }; +static_assert(std::forward_iterator); +static_assert(std::ranges::forward_range); + /** * A wrapper around Bindings that ensures that its always in sorted * order at the end. The only way to consume a BindingsBuilder is to