libexpr: make "or"-as-variable less bogus

The previous place where OR_KW was inserted into the grammar to allow
expressions like "map or [...]" led to a number of weird outcomes. By
moving it to expr_simple, expressions using "or" as a variable are now
parsed consistently with the rest of the language. Conflicts are
prevented by telling Bison that OR_KW has higher precedence than '.'.

src/libexpr/nixexpr.cc

@@ -663,32 +663,4 @@ 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,10 +96,6 @@ 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 \
@@ -358,16 +354,10 @@ 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), cursedOrEndPos({})
-    { }
-    ExprCall(const PosIdx & pos, Expr * fun, std::vector<Expr *> && args, PosIdx && cursedOrEndPos)
-        : fun(fun), args(args), pos(pos), cursedOrEndPos(cursedOrEndPos)
+        : fun(fun), args(args), pos(pos)
     { }
     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

@@ -177,6 +177,9 @@ static Expr * makeCall(PosIdx pos, Expr * fn, Expr * arg) {
 %nonassoc '?'
 %nonassoc NEGATE
 
+%precedence '.'
+%precedence OR_KW
+
 %%
 
 start: expr { state->result = $1; };
@@ -264,28 +267,15 @@ expr_op
   ;
 
 expr_app
-  : 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_app expr_select { $$ = makeCall(CUR_POS, $1, $2); }
+  | expr_select
   ;
 
 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; $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_)}, state->positions.add(state->origin, @$.endOffset)); }
+    { $$ = new ExprSelect(CUR_POS, $1, std::move(*$3), $5); delete $3; }
   | expr_simple
   ;
 
@@ -297,6 +287,9 @@ expr_simple
       else
           $$ = new ExprVar(CUR_POS, state->symbols.create($1));
   }
+  | /* Backwards compatibility: because Nixpkgs has a function named ‘or’,
+       allow stuff like ‘map or [...]’. */
+    OR_KW { $$ = new ExprVar(CUR_POS, state->s.or_); }
   | INT_LIT { $$ = new ExprInt($1); }
   | FLOAT_LIT { $$ = new ExprFloat($1); }
   | '"' string_parts '"' { $$ = $2; }
@@ -481,7 +474,7 @@ string_attr
   ;
 
 expr_list
-  : expr_list expr_select { $$ = $1; $1->elems.push_back($2); /* !!! dangerous */; $2->warnIfCursedOr(state->symbols, state->positions); }
+  : expr_list expr_select { $$ = $1; $1->elems.push_back($2); /* !!! dangerous */ }
   | { $$ = new ExprList; }
   ;
 

tests/unit/libexpr/trivial.cc

@@ -244,7 +244,13 @@ namespace nix {
         ASSERT_THAT(*b->value, IsIntEq(1));
     }
 
-    TEST_F(TrivialExpressionTest, orCantBeUsed) {
-        ASSERT_THROW(eval("let or = 1; in or"), Error);
+    TEST_F(TrivialExpressionTest, orCanBeUsed) {
+        auto v = eval("let or = 1; in or");
+        ASSERT_THAT(v, IsIntEq(1));
+    }
+
+    TEST_F(TrivialExpressionTest, orHasCorrectPrecedence) {
+        auto v = eval("let inherit (builtins) add; or = 2; in add 1 or");
+        ASSERT_THAT(v, IsIntEq(3));
     }
 } /* namespace nix */