Merge "libexpr: Deprecate overriding __sub and the like" into main

doc/manual/rl-next/deprecated-features.md

@@ -1,7 +1,7 @@
 ---
 synopsis: Deprecated language features
-issues: [fj#437]
-cls: [1785, 1736, 1735, 1744]
+issues: [fj#437, 861]
+cls: [1785, 1736, 1735, 1744, 2206]
 category: Breaking Changes
 credits: [piegames, horrors]
 ---
@@ -15,3 +15,4 @@ and can be disabled via the flags for backwards compatibility (opt-out with `--e
 - `rec-set-overrides`: **__overrides** is an old arcane syntax which has not been in use for more than a decade.
   It is soft-deprecated with a warning only, with the plan to turn that into an error in a future release.
 - `ancient-let`: **The old `let` syntax** (`let { body = …; … }`) is soft-deprecated with a warning as well. Use the regular `let … in` instead.
+- `shadow-internal-symbols`: Arithmetic expressions like `5 - 3` internally expand to `__sub 5 3`, where `__sub` maps to a subtraction builtin. Shadowing such a symbols would affect the evaluation of such operations, but in a very inconsistent way, and is therefore deprecated now. **Affected symbols are:** `__sub`, `__mul`, `__div`, `__lessThan`, `__findFile` and `__nixPath`. Note that these symbols may still be used as variable names as long as they do not shadow internal operations, so e.g. `let __sub = x: y: x + y; in __sub 3 5` remains valid code.

lix/libexpr/nixexpr.cc

@@ -342,6 +342,17 @@ void ExprVar::bindVars(EvalState & es, const std::shared_ptr<const StaticEnv> &
         } else {
             auto i = curEnv->find(name);
             if (i != curEnv->vars.end()) {
+                if (this->needsRoot && !curEnv->isRoot) {
+                    throw ParseError({
+                        .msg = HintFmt(
+                            "Shadowing symbol '%s' used in internal expressions is not allowed. Use %s to disable this error.",
+                            es.symbols[name],
+                            "--extra-deprecated-features shadow-internal-symbols"
+                        ),
+                        .pos = es.positions[pos]
+                    });
+                }
+
                 this->level = level;
                 displ = i->second;
                 return;

lix/libexpr/nixexpr.hh

@@ -132,8 +132,14 @@ struct ExprVar : Expr
     Level level;
     Displacement displ;
 
-    ExprVar(Symbol name) : name(name) { };
-    ExprVar(const PosIdx & pos, Symbol name) : pos(pos), name(name) { };
+    /* Variables like `__sub` as generated from expressions like `5 - 3` shouldn't be overridden.
+     * This is implemented by having a blessed "root" env that contains the definitions (usually `builtins`)
+     * and checking that this var only binds against that env.
+     */
+    bool needsRoot;
+
+    ExprVar(Symbol name) : name(name), needsRoot(false) { };
+    ExprVar(const PosIdx & pos, Symbol name, bool needsRoot = false) : pos(pos), name(name), needsRoot(needsRoot) { };
     Value * maybeThunk(EvalState & state, Env & env) override;
     PosIdx getPos() const override { return pos; }
     COMMON_METHODS
@@ -476,6 +482,9 @@ struct StaticEnv
     typedef std::vector<std::pair<Symbol, Displacement>> Vars;
     Vars vars;
 
+    /* See ExprVar::needsRoot */
+    bool isRoot = false;
+
     StaticEnv(ExprWith * isWith, const StaticEnv * up, size_t expectedSize = 0) : isWith(isWith), up(up) {
         vars.reserve(expectedSize);
     };

lix/libexpr/parser/parser-impl1.inc.cc

@@ -89,12 +89,12 @@ struct ExprState
         return std::make_unique<Op>(pos, std::move(left), std::move(right));
     }
 
-    std::unique_ptr<Expr> call(PosIdx pos, Symbol fn, bool flip = false)
+    std::unique_ptr<Expr> call(PosIdx pos, State & state, Symbol fn, bool flip = false)
     {
         std::vector<std::unique_ptr<Expr>> args(2);
         args[flip ? 0 : 1] = popExprOnly();
         args[flip ? 1 : 0] = popExprOnly();
-        return std::make_unique<ExprCall>(pos, std::make_unique<ExprVar>(fn), std::move(args));
+        return std::make_unique<ExprCall>(pos, state.mkInternalVar(pos, fn), std::move(args));
     }
 
     std::unique_ptr<Expr> pipe(PosIdx pos, State & state, bool flip = false)
@@ -122,7 +122,7 @@ struct ExprState
 
     std::unique_ptr<Expr> order(PosIdx pos, bool less, State & state)
     {
-        return call(pos, state.s.lessThan, !less);
+        return call(pos, state, state.s.lessThan, !less);
     }
 
     std::unique_ptr<Expr> concatStrings(PosIdx pos)
@@ -138,7 +138,7 @@ struct ExprState
         std::vector<std::unique_ptr<Expr>> args(2);
         args[0] = std::make_unique<ExprInt>(0);
         args[1] = popExprOnly();
-        return std::make_unique<ExprCall>(pos, std::make_unique<ExprVar>(state.s.sub), std::move(args));
+        return std::make_unique<ExprCall>(pos, state.mkInternalVar(pos, state.s.sub), std::move(args));
     }
 
     std::pair<PosIdx, std::unique_ptr<Expr>> applyOp(PosIdx pos, auto & op, State & state) {
@@ -163,9 +163,9 @@ struct ExprState
                 [&] (Op::update)      { return applyBinary<ExprOpUpdate>(pos); },
                 [&] (Op::not_)        { return applyUnary<ExprOpNot>(); },
                 [&] (Op::plus)        { return concatStrings(pos); },
-                [&] (Op::minus)       { return call(pos, state.s.sub); },
-                [&] (Op::mul)         { return call(pos, state.s.mul); },
-                [&] (Op::div)         { return call(pos, state.s.div); },
+                [&] (Op::minus)       { return call(pos, state, state.s.sub); },
+                [&] (Op::mul)         { return call(pos, state, state.s.mul); },
+                [&] (Op::div)         { return call(pos, state, state.s.div); },
                 [&] (Op::concat)      { return applyBinary<ExprOpConcatLists>(pos); },
                 [&] (has_attr & a)    { return applyUnary<ExprOpHasAttr>(std::move(a.path)); },
                 [&] (Op::unary_minus) { return negate(pos, state); },
@@ -608,14 +608,15 @@ template<> struct BuildAST<grammar::v1::path::home_anchor> {
 
 template<> struct BuildAST<grammar::v1::path::searched_path> {
     static void apply(const auto & in, StringState & s, State & ps) {
+        auto pos = ps.at(in);
         std::vector<std::unique_ptr<Expr>> args{2};
-        args[0] = std::make_unique<ExprVar>(ps.s.nixPath);
+        args[0] = ps.mkInternalVar(pos, ps.s.nixPath);
         args[1] = std::make_unique<ExprString>(in.string());
         s.parts.emplace_back(
-            ps.at(in),
+            pos,
             std::make_unique<ExprCall>(
-                ps.at(in),
-                std::make_unique<ExprVar>(ps.s.findFile),
+                pos,
+                ps.mkInternalVar(pos, ps.s.findFile),
                 std::move(args)));
     }
 };

lix/libexpr/parser/state.hh

@@ -39,6 +39,12 @@ struct State
     std::unique_ptr<Formals> validateFormals(std::unique_ptr<Formals> formals, PosIdx pos = noPos, Symbol arg = {});
     std::unique_ptr<Expr> stripIndentation(const PosIdx pos, std::vector<IndStringLine> && line);
 
+    /* Creates an ExprVar or an ExprVarRoot depending on the feature settings.
+     * The symbol is synthetic, but for the purpose of error handling the pos is required
+     * and should point to the expression where the var is used
+     */
+    inline std::unique_ptr<ExprVar> mkInternalVar(PosIdx pos, Symbol name);
+
     // lazy positioning means we don't get byte offsets directly, in.position() would work
     // but also requires line and column (which is expensive)
     PosIdx at(const auto & in)
@@ -52,6 +58,10 @@ struct State
     }
 };
 
+std::unique_ptr<ExprVar> State::mkInternalVar(PosIdx pos, Symbol name) {
+    return std::make_unique<ExprVar>(pos, name, !featureSettings.isEnabled(Dep::ShadowInternalSymbols));
+}
+
 inline void State::dupAttr(const AttrPath & attrPath, const PosIdx pos, const PosIdx prevPos)
 {
     throw ParseError({

lix/libexpr/primops.cc

@@ -2845,6 +2845,7 @@ void EvalState::createBaseEnv()
     baseEnv.values[0]->attrs->sort();
 
     staticBaseEnv->sort();
+    staticBaseEnv->isRoot = true;
 
     /* Note: we have to initialize the 'derivation' constant *after*
        building baseEnv/staticBaseEnv because it uses 'builtins'. */

lix/libutil/deprecated-features/shadow-internal-symbols.md

@@ -0,0 +1,5 @@
+---
+name: shadow-internal-symbols
+internalName: ShadowInternalSymbols
+---
+Allow shadowing the symbols `__sub`, `__mul`, `__div`, `__lessThan`, `__findFile` and `__nixPath` when used in the internal AST expansion. (`5 - 3` expands to `__sub 5 3` etc.)

lix/libutil/meson.build

@@ -155,6 +155,7 @@ deprecated_feature_definitions = files(
   'deprecated-features/ancient-let.md',
   'deprecated-features/rec-set-overrides.md',
   'deprecated-features/url-literals.md',
+  'deprecated-features/shadow-internal-symbols.md',
 )
 
 experimental_features_gen = custom_target(

tests/functional/lang/eval-okay-arithmetic-override.exp

@@ -0,0 +1 @@
+[ 8 "multiplied" "subtracted" "subtracted" "divided" "compared" ]

tests/functional/lang/eval-okay-arithmetic-override.flags

@@ -0,0 +1 @@
+--extra-deprecated-features shadow-internal-symbols

tests/functional/lang/eval-okay-arithmetic-override.nix

@@ -0,0 +1,14 @@
+let
+  __sub = _: _: "subtracted";
+  __mul = _: _: "multiplied";
+  __div = _: _: "divided";
+  __lessThan = _: _: "compared";
+in
+[
+  (3 + 5)
+  (4 * 4)
+  (-3)
+  (12 - 3)
+  (0 / 1)
+  (42 < 16)
+]

tests/functional/lang/eval-okay-search-path.flags

@@ -1 +1 @@
--I lang/dir1 -I lang/dir2 -I dir5=lang/dir3
+--extra-deprecated-features shadow-internal-symbols -I lang/dir1 -I lang/dir2 -I dir5=lang/dir3

tests/functional/lang/parse-okay-arithmetic.exp

@@ -0,0 +1 @@
+(__sub (7 + (__div (__mul (__sub 0 5) 12) 3)) 1)

tests/functional/lang/parse-okay-arithmetic.nix

@@ -0,0 +1 @@
+7 + (-5) * 12 / 3 - 1

tests/functional/restricted.sh

@@ -14,8 +14,8 @@ nix-instantiate --restrict-eval --eval -E 'builtins.readFile ./simple.nix' -I sr
 (! nix-instantiate --restrict-eval --eval -E 'builtins.readDir ../../lix/legacy')
 nix-instantiate --restrict-eval --eval -E 'builtins.readDir ../../lix/legacy' -I src=../../lix
 
-(! nix-instantiate --restrict-eval --eval -E 'let __nixPath = [ { prefix = "foo"; path = ./.; } ]; in <foo>')
-nix-instantiate --restrict-eval --eval -E 'let __nixPath = [ { prefix = "foo"; path = ./.; } ]; in <foo>' -I src=.
+(! nix-instantiate --extra-deprecated-features shadow-internal-symbols --restrict-eval --eval -E 'let __nixPath = [ { prefix = "foo"; path = ./.; } ]; in <foo>')
+nix-instantiate --extra-deprecated-features shadow-internal-symbols --restrict-eval --eval -E 'let __nixPath = [ { prefix = "foo"; path = ./.; } ]; in <foo>' -I src=.
 
 p=$(nix eval --raw --expr "builtins.fetchurl \"file://$(pwd)/restricted.sh\"" --impure --restrict-eval --allowed-uris "file://$(pwd)")
 cmp $p restricted.sh

tests/unit/libexpr/trivial.cc

@@ -246,4 +246,38 @@ namespace nix {
         v = eval("let add = l: r: l + r; in ''a'' |> add <| ''c''", true, mockFeatureSettings);
         ASSERT_THAT(v, IsStringEq("ac"));
     }
+
+    TEST_F(TrivialExpressionTest, shadowInternalSymbols) {
+        FeatureSettings mockFeatureSettings;
+        mockFeatureSettings.set("deprecated-features", "shadow-internal-symbols");
+
+        auto v = eval("let __sub = _: _: ''subtracted''; in -3", true, mockFeatureSettings);
+        ASSERT_THAT(v, IsStringEq("subtracted"));
+        ASSERT_THROW(eval("let __sub = _: _: ''subtracted''; in -3"), Error);
+
+        v = eval("let __sub = _: _: ''subtracted''; in 12 - 3", true, mockFeatureSettings);
+        ASSERT_THAT(v, IsStringEq("subtracted"));
+        ASSERT_THROW(eval("let __sub = _: _: ''subtracted''; in 12 - 3"), Error);
+
+        v = eval("let __mul = _: _: ''multiplied''; in 4 * 4", true, mockFeatureSettings);
+        ASSERT_THAT(v, IsStringEq("multiplied"));
+        ASSERT_THROW(eval("let __mul = _: _: ''multiplied''; in 4 * 4"), Error);
+
+        v = eval("let __div = _: _: ''divided''; in 0 / 1", true, mockFeatureSettings);
+        ASSERT_THAT(v, IsStringEq("divided"));
+        ASSERT_THROW(eval("let __div = _: _: ''divided''; in 0 / 1"), Error);
+
+        v = eval("let __lessThan = _: _: ''compared''; in 42 < 16", true, mockFeatureSettings);
+        ASSERT_THAT(v, IsStringEq("compared"));
+        ASSERT_THROW(eval("let __lessThan = _: _: ''compared''; in 42 < 16"), Error);
+
+        v = eval(
+            "let __findFile = path: entry: assert path == ''foo''; entry; __nixPath = ''foo''; in <bar>",
+            true,
+            mockFeatureSettings
+        );
+        ASSERT_THAT(v, IsStringEq("bar"));
+        ASSERT_THROW(eval("let __findFile = _: _: ''found''; in import <foo>"), Error);
+        ASSERT_THROW(eval("let __nixPath = null; in import <foo>"), Error);
+    }
 } /* namespace nix */