From d57b43f1a4d8bdfb2ad0552befbd551dfa0e31fe Mon Sep 17 00:00:00 2001 From: Rebecca Turner Date: Wed, 6 Dec 2023 12:42:53 -0800 Subject: [PATCH] Print the value in `error: cannot coerce` messages This extends the `error: cannot coerce a TYPE to a string` message to print the value that could not be coerced. This helps with debugging by making it easier to track down where the value is being produced from, especially in errors with deep or unhelpful stack traces. --- .../rl-next/print-value-in-coercion-error.md | 75 +++++++++++++++++++ .../src/language/string-interpolation.md | 2 +- src/libexpr/eval.cc | 11 ++- src/libexpr/print-ambiguous.cc | 1 + src/libexpr/print-options.hh | 22 +++++- src/libexpr/print.cc | 18 ++++- src/libexpr/print.hh | 21 +++++- ...al-fail-bad-string-interpolation-1.err.exp | 2 +- ...al-fail-bad-string-interpolation-3.err.exp | 2 +- ...al-fail-bad-string-interpolation-4.err.exp | 2 +- tests/unit/libexpr/error_traces.cc | 28 +++---- tests/unit/libexpr/value/print.cc | 10 +-- 12 files changed, 161 insertions(+), 33 deletions(-) create mode 100644 doc/manual/rl-next/print-value-in-coercion-error.md diff --git a/doc/manual/rl-next/print-value-in-coercion-error.md b/doc/manual/rl-next/print-value-in-coercion-error.md new file mode 100644 index 000000000000..22b2765ebf91 --- /dev/null +++ b/doc/manual/rl-next/print-value-in-coercion-error.md @@ -0,0 +1,75 @@ +--- +synopsis: Coercion errors include the failing value +issues: #561 +prs: #9754 +--- +description: { + +The `error: cannot coerce a to a string` message now includes the value +which caused the error. This makes debugging much easier: + +``` +$ cat bad.nix +let + pkgs = import {}; + system = pkgs.lib.systems.elaborate "x86_64-linux"; +in + import {inherit system;} +``` + +Previously, attempting to evaluate this expression would produce a confusing error message: + +``` +$ nix-instantiate --eval bad.nix +error: + … while evaluating a branch condition + + at /nix/store/m8ah0r1ih2shq35vp3hj1k0m1c4hsfga-nixpkgs/nixpkgs/pkgs/stdenv/booter.nix:64:9: + + 63| go = pred: n: + 64| if n == len + | ^ + 65| then rnul pred + + … while calling the 'length' builtin + + at /nix/store/m8ah0r1ih2shq35vp3hj1k0m1c4hsfga-nixpkgs/nixpkgs/pkgs/stdenv/booter.nix:62:13: + + 61| let + 62| len = builtins.length list; + | ^ + 63| go = pred: n: + + (stack trace truncated; use '--show-trace' to show the full trace) + + error: cannot coerce a set to a string +``` + +Now, the error message includes the set itself. This makes debugging much +simpler, especially when the trace doesn't show the failing expression: + +``` +$ nix-instantiate --eval bad.nix +error: + … while evaluating a branch condition + at /nix/store/m8ah0r1ih2shq35vp3hj1k0m1c4hsfga-nixpkgs/nixpkgs/pkgs/stdenv/booter.nix:64:9: + 63| go = pred: n: + 64| if n == len + | ^ + 65| then rnul pred + + … while calling the 'length' builtin + at /nix/store/m8ah0r1ih2shq35vp3hj1k0m1c4hsfga-nixpkgs/nixpkgs/pkgs/stdenv/booter.nix:62:13: + 61| let + 62| len = builtins.length list; + | ^ + 63| go = pred: n: + + (stack trace truncated; use '--show-trace' to show the full trace) + + error: cannot coerce a set to a string: { aesSupport = «thunk»; + avx2Support = «thunk»; avx512Support = «thunk»; avxSupport = «thunk»; + canExecute = «thunk»; config = «thunk»; darwinArch = «thunk»; darwinMinVersion + = «thunk»; darwinMinVersionVariable = «thunk»; darwinPlatform = «thunk»; «84 + attributes elided»} +``` diff --git a/doc/manual/src/language/string-interpolation.md b/doc/manual/src/language/string-interpolation.md index e999b287b9c6..6e28d26646bc 100644 --- a/doc/manual/src/language/string-interpolation.md +++ b/doc/manual/src/language/string-interpolation.md @@ -189,7 +189,7 @@ If neither is present, an error is thrown. > "${a}" > ``` > -> error: cannot coerce a set to a string +> error: cannot coerce a set to a string: { } > > at «string»:4:2: > diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 0659a217393b..4c3ad056527b 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -29,7 +29,6 @@ #include #include #include -#include #include #include @@ -2233,7 +2232,9 @@ BackedStringView EvalState::coerceToString( return std::move(*maybeString); auto i = v.attrs->find(sOutPath); if (i == v.attrs->end()) { - error("cannot coerce %1% to a string", showType(v)) + error("cannot coerce %1% to a string: %2%", + showType(v), + ValuePrinter(*this, v, debugPrintOptions)) .withTrace(pos, errorCtx) .debugThrow(); } @@ -2279,7 +2280,9 @@ BackedStringView EvalState::coerceToString( } } - error("cannot coerce %1% to a string", showType(v)) + error("cannot coerce %1% to a string: %2%", + showType(v), + ValuePrinter(*this, v, debugPrintOptions)) .withTrace(pos, errorCtx) .debugThrow(); } @@ -2639,7 +2642,7 @@ void EvalState::printStatistics() std::string ExternalValueBase::coerceToString(const Pos & pos, NixStringContext & context, bool copyMore, bool copyToStore) const { throw TypeError({ - .msg = hintfmt("cannot coerce %1% to a string", showType()) + .msg = hintfmt("cannot coerce %1% to a string: %2%", showType(), *this) }); } diff --git a/src/libexpr/print-ambiguous.cc b/src/libexpr/print-ambiguous.cc index 07c398dd25a6..521250cecdb8 100644 --- a/src/libexpr/print-ambiguous.cc +++ b/src/libexpr/print-ambiguous.cc @@ -1,6 +1,7 @@ #include "print-ambiguous.hh" #include "print.hh" #include "signals.hh" +#include "eval.hh" namespace nix { diff --git a/src/libexpr/print-options.hh b/src/libexpr/print-options.hh index 11ff9ae879eb..256564f336a5 100644 --- a/src/libexpr/print-options.hh +++ b/src/libexpr/print-options.hh @@ -36,11 +36,17 @@ struct PrintOptions */ size_t maxDepth = std::numeric_limits::max(); /** - * Maximum number of attributes in an attribute set to print. + * Maximum number of attributes in attribute sets to print. + * + * Note that this is a limit for the entire print invocation, not for each + * attribute set encountered. */ size_t maxAttrs = std::numeric_limits::max(); /** * Maximum number of list items to print. + * + * Note that this is a limit for the entire print invocation, not for each + * list encountered. */ size_t maxListItems = std::numeric_limits::max(); /** @@ -49,4 +55,18 @@ struct PrintOptions size_t maxStringLength = std::numeric_limits::max(); }; +/** + * `PrintOptions` suitable for debugging. + * + * These options are used for printing values in error messages without + * printing "too much" output. + */ +static PrintOptions debugPrintOptions = PrintOptions { + .ansiColors = true, + .maxDepth = 10, + .maxAttrs = 10, + .maxListItems = 10, + .maxStringLength = 1024 +}; + } diff --git a/src/libexpr/print.cc b/src/libexpr/print.cc index db26ed4c285c..702e4bfe8c10 100644 --- a/src/libexpr/print.cc +++ b/src/libexpr/print.cc @@ -7,6 +7,7 @@ #include "store-api.hh" #include "terminal.hh" #include "english.hh" +#include "eval.hh" namespace nix { @@ -19,7 +20,7 @@ void printElided( { if (ansiColors) output << ANSI_FAINT; - output << " «"; + output << "«"; pluralize(output, value, single, plural); output << " elided»"; if (ansiColors) @@ -36,7 +37,7 @@ printLiteralString(std::ostream & str, const std::string_view string, size_t max str << "\""; for (auto i = string.begin(); i != string.end(); ++i) { if (charsPrinted >= maxLength) { - str << "\""; + str << "\" "; printElided(str, string.length() - charsPrinted, "byte", "bytes", ansiColors); return str; } @@ -160,6 +161,8 @@ class Printer EvalState & state; PrintOptions options; std::optional seen; + size_t attrsPrinted = 0; + size_t listItemsPrinted = 0; void printRepeated() { @@ -278,7 +281,6 @@ class Printer else std::sort(sorted.begin(), sorted.end(), ImportantFirstAttrNameCmp()); - size_t attrsPrinted = 0; for (auto & i : sorted) { if (attrsPrinted >= options.maxAttrs) { printElided(sorted.size() - attrsPrinted, "attribute", "attributes"); @@ -306,7 +308,6 @@ class Printer output << "[ "; if (depth < options.maxDepth) { - size_t listItemsPrinted = 0; for (auto elem : v.listItems()) { if (listItemsPrinted >= options.maxListItems) { printElided(v.listSize() - listItemsPrinted, "item", "items"); @@ -485,6 +486,9 @@ class Printer void print(Value & v) { + attrsPrinted = 0; + listItemsPrinted = 0; + if (options.trackRepeated) { seen.emplace(); } else { @@ -501,4 +505,10 @@ void printValue(EvalState & state, std::ostream & output, Value & v, PrintOption Printer(output, state, options).print(v); } +std::ostream & operator<<(std::ostream & output, const ValuePrinter & printer) +{ + printValue(printer.state, output, printer.value, printer.options); + return output; +} + } diff --git a/src/libexpr/print.hh b/src/libexpr/print.hh index 40207d77773f..a8300264a300 100644 --- a/src/libexpr/print.hh +++ b/src/libexpr/print.hh @@ -9,11 +9,13 @@ #include -#include "eval.hh" #include "print-options.hh" namespace nix { +class EvalState; +struct Value; + /** * Print a string as a Nix string literal. * @@ -59,4 +61,21 @@ std::ostream & printIdentifier(std::ostream & o, std::string_view s); void printValue(EvalState & state, std::ostream & str, Value & v, PrintOptions options = PrintOptions {}); +/** + * A partially-applied form of `printValue` which can be formatted using `<<` + * without allocating an intermediate string. + */ +class ValuePrinter { + friend std::ostream & operator << (std::ostream & output, const ValuePrinter & printer); +private: + EvalState & state; + Value & value; + PrintOptions options; + +public: + ValuePrinter(EvalState & state, Value & value, PrintOptions options = PrintOptions {}) + : state(state), value(value), options(options) { } +}; + +std::ostream & operator<<(std::ostream & output, const ValuePrinter & printer); } diff --git a/tests/functional/lang/eval-fail-bad-string-interpolation-1.err.exp b/tests/functional/lang/eval-fail-bad-string-interpolation-1.err.exp index b461b2e02adf..5ae53034d187 100644 --- a/tests/functional/lang/eval-fail-bad-string-interpolation-1.err.exp +++ b/tests/functional/lang/eval-fail-bad-string-interpolation-1.err.exp @@ -5,4 +5,4 @@ error: | ^ 2| - error: cannot coerce a function to a string + error: cannot coerce a function to a string: «lambda @ /pwd/lang/eval-fail-bad-string-interpolation-1.nix:1:4» diff --git a/tests/functional/lang/eval-fail-bad-string-interpolation-3.err.exp b/tests/functional/lang/eval-fail-bad-string-interpolation-3.err.exp index 95f4c2460043..170a3d13233c 100644 --- a/tests/functional/lang/eval-fail-bad-string-interpolation-3.err.exp +++ b/tests/functional/lang/eval-fail-bad-string-interpolation-3.err.exp @@ -5,4 +5,4 @@ error: | ^ 2| - error: cannot coerce a function to a string + error: cannot coerce a function to a string: «lambda @ /pwd/lang/eval-fail-bad-string-interpolation-3.nix:1:5» diff --git a/tests/functional/lang/eval-fail-bad-string-interpolation-4.err.exp b/tests/functional/lang/eval-fail-bad-string-interpolation-4.err.exp index 4950f8ddb809..5119238d77d8 100644 --- a/tests/functional/lang/eval-fail-bad-string-interpolation-4.err.exp +++ b/tests/functional/lang/eval-fail-bad-string-interpolation-4.err.exp @@ -6,4 +6,4 @@ error: | ^ 10| - error: cannot coerce a set to a string + error: cannot coerce a set to a string: { a = { a = { a = { a = "ha"; b = "ha"; c = "ha"; d = "ha"; e = "ha"; f = "ha"; g = "ha"; h = "ha"; j = "ha"; }; «4294967295 attributes elided»}; «4294967294 attributes elided»}; «4294967293 attributes elided»} diff --git a/tests/unit/libexpr/error_traces.cc b/tests/unit/libexpr/error_traces.cc index 81498f65a3f8..6814654e2765 100644 --- a/tests/unit/libexpr/error_traces.cc +++ b/tests/unit/libexpr/error_traces.cc @@ -295,7 +295,7 @@ namespace nix { TEST_F(ErrorTraceTest, toPath) { ASSERT_TRACE2("toPath []", TypeError, - hintfmt("cannot coerce %s to a string", "a list"), + hintfmt("cannot coerce %s to a string: %s", "a list", "[ ]"), hintfmt("while evaluating the first argument passed to builtins.toPath")); ASSERT_TRACE2("toPath \"foo\"", @@ -309,7 +309,7 @@ namespace nix { TEST_F(ErrorTraceTest, storePath) { ASSERT_TRACE2("storePath true", TypeError, - hintfmt("cannot coerce %s to a string", "a Boolean"), + hintfmt("cannot coerce %s to a string: %s", "a Boolean", ANSI_CYAN "true" ANSI_NORMAL), hintfmt("while evaluating the first argument passed to 'builtins.storePath'")); } @@ -318,7 +318,7 @@ namespace nix { TEST_F(ErrorTraceTest, pathExists) { ASSERT_TRACE2("pathExists []", TypeError, - hintfmt("cannot coerce %s to a string", "a list"), + hintfmt("cannot coerce %s to a string: %s", "a list", "[ ]"), hintfmt("while realising the context of a path")); ASSERT_TRACE2("pathExists \"zorglub\"", @@ -332,7 +332,7 @@ namespace nix { TEST_F(ErrorTraceTest, baseNameOf) { ASSERT_TRACE2("baseNameOf []", TypeError, - hintfmt("cannot coerce %s to a string", "a list"), + hintfmt("cannot coerce %s to a string: %s", "a list", "[ ]"), hintfmt("while evaluating the first argument passed to builtins.baseNameOf")); } @@ -377,7 +377,7 @@ namespace nix { TEST_F(ErrorTraceTest, filterSource) { ASSERT_TRACE2("filterSource [] []", TypeError, - hintfmt("cannot coerce %s to a string", "a list"), + hintfmt("cannot coerce %s to a string: %s", "a list", "[ ]"), hintfmt("while evaluating the second argument (the path to filter) passed to 'builtins.filterSource'")); ASSERT_TRACE2("filterSource [] \"foo\"", @@ -1038,7 +1038,7 @@ namespace nix { TEST_F(ErrorTraceTest, toString) { ASSERT_TRACE2("toString { a = 1; }", TypeError, - hintfmt("cannot coerce %s to a string", "a set"), + hintfmt("cannot coerce %s to a string: %s", "a set", "{ a = " ANSI_CYAN "1" ANSI_NORMAL "; }"), hintfmt("while evaluating the first argument passed to builtins.toString")); } @@ -1057,7 +1057,7 @@ namespace nix { ASSERT_TRACE2("substring 0 3 {}", TypeError, - hintfmt("cannot coerce %s to a string", "a set"), + hintfmt("cannot coerce %s to a string: %s", "a set", "{ }"), hintfmt("while evaluating the third argument (the string) passed to builtins.substring")); ASSERT_TRACE1("substring (-3) 3 \"sometext\"", @@ -1070,7 +1070,7 @@ namespace nix { TEST_F(ErrorTraceTest, stringLength) { ASSERT_TRACE2("stringLength {} # TODO: context is missing ???", TypeError, - hintfmt("cannot coerce %s to a string", "a set"), + hintfmt("cannot coerce %s to a string: %s", "a set", "{ }"), hintfmt("while evaluating the argument passed to builtins.stringLength")); } @@ -1143,7 +1143,7 @@ namespace nix { ASSERT_TRACE2("concatStringsSep \"foo\" [ 1 2 {} ] # TODO: coerce to string is buggy", TypeError, - hintfmt("cannot coerce %s to a string", "an integer"), + hintfmt("cannot coerce %s to a string: %s", "an integer", ANSI_CYAN "1" ANSI_NORMAL), hintfmt("while evaluating one element of the list of strings to concat passed to builtins.concatStringsSep")); } @@ -1229,12 +1229,12 @@ namespace nix { ASSERT_TRACE2("derivationStrict { name = \"foo\"; builder = 1; system = {}; }", TypeError, - hintfmt("cannot coerce %s to a string", "a set"), + hintfmt("cannot coerce %s to a string: %s", "a set", "{ }"), hintfmt("while evaluating the attribute 'system' of derivation 'foo'")); ASSERT_TRACE2("derivationStrict { name = \"foo\"; builder = 1; system = 1; outputs = {}; }", TypeError, - hintfmt("cannot coerce %s to a string", "a set"), + hintfmt("cannot coerce %s to a string: %s", "a set", "{ }"), hintfmt("while evaluating the attribute 'outputs' of derivation 'foo'")); ASSERT_TRACE2("derivationStrict { name = \"foo\"; builder = 1; system = 1; outputs = \"drv\"; }", @@ -1279,17 +1279,17 @@ namespace nix { ASSERT_TRACE2("derivationStrict { name = \"foo\"; builder = 1; system = 1; outputs = \"out\"; args = [ {} ]; }", TypeError, - hintfmt("cannot coerce %s to a string", "a set"), + hintfmt("cannot coerce %s to a string: %s", "a set", "{ }"), hintfmt("while evaluating an element of the argument list")); ASSERT_TRACE2("derivationStrict { name = \"foo\"; builder = 1; system = 1; outputs = \"out\"; args = [ \"a\" {} ]; }", TypeError, - hintfmt("cannot coerce %s to a string", "a set"), + hintfmt("cannot coerce %s to a string: %s", "a set", "{ }"), hintfmt("while evaluating an element of the argument list")); ASSERT_TRACE2("derivationStrict { name = \"foo\"; builder = 1; system = 1; outputs = \"out\"; FOO = {}; }", TypeError, - hintfmt("cannot coerce %s to a string", "a set"), + hintfmt("cannot coerce %s to a string: %s", "a set", "{ }"), hintfmt("while evaluating the attribute 'FOO' of derivation 'foo'")); } diff --git a/tests/unit/libexpr/value/print.cc b/tests/unit/libexpr/value/print.cc index 98131112e40b..c4264a38dbc4 100644 --- a/tests/unit/libexpr/value/print.cc +++ b/tests/unit/libexpr/value/print.cc @@ -370,7 +370,7 @@ TEST_F(ValuePrintingTests, ansiColorsStringElided) v.mkString("puppy"); test(v, - ANSI_MAGENTA "\"pup\"" ANSI_FAINT " «2 bytes elided»" ANSI_NORMAL, + ANSI_MAGENTA "\"pup\" " ANSI_FAINT "«2 bytes elided»" ANSI_NORMAL, PrintOptions { .ansiColors = true, .maxStringLength = 3 @@ -756,7 +756,7 @@ TEST_F(ValuePrintingTests, ansiColorsAttrsElided) vAttrs.mkAttrs(builder.finish()); test(vAttrs, - "{ one = " ANSI_CYAN "1" ANSI_NORMAL "; " ANSI_FAINT " «1 attribute elided»" ANSI_NORMAL "}", + "{ one = " ANSI_CYAN "1" ANSI_NORMAL "; " ANSI_FAINT "«1 attribute elided»" ANSI_NORMAL "}", PrintOptions { .ansiColors = true, .maxAttrs = 1 @@ -769,7 +769,7 @@ TEST_F(ValuePrintingTests, ansiColorsAttrsElided) vAttrs.mkAttrs(builder.finish()); test(vAttrs, - "{ one = " ANSI_CYAN "1" ANSI_NORMAL "; " ANSI_FAINT " «2 attributes elided»" ANSI_NORMAL "}", + "{ one = " ANSI_CYAN "1" ANSI_NORMAL "; " ANSI_FAINT "«2 attributes elided»" ANSI_NORMAL "}", PrintOptions { .ansiColors = true, .maxAttrs = 1 @@ -793,7 +793,7 @@ TEST_F(ValuePrintingTests, ansiColorsListElided) vList.bigList.size = 2; test(vList, - "[ " ANSI_CYAN "1" ANSI_NORMAL " " ANSI_FAINT " «1 item elided»" ANSI_NORMAL "]", + "[ " ANSI_CYAN "1" ANSI_NORMAL " " ANSI_FAINT "«1 item elided»" ANSI_NORMAL "]", PrintOptions { .ansiColors = true, .maxListItems = 1 @@ -806,7 +806,7 @@ TEST_F(ValuePrintingTests, ansiColorsListElided) vList.bigList.size = 3; test(vList, - "[ " ANSI_CYAN "1" ANSI_NORMAL " " ANSI_FAINT " «2 items elided»" ANSI_NORMAL "]", + "[ " ANSI_CYAN "1" ANSI_NORMAL " " ANSI_FAINT "«2 items elided»" ANSI_NORMAL "]", PrintOptions { .ansiColors = true, .maxListItems = 1