From da332d678e7ad5b544eb6283d3c8c8da9e38ba4b Mon Sep 17 00:00:00 2001 From: Ryan Hendrickson Date: Fri, 20 Sep 2024 15:57:36 -0400 Subject: [PATCH] libexpr: deprecate the bogus "or"-as-variable As a prelude to making "or" work like a normal variable, emit a warning any time the "fn or" production is used in a context that will change how it is parsed when that production is refactored. In detail: in the future, OR_KW will be moved to expr_simple, and the cursed ExprCall production that is currently part of the expr_select nonterminal will be generated "normally" in expr_app instead. Any productions that accept an expr_select will be affected, except for the expr_app nonterminal itself (because, while expr_app has a production accepting a bare expr_select, its other production will continue to accept "fn or" expressions). So all we need to do is emit an appropriate warning when an expr_simple representing a cursed ExprCall is accepted in one of those productions without first going through expr_app. As the warning message describes, users can suppress the warning by wrapping their problematic "fn or" expressions in parentheses. For example, "f g or" can be made future-proof by rewriting it as "f (g or)"; similarly "[ x y or ]" can be rewritten as "[ x (y or) ]", etc. The parentheses preserve the current grouping behavior, as in the future "f g or" will be parsed as "(f g) or", just like "f g anything-else" is grouped. (Mechanically, this suppresses the warning because the problem ExprCalls go through the "expr_app : expr_select" production, which resets the cursed status on the ExprCall.) --- src/libexpr/nixexpr.cc | 28 +++++++++++++++++++ src/libexpr/nixexpr.hh | 12 +++++++- src/libexpr/parser.y | 23 ++++++++++----- .../eval-okay-deprecate-cursed-or.err.exp | 12 ++++++++ .../lang/eval-okay-deprecate-cursed-or.exp | 1 + .../lang/eval-okay-deprecate-cursed-or.nix | 11 ++++++++ 6 files changed, 79 insertions(+), 8 deletions(-) create mode 100644 tests/functional/lang/eval-okay-deprecate-cursed-or.err.exp create mode 100644 tests/functional/lang/eval-okay-deprecate-cursed-or.exp create mode 100644 tests/functional/lang/eval-okay-deprecate-cursed-or.nix diff --git a/src/libexpr/nixexpr.cc b/src/libexpr/nixexpr.cc index dbc74faf9..063ff0753 100644 --- a/src/libexpr/nixexpr.cc +++ b/src/libexpr/nixexpr.cc @@ -663,4 +663,32 @@ std::string DocComment::getInnerText(const PosTable & positions) const { return docStr; } + + +/* ‘Cursed or’ handling. + * + * In parser.y, every use of expr_select in a production must call one of the + * two below functions. + * + * To be removed by https://github.com/NixOS/nix/pull/11121 + */ + +void ExprCall::resetCursedOr() +{ + cursedOrEndPos.reset(); +} + +void ExprCall::warnIfCursedOr(const SymbolTable & symbols, const PosTable & positions) +{ + if (cursedOrEndPos.has_value()) { + std::ostringstream out; + out << "at " << positions[pos] << ": " + "This expression uses `or` as an identifier in a way that will change in a future Nix release.\n" + "Wrap this entire expression in parentheses to preserve its current meaning:\n" + " (" << positions[pos].getSnippetUpTo(positions[*cursedOrEndPos]).value_or("could not read expression") << ")\n" + "Give feedback at https://github.com/NixOS/nix/pull/11121"; + warn(out.str()); + } +} + } diff --git a/src/libexpr/nixexpr.hh b/src/libexpr/nixexpr.hh index 7868834f1..bdf4e214a 100644 --- a/src/libexpr/nixexpr.hh +++ b/src/libexpr/nixexpr.hh @@ -96,6 +96,10 @@ struct Expr virtual void setName(Symbol name); virtual void setDocComment(DocComment docComment) { }; virtual PosIdx getPos() const { return noPos; } + + // These are temporary methods to be used only in parser.y + virtual void resetCursedOr() { }; + virtual void warnIfCursedOr(const SymbolTable & symbols, const PosTable & positions) { }; }; #define COMMON_METHODS \ @@ -354,10 +358,16 @@ struct ExprCall : Expr Expr * fun; std::vector args; PosIdx pos; + std::optional cursedOrEndPos; // used during parsing to warn about https://github.com/NixOS/nix/issues/11118 ExprCall(const PosIdx & pos, Expr * fun, std::vector && args) - : fun(fun), args(args), pos(pos) + : fun(fun), args(args), pos(pos), cursedOrEndPos({}) + { } + ExprCall(const PosIdx & pos, Expr * fun, std::vector && args, PosIdx && cursedOrEndPos) + : fun(fun), args(args), pos(pos), cursedOrEndPos(cursedOrEndPos) { } PosIdx getPos() const override { return pos; } + virtual void resetCursedOr() override; + virtual void warnIfCursedOr(const SymbolTable & symbols, const PosTable & positions) override; COMMON_METHODS }; diff --git a/src/libexpr/parser.y b/src/libexpr/parser.y index a79abbf16..944c7b1af 100644 --- a/src/libexpr/parser.y +++ b/src/libexpr/parser.y @@ -264,19 +264,28 @@ expr_op ; expr_app - : expr_app expr_select { $$ = makeCall(CUR_POS, $1, $2); } - | expr_select + : expr_app expr_select { $$ = makeCall(CUR_POS, $1, $2); $2->warnIfCursedOr(state->symbols, state->positions); } + | /* Once a ‘cursed or’ reaches this nonterminal, it is no longer cursed, + because the uncursed parse would also produce an expr_app. But we need + to remove the cursed status in order to prevent valid things like + `f (g or)` from triggering the warning. */ + expr_select { $$ = $1; $$->resetCursedOr(); } ; expr_select : expr_simple '.' attrpath { $$ = new ExprSelect(CUR_POS, $1, std::move(*$3), nullptr); delete $3; } | expr_simple '.' attrpath OR_KW expr_select - { $$ = new ExprSelect(CUR_POS, $1, std::move(*$3), $5); delete $3; } - | /* Backwards compatibility: because Nixpkgs has a rarely used - function named ‘or’, allow stuff like ‘map or [...]’. */ + { $$ = new ExprSelect(CUR_POS, $1, std::move(*$3), $5); delete $3; $5->warnIfCursedOr(state->symbols, state->positions); } + | /* Backwards compatibility: because Nixpkgs has a function named ‘or’, + allow stuff like ‘map or [...]’. This production is problematic (see + https://github.com/NixOS/nix/issues/11118) and will be refactored in the + future by treating `or` as a regular identifier. The refactor will (in + very rare cases, we think) change the meaning of expressions, so we mark + the ExprCall with data (establishing that it is a ‘cursed or’) that can + be used to emit a warning when an affected expression is parsed. */ expr_simple OR_KW - { $$ = new ExprCall(CUR_POS, $1, {new ExprVar(CUR_POS, state->s.or_)}); } + { $$ = new ExprCall(CUR_POS, $1, {new ExprVar(CUR_POS, state->s.or_)}, state->positions.add(state->origin, @$.endOffset)); } | expr_simple ; @@ -472,7 +481,7 @@ string_attr ; expr_list - : expr_list expr_select { $$ = $1; $1->elems.push_back($2); /* !!! dangerous */ } + : expr_list expr_select { $$ = $1; $1->elems.push_back($2); /* !!! dangerous */; $2->warnIfCursedOr(state->symbols, state->positions); } | { $$ = new ExprList; } ; diff --git a/tests/functional/lang/eval-okay-deprecate-cursed-or.err.exp b/tests/functional/lang/eval-okay-deprecate-cursed-or.err.exp new file mode 100644 index 000000000..4a656827a --- /dev/null +++ b/tests/functional/lang/eval-okay-deprecate-cursed-or.err.exp @@ -0,0 +1,12 @@ +warning: at /pwd/lang/eval-okay-deprecate-cursed-or.nix:3:47: This expression uses `or` as an identifier in a way that will change in a future Nix release. +Wrap this entire expression in parentheses to preserve its current meaning: + ((x: x) or) +Give feedback at https://github.com/NixOS/nix/pull/11121 +warning: at /pwd/lang/eval-okay-deprecate-cursed-or.nix:4:39: This expression uses `or` as an identifier in a way that will change in a future Nix release. +Wrap this entire expression in parentheses to preserve its current meaning: + ((x: x + 1) or) +Give feedback at https://github.com/NixOS/nix/pull/11121 +warning: at /pwd/lang/eval-okay-deprecate-cursed-or.nix:5:44: This expression uses `or` as an identifier in a way that will change in a future Nix release. +Wrap this entire expression in parentheses to preserve its current meaning: + ((x: x) or) +Give feedback at https://github.com/NixOS/nix/pull/11121 diff --git a/tests/functional/lang/eval-okay-deprecate-cursed-or.exp b/tests/functional/lang/eval-okay-deprecate-cursed-or.exp new file mode 100644 index 000000000..573541ac9 --- /dev/null +++ b/tests/functional/lang/eval-okay-deprecate-cursed-or.exp @@ -0,0 +1 @@ +0 diff --git a/tests/functional/lang/eval-okay-deprecate-cursed-or.nix b/tests/functional/lang/eval-okay-deprecate-cursed-or.nix new file mode 100644 index 000000000..a4f9e747f --- /dev/null +++ b/tests/functional/lang/eval-okay-deprecate-cursed-or.nix @@ -0,0 +1,11 @@ +let + # These are cursed and should warn + cursed0 = builtins.length (let or = 1; in [ (x: x) or ]); + cursed1 = let or = 1; in (x: x * 2) (x: x + 1) or; + cursed2 = let or = 1; in { a = 2; }.a or (x: x) or; + + # These are uses of `or` as an identifier that are not cursed + allowed0 = let or = (x: x); in map or []; + allowed1 = let f = (x: x); or = f; in f (f or); +in +0