From f6c4034e2f4412db4e8f72e89402f084d48d6ab4 Mon Sep 17 00:00:00 2001 From: Zitrone Date: Thu, 6 Feb 2025 14:54:36 +0000 Subject: [PATCH] libexpr: show all missing and unexpected arguments instead of just one also only give suggestions for unused arguments Change-Id: Iae8e72defbbe2571a803dfd7216745b39848ccb2 --- doc/manual/change-authors.yml | 4 + .../improve-error-messages-for-lambdas.md | 29 +++++++ lix/libexpr/eval.cc | 85 ++++++++++++------- lix/libutil/english.hh | 28 ++++++ ...l-fail-missing-and-unexpected-args.err.exp | 13 +++ .../eval-fail-missing-and-unexpected-args.nix | 1 + .../lang/eval-fail-missing-args.err.exp | 12 +++ .../lang/eval-fail-missing-args.nix | 1 + .../lang/eval-fail-undeclared-arg.err.exp | 1 - .../lang/eval-fail-undeclared-args.err.exp | 13 +++ .../lang/eval-fail-undeclared-args.nix | 1 + tests/functional/suggestions.sh | 4 + 12 files changed, 158 insertions(+), 34 deletions(-) create mode 100644 doc/manual/rl-next/improve-error-messages-for-lambdas.md create mode 100644 tests/functional/lang/eval-fail-missing-and-unexpected-args.err.exp create mode 100644 tests/functional/lang/eval-fail-missing-and-unexpected-args.nix create mode 100644 tests/functional/lang/eval-fail-missing-args.err.exp create mode 100644 tests/functional/lang/eval-fail-missing-args.nix create mode 100644 tests/functional/lang/eval-fail-undeclared-args.err.exp create mode 100644 tests/functional/lang/eval-fail-undeclared-args.nix diff --git a/doc/manual/change-authors.yml b/doc/manual/change-authors.yml index 91a4eb898..bf2e96572 100644 --- a/doc/manual/change-authors.yml +++ b/doc/manual/change-authors.yml @@ -149,6 +149,10 @@ puck: forgejo: puck github: puckipedia +quantenzitrone: + display_name: Zitrone + forgejo: quantenzitrone + quantumjump: display_name: Quantum Jump github: QuantumBJump diff --git a/doc/manual/rl-next/improve-error-messages-for-lambdas.md b/doc/manual/rl-next/improve-error-messages-for-lambdas.md new file mode 100644 index 000000000..846d2deec --- /dev/null +++ b/doc/manual/rl-next/improve-error-messages-for-lambdas.md @@ -0,0 +1,29 @@ +--- +synopsis: 'Show all missing and unexpected arguments in errorneous function calls' +issues: [] +cls: [2477] +category: Improvements +credits: [quantenzitrone] +--- + +When calling a function that expects an attribute set, lix will now show all +missing and unexpected arguments. +e.g. with `({ a, b, c } : a + b + c) { a = 1; d = 1; }` lix will now show the error: +``` +[...] +error: function 'anonymous lambda' called without required arguments 'b' and 'c' and with unexpected argument 'd' +[...] +``` +Previously lix would just show `b`. +Furthermore lix will now only suggest arguments that aren't yet used. +e.g. with `({ a?1, b?1, c?1 } : a + b + c) { a = 1; d = 1; e = 1; }` lix will now show the error: +``` +[...] +error: function 'anonymous lambda' called with unexpected arguments 'd' and 'e' + at «string»:1:2: + 1| ({ a?1, b?1, c?1 } : a + b + c) { a = 1; d = 1; e = 1; } + | ^ + Did you mean one of b or c? +``` +Previously lix would also suggest `a`. +Suggestions are unfortunately still currently just for the first missing argument. diff --git a/lix/libexpr/eval.cc b/lix/libexpr/eval.cc index e11ac712a..62fb772b9 100644 --- a/lix/libexpr/eval.cc +++ b/lix/libexpr/eval.cc @@ -1,8 +1,11 @@ #include "lix/libexpr/eval.hh" #include "lix/libexpr/eval-settings.hh" #include "lix/libutil/archive.hh" +#include "lix/libutil/ansicolor.hh" #include "lix/libutil/async.hh" #include "lix/libutil/error.hh" +#include "lix/libutil/english.hh" +#include "lix/libutil/fmt.hh" #include "lix/libutil/hash.hh" #include "lix/libexpr/primops.hh" #include "lix/libexpr/print-options.hh" @@ -23,12 +26,15 @@ #include "lix/libfetchers/fetch-to-store.hh" #include "lix/libexpr/flake/flakeref.hh" #include "lix/libutil/exit.hh" +#include "symbol-table.hh" #include #include +#include #include #include #include +#include #include #include #include @@ -1428,23 +1434,22 @@ public: }; }; -/** Currently these each just take one, but maybe in the future we could have diagnostics - * for all unexpected and missing arguments? - */ struct FormalsMatch { - std::vector missing; - std::vector unexpected; + std::vector missing; + std::vector unexpected; + std::set unused; }; /** Matchup an attribute argument set to a lambda's formal arguments, * or return what arguments were required but not given, or given but not allowed. - * (currently returns only one, for each). */ -FormalsMatch matchupLambdaAttrs(EvalState & state, Env & env, Displacement & displ, AttrsPattern const & pattern, Bindings & attrs) +FormalsMatch matchupLambdaAttrs(EvalState & state, Env & env, Displacement & displ, AttrsPattern const & pattern, Bindings & attrs, SymbolTable & symbols) { size_t attrsUsed = 0; + FormalsMatch result; + for (auto const & formal : pattern.formals) { // The attribute whose name matches the name of the formal we're matching up, if it exists. @@ -1459,15 +1464,14 @@ FormalsMatch matchupLambdaAttrs(EvalState & state, Env & env, Displacement & dis } // The argument for this formal wasn't given. + result.unused.insert(symbols[formal.name]); // If the formal has a default, use it. if (formal.def) { env.values[displ] = formal.def->maybeThunk(state, env); displ += 1; } else { // Otherwise, let our caller know what was missing. - return FormalsMatch{ - .missing = {formal.name}, - }; + result.missing.push_back(symbols[formal.name]); } } @@ -1476,16 +1480,12 @@ FormalsMatch matchupLambdaAttrs(EvalState & state, Env & env, Displacement & dis // Return the first unexpected argument. for (Attr const & attr : attrs) { if (!pattern.has(attr.name)) { - return FormalsMatch{ - .unexpected = {attr.name}, - }; + result.unexpected.push_back(symbols[attr.name]); } } - - abort(); // unreachable. } - return FormalsMatch{}; + return result; } Env & SimplePattern::match(ExprLambda & lambda, EvalState & state, Env & up, Value * arg, const PosIdx pos) @@ -1522,25 +1522,44 @@ Env & AttrsPattern::match(ExprLambda & lambda, EvalState & state, Env & up, Valu env2, displ, *this, - *arg->attrs + *arg->attrs, + ctx.symbols ); - for (auto const & missingArg : formalsMatch.missing) { - auto const missing = ctx.symbols[missingArg]; - ctx.errors.make("function '%s' called without required argument '%s'", lambda.getName(ctx.symbols), missing) - .atPos(lambda.pos) - .withTrace(pos, "from call site") - .withFrame(up, lambda) - .debugThrow(); - } - for (auto const & unexpectedArg : formalsMatch.unexpected) { - auto const unex = ctx.symbols[unexpectedArg]; - std::set formalNames; - for (auto const & formal : formals) { - formalNames.insert(ctx.symbols[formal.name]); + + + if (!formalsMatch.unexpected.empty() || !formalsMatch.missing.empty()) { + Suggestions sug; // empty suggestions -> no suggestions + if (!formalsMatch.unexpected.empty()) { + // suggestions only for the first unexpected argument + // TODO: suggestions for all unexpected arguments + sug = Suggestions::bestMatches(formalsMatch.unused, formalsMatch.unexpected.front()); } - auto sug = Suggestions::bestMatches(formalNames, unex); - ctx.errors.make("function '%s' called with unexpected argument '%s'", lambda.getName(ctx.symbols), unex) - .atPos(lambda.pos) + + auto argFmt = [](const SymbolStr & argument) { return HintFmt("'%s'", argument); }; + + [&](){ + if (formalsMatch.unexpected.empty() && !formalsMatch.missing.empty()) { + return ctx.errors.make( + "function '%s' called without required argument%s %s", lambda.getName(ctx.symbols), + Uncolored((formalsMatch.missing.size() == 1) ? "" : "s"), + Uncolored(concatStringsCommaAnd(argFmt, formalsMatch.missing)) + ); + } else if (!formalsMatch.unexpected.empty() && formalsMatch.missing.empty()) { + return ctx.errors.make( + "function '%s' called with unexpected argument%s %s", lambda.getName(ctx.symbols), + Uncolored((formalsMatch.unexpected.size() == 1) ? "" : "s"), + Uncolored(concatStringsCommaAnd(argFmt, formalsMatch.unexpected)) + ); + } else { + return ctx.errors.make( + "function '%s' called without required argument%s %s and with unexpected argument%s %s", lambda.getName(ctx.symbols), + Uncolored((formalsMatch.missing.size() == 1) ? "" : "s"), + Uncolored(concatStringsCommaAnd(argFmt, formalsMatch.missing)), + Uncolored((formalsMatch.unexpected.size() == 1) ? "" : "s"), + Uncolored(concatStringsCommaAnd(argFmt, formalsMatch.unexpected)) + ); + } + }().atPos(lambda.pos) .withTrace(pos, "from call site") .withSuggestions(sug) .withFrame(up, lambda) diff --git a/lix/libutil/english.hh b/lix/libutil/english.hh index d9289c663..56010766c 100644 --- a/lix/libutil/english.hh +++ b/lix/libutil/english.hh @@ -2,6 +2,10 @@ ///@file #include +#include +#include +#include +#include namespace nix { @@ -16,4 +20,28 @@ std::ostream & pluralize( const std::string_view single, const std::string_view plural); + +/** Concatenates a given iterator of strings with commas and 'and', + * while transforming them with the given function. + * e.g. ["foo", "bar", "baz"] might get concatenated to "foobar, barbar and bazbar". + * the lambda for this might look like this: + * `[](std::string & arg) { return arg + "bar"; };` + */ +template +std::string concatStringsCommaAnd(F transform, const R & args) +{ + std::stringstream result; + if (args.size() != 0) { + result << transform(*args.begin()); + } + if (args.size() >= 2) { + // will not do anything for size <= 2 + std::for_each(std::next(args.begin()), std::prev(args.end()), [&](auto const & arg) { + result << ", " << transform(arg); + }); + result << " and " << transform(*args.rbegin()); + } + return result.str(); +}; + } diff --git a/tests/functional/lang/eval-fail-missing-and-unexpected-args.err.exp b/tests/functional/lang/eval-fail-missing-and-unexpected-args.err.exp new file mode 100644 index 000000000..e08e3f469 --- /dev/null +++ b/tests/functional/lang/eval-fail-missing-and-unexpected-args.err.exp @@ -0,0 +1,13 @@ +error: + … from call site + at /pwd/lang/eval-fail-missing-and-unexpected-args.nix:1:1: + 1| ({a, b, c}: a + b + c) {c = 1; d = 1; e = 1; f = 1;} + | ^ + 2| + + error: function 'anonymous lambda' called without required arguments 'a' and 'b' and with unexpected arguments 'd', 'e' and 'f' + at /pwd/lang/eval-fail-missing-and-unexpected-args.nix:1:2: + 1| ({a, b, c}: a + b + c) {c = 1; d = 1; e = 1; f = 1;} + | ^ + 2| + Did you mean one of a or b? diff --git a/tests/functional/lang/eval-fail-missing-and-unexpected-args.nix b/tests/functional/lang/eval-fail-missing-and-unexpected-args.nix new file mode 100644 index 000000000..c456c3a99 --- /dev/null +++ b/tests/functional/lang/eval-fail-missing-and-unexpected-args.nix @@ -0,0 +1 @@ +({a, b, c}: a + b + c) {c = 1; d = 1; e = 1; f = 1;} diff --git a/tests/functional/lang/eval-fail-missing-args.err.exp b/tests/functional/lang/eval-fail-missing-args.err.exp new file mode 100644 index 000000000..a1a65a68b --- /dev/null +++ b/tests/functional/lang/eval-fail-missing-args.err.exp @@ -0,0 +1,12 @@ +error: + … from call site + at /pwd/lang/eval-fail-missing-args.nix:1:1: + 1| ({a, b, c, d}: a + b + c + d) { } + | ^ + 2| + + error: function 'anonymous lambda' called without required arguments 'a', 'b', 'c' and 'd' + at /pwd/lang/eval-fail-missing-args.nix:1:2: + 1| ({a, b, c, d}: a + b + c + d) { } + | ^ + 2| diff --git a/tests/functional/lang/eval-fail-missing-args.nix b/tests/functional/lang/eval-fail-missing-args.nix new file mode 100644 index 000000000..1099312b6 --- /dev/null +++ b/tests/functional/lang/eval-fail-missing-args.nix @@ -0,0 +1 @@ +({a, b, c, d}: a + b + c + d) { } diff --git a/tests/functional/lang/eval-fail-undeclared-arg.err.exp b/tests/functional/lang/eval-fail-undeclared-arg.err.exp index 6e13a138e..3a43c6967 100644 --- a/tests/functional/lang/eval-fail-undeclared-arg.err.exp +++ b/tests/functional/lang/eval-fail-undeclared-arg.err.exp @@ -10,4 +10,3 @@ error: 1| ({x, z}: x + z) {x = "foo"; y = "bla"; z = "bar";} | ^ 2| - Did you mean one of x or z? diff --git a/tests/functional/lang/eval-fail-undeclared-args.err.exp b/tests/functional/lang/eval-fail-undeclared-args.err.exp new file mode 100644 index 000000000..8dc09bbd7 --- /dev/null +++ b/tests/functional/lang/eval-fail-undeclared-args.err.exp @@ -0,0 +1,13 @@ +error: + … from call site + at /pwd/lang/eval-fail-undeclared-args.nix:1:1: + 1| ({a ? "foo", b ? "bar"}: a + b) {c = "meow"; d = "meow";} + | ^ + 2| + + error: function 'anonymous lambda' called with unexpected arguments 'c' and 'd' + at /pwd/lang/eval-fail-undeclared-args.nix:1:2: + 1| ({a ? "foo", b ? "bar"}: a + b) {c = "meow"; d = "meow";} + | ^ + 2| + Did you mean one of a or b? diff --git a/tests/functional/lang/eval-fail-undeclared-args.nix b/tests/functional/lang/eval-fail-undeclared-args.nix new file mode 100644 index 000000000..8affb63e3 --- /dev/null +++ b/tests/functional/lang/eval-fail-undeclared-args.nix @@ -0,0 +1 @@ +({a ? "foo", b ? "bar"}: a + b) {c = "meow"; d = "meow";} diff --git a/tests/functional/suggestions.sh b/tests/functional/suggestions.sh index f18fefef9..480e92f81 100644 --- a/tests/functional/suggestions.sh +++ b/tests/functional/suggestions.sh @@ -40,5 +40,9 @@ NIX_EVAL_STDERR_WITH_SUGGESTIONS=$(! nix build --impure --expr '(builtins.getFla fail "The evaluator should suggest the three closest possiblities" NIX_EVAL_STDERR_WITH_SUGGESTIONS=$(! nix build --impure --expr '({ foo }: foo) { foo = 1; fob = 2; }' 2>&1 1>/dev/null) +[[ ! "$NIX_EVAL_STDERR_WITH_SUGGESTIONS" =~ "Did you mean" ]] || \ + fail "The evaluator shouldn't suggest anything if all arguments are already provided." + +NIX_EVAL_STDERR_WITH_SUGGESTIONS=$(! nix build --impure --expr '({ foo ? 1 }: foo) { fob = 2; }' 2>&1 1>/dev/null) [[ "$NIX_EVAL_STDERR_WITH_SUGGESTIONS" =~ "Did you mean foo?" ]] || \ fail "The evaluator should suggest the three closest possiblities"