From 81d5f0a7d959264c58801704b23e1904130c564d Mon Sep 17 00:00:00 2001 From: piegames Date: Mon, 18 Nov 2024 12:56:46 +0100 Subject: [PATCH] libexpr: Deprecate overriding __sub and the like It was never intended to be a feature to be used, and moreover it is inconsistent: One cannot override `+`, and overriding `__lessThan` won't affect the builtins which do comparisons. Change-Id: Iaba54a05aa4c2eb37cdb3dc0d731fcee5a86deba --- doc/manual/rl-next/deprecated-features.md | 5 +-- lix/libexpr/nixexpr.cc | 11 ++++++ lix/libexpr/nixexpr.hh | 13 +++++-- lix/libexpr/parser/parser-impl1.inc.cc | 23 +++++++------ lix/libexpr/parser/state.hh | 10 ++++++ lix/libexpr/primops.cc | 1 + .../shadow-internal-symbols.md | 5 +++ lix/libutil/meson.build | 1 + .../lang/eval-okay-arithmetic-override.exp | 1 + .../lang/eval-okay-arithmetic-override.flags | 1 + .../lang/eval-okay-arithmetic-override.nix | 14 ++++++++ .../lang/eval-okay-search-path.flags | 2 +- .../functional/lang/parse-okay-arithmetic.exp | 1 + .../functional/lang/parse-okay-arithmetic.nix | 1 + tests/functional/restricted.sh | 4 +-- tests/unit/libexpr/trivial.cc | 34 +++++++++++++++++++ 16 files changed, 109 insertions(+), 18 deletions(-) create mode 100644 lix/libutil/deprecated-features/shadow-internal-symbols.md create mode 100644 tests/functional/lang/eval-okay-arithmetic-override.exp create mode 100644 tests/functional/lang/eval-okay-arithmetic-override.flags create mode 100644 tests/functional/lang/eval-okay-arithmetic-override.nix create mode 100644 tests/functional/lang/parse-okay-arithmetic.exp create mode 100644 tests/functional/lang/parse-okay-arithmetic.nix diff --git a/doc/manual/rl-next/deprecated-features.md b/doc/manual/rl-next/deprecated-features.md index 97a660da2..8a85a80bf 100644 --- a/doc/manual/rl-next/deprecated-features.md +++ b/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. diff --git a/lix/libexpr/nixexpr.cc b/lix/libexpr/nixexpr.cc index 58f0aa43d..7f7fea581 100644 --- a/lix/libexpr/nixexpr.cc +++ b/lix/libexpr/nixexpr.cc @@ -342,6 +342,17 @@ void ExprVar::bindVars(EvalState & es, const std::shared_ptr & } 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; diff --git a/lix/libexpr/nixexpr.hh b/lix/libexpr/nixexpr.hh index 754e45a4a..292ed18ef 100644 --- a/lix/libexpr/nixexpr.hh +++ b/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> 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); }; diff --git a/lix/libexpr/parser/parser-impl1.inc.cc b/lix/libexpr/parser/parser-impl1.inc.cc index c937d17fd..a1b5afa8a 100644 --- a/lix/libexpr/parser/parser-impl1.inc.cc +++ b/lix/libexpr/parser/parser-impl1.inc.cc @@ -89,12 +89,12 @@ struct ExprState return std::make_unique(pos, std::move(left), std::move(right)); } - std::unique_ptr call(PosIdx pos, Symbol fn, bool flip = false) + std::unique_ptr call(PosIdx pos, State & state, Symbol fn, bool flip = false) { std::vector> args(2); args[flip ? 0 : 1] = popExprOnly(); args[flip ? 1 : 0] = popExprOnly(); - return std::make_unique(pos, std::make_unique(fn), std::move(args)); + return std::make_unique(pos, state.mkInternalVar(pos, fn), std::move(args)); } std::unique_ptr pipe(PosIdx pos, State & state, bool flip = false) @@ -122,7 +122,7 @@ struct ExprState std::unique_ptr 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 concatStrings(PosIdx pos) @@ -138,7 +138,7 @@ struct ExprState std::vector> args(2); args[0] = std::make_unique(0); args[1] = popExprOnly(); - return std::make_unique(pos, std::make_unique(state.s.sub), std::move(args)); + return std::make_unique(pos, state.mkInternalVar(pos, state.s.sub), std::move(args)); } std::pair> applyOp(PosIdx pos, auto & op, State & state) { @@ -163,9 +163,9 @@ struct ExprState [&] (Op::update) { return applyBinary(pos); }, [&] (Op::not_) { return applyUnary(); }, [&] (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(pos); }, [&] (has_attr & a) { return applyUnary(std::move(a.path)); }, [&] (Op::unary_minus) { return negate(pos, state); }, @@ -608,14 +608,15 @@ template<> struct BuildAST { template<> struct BuildAST { static void apply(const auto & in, StringState & s, State & ps) { + auto pos = ps.at(in); std::vector> args{2}; - args[0] = std::make_unique(ps.s.nixPath); + args[0] = ps.mkInternalVar(pos, ps.s.nixPath); args[1] = std::make_unique(in.string()); s.parts.emplace_back( - ps.at(in), + pos, std::make_unique( - ps.at(in), - std::make_unique(ps.s.findFile), + pos, + ps.mkInternalVar(pos, ps.s.findFile), std::move(args))); } }; diff --git a/lix/libexpr/parser/state.hh b/lix/libexpr/parser/state.hh index af2256f70..89a87e85e 100644 --- a/lix/libexpr/parser/state.hh +++ b/lix/libexpr/parser/state.hh @@ -39,6 +39,12 @@ struct State std::unique_ptr validateFormals(std::unique_ptr formals, PosIdx pos = noPos, Symbol arg = {}); std::unique_ptr stripIndentation(const PosIdx pos, std::vector && 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 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 State::mkInternalVar(PosIdx pos, Symbol name) { + return std::make_unique(pos, name, !featureSettings.isEnabled(Dep::ShadowInternalSymbols)); +} + inline void State::dupAttr(const AttrPath & attrPath, const PosIdx pos, const PosIdx prevPos) { throw ParseError({ diff --git a/lix/libexpr/primops.cc b/lix/libexpr/primops.cc index 9bc32fd80..224792847 100644 --- a/lix/libexpr/primops.cc +++ b/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'. */ diff --git a/lix/libutil/deprecated-features/shadow-internal-symbols.md b/lix/libutil/deprecated-features/shadow-internal-symbols.md new file mode 100644 index 000000000..d065869a2 --- /dev/null +++ b/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.) diff --git a/lix/libutil/meson.build b/lix/libutil/meson.build index e30e990c2..1f8904b3b 100644 --- a/lix/libutil/meson.build +++ b/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( diff --git a/tests/functional/lang/eval-okay-arithmetic-override.exp b/tests/functional/lang/eval-okay-arithmetic-override.exp new file mode 100644 index 000000000..566cb0216 --- /dev/null +++ b/tests/functional/lang/eval-okay-arithmetic-override.exp @@ -0,0 +1 @@ +[ 8 "multiplied" "subtracted" "subtracted" "divided" "compared" ] diff --git a/tests/functional/lang/eval-okay-arithmetic-override.flags b/tests/functional/lang/eval-okay-arithmetic-override.flags new file mode 100644 index 000000000..31b1cc41d --- /dev/null +++ b/tests/functional/lang/eval-okay-arithmetic-override.flags @@ -0,0 +1 @@ +--extra-deprecated-features shadow-internal-symbols diff --git a/tests/functional/lang/eval-okay-arithmetic-override.nix b/tests/functional/lang/eval-okay-arithmetic-override.nix new file mode 100644 index 000000000..45482eeed --- /dev/null +++ b/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) +] diff --git a/tests/functional/lang/eval-okay-search-path.flags b/tests/functional/lang/eval-okay-search-path.flags index dfad1c611..45677d86e 100644 --- a/tests/functional/lang/eval-okay-search-path.flags +++ b/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 diff --git a/tests/functional/lang/parse-okay-arithmetic.exp b/tests/functional/lang/parse-okay-arithmetic.exp new file mode 100644 index 000000000..5e0528669 --- /dev/null +++ b/tests/functional/lang/parse-okay-arithmetic.exp @@ -0,0 +1 @@ +(__sub (7 + (__div (__mul (__sub 0 5) 12) 3)) 1) diff --git a/tests/functional/lang/parse-okay-arithmetic.nix b/tests/functional/lang/parse-okay-arithmetic.nix new file mode 100644 index 000000000..cff3cb4a5 --- /dev/null +++ b/tests/functional/lang/parse-okay-arithmetic.nix @@ -0,0 +1 @@ +7 + (-5) * 12 / 3 - 1 diff --git a/tests/functional/restricted.sh b/tests/functional/restricted.sh index ca537ceff..f35a72f37 100644 --- a/tests/functional/restricted.sh +++ b/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 ') -nix-instantiate --restrict-eval --eval -E 'let __nixPath = [ { prefix = "foo"; path = ./.; } ]; in ' -I src=. +(! nix-instantiate --extra-deprecated-features shadow-internal-symbols --restrict-eval --eval -E 'let __nixPath = [ { prefix = "foo"; path = ./.; } ]; in ') +nix-instantiate --extra-deprecated-features shadow-internal-symbols --restrict-eval --eval -E 'let __nixPath = [ { prefix = "foo"; path = ./.; } ]; in ' -I src=. p=$(nix eval --raw --expr "builtins.fetchurl \"file://$(pwd)/restricted.sh\"" --impure --restrict-eval --allowed-uris "file://$(pwd)") cmp $p restricted.sh diff --git a/tests/unit/libexpr/trivial.cc b/tests/unit/libexpr/trivial.cc index 905e258bc..aa70ae7a2 100644 --- a/tests/unit/libexpr/trivial.cc +++ b/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 ", + true, + mockFeatureSettings + ); + ASSERT_THAT(v, IsStringEq("bar")); + ASSERT_THROW(eval("let __findFile = _: _: ''found''; in import "), Error); + ASSERT_THROW(eval("let __nixPath = null; in import "), Error); + } } /* namespace nix */