From 963b8aa39b169bf5c054449ddce39d60faacf298 Mon Sep 17 00:00:00 2001
From: Guillaume Maudoux <guillaume.maudoux@tweag.io>
Date: Fri, 18 Mar 2022 23:17:50 +0100
Subject: [PATCH] Explain current error trace impl

---
 src/libutil/error.cc | 95 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 94 insertions(+), 1 deletion(-)

diff --git a/src/libutil/error.cc b/src/libutil/error.cc
index ae0bbc06f..c66dfa112 100644
--- a/src/libutil/error.cc
+++ b/src/libutil/error.cc
@@ -288,7 +288,100 @@ std::ostream & showErrorInfo(std::ostream & out, const ErrorInfo & einfo, bool s
             "?" << std::endl;
     }
 
-    // traces
+    /*
+     * Traces
+     * ------
+     *
+     *  The semantics of traces is a bit weird. We have only one option to
+     *  print them and to make them verbose (--show-trace). In the code they
+     *  are always collected, but they are not printed by default. The code
+     *  also collects more traces when the option is on. This means that there
+     *  is no way to print the simplified traces at all.
+     *
+     *  I (layus) designed the code to attach positions to a restricted set of
+     *  messages. This means that we have  a lot of traces with no position at
+     *  all, including most of the base error messages. For example "type
+     *  error: found a string while a set was expected" has no position, but
+     *  will come with several traces detailing it's precise relation to the
+     *  closest know position. This makes erroring without printing traces
+     *  quite useless.
+     *
+     *  This is why I introduced the idea to always print a few traces on
+     *  error. The number 3 is quite arbitrary, and was selected so as not to
+     *  clutter the console on error. For the same reason, a trace with an
+     *  error position takes more space, and counts as two traces towards the
+     *  limit.
+     *
+     *  The rest is truncated, unless --show-trace is passed. This preserves
+     *  the same bad semantics of --show-trace to both show the trace and
+     *  augment it with new data. Not too sure what is the best course of
+     *  action.
+     *
+     *  The issue is that it is fundamentally hard to provide a trace for a
+     *  lazy language. The trace will only cover the current spine of the
+     *  evaluation, missing things that have been evaluated before. For
+     *  example, most type errors are hard to inspect because there is not
+     *  trace for the faulty value. These errors should really print the faulty
+     *  value itself.
+     *
+     *  In function calls, the --show-trace flag triggers extra traces for each
+     *  function invocation. These work as scopes, allowing to follow the
+     *  current spine of the evaluation graph. Without that flag, the error
+     *  trace should restrict itself to a restricted prefix of that trace,
+     *  until the first scope. If we ever get to such a precise error
+     *  reporting, there would be no need to add an arbitrary limit here. We
+     *  could always print the full trace, and it would just be small without
+     *  the flag.
+     *
+     *  One idea I had is for XxxError.addTrace() to perform nothing if one
+     *  scope has already been traced. Alternatively, we could stop here when
+     *  we encounter such a scope instead of after an arbitrary number of
+     *  traces. This however requires to augment traces with the notion of
+     *  "scope".
+     *
+     *  This is particularly visible in code like evalAttrs(...) where we have
+     *  to make a decision between the two following options.
+     *
+     *  ``` long traces
+     *  inline void EvalState::evalAttrs(Env & env, Expr * e, Value & v, const Pos & pos, const std::string_view & errorCtx)
+     *  {
+     *      try {
+     *          e->eval(*this, env, v);
+     *          if (v.type() != nAttrs)
+     *              throwTypeError("value is %1% while a set was expected", v);
+     *      } catch (Error & e) {
+     *          e.addTrace(pos, errorCtx);
+     *          throw;
+     *      }
+     *  }
+     *  ```
+     *
+     *  ``` short traces
+     *  inline void EvalState::evalAttrs(Env & env, Expr * e, Value & v, const Pos & pos, const std::string_view & errorCtx)
+     *  {
+     *      e->eval(*this, env, v);
+     *      try {
+     *          if (v.type() != nAttrs)
+     *              throwTypeError("value is %1% while a set was expected", v);
+     *      } catch (Error & e) {
+     *          e.addTrace(pos, errorCtx);
+     *          throw;
+     *      }
+     *  }
+     *  ```
+     *
+     *  The second example can be rewritten more concisely, but kept in this
+     *  form to highlight the symmetry. The first option adds more information,
+     *  because whatever caused an error down the line, in the generic eval
+     *  function, will get annotated with the code location that uses and
+     *  required it. The second option is less verbose, but does not provide
+     *  any context at all as to where and why a failing value was required.
+     *
+     *  Scopes would fix that, by adding context only when --show-trace is
+     *  passed, and keeping the trace terse otherwise.
+     *
+     */
+
     if (!einfo.traces.empty()) {
         unsigned int count = 0;
         for (auto iter = einfo.traces.rbegin(); iter != einfo.traces.rend(); ++iter) {