From ddabd94f82787bd4f47fff70818d16b0a0dbbfc0 Mon Sep 17 00:00:00 2001 From: Sergei Zimmerman Date: Sun, 14 Sep 2025 22:52:37 +0300 Subject: [PATCH] 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