From faf00ad022402e602140cd8e4a339dba504f2652 Mon Sep 17 00:00:00 2001 From: Jade Lovelace Date: Thu, 5 Dec 2024 14:34:38 -0800 Subject: [PATCH] libexpr: significantly improve error messages for bad attr paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit makes Lix include the summarized content of the value being indexed when it is bad. lix/lix2 » nix eval --expr '{x.y = 2;}' 'x.y.z' error: the value being indexed in the selection path 'x.y.z' at 'x.y' should be a set but is an integer: 2 lix/lix2 » nix eval --expr '{x.y = { a = 3; };}' 'x.y.z' error: attribute 'z' in selection path 'x.y.z' not found inside path 'x.y', whose contents are: { a = 3; } Did you mean a? lix/lix2 » nix eval --expr '{x.y = { a = 3; };}' 'x.y.1' error: the expression selected by the selection path 'x.y.1' should be a list but is a set: { a = 3; } Change-Id: I3202aba0e437e00b4c6d3ee287a2d9a7c6892dbf --- doc/manual/rl-next/better-errors-attrpaths.md | 25 ++++++++ lix/libexpr/attr-path.cc | 60 +++++++++++++------ tests/functional/misc.sh | 7 --- tests/functional2/eval/test_attr_paths.py | 56 +++++++++++++++++ .../{ => eval}/test_eval_trivial.py | 2 +- 5 files changed, 125 insertions(+), 25 deletions(-) create mode 100644 doc/manual/rl-next/better-errors-attrpaths.md create mode 100644 tests/functional2/eval/test_attr_paths.py rename tests/functional2/{ => eval}/test_eval_trivial.py (63%) diff --git a/doc/manual/rl-next/better-errors-attrpaths.md b/doc/manual/rl-next/better-errors-attrpaths.md new file mode 100644 index 000000000..4a087551f --- /dev/null +++ b/doc/manual/rl-next/better-errors-attrpaths.md @@ -0,0 +1,25 @@ +--- +synopsis: "Improved error messages for bad attr paths" +category: Improvements +cls: [2277, 2280] +credits: jade +--- + +Lix now includes much more detail when a bad attribute path is accessed at the command line: + +``` + » nix eval -f '' lixVersions.lix_2_92 +error: attribute 'lix_2_92' in selection path 'lixVersions.lix_2_92' not found + Did you mean one of lix_2_90 or lix_2_91? +``` + +After: + +``` + » nix eval --impure -f '' lixVersions.lix_2_92 +error: attribute 'lix_2_92' in selection path 'lixVersions.lix_2_92' not found inside path 'lixVersions', whose contents are: { __unfix__ = «lambda @ /nix/store/hfz1qqd0z8amlgn8qwich1dvkmldik36-source/lib/fixed-points.nix: +447:7»; buildLix = «thunk»; extend = «thunk»; latest = «thunk»; lix_2_90 = «thunk»; lix_2_91 = «thunk»; override = «thunk»; overrideDerivation = «thunk»; recurseForDerivations = true; stable = «thunk»; } + Did you mean one of lix_2_90 or lix_2_91? +``` + +This should avoid some unnecessary trips to the repl or to the debugger by giving some information about the value being selected on that was unexpected. diff --git a/lix/libexpr/attr-path.cc b/lix/libexpr/attr-path.cc index 62ebf0193..306df805a 100644 --- a/lix/libexpr/attr-path.cc +++ b/lix/libexpr/attr-path.cc @@ -1,5 +1,6 @@ #include "lix/libexpr/attr-path.hh" #include "lix/libexpr/eval-inline.hh" +#include "print-options.hh" #include #include @@ -96,12 +97,18 @@ std::pair findAlongAttrPath(EvalState & state, const std::strin throw Error("empty attribute name in selection path '%1%'", attrPath); if (v->type() != nAttrs) { - auto pathPart = std::vector(tokens.begin(), tokens.begin() + attrPathIdx); - state.ctx.errors.make( - "the value being indexed in the selection path '%1%' at '%2%' should be a set but is %3%", - attrPath, - unparseAttrPath(pathPart), - showType(*v)).debugThrow(); + auto pathPart = + std::vector(tokens.begin(), tokens.begin() + attrPathIdx); + state.ctx.errors + .make( + "the value being indexed in the selection path '%1%' at '%2%' should be a " + "set but is %3%: %4%", + attrPath, + unparseAttrPath(pathPart), + showType(*v), + ValuePrinter(state, *v, errorPrintOptions) + ) + .debugThrow(); } Bindings::iterator a = v->attrs->find(state.ctx.symbols.create(attr)); @@ -111,21 +118,40 @@ std::pair findAlongAttrPath(EvalState & state, const std::strin attrNames.insert(state.ctx.symbols[attr.name]); auto suggestions = Suggestions::bestMatches(attrNames, attr); - throw AttrPathNotFound(suggestions, "attribute '%1%' in selection path '%2%' not found", attr, attrPath); + auto pathPart = + std::vector(tokens.begin(), tokens.begin() + attrPathIdx); + throw AttrPathNotFound( + suggestions, + "attribute '%1%' in selection path '%2%' not found inside path '%3%', whose " + "contents are: %4%", + attr, + attrPath, + unparseAttrPath(pathPart), + ValuePrinter(state, *v, errorPrintOptions) + ); } v = &*a->value; pos = a->pos; - } - - else { - - if (!v->isList()) - state.ctx.errors.make( - "the expression selected by the selection path '%1%' should be a list but is %2%", + } else { + if (!v->isList()) { + state.ctx.errors + .make( + "the expression selected by the selection path '%1%' should be a list but " + "is %2%: %3%", + attrPath, + showType(*v), + ValuePrinter(state, *v, errorPrintOptions) + ) + .debugThrow(); + } + if (*attrIndex >= v->listSize()) { + throw AttrPathNotFound( + "list index %1% in selection path '%2%' is out of range for list %3%", + *attrIndex, attrPath, - showType(*v)).debugThrow(); - if (*attrIndex >= v->listSize()) - throw AttrPathNotFound("list index %1% in selection path '%2%' is out of range", *attrIndex, attrPath); + ValuePrinter(state, *v, errorPrintOptions) + ); + } v = v->listElems()[*attrIndex]; pos = noPos; diff --git a/tests/functional/misc.sh b/tests/functional/misc.sh index d4379b7ce..b142fdc93 100644 --- a/tests/functional/misc.sh +++ b/tests/functional/misc.sh @@ -24,13 +24,6 @@ eval_stdin_res=$(echo 'let a = {} // a; in a.foo' | nix-instantiate --eval -E - echo $eval_stdin_res | grep "at «stdin»:1:15:" echo $eval_stdin_res | grep "infinite recursion encountered" -# Attribute path errors -expectStderr 1 nix-instantiate --eval -E '{}' -A '"x' | grepQuiet "missing closing quote in selection path" -expectStderr 1 nix-instantiate --eval -E '[]' -A 'x' | grepQuiet "should be a set" -expectStderr 1 nix-instantiate --eval -E '{}' -A '1' | grepQuiet "should be a list" -expectStderr 1 nix-instantiate --eval -E '{}' -A '.' | grepQuiet "empty attribute name" -expectStderr 1 nix-instantiate --eval -E '[]' -A '1' | grepQuiet "out of range" - # Unknown setting warning # NOTE(cole-h): behavior is different depending on the order, which is why we test an unknown option # before and after the `'{}'`! diff --git a/tests/functional2/eval/test_attr_paths.py b/tests/functional2/eval/test_attr_paths.py new file mode 100644 index 000000000..5c7874958 --- /dev/null +++ b/tests/functional2/eval/test_attr_paths.py @@ -0,0 +1,56 @@ +from functional2.testlib.fixtures import Nix +import pytest +from typing import NamedTuple +from textwrap import dedent + + +class ShouldError(NamedTuple): + expr: str + attr: str + error: str + + +ERR_CASES: list[ShouldError] = [ + # FIXME(jade): expect-test system for pytest that allows for updating these easily + ShouldError('{}', '"x', + """error: missing closing quote in selection path '"x'"""), + ShouldError( + '[]', 'x', + """error: the value being indexed in the selection path 'x' at '' should be a set but is a list: [ ]""" + ), + ShouldError( + '{}', '1', + """error: the expression selected by the selection path '1' should be a list but is a set: { }""" + ), + ShouldError('{}', '.', + """error: empty attribute name in selection path '.'"""), + ShouldError('{ x."" = 2; }', 'x.""', + """error: empty attribute name in selection path 'x.""'"""), + ShouldError('{ x."".y = 2; }', 'x."".y', + """error: empty attribute name in selection path 'x."".y'"""), + ShouldError( + '[]', '1', + """error: list index 1 in selection path '1' is out of range for list [ ]""" + ), + ShouldError( + '{ x.y = { z = 2; a = 3; }; }', 'x.y.c', + dedent("""\ + error: attribute 'c' in selection path 'x.y.c' not found inside path 'x.y', whose contents are: { a = 3; z = 2; } + Did you mean one of a or z?""") + ), +] + + +# I do not know why it does this, but I guess it makes sense as allowing a tool +# to pass -A unconditionally and then allow a blank attribute to mean the whole +# thing +def test_attrpath_accepts_empty_attr_as_no_attr(nix: Nix): + assert nix.nix_instantiate(['--eval', '--expr', '{}', '-A', + '']).run().ok().stdout_plain == '{ }' + + +@pytest.mark.parametrize(['expr', 'attr', 'error'], ERR_CASES) +def test_attrpath_error(nix: Nix, expr: str, attr: str, error: str): + res = nix.nix_instantiate(['--eval', '--expr', expr, '-A', attr]).run() + + assert res.expect(1).stderr_plain == error diff --git a/tests/functional2/test_eval_trivial.py b/tests/functional2/eval/test_eval_trivial.py similarity index 63% rename from tests/functional2/test_eval_trivial.py rename to tests/functional2/eval/test_eval_trivial.py index 8bde9d22c..0e43ea5fd 100644 --- a/tests/functional2/test_eval_trivial.py +++ b/tests/functional2/eval/test_eval_trivial.py @@ -1,4 +1,4 @@ -from .testlib.fixtures import Nix +from functional2.testlib.fixtures import Nix def test_trivial_addition(nix: Nix): assert nix.eval('1 + 1').json() == 2