From 4e0804c920558575a4b3486df1e595445bf67555 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Sun, 9 Apr 2023 22:42:20 +0200 Subject: [PATCH 1/5] Deduplicate string literal rendering, fix 4909 --- src/libcmd/repl.cc | 20 ++++++-------------- src/libexpr/eval.cc | 13 +++---------- src/libexpr/nixexpr.cc | 19 ++++--------------- src/libexpr/value/print.cc | 26 ++++++++++++++++++++++++++ src/libexpr/value/print.hh | 30 ++++++++++++++++++++++++++++++ src/libstore/derivations.cc | 9 +++++++++ tests/repl.sh | 8 ++++++++ 7 files changed, 86 insertions(+), 39 deletions(-) create mode 100644 src/libexpr/value/print.cc create mode 100644 src/libexpr/value/print.hh diff --git a/src/libcmd/repl.cc b/src/libcmd/repl.cc index 57848a5d3..1366622c7 100644 --- a/src/libcmd/repl.cc +++ b/src/libcmd/repl.cc @@ -40,6 +40,7 @@ extern "C" { #include "markdown.hh" #include "local-fs-store.hh" #include "progress-bar.hh" +#include "value/print.hh" #if HAVE_BOEHMGC #define GC_INCLUDE_NEW @@ -894,17 +895,6 @@ std::ostream & NixRepl::printValue(std::ostream & str, Value & v, unsigned int m } -std::ostream & printStringValue(std::ostream & str, const char * string) { - str << "\""; - for (const char * i = string; *i; i++) - if (*i == '\"' || *i == '\\') str << "\\" << *i; - else if (*i == '\n') str << "\\n"; - else if (*i == '\r') str << "\\r"; - else if (*i == '\t') str << "\\t"; - else str << *i; - str << "\""; - return str; -} // FIXME: lot of cut&paste from Nix's eval.cc. @@ -922,12 +912,14 @@ std::ostream & NixRepl::printValue(std::ostream & str, Value & v, unsigned int m break; case nBool: - str << ANSI_CYAN << (v.boolean ? "true" : "false") << ANSI_NORMAL; + str << ANSI_CYAN; + printLiteral(str, v.boolean); + str << ANSI_NORMAL; break; case nString: str << ANSI_WARNING; - printStringValue(str, v.string.s); + printLiteral(str, v.string.s); str << ANSI_NORMAL; break; @@ -967,7 +959,7 @@ std::ostream & NixRepl::printValue(std::ostream & str, Value & v, unsigned int m if (isVarName(i.first)) str << i.first; else - printStringValue(str, i.first.c_str()); + printLiteral(str, i.first); str << " = "; if (seen.count(i.second)) str << "«repeated»"; diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 18cfd9531..06208897f 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -9,6 +9,7 @@ #include "filetransfer.hh" #include "function-trace.hh" #include "profiles.hh" +#include "value/print.hh" #include #include @@ -104,18 +105,10 @@ void Value::print(const SymbolTable & symbols, std::ostream & str, str << integer; break; case tBool: - str << (boolean ? "true" : "false"); + printLiteral(str, boolean); break; case tString: - str << "\""; - for (const char * i = string.s; *i; i++) - if (*i == '\"' || *i == '\\') str << "\\" << *i; - else if (*i == '\n') str << "\\n"; - else if (*i == '\r') str << "\\r"; - else if (*i == '\t') str << "\\t"; - else if (*i == '$' && *(i+1) == '{') str << "\\" << *i; - else str << *i; - str << "\""; + printLiteral(str, string.s); break; case tPath: str << path; // !!! escaping? diff --git a/src/libexpr/nixexpr.cc b/src/libexpr/nixexpr.cc index eb6f062b4..ca6df0af3 100644 --- a/src/libexpr/nixexpr.cc +++ b/src/libexpr/nixexpr.cc @@ -3,6 +3,7 @@ #include "eval.hh" #include "symbol-table.hh" #include "util.hh" +#include "value/print.hh" #include @@ -62,18 +63,6 @@ Pos::operator std::shared_ptr() const /* Displaying abstract syntax trees. */ -static void showString(std::ostream & str, std::string_view s) -{ - str << '"'; - for (auto c : s) - if (c == '"' || c == '\\' || c == '$') str << "\\" << c; - else if (c == '\n') str << "\\n"; - else if (c == '\r') str << "\\r"; - else if (c == '\t') str << "\\t"; - else str << c; - str << '"'; -} - std::ostream & operator <<(std::ostream & str, const SymbolStr & symbol) { std::string_view s = symbol; @@ -85,7 +74,7 @@ std::ostream & operator <<(std::ostream & str, const SymbolStr & symbol) else { char c = s[0]; if (!((c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || c == '_')) { - showString(str, s); + printLiteral(str, s); return str; } for (auto c : s) @@ -93,7 +82,7 @@ std::ostream & operator <<(std::ostream & str, const SymbolStr & symbol) (c >= 'A' && c <= 'Z') || (c >= '0' && c <= '9') || c == '_' || c == '\'' || c == '-')) { - showString(str, s); + printLiteral(str, s); return str; } str << s; @@ -118,7 +107,7 @@ void ExprFloat::show(const SymbolTable & symbols, std::ostream & str) const void ExprString::show(const SymbolTable & symbols, std::ostream & str) const { - showString(str, s); + printLiteral(str, s); } void ExprPath::show(const SymbolTable & symbols, std::ostream & str) const diff --git a/src/libexpr/value/print.cc b/src/libexpr/value/print.cc new file mode 100644 index 000000000..cf97def46 --- /dev/null +++ b/src/libexpr/value/print.cc @@ -0,0 +1,26 @@ +#include "value/print.hh" + +namespace nix { + +std::ostream & +printLiteral(std::ostream & str, const std::string_view string) { + str << "\""; + for (auto i = string.begin(); i != string.end(); ++i) { + if (*i == '\"' || *i == '\\') str << "\\" << *i; + else if (*i == '\n') str << "\\n"; + else if (*i == '\r') str << "\\r"; + else if (*i == '\t') str << "\\t"; + else if (*i == '$' && *(i+1) == '{') str << "\\" << *i; + else str << *i; + } + str << "\""; + return str; +} + +std::ostream & +printLiteral(std::ostream & str, bool boolean) { + str << (boolean ? "true" : "false"); + return str; +} + +} diff --git a/src/libexpr/value/print.hh b/src/libexpr/value/print.hh new file mode 100644 index 000000000..31c94eb85 --- /dev/null +++ b/src/libexpr/value/print.hh @@ -0,0 +1,30 @@ +#pragma once +/** + * @file + * @brief Common printing functions for the Nix language + * + * While most types come with their own methods for printing, they share some + * functions that are placed here. + */ + +#include + +namespace nix { + /** + * Print a string as a Nix string literal. + * + * Quotes and fairly minimal escaping are added. + * + * @param s The logical string + */ + std::ostream & printLiteral(std::ostream & o, std::string_view s); + inline std::ostream & printLiteral(std::ostream & o, const char * s) { + return printLiteral(o, std::string_view(s)); + } + inline std::ostream & printLiteral(std::ostream & o, const std::string & s) { + return printLiteral(o, std::string_view(s)); + } + + /** Print `true` or `false`. */ + std::ostream & printLiteral(std::ostream & o, bool b); +} diff --git a/src/libstore/derivations.cc b/src/libstore/derivations.cc index abdfb1978..9948862e5 100644 --- a/src/libstore/derivations.cc +++ b/src/libstore/derivations.cc @@ -313,6 +313,15 @@ Derivation parseDerivation(const Store & store, std::string && s, std::string_vi } +/** + * Print a derivation string literal to an std::string. + * + * This syntax does not generalize to the expression language, which needs to + * escape `$`. + * + * @param res Where to print to + * @param s Which logical string to print + */ static void printString(std::string & res, std::string_view s) { boost::container::small_vector buffer; diff --git a/tests/repl.sh b/tests/repl.sh index be8adb742..2b3789521 100644 --- a/tests/repl.sh +++ b/tests/repl.sh @@ -79,6 +79,14 @@ testReplResponse ' "result: ${a}" ' "result: 2" +# check dollar escaping https://github.com/NixOS/nix/issues/4909 +# note the escaped \, +# \\ +# because the second argument is a regex +testReplResponse ' +"$" + "{hi}" +' '"\\${hi}"' + testReplResponse ' drvPath ' '".*-simple.drv"' \ From 9c74df5bb4f41c938a4f6942492f5ce92b0ef371 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Sat, 15 Apr 2023 20:56:51 +0200 Subject: [PATCH 2/5] Format Co-authored-by: Eelco Dolstra Co-authored-by: John Ericson --- src/libexpr/value/print.cc | 6 ++++-- src/libstore/derivations.cc | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/libexpr/value/print.cc b/src/libexpr/value/print.cc index cf97def46..c67ff9f87 100644 --- a/src/libexpr/value/print.cc +++ b/src/libexpr/value/print.cc @@ -3,7 +3,8 @@ namespace nix { std::ostream & -printLiteral(std::ostream & str, const std::string_view string) { +printLiteral(std::ostream & str, const std::string_view string) +{ str << "\""; for (auto i = string.begin(); i != string.end(); ++i) { if (*i == '\"' || *i == '\\') str << "\\" << *i; @@ -18,7 +19,8 @@ printLiteral(std::ostream & str, const std::string_view string) { } std::ostream & -printLiteral(std::ostream & str, bool boolean) { +printLiteral(std::ostream & str, bool boolean) +{ str << (boolean ? "true" : "false"); return str; } diff --git a/src/libstore/derivations.cc b/src/libstore/derivations.cc index 9948862e5..7eb5cd275 100644 --- a/src/libstore/derivations.cc +++ b/src/libstore/derivations.cc @@ -314,7 +314,7 @@ Derivation parseDerivation(const Store & store, std::string && s, std::string_vi /** - * Print a derivation string literal to an std::string. + * Print a derivation string literal to an `std::string`. * * This syntax does not generalize to the expression language, which needs to * escape `$`. From 1e2dd669bcdd8df6cdaac061e035828626906447 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Sun, 16 Apr 2023 12:56:31 +0200 Subject: [PATCH 3/5] printLiteral: Do not overload --- src/libcmd/repl.cc | 6 +++--- src/libexpr/eval.cc | 4 ++-- src/libexpr/nixexpr.cc | 6 +++--- src/libexpr/value/print.cc | 4 ++-- src/libexpr/value/print.hh | 12 ++++++------ 5 files changed, 16 insertions(+), 16 deletions(-) diff --git a/src/libcmd/repl.cc b/src/libcmd/repl.cc index 1366622c7..41cf77424 100644 --- a/src/libcmd/repl.cc +++ b/src/libcmd/repl.cc @@ -913,13 +913,13 @@ std::ostream & NixRepl::printValue(std::ostream & str, Value & v, unsigned int m case nBool: str << ANSI_CYAN; - printLiteral(str, v.boolean); + printLiteralBool(str, v.boolean); str << ANSI_NORMAL; break; case nString: str << ANSI_WARNING; - printLiteral(str, v.string.s); + printLiteralString(str, v.string.s); str << ANSI_NORMAL; break; @@ -959,7 +959,7 @@ std::ostream & NixRepl::printValue(std::ostream & str, Value & v, unsigned int m if (isVarName(i.first)) str << i.first; else - printLiteral(str, i.first); + printLiteralString(str, i.first); str << " = "; if (seen.count(i.second)) str << "«repeated»"; diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 06208897f..bd05fc156 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -105,10 +105,10 @@ void Value::print(const SymbolTable & symbols, std::ostream & str, str << integer; break; case tBool: - printLiteral(str, boolean); + printLiteralBool(str, boolean); break; case tString: - printLiteral(str, string.s); + printLiteralString(str, string.s); break; case tPath: str << path; // !!! escaping? diff --git a/src/libexpr/nixexpr.cc b/src/libexpr/nixexpr.cc index ca6df0af3..1b5d522d3 100644 --- a/src/libexpr/nixexpr.cc +++ b/src/libexpr/nixexpr.cc @@ -74,7 +74,7 @@ std::ostream & operator <<(std::ostream & str, const SymbolStr & symbol) else { char c = s[0]; if (!((c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || c == '_')) { - printLiteral(str, s); + printLiteralString(str, s); return str; } for (auto c : s) @@ -82,7 +82,7 @@ std::ostream & operator <<(std::ostream & str, const SymbolStr & symbol) (c >= 'A' && c <= 'Z') || (c >= '0' && c <= '9') || c == '_' || c == '\'' || c == '-')) { - printLiteral(str, s); + printLiteralString(str, s); return str; } str << s; @@ -107,7 +107,7 @@ void ExprFloat::show(const SymbolTable & symbols, std::ostream & str) const void ExprString::show(const SymbolTable & symbols, std::ostream & str) const { - printLiteral(str, s); + printLiteralString(str, s); } void ExprPath::show(const SymbolTable & symbols, std::ostream & str) const diff --git a/src/libexpr/value/print.cc b/src/libexpr/value/print.cc index c67ff9f87..bd241d9d8 100644 --- a/src/libexpr/value/print.cc +++ b/src/libexpr/value/print.cc @@ -3,7 +3,7 @@ namespace nix { std::ostream & -printLiteral(std::ostream & str, const std::string_view string) +printLiteralString(std::ostream & str, const std::string_view string) { str << "\""; for (auto i = string.begin(); i != string.end(); ++i) { @@ -19,7 +19,7 @@ printLiteral(std::ostream & str, const std::string_view string) } std::ostream & -printLiteral(std::ostream & str, bool boolean) +printLiteralBool(std::ostream & str, bool boolean) { str << (boolean ? "true" : "false"); return str; diff --git a/src/libexpr/value/print.hh b/src/libexpr/value/print.hh index 31c94eb85..98dd2008d 100644 --- a/src/libexpr/value/print.hh +++ b/src/libexpr/value/print.hh @@ -17,14 +17,14 @@ namespace nix { * * @param s The logical string */ - std::ostream & printLiteral(std::ostream & o, std::string_view s); - inline std::ostream & printLiteral(std::ostream & o, const char * s) { - return printLiteral(o, std::string_view(s)); + std::ostream & printLiteralString(std::ostream & o, std::string_view s); + inline std::ostream & printLiteralString(std::ostream & o, const char * s) { + return printLiteralString(o, std::string_view(s)); } - inline std::ostream & printLiteral(std::ostream & o, const std::string & s) { - return printLiteral(o, std::string_view(s)); + inline std::ostream & printLiteralString(std::ostream & o, const std::string & s) { + return printLiteralString(o, std::string_view(s)); } /** Print `true` or `false`. */ - std::ostream & printLiteral(std::ostream & o, bool b); + std::ostream & printLiteralBool(std::ostream & o, bool b); } From 28a5cdde02964306e7eb443f696c8d5d59ebf9e9 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Sun, 16 Apr 2023 13:10:45 +0200 Subject: [PATCH 4/5] libexpr/value/print.* -> libexpr/print.* Generalizes the file to sensibly allow printing any part of the language syntax. --- src/libcmd/repl.cc | 2 +- src/libexpr/eval.cc | 2 +- src/libexpr/nixexpr.cc | 2 +- src/libexpr/{value => }/print.cc | 2 +- src/libexpr/{value => }/print.hh | 0 5 files changed, 4 insertions(+), 4 deletions(-) rename src/libexpr/{value => }/print.cc (96%) rename src/libexpr/{value => }/print.hh (100%) diff --git a/src/libcmd/repl.cc b/src/libcmd/repl.cc index 41cf77424..806dce024 100644 --- a/src/libcmd/repl.cc +++ b/src/libcmd/repl.cc @@ -40,7 +40,7 @@ extern "C" { #include "markdown.hh" #include "local-fs-store.hh" #include "progress-bar.hh" -#include "value/print.hh" +#include "print.hh" #if HAVE_BOEHMGC #define GC_INCLUDE_NEW diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index bd05fc156..6668add8c 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -9,7 +9,7 @@ #include "filetransfer.hh" #include "function-trace.hh" #include "profiles.hh" -#include "value/print.hh" +#include "print.hh" #include #include diff --git a/src/libexpr/nixexpr.cc b/src/libexpr/nixexpr.cc index 1b5d522d3..d8f3cd701 100644 --- a/src/libexpr/nixexpr.cc +++ b/src/libexpr/nixexpr.cc @@ -3,7 +3,7 @@ #include "eval.hh" #include "symbol-table.hh" #include "util.hh" -#include "value/print.hh" +#include "print.hh" #include diff --git a/src/libexpr/value/print.cc b/src/libexpr/print.cc similarity index 96% rename from src/libexpr/value/print.cc rename to src/libexpr/print.cc index bd241d9d8..282903b72 100644 --- a/src/libexpr/value/print.cc +++ b/src/libexpr/print.cc @@ -1,4 +1,4 @@ -#include "value/print.hh" +#include "print.hh" namespace nix { diff --git a/src/libexpr/value/print.hh b/src/libexpr/print.hh similarity index 100% rename from src/libexpr/value/print.hh rename to src/libexpr/print.hh From b6125772d7d5f82d48873fc93a7f261832154b14 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Sun, 16 Apr 2023 14:07:35 +0200 Subject: [PATCH 5/5] libexpr: Move identifier-like printing to print.cc --- src/libcmd/repl.cc | 6 ++--- src/libexpr/nixexpr.cc | 27 +++-------------------- src/libexpr/print.cc | 50 ++++++++++++++++++++++++++++++++++++++++++ src/libexpr/print.hh | 18 +++++++++++++++ 4 files changed, 73 insertions(+), 28 deletions(-) diff --git a/src/libcmd/repl.cc b/src/libcmd/repl.cc index 806dce024..80c08bf1c 100644 --- a/src/libcmd/repl.cc +++ b/src/libcmd/repl.cc @@ -426,6 +426,7 @@ StringSet NixRepl::completePrefix(const std::string & prefix) } +// FIXME: DRY and match or use the parser static bool isVarName(std::string_view s) { if (s.size() == 0) return false; @@ -956,10 +957,7 @@ std::ostream & NixRepl::printValue(std::ostream & str, Value & v, unsigned int m sorted.emplace(state->symbols[i.name], i.value); for (auto & i : sorted) { - if (isVarName(i.first)) - str << i.first; - else - printLiteralString(str, i.first); + printAttributeName(str, i.first); str << " = "; if (seen.count(i.second)) str << "«repeated»"; diff --git a/src/libexpr/nixexpr.cc b/src/libexpr/nixexpr.cc index d8f3cd701..1557cbbeb 100644 --- a/src/libexpr/nixexpr.cc +++ b/src/libexpr/nixexpr.cc @@ -61,33 +61,12 @@ Pos::operator std::shared_ptr() const return pos; } -/* Displaying abstract syntax trees. */ - +// FIXME: remove, because *symbols* are abstract and do not have a single +// textual representation; see printIdentifier() std::ostream & operator <<(std::ostream & str, const SymbolStr & symbol) { std::string_view s = symbol; - - if (s.empty()) - str << "\"\""; - else if (s == "if") // FIXME: handle other keywords - str << '"' << s << '"'; - else { - char c = s[0]; - if (!((c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || c == '_')) { - printLiteralString(str, s); - return str; - } - for (auto c : s) - if (!((c >= 'a' && c <= 'z') || - (c >= 'A' && c <= 'Z') || - (c >= '0' && c <= '9') || - c == '_' || c == '\'' || c == '-')) { - printLiteralString(str, s); - return str; - } - str << s; - } - return str; + return printIdentifier(str, s); } void Expr::show(const SymbolTable & symbols, std::ostream & str) const diff --git a/src/libexpr/print.cc b/src/libexpr/print.cc index 282903b72..d08672cfc 100644 --- a/src/libexpr/print.cc +++ b/src/libexpr/print.cc @@ -25,4 +25,54 @@ printLiteralBool(std::ostream & str, bool boolean) return str; } +std::ostream & +printIdentifier(std::ostream & str, std::string_view s) { + if (s.empty()) + str << "\"\""; + else if (s == "if") // FIXME: handle other keywords + str << '"' << s << '"'; + else { + char c = s[0]; + if (!((c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || c == '_')) { + printLiteralString(str, s); + return str; + } + for (auto c : s) + if (!((c >= 'a' && c <= 'z') || + (c >= 'A' && c <= 'Z') || + (c >= '0' && c <= '9') || + c == '_' || c == '\'' || c == '-')) { + printLiteralString(str, s); + return str; + } + str << s; + } + return str; +} + +// FIXME: keywords +static bool isVarName(std::string_view s) +{ + if (s.size() == 0) return false; + char c = s[0]; + if ((c >= '0' && c <= '9') || c == '-' || c == '\'') return false; + for (auto & i : s) + if (!((i >= 'a' && i <= 'z') || + (i >= 'A' && i <= 'Z') || + (i >= '0' && i <= '9') || + i == '_' || i == '-' || i == '\'')) + return false; + return true; +} + +std::ostream & +printAttributeName(std::ostream & str, std::string_view name) { + if (isVarName(name)) + str << name; + else + printLiteralString(str, name); + return str; +} + + } diff --git a/src/libexpr/print.hh b/src/libexpr/print.hh index 98dd2008d..f9cfc3964 100644 --- a/src/libexpr/print.hh +++ b/src/libexpr/print.hh @@ -27,4 +27,22 @@ namespace nix { /** Print `true` or `false`. */ std::ostream & printLiteralBool(std::ostream & o, bool b); + + /** + * Print a string as an attribute name in the Nix expression language syntax. + * + * Prints a quoted string if necessary. + */ + std::ostream & printAttributeName(std::ostream & o, std::string_view s); + + /** + * Print a string as an identifier in the Nix expression language syntax. + * + * FIXME: "identifier" is ambiguous. Identifiers do not have a single + * textual representation. They can be used in variable references, + * let bindings, left-hand sides or attribute names in a select + * expression, or something else entirely, like JSON. Use one of the + * `print*` functions instead. + */ + std::ostream & printIdentifier(std::ostream & o, std::string_view s); }