From 7210ed1b87410a0df597c0c4efe642bf82cc2b06 Mon Sep 17 00:00:00 2001
From: piegames <git@piegames.de>
Date: Sun, 18 Aug 2024 16:02:23 +0200
Subject: [PATCH] libexpr: Soft-deprecate __overrides

Change-Id: I787e69e1dad6edc5ccdb747b74a9ccd6e8e13bb3
---
 doc/manual/rl-next/deprecated-features.md     | 14 +++++++----
 src/libexpr/eval.cc                           |  1 +
 src/libexpr/nixexpr.hh                        |  2 +-
 src/libexpr/parser/parser.cc                  |  6 +++++
 src/libexpr/parser/state.hh                   | 19 ++++++++++++++
 src/libutil/deprecated-features.cc            |  9 +++++++
 src/libutil/deprecated-features.hh            |  1 +
 tests/functional/lang.sh                      | 25 ++++++++++++++-----
 .../lang/eval-fail-set-override.flags         |  1 +
 tests/functional/lang/eval-okay-attrs6.flags  |  1 +
 .../lang/eval-okay-inherit-from.flags         |  1 +
 .../functional/lang/eval-okay-overrides.flags |  1 +
 .../parse-okay-rec-set-override-nowarning.exp |  1 +
 ...arse-okay-rec-set-override-nowarning.flags |  1 +
 .../parse-okay-rec-set-override-nowarning.nix |  1 +
 ...arse-okay-rec-set-override-warning.err.exp |  2 ++
 .../parse-okay-rec-set-override-warning.exp   |  1 +
 .../parse-okay-rec-set-override-warning.nix   |  9 +++++++
 .../lang/parse-okay-regression-20041027.flags |  1 +
 tests/functional/lang/parse-okay-url.flags    |  1 +
 20 files changed, 86 insertions(+), 12 deletions(-)
 create mode 100644 tests/functional/lang/eval-fail-set-override.flags
 create mode 100644 tests/functional/lang/eval-okay-attrs6.flags
 create mode 100644 tests/functional/lang/eval-okay-inherit-from.flags
 create mode 100644 tests/functional/lang/eval-okay-overrides.flags
 create mode 120000 tests/functional/lang/parse-okay-rec-set-override-nowarning.exp
 create mode 100644 tests/functional/lang/parse-okay-rec-set-override-nowarning.flags
 create mode 120000 tests/functional/lang/parse-okay-rec-set-override-nowarning.nix
 create mode 100644 tests/functional/lang/parse-okay-rec-set-override-warning.err.exp
 create mode 100644 tests/functional/lang/parse-okay-rec-set-override-warning.exp
 create mode 100644 tests/functional/lang/parse-okay-rec-set-override-warning.nix
 create mode 100644 tests/functional/lang/parse-okay-regression-20041027.flags
 create mode 100644 tests/functional/lang/parse-okay-url.flags

diff --git a/doc/manual/rl-next/deprecated-features.md b/doc/manual/rl-next/deprecated-features.md
index d800ac248..3f9756fc9 100644
--- a/doc/manual/rl-next/deprecated-features.md
+++ b/doc/manual/rl-next/deprecated-features.md
@@ -1,12 +1,16 @@
 ---
-synopsis: Deprecated URL literals
+synopsis: Deprecated language features
 issues: [fj#437]
-cls: [1736, 1735, 1744]
+cls: [1785, 1736, 1735, 1744]
 category: Breaking Changes
 credits: [piegames, horrors]
 ---
 
-URL literals have long been obsolete and discouraged of use, and now they are officially deprecated.
-This means that all URLs must be properly put within quotes like all other strings.
+A system for deprecation (and then the planned removal) of undesired language features has been put into place.
+It is controlled via feature flags much like experimental features, except that the deprecations are enabled default,
+and can be disabled via the flags for backwards compatibility (opt-out with `--extra-deprecated-features` or the Nix configuration file).
 
-To ease migration, they can still be enabled with `--extra-deprecated-features url-literals` for now.
+- `url-literals`: **URL literals** have long been obsolete and discouraged of use, and now they are officially deprecated.
+  This means that all URLs must be properly put within quotes like all other strings.
+- `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.
diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc
index fcc28d1ca..227cd254a 100644
--- a/src/libexpr/eval.cc
+++ b/src/libexpr/eval.cc
@@ -249,6 +249,7 @@ EvalState::EvalState(
         .findFile = symbols.create("__findFile"),
         .nixPath = symbols.create("__nixPath"),
         .body = symbols.create("body"),
+        .overrides = symbols.create("__overrides"),
     }
     , repair(NoRepair)
     , emptyBindings(0)
diff --git a/src/libexpr/nixexpr.hh b/src/libexpr/nixexpr.hh
index 49e2e4147..52f254813 100644
--- a/src/libexpr/nixexpr.hh
+++ b/src/libexpr/nixexpr.hh
@@ -48,7 +48,7 @@ protected:
 
 public:
     struct AstSymbols {
-        Symbol sub, lessThan, mul, div, or_, findFile, nixPath, body;
+        Symbol sub, lessThan, mul, div, or_, findFile, nixPath, body, overrides;
     };
 
 
diff --git a/src/libexpr/parser/parser.cc b/src/libexpr/parser/parser.cc
index d57c33a30..6e8ab2fe4 100644
--- a/src/libexpr/parser/parser.cc
+++ b/src/libexpr/parser/parser.cc
@@ -676,6 +676,12 @@ template<> struct BuildAST<grammar::expr::ancient_let> : change_head<BindingsSta
 
 template<> struct BuildAST<grammar::expr::rec_set> : change_head<BindingsState> {
     static void success(const auto & in, BindingsState & b, ExprState & s, State & ps) {
+        // Before inserting new attrs, check for __override and throw an error
+        // (the error will initially be a warning to ease migration)
+        if (!featureSettings.isEnabled(Dep::RecSetOverrides) && b.attrs.attrs.contains(ps.s.overrides)) {
+            ps.overridesFound(ps.at(in));
+        }
+
         b.attrs.pos = ps.at(in);
         b.attrs.recursive = true;
         s.pushExpr<ExprAttrs>(b.attrs.pos, std::move(b.attrs));
diff --git a/src/libexpr/parser/state.hh b/src/libexpr/parser/state.hh
index 9dddd28e1..1d57e2f5f 100644
--- a/src/libexpr/parser/state.hh
+++ b/src/libexpr/parser/state.hh
@@ -2,6 +2,7 @@
 ///@file
 
 #include "eval.hh"
+#include "logging.hh"
 
 namespace nix::parser {
 
@@ -23,6 +24,7 @@ struct State
 
     void dupAttr(const AttrPath & attrPath, const PosIdx pos, const PosIdx prevPos);
     void dupAttr(Symbol attr, const PosIdx pos, const PosIdx prevPos);
+    void overridesFound(const PosIdx pos);
     void addAttr(ExprAttrs * attrs, AttrPath && attrPath, std::unique_ptr<Expr> e, const PosIdx pos);
     std::unique_ptr<Formals> validateFormals(std::unique_ptr<Formals> formals, PosIdx pos = noPos, Symbol arg = {});
     std::unique_ptr<Expr> stripIndentation(const PosIdx pos,
@@ -58,6 +60,17 @@ inline void State::dupAttr(Symbol attr, const PosIdx pos, const PosIdx prevPos)
     });
 }
 
+inline void State::overridesFound(const PosIdx pos) {
+    // Added 2024-09-18. Turn into an error at some point in the future.
+    // See the documentation on deprecated features for more details.
+    warn(
+        "%s found at %s. This feature is deprecated and will be removed in the future. Use %s to silence this warning.",
+        "__overrides",
+        positions[pos],
+        "--extra-deprecated-features rec-set-overrides"
+    );
+}
+
 inline void State::addAttr(ExprAttrs * attrs, AttrPath && attrPath, std::unique_ptr<Expr> e, const PosIdx pos)
 {
     AttrPath::iterator i;
@@ -123,6 +136,12 @@ inline void State::addAttr(ExprAttrs * attrs, AttrPath && attrPath, std::unique_
                 dupAttr(attrPath, pos, j->second.pos);
             }
         } else {
+            // Before inserting new attrs, check for __override and throw an error
+            // (the error will initially be a warning to ease migration)
+            if (attrs->recursive && !featureSettings.isEnabled(Dep::RecSetOverrides) && i->symbol == s.overrides) {
+                overridesFound(pos);
+            }
+
             // This attr path is not defined. Let's create it.
             e->setName(i->symbol);
             attrs->attrs.emplace(std::piecewise_construct,
diff --git a/src/libutil/deprecated-features.cc b/src/libutil/deprecated-features.cc
index 877d69da0..de4efd93b 100644
--- a/src/libutil/deprecated-features.cc
+++ b/src/libutil/deprecated-features.cc
@@ -27,6 +27,15 @@ struct DeprecatedFeatureDetails
 constexpr size_t numDepFeatures = 1 + static_cast<size_t>(Dep::UrlLiterals);
 
 constexpr std::array<DeprecatedFeatureDetails, numDepFeatures> depFeatureDetails = {{
+    {
+        .tag = Dep::RecSetOverrides,
+        .name = "rec-set-overrides",
+        .description = R"(
+            Allow `__overrides` in recursive attribute sets.
+
+            Use fix point functions (e.g. `lib.fix` in Nixpkgs) instead.
+        )",
+    },
     {
         .tag = Dep::UrlLiterals,
         .name = "url-literals",
diff --git a/src/libutil/deprecated-features.hh b/src/libutil/deprecated-features.hh
index 3776e6c29..d4d8a57a9 100644
--- a/src/libutil/deprecated-features.hh
+++ b/src/libutil/deprecated-features.hh
@@ -18,6 +18,7 @@ namespace nix {
  */
 enum struct DeprecatedFeature
 {
+    RecSetOverrides,
     UrlLiterals,
 };
 
diff --git a/tests/functional/lang.sh b/tests/functional/lang.sh
index aa41b41d8..59db10340 100755
--- a/tests/functional/lang.sh
+++ b/tests/functional/lang.sh
@@ -53,10 +53,11 @@ done
 for i in lang/parse-okay-*.nix; do
     echo "parsing $i (should succeed)";
     i=$(basename "$i" .nix)
-    # Hard-code that these two files are allowed to use url literals (because they test them)
-    if [[ "$i" == "parse-okay-url" || "$i" == "parse-okay-regression-20041027" ]]
-    then
-        extraArgs="--extra-deprecated-features url-literals"
+
+    if [ -e "lang/$i.flags" ]; then
+        extraArgs="$(cat "lang/$i.flags")"
+    else
+        extraArgs=""
     fi
     if
         expect 0 nix-instantiate --parse ${extraArgs-} - < "lang/$i.nix" \
@@ -75,8 +76,14 @@ done
 for i in lang/eval-fail-*.nix; do
     echo "evaluating $i (should fail)";
     i=$(basename "$i" .nix)
+
+    if [ -e "lang/$i.flags" ]; then
+        extraArgs="$(cat "lang/$i.flags")"
+    else
+        extraArgs=""
+    fi
     if
-        expectStderr 1 nix-instantiate --eval --strict --show-trace "lang/$i.nix" \
+        expectStderr 1 nix-instantiate --eval --strict --show-trace ${extraArgs-} "lang/$i.nix" \
             | sed "s!$(pwd)!/pwd!g" > "lang/$i.err"
     then
         diffAndAccept "$i" err err.exp
@@ -90,8 +97,14 @@ for i in lang/eval-okay-*.nix; do
     echo "evaluating $i (should succeed)";
     i=$(basename "$i" .nix)
 
+    if [ -e "lang/$i.flags" ]; then
+        extraArgs="$(cat "lang/$i.flags")"
+    else
+        extraArgs=""
+    fi
+
     if test -e "lang/$i.exp.xml"; then
-        if expect 0 nix-instantiate --eval --xml --no-location --strict \
+        if expect 0 nix-instantiate --eval --xml --no-location --strict ${extraArgs-} \
                 "lang/$i.nix" > "lang/$i.out.xml"
         then
             diffAndAccept "$i" out.xml exp.xml
diff --git a/tests/functional/lang/eval-fail-set-override.flags b/tests/functional/lang/eval-fail-set-override.flags
new file mode 100644
index 000000000..a0e513a81
--- /dev/null
+++ b/tests/functional/lang/eval-fail-set-override.flags
@@ -0,0 +1 @@
+--extra-deprecated-features rec-set-overrides
diff --git a/tests/functional/lang/eval-okay-attrs6.flags b/tests/functional/lang/eval-okay-attrs6.flags
new file mode 100644
index 000000000..a0e513a81
--- /dev/null
+++ b/tests/functional/lang/eval-okay-attrs6.flags
@@ -0,0 +1 @@
+--extra-deprecated-features rec-set-overrides
diff --git a/tests/functional/lang/eval-okay-inherit-from.flags b/tests/functional/lang/eval-okay-inherit-from.flags
new file mode 100644
index 000000000..a0e513a81
--- /dev/null
+++ b/tests/functional/lang/eval-okay-inherit-from.flags
@@ -0,0 +1 @@
+--extra-deprecated-features rec-set-overrides
diff --git a/tests/functional/lang/eval-okay-overrides.flags b/tests/functional/lang/eval-okay-overrides.flags
new file mode 100644
index 000000000..a0e513a81
--- /dev/null
+++ b/tests/functional/lang/eval-okay-overrides.flags
@@ -0,0 +1 @@
+--extra-deprecated-features rec-set-overrides
diff --git a/tests/functional/lang/parse-okay-rec-set-override-nowarning.exp b/tests/functional/lang/parse-okay-rec-set-override-nowarning.exp
new file mode 120000
index 000000000..687e738ee
--- /dev/null
+++ b/tests/functional/lang/parse-okay-rec-set-override-nowarning.exp
@@ -0,0 +1 @@
+parse-okay-rec-set-override-warning.exp
\ No newline at end of file
diff --git a/tests/functional/lang/parse-okay-rec-set-override-nowarning.flags b/tests/functional/lang/parse-okay-rec-set-override-nowarning.flags
new file mode 100644
index 000000000..a0e513a81
--- /dev/null
+++ b/tests/functional/lang/parse-okay-rec-set-override-nowarning.flags
@@ -0,0 +1 @@
+--extra-deprecated-features rec-set-overrides
diff --git a/tests/functional/lang/parse-okay-rec-set-override-nowarning.nix b/tests/functional/lang/parse-okay-rec-set-override-nowarning.nix
new file mode 120000
index 000000000..f92d1596f
--- /dev/null
+++ b/tests/functional/lang/parse-okay-rec-set-override-nowarning.nix
@@ -0,0 +1 @@
+parse-okay-rec-set-override-warning.nix
\ No newline at end of file
diff --git a/tests/functional/lang/parse-okay-rec-set-override-warning.err.exp b/tests/functional/lang/parse-okay-rec-set-override-warning.err.exp
new file mode 100644
index 000000000..5ed2d7dee
--- /dev/null
+++ b/tests/functional/lang/parse-okay-rec-set-override-warning.err.exp
@@ -0,0 +1,2 @@
+warning: __overrides found at «stdin»:3:16. This feature is deprecated and will be removed in the future. Use --extra-deprecated-features rec-set-overrides to silence this warning.
+warning: __overrides found at «stdin»:4:2. This feature is deprecated and will be removed in the future. Use --extra-deprecated-features rec-set-overrides to silence this warning.
diff --git a/tests/functional/lang/parse-okay-rec-set-override-warning.exp b/tests/functional/lang/parse-okay-rec-set-override-warning.exp
new file mode 100644
index 000000000..ea17c88f5
--- /dev/null
+++ b/tests/functional/lang/parse-okay-rec-set-override-warning.exp
@@ -0,0 +1 @@
+[ ({ a = rec { __overrides = { }; }; }) (rec { __overrides = { }; }) ({ __overrides = { }; }) (rec { "${("__overrides" + "")}" = { }; }) ]
diff --git a/tests/functional/lang/parse-okay-rec-set-override-warning.nix b/tests/functional/lang/parse-okay-rec-set-override-warning.nix
new file mode 100644
index 000000000..a81d46d53
--- /dev/null
+++ b/tests/functional/lang/parse-okay-rec-set-override-warning.nix
@@ -0,0 +1,9 @@
+[
+	# Should warn
+	{ a = rec {}; a.__overrides = {}; }
+	rec { __overrides = {}; }
+	# Should not warn: Not recursive
+	{ __overrides = {}; }
+	# Should not warn: Dynamic
+	rec { ${"__overrides" + ""} = {}; }
+]
diff --git a/tests/functional/lang/parse-okay-regression-20041027.flags b/tests/functional/lang/parse-okay-regression-20041027.flags
new file mode 100644
index 000000000..86329a66e
--- /dev/null
+++ b/tests/functional/lang/parse-okay-regression-20041027.flags
@@ -0,0 +1 @@
+--extra-deprecated-features url-literals
diff --git a/tests/functional/lang/parse-okay-url.flags b/tests/functional/lang/parse-okay-url.flags
new file mode 100644
index 000000000..86329a66e
--- /dev/null
+++ b/tests/functional/lang/parse-okay-url.flags
@@ -0,0 +1 @@
+--extra-deprecated-features url-literals