Merge pull request #11560 from rhendric/rhendric/deprecate-cursed-or

libexpr: deprecate the bogus "or"-as-variable

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());
+    }
+}
+
 }

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<Expr *> args;
     PosIdx pos;
+    std::optional<PosIdx> cursedOrEndPos; // used during parsing to warn about https://github.com/NixOS/nix/issues/11118
     ExprCall(const PosIdx & pos, Expr * fun, std::vector<Expr *> && args)
-        : fun(fun), args(args), pos(pos)
+        : fun(fun), args(args), pos(pos), cursedOrEndPos({})
+    { }
+    ExprCall(const PosIdx & pos, Expr * fun, std::vector<Expr *> && 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
 };
 

src/libexpr/parser.y

@@ -264,19 +264,28 @@ expr_op
   ;
 
 expr_app
-  : expr_app expr_select { $$ = makeCall(CUR_POS, $1, $2); }
+  : expr_app expr_select { $$ = makeCall(CUR_POS, $1, $2); $2->warnIfCursedOr(state->symbols, state->positions); }
-  | expr_select
+  | /* 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; }
+    { $$ = new ExprSelect(CUR_POS, $1, std::move(*$3), $5); delete $3; $5->warnIfCursedOr(state->symbols, state->positions); }
-  | /* Backwards compatibility: because Nixpkgs has a rarely used
+  | /* Backwards compatibility: because Nixpkgs has a function named ‘or’,
-       function named ‘or’, allow stuff like ‘map 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; }
   ;
 

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

tests/functional/lang/eval-okay-deprecate-cursed-or.exp

@@ -0,0 +1 @@
+0

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