libexpr: improve error messages for builtins.genericClosure

Show which element(s) are involved at each error point: - When an element is missing the "key" attribute, show the element - When an element is not an attribute set, show the element - When comparing keys fails, show both elements being compared - When calling operator fails, show which element was being processed This provides concrete context using ValuePrinter with errorPrintOptions. Note: errorPrintOptions uses maxDepth=10 by default, which may print quite deeply nested structures in error messages. This could potentially be overwhelming, but follows the existing default for error contexts.
2025-11-08 19:46:02 +01:00 · 2025-11-06 21:30:40 +01:00 · 2025-11-06 21:30:40 +01:00 · d262efc240
commit d262efc240
parent ca787bc3e0
10 changed files with 188 additions and 46 deletions
--- a/src/libexpr-tests/error_traces.cc
+++ b/src/libexpr-tests/error_traces.cc
@ -165,35 +165,48 @@ TEST_F(ErrorTraceTest, genericClosure)
        HintFmt("expected a function but found %s: %s", "a Boolean", Uncolored(ANSI_CYAN "true" ANSI_NORMAL)),
        HintFmt("while evaluating the 'operator' attribute passed as argument to builtins.genericClosure"));

-    ASSERT_TRACE2(
+    ASSERT_TRACE3(
        "genericClosure { startSet = [{ key = 1;}]; operator = item: true; }",
        TypeError,
        HintFmt("expected a list but found %s: %s", "a Boolean", Uncolored(ANSI_CYAN "true" ANSI_NORMAL)),
-        HintFmt("while evaluating the return value of the `operator` passed to builtins.genericClosure"));
+        HintFmt("while evaluating the return value of the `operator` passed to builtins.genericClosure"),
+        HintFmt(
+            "while calling %s on genericClosure element %s",
+            "operator",
+            Uncolored("{ key = " ANSI_CYAN "1" ANSI_NORMAL "; }")));

-    ASSERT_TRACE2(
+    ASSERT_TRACE3(
        "genericClosure { startSet = [{ key = 1;}]; operator = item: [ true ]; }",
        TypeError,
        HintFmt("expected a set but found %s: %s", "a Boolean", Uncolored(ANSI_CYAN "true" ANSI_NORMAL)),
-        HintFmt("while evaluating one of the elements generated by (or initially passed to) builtins.genericClosure"));
+        HintFmt(""),
+        HintFmt("in genericClosure element %s", Uncolored(ANSI_CYAN "true" ANSI_NORMAL)));

-    ASSERT_TRACE2(
+    ASSERT_TRACE3(
        "genericClosure { startSet = [{ key = 1;}]; operator = item: [ {} ]; }",
        TypeError,
        HintFmt("attribute '%s' missing", "key"),
-        HintFmt("in one of the attrsets generated by (or initially passed to) builtins.genericClosure"));
+        HintFmt(""),
+        HintFmt("in genericClosure element %s", Uncolored("{ }")));

-    ASSERT_TRACE2(
+    ASSERT_TRACE3(
        "genericClosure { startSet = [{ key = 1;}]; operator = item: [{ key = ''a''; }]; }",
        EvalError,
-        HintFmt("cannot compare %s with %s", "a string", "an integer"),
-        HintFmt("while comparing the `key` attributes of two genericClosure elements"));
+        HintFmt(
+            "cannot compare %s with %s; values are %s and %s",
+            "a string",
+            "an integer",
+            Uncolored(ANSI_MAGENTA "\"a\"" ANSI_NORMAL),
+            Uncolored(ANSI_CYAN "1" ANSI_NORMAL)),
+        HintFmt("with element %s", Uncolored("{ key = " ANSI_CYAN "1" ANSI_NORMAL "; }")),
+        HintFmt("while comparing element %s", Uncolored("{ key = " ANSI_MAGENTA "\"a\"" ANSI_NORMAL "; }")));

-    ASSERT_TRACE2(
+    ASSERT_TRACE3(
        "genericClosure { startSet = [ true ]; operator = item: [{ key = ''a''; }]; }",
        TypeError,
        HintFmt("expected a set but found %s: %s", "a Boolean", Uncolored(ANSI_CYAN "true" ANSI_NORMAL)),
-        HintFmt("while evaluating one of the elements generated by (or initially passed to) builtins.genericClosure"));
+        HintFmt(""),
+        HintFmt("in genericClosure element %s", Uncolored(ANSI_CYAN "true" ANSI_NORMAL)));
 }

 TEST_F(ErrorTraceTest, replaceStrings)
@ -1050,17 +1063,35 @@ TEST_F(ErrorTraceTest, bitXor)

 TEST_F(ErrorTraceTest, lessThan)
 {
-    ASSERT_TRACE1("lessThan 1 \"foo\"", EvalError, HintFmt("cannot compare %s with %s", "an integer", "a string"));
+    ASSERT_TRACE1(
+        "lessThan 1 \"foo\"",
+        EvalError,
+        HintFmt(
+            "cannot compare %s with %s; values are %s and %s",
+            "an integer",
+            "a string",
+            Uncolored(ANSI_CYAN "1" ANSI_NORMAL),
+            Uncolored(ANSI_MAGENTA "\"foo\"" ANSI_NORMAL)));

    ASSERT_TRACE1(
        "lessThan {} {}",
        EvalError,
-        HintFmt("cannot compare %s with %s; values of that type are incomparable", "a set", "a set"));
+        HintFmt(
+            "cannot compare %s with %s; values of that type are incomparable (values are %s and %s)",
+            "a set",
+            "a set",
+            Uncolored("{ }"),
+            Uncolored("{ }")));

    ASSERT_TRACE2(
        "lessThan [ 1 2 ] [ \"foo\" ]",
        EvalError,
-        HintFmt("cannot compare %s with %s", "an integer", "a string"),
+        HintFmt(
+            "cannot compare %s with %s; values are %s and %s",
+            "an integer",
+            "a string",
+            Uncolored(ANSI_CYAN "1" ANSI_NORMAL),
+            Uncolored(ANSI_MAGENTA "\"foo\"" ANSI_NORMAL)),
        HintFmt("while comparing two list elements"));
 }

--- a/src/libexpr/primops.cc
+++ b/src/libexpr/primops.cc
@ -681,7 +681,14 @@ struct CompareValues
            if (v1->type() == nInt && v2->type() == nFloat)
                return v1->integer().value < v2->fpoint();
            if (v1->type() != v2->type())
-                state.error<EvalError>("cannot compare %s with %s", showType(*v1), showType(*v2)).debugThrow();
+                state
+                    .error<EvalError>(
+                        "cannot compare %s with %s; values are %s and %s",
+                        showType(*v1),
+                        showType(*v2),
+                        ValuePrinter(state, *v1, errorPrintOptions),
+                        ValuePrinter(state, *v2, errorPrintOptions))
+                    .debugThrow();
 // Allow selecting a subset of enum values
 #pragma GCC diagnostic push
 #pragma GCC diagnostic ignored "-Wswitch-enum"
@ -711,7 +718,11 @@ struct CompareValues
            default:
                state
                    .error<EvalError>(
-                        "cannot compare %s with %s; values of that type are incomparable", showType(*v1), showType(*v2))
+                        "cannot compare %s with %s; values of that type are incomparable (values are %s and %s)",
+                        showType(*v1),
+                        showType(*v2),
+                        ValuePrinter(state, *v1, errorPrintOptions),
+                        ValuePrinter(state, *v2, errorPrintOptions))
                    .debugThrow();
 #pragma GCC diagnostic pop
            }
@ -757,42 +768,79 @@ static void prim_genericClosure(EvalState & state, const PosIdx pos, Value ** ar
       `workSet', adding the result to `workSet', continuing until
       no new elements are found. */
    ValueList res;
-    // `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<Value *, decltype(cmp)> doneKeys(cmp);
+    // Track which element each key came from
+    auto cmp = CompareValues(state, noPos, "");
+    std::map<Value *, Value *, decltype(cmp)> keyToElem(cmp);
    while (!workSet.empty()) {
        Value * e = *(workSet.begin());
        workSet.pop_front();

-        state.forceAttrs(
-            *e,
-            noPos,
-            "while evaluating one of the elements generated by (or initially passed to) builtins.genericClosure");
+        try {
+            state.forceAttrs(*e, noPos, "");
+        } catch (Error & err) {
+            err.addTrace(nullptr, "in genericClosure element %s", ValuePrinter(state, *e, errorPrintOptions));
+            throw;
+        }

-        auto key = state.getAttr(
-            state.s.key,
-            e->attrs(),
-            "in one of the attrsets generated by (or initially passed to) builtins.genericClosure");
+        const Attr * key;
+        try {
+            key = state.getAttr(state.s.key, e->attrs(), "");
+        } catch (Error & err) {
+            err.addTrace(nullptr, "in genericClosure element %s", ValuePrinter(state, *e, errorPrintOptions));
+            throw;
+        }
        state.forceValue(*key->value, noPos);

-        if (!doneKeys.insert(key->value).second)
-            continue;
+        try {
+            auto [it, inserted] = keyToElem.insert({key->value, e});
+            if (!inserted)
+                continue;
+        } catch (Error & err) {
+            // Try to find which element we're comparing against
+            Value * otherElem = nullptr;
+            for (auto & [otherKey, elem] : keyToElem) {
+                try {
+                    cmp(key->value, otherKey);
+                } catch (Error &) {
+                    // Found the element we're comparing against
+                    otherElem = elem;
+                    break;
+                }
+            }
+            if (otherElem) {
+                // Traces are printed in reverse order; pre-swap them.
+                err.addTrace(nullptr, "with element %s", ValuePrinter(state, *otherElem, errorPrintOptions));
+                err.addTrace(nullptr, "while comparing element %s", ValuePrinter(state, *e, errorPrintOptions));
+            } else {
+                // Couldn't find the specific element, just show current
+                err.addTrace(nullptr, "while checking key of element %s", ValuePrinter(state, *e, errorPrintOptions));
+            }
+            throw;
+        }
        res.push_back(e);

        /* Call the `operator' function with `e' as argument. */
        Value newElements;
-        state.callFunction(*op->value, {&e, 1}, newElements, noPos);
-        state.forceList(
-            newElements,
-            noPos,
-            "while evaluating the return value of the `operator` passed to builtins.genericClosure");
+        try {
+            state.callFunction(*op->value, {&e, 1}, newElements, noPos);
+            state.forceList(
+                newElements,
+                noPos,
+                "while evaluating the return value of the `operator` passed to builtins.genericClosure");

-        /* Add the values returned by the operator to the work set. */
-        for (auto elem : newElements.listView()) {
-            state.forceValue(*elem, noPos); // "while evaluating one one of the elements returned by the `operator`
-                                            // passed to builtins.genericClosure");
-            workSet.push_back(elem);
+            /* Add the values returned by the operator to the work set. */
+            for (auto elem : newElements.listView()) {
+                state.forceValue(*elem, noPos); // "while evaluating one one of the elements returned by the `operator`
+                                                // passed to builtins.genericClosure");
+                workSet.push_back(elem);
+            }
+        } catch (Error & err) {
+            err.addTrace(
+                nullptr,
+                "while calling %s on genericClosure element %s",
+                state.symbols[state.s.operator_],
+                ValuePrinter(state, *e, errorPrintOptions));
+            throw;
        }
    }

--- a/tests/functional/lang/eval-fail-genericClosure-deeply-nested-element.err.exp
+++ b/tests/functional/lang/eval-fail-genericClosure-deeply-nested-element.err.exp
@ -0,0 +1,18 @@
+error:
+       … while calling the 'seq' builtin
+         at /pwd/lang/eval-fail-genericClosure-deeply-nested-element.nix:25:1:
+           24| in
+           25| builtins.seq finiteVal (
+             | ^
+           26|   builtins.genericClosure {
+
+       … while calling the 'genericClosure' builtin
+         at /pwd/lang/eval-fail-genericClosure-deeply-nested-element.nix:26:3:
+           25| builtins.seq finiteVal (
+           26|   builtins.genericClosure {
+             |   ^
+           27|     startSet = [
+
+       … in genericClosure element { finite = { a0 = { a1 = { a2 = { a3 = { a4 = { a5 = { a6 = { a7 = { a8 = { ... }; }; }; }; }; }; }; }; }; }; «1 attribute elided» }
+
+       error: attribute 'key' missing
--- a/tests/functional/lang/eval-fail-genericClosure-deeply-nested-element.nix
+++ b/tests/functional/lang/eval-fail-genericClosure-deeply-nested-element.nix
@ -0,0 +1,35 @@
+let
+  finite = {
+    a0 = {
+      a1 = {
+        a2 = {
+          a3 = {
+            a4 = {
+              a5 = {
+                a6 = {
+                  a7 = {
+                    a8 = {
+                      a9 = "deep";
+                    };
+                  };
+                };
+              };
+            };
+          };
+        };
+      };
+    };
+  };
+  finiteVal = builtins.deepSeq finite finite;
+in
+builtins.seq finiteVal (
+  builtins.genericClosure {
+    startSet = [
+      {
+        infinite = import ./infinite-nesting.nix;
+        finite = finiteVal;
+      }
+    ];
+    operator = x: [ (import ./infinite-nesting.nix) ];
+  }
+)
--- a/tests/functional/lang/eval-fail-genericClosure-element-missing-key.err.exp
+++ b/tests/functional/lang/eval-fail-genericClosure-element-missing-key.err.exp
@ -5,6 +5,6 @@ error:
             | ^
            2|   startSet = [ { nokey = 1; } ];

-       … in one of the attrsets generated by (or initially passed to) builtins.genericClosure
+       … in genericClosure element { nokey = 1; }

       error: attribute 'key' missing
--- a/tests/functional/lang/eval-fail-genericClosure-element-not-attrset.err.exp
+++ b/tests/functional/lang/eval-fail-genericClosure-element-not-attrset.err.exp
@ -5,6 +5,6 @@ error:
             | ^
            2|   startSet = [ "not an attrset" ];

-       … while evaluating one of the elements generated by (or initially passed to) builtins.genericClosure
+       … in genericClosure element "not an attrset"

       error: expected a set but found a string: "not an attrset"
--- a/tests/functional/lang/eval-fail-genericClosure-keys-incompatible-types.err.exp
+++ b/tests/functional/lang/eval-fail-genericClosure-keys-incompatible-types.err.exp
@ -5,6 +5,8 @@ error:
             | ^
            2|   startSet = [

-       … while comparing the `key` attributes of two genericClosure elements
+       … while comparing element { key = "string"; }

-       error: cannot compare a string with an integer
+       … with element { key = 1; }
+
+       error: cannot compare a string with an integer; values are "string" and 1
--- a/tests/functional/lang/eval-fail-genericClosure-keys-uncomparable.err.exp
+++ b/tests/functional/lang/eval-fail-genericClosure-keys-uncomparable.err.exp
@ -5,6 +5,8 @@ error:
             | ^
            2|   startSet = [

-       … while comparing the `key` attributes of two genericClosure elements
+       … while comparing element { key = { }; }

-       error: cannot compare a set with a set; values of that type are incomparable
+       … with element { key = { }; }
+
+       error: cannot compare a set with a set; values of that type are incomparable (values are { } and { })
--- a/tests/functional/lang/eval-fail-genericClosure-operator-not-list.err.exp
+++ b/tests/functional/lang/eval-fail-genericClosure-operator-not-list.err.exp
@ -5,6 +5,8 @@ error:
             | ^
            2|   startSet = [ { key = 1; } ];

+       … while calling operator on genericClosure element { key = 1; }
+
       … while evaluating the return value of the `operator` passed to builtins.genericClosure

       error: expected a list but found a string: "not a list"
--- a/tests/functional/lang/infinite-nesting.nix
+++ b/tests/functional/lang/infinite-nesting.nix
@ -0,0 +1,4 @@
+let
+  mkInfinite = i: { "a${toString i}" = mkInfinite (i + 1); };
+in
+mkInfinite 0