Cleanup fmt.hh

When I started contributing to Nix, I found the mix of definitions and
names in `fmt.hh` to be rather confusing, especially the small
difference between `hintfmt` and `hintformat`. I've renamed many classes
and added documentation to most definitions.

- `formatHelper` is no longer exported.
- `fmt`'s documentation is now with `fmt` rather than (misleadingly)
  above `formatHelper`.
- `yellowtxt` is renamed to `Magenta`.

  `yellowtxt` wraps its value with `ANSI_WARNING`, but `ANSI_WARNING`
  has been equal to `ANSI_MAGENTA` for a long time. Now the name is
  updated.
- `normaltxt` is renamed to `Uncolored`.
- `hintfmt` has been merged into `hintformat` as extra constructor
  functions.
- `hintformat` has been renamed to `hintfmt`.
- The single-argument `hintformat(std::string)` constructor has been
  renamed to a static member `hintformat::interpolate` to avoid pitfalls
  with using user-generated strings as format strings.

src/build-remote/build-remote.cc

@@ -202,7 +202,7 @@ static int main_build_remote(int argc, char * * argv)
                         else
                             drvstr = "<unknown>";
 
-                        auto error = hintformat(errorText);
+                        auto error = hintfmt(errorText);
                         error
                             % drvstr
                             % neededSystem

src/libexpr/eval.hh

@@ -148,7 +148,7 @@ struct DebugTrace {
     std::shared_ptr<Pos> pos;
     const Expr & expr;
     const Env & env;
-    hintformat hint;
+    hintfmt hint;
     bool isError;
 };
 

src/libexpr/value/context.hh

@@ -20,7 +20,7 @@ public:
     {
         raw = raw_;
         auto hf = hintfmt(args...);
-        err.msg = hintfmt("Bad String Context element: %1%: %2%", normaltxt(hf.str()), raw);
+        err.msg = hintfmt("Bad String Context element: %1%: %2%", Uncolored(hf.str()), raw);
     }
 };
 

src/libstore/build/derivation-goal.cc

@@ -708,7 +708,7 @@ void DerivationGoal::tryToBuild()
     if (!outputLocks.lockPaths(lockFiles, "", false)) {
         if (!actLock)
             actLock = std::make_unique<Activity>(*logger, lvlWarn, actBuildWaiting,
-                fmt("waiting for lock on %s", magentatxt(showPaths(lockFiles))));
+                fmt("waiting for lock on %s", Magenta(showPaths(lockFiles))));
         worker.waitForAWhile(shared_from_this());
         return;
     }
@@ -762,7 +762,7 @@ void DerivationGoal::tryToBuild()
                    the wake-up timeout expires. */
                 if (!actLock)
                     actLock = std::make_unique<Activity>(*logger, lvlWarn, actBuildWaiting,
-                        fmt("waiting for a machine to build '%s'", magentatxt(worker.store.printStorePath(drvPath))));
+                        fmt("waiting for a machine to build '%s'", Magenta(worker.store.printStorePath(drvPath))));
                 worker.waitForAWhile(shared_from_this());
                 outputLocks.unlock();
                 return;
@@ -987,7 +987,7 @@ void DerivationGoal::buildDone()
             diskFull |= cleanupDecideWhetherDiskFull();
 
             auto msg = fmt("builder for '%s' %s",
-                magentatxt(worker.store.printStorePath(drvPath)),
+                Magenta(worker.store.printStorePath(drvPath)),
                 statusToString(status));
 
             if (!logger->isVerbose() && !logTail.empty()) {
@@ -1523,7 +1523,7 @@ void DerivationGoal::done(
     outputLocks.unlock();
     buildResult.status = status;
     if (ex)
-        buildResult.errorMsg = fmt("%s", normaltxt(ex->info().msg));
+        buildResult.errorMsg = fmt("%s", Uncolored(ex->info().msg));
     if (buildResult.status == BuildResult::TimedOut)
         worker.timedOut = true;
     if (buildResult.status == BuildResult::PermanentFailure)

src/libstore/build/local-derivation-goal.cc

@@ -232,7 +232,7 @@ void LocalDerivationGoal::tryLocalBuild()
         if (!buildUser) {
             if (!actLock)
                 actLock = std::make_unique<Activity>(*logger, lvlWarn, actBuildWaiting,
-                    fmt("waiting for a free build user ID for '%s'", magentatxt(worker.store.printStorePath(drvPath))));
+                    fmt("waiting for a free build user ID for '%s'", Magenta(worker.store.printStorePath(drvPath))));
             worker.waitForAWhile(shared_from_this());
             return;
         }

src/libstore/filetransfer.cc

@@ -887,7 +887,7 @@ FileTransferError::FileTransferError(FileTransfer::Error error, std::optional<st
     // to print different messages for different verbosity levels. For now
     // we add some heuristics for detecting when we want to show the response.
     if (response && (response->size() < 1024 || response->find("<html>") != std::string::npos))
-        err.msg = hintfmt("%1%\n\nresponse body:\n\n%2%", normaltxt(hf.str()), chomp(*response));
+        err.msg = hintfmt("%1%\n\nresponse body:\n\n%2%", Uncolored(hf.str()), chomp(*response));
     else
         err.msg = hf;
 }

src/libstore/sqlite.cc

@@ -10,19 +10,19 @@
 
 namespace nix {
 
-SQLiteError::SQLiteError(const char *path, const char *errMsg, int errNo, int extendedErrNo, int offset, hintformat && hf)
+SQLiteError::SQLiteError(const char *path, const char *errMsg, int errNo, int extendedErrNo, int offset, hintfmt && hf)
   : Error(""), path(path), errMsg(errMsg), errNo(errNo), extendedErrNo(extendedErrNo), offset(offset)
 {
     auto offsetStr = (offset == -1) ? "" : "at offset " + std::to_string(offset) + ": ";
     err.msg = hintfmt("%s: %s%s, %s (in '%s')",
-        normaltxt(hf.str()),
+        Uncolored(hf.str()),
         offsetStr,
         sqlite3_errstr(extendedErrNo),
         errMsg,
         path ? path : "(in-memory)");
 }
 
-[[noreturn]] void SQLiteError::throw_(sqlite3 * db, hintformat && hf)
+[[noreturn]] void SQLiteError::throw_(sqlite3 * db, hintfmt && hf)
 {
     int err = sqlite3_errcode(db);
     int exterr = sqlite3_extended_errcode(db);

src/libstore/sqlite.hh

@@ -145,16 +145,16 @@ struct SQLiteError : Error
         throw_(db, hintfmt(fs, args...));
     }
 
-    SQLiteError(const char *path, const char *errMsg, int errNo, int extendedErrNo, int offset, hintformat && hf);
+    SQLiteError(const char *path, const char *errMsg, int errNo, int extendedErrNo, int offset, hintfmt && hf);
 
 protected:
 
     template<typename... Args>
     SQLiteError(const char *path, const char *errMsg, int errNo, int extendedErrNo, int offset, const std::string & fs, const Args & ... args)
-      : SQLiteError(path, errNo, extendedErrNo, offset, hintfmt(fs, args...))
+      : SQLiteError(path, errMsg, errNo, extendedErrNo, offset, hintfmt(fs, args...))
     { }
 
-    [[noreturn]] static void throw_(sqlite3 * db, hintformat && hf);
+    [[noreturn]] static void throw_(sqlite3 * db, hintfmt && hf);
 
 };
 

src/libutil/error.cc

@@ -11,7 +11,7 @@
 
 namespace nix {
 
-void BaseError::addTrace(std::shared_ptr<Pos> && e, hintformat hint, bool frame)
+void BaseError::addTrace(std::shared_ptr<Pos> && e, hintfmt hint, bool frame)
 {
     err.traces.push_front(Trace { .pos = std::move(e), .hint = hint, .frame = frame });
 }
@@ -37,7 +37,7 @@ const std::string & BaseError::calcWhat() const
 
 std::optional<std::string> ErrorInfo::programName = std::nullopt;
 
-std::ostream & operator <<(std::ostream & os, const hintformat & hf)
+std::ostream & operator <<(std::ostream & os, const hintfmt & hf)
 {
     return os << hf.str();
 }

src/libutil/error.hh

@@ -63,7 +63,7 @@ void printCodeLines(std::ostream & out,
 
 struct Trace {
     std::shared_ptr<Pos> pos;
-    hintformat hint;
+    hintfmt hint;
     bool frame;
 };
 
@@ -74,7 +74,7 @@ inline bool operator>=(const Trace& lhs, const Trace& rhs);
 
 struct ErrorInfo {
     Verbosity level;
-    hintformat msg;
+    hintfmt msg;
     std::shared_ptr<Pos> pos;
     std::list<Trace> traces;
 
@@ -126,7 +126,7 @@ public:
         : err { .level = lvlError, .msg = hintfmt(args...), .suggestions = sug }
     { }
 
-    BaseError(hintformat hint)
+    BaseError(hintfmt hint)
         : err { .level = lvlError, .msg = hint }
     { }
 
@@ -162,7 +162,7 @@ public:
         addTrace(std::move(e), hintfmt(std::string(fs), args...));
     }
 
-    void addTrace(std::shared_ptr<Pos> && e, hintformat hint, bool frame = false);
+    void addTrace(std::shared_ptr<Pos> && e, hintfmt hint, bool frame = false);
 
     bool hasTrace() const { return !err.traces.empty(); }
 
@@ -215,7 +215,7 @@ public:
         : SystemError(""), errNo(errNo)
     {
         auto hf = hintfmt(args...);
-        err.msg = hintfmt("%1%: %2%", normaltxt(hf.str()), strerror(errNo));
+        err.msg = hintfmt("%1%: %2%", Uncolored(hf.str()), strerror(errNo));
     }
 
     /**

src/libutil/fmt.hh

@@ -8,37 +8,53 @@
 
 namespace nix {
 
-
+namespace {
 /**
- * Inherit some names from other namespaces for convenience.
- */
-using boost::format;
-
-
-/**
- * A variadic template that does nothing. Useful to call a function
- * for all variadic arguments but ignoring the result.
- */
-struct nop { template<typename... T> nop(T...) {} };
-
-
-/**
- * A helper for formatting strings. ‘fmt(format, a_0, ..., a_n)’ is
- * equivalent to ‘boost::format(format) % a_0 % ... %
- * ... a_n’. However, ‘fmt(s)’ is equivalent to ‘s’ (so no %-expansion
- * takes place).
+ * A helper for writing `boost::format` expressions.
+ *
+ * These are equivalent:
+ *
+ * ```
+ * formatHelper(formatter, a_0, ..., a_n)
+ * formatter % a_0 % ... % a_n
+ * ```
+ *
+ * With a single argument, `formatHelper(s)` is a no-op.
  */
 template<class F>
 inline void formatHelper(F & f)
-{
-}
+{ }
 
 template<class F, typename T, typename... Args>
 inline void formatHelper(F & f, const T & x, const Args & ... args)
 {
+    // Interpolate one argument and then recurse.
     formatHelper(f % x, args...);
 }
+}
 
+/**
+ * A helper for writing a `boost::format` expression to a string.
+ *
+ * These are (roughly) equivalent:
+ *
+ * ```
+ * fmt(formatString, a_0, ..., a_n)
+ * (boost::format(formatString) % a_0 % ... % a_n).str()
+ * ```
+ *
+ * However, when called with a single argument, the string is returned
+ * unchanged.
+ *
+ * If you write code like this:
+ *
+ * ```
+ * std::cout << boost::format(stringFromUserInput) << std::endl;
+ * ```
+ *
+ * And `stringFromUserInput` contains formatting placeholders like `%s`, then
+ * the code will crash at runtime. `fmt` helps you avoid this pitfall.
+ */
 inline std::string fmt(const std::string & s)
 {
     return s;
@@ -63,61 +79,107 @@ inline std::string fmt(const std::string & fs, const Args & ... args)
     return f.str();
 }
 
-// format function for hints in errors.  same as fmt, except templated values
-// are always in magenta.
+/**
+ * Values wrapped in this struct are printed in magenta.
+ *
+ * By default, arguments to `hintfmt` are printed in magenta. To avoid this,
+ * either wrap the argument in `Uncolored` or add a specialization of
+ * `hintfmt::operator%`.
+ */
 template <class T>
-struct magentatxt
+struct Magenta
 {
-    magentatxt(const T &s) : value(s) {}
+    Magenta(const T &s) : value(s) {}
     const T & value;
 };
 
 template <class T>
-std::ostream & operator<<(std::ostream & out, const magentatxt<T> & y)
+std::ostream & operator<<(std::ostream & out, const Magenta<T> & y)
 {
     return out << ANSI_WARNING << y.value << ANSI_NORMAL;
 }
 
+/**
+ * Values wrapped in this class are printed without coloring.
+ *
+ * By default, arguments to `hintfmt` are printed in magenta (see `Magenta`).
+ */
 template <class T>
-struct normaltxt
+struct Uncolored
 {
-    normaltxt(const T & s) : value(s) {}
+    Uncolored(const T & s) : value(s) {}
     const T & value;
 };
 
 template <class T>
-std::ostream & operator<<(std::ostream & out, const normaltxt<T> & y)
+std::ostream & operator<<(std::ostream & out, const Uncolored<T> & y)
 {
     return out << ANSI_NORMAL << y.value;
 }
 
-class hintformat
+/**
+ * A wrapper around `boost::format` which colors interpolated arguments in
+ * magenta by default.
+ */
+class hintfmt
 {
+private:
+    boost::format fmt;
+
 public:
-    hintformat(const std::string & format) : fmt(format)
+    /**
+     * Construct a `hintfmt` from a format string, with values to be
+     * interpolated later with `%`.
+     *
+     * This isn't exposed as a single-argument constructor to avoid
+     * accidentally constructing `hintfmt`s with user-controlled strings. See
+     * the note on `fmt` for more information.
+     */
+    static hintfmt interpolate(const std::string & formatString)
     {
-        fmt.exceptions(boost::io::all_error_bits ^
+        hintfmt result((boost::format(formatString)));
+        result.fmt.exceptions(
+            boost::io::all_error_bits ^
             boost::io::too_many_args_bit ^
             boost::io::too_few_args_bit);
+        return result;
     }
 
-    hintformat(const hintformat & hf)
+    /**
+     * Format the given string literally, without interpolating format
+     * placeholders.
+     */
+    hintfmt(const std::string & literal)
+        : hintfmt("%s", Uncolored(literal))
+    { }
+
+    /**
+     * Interpolate the given arguments into the format string.
+     */
+    template<typename... Args>
+    hintfmt(const std::string & format, const Args & ... args)
+        : fmt(format)
+    {
+        formatHelper(*this, args...);
+    }
+
+    hintfmt(const hintfmt & hf)
         : fmt(hf.fmt)
     { }
 
-    hintformat(format && fmt)
+    hintfmt(boost::format && fmt)
         : fmt(std::move(fmt))
     { }
 
     template<class T>
-    hintformat & operator%(const T & value)
+    hintfmt & operator%(const T & value)
     {
-        fmt % magentatxt(value);
+        fmt % Magenta(value);
         return *this;
     }
 
     template<class T>
-    hintformat & operator%(const normaltxt<T> & value)
+    hintfmt & operator%(const Uncolored<T> & value)
     {
         fmt % value.value;
         return *this;
@@ -127,25 +189,8 @@ public:
     {
         return fmt.str();
     }
-
-private:
-    format fmt;
 };
 
-std::ostream & operator<<(std::ostream & os, const hintformat & hf);
-
-template<typename... Args>
-inline hintformat hintfmt(const std::string & fs, const Args & ... args)
-{
-    hintformat f(fs);
-    formatHelper(f, args...);
-    return f;
-}
-
-inline hintformat hintfmt(const std::string & plain_string)
-{
-    // we won't be receiving any args in this case, so just print the original string
-    return hintfmt("%s", normaltxt(plain_string));
-}
+std::ostream & operator<<(std::ostream & os, const hintfmt & hf);
 
 }

src/libutil/logging.hh

@@ -120,6 +120,17 @@ public:
     { }
 };
 
+/**
+ * A variadic template that does nothing.
+ *
+ * Useful to call a function with each argument in a parameter pack.
+ */
+struct nop
+{
+    template<typename... T> nop(T...)
+    { }
+};
+
 ActivityId getCurActivity();
 void setCurActivity(const ActivityId activityId);
 

tests/unit/libexpr/error_traces.cc

@@ -53,7 +53,6 @@ namespace nix {
                 state.error<EvalError>("beans").debugThrow();
             } catch (Error & e2) {
                 e.addTrace(state.positions[noPos], "beans2", "");
-                //e2.addTrace(state.positions[noPos], "Something", "");
                 ASSERT_TRUE(e.info().traces.size() == 2);
                 ASSERT_TRUE(e2.info().traces.size() == 0);
                 ASSERT_FALSE(&e.info() == &e2.info());

tests/unit/libutil/logging.cc

@@ -62,7 +62,7 @@ namespace nix {
             throw TestError(e.info());
         } catch (Error &e) {
             ErrorInfo ei = e.info();
-            ei.msg = hintfmt("%s; subsequent error message.", normaltxt(e.info().msg.str()));
+            ei.msg = hintfmt("%s; subsequent error message.", Uncolored(e.info().msg.str()));
 
             testing::internal::CaptureStderr();
             logger->logEI(ei);