From 149bd63afb30c5ae58eb3cc03fc208f89547cc16 Mon Sep 17 00:00:00 2001 From: Rebecca Turner Date: Sat, 3 Feb 2024 19:16:30 -0800 Subject: [PATCH] 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 | 2 +- src/libexpr/eval.hh | 2 +- src/libexpr/value/context.hh | 2 +- src/libstore/build/derivation-goal.cc | 8 +- src/libstore/build/local-derivation-goal.cc | 2 +- src/libstore/filetransfer.cc | 2 +- src/libstore/sqlite.cc | 6 +- src/libstore/sqlite.hh | 6 +- src/libutil/error.cc | 4 +- src/libutil/error.hh | 10 +- src/libutil/fmt.hh | 157 +++++++++++++------- src/libutil/logging.hh | 11 ++ tests/unit/libexpr/error_traces.cc | 1 - tests/unit/libutil/logging.cc | 2 +- 14 files changed, 135 insertions(+), 80 deletions(-) diff --git a/src/build-remote/build-remote.cc b/src/build-remote/build-remote.cc index 519e03242..94b672976 100644 --- a/src/build-remote/build-remote.cc +++ b/src/build-remote/build-remote.cc @@ -202,7 +202,7 @@ static int main_build_remote(int argc, char * * argv) else drvstr = ""; - auto error = hintformat(errorText); + auto error = hintfmt(errorText); error % drvstr % neededSystem diff --git a/src/libexpr/eval.hh b/src/libexpr/eval.hh index 3c7c5da27..f72135527 100644 --- a/src/libexpr/eval.hh +++ b/src/libexpr/eval.hh @@ -148,7 +148,7 @@ struct DebugTrace { std::shared_ptr pos; const Expr & expr; const Env & env; - hintformat hint; + hintfmt hint; bool isError; }; diff --git a/src/libexpr/value/context.hh b/src/libexpr/value/context.hh index 51fd30a44..2abd1c9d4 100644 --- a/src/libexpr/value/context.hh +++ b/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); } }; diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index 454c35763..d3bbdf1ed 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -708,7 +708,7 @@ void DerivationGoal::tryToBuild() if (!outputLocks.lockPaths(lockFiles, "", false)) { if (!actLock) actLock = std::make_unique(*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(*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) diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index ce8943efe..a2f411b8a 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -232,7 +232,7 @@ void LocalDerivationGoal::tryLocalBuild() if (!buildUser) { if (!actLock) actLock = std::make_unique(*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; } diff --git a/src/libstore/filetransfer.cc b/src/libstore/filetransfer.cc index dcbec4acd..eb39be158 100644 --- a/src/libstore/filetransfer.cc +++ b/src/libstore/filetransfer.cc @@ -887,7 +887,7 @@ FileTransferError::FileTransferError(FileTransfer::Error error, std::optionalsize() < 1024 || response->find("") != 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; } diff --git a/src/libstore/sqlite.cc b/src/libstore/sqlite.cc index d7432a305..ff14ec420 100644 --- a/src/libstore/sqlite.cc +++ b/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); diff --git a/src/libstore/sqlite.hh b/src/libstore/sqlite.hh index 0c08267f7..33ebb5892 100644 --- a/src/libstore/sqlite.hh +++ b/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 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); }; diff --git a/src/libutil/error.cc b/src/libutil/error.cc index e4e50d73b..e3b30b3a1 100644 --- a/src/libutil/error.cc +++ b/src/libutil/error.cc @@ -11,7 +11,7 @@ namespace nix { -void BaseError::addTrace(std::shared_ptr && e, hintformat hint, bool frame) +void BaseError::addTrace(std::shared_ptr && 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 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(); } diff --git a/src/libutil/error.hh b/src/libutil/error.hh index 4fb822843..966f4d770 100644 --- a/src/libutil/error.hh +++ b/src/libutil/error.hh @@ -63,7 +63,7 @@ void printCodeLines(std::ostream & out, struct Trace { std::shared_ptr 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; std::list 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 && e, hintformat hint, bool frame = false); + void addTrace(std::shared_ptr && 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)); } /** diff --git a/src/libutil/fmt.hh b/src/libutil/fmt.hh index 6430c7707..9c2cc1e85 100644 --- a/src/libutil/fmt.hh +++ b/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 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 inline void formatHelper(F & f) -{ -} +{ } template 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 -struct magentatxt +struct Magenta { - magentatxt(const T &s) : value(s) {} + Magenta(const T &s) : value(s) {} const T & value; }; template -std::ostream & operator<<(std::ostream & out, const magentatxt & y) +std::ostream & operator<<(std::ostream & out, const Magenta & 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 -struct normaltxt +struct Uncolored { - normaltxt(const T & s) : value(s) {} + Uncolored(const T & s) : value(s) {} const T & value; }; template -std::ostream & operator<<(std::ostream & out, const normaltxt & y) +std::ostream & operator<<(std::ostream & out, const Uncolored & 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 ^ - boost::io::too_many_args_bit ^ - boost::io::too_few_args_bit); + 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 + 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 - hintformat & operator%(const T & value) + hintfmt & operator%(const T & value) { - fmt % magentatxt(value); + fmt % Magenta(value); return *this; } template - hintformat & operator%(const normaltxt & value) + hintfmt & operator%(const Uncolored & 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 -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); } diff --git a/src/libutil/logging.hh b/src/libutil/logging.hh index 183f2d8e1..9e81132e3 100644 --- a/src/libutil/logging.hh +++ b/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 nop(T...) + { } +}; + ActivityId getCurActivity(); void setCurActivity(const ActivityId activityId); diff --git a/tests/unit/libexpr/error_traces.cc b/tests/unit/libexpr/error_traces.cc index 8e8726195..3cfa2b61b 100644 --- a/tests/unit/libexpr/error_traces.cc +++ b/tests/unit/libexpr/error_traces.cc @@ -53,7 +53,6 @@ namespace nix { state.error("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()); diff --git a/tests/unit/libutil/logging.cc b/tests/unit/libutil/logging.cc index 8950a26d4..c8c7c091f 100644 --- a/tests/unit/libutil/logging.cc +++ b/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);