From eb0280f7154ff3daeccd41e0ba6bb5ffd5b5a674 Mon Sep 17 00:00:00 2001
From: eldritch horrors <pennae@lix.systems>
Date: Sat, 26 Apr 2025 22:14:05 +0200
Subject: [PATCH] libexpr: add set-to-string error frame

this is much more useful than a frame pointing to the set but claiming
it as a function being called. if the function is actually at fault we
will now point to its attribute, although the position may be slightly
wrong if __toString was defined from set updates or builtin functions.

Change-Id: Ib3eb237a276d94426d9c6fc0e26eea72382d34a2
---
 doc/manual/rl-next/tostring-frames.md         | 60 +++++++++++++++++++
 lix/libexpr/eval.cc                           | 13 ++--
 .../lang/eval-fail-toString.err.exp           | 14 +++++
 tests/functional/lang/eval-fail-toString.nix  |  1 +
 4 files changed, 84 insertions(+), 4 deletions(-)
 create mode 100644 doc/manual/rl-next/tostring-frames.md
 create mode 100644 tests/functional/lang/eval-fail-toString.err.exp
 create mode 100644 tests/functional/lang/eval-fail-toString.nix

diff --git a/doc/manual/rl-next/tostring-frames.md b/doc/manual/rl-next/tostring-frames.md
new file mode 100644
index 000000000..3efda9385
--- /dev/null
+++ b/doc/manual/rl-next/tostring-frames.md
@@ -0,0 +1,60 @@
+---
+synopsis: 'Implicit `__toString` now have stack trace entries'
+issues: []
+cls: [3055]
+category: Improvements
+credits: [horrors]
+---
+
+Coercion of attribute sets to strings via their `__toString` attribute now produce stack
+frames pointing to the coercion site and the attribute definition. This makes locating a
+coercion function error easier as the fault location is now more likely to be presented.
+
+Previously:
+```
+nix-repl> builtins.substring 1 1 "${{ __toString = self: throw ''bar''; }}"
+error:
+       … while calling the 'substring' builtin
+         at «string»:1:1:
+            1| builtins.substring 1 1 "${{ __toString = self: throw ''bar''; }}"
+             | ^
+
+       … caused by explicit throw
+         at «string»:1:48:
+            1| builtins.substring 1 1 "${{ __toString = self: throw ''bar''; }}"
+             |                                                ^
+
+       error: bar
+```
+
+Now:
+```
+nix-repl> builtins.substring 1 1 "${{ __toString = self: throw ''bar''; }}"
+error:
+       … while calling the 'substring' builtin
+         at «string»:1:1:
+            1| builtins.substring 1 1 "${{ __toString = self: throw ''bar''; }}"
+             | ^
+
+       … while converting a set to string
+         at «string»:1:25:
+            1| builtins.substring 1 1 "${{ __toString = self: throw ''bar''; }}"
+             |                         ^
+
+       … from call site
+         at «string»:1:29:
+            1| builtins.substring 1 1 "${{ __toString = self: throw ''bar''; }}"
+             |                             ^
+
+       … while calling '__toString'
+         at «string»:1:42:
+            1| builtins.substring 1 1 "${{ __toString = self: throw ''bar''; }}"
+             |                                          ^
+
+       … caused by explicit throw
+         at «string»:1:48:
+            1| builtins.substring 1 1 "${{ __toString = self: throw ''bar''; }}"
+             |                                                ^
+
+       error: bar
+```
diff --git a/lix/libexpr/eval.cc b/lix/libexpr/eval.cc
index 0769eef20..8cee40a25 100644
--- a/lix/libexpr/eval.cc
+++ b/lix/libexpr/eval.cc
@@ -2309,10 +2309,15 @@ std::optional<std::string> EvalState::tryAttrsToString(const PosIdx pos, Value &
     auto i = v.attrs->find(ctx.s.toString);
     if (i != v.attrs->end()) {
         Value v1;
-        callFunction(*i->value, v, v1, pos);
-        return coerceToString(pos, v1, context,
-                "while evaluating the result of the `__toString` attribute",
-                coerceMore, copyToStore).toOwned();
+        try {
+            callFunction(*i->value, v, v1, i->pos);
+            return coerceToString(pos, v1, context,
+                    "while evaluating the result of the `__toString` attribute",
+                    coerceMore, copyToStore).toOwned();
+        } catch (EvalError & e) {
+            e.addTrace(ctx.positions[pos], "while converting a set to string");
+            throw;
+        }
     }
 
     return {};
diff --git a/tests/functional/lang/eval-fail-toString.err.exp b/tests/functional/lang/eval-fail-toString.err.exp
new file mode 100644
index 000000000..1b9fef4b1
--- /dev/null
+++ b/tests/functional/lang/eval-fail-toString.err.exp
@@ -0,0 +1,14 @@
+error:
+       … while calling the 'toString' builtin
+         at /pwd/lang/eval-fail-toString.nix:1:1:
+            1| toString { __toString = 1; }
+             | ^
+            2|
+
+       … while converting a set to string
+
+       error: attempt to call something which is not a function but an integer: 1
+       at /pwd/lang/eval-fail-toString.nix:1:12:
+            1| toString { __toString = 1; }
+             |            ^
+            2|
diff --git a/tests/functional/lang/eval-fail-toString.nix b/tests/functional/lang/eval-fail-toString.nix
new file mode 100644
index 000000000..485d5eab5
--- /dev/null
+++ b/tests/functional/lang/eval-fail-toString.nix
@@ -0,0 +1 @@
+toString { __toString = 1; }