builtins.genericClosure: improve errors

src/libexpr-tests/error_traces.cc

@@ -139,63 +139,6 @@ TEST_F(ErrorTraceTest, NestedThrows)
 #define ASSERT_DERIVATION_TRACE3(args, type, message, context1, context2) \
     ASSERT_TRACE4(args, type, message, context1, context2, DERIVATION_TRACE_HINTFMT("foo"))
 
-TEST_F(ErrorTraceTest, genericClosure)
-{
-    ASSERT_TRACE2(
-        "genericClosure 1",
-        TypeError,
-        HintFmt("expected a set but found %s: %s", "an integer", Uncolored(ANSI_CYAN "1" ANSI_NORMAL)),
-        HintFmt("while evaluating the first argument passed to builtins.genericClosure"));
-
-    ASSERT_TRACE2(
-        "genericClosure {}",
-        TypeError,
-        HintFmt("attribute '%s' missing", "startSet"),
-        HintFmt("in the attrset passed as argument to builtins.genericClosure"));
-
-    ASSERT_TRACE2(
-        "genericClosure { startSet = 1; }",
-        TypeError,
-        HintFmt("expected a list but found %s: %s", "an integer", Uncolored(ANSI_CYAN "1" ANSI_NORMAL)),
-        HintFmt("while evaluating the 'startSet' attribute passed as argument to builtins.genericClosure"));
-
-    ASSERT_TRACE2(
-        "genericClosure { startSet = [{ key = 1;}]; operator = true; }",
-        TypeError,
-        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(
-        "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"));
-
-    ASSERT_TRACE2(
-        "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"));
-
-    ASSERT_TRACE2(
-        "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"));
-
-    ASSERT_TRACE2(
-        "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"));
-
-    ASSERT_TRACE2(
-        "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"));
-}
-
 TEST_F(ErrorTraceTest, replaceStrings)
 {
     ASSERT_TRACE2(
@@ -1050,17 +993,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"));
 }
 

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");
-
-        auto key = state.getAttr(
-            state.s.key,
-            e->attrs(),
-            "in one of the attrsets 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;
+        }
+
+        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");
-
-        /* 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);
+        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);
+            }
+        } catch (Error & err) {
+            err.addTrace(
+                nullptr,
+                "while calling %s on genericClosure element %s",
+                state.symbols[state.s.operator_],
+                ValuePrinter(state, *e, errorPrintOptions));
+            throw;
         }
     }
 

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

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) ];
+  }
+)

tests/functional/lang/eval-fail-genericClosure-element-missing-key.err.exp

@@ -0,0 +1,10 @@
+error:
+       … while calling the 'genericClosure' builtin
+         at /pwd/lang/eval-fail-genericClosure-element-missing-key.nix:1:1:
+            1| builtins.genericClosure {
+             | ^
+            2|   startSet = [ { nokey = 1; } ];
+
+       … in genericClosure element { nokey = 1; }
+
+       error: attribute 'key' missing

tests/functional/lang/eval-fail-genericClosure-element-missing-key.nix

@@ -0,0 +1,4 @@
+builtins.genericClosure {
+  startSet = [ { nokey = 1; } ];
+  operator = x: [ ];
+}

tests/functional/lang/eval-fail-genericClosure-element-not-attrset.err.exp

@@ -0,0 +1,10 @@
+error:
+       … while calling the 'genericClosure' builtin
+         at /pwd/lang/eval-fail-genericClosure-element-not-attrset.nix:1:1:
+            1| builtins.genericClosure {
+             | ^
+            2|   startSet = [ "not an attrset" ];
+
+       … in genericClosure element "not an attrset"
+
+       error: expected a set but found a string: "not an attrset"

tests/functional/lang/eval-fail-genericClosure-element-not-attrset.nix

@@ -0,0 +1,4 @@
+builtins.genericClosure {
+  startSet = [ "not an attrset" ];
+  operator = x: [ ];
+}

tests/functional/lang/eval-fail-genericClosure-keys-incompatible-types.err.exp

@@ -0,0 +1,12 @@
+error:
+       … while calling the 'genericClosure' builtin
+         at /pwd/lang/eval-fail-genericClosure-keys-incompatible-types.nix:1:1:
+            1| builtins.genericClosure {
+             | ^
+            2|   startSet = [
+
+       … while comparing element { key = "string"; }
+
+       … with element { key = 1; }
+
+       error: cannot compare a string with an integer; values are "string" and 1

tests/functional/lang/eval-fail-genericClosure-keys-incompatible-types.nix

@@ -0,0 +1,7 @@
+builtins.genericClosure {
+  startSet = [
+    { key = 1; }
+    { key = "string"; }
+  ];
+  operator = x: [ ];
+}

tests/functional/lang/eval-fail-genericClosure-keys-uncomparable.err.exp

@@ -0,0 +1,12 @@
+error:
+       … while calling the 'genericClosure' builtin
+         at /pwd/lang/eval-fail-genericClosure-keys-uncomparable.nix:1:1:
+            1| builtins.genericClosure {
+             | ^
+            2|   startSet = [
+
+       … while comparing element { key = { }; }
+
+       … with element { key = { }; }
+
+       error: cannot compare a set with a set; values of that type are incomparable (values are { } and { })

tests/functional/lang/eval-fail-genericClosure-keys-uncomparable.nix

@@ -0,0 +1,7 @@
+builtins.genericClosure {
+  startSet = [
+    { key = { }; }
+    { key = { }; }
+  ];
+  operator = x: [ ];
+}

tests/functional/lang/eval-fail-genericClosure-missing-operator.err.exp

@@ -0,0 +1,10 @@
+error:
+       … while calling the 'genericClosure' builtin
+         at /pwd/lang/eval-fail-genericClosure-missing-operator.nix:1:1:
+            1| builtins.genericClosure {
+             | ^
+            2|   startSet = [ { key = 1; } ];
+
+       … in the attrset passed as argument to builtins.genericClosure
+
+       error: attribute 'operator' missing

tests/functional/lang/eval-fail-genericClosure-missing-operator.nix

@@ -0,0 +1,3 @@
+builtins.genericClosure {
+  startSet = [ { key = 1; } ];
+}

tests/functional/lang/eval-fail-genericClosure-missing-startSet.err.exp

@@ -0,0 +1,10 @@
+error:
+       … while calling the 'genericClosure' builtin
+         at /pwd/lang/eval-fail-genericClosure-missing-startSet.nix:1:1:
+            1| builtins.genericClosure {
+             | ^
+            2|   operator = x: [ ];
+
+       … in the attrset passed as argument to builtins.genericClosure
+
+       error: attribute 'startSet' missing

tests/functional/lang/eval-fail-genericClosure-missing-startSet.nix

@@ -0,0 +1,3 @@
+builtins.genericClosure {
+  operator = x: [ ];
+}