From 1edd6fada53553b89847ac3981ac28025857ca02 Mon Sep 17 00:00:00 2001 From: pennae Date: Mon, 29 Jan 2024 06:19:23 +0100 Subject: [PATCH] report inherit attr errors at the duplicate name previously we reported the error at the beginning of the binding block (for plain inherits) or the beginning of the attr list (for inherit-from), effectively hiding where exactly the error happened. this also carries over to runtime positions of attributes in sets as reported by unsafeGetAttrPos. we're not worried about this changing observable eval behavior because it *is* marked unsafe, and the new behavior is much more useful. --- doc/manual/rl-next/inherit-error-positions.md | 6 +++++ src/libexpr/parser.y | 25 ++++++++++--------- .../lang/eval-okay-inherit-attr-pos.exp | 1 + .../lang/eval-okay-inherit-attr-pos.nix | 12 +++++++++ .../lang/parse-fail-dup-attrs-2.err.exp | 4 +-- .../lang/parse-fail-dup-attrs-3.err.exp | 4 +-- .../lang/parse-fail-dup-attrs-7.err.exp | 6 ++--- .../parse-fail-regression-20060610.err.exp | 6 ++--- 8 files changed, 42 insertions(+), 22 deletions(-) create mode 100644 doc/manual/rl-next/inherit-error-positions.md create mode 100644 tests/functional/lang/eval-okay-inherit-attr-pos.exp create mode 100644 tests/functional/lang/eval-okay-inherit-attr-pos.nix diff --git a/doc/manual/rl-next/inherit-error-positions.md b/doc/manual/rl-next/inherit-error-positions.md new file mode 100644 index 000000000..643080e9e --- /dev/null +++ b/doc/manual/rl-next/inherit-error-positions.md @@ -0,0 +1,6 @@ +--- +synopsis: fix duplicate attribute error positions for `inherit` +prs: 9874 +--- + +When an inherit caused a duplicate attribute error the position of the error was not reported correctly, placing the error with the inherit itself or at the start of the bindings block instead of the offending attribute name. diff --git a/src/libexpr/parser.y b/src/libexpr/parser.y index b0aee7b41..9a543d636 100644 --- a/src/libexpr/parser.y +++ b/src/libexpr/parser.y @@ -87,6 +87,7 @@ void yyerror(YYLTYPE * loc, yyscan_t scanner, ParserState * state, const char * nix::StringToken uri; nix::StringToken str; std::vector * attrNames; + std::vector> * inheritAttrs; std::vector> * string_parts; std::vector>> * ind_string_parts; } @@ -97,7 +98,8 @@ void yyerror(YYLTYPE * loc, yyscan_t scanner, ParserState * state, const char * %type binds %type formals %type formal -%type attrs attrpath +%type attrpath +%type attrs %type string_parts_interpolated %type ind_string_parts %type path_start string_parts string_attr @@ -309,13 +311,12 @@ binds : binds attrpath '=' expr ';' { $$ = $1; state->addAttr($$, std::move(*$2), $4, state->at(@2)); delete $2; } | binds INHERIT attrs ';' { $$ = $1; - for (auto & i : *$3) { + for (auto & [i, iPos] : *$3) { if ($$->attrs.find(i.symbol) != $$->attrs.end()) - state->dupAttr(i.symbol, state->at(@3), $$->attrs[i.symbol].pos); - auto pos = state->at(@3); + state->dupAttr(i.symbol, iPos, $$->attrs[i.symbol].pos); $$->attrs.emplace( i.symbol, - ExprAttrs::AttrDef(new ExprVar(CUR_POS, i.symbol), pos, ExprAttrs::AttrDef::Kind::Inherited)); + ExprAttrs::AttrDef(new ExprVar(iPos, i.symbol), iPos, ExprAttrs::AttrDef::Kind::Inherited)); } delete $3; } @@ -325,14 +326,14 @@ binds $$->inheritFromExprs = std::make_unique>(); $$->inheritFromExprs->push_back($4); auto from = new nix::ExprInheritFrom(state->at(@4), $$->inheritFromExprs->size() - 1); - for (auto & i : *$6) { + for (auto & [i, iPos] : *$6) { if ($$->attrs.find(i.symbol) != $$->attrs.end()) - state->dupAttr(i.symbol, state->at(@6), $$->attrs[i.symbol].pos); + state->dupAttr(i.symbol, iPos, $$->attrs[i.symbol].pos); $$->attrs.emplace( i.symbol, ExprAttrs::AttrDef( - new ExprSelect(CUR_POS, from, i.symbol), - state->at(@6), + new ExprSelect(iPos, from, i.symbol), + iPos, ExprAttrs::AttrDef::Kind::InheritedFrom)); } delete $6; @@ -341,12 +342,12 @@ binds ; attrs - : attrs attr { $$ = $1; $1->push_back(AttrName(state->symbols.create($2))); } + : attrs attr { $$ = $1; $1->emplace_back(AttrName(state->symbols.create($2)), state->at(@2)); } | attrs string_attr { $$ = $1; ExprString * str = dynamic_cast($2); if (str) { - $$->push_back(AttrName(state->symbols.create(str->s))); + $$->emplace_back(AttrName(state->symbols.create(str->s)), state->at(@2)); delete str; } else throw ParseError({ @@ -354,7 +355,7 @@ attrs .pos = state->positions[state->at(@2)] }); } - | { $$ = new AttrPath; } + | { $$ = new std::vector>; } ; attrpath diff --git a/tests/functional/lang/eval-okay-inherit-attr-pos.exp b/tests/functional/lang/eval-okay-inherit-attr-pos.exp new file mode 100644 index 000000000..e87d037c6 --- /dev/null +++ b/tests/functional/lang/eval-okay-inherit-attr-pos.exp @@ -0,0 +1 @@ +[ { column = 17; file = "/pwd/lang/eval-okay-inherit-attr-pos.nix"; line = 4; } { column = 19; file = "/pwd/lang/eval-okay-inherit-attr-pos.nix"; line = 4; } { column = 21; file = "/pwd/lang/eval-okay-inherit-attr-pos.nix"; line = 5; } { column = 23; file = "/pwd/lang/eval-okay-inherit-attr-pos.nix"; line = 5; } ] diff --git a/tests/functional/lang/eval-okay-inherit-attr-pos.nix b/tests/functional/lang/eval-okay-inherit-attr-pos.nix new file mode 100644 index 000000000..017ab1d36 --- /dev/null +++ b/tests/functional/lang/eval-okay-inherit-attr-pos.nix @@ -0,0 +1,12 @@ +let + d = 0; + x = 1; + y = { inherit d x; }; + z = { inherit (y) d x; }; +in + [ + (builtins.unsafeGetAttrPos "d" y) + (builtins.unsafeGetAttrPos "x" y) + (builtins.unsafeGetAttrPos "d" z) + (builtins.unsafeGetAttrPos "x" z) + ] diff --git a/tests/functional/lang/parse-fail-dup-attrs-2.err.exp b/tests/functional/lang/parse-fail-dup-attrs-2.err.exp index 4607a5d59..3105e60de 100644 --- a/tests/functional/lang/parse-fail-dup-attrs-2.err.exp +++ b/tests/functional/lang/parse-fail-dup-attrs-2.err.exp @@ -1,6 +1,6 @@ error: attribute 'x' already defined at «stdin»:9:5 - at «stdin»:10:17: + at «stdin»:10:18: 9| x = 789; 10| inherit (as) x; - | ^ + | ^ 11| }; diff --git a/tests/functional/lang/parse-fail-dup-attrs-3.err.exp b/tests/functional/lang/parse-fail-dup-attrs-3.err.exp index 4607a5d59..3105e60de 100644 --- a/tests/functional/lang/parse-fail-dup-attrs-3.err.exp +++ b/tests/functional/lang/parse-fail-dup-attrs-3.err.exp @@ -1,6 +1,6 @@ error: attribute 'x' already defined at «stdin»:9:5 - at «stdin»:10:17: + at «stdin»:10:18: 9| x = 789; 10| inherit (as) x; - | ^ + | ^ 11| }; diff --git a/tests/functional/lang/parse-fail-dup-attrs-7.err.exp b/tests/functional/lang/parse-fail-dup-attrs-7.err.exp index 2daddf380..4e0a48eff 100644 --- a/tests/functional/lang/parse-fail-dup-attrs-7.err.exp +++ b/tests/functional/lang/parse-fail-dup-attrs-7.err.exp @@ -1,6 +1,6 @@ -error: attribute 'x' already defined at «stdin»:6:12 - at «stdin»:7:12: +error: attribute 'x' already defined at «stdin»:6:13 + at «stdin»:7:13: 6| inherit x; 7| inherit x; - | ^ + | ^ 8| }; diff --git a/tests/functional/lang/parse-fail-regression-20060610.err.exp b/tests/functional/lang/parse-fail-regression-20060610.err.exp index d8875a6a5..6ae7c01bf 100644 --- a/tests/functional/lang/parse-fail-regression-20060610.err.exp +++ b/tests/functional/lang/parse-fail-regression-20060610.err.exp @@ -1,6 +1,6 @@ error: undefined variable 'gcc' - at «stdin»:8:12: - 7| + at «stdin»:9:13: 8| body = ({ - | ^ 9| inherit gcc; + | ^ + 10| }).gcc;