diff --git a/.clang-tidy b/.clang-tidy index 0887b867087..6092c4e4678 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -1,3 +1,13 @@ -# We use pointers to aggregates in a couple of places, intentionally. -# void * would look weird. -Checks: '-bugprone-sizeof-expression' +Checks: + - bugprone-* + - clang-analyzer-* + - cppcoreguidelines-* + - modernize-* + - -modernize-use-trailing-return-type + - performance-* + - portability-* + - readability-* + - -readability-identifier-length +FormatStyle: file +InheritParentConfig: true +User: connorbaker \ No newline at end of file diff --git a/.gitignore b/.gitignore index de1183977b3..68de0dc778f 100644 --- a/.gitignore +++ b/.gitignore @@ -155,6 +155,8 @@ result-* # IDE .vscode/ .idea/ +.direnv/ +.envrc .pre-commit-config.yaml diff --git a/flake.nix b/flake.nix index 06025e3b79c..4076d9d68f5 100644 --- a/flake.nix +++ b/flake.nix @@ -98,6 +98,42 @@ useLLVM = true; }; overlays = [ + # Update immer and range-v3 to the latest + # TODO(@connorbaker): Upstream this. + (final: prev: { + immer = + let + inherit (final.lib.strings) cmakeBool; + in + prev.immer.overrideAttrs (finalAttrs: _: { + strictDeps = true; + version = "0.8.1-unstable-2024-09-18"; + src = final.fetchFromGitHub { + owner = "arximboldi"; + repo = "immer"; + rev = "df6ef46d97e1fe81f397015b9aeb32505cef653b"; + hash = "sha256-fV6Rqbg/vtUH2DdgLYULl0zLM3WUSG1qYLZtqAhaWQw="; + }; + doCheck = false; + # TODO(@connorbaker): Add support for doCheck = true; + cmakeFlags = [ + (cmakeBool "immer_BUILD_DOCS" false) + (cmakeBool "immer_BUILD_EXAMPLES" finalAttrs.doCheck) + (cmakeBool "immer_BUILD_EXTRAS" false) + (cmakeBool "immer_BUILD_TESTS" finalAttrs.doCheck) + ]; + }); + range-v3 = prev.range-v3.overrideAttrs { + strictDeps = true; + version = "0.12.0-unstable-2024-10-01"; + src = final.fetchFromGitHub { + owner = "ericniebler"; + repo = "range-v3"; + rev = "7e6f34b1e820fb8321346888ef0558a0ec842b8e"; + hash = "sha256-FbLZ8CHnLdTs0FRdLPTOBiw2lcuIuSoFSb8E4MD8DWQ="; + }; + }; + }) (overlayFor (p: p.${stdenv})) ]; }; diff --git a/packaging/dev-shell.nix b/packaging/dev-shell.nix index 30ac518d5f7..b30f1f0d830 100644 --- a/packaging/dev-shell.nix +++ b/packaging/dev-shell.nix @@ -112,7 +112,10 @@ in { # TODO: Remove the darwin check once # https://github.com/NixOS/nixpkgs/pull/291814 is available ++ lib.optional (stdenv.cc.isClang && !stdenv.buildPlatform.isDarwin) pkgs.buildPackages.bear - ++ lib.optional (stdenv.cc.isClang && stdenv.hostPlatform == stdenv.buildPlatform) (lib.hiPrio pkgs.buildPackages.clang-tools); + ++ lib.optional (stdenv.cc.isClang && stdenv.hostPlatform == stdenv.buildPlatform) [ + (lib.hiPrio pkgs.buildPackages.clang-tools_18) + pkgs.buildPackages.llvmPackages_18.lldb + ]; buildInputs = attrs.buildInputs or [] ++ pkgs.nixComponents.nix-util.buildInputs diff --git a/src/libcmd/repl.cc b/src/libcmd/repl.cc index f292f06bb81..beb90143b6d 100644 --- a/src/libcmd/repl.cc +++ b/src/libcmd/repl.cc @@ -741,7 +741,7 @@ void NixRepl::loadFlake(const std::string & flakeRefS) void NixRepl::initEnv() { - env = &state->allocEnv(envSize); + env = state->allocEnv(envSize); env->up = &state->baseEnv; displ = 0; staticEnv->vars.clear(); diff --git a/src/libexpr-c/nix_api_expr_internal.h b/src/libexpr-c/nix_api_expr_internal.h index 12f24b6eb07..1bb5042b86f 100644 --- a/src/libexpr-c/nix_api_expr_internal.h +++ b/src/libexpr-c/nix_api_expr_internal.h @@ -19,11 +19,6 @@ struct BindingsBuilder nix::BindingsBuilder builder; }; -struct ListBuilder -{ - nix::ListBuilder builder; -}; - struct nix_value { nix::Value value; diff --git a/src/libexpr-c/nix_api_value.cc b/src/libexpr-c/nix_api_value.cc index bae078d312f..5514d9c0b61 100644 --- a/src/libexpr-c/nix_api_value.cc +++ b/src/libexpr-c/nix_api_value.cc @@ -324,7 +324,7 @@ nix_value * nix_get_list_byidx(nix_c_context * context, const nix_value * value, try { auto & v = check_value_in(value); assert(v.type() == nix::nList); - auto * p = v.listElems()[ix]; + auto * p = v.list()[ix]; nix_gc_incref(nullptr, p); if (p != nullptr) state->state.forceValue(*p, nix::noPos); @@ -490,53 +490,6 @@ nix_err nix_init_external(nix_c_context * context, nix_value * value, ExternalVa NIXC_CATCH_ERRS } -ListBuilder * nix_make_list_builder(nix_c_context * context, EvalState * state, size_t capacity) -{ - if (context) - context->last_err_code = NIX_OK; - try { - auto builder = state->state.buildList(capacity); - return new -#if HAVE_BOEHMGC - (NoGC) -#endif - ListBuilder{std::move(builder)}; - } - NIXC_CATCH_ERRS_NULL -} - -nix_err -nix_list_builder_insert(nix_c_context * context, ListBuilder * list_builder, unsigned int index, nix_value * value) -{ - if (context) - context->last_err_code = NIX_OK; - try { - auto & e = check_value_not_null(value); - list_builder->builder[index] = &e; - } - NIXC_CATCH_ERRS -} - -void nix_list_builder_free(ListBuilder * list_builder) -{ -#if HAVE_BOEHMGC - GC_FREE(list_builder); -#else - delete list_builder; -#endif -} - -nix_err nix_make_list(nix_c_context * context, ListBuilder * list_builder, nix_value * value) -{ - if (context) - context->last_err_code = NIX_OK; - try { - auto & v = check_value_out(value); - v.mkList(list_builder->builder); - } - NIXC_CATCH_ERRS -} - nix_err nix_init_primop(nix_c_context * context, nix_value * value, PrimOp * p) { if (context) diff --git a/src/libexpr-c/nix_api_value.h b/src/libexpr-c/nix_api_value.h index 8a0813ebe1f..f8337028d78 100644 --- a/src/libexpr-c/nix_api_value.h +++ b/src/libexpr-c/nix_api_value.h @@ -50,15 +50,6 @@ typedef struct EvalState EvalState; */ typedef struct BindingsBuilder BindingsBuilder; -/** @brief Stores an under-construction list - * @ingroup value_manip - * - * Do not reuse. - * @see nix_make_list_builder, nix_list_builder_free, nix_make_list - * @see nix_list_builder_insert - */ -typedef struct ListBuilder ListBuilder; - /** @brief PrimOp function * @ingroup primops * @@ -393,39 +384,6 @@ nix_err nix_init_apply(nix_c_context * context, nix_value * value, nix_value * f */ nix_err nix_init_external(nix_c_context * context, nix_value * value, ExternalValue * val); -/** @brief Create a list from a list builder - * @param[out] context Optional, stores error information - * @param[in] list_builder list builder to use. Make sure to unref this afterwards. - * @param[out] value Nix value to modify - * @return error code, NIX_OK on success. - */ -nix_err nix_make_list(nix_c_context * context, ListBuilder * list_builder, nix_value * value); - -/** @brief Create a list builder - * @param[out] context Optional, stores error information - * @param[in] state nix evaluator state - * @param[in] capacity how many bindings you'll add. Don't exceed. - * @return owned reference to a list builder. Make sure to unref when you're done. - */ -ListBuilder * nix_make_list_builder(nix_c_context * context, EvalState * state, size_t capacity); - -/** @brief Insert bindings into a builder - * @param[out] context Optional, stores error information - * @param[in] list_builder ListBuilder to insert into - * @param[in] index index to manipulate - * @param[in] value value to insert - * @return error code, NIX_OK on success. - */ -nix_err -nix_list_builder_insert(nix_c_context * context, ListBuilder * list_builder, unsigned int index, nix_value * value); - -/** @brief Free a list builder - * - * Does not fail. - * @param[in] list_builder The builder to free. - */ -void nix_list_builder_free(ListBuilder * list_builder); - /** @brief Create an attribute set from a bindings builder * @param[out] context Optional, stores error information * @param[out] value Nix value to modify diff --git a/src/libexpr-tests/error_traces.cc b/src/libexpr-tests/error_traces.cc index be379a90993..9fd0e3e47bd 100644 --- a/src/libexpr-tests/error_traces.cc +++ b/src/libexpr-tests/error_traces.cc @@ -708,11 +708,11 @@ namespace nix { ASSERT_TRACE2("head 1", TypeError, HintFmt("expected a list but found %s: %s", "an integer", Uncolored(ANSI_CYAN "1" ANSI_NORMAL)), - HintFmt("while evaluating the first argument passed to builtins.elemAt")); + HintFmt("while evaluating the first argument passed to builtins.head")); ASSERT_TRACE1("head []", Error, - HintFmt("list index %d is out of bounds", 0)); + HintFmt("'head' called on an empty list")); } diff --git a/src/libexpr-tests/meson.build b/src/libexpr-tests/meson.build index 5a5c9f1d48a..34df948f065 100644 --- a/src/libexpr-tests/meson.build +++ b/src/libexpr-tests/meson.build @@ -2,6 +2,8 @@ project('nix-expr-tests', 'cpp', version : files('.version'), default_options : [ 'cpp_std=c++2a', + # Set -Wno-array-bounds to avoid warnings in Immer's generated code. + 'cpp_args=-Warray-bounds=0', # TODO(Qyriad): increase the warning level 'warning_level=1', 'debug=true', diff --git a/src/libexpr-tests/nix_api_value.cc b/src/libexpr-tests/nix_api_value.cc index 7fc8b4f641f..a88389bff6b 100644 --- a/src/libexpr-tests/nix_api_value.cc +++ b/src/libexpr-tests/nix_api_value.cc @@ -143,35 +143,6 @@ TEST_F(nix_api_expr_test, nix_build_and_init_list_invalid) assert_ctx_err(); } -TEST_F(nix_api_expr_test, nix_build_and_init_list) -{ - int size = 10; - ListBuilder * builder = nix_make_list_builder(ctx, state, size); - - nix_value * intValue = nix_alloc_value(ctx, state); - nix_value * intValue2 = nix_alloc_value(ctx, state); - - // `init` and `insert` can be called in any order - nix_init_int(ctx, intValue, 42); - nix_list_builder_insert(ctx, builder, 0, intValue); - nix_list_builder_insert(ctx, builder, 1, intValue2); - nix_init_int(ctx, intValue2, 43); - - nix_make_list(ctx, builder, value); - nix_list_builder_free(builder); - - ASSERT_EQ(42, nix_get_int(ctx, nix_get_list_byidx(ctx, value, state, 0))); - ASSERT_EQ(43, nix_get_int(ctx, nix_get_list_byidx(ctx, value, state, 1))); - ASSERT_EQ(nullptr, nix_get_list_byidx(ctx, value, state, 2)); - ASSERT_EQ(10, nix_get_list_size(ctx, value)); - - ASSERT_STREQ("a list", nix_get_typename(ctx, value)); - ASSERT_EQ(NIX_TYPE_LIST, nix_get_type(ctx, value)); - - // Clean up - nix_gc_decref(ctx, intValue); -} - TEST_F(nix_api_expr_test, nix_build_and_init_attr_invalid) { ASSERT_EQ(nullptr, nix_get_attr_byname(ctx, nullptr, state, 0)); diff --git a/src/libexpr-tests/primops.cc b/src/libexpr-tests/primops.cc index 5b589823798..477f18bf8de 100644 --- a/src/libexpr-tests/primops.cc +++ b/src/libexpr-tests/primops.cc @@ -135,8 +135,8 @@ namespace nix { TEST_F(PrimOpTest, attrValues) { auto v = eval("builtins.attrValues { x = \"foo\"; a = 1; }"); ASSERT_THAT(v, IsListOfSize(2)); - ASSERT_THAT(*v.listElems()[0], IsIntEq(1)); - ASSERT_THAT(*v.listElems()[1], IsStringEq("foo")); + ASSERT_THAT(*v.list()[0], IsIntEq(1)); + ASSERT_THAT(*v.list()[1], IsStringEq("foo")); } TEST_F(PrimOpTest, getAttr) { @@ -235,8 +235,8 @@ namespace nix { TEST_F(PrimOpTest, catAttrs) { auto v = eval("builtins.catAttrs \"a\" [{a = 1;} {b = 0;} {a = 2;}]"); ASSERT_THAT(v, IsListOfSize(2)); - ASSERT_THAT(*v.listElems()[0], IsIntEq(1)); - ASSERT_THAT(*v.listElems()[1], IsIntEq(2)); + ASSERT_THAT(*v.list()[0], IsIntEq(1)); + ASSERT_THAT(*v.list()[1], IsIntEq(2)); } TEST_F(PrimOpTest, functionArgs) { @@ -304,7 +304,7 @@ namespace nix { TEST_F(PrimOpTest, tail) { auto v = eval("builtins.tail [ 3 2 1 0 ]"); ASSERT_THAT(v, IsListOfSize(3)); - for (const auto [n, elem] : enumerate(v.listItems())) + for (const auto [n, elem] : enumerate(v.list())) ASSERT_THAT(*elem, IsIntEq(2 - static_cast(n))); } @@ -315,17 +315,17 @@ namespace nix { TEST_F(PrimOpTest, map) { auto v = eval("map (x: \"foo\" + x) [ \"bar\" \"bla\" \"abc\" ]"); ASSERT_THAT(v, IsListOfSize(3)); - auto elem = v.listElems()[0]; + auto elem = v.list()[0]; ASSERT_THAT(*elem, IsThunk()); state.forceValue(*elem, noPos); ASSERT_THAT(*elem, IsStringEq("foobar")); - elem = v.listElems()[1]; + elem = v.list()[1]; ASSERT_THAT(*elem, IsThunk()); state.forceValue(*elem, noPos); ASSERT_THAT(*elem, IsStringEq("foobla")); - elem = v.listElems()[2]; + elem = v.list()[2]; ASSERT_THAT(*elem, IsThunk()); state.forceValue(*elem, noPos); ASSERT_THAT(*elem, IsStringEq("fooabc")); @@ -334,7 +334,7 @@ namespace nix { TEST_F(PrimOpTest, filter) { auto v = eval("builtins.filter (x: x == 2) [ 3 2 3 2 3 2 ]"); ASSERT_THAT(v, IsListOfSize(3)); - for (const auto elem : v.listItems()) + for (const auto elem : v.list()) ASSERT_THAT(*elem, IsIntEq(2)); } @@ -351,7 +351,7 @@ namespace nix { TEST_F(PrimOpTest, concatLists) { auto v = eval("builtins.concatLists [[1 2] [3 4]]"); ASSERT_THAT(v, IsListOfSize(4)); - for (const auto [i, elem] : enumerate(v.listItems())) + for (const auto [i, elem] : enumerate(v.list())) ASSERT_THAT(*elem, IsIntEq(static_cast(i)+1)); } @@ -389,7 +389,7 @@ namespace nix { auto v = eval("builtins.genList (x: x + 1) 3"); ASSERT_EQ(v.type(), nList); ASSERT_EQ(v.listSize(), 3); - for (const auto [i, elem] : enumerate(v.listItems())) { + for (const auto [i, elem] : enumerate(v.list())) { ASSERT_THAT(*elem, IsThunk()); state.forceValue(*elem, noPos); ASSERT_THAT(*elem, IsIntEq(static_cast(i)+1)); @@ -402,7 +402,7 @@ namespace nix { ASSERT_EQ(v.listSize(), 6); const std::vector numbers = { 42, 77, 147, 249, 483, 526 }; - for (const auto [n, elem] : enumerate(v.listItems())) + for (const auto [n, elem] : enumerate(v.list())) ASSERT_THAT(*elem, IsIntEq(numbers[n])); } @@ -413,17 +413,17 @@ namespace nix { auto right = v.attrs()->get(createSymbol("right")); ASSERT_NE(right, nullptr); ASSERT_THAT(*right->value, IsListOfSize(2)); - ASSERT_THAT(*right->value->listElems()[0], IsIntEq(23)); - ASSERT_THAT(*right->value->listElems()[1], IsIntEq(42)); + ASSERT_THAT(*right->value->list()[0], IsIntEq(23)); + ASSERT_THAT(*right->value->list()[1], IsIntEq(42)); auto wrong = v.attrs()->get(createSymbol("wrong")); ASSERT_NE(wrong, nullptr); ASSERT_EQ(wrong->value->type(), nList); ASSERT_EQ(wrong->value->listSize(), 3); ASSERT_THAT(*wrong->value, IsListOfSize(3)); - ASSERT_THAT(*wrong->value->listElems()[0], IsIntEq(1)); - ASSERT_THAT(*wrong->value->listElems()[1], IsIntEq(9)); - ASSERT_THAT(*wrong->value->listElems()[2], IsIntEq(3)); + ASSERT_THAT(*wrong->value->list()[0], IsIntEq(1)); + ASSERT_THAT(*wrong->value->list()[1], IsIntEq(9)); + ASSERT_THAT(*wrong->value->list()[2], IsIntEq(3)); } TEST_F(PrimOpTest, concatMap) { @@ -432,7 +432,7 @@ namespace nix { ASSERT_EQ(v.listSize(), 6); const std::vector numbers = { 1, 2, 0, 3, 4, 0 }; - for (const auto [n, elem] : enumerate(v.listItems())) + for (const auto [n, elem] : enumerate(v.list())) ASSERT_THAT(*elem, IsIntEq(numbers[n])); } @@ -656,7 +656,7 @@ namespace nix { ASSERT_THAT(v, IsListOfSize(4)); const std::vector strings = { "1", "2", "3", "git" }; - for (const auto [n, p] : enumerate(v.listItems())) + for (const auto [n, p] : enumerate(v.list())) ASSERT_THAT(*p, IsStringEq(strings[n])); } @@ -746,12 +746,12 @@ namespace nix { auto v = eval("builtins.split \"(a)b\" \"abc\""); ASSERT_THAT(v, IsListOfSize(3)); - ASSERT_THAT(*v.listElems()[0], IsStringEq("")); + ASSERT_THAT(*v.list()[0], IsStringEq("")); - ASSERT_THAT(*v.listElems()[1], IsListOfSize(1)); - ASSERT_THAT(*v.listElems()[1]->listElems()[0], IsStringEq("a")); + ASSERT_THAT(*v.list()[1], IsListOfSize(1)); + ASSERT_THAT(*v.list()[1]->list()[0], IsStringEq("a")); - ASSERT_THAT(*v.listElems()[2], IsStringEq("c")); + ASSERT_THAT(*v.list()[2], IsStringEq("c")); } TEST_F(PrimOpTest, split2) { @@ -759,17 +759,17 @@ namespace nix { auto v = eval("builtins.split \"([ac])\" \"abc\""); ASSERT_THAT(v, IsListOfSize(5)); - ASSERT_THAT(*v.listElems()[0], IsStringEq("")); + ASSERT_THAT(*v.list()[0], IsStringEq("")); - ASSERT_THAT(*v.listElems()[1], IsListOfSize(1)); - ASSERT_THAT(*v.listElems()[1]->listElems()[0], IsStringEq("a")); + ASSERT_THAT(*v.list()[1], IsListOfSize(1)); + ASSERT_THAT(*v.list()[1]->list()[0], IsStringEq("a")); - ASSERT_THAT(*v.listElems()[2], IsStringEq("b")); + ASSERT_THAT(*v.list()[2], IsStringEq("b")); - ASSERT_THAT(*v.listElems()[3], IsListOfSize(1)); - ASSERT_THAT(*v.listElems()[3]->listElems()[0], IsStringEq("c")); + ASSERT_THAT(*v.list()[3], IsListOfSize(1)); + ASSERT_THAT(*v.list()[3]->list()[0], IsStringEq("c")); - ASSERT_THAT(*v.listElems()[4], IsStringEq("")); + ASSERT_THAT(*v.list()[4], IsStringEq("")); } TEST_F(PrimOpTest, split3) { @@ -777,36 +777,36 @@ namespace nix { ASSERT_THAT(v, IsListOfSize(5)); // First list element - ASSERT_THAT(*v.listElems()[0], IsStringEq("")); + ASSERT_THAT(*v.list()[0], IsStringEq("")); // 2nd list element is a list [ "" null ] - ASSERT_THAT(*v.listElems()[1], IsListOfSize(2)); - ASSERT_THAT(*v.listElems()[1]->listElems()[0], IsStringEq("a")); - ASSERT_THAT(*v.listElems()[1]->listElems()[1], IsNull()); + ASSERT_THAT(*v.list()[1], IsListOfSize(2)); + ASSERT_THAT(*v.list()[1]->list()[0], IsStringEq("a")); + ASSERT_THAT(*v.list()[1]->list()[1], IsNull()); // 3rd element - ASSERT_THAT(*v.listElems()[2], IsStringEq("b")); + ASSERT_THAT(*v.list()[2], IsStringEq("b")); // 4th element is a list: [ null "c" ] - ASSERT_THAT(*v.listElems()[3], IsListOfSize(2)); - ASSERT_THAT(*v.listElems()[3]->listElems()[0], IsNull()); - ASSERT_THAT(*v.listElems()[3]->listElems()[1], IsStringEq("c")); + ASSERT_THAT(*v.list()[3], IsListOfSize(2)); + ASSERT_THAT(*v.list()[3]->list()[0], IsNull()); + ASSERT_THAT(*v.list()[3]->list()[1], IsStringEq("c")); // 5th element is the empty string - ASSERT_THAT(*v.listElems()[4], IsStringEq("")); + ASSERT_THAT(*v.list()[4], IsStringEq("")); } TEST_F(PrimOpTest, split4) { auto v = eval("builtins.split \"([[:upper:]]+)\" \" FOO \""); ASSERT_THAT(v, IsListOfSize(3)); - auto first = v.listElems()[0]; - auto second = v.listElems()[1]; - auto third = v.listElems()[2]; + auto first = v.list()[0]; + auto second = v.list()[1]; + auto third = v.list()[2]; ASSERT_THAT(*first, IsStringEq(" ")); ASSERT_THAT(*second, IsListOfSize(1)); - ASSERT_THAT(*second->listElems()[0], IsStringEq("FOO")); + ASSERT_THAT(*second->list()[0], IsStringEq("FOO")); ASSERT_THAT(*third, IsStringEq(" ")); } @@ -824,14 +824,14 @@ namespace nix { TEST_F(PrimOpTest, match3) { auto v = eval("builtins.match \"a(b)(c)\" \"abc\""); ASSERT_THAT(v, IsListOfSize(2)); - ASSERT_THAT(*v.listElems()[0], IsStringEq("b")); - ASSERT_THAT(*v.listElems()[1], IsStringEq("c")); + ASSERT_THAT(*v.list()[0], IsStringEq("b")); + ASSERT_THAT(*v.list()[1], IsStringEq("c")); } TEST_F(PrimOpTest, match4) { auto v = eval("builtins.match \"[[:space:]]+([[:upper:]]+)[[:space:]]+\" \" FOO \""); ASSERT_THAT(v, IsListOfSize(1)); - ASSERT_THAT(*v.listElems()[0], IsStringEq("FOO")); + ASSERT_THAT(*v.list()[0], IsStringEq("FOO")); } TEST_F(PrimOpTest, match5) { @@ -848,7 +848,7 @@ namespace nix { // ensure that the list is sorted const std::vector expected { "a", "x", "y", "z" }; - for (const auto [n, elem] : enumerate(v.listItems())) + for (const auto [n, elem] : enumerate(v.list())) ASSERT_THAT(*elem, IsStringEq(expected[n])); } diff --git a/src/libexpr-tests/value/print.cc b/src/libexpr-tests/value/print.cc index 43b54503546..9a6c95ad361 100644 --- a/src/libexpr-tests/value/print.cc +++ b/src/libexpr-tests/value/print.cc @@ -79,11 +79,12 @@ TEST_F(ValuePrintingTests, tList) Value vTwo; vTwo.mkInt(2); - auto list = state.buildList(3); - list.elems[0] = &vOne; - list.elems[1] = &vTwo; + // TODO(@connorbaker): Test fails because previously, using ListBuilder and over-allocating would set entries to nullptr. + // That's no longer the case with persistent vectors. + auto list = ValueList({ &vOne, &vTwo, nullptr }); + Value vList; - vList.mkList(list); + vList.mkList(&list); test(vList, "[ 1 2 «nullptr» ]"); } @@ -249,12 +250,9 @@ TEST_F(ValuePrintingTests, depthList) Value vNested; vNested.mkAttrs(builder2.finish()); - auto list = state.buildList(3); - list.elems[0] = &vOne; - list.elems[1] = &vTwo; - list.elems[2] = &vNested; + auto list = ValueList({ &vOne, &vTwo, &vNested }); Value vList; - vList.mkList(list); + vList.mkList(&list); test(vList, "[ 1 2 { ... } ]", PrintOptions { .maxDepth = 1 }); test(vList, "[ 1 2 { nested = { ... }; one = 1; two = 2; } ]", PrintOptions { .maxDepth = 2 }); @@ -539,11 +537,11 @@ TEST_F(ValuePrintingTests, ansiColorsList) Value vTwo; vTwo.mkInt(2); - auto list = state.buildList(3); - list.elems[0] = &vOne; - list.elems[1] = &vTwo; + // TODO(@connorbaker): Test fails because previously, using ListBuilder and over-allocating would set entries to nullptr. + // That's no longer the case with persistent vectors. + auto list = ValueList({ &vOne, &vTwo, nullptr }); Value vList; - vList.mkList(list); + vList.mkList(&list); test(vList, "[ " ANSI_CYAN "1" ANSI_NORMAL " " ANSI_CYAN "2" ANSI_NORMAL " " ANSI_MAGENTA "«nullptr»" ANSI_NORMAL " ]", @@ -670,11 +668,9 @@ TEST_F(ValuePrintingTests, ansiColorsListRepeated) Value vEmpty; vEmpty.mkAttrs(emptyBuilder.finish()); - auto list = state.buildList(2); - list.elems[0] = &vEmpty; - list.elems[1] = &vEmpty; + auto list = ValueList({ &vEmpty, &vEmpty }); Value vList; - vList.mkList(list); + vList.mkList(&list); test(vList, "[ { } " ANSI_MAGENTA "«repeated»" ANSI_NORMAL " ]", @@ -690,11 +686,9 @@ TEST_F(ValuePrintingTests, listRepeated) Value vEmpty; vEmpty.mkAttrs(emptyBuilder.finish()); - auto list = state.buildList(2); - list.elems[0] = &vEmpty; - list.elems[1] = &vEmpty; + auto list = ValueList({ &vEmpty, &vEmpty }); Value vList; - vList.mkList(list); + vList.mkList(&list); test(vList, "[ { } «repeated» ]", PrintOptions { }); test(vList, @@ -751,11 +745,9 @@ TEST_F(ValuePrintingTests, ansiColorsListElided) vTwo.mkInt(2); { - auto list = state.buildList(2); - list.elems[0] = &vOne; - list.elems[1] = &vTwo; + auto list = ValueList({ &vOne, &vTwo }); Value vList; - vList.mkList(list); + vList.mkList(&list); test(vList, "[ " ANSI_CYAN "1" ANSI_NORMAL " " ANSI_FAINT "«1 item elided»" ANSI_NORMAL " ]", @@ -769,12 +761,9 @@ TEST_F(ValuePrintingTests, ansiColorsListElided) vThree.mkInt(3); { - auto list = state.buildList(3); - list.elems[0] = &vOne; - list.elems[1] = &vTwo; - list.elems[2] = &vThree; + auto list = ValueList({ &vOne, &vTwo, &vThree }); Value vList; - vList.mkList(list); + vList.mkList(&list); test(vList, "[ " ANSI_CYAN "1" ANSI_NORMAL " " ANSI_FAINT "«2 items elided»" ANSI_NORMAL " ]", diff --git a/src/libexpr/attr-path.cc b/src/libexpr/attr-path.cc index 2f67260c532..8953eb4dc4a 100644 --- a/src/libexpr/attr-path.cc +++ b/src/libexpr/attr-path.cc @@ -95,7 +95,7 @@ std::pair findAlongAttrPath(EvalState & state, const std::strin if (*attrIndex >= v->listSize()) throw AttrPathNotFound("list index %1% in selection path '%2%' is out of range", *attrIndex, attrPath); - v = v->listElems()[*attrIndex]; + v = v->list()[*attrIndex]; pos = noPos; } diff --git a/src/libexpr/eval-cache.cc b/src/libexpr/eval-cache.cc index ea3319f9939..ffc7b3fd3b6 100644 --- a/src/libexpr/eval-cache.cc +++ b/src/libexpr/eval-cache.cc @@ -6,6 +6,7 @@ #include "store-api.hh" // Need specialization involving `SymbolStr` just in this one module. #include "strings-inline.hh" +#include namespace nix::eval_cache { @@ -718,9 +719,10 @@ std::vector AttrCursor::getListOfStrings() root->state.error("'%s' is not a list", getAttrPathStr()).debugThrow(); std::vector res; - - for (auto & elem : v.listItems()) + res.reserve(v.listSize()); + immer::for_each(v.list(), [&](auto * elem) { res.push_back(std::string(root->state.forceStringNoCtx(*elem, noPos, "while evaluating an attribute for caching"))); + }); if (root->db) cachedValue = {root->db->setListOfStrings(getKey(), res), res}; diff --git a/src/libexpr/eval-gc.cc b/src/libexpr/eval-gc.cc index 07ce05a2c73..45a8cf67cc1 100644 --- a/src/libexpr/eval-gc.cc +++ b/src/libexpr/eval-gc.cc @@ -35,10 +35,7 @@ static void * oomHandler(size_t requested) static inline void initGCReal() { /* Initialise the Boehm garbage collector. */ - - /* Don't look for interior pointers. This reduces the odds of - misdetection a bit. */ - GC_set_all_interior_pointers(0); + GC_set_all_interior_pointers(1); /* We don't have any roots in data segments, so don't scan from there. */ diff --git a/src/libexpr/eval-inline.hh b/src/libexpr/eval-inline.hh index d5ce238b2ed..8ef0acfc235 100644 --- a/src/libexpr/eval-inline.hh +++ b/src/libexpr/eval-inline.hh @@ -1,89 +1,134 @@ #pragma once ///@file +#include "config-expr.hh" #include "print.hh" #include "eval.hh" #include "eval-error.hh" #include "eval-settings.hh" +#include namespace nix { /** * Note: Various places expect the allocated memory to be zeroed. */ -[[gnu::always_inline]] +[[nodiscard]] +[[using gnu: hot, always_inline, returns_nonnull, malloc, alloc_size(1)]] inline void * allocBytes(size_t n) { - void * p; -#if HAVE_BOEHMGC - p = GC_MALLOC(n); -#else - p = calloc(n, 1); -#endif - if (!p) throw std::bad_alloc(); + void * p = nullptr; + + if constexpr (HAVE_BOEHMGC) { + p = GC_MALLOC(n); + } else { + p = calloc(n, 1); + } + + if (p == nullptr) [[unlikely]] { + throw std::bad_alloc(); + } return p; } -[[gnu::always_inline]] -Value * EvalState::allocValue() +[[nodiscard]] +[[using gnu: hot, always_inline, returns_nonnull, malloc]] +inline Value * EvalState::allocValue() { -#if HAVE_BOEHMGC - /* We use the boehm batch allocator to speed up allocations of Values (of which there are many). + void * p = nullptr; + + if constexpr (HAVE_BOEHMGC) { + /* We use the boehm batch allocator to speed up allocations of Values (of which there are many). GC_malloc_many returns a linked list of objects of the given size, where the first word of each object is also the pointer to the next object in the list. This also means that we have to explicitly clear the first word of every object we take. */ - if (!*valueAllocCache) { - *valueAllocCache = GC_malloc_many(sizeof(Value)); - if (!*valueAllocCache) throw std::bad_alloc(); - } + if (*valueAllocCache == nullptr) [[unlikely]] { + *valueAllocCache = GC_malloc_many(sizeof(Value)); + } + + if (*valueAllocCache == nullptr) [[unlikely]] { + throw std::bad_alloc(); + } - /* GC_NEXT is a convenience macro for accessing the first word of an object. - Take the first list item, advance the list to the next item, and clear the next pointer. */ - void * p = *valueAllocCache; - *valueAllocCache = GC_NEXT(p); - GC_NEXT(p) = nullptr; -#else - void * p = allocBytes(sizeof(Value)); -#endif + /* GC_NEXT is a convenience macro for accessing the first word of an object. + Take the first list item, advance the list to the next item, and clear the next pointer. */ + p = *valueAllocCache; + *valueAllocCache = GC_NEXT(p); + GC_NEXT(p) = nullptr; + } else { + p = allocBytes(sizeof(Value)); + } nrValues++; - return (Value *) p; + return static_cast(p); +} + + +[[nodiscard]] +[[using gnu: hot, always_inline, returns_nonnull, malloc]] +inline ValueList * EvalState::allocList() +{ + void * p = nullptr; + + // See the comment in allocValue for an explanation of this block. + if constexpr (HAVE_BOEHMGC) { + if (*listAllocCache == nullptr) [[unlikely]] { + *listAllocCache = GC_malloc_many(sizeof(ValueList)); + } + + if (*listAllocCache == nullptr) [[unlikely]] { + throw std::bad_alloc(); + } + + p = *listAllocCache; + *listAllocCache = GC_NEXT(p); + GC_NEXT(p) = nullptr; + } else { + p = allocBytes(sizeof(ValueList)); + } + + return ::new (p) ValueList; } -[[gnu::always_inline]] -Env & EvalState::allocEnv(size_t size) +[[nodiscard]] +[[using gnu: hot, always_inline, returns_nonnull, malloc]] +inline Env * EvalState::allocEnv(size_t size) { nrEnvs++; nrValuesInEnvs += size; - Env * env; + void * p = nullptr; + + if constexpr (HAVE_BOEHMGC) { + if (size == 1) { + /* see allocValue for explanations. */ + if (*env1AllocCache == nullptr) [[unlikely]] { + *env1AllocCache = GC_malloc_many(sizeof(Env) + sizeof(Value *)); + } -#if HAVE_BOEHMGC - if (size == 1) { - /* see allocValue for explanations. */ - if (!*env1AllocCache) { - *env1AllocCache = GC_malloc_many(sizeof(Env) + sizeof(Value *)); - if (!*env1AllocCache) throw std::bad_alloc(); + if (*env1AllocCache == nullptr) [[unlikely]] { + throw std::bad_alloc(); + } + + p = *env1AllocCache; + *env1AllocCache = GC_NEXT(p); + GC_NEXT(p) = nullptr; } + } - void * p = *env1AllocCache; - *env1AllocCache = GC_NEXT(p); - GC_NEXT(p) = nullptr; - env = (Env *) p; - } else -#endif - env = (Env *) allocBytes(sizeof(Env) + size * sizeof(Value *)); + if (p == nullptr) { + p = allocBytes(sizeof(Env) + size * sizeof(Value *)); + } /* We assume that env->values has been cleared by the allocator; maybeThunk() and lookupVar fromWith expect this. */ - - return *env; + return static_cast(p); } -[[gnu::always_inline]] -void EvalState::forceValue(Value & v, const PosIdx pos) +[[using gnu: hot, always_inline]] +inline void EvalState::forceValue(Value & v, const PosIdx pos) { if (v.isThunk()) { Env * env = v.payload.thunk.env; @@ -98,12 +143,15 @@ void EvalState::forceValue(Value & v, const PosIdx pos) throw; } } - else if (v.isApp()) + else if (v.isApp()) { callFunction(*v.payload.app.left, *v.payload.app.right, v, pos); + } + // TODO(@connorbaker): Somewhere, somehow, an uninitialized value is being forced. + assert(v.isValid()); } -[[gnu::always_inline]] +[[using gnu: hot, always_inline]] inline void EvalState::forceAttrs(Value & v, const PosIdx pos, std::string_view errorCtx) { forceAttrs(v, [&]() { return pos; }, errorCtx); @@ -111,12 +159,12 @@ inline void EvalState::forceAttrs(Value & v, const PosIdx pos, std::string_view template -[[gnu::always_inline]] +[[using gnu: hot, always_inline]] inline void EvalState::forceAttrs(Value & v, Callable getPos, std::string_view errorCtx) { PosIdx pos = getPos(); forceValue(v, pos); - if (v.type() != nAttrs) { + if (v.type() != nAttrs) [[ unlikely ]] { error( "expected a set but found %1%: %2%", showType(v), @@ -126,11 +174,11 @@ inline void EvalState::forceAttrs(Value & v, Callable getPos, std::string_view e } -[[gnu::always_inline]] +[[using gnu: hot, always_inline]] inline void EvalState::forceList(Value & v, const PosIdx pos, std::string_view errorCtx) { forceValue(v, pos); - if (!v.isList()) { + if (!v.isList()) [[ unlikely ]] { error( "expected a list but found %1%: %2%", showType(v), @@ -139,12 +187,14 @@ inline void EvalState::forceList(Value & v, const PosIdx pos, std::string_view e } } -[[gnu::always_inline]] + +[[using gnu: hot, always_inline]] inline CallDepth EvalState::addCallDepth(const PosIdx pos) { - if (callDepth > settings.maxCallDepth) + if (callDepth > settings.maxCallDepth) [[unlikely ]] { error("stack overflow; max-call-depth exceeded").atPos(pos).debugThrow(); + } - return CallDepth(callDepth); + return {callDepth}; }; } diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index e21f70553a7..c54a942534b 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -20,19 +20,26 @@ #include "fetch-to-store.hh" #include "tarball.hh" #include "parser-tab.hh" +#include "value.hh" #include -#include -#include #include -#include -#include -#include #include #include +#include +#include +#include +#include +#include +#include +#include +#include +#include -#include #include +#include +#include +#include #ifndef _WIN32 // TODO use portable implementation # include @@ -273,8 +280,9 @@ EvalState::EvalState( , regexCache(makeRegexCache()) #if HAVE_BOEHMGC , valueAllocCache(std::allocate_shared(traceable_allocator(), nullptr)) + , listAllocCache(std::allocate_shared(traceable_allocator(), nullptr)) , env1AllocCache(std::allocate_shared(traceable_allocator(), nullptr)) - , baseEnvP(std::allocate_shared(traceable_allocator(), &allocEnv(BASE_ENV_SIZE))) + , baseEnvP(std::allocate_shared(traceable_allocator(), allocEnv(BASE_ENV_SIZE))) , baseEnv(**baseEnvP) #else , baseEnv(allocEnv(BASE_ENV_SIZE)) @@ -290,7 +298,7 @@ EvalState::EvalState( static_assert(sizeof(Env) <= 16, "environment must be <= 16 bytes"); - vEmptyList.mkList(buildList(0)); + vEmptyList.mkList(allocList()); vNull.mkNull(); vTrue.mkBool(true); vFalse.mkBool(false); @@ -874,13 +882,6 @@ inline Value * EvalState::lookupVar(Env * env, const ExprVar & var, bool noEval) } } -ListBuilder::ListBuilder(EvalState & state, size_t size) - : size(size) - , elems(size <= 2 ? inlineElems : (Value * *) allocBytes(size * sizeof(Value *))) -{ - state.nrListElems += size; -} - Value * EvalState::getBool(bool b) { return b ? &vTrue : &vFalse; } @@ -1167,29 +1168,30 @@ void ExprPath::eval(EvalState & state, Env & env, Value & v) Env * ExprAttrs::buildInheritFromEnv(EvalState & state, Env & up) { - Env & inheritEnv = state.allocEnv(inheritFromExprs->size()); - inheritEnv.up = &up; + Env * inheritEnv = state.allocEnv(inheritFromExprs->size()); + inheritEnv->up = &up; Displacement displ = 0; - for (auto from : *inheritFromExprs) - inheritEnv.values[displ++] = from->maybeThunk(state, up); + for (auto * from : *inheritFromExprs) { + inheritEnv->values[displ++] = from->maybeThunk(state, up); + } - return &inheritEnv; + return inheritEnv; } void ExprAttrs::eval(EvalState & state, Env & env, Value & v) { auto bindings = state.buildBindings(attrs.size() + dynamicAttrs.size()); - auto dynamicEnv = &env; + auto * dynamicEnv = &env; bool sort = false; if (recursive) { /* Create a new environment that contains the attributes in this `rec'. */ - Env & env2(state.allocEnv(attrs.size())); - env2.up = &env; - dynamicEnv = &env2; - Env * inheritEnv = inheritFromExprs ? buildInheritFromEnv(state, env2) : nullptr; + Env * env2(state.allocEnv(attrs.size())); + env2->up = &env; + dynamicEnv = env2; + Env * inheritEnv = inheritFromExprs ? buildInheritFromEnv(state, *env2) : nullptr; AttrDefs::iterator overrides = attrs.find(state.sOverrides); bool hasOverrides = overrides != attrs.end(); @@ -1202,10 +1204,11 @@ void ExprAttrs::eval(EvalState & state, Env & env, Value & v) Value * vAttr; if (hasOverrides && i.second.kind != AttrDef::Kind::Inherited) { vAttr = state.allocValue(); - mkThunk(*vAttr, *i.second.chooseByKind(&env2, &env, inheritEnv), i.second.e); - } else - vAttr = i.second.e->maybeThunk(state, *i.second.chooseByKind(&env2, &env, inheritEnv)); - env2.values[displ++] = vAttr; + mkThunk(*vAttr, *i.second.chooseByKind(env2, &env, inheritEnv), i.second.e); + } else { + vAttr = i.second.e->maybeThunk(state, *i.second.chooseByKind(env2, &env, inheritEnv)); + } + env2->values[displ++] = vAttr; bindings.insert(i.first, vAttr, i.second.pos); } @@ -1225,7 +1228,7 @@ void ExprAttrs::eval(EvalState & state, Env & env, Value & v) AttrDefs::iterator j = attrs.find(i.name); if (j != attrs.end()) { (*bindings.bindings)[j->second.displ] = i; - env2.values[j->second.displ] = i.value; + env2->values[j->second.displ] = i.value; } else bindings.push_back(i); } @@ -1273,26 +1276,26 @@ void ExprLet::eval(EvalState & state, Env & env, Value & v) { /* Create a new environment that contains the attributes in this `let'. */ - Env & env2(state.allocEnv(attrs->attrs.size())); - env2.up = &env; + Env * env2(state.allocEnv(attrs->attrs.size())); + env2->up = &env; - Env * inheritEnv = attrs->inheritFromExprs ? attrs->buildInheritFromEnv(state, env2) : nullptr; + Env * inheritEnv = attrs->inheritFromExprs ? attrs->buildInheritFromEnv(state, *env2) : nullptr; /* The recursive attributes are evaluated in the new environment, while the inherited attributes are evaluated in the original environment. */ Displacement displ = 0; for (auto & i : attrs->attrs) { - env2.values[displ++] = i.second.e->maybeThunk( + env2->values[displ++] = i.second.e->maybeThunk( state, - *i.second.chooseByKind(&env2, &env, inheritEnv)); + *i.second.chooseByKind(env2, &env, inheritEnv)); } auto dts = state.debugRepl ? makeDebugTraceStacker( state, *this, - env2, + *env2, getPos() ? std::make_shared(state.positions[getPos()]) : nullptr, @@ -1301,16 +1304,32 @@ void ExprLet::eval(EvalState & state, Env & env, Value & v) ) : nullptr; - body->eval(state, env2, v); + body->eval(state, *env2, v); } void ExprList::eval(EvalState & state, Env & env, Value & v) { - auto list = state.buildList(elems.size()); - for (const auto & [n, v2] : enumerate(list)) - v2 = elems[n]->maybeThunk(state, env); - v.mkList(list); + if (elems.empty()) { + v = state.vEmptyList; + return; + } + // TODO(@connorbaker): Tried to switch to using transient here, but started getting this error when calling transience.push_back: + // nix: /nix/store/gkghbi5d6849mwsbcdhnqljz2xnjvnis-immer-0.8.1-unstable-2024-09-18/include/immer/transience/gc_transience_policy.hpp:95: + // ownee &immer::gc_transience_policy::apply>::type::ownee::operator=(edit) + // [HeapPolicy = immer::heap_policy]: Assertion `e != noone' failed. + // That's an indication the value is being used after it's been freed. Not sure why that's happening. + // NOTE: Running with GC_DONT_GC=1 doesn't seem to trigger the error, so it's likely a GC issue. + v.mkList(ranges::fold_left(elems, state.allocList(), [&](auto * const list, auto * const elem) { + *list = std::move(*list).push_back(elem->maybeThunk(state, env)); + return list; + })); + // TODO(@connorbaker): This doesn't work either, which leads me to suspect the implementation of the mutable data + // structure isn't correctly interfacing with the GC. Is this because it is intended to store values, rather than + // references to values like I'm using it for currently? + // v.mkList(state.allocListFromRange(elems | ranges::views::transform([&](const auto & elem) { + // return elem->maybeThunk(state, env); + // }))); } @@ -1493,13 +1512,13 @@ void EvalState::callFunction(Value & fun, size_t nrArgs, Value * * args, Value & auto size = (!lambda.arg ? 0 : 1) + (lambda.hasFormals() ? lambda.formals->formals.size() : 0); - Env & env2(allocEnv(size)); - env2.up = vCur.payload.lambda.env; + Env * env2(allocEnv(size)); + env2->up = vCur.payload.lambda.env; Displacement displ = 0; if (!lambda.hasFormals()) - env2.values[displ++] = args[0]; + env2->values[displ++] = args[0]; else { try { forceAttrs(*args[0], lambda.pos, "while evaluating the value passed for the lambda argument"); @@ -1509,7 +1528,7 @@ void EvalState::callFunction(Value & fun, size_t nrArgs, Value * * args, Value & } if (lambda.arg) - env2.values[displ++] = args[0]; + env2->values[displ++] = args[0]; /* For each formal argument, get the actual argument. If there is no matching actual argument but the formal @@ -1527,10 +1546,10 @@ void EvalState::callFunction(Value & fun, size_t nrArgs, Value * * args, Value & .withFrame(*fun.payload.lambda.env, lambda) .debugThrow(); } - env2.values[displ++] = i.def->maybeThunk(*this, env2); + env2->values[displ++] = i.def->maybeThunk(*this, *env2); } else { attrsUsed++; - env2.values[displ++] = j->value; + env2->values[displ++] = j->value; } } @@ -1539,7 +1558,7 @@ void EvalState::callFunction(Value & fun, size_t nrArgs, Value * * args, Value & if (!lambda.formals->ellipsis && attrsUsed != args[0]->attrs()->size()) { /* Nope, so show the first unexpected argument to the user. */ - for (auto & i : *args[0]->attrs()) + for (const auto & i : *args[0]->attrs()) if (!lambda.formals->has(i.name)) { std::set formalNames; for (auto & formal : lambda.formals->formals) @@ -1565,14 +1584,14 @@ void EvalState::callFunction(Value & fun, size_t nrArgs, Value * * args, Value & try { auto dts = debugRepl ? makeDebugTraceStacker( - *this, *lambda.body, env2, positions[lambda.pos], + *this, *lambda.body, *env2, positions[lambda.pos], "while calling %s", lambda.name ? concatStrings("'", symbols[lambda.name], "'") : "anonymous lambda") : nullptr; - lambda.body->eval(*this, env2, vCur); + lambda.body->eval(*this, *env2, vCur); } catch (Error & e) { if (loggerSettings.showTrace.get()) { addErrorTrace( @@ -1788,11 +1807,11 @@ values, or passed explicitly with '--arg' or '--argstr'. See void ExprWith::eval(EvalState & state, Env & env, Value & v) { - Env & env2(state.allocEnv(1)); - env2.up = &env; - env2.values[0] = attrs->maybeThunk(state, env); + Env * env2(state.allocEnv(1)); + env2->up = &env; + env2->values[0] = attrs->maybeThunk(state, env); - body->eval(state, env2, v); + body->eval(state, *env2, v); } @@ -1907,40 +1926,45 @@ void ExprOpUpdate::eval(EvalState & state, Env & env, Value & v) void ExprOpConcatLists::eval(EvalState & state, Env & env, Value & v) { - Value v1; e1->eval(state, env, v1); - Value v2; e2->eval(state, env, v2); - Value * lists[2] = { &v1, &v2 }; - state.concatLists(v, 2, lists, pos, "while evaluating one of the elements to concatenate"); -} + state.nrListConcats++; + Value v1; e1->eval(state, env, v1); + state.forceList(v1, pos, "in the left operand of the list concatenation operator"); -void EvalState::concatLists(Value & v, size_t nrLists, Value * const * lists, const PosIdx pos, std::string_view errorCtx) -{ - nrListConcats++; + Value v2; e2->eval(state, env, v2); + state.forceList(v2, pos, "in the right operand of the list concatenation operator"); - Value * nonEmpty = 0; - size_t len = 0; - for (size_t n = 0; n < nrLists; ++n) { - forceList(*lists[n], pos, errorCtx); - auto l = lists[n]->listSize(); - len += l; - if (l) nonEmpty = lists[n]; + if (v1.listSize() == 0) { + v = v2; + } else if (v2.listSize() == 0) { + v = v1; + } else { + auto * const list = state.allocList(); + *list = v1.list() + v2.list(); + v.mkList(list); } +} - if (nonEmpty && len == nonEmpty->listSize()) { - v = *nonEmpty; - return; - } - auto list = buildList(len); - auto out = list.elems; - for (size_t n = 0, pos = 0; n < nrLists; ++n) { - auto l = lists[n]->listSize(); - if (l) - memcpy(out + pos, lists[n]->listElems(), l * sizeof(Value *)); - pos += l; +void EvalState::concatLists(Value & v, const ValueList & lists, const PosIdx pos, std::string_view errorCtx) +{ + // TODO(@connorbaker): Because concatenation is associative, we can reduce the number of concatenations by doing a tree-like + // concatenation instead of a linear one. Consider using a binary search to do so. + const auto numElems = lists.size(); + if (numElems == 0) { + v = vEmptyList; + } else if (numElems == 1) { + auto *const head = lists.front(); + forceList(*head, pos, errorCtx); + v = *head; + } else { + nrListConcats += numElems - 1; + v.mkList(immer::accumulate(lists, allocList(), [&](auto * const concatenated, auto * const list) { + forceList(*list, pos, errorCtx); + *concatenated = std::move(*concatenated) + list->list(); + return concatenated; + })); } - v.mkList(list); } @@ -2083,7 +2107,7 @@ void EvalState::forceValueDeep(Value & v) forceValue(v, v.determinePos(noPos)); if (v.type() == nAttrs) { - for (auto & i : *v.attrs()) + for (const auto & i : *v.attrs()) { try { // If the value is a thunk, we're evaling. Otherwise no trace necessary. auto dts = debugRepl && i.value->isThunk() @@ -2096,11 +2120,16 @@ void EvalState::forceValueDeep(Value & v) addErrorTrace(e, i.pos, "while evaluating the attribute '%1%'", symbols[i.name]); throw; } + } } else if (v.isList()) { - for (auto v2 : v.listItems()) + // TODO(@connorbaker): Replacing this with + // immer::for_each(v.list(), [&](const auto & v) { recurse(*v); }); + // Increases the heap size by a fair amount, potentially because of the lambda capture. + for (auto * const v2 : v.list()) { recurse(*v2); + } } }; @@ -2326,7 +2355,7 @@ BackedStringView EvalState::coerceToString( if (v.isList()) { std::string result; - for (auto [n, v2] : enumerate(v.listItems())) { + for (const auto [n, v2] : enumerate(v.list())) { try { result += *coerceToString(pos, *v2, context, "while evaluating one element of the list", @@ -2337,8 +2366,9 @@ BackedStringView EvalState::coerceToString( } if (n < v.listSize() - 1 /* !!! not quite correct */ - && (!v2->isList() || v2->listSize() != 0)) + && (!v2->isList() || v2->listSize() != 0)) { result += " "; +} } return result; } @@ -2578,9 +2608,10 @@ void EvalState::assertEqValues(Value & v1, Value & v2, const PosIdx pos, std::st ValuePrinter(*this, v2, errorPrintOptions)) .debugThrow(); } + // TODO(@connorbaker): Rewrite to optimize for Immer. for (size_t n = 0; n < v1.listSize(); ++n) { try { - assertEqValues(*v1.listElems()[n], *v2.listElems()[n], pos, errorCtx); + assertEqValues(*v1.list()[n], *v2.list()[n], pos, errorCtx); } catch (Error & e) { e.addTrace(positions[pos], "while comparing list element %d", n); throw; @@ -2729,12 +2760,19 @@ bool EvalState::eqValues(Value & v1, Value & v2, const PosIdx pos, std::string_v case nNull: return true; - case nList: - if (v1.listSize() != v2.listSize()) return false; - for (size_t n = 0; n < v1.listSize(); ++n) - if (!eqValues(*v1.listElems()[n], *v2.listElems()[n], pos, errorCtx)) return false; - return true; - + case nList: { + // TODO(@connorbaker): Cannot inline l1 and l2 because ranges::views::zip_with requires a borrowed range. + // If we do it inline, since v1.list() and v2.list() are rvalues, they're owned ranges. + const auto l1 = v1.list(); + const auto l2 = v2.list(); + return l1.size() == l2.size() + && ranges::all_of( + ranges::views::zip_with( + [&](auto * const a, auto * const b) { return eqValues(*a, *b, pos, errorCtx); }, + l1, + l2), + std::identity{}); + } case nAttrs: { /* If both sets denote a derivation (type = "derivation"), then compare their outPaths. */ diff --git a/src/libexpr/eval.hh b/src/libexpr/eval.hh index a1882dded49..05bf9c8493c 100644 --- a/src/libexpr/eval.hh +++ b/src/libexpr/eval.hh @@ -360,6 +360,11 @@ private: */ std::shared_ptr valueAllocCache; + /** + * Allocation cache for GC'd ValueList objects. + */ + std::shared_ptr listAllocCache; + /** * Allocation cache for size-1 Env objects. */ @@ -708,8 +713,9 @@ public: /** * Allocation primitives. */ - inline Value * allocValue(); - inline Env & allocEnv(size_t size); + ValueList * allocList(); + Value * allocValue(); + Env * allocEnv(size_t size); Bindings * allocBindings(size_t capacity); @@ -718,11 +724,6 @@ public: return BindingsBuilder(*this, allocBindings(capacity)); } - ListBuilder buildList(size_t size) - { - return ListBuilder(*this, size); - } - /** * Return a boolean `Value *` without allocating. */ @@ -774,7 +775,7 @@ public: const SingleDerivedPath & p, Value & v); - void concatLists(Value & v, size_t nrLists, Value * const * lists, const PosIdx pos, std::string_view errorCtx); + void concatLists(Value & v, const ValueList & lists, const PosIdx pos, std::string_view errorCtx); /** * Print statistics, if enabled. @@ -872,7 +873,6 @@ private: friend void prim_split(EvalState & state, const PosIdx pos, Value * * args, Value & v); friend struct Value; - friend class ListBuilder; }; struct DebugTraceStacker { diff --git a/src/libexpr/get-drvs.cc b/src/libexpr/get-drvs.cc index 1ac13fcd2b1..7ffbdf247d4 100644 --- a/src/libexpr/get-drvs.cc +++ b/src/libexpr/get-drvs.cc @@ -5,6 +5,8 @@ #include "path-with-outputs.hh" #include +#include +#include #include @@ -115,38 +117,40 @@ PackageInfo::Outputs PackageInfo::queryOutputs(bool withPaths, bool onlyOutputsT const Attr * i; if (attrs && (i = attrs->get(state->sOutputs))) { state->forceList(*i->value, i->pos, "while evaluating the 'outputs' attribute of a derivation"); - - /* For each output... */ - for (auto elem : i->value->listItems()) { + immer::for_each(i->value->list(), [&](auto * elem) { std::string output(state->forceStringNoCtx(*elem, i->pos, "while evaluating the name of an output of a derivation")); if (withPaths) { /* Evaluate the corresponding set. */ - auto out = attrs->get(state->symbols.create(output)); - if (!out) continue; // FIXME: throw error? + const auto *out = attrs->get(state->symbols.create(output)); + if (!out) return; // FIXME: throw error? state->forceAttrs(*out->value, i->pos, "while evaluating an output of a derivation"); /* And evaluate its ‘outPath’ attribute. */ - auto outPath = out->value->attrs()->get(state->sOutPath); - if (!outPath) continue; // FIXME: throw error? + const auto *outPath = out->value->attrs()->get(state->sOutPath); + if (!outPath) return; // FIXME: throw error? NixStringContext context; outputs.emplace(output, state->coerceToStorePath(outPath->pos, *outPath->value, context, "while evaluating an output path of a derivation")); - } else + } else { outputs.emplace(output, std::nullopt); - } - } else + } + }); + } else { outputs.emplace("out", withPaths ? std::optional{queryOutPath()} : std::nullopt); + } } - if (!onlyOutputsToInstall || !attrs) + if (!onlyOutputsToInstall || (attrs == nullptr)) { return outputs; + } const Attr * i; if (attrs && (i = attrs->get(state->sOutputSpecified)) && state->forceBool(*i->value, i->pos, "while evaluating the 'outputSpecified' attribute of a derivation")) { Outputs result; auto out = outputs.find(queryOutputName()); - if (out == outputs.end()) + if (out == outputs.end()) { throw Error("derivation does not have output '%s'", queryOutputName()); + } result.insert(*out); return result; } @@ -159,12 +163,12 @@ PackageInfo::Outputs PackageInfo::queryOutputs(bool withPaths, bool onlyOutputsT /* ^ this shows during `nix-env -i` right under the bad derivation */ if (!outTI->isList()) throw errMsg; Outputs result; - for (auto elem : outTI->listItems()) { + immer::for_each(outTI->list(), [&](const auto & elem) { if (elem->type() != nString) throw errMsg; auto out = outputs.find(elem->c_str()); if (out == outputs.end()) throw errMsg; result.insert(*out); - } + }); return result; } } @@ -206,17 +210,22 @@ bool PackageInfo::checkMeta(Value & v) { state->forceValue(v, v.determinePos(noPos)); if (v.type() == nList) { - for (auto elem : v.listItems()) - if (!checkMeta(*elem)) return false; - return true; + // TODO(@connorbaker): Does immer::all_of support short-circuiting? + return ranges::all_of(v.list(), [&](const auto & elem) { + return checkMeta(*elem); + }); } - else if (v.type() == nAttrs) { - if (v.attrs()->get(state->sOutPath)) return false; - for (auto & i : *v.attrs()) - if (!checkMeta(*i.value)) return false; - return true; + + if (v.type() == nAttrs) { + if (v.attrs()->get(state->sOutPath) != nullptr) { + return false; + } + return ranges::all_of(*v.attrs(), [&](const auto & i) { + return checkMeta(*i.value); + }); } - else return v.type() == nInt || v.type() == nBool || v.type() == nString || + + return v.type() == nInt || v.type() == nBool || v.type() == nString || v.type() == nFloat; } @@ -224,7 +233,7 @@ bool PackageInfo::checkMeta(Value & v) Value * PackageInfo::queryMeta(const std::string & name) { if (!getMeta()) return 0; - auto a = meta->get(state->symbols.create(name)); + const auto *a = meta->get(state->symbols.create(name)); if (!a || !checkMeta(*a->value)) return 0; return a->value; } @@ -400,7 +409,7 @@ static void getDerivations(EvalState & state, Value & vIn, } else if (v.type() == nList) { - for (auto [n, elem] : enumerate(v.listItems())) { + for (auto [n, elem] : enumerate(v.list())) { std::string pathPrefix2 = addToPath(pathPrefix, fmt("%d", n)); if (getDerivation(state, *elem, pathPrefix2, drvs, done, ignoreAssertionFailures)) getDerivations(state, *elem, pathPrefix2, autoArgs, drvs, done, ignoreAssertionFailures); diff --git a/src/libexpr/json-to-value.cc b/src/libexpr/json-to-value.cc index 9ac56541ab9..0206b5fac5e 100644 --- a/src/libexpr/json-to-value.cc +++ b/src/libexpr/json-to-value.cc @@ -3,6 +3,7 @@ #include "eval.hh" #include +#include #include #include @@ -58,10 +59,10 @@ class JSONSax : nlohmann::json_sax { ValueVector values; std::unique_ptr resolve(EvalState & state) override { - auto list = state.buildList(values.size()); - for (const auto & [n, v2] : enumerate(list)) - v2 = values[n]; - parent->value(state).mkList(list); + parent->value(state).mkList(ranges::fold_left(values, state.allocList(), [](auto * const list, auto * const v) { + *list = std::move(*list).push_back(v); + return list; + })); return std::move(parent); } void add() override { diff --git a/src/libexpr/meson.build b/src/libexpr/meson.build index 4d8a38b435c..d3767622127 100644 --- a/src/libexpr/meson.build +++ b/src/libexpr/meson.build @@ -2,6 +2,8 @@ project('nix-expr', 'cpp', version : files('.version'), default_options : [ 'cpp_std=c++2a', + # Set -Wno-array-bounds to avoid warnings in Immer's generated code. + 'cpp_args=-Warray-bounds=0', # TODO(Qyriad): increase the warning level 'warning_level=1', 'debug=true', @@ -38,6 +40,22 @@ boost = dependency( # put in `deps_other`. deps_other += boost +immer = dependency( + 'Immer', + version : '>=0.8.0', + method : 'cmake', + include_type: 'system', +) +deps_other += immer + +range_v3 = dependency( + 'range-v3', + version : '>=0.12.0', + method : 'cmake', + include_type: 'system', +) +deps_other += range_v3 + nlohmann_json = dependency('nlohmann_json', version : '>= 3.9') deps_public += nlohmann_json @@ -53,6 +71,7 @@ if bdw_gc.found() configdata.set(define_name, define_value) endforeach configdata.set('GC_THREADS', 1) + configdata.set('IMMER_HAS_LIBGC', 1) endif configdata.set('HAVE_BOEHMGC', bdw_gc.found().to_int()) diff --git a/src/libexpr/package.nix b/src/libexpr/package.nix index d97e7f3a82f..300d67f0d14 100644 --- a/src/libexpr/package.nix +++ b/src/libexpr/package.nix @@ -4,6 +4,7 @@ , bison , flex +, immer , cmake # for resolving toml11 dep , nix-util @@ -12,6 +13,7 @@ , boost , boehmgc , nlohmann_json +, range-v3 , toml11 # Configuration Options @@ -76,7 +78,9 @@ mkMesonLibrary (finalAttrs: { # Hack for sake of the dev shell passthru.externalPropagatedBuildInputs = [ boost + immer nlohmann_json + range-v3 ] ++ lib.optional enableGC boehmgc; preConfigure = diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 45d9f86ac05..8ebf03286c4 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -16,8 +16,13 @@ #include "fetch-to-store.hh" #include +#include +#include +#include #include - +#include +#include +#include #include #include #include @@ -26,6 +31,8 @@ #include #include #include +#include +#include #ifndef _WIN32 # include @@ -35,6 +42,16 @@ namespace nix { +// TODO(@connorbaker): Be careful when using containers from the standard library, as they need to be configured to use +// the Boehm GC's allocator! Look for and replace usages of vector, pair, map, etc. with the GC versions. + +// TODO(@connorbaker): Look for uses of values after they've been added to a ValueList -- immer uses `std::move` to +// avoid copying, but not sure what the implications are. + +// TODO(@connorbaker): Rewrite functions involving lists to make use of persistence -- currently they're largely written +// in a way that duplicates data. + +// TODO(@connorbaker): Lift placeholder values out of lambdas and into the enclosing scope to avoid repeated construction/destruction. /************************************************************* * Miscellaneous @@ -196,15 +213,16 @@ void derivationToValue(EvalState & state, const PosIdx pos, const SourcePath & p NixStringContextElem::DrvDeep { .drvPath = storePath }, }); attrs.alloc(state.sName).mkString(drv.env["name"]); - - auto list = state.buildList(drv.outputs.size()); - for (const auto & [i, o] : enumerate(drv.outputs)) { - mkOutputString(state, attrs, storePath, o); - (list[i] = state.allocValue())->mkString(o.first); - } - attrs.alloc(state.sOutputs).mkList(list); - - auto w = state.allocValue(); + attrs.alloc(state.sOutputs) + .mkList(ranges::fold_left(drv.outputs, state.allocList(), [&](auto * const list, const auto & output) { + mkOutputString(state, attrs, storePath, output); + auto * const outputString = state.allocValue(); + outputString->mkString(output.first); + *list = std::move(*list).push_back(outputString); + return list; + })); + + auto *w = state.allocValue(); w->mkAttrs(attrs); if (!state.vImportedDrvToDerivation) { @@ -231,7 +249,7 @@ void derivationToValue(EvalState & state, const PosIdx pos, const SourcePath & p static void scopedImport(EvalState & state, const PosIdx pos, SourcePath & path, Value * vScope, Value & v) { state.forceAttrs(*vScope, pos, "while evaluating the first argument passed to builtins.scopedImport"); - Env * env = &state.allocEnv(vScope->attrs()->size()); + Env * env = state.allocEnv(vScope->attrs()->size()); env->up = &state.baseEnv; auto staticEnv = std::make_shared(nullptr, state.staticBaseEnv.get(), vScope->attrs()->size()); @@ -402,8 +420,8 @@ void prim_importNative(EvalState & state, const PosIdx pos, Value * * args, Valu void prim_exec(EvalState & state, const PosIdx pos, Value * * args, Value & v) { state.forceList(*args[0], pos, "while evaluating the first argument passed to builtins.exec"); - auto elems = args[0]->listElems(); - auto count = args[0]->listSize(); + auto elems = args[0]->list(); + auto count = elems.size(); if (count == 0) state.error("at least one argument to 'exec' required").atPos(pos).debugThrow(); NixStringContext context; @@ -411,12 +429,11 @@ void prim_exec(EvalState & state, const PosIdx pos, Value * * args, Value & v) "while evaluating the first element of the argument passed to builtins.exec", false, false).toOwned(); Strings commandArgs; - for (unsigned int i = 1; i < args[0]->listSize(); ++i) { - commandArgs.push_back( - state.coerceToString(pos, *elems[i], context, - "while evaluating an element of the argument passed to builtins.exec", - false, false).toOwned()); - } + immer::for_each(elems.drop(1), [&](auto * const v) { + commandArgs.push_back(state.coerceToString(pos, *v, context, + "while evaluating an element of the argument passed to builtins.exec", + false, false).toOwned()); + }); try { auto _ = state.realiseContext(context); // FIXME: Handle CA derivations } catch (InvalidPathError & e) { @@ -424,7 +441,7 @@ void prim_exec(EvalState & state, const PosIdx pos, Value * * args, Value & v) } auto output = runProgram(program, true, commandArgs); - Expr * parsed; + Expr * parsed = nullptr; try { parsed = state.parseExprFromString(std::move(output), state.rootPath(CanonPath::root)); } catch (Error & e) { @@ -642,15 +659,10 @@ struct CompareValues return strcmp(v1->payload.path.path, v2->payload.path.path) < 0; case nList: // Lexicographic comparison - for (size_t i = 0;; i++) { - if (i == v2->listSize()) { - return false; - } else if (i == v1->listSize()) { - return true; - } else if (!state.eqValues(*v1->listElems()[i], *v2->listElems()[i], pos, errorCtx)) { - return (*this)(v1->listElems()[i], v2->listElems()[i], "while comparing two list elements"); - } - } + return ranges::lexicographical_compare(v1->list(), v2->list(), [&](auto * const a, auto * const b) { + // If they are not equal, return the result of comparing them + return !state.eqValues(*a, *b, pos, errorCtx) && (*this)(a, b, "while comparing two list elements"); + }); default: state.error("cannot compare %s with %s; values of that type are incomparable", showType(*v1), showType(*v2)).debugThrow(); #pragma GCC diagnostic pop @@ -663,10 +675,6 @@ struct CompareValues } }; - -typedef std::list> ValueList; - - static Bindings::const_iterator getAttr( EvalState & state, Symbol attrSym, @@ -685,60 +693,54 @@ static void prim_genericClosure(EvalState & state, const PosIdx pos, Value * * a state.forceAttrs(*args[0], noPos, "while evaluating the first argument passed to builtins.genericClosure"); /* Get the start set. */ - auto startSet = getAttr(state, state.sStartSet, args[0]->attrs(), "in the attrset passed as argument to builtins.genericClosure"); + const auto *const startSet = getAttr(state, state.sStartSet, args[0]->attrs(), "in the attrset passed as argument to builtins.genericClosure"); state.forceList(*startSet->value, noPos, "while evaluating the 'startSet' attribute passed as argument to builtins.genericClosure"); - ValueList workSet; - for (auto elem : startSet->value->listItems()) - workSet.push_back(elem); - if (startSet->value->listSize() == 0) { v = *startSet->value; return; } /* Get the operator. */ - auto op = getAttr(state, state.sOperator, args[0]->attrs(), "in the attrset passed as argument to builtins.genericClosure"); + const auto *const op = getAttr(state, state.sOperator, args[0]->attrs(), "in the attrset passed as argument to builtins.genericClosure"); state.forceFunction(*op->value, noPos, "while evaluating the 'operator' attribute passed as argument to builtins.genericClosure"); /* Construct the closure by applying the operator to elements of `workSet', adding the result to `workSet', continuing until no new elements are found. */ - ValueList res; + auto * const res = state.allocList(); // `doneKeys' doesn't need to be a GC root, because its values are // reachable from res. - auto cmp = CompareValues(state, noPos, "while comparing the `key` attributes of two genericClosure elements"); - std::set doneKeys(cmp); + const auto cmp = CompareValues(state, noPos, "while comparing the `key` attributes of two genericClosure elements"); + std::set> doneKeys(cmp); + + Value newElements; + auto workSet = startSet->value->list(); while (!workSet.empty()) { - Value * e = *(workSet.begin()); - workSet.pop_front(); + auto * e = workSet.front(); + workSet = std::move(workSet).drop(1); state.forceAttrs(*e, noPos, "while evaluating one of the elements generated by (or initially passed to) builtins.genericClosure"); - auto key = getAttr(state, state.sKey, e->attrs(), "in one of the attrsets generated by (or initially passed to) builtins.genericClosure"); + const auto *const key = getAttr(state, state.sKey, e->attrs(), "in one of the attrsets generated by (or initially passed to) builtins.genericClosure"); state.forceValue(*key->value, noPos); if (!doneKeys.insert(key->value).second) continue; - res.push_back(e); + *res = std::move(*res).push_back(e); /* Call the `operator' function with `e' as argument. */ - Value newElements; state.callFunction(*op->value, 1, &e, newElements, noPos); state.forceList(newElements, noPos, "while evaluating the return value of the `operator` passed to builtins.genericClosure"); + immer::for_each(newElements.list(), [&](auto * const elem){ + state.forceValue(*elem, noPos); + }); /* Add the values returned by the operator to the work set. */ - for (auto elem : newElements.listItems()) { - state.forceValue(*elem, noPos); // "while evaluating one one of the elements returned by the `operator` passed to builtins.genericClosure"); - workSet.push_back(elem); - } + workSet = std::move(workSet) + newElements.list(); } - /* Create the result list. */ - auto list = state.buildList(res.size()); - for (const auto & [n, i] : enumerate(res)) - list[n] = i; - v.mkList(list); + v.mkList(res); } static RegisterPrimOp primop_genericClosure(PrimOp { @@ -1324,12 +1326,11 @@ static void derivationStrictInternal( command-line arguments to the builder. */ else if (i->name == state.sArgs) { state.forceList(*i->value, pos, context_below); - for (auto elem : i->value->listItems()) { - auto s = state.coerceToString(pos, *elem, context, + immer::for_each(i->value->list(), [&](auto * const elem){ + drv.args.push_back(state.coerceToString(pos, *elem, context, "while evaluating an element of the argument list", - true).toOwned(); - drv.args.push_back(s); - } + true).toOwned()); + }); } /* All other attributes are passed to the builder through @@ -1356,8 +1357,9 @@ static void derivationStrictInternal( /* Require ‘outputs’ to be a list of strings. */ state.forceList(*i->value, pos, context_below); Strings ss; - for (auto elem : i->value->listItems()) + immer::for_each(i->value->list(), [&](auto * const elem){ ss.emplace_back(state.forceStringNoCtx(*elem, pos, context_below)); + }); handleOutputs(ss); } @@ -1831,18 +1833,17 @@ static void prim_findFile(EvalState & state, const PosIdx pos, Value * * args, V state.forceList(*args[0], pos, "while evaluating the first argument passed to builtins.findFile"); LookupPath lookupPath; - - for (auto v2 : args[0]->listItems()) { + std::string prefix; + NixStringContext context; + immer::for_each(args[0]->list(), [&](auto * const v2){ state.forceAttrs(*v2, pos, "while evaluating an element of the list passed to builtins.findFile"); - std::string prefix; auto i = v2->attrs()->find(state.sPrefix); if (i != v2->attrs()->end()) prefix = state.forceStringNoCtx(*i->value, pos, "while evaluating the `prefix` attribute of an element of the list passed to builtins.findFile"); i = getAttr(state, state.sPath, v2->attrs(), "in an element of the __nixPath"); - NixStringContext context; auto path = state.coerceToString(pos, *i->value, context, "while evaluating the `path` attribute of an element of the list passed to builtins.findFile", false, false).toOwned(); @@ -1862,7 +1863,7 @@ static void prim_findFile(EvalState & state, const PosIdx pos, Value * * args, V .prefix = LookupPath::Prefix { .s = prefix }, .path = LookupPath::Path { .s = path }, }); - } + }); auto path = state.forceStringNoCtx(*args[1], pos, "while evaluating the second argument passed to builtins.findFile"); @@ -2674,16 +2675,19 @@ static RegisterPrimOp primop_path({ static void prim_attrNames(EvalState & state, const PosIdx pos, Value * * args, Value & v) { state.forceAttrs(*args[0], pos, "while evaluating the argument passed to builtins.attrNames"); - - auto list = state.buildList(args[0]->attrs()->size()); - - for (const auto & [n, i] : enumerate(*args[0]->attrs())) - (list[n] = state.allocValue())->mkString(state.symbols[i.name]); - - std::sort(list.begin(), list.end(), - [](Value * v1, Value * v2) { return strcmp(v1->c_str(), v2->c_str()) < 0; }); - - v.mkList(list); + const auto *const attrs = args[0]->attrs(); + + const auto symbols = *attrs + | ranges::views::transform([&](const auto & attr) { return std::string_view(state.symbols[attr.name]); }) + | ranges::to + | ranges::actions::sort; + + v.mkList(ranges::fold_left(symbols, state.allocList(), [&](auto * const list, const auto & symbol) { + auto * const v = state.allocValue(); + v->mkString(symbol); + *list = std::move(*list).push_back(v); + return list; + })); } static RegisterPrimOp primop_attrNames({ @@ -2703,22 +2707,16 @@ static void prim_attrValues(EvalState & state, const PosIdx pos, Value * * args, { state.forceAttrs(*args[0], pos, "while evaluating the argument passed to builtins.attrValues"); - auto list = state.buildList(args[0]->attrs()->size()); - - for (const auto & [n, i] : enumerate(*args[0]->attrs())) - list[n] = (Value *) &i; - - std::sort(list.begin(), list.end(), - [&](Value * v1, Value * v2) { - std::string_view s1 = state.symbols[((Attr *) v1)->name], - s2 = state.symbols[((Attr *) v2)->name]; - return s1 < s2; + const auto attrs = *args[0]->attrs() + | ranges::to + | ranges::actions::sort([&](const auto & a, const auto & b) { + return std::string_view(state.symbols[a.name]) < std::string_view(state.symbols[b.name]); }); - for (auto & v : list) - v = ((Attr *) v)->value; - - v.mkList(list); + v.mkList(ranges::fold_left(attrs, state.allocList(), [](auto * const list, const auto & attr) { + *list = std::move(*list).push_back(attr.value); + return list; + })); } static RegisterPrimOp primop_attrValues({ @@ -2869,23 +2867,24 @@ static void prim_removeAttrs(EvalState & state, const PosIdx pos, Value * * args /* Get the attribute names to be removed. We keep them as Attrs instead of Symbols so std::set_difference can be used to remove them from attrs[0]. */ - // 64: large enough to fit the attributes of a derivation - boost::container::small_vector names; - names.reserve(args[1]->listSize()); - for (auto elem : args[1]->listItems()) { - state.forceStringNoCtx(*elem, pos, "while evaluating the values of the second argument passed to builtins.removeAttrs"); - names.emplace_back(state.symbols.create(elem->string_view()), nullptr); - } - std::sort(names.begin(), names.end()); + const auto names = args[1]->list() + | std::views::transform([&](auto * const elem) { + state.forceStringNoCtx(*elem, pos, "while evaluating the values of the second argument passed to builtins.removeAttrs"); + return Attr(state.symbols.create(elem->string_view()), nullptr); + }) + | ranges::to + | ranges::actions::sort(std::less<>()); /* Copy all attributes not in that set. Note that we don't need to sort v.attrs because it's a subset of an already sorted vector. */ auto attrs = state.buildBindings(args[0]->attrs()->size()); - std::set_difference( - args[0]->attrs()->begin(), args[0]->attrs()->end(), - names.begin(), names.end(), - std::back_inserter(attrs)); + std::ranges::set_difference( + *args[0]->attrs(), + names, + std::back_inserter(attrs), + std::less<>() + ); v.mkAttrs(attrs.alreadySorted()); } @@ -2916,21 +2915,21 @@ static void prim_listToAttrs(EvalState & state, const PosIdx pos, Value * * args auto attrs = state.buildBindings(args[0]->listSize()); - std::set seen; + std::set, traceable_allocator> seen; - for (auto v2 : args[0]->listItems()) { + immer::for_each(args[0]->list(), [&](auto * const v2) { state.forceAttrs(*v2, pos, "while evaluating an element of the list passed to builtins.listToAttrs"); - auto j = getAttr(state, state.sName, v2->attrs(), "in a {name=...; value=...;} pair"); + const auto * const j = getAttr(state, state.sName, v2->attrs(), "in a {name=...; value=...;} pair"); - auto name = state.forceStringNoCtx(*j->value, j->pos, "while evaluating the `name` attribute of an element of the list passed to builtins.listToAttrs"); + const auto name = state.forceStringNoCtx(*j->value, j->pos, "while evaluating the `name` attribute of an element of the list passed to builtins.listToAttrs"); - auto sym = state.symbols.create(name); + const auto sym = state.symbols.create(name); if (seen.insert(sym).second) { - auto j2 = getAttr(state, state.sValue, v2->attrs(), "in a {name=...; value=...;} pair"); + const auto * const j2 = getAttr(state, state.sValue, v2->attrs(), "in a {name=...; value=...;} pair"); attrs.insert(sym, j2->value, j2->pos); } - } + }); v.mkAttrs(attrs); } @@ -3048,20 +3047,13 @@ static void prim_catAttrs(EvalState & state, const PosIdx pos, Value * * args, V { auto attrName = state.symbols.create(state.forceStringNoCtx(*args[0], pos, "while evaluating the first argument passed to builtins.catAttrs")); state.forceList(*args[1], pos, "while evaluating the second argument passed to builtins.catAttrs"); - - SmallValueVector res(args[1]->listSize()); - size_t found = 0; - - for (auto v2 : args[1]->listItems()) { + v.mkList(immer::accumulate(args[1]->list(), state.allocList(), [&](auto * const list, auto * const v2) { state.forceAttrs(*v2, pos, "while evaluating an element in the list passed as second argument to builtins.catAttrs"); - if (auto i = v2->attrs()->get(attrName)) - res[found++] = i->value; - } - - auto list = state.buildList(found); - for (unsigned int n = 0; n < found; ++n) - list[n] = res[n]; - v.mkList(list); + if (const auto * const i = v2->attrs()->get(attrName)) { + *list = std::move(*list).push_back(i->value); + } + return list; + })); } static RegisterPrimOp primop_catAttrs({ @@ -3154,52 +3146,29 @@ static RegisterPrimOp primop_mapAttrs({ static void prim_zipAttrsWith(EvalState & state, const PosIdx pos, Value * * args, Value & v) { - // we will first count how many values are present for each given key. - // we then allocate a single attrset and pre-populate it with lists of - // appropriate sizes, stash the pointers to the list elements of each, - // and populate the lists. after that we replace the list in the every - // attribute with the merge function application. this way we need not - // use (slightly slower) temporary storage the GC does not know about. - - struct Item - { - size_t size = 0; - size_t pos = 0; - std::optional list; - }; - - std::map, traceable_allocator>> attrsSeen; + std::map, traceable_allocator>> attrsSeen; state.forceFunction(*args[0], pos, "while evaluating the first argument passed to builtins.zipAttrsWith"); state.forceList(*args[1], pos, "while evaluating the second argument passed to builtins.zipAttrsWith"); - const auto listItems = args[1]->listItems(); - for (auto & vElem : listItems) { + immer::for_each(args[1]->list(), [&](auto * const vElem) { state.forceAttrs(*vElem, noPos, "while evaluating a value of the list passed as second argument to builtins.zipAttrsWith"); - for (auto & attr : *vElem->attrs()) - attrsSeen.try_emplace(attr.name).first->second.size++; - } - - for (auto & [sym, elem] : attrsSeen) - elem.list.emplace(state.buildList(elem.size)); - - for (auto & vElem : listItems) { - for (auto & attr : *vElem->attrs()) { - auto & item = attrsSeen.at(attr.name); - (*item.list)[item.pos++] = attr.value; + for (const auto attr : *vElem->attrs()) { + attrsSeen.try_emplace(attr.name, state.allocList()); + *attrsSeen[attr.name] = std::move(*attrsSeen[attr.name]).push_back(attr.value); } - } + }); auto attrs = state.buildBindings(attrsSeen.size()); - for (auto & [sym, elem] : attrsSeen) { - auto name = state.allocValue(); + for (const auto [sym, list] : attrsSeen) { + auto * const name = state.allocValue(); name->mkString(state.symbols[sym]); - auto call1 = state.allocValue(); + auto * const arg = state.allocValue(); + arg->mkList(list); + auto * const call1 = state.allocValue(); call1->mkApp(args[0], name); - auto call2 = state.allocValue(); - auto arg = state.allocValue(); - arg->mkList(*elem.list); + auto * const call2 = state.allocValue(); call2->mkApp(call1, arg); attrs.insert(sym, call2); } @@ -3269,8 +3238,8 @@ static void elemAt(EvalState & state, const PosIdx pos, Value & list, int n, Val "list index %1% is out of bounds", n ).atPos(pos).debugThrow(); - state.forceValue(*list.listElems()[n], pos); - v = *list.listElems()[n]; + state.forceValue(*list.list()[n], pos); + v = *list.list()[n]; } /* Return the n-1'th element of a list. */ @@ -3293,7 +3262,11 @@ static RegisterPrimOp primop_elemAt({ /* Return the first element of a list. */ static void prim_head(EvalState & state, const PosIdx pos, Value * * args, Value & v) { - elemAt(state, pos, *args[0], 0, v); + state.forceList(*args[0], pos, "while evaluating the first argument passed to builtins.head"); + if (args[0]->listSize() == 0) + state.error("'head' called on an empty list").atPos(pos).debugThrow(); + state.forceValue(*args[0]->list().front(), pos); + v = *args[0]->list().front(); } static RegisterPrimOp primop_head({ @@ -3316,10 +3289,9 @@ static void prim_tail(EvalState & state, const PosIdx pos, Value * * args, Value if (args[0]->listSize() == 0) state.error("'tail' called on an empty list").atPos(pos).debugThrow(); - auto list = state.buildList(args[0]->listSize() - 1); - for (const auto & [n, v] : enumerate(list)) - v = args[0]->listElems()[n + 1]; - v.mkList(list); + auto * const tail = state.allocList(); + *tail = args[0]->list().drop(1); + v.mkList(tail); } static RegisterPrimOp primop_tail({ @@ -3350,11 +3322,12 @@ static void prim_map(EvalState & state, const PosIdx pos, Value * * args, Value state.forceFunction(*args[0], pos, "while evaluating the first argument passed to builtins.map"); - auto list = state.buildList(args[1]->listSize()); - for (const auto & [n, v] : enumerate(list)) - (v = state.allocValue())->mkApp( - args[0], args[1]->listElems()[n]); - v.mkList(list); + v.mkList(immer::accumulate(args[1]->list(), state.allocList(), [&](auto * const list, auto * const v) { + auto * const newV = state.allocValue(); + newV->mkApp(args[0], v); + *list = std::move(*list).push_back(newV); + return list; + })); } static RegisterPrimOp primop_map({ @@ -3387,26 +3360,14 @@ static void prim_filter(EvalState & state, const PosIdx pos, Value * * args, Val state.forceFunction(*args[0], pos, "while evaluating the first argument passed to builtins.filter"); - SmallValueVector vs(args[1]->listSize()); - size_t k = 0; - - bool same = true; - for (unsigned int n = 0; n < args[1]->listSize(); ++n) { - Value res; - state.callFunction(*args[0], *args[1]->listElems()[n], res, noPos); - if (state.forceBool(res, pos, "while evaluating the return value of the filtering function passed to builtins.filter")) - vs[k++] = args[1]->listElems()[n]; - else - same = false; - } - - if (same) - v = *args[1]; - else { - auto list = state.buildList(k); - for (const auto & [n, v] : enumerate(list)) v = vs[n]; - v.mkList(list); - } + Value res; + v.mkList(immer::accumulate(args[1]->list(), state.allocList(), [&](auto * const list, auto * const arg) { + state.callFunction(*args[0], *arg, res, noPos); + if (state.forceBool(res, pos, "while evaluating the return value of the filtering function passed to builtins.filter")) { + *list = std::move(*list).push_back(arg); + } + return list; + })); } static RegisterPrimOp primop_filter({ @@ -3422,14 +3383,11 @@ static RegisterPrimOp primop_filter({ /* Return true if a list contains a given element. */ static void prim_elem(EvalState & state, const PosIdx pos, Value * * args, Value & v) { - bool res = false; state.forceList(*args[1], pos, "while evaluating the second argument passed to builtins.elem"); - for (auto elem : args[1]->listItems()) - if (state.eqValues(*args[0], *elem, pos, "while searching for the presence of the given element in the list")) { - res = true; - break; - } - v.mkBool(res); + // TODO(@connorbaker): immer doesn't provide any_of. + v.mkBool(ranges::any_of(args[1]->list(), [&](auto * const elem) { + return state.eqValues(*args[0], *elem, pos, "while searching for the presence of the given element in the list"); + })); } static RegisterPrimOp primop_elem({ @@ -3446,7 +3404,7 @@ static RegisterPrimOp primop_elem({ static void prim_concatLists(EvalState & state, const PosIdx pos, Value * * args, Value & v) { state.forceList(*args[0], pos, "while evaluating the first argument passed to builtins.concatLists"); - state.concatLists(v, args[0]->listSize(), args[0]->listElems(), pos, "while evaluating a value of the list passed to builtins.concatLists"); + state.concatLists(v, args[0]->list(), pos, "while evaluating a value of the list passed to builtins.concatLists"); } static RegisterPrimOp primop_concatLists({ @@ -3484,7 +3442,7 @@ static void prim_foldlStrict(EvalState & state, const PosIdx pos, Value * * args if (args[2]->listSize()) { Value * vCur = args[1]; - for (auto [n, elem] : enumerate(args[2]->listItems())) { + for (auto [n, elem] : enumerate(args[2]->list())) { Value * vs []{vCur, elem}; vCur = n == args[2]->listSize() - 1 ? &v : state.allocValue(); state.callFunction(*args[0], 2, vs, *vCur, pos); @@ -3494,6 +3452,17 @@ static void prim_foldlStrict(EvalState & state, const PosIdx pos, Value * * args state.forceValue(*args[1], pos); v = *args[1]; } + + // TODO(@connorbaker): Find a way to rewrite this so it doesn't need to allocate a new value through each iteration. + // Value * vs[3] {state.allocValue(), state.allocValue(), state.allocValue()}; + // *vs[2] = *args[1]; + // for (size_t n = 0; n < args[2]->listSize(); ++n) { + // *vs[0] = *vs[2]; + // *vs[1] = *args[2]->list()[n]; + // state.callFunction(*args[0], 2, vs, *vs[2], pos); + // } + // v = *vs[2]; + // state.forceValue(v, pos); } static RegisterPrimOp primop_foldlStrict({ @@ -3516,32 +3485,15 @@ static RegisterPrimOp primop_foldlStrict({ .fun = prim_foldlStrict, }); -static void anyOrAll(bool any, EvalState & state, const PosIdx pos, Value * * args, Value & v) -{ - state.forceFunction(*args[0], pos, std::string("while evaluating the first argument passed to builtins.") + (any ? "any" : "all")); - state.forceList(*args[1], pos, std::string("while evaluating the second argument passed to builtins.") + (any ? "any" : "all")); - - std::string_view errorCtx = any - ? "while evaluating the return value of the function passed to builtins.any" - : "while evaluating the return value of the function passed to builtins.all"; - - Value vTmp; - for (auto elem : args[1]->listItems()) { - state.callFunction(*args[0], *elem, vTmp, pos); - bool res = state.forceBool(vTmp, pos, errorCtx); - if (res == any) { - v.mkBool(any); - return; - } - } - - v.mkBool(!any); -} - - static void prim_any(EvalState & state, const PosIdx pos, Value * * args, Value & v) { - anyOrAll(true, state, pos, args, v); + state.forceFunction(*args[0], pos, std::string("while evaluating the first argument passed to builtins.any")); + state.forceList(*args[1], pos, std::string("while evaluating the second argument passed to builtins.any")); + std::string_view errorCtx = "while evaluating the return value of the function passed to builtins.any"; + v.mkBool(ranges::any_of(args[1]->list(), [&](auto * const elem) { + state.callFunction(*args[0], *elem, v, pos); + return state.forceBool(v, pos, errorCtx); + })); } static RegisterPrimOp primop_any({ @@ -3556,7 +3508,13 @@ static RegisterPrimOp primop_any({ static void prim_all(EvalState & state, const PosIdx pos, Value * * args, Value & v) { - anyOrAll(false, state, pos, args, v); + state.forceFunction(*args[0], pos, std::string("while evaluating the first argument passed to builtins.all")); + state.forceList(*args[1], pos, std::string("while evaluating the second argument passed to builtins.all")); + std::string_view errorCtx = "while evaluating the return value of the function passed to builtins.all"; + v.mkBool(ranges::all_of(args[1]->list(), [&](auto * const elem) { + state.callFunction(*args[0], *elem, v, pos); + return state.forceBool(v, pos, errorCtx); + })); } static RegisterPrimOp primop_all({ @@ -3582,13 +3540,14 @@ static void prim_genList(EvalState & state, const PosIdx pos, Value * * args, Va // as evaluating map without accessing any values makes little sense. state.forceFunction(*args[0], noPos, "while evaluating the first argument passed to builtins.genList"); - auto list = state.buildList(len); - for (const auto & [n, v] : enumerate(list)) { - auto arg = state.allocValue(); + v.mkList(ranges::fold_left(ranges::views::iota(size_t(0), len), state.allocList(), [&](auto * const list, const auto n) { + auto * const arg = state.allocValue(); arg->mkInt(n); - (v = state.allocValue())->mkApp(args[0], arg); - } - v.mkList(list); + auto * const app = state.allocValue(); + app->mkApp(args[0], arg); + *list = std::move(*list).push_back(app); + return list; + })); } static RegisterPrimOp primop_genList({ @@ -3622,17 +3581,21 @@ static void prim_sort(EvalState & state, const PosIdx pos, Value * * args, Value state.forceFunction(*args[0], pos, "while evaluating the first argument passed to builtins.sort"); - auto list = state.buildList(len); - for (const auto & [n, v] : enumerate(list)) - state.forceValue(*(v = args[1]->listElems()[n]), pos); + ValueVector vec; + vec.reserve(len); + immer::for_each(args[1]->list(), [&](auto * const v) { + state.forceValue(*v, pos); + vec.push_back(v); + }); - auto comparator = [&](Value * a, Value * b) { + auto comparator = [&](Value * const a, Value * const b) { /* Optimization: if the comparator is lessThan, bypass callFunction. */ if (args[0]->isPrimOp()) { - auto ptr = args[0]->primOp()->fun.target(); - if (ptr && *ptr == prim_lessThan) + const auto * const ptr = args[0]->primOp()->fun.target(); + if ((ptr != nullptr) && *ptr == prim_lessThan) { return CompareValues(state, noPos, "while evaluating the ordering function passed to builtins.sort")(a, b); + } } Value * vs[] = {a, b}; @@ -3644,9 +3607,12 @@ static void prim_sort(EvalState & state, const PosIdx pos, Value * * args, Value /* FIXME: std::sort can segfault if the comparator is not a strict weak ordering. What to do? std::stable_sort() seems more resilient, but no guarantees... */ - std::stable_sort(list.begin(), list.end(), comparator); + std::stable_sort(vec.begin(), vec.end(), comparator); - v.mkList(list); + v.mkList(ranges::fold_left(vec, state.allocList(), [&](auto * const list, auto * const v) { + *list = std::move(*list).push_back(v); + return list; + })); } static RegisterPrimOp primop_sort({ @@ -3675,33 +3641,24 @@ static void prim_partition(EvalState & state, const PosIdx pos, Value * * args, state.forceFunction(*args[0], pos, "while evaluating the first argument passed to builtins.partition"); state.forceList(*args[1], pos, "while evaluating the second argument passed to builtins.partition"); - auto len = args[1]->listSize(); - - ValueVector right, wrong; + auto * const rlist = state.allocList(); + auto * const wlist = state.allocList(); - for (unsigned int n = 0; n < len; ++n) { - auto vElem = args[1]->listElems()[n]; - state.forceValue(*vElem, pos); - Value res; - state.callFunction(*args[0], *vElem, res, pos); - if (state.forceBool(res, pos, "while evaluating the return value of the partition function passed to builtins.partition")) - right.push_back(vElem); - else - wrong.push_back(vElem); - } + Value res; + immer::for_each(args[1]->list(), [&](auto * const v) { + state.forceValue(*v, pos); + state.callFunction(*args[0], *v, res, pos); + if (state.forceBool(res, pos, "while evaluating the return value of the partition function passed to builtins.partition")) { + *rlist = std::move(*rlist).push_back(v); + } + else { + *wlist = std::move(*wlist).push_back(v); + } + }); auto attrs = state.buildBindings(2); - auto rsize = right.size(); - auto rlist = state.buildList(rsize); - if (rsize) - memcpy(rlist.elems, right.data(), sizeof(Value *) * rsize); attrs.alloc(state.sRight).mkList(rlist); - - auto wsize = wrong.size(); - auto wlist = state.buildList(wsize); - if (wsize) - memcpy(wlist.elems, wrong.data(), sizeof(Value *) * wsize); attrs.alloc(state.sWrong).mkList(wlist); v.mkAttrs(attrs); @@ -3735,27 +3692,24 @@ static void prim_groupBy(EvalState & state, const PosIdx pos, Value * * args, Va state.forceFunction(*args[0], pos, "while evaluating the first argument passed to builtins.groupBy"); state.forceList(*args[1], pos, "while evaluating the second argument passed to builtins.groupBy"); - ValueVectorMap attrs; + // TODO(@connorbaker): Rewrite and optimize this function. + std::map, traceable_allocator>> attrsSeen; - for (auto vElem : args[1]->listItems()) { - Value res; + Value res; + immer::for_each(args[1]->list(), [&](auto * const vElem) { state.callFunction(*args[0], *vElem, res, pos); - auto name = state.forceStringNoCtx(res, pos, "while evaluating the return value of the grouping function passed to builtins.groupBy"); - auto sym = state.symbols.create(name); - auto vector = attrs.try_emplace(sym, ValueVector()).first; - vector->second.push_back(vElem); - } + const auto name = state.forceStringNoCtx(res, pos, "while evaluating the return value of the grouping function passed to builtins.groupBy"); + const auto sym = state.symbols.create(name); - auto attrs2 = state.buildBindings(attrs.size()); - - for (auto & i : attrs) { - auto size = i.second.size(); - auto list = state.buildList(size); - memcpy(list.elems, i.second.data(), sizeof(Value *) * size); - attrs2.alloc(i.first).mkList(list); - } + attrsSeen.try_emplace(sym, state.allocList()); + *attrsSeen[sym] = std::move(*attrsSeen[sym]).push_back(vElem); + }); - v.mkAttrs(attrs2.alreadySorted()); + auto attrs = state.buildBindings(attrsSeen.size()); + ranges::for_each(attrsSeen, [&](const auto pair) { + attrs.alloc(pair.first).mkList(pair.second); + }); + v.mkAttrs(attrs.alreadySorted()); } static RegisterPrimOp primop_groupBy({ @@ -3786,28 +3740,13 @@ static void prim_concatMap(EvalState & state, const PosIdx pos, Value * * args, { state.forceFunction(*args[0], pos, "while evaluating the first argument passed to builtins.concatMap"); state.forceList(*args[1], pos, "while evaluating the second argument passed to builtins.concatMap"); - auto nrLists = args[1]->listSize(); - - // List of returned lists before concatenation. References to these Values must NOT be persisted. - SmallTemporaryValueVector lists(nrLists); - size_t len = 0; - - for (unsigned int n = 0; n < nrLists; ++n) { - Value * vElem = args[1]->listElems()[n]; - state.callFunction(*args[0], *vElem, lists[n], pos); - state.forceList(lists[n], lists[n].determinePos(args[0]->determinePos(pos)), "while evaluating the return value of the function passed to builtins.concatMap"); - len += lists[n].listSize(); - } - - auto list = state.buildList(len); - auto out = list.elems; - for (unsigned int n = 0, pos = 0; n < nrLists; ++n) { - auto l = lists[n].listSize(); - if (l) - memcpy(out + pos, lists[n].listElems(), l * sizeof(Value *)); - pos += l; - } - v.mkList(list); + Value res; + v.mkList(immer::accumulate(args[1]->list(), state.allocList(), [&](auto * const list, auto * const v) { + state.callFunction(*args[0], *v, res, pos); + state.forceList(res, res.determinePos(args[0]->determinePos(pos)), "while evaluating the return value of the function passed to builtins.concatMap"); + *list = std::move(*list) + res.list(); + return list; + })); } static RegisterPrimOp primop_concatMap({ @@ -4292,20 +4231,17 @@ void prim_match(EvalState & state, const PosIdx pos, Value * * args, Value & v) NixStringContext context; const auto str = state.forceString(*args[1], context, pos, "while evaluating the second argument passed to builtins.match"); - std::cmatch match; - if (!std::regex_match(str.begin(), str.end(), match, regex)) { + std::cmatch matches; + if (!std::regex_match(str.begin(), str.end(), matches, regex)) { v.mkNull(); return; } // the first match is the whole string - auto list = state.buildList(match.size() - 1); - for (const auto & [i, v2] : enumerate(list)) - if (!match[i + 1].matched) - v2 = &state.vNull; - else - v2 = mkString(state, match[i + 1]); - v.mkList(list); + v.mkList(ranges::fold_left(std::span(matches).subspan(1), state.allocList(), [&](auto * const listOfMatches, const auto match) { + *listOfMatches = std::move(*listOfMatches).push_back(match.matched ? mkString(state, match) : &state.vNull); + return listOfMatches; + })); } catch (std::regex_error & e) { if (e.code() == std::regex_constants::error_space) { @@ -4374,44 +4310,48 @@ void prim_split(EvalState & state, const PosIdx pos, Value * * args, Value & v) // Any matches results are surrounded by non-matching results. const size_t len = std::distance(begin, end); - auto list = state.buildList(2 * len + 1); - size_t idx = 0; + auto * const listOfMatches = state.allocList(); if (len == 0) { - list[0] = args[1]; - v.mkList(list); + *listOfMatches = std::move(*listOfMatches).push_back(args[1]); + v.mkList(listOfMatches); return; } + size_t idx = 0; + for (auto i = begin; i != end; ++i) { assert(idx <= 2 * len + 1 - 3); - auto match = *i; + const auto match = *i; // Add a string for non-matched characters. - list[idx++] = mkString(state, match.prefix()); + *listOfMatches = std::move(*listOfMatches).push_back(mkString(state, match.prefix())); + idx++; // Add a list for matched substrings. - const size_t slen = match.size() - 1; - - // Start at 1, beacause the first match is the whole string. - auto list2 = state.buildList(slen); - for (const auto & [si, v2] : enumerate(list2)) { - if (!match[si + 1].matched) - v2 = &state.vNull; - else - v2 = mkString(state, match[si + 1]); - } + auto * const listOfSubMatches = state.allocList(); + + // NOTE: Start at 1, beacause the first match is the whole string. + for (const auto subMatch : std::span(match).subspan(1)) + *listOfSubMatches = std::move(*listOfSubMatches).push_back(subMatch.matched + ? mkString(state, subMatch) + : &state.vNull); - (list[idx++] = state.allocValue())->mkList(list2); + auto * const scratchValue = state.allocValue(); + scratchValue->mkList(listOfSubMatches); + *listOfMatches = std::move(*listOfMatches).push_back(scratchValue); + idx++; // Add a string for non-matched suffix characters. - if (idx == 2 * len) - list[idx++] = mkString(state, match.suffix()); + if (idx == 2 * len) { + *listOfMatches = std::move(*listOfMatches).push_back(mkString(state, match.suffix())); + idx++; + } } assert(idx == 2 * len + 1); - v.mkList(list); + v.mkList(listOfMatches); } catch (std::regex_error & e) { if (e.code() == std::regex_constants::error_space) { @@ -4467,16 +4407,28 @@ static void prim_concatStringsSep(EvalState & state, const PosIdx pos, Value * * { NixStringContext context; - auto sep = state.forceString(*args[0], context, pos, "while evaluating the first argument (the separator string) passed to builtins.concatStringsSep"); + const auto sep = state.forceString(*args[0], context, pos, "while evaluating the first argument (the separator string) passed to builtins.concatStringsSep"); state.forceList(*args[1], pos, "while evaluating the second argument (the list of strings to concat) passed to builtins.concatStringsSep"); + const auto list = args[1]->list(); + const auto numElements = list.size(); std::string res; - res.reserve((args[1]->listSize() + 32) * sep.size()); - bool first = true; - for (auto elem : args[1]->listItems()) { - if (first) first = false; else res += sep; - res += *state.coerceToString(pos, *elem, context, "while evaluating one element of the list of strings to concat passed to builtins.concatStringsSep"); + if (numElements == 0) { + v.mkString(res, context); + return; + } + + // Manually handle the first element for the singleton case. + auto * const head = list.front(); + res = *state.coerceToString(pos, *head, context, "while evaluating one element of the list of strings to concat passed to builtins.concatStringsSep"); + + if (numElements > 1) { + res.reserve((numElements + 32) * sep.size()); + immer::for_each(list.drop(1), [&](auto * const elem) { + res += sep; + res += *state.coerceToString(pos, *elem, context, "while evaluating one element of the list of strings to concat passed to builtins.concatStringsSep"); + }); } v.mkString(res, context); @@ -4504,11 +4456,12 @@ static void prim_replaceStrings(EvalState & state, const PosIdx pos, Value * * a std::vector from; from.reserve(args[0]->listSize()); - for (auto elem : args[0]->listItems()) - from.emplace_back(state.forceString(*elem, pos, "while evaluating one of the strings to replace passed to builtins.replaceStrings")); + immer::for_each(args[0]->list(), [&](auto * const elem) { + from.emplace_back(state.forceStringNoCtx(*elem, pos, "while evaluating one of the strings to replace passed to builtins.replaceStrings")); + }); std::unordered_map cache; - auto to = args[1]->listItems(); + auto to = args[1]->list(); NixStringContext context; auto s = state.forceString(*args[2], context, pos, "while evaluating the third argument passed to builtins.replaceStrings"); @@ -4633,10 +4586,13 @@ static void prim_splitVersion(EvalState & state, const PosIdx pos, Value * * arg break; components.emplace_back(component); } - auto list = state.buildList(components.size()); - for (const auto & [n, component] : enumerate(components)) - (list[n] = state.allocValue())->mkString(std::move(component)); - v.mkList(list); + + v.mkList(ranges::fold_left(components, state.allocList(), [&](auto * const list, const auto component) { + auto * const v = state.allocValue(); + v->mkString(component); + *list = std::move(*list).push_back(std::move(v)); + return list; + })); } static RegisterPrimOp primop_splitVersion({ @@ -4878,12 +4834,14 @@ void EvalState::createBaseEnv() }); /* Add a value containing the current Nix expression search path. */ - auto list = buildList(lookupPath.elements.size()); - for (const auto & [n, i] : enumerate(lookupPath.elements)) { + auto * const list = allocList(); + for (const auto & i : lookupPath.elements) { auto attrs = buildBindings(2); attrs.alloc("path").mkString(i.path.s); attrs.alloc("prefix").mkString(i.prefix.s); - (list[n] = allocValue())->mkAttrs(attrs); + auto * const v = allocValue(); + v->mkAttrs(attrs); + *list = std::move(*list).push_back(std::move(v)); } v.mkList(list); addConstant("__nixPath", v, { diff --git a/src/libexpr/primops/context.cc b/src/libexpr/primops/context.cc index 02683b173aa..c783b0c4878 100644 --- a/src/libexpr/primops/context.cc +++ b/src/libexpr/primops/context.cc @@ -218,9 +218,12 @@ static void prim_getContext(EvalState & state, const PosIdx pos, Value * * args, if (info.second.allOutputs) infoAttrs.alloc(sAllOutputs).mkBool(true); if (!info.second.outputs.empty()) { - auto list = state.buildList(info.second.outputs.size()); - for (const auto & [i, output] : enumerate(info.second.outputs)) - (list[i] = state.allocValue())->mkString(output); + auto list = state.allocList(); + for (const auto & [i, output] : enumerate(info.second.outputs)) { + auto v = state.allocValue(); + v->mkString(output); + *list = list->push_back(v); + } infoAttrs.alloc(state.sOutputs).mkList(list); } attrs.alloc(state.store->printStorePath(info.first)).mkAttrs(infoAttrs); @@ -310,7 +313,7 @@ static void prim_appendContext(EvalState & state, const PosIdx pos, Value * * ar name ).atPos(i.pos).debugThrow(); } - for (auto elem : attr->value->listItems()) { + for (auto elem : attr->value->list()) { auto outputName = state.forceStringNoCtx(*elem, attr->pos, "while evaluating an output name within a string context"); context.emplace(NixStringContextElem::Built { .drvPath = makeConstantStorePathRef(namePath), diff --git a/src/libexpr/primops/fromTOML.cc b/src/libexpr/primops/fromTOML.cc index 264046711a6..c2e353075f6 100644 --- a/src/libexpr/primops/fromTOML.cc +++ b/src/libexpr/primops/fromTOML.cc @@ -38,9 +38,12 @@ static void prim_fromTOML(EvalState & state, const PosIdx pos, Value * * args, V { auto array = toml::get>(t); - auto list = state.buildList(array.size()); - for (const auto & [n, v] : enumerate(list)) - visit(*(v = state.allocValue()), array[n]); + auto list = state.allocList(); + for (const auto & tomlValue :array) { + auto v = state.allocValue(); + visit(*v, tomlValue); + *list = list->push_back(v); + } v.mkList(list); } break;; diff --git a/src/libexpr/print-ambiguous.cc b/src/libexpr/print-ambiguous.cc index a40c98643e3..815cca17ecd 100644 --- a/src/libexpr/print-ambiguous.cc +++ b/src/libexpr/print-ambiguous.cc @@ -50,11 +50,11 @@ void printAmbiguous( break; } case nList: - if (seen && v.listSize() && !seen->insert(v.listElems()).second) + if (seen && v.listSize() && !seen->insert(&v).second) str << "«repeated»"; else { str << "[ "; - for (auto v2 : v.listItems()) { + for (auto v2 : v.list()) { if (v2) printAmbiguous(*v2, symbols, str, seen, depth - 1); else diff --git a/src/libexpr/print-options.hh b/src/libexpr/print-options.hh index 080ba26b86c..6941126c586 100644 --- a/src/libexpr/print-options.hh +++ b/src/libexpr/print-options.hh @@ -4,6 +4,7 @@ * @brief Options for printing Nix values. */ +#include #include namespace nix { diff --git a/src/libexpr/print.cc b/src/libexpr/print.cc index d62aaf25f78..55fccedb992 100644 --- a/src/libexpr/print.cc +++ b/src/libexpr/print.cc @@ -380,7 +380,7 @@ class Printer /** * @note This may force items. */ - bool shouldPrettyPrintList(std::span list) + bool shouldPrettyPrintList(const ValueList list) { if (!options.shouldPrettyPrint() || list.empty()) { return false; @@ -415,16 +415,16 @@ class Printer if (depth < options.maxDepth) { increaseIndent(); output << "["; - auto listItems = v.listItems(); - auto prettyPrint = shouldPrettyPrintList(listItems); + auto list = v.list(); + auto prettyPrint = shouldPrettyPrintList(list); size_t currentListItemsPrinted = 0; - for (auto elem : listItems) { + for (auto elem : list) { printSpace(prettyPrint); if (totalListItemsPrinted >= options.maxListItems) { - printElided(listItems.size() - currentListItemsPrinted, "item", "items"); + printElided(list.size() - currentListItemsPrinted, "item", "items"); break; } diff --git a/src/libexpr/value-to-json.cc b/src/libexpr/value-to-json.cc index 8044fe3472e..e19977f7115 100644 --- a/src/libexpr/value-to-json.cc +++ b/src/libexpr/value-to-json.cc @@ -72,7 +72,7 @@ json printValueAsJSON(EvalState & state, bool strict, case nList: { out = json::array(); int i = 0; - for (auto elem : v.listItems()) { + for (auto elem : v.list()) { try { out.push_back(printValueAsJSON(state, strict, *elem, pos, context, copyToStore)); } catch (Error & e) { diff --git a/src/libexpr/value-to-xml.cc b/src/libexpr/value-to-xml.cc index 9734ebec498..75ff433e820 100644 --- a/src/libexpr/value-to-xml.cc +++ b/src/libexpr/value-to-xml.cc @@ -114,7 +114,7 @@ static void printValueAsXML(EvalState & state, bool strict, bool location, case nList: { XMLOpenElement _(doc, "list"); - for (auto v2 : v.listItems()) + for (auto v2 : v.list()) printValueAsXML(state, strict, location, *v2, doc, context, drvsSeen, pos); break; } diff --git a/src/libexpr/value.hh b/src/libexpr/value.hh index d9816148852..c92576499f0 100644 --- a/src/libexpr/value.hh +++ b/src/libexpr/value.hh @@ -2,7 +2,6 @@ ///@file #include -#include #include "eval-gc.hh" #include "symbol-table.hh" @@ -13,11 +12,30 @@ #include +#include +#include +#include +#include +#include +#include + +// declare a memory policy for using a tracing garbage collector +using gc_policy = immer::memory_policy, + immer::no_refcount_policy, + immer::default_lock_policy, + immer::gc_transience_policy, + false, + false>; + namespace nix { struct Value; class BindingsBuilder; +// alias the vector type so we are not concerned about memory policies +// in the places where we actually use it +typedef immer::flex_vector ValueList; +typedef ValueList::transient_type ValueListTransient; typedef enum { tUninitialized = 0, @@ -27,9 +45,7 @@ typedef enum { tPath, tNull, tAttrs, - tList1, - tList2, - tListN, + tList, tThunk, tApp, tLambda, @@ -133,36 +149,6 @@ class ExternalValueBase std::ostream & operator << (std::ostream & str, const ExternalValueBase & v); -class ListBuilder -{ - const size_t size; - Value * inlineElems[2] = {nullptr, nullptr}; -public: - Value * * elems; - ListBuilder(EvalState & state, size_t size); - - // NOTE: Can be noexcept because we are just copying integral values and - // raw pointers. - ListBuilder(ListBuilder && x) noexcept - : size(x.size) - , inlineElems{x.inlineElems[0], x.inlineElems[1]} - , elems(size <= 2 ? inlineElems : x.elems) - { } - - Value * & operator [](size_t n) - { - return elems[n]; - } - - typedef Value * * iterator; - - iterator begin() { return &elems[0]; } - iterator end() { return &elems[size]; } - - friend struct Value; -}; - - struct Value { private: @@ -244,11 +230,7 @@ public: Path path; Bindings * attrs; - struct { - size_t size; - Value * const * elems; - } bigList; - Value * smallList[2]; + ValueList * list; ClosureThunk thunk; FunctionApplicationThunk app; Lambda lambda; @@ -277,7 +259,7 @@ public: case tPath: return nPath; case tNull: return nNull; case tAttrs: return nAttrs; - case tList1: case tList2: case tListN: return nList; + case tList: return nList; case tLambda: case tPrimOp: case tPrimOpApp: return nFunction; case tExternal: return nExternal; case tFloat: return nFloat; @@ -286,6 +268,7 @@ public: if (invalidIsThunk) return nThunk; else + printError("Value::type: invalid internal type: %d", internalType); unreachable(); } @@ -356,14 +339,9 @@ public: Value & mkAttrs(BindingsBuilder & bindings); - void mkList(const ListBuilder & builder) + void mkList(ValueList * list) { - if (builder.size == 1) - finishValue(tList1, { .smallList = { builder.inlineElems[0] } }); - else if (builder.size == 2) - finishValue(tList2, { .smallList = { builder.inlineElems[0], builder.inlineElems[1] } }); - else - finishValue(tListN, { .bigList = { .size = builder.size, .elems = builder.elems } }); + finishValue(tList, { .list = list }); } inline void mkThunk(Env * e, Expr * ex) @@ -407,28 +385,12 @@ public: bool isList() const { - return internalType == tList1 || internalType == tList2 || internalType == tListN; - } - - Value * const * listElems() - { - return internalType == tList1 || internalType == tList2 ? payload.smallList : payload.bigList.elems; - } - - std::span listItems() const - { - assert(isList()); - return std::span(listElems(), listSize()); - } - - Value * const * listElems() const - { - return internalType == tList1 || internalType == tList2 ? payload.smallList : payload.bigList.elems; + return internalType == tList; } size_t listSize() const { - return internalType == tList1 ? 1 : internalType == tList2 ? 2 : payload.bigList.size; + return payload.list->size(); } PosIdx determinePos(const PosIdx pos) const; @@ -471,6 +433,9 @@ public: const Bindings * attrs() const { return payload.attrs; } + ValueList list() const + { return *payload.list; } + const PrimOp * primOp() const { return payload.primOp; } diff --git a/src/libflake/flake/flake.cc b/src/libflake/flake/flake.cc index edb76f86154..7a5eff6bbc5 100644 --- a/src/libflake/flake/flake.cc +++ b/src/libflake/flake/flake.cc @@ -287,7 +287,7 @@ static Flake readFlake( Explicit { state.forceBool(*setting.value, setting.pos, "") }); else if (setting.value->type() == nList) { std::vector ss; - for (auto elem : setting.value->listItems()) { + for (auto elem : setting.value->list()) { if (elem->type() != nString) state.error("list element in flake configuration setting '%s' is %s while a string is expected", state.symbols[setting.name], showType(*setting.value)).debugThrow(); diff --git a/src/nix-env/nix-env.cc b/src/nix-env/nix-env.cc index ba2baccee85..d81bb19c51a 100644 --- a/src/nix-env/nix-env.cc +++ b/src/nix-env/nix-env.cc @@ -1251,7 +1251,7 @@ static void opQuery(Globals & globals, Strings opFlags, Strings opArgs) } else if (v->type() == nList) { attrs2["type"] = "strings"; XMLOpenElement m(xml, "meta", attrs2); - for (auto elem : v->listItems()) { + for (auto elem : v->list()) { if (elem->type() != nString) continue; XMLAttrs attrs3; attrs3["value"] = elem->c_str(); diff --git a/src/nix-env/user-env.cc b/src/nix-env/user-env.cc index ee62077c0a7..eecb604b398 100644 --- a/src/nix-env/user-env.cc +++ b/src/nix-env/user-env.cc @@ -50,7 +50,7 @@ bool createUserEnv(EvalState & state, PackageInfos & elems, /* Construct the whole top level derivation. */ StorePathSet references; - auto list = state.buildList(elems.size()); + auto list = state.allocList(); for (const auto & [n, i] : enumerate(elems)) { /* Create a pseudo-derivation containing the name, system, output paths, and optionally the derivation path, as well @@ -71,9 +71,11 @@ bool createUserEnv(EvalState & state, PackageInfos & elems, attrs.alloc(state.sDrvPath).mkString(state.store->printStorePath(*drvPath)); // Copy each output meant for installation. - auto outputsList = state.buildList(outputs.size()); - for (const auto & [m, j] : enumerate(outputs)) { - (outputsList[m] = state.allocValue())->mkString(j.first); + auto outputsList = state.allocList(); + for (const auto & j : outputs) { + auto v = state.allocValue(); + v->mkString(j.first); + *outputsList = outputsList->push_back(v); auto outputAttrs = state.buildBindings(2); outputAttrs.alloc(state.sOutPath).mkString(state.store->printStorePath(*j.second)); attrs.alloc(j.first).mkAttrs(outputAttrs); @@ -87,6 +89,7 @@ bool createUserEnv(EvalState & state, PackageInfos & elems, } attrs.alloc(state.sOutputs).mkList(outputsList); + // Copy the meta attributes. auto meta = state.buildBindings(metaNames.size()); for (auto & j : metaNames) { @@ -96,8 +99,9 @@ bool createUserEnv(EvalState & state, PackageInfos & elems, } attrs.alloc(state.sMeta).mkAttrs(meta); - - (list[n] = state.allocValue())->mkAttrs(attrs); + auto v = state.allocValue(); + v->mkAttrs(attrs); + *list = list->push_back(v); if (drvPath) references.insert(*drvPath); } diff --git a/src/nix/meson.build b/src/nix/meson.build index 60ee480358c..023b597d174 100644 --- a/src/nix/meson.build +++ b/src/nix/meson.build @@ -2,6 +2,8 @@ project('nix', 'cpp', version : files('.version'), default_options : [ 'cpp_std=c++2a', + # Set -Wno-array-bounds to avoid warnings in Immer's generated code. + 'cpp_args=-Warray-bounds=0', # TODO(Qyriad): increase the warning level 'warning_level=1', 'debug=true', diff --git a/src/nix/prefetch.cc b/src/nix/prefetch.cc index db7d9e4efe6..67aee9d4c39 100644 --- a/src/nix/prefetch.cc +++ b/src/nix/prefetch.cc @@ -44,7 +44,7 @@ std::string resolveMirrorUrl(EvalState & state, const std::string & url) if (mirrorList->value->listSize() < 1) throw Error("mirror URL '%s' did not expand to anything", url); - std::string mirror(state.forceString(*mirrorList->value->listElems()[0], noPos, "while evaluating the first available mirror")); + std::string mirror(state.forceString(*mirrorList->value->list()[0], noPos, "while evaluating the first available mirror")); return mirror + (hasSuffix(mirror, "/") ? "" : "/") + s.substr(p + 1); } @@ -222,7 +222,7 @@ static int main_nix_prefetch_url(int argc, char * * argv) state->forceList(*attr->value, noPos, "while evaluating the urls to prefetch"); if (attr->value->listSize() < 1) throw Error("'urls' list is empty"); - url = state->forceString(*attr->value->listElems()[0], noPos, "while evaluating the first url from the urls list"); + url = state->forceString(*attr->value->list()[0], noPos, "while evaluating the first url from the urls list"); /* Extract the hash mode. */ auto attr2 = v.attrs()->get(state->symbols.create("outputHashMode")); diff --git a/tests/functional/lang/eval-fail-list.err.exp b/tests/functional/lang/eval-fail-list.err.exp index d492f8bd2e4..3ad791dc4d5 100644 --- a/tests/functional/lang/eval-fail-list.err.exp +++ b/tests/functional/lang/eval-fail-list.err.exp @@ -1,5 +1,5 @@ error: - … while evaluating one of the elements to concatenate + … in the left operand of the list concatenation operator at /pwd/lang/eval-fail-list.nix:1:2: 1| 8++1 | ^