From 56dee9186f5d98fd04b0c0fbdad975cf1bcf565c Mon Sep 17 00:00:00 2001
From: piegames <git@piegames.de>
Date: Thu, 12 Dec 2024 18:21:53 +0100
Subject: [PATCH] libexpr: Warn on incorrect string escapes

In Nixpkgs, there are several strings like "\d\.\d" which attempt to be
a regex but are just literally "d.d". The escaping rules are silly and
we should warn our users about that.

Co-authored-by: Commentator2.0 <lix@crystal-cavern.systems>
Change-Id: I779b0757358fc9adc34dc140e1670b83abc93b67
---
 doc/manual/rl-next/deprecated-features.md     |  3 +-
 lix/libexpr/parser/parser-impl1.inc.cc        | 34 ++++++++++++---
 lix/libexpr/parser/state.hh                   | 16 +++++++
 .../broken-string-escape.md                   | 17 ++++++++
 lix/libutil/meson.build                       |  1 +
 .../lang/builtins.fromJSON/eval-okay.err.exp  |  0
 .../functional2/lang/builtins.fromJSON/in.nix |  2 +-
 .../depr-warning-escapes.err.exp              | 42 +++++++++++++++++++
 .../depr-warning-escapes.out.exp              |  1 +
 .../string-indented/eval-okay-escapes.out.exp |  1 +
 .../lang/string-indented/in-escapes.nix       |  5 +++
 .../lang/string-indented/test.toml            |  5 ++-
 .../lang/string/eval-okay-escapes.err.exp     | 26 ++++++++++++
 .../lang/string/eval-okay-escapes.out.exp     |  1 +
 tests/functional2/lang/string/in-escapes.nix  |  2 +
 tests/functional2/lang/string/test.toml       |  9 ++++
 tests/nixpkgs/eval.nix                        |  1 +
 tests/nixpkgs/lib.nix                         |  1 +
 18 files changed, 158 insertions(+), 9 deletions(-)
 create mode 100644 lix/libutil/deprecated-features/broken-string-escape.md
 create mode 100644 tests/functional2/lang/builtins.fromJSON/eval-okay.err.exp
 create mode 100644 tests/functional2/lang/string-indented/depr-warning-escapes.err.exp
 create mode 100644 tests/functional2/lang/string-indented/depr-warning-escapes.out.exp
 create mode 100644 tests/functional2/lang/string-indented/eval-okay-escapes.out.exp
 create mode 100644 tests/functional2/lang/string-indented/in-escapes.nix
 create mode 100644 tests/functional2/lang/string/eval-okay-escapes.err.exp
 create mode 100644 tests/functional2/lang/string/eval-okay-escapes.out.exp
 create mode 100644 tests/functional2/lang/string/in-escapes.nix
 create mode 100644 tests/functional2/lang/string/test.toml

diff --git a/doc/manual/rl-next/deprecated-features.md b/doc/manual/rl-next/deprecated-features.md
index b800e24f0..450bf6575 100644
--- a/doc/manual/rl-next/deprecated-features.md
+++ b/doc/manual/rl-next/deprecated-features.md
@@ -1,7 +1,7 @@
 ---
 synopsis: 'more deprecated features'
 issues: []
-cls: [2092]
+cls: [2092, 2310]
 category: Breaking Changes
 credits: [piegames, commentator2.0]
 ---
@@ -9,3 +9,4 @@ This release cycle features a new batch of deprecated (anti-)features.
 You can opt in into the old behavior with `--extra-deprecated-features` or any equivalent configuration option.
 
 - `broken-string-indentation` indented strings (those starting with  `''`) might produce unintended results due to how the whitespace stripping is done. Those cases will now warn the user.
+- `broken-string-escape` "escaped" characters without a properly defined escape sequence evaluate to "themselves". This is in most cases unintended behaviour, both for writing regexes, and using legacy or uncommon escape sequences like `\f`. The user will now be warned, if those are present.
diff --git a/lix/libexpr/parser/parser-impl1.inc.cc b/lix/libexpr/parser/parser-impl1.inc.cc
index d134e9bdb..796036f2a 100644
--- a/lix/libexpr/parser/parser-impl1.inc.cc
+++ b/lix/libexpr/parser/parser-impl1.inc.cc
@@ -580,14 +580,17 @@ struct StringState : SubexprState {
                 if (c == 'n') *t = '\n';
                 else if (c == 'r') *t = '\r';
                 else if (c == 't') *t = '\t';
-                else *t = c;
+                else {
+                    *t = c;
+                }
             }
             else if (c == '\r') {
                 /* Normalise CR and CR/LF into LF. */
                 *t = '\n';
                 if (*s == '\n') s++; /* cr/lf */
+            } else {
+                *t = c;
             }
-            else *t = c;
             t++;
         }
         if (!ps.featureSettings.isEnabled(Dep::NulBytes) && size_t(s - str.data() - 1) != str.size())
@@ -640,8 +643,15 @@ template<> struct BuildAST<grammar::v1::string::interpolation> {
 
 template<> struct BuildAST<grammar::v1::string::escape> {
     static void apply(const auto & in, StringState & s, State & ps) {
-        if (!ps.featureSettings.isEnabled(Dep::NulBytes) && *in.begin() == '\0')
+        char c = *in.begin();
+        if (!ps.featureSettings.isEnabled(Dep::NulBytes) && c == '\0') {
             ps.nulFound(ps.at(in));
+        }
+        if (!ps.featureSettings.isEnabled(Dep::BrokenStringEscape) && c != '\\' && c != '$'
+            && c != '"' && c != 'r' && c != 'n' && c != 't')
+        {
+            ps.badEscapeFound(ps.at(in), c, "\\");
+        }
         s.append(ps.at(in), "\\"); // FIXME compat with old parser
         s.append(ps.at(in), in.string_view());
     }
@@ -691,17 +701,31 @@ template<> struct BuildAST<grammar::v1::ind_string::interpolation> {
 
 template<> struct BuildAST<grammar::v1::ind_string::escape> {
     static void apply(const auto & in, IndStringState & s, State & ps) {
-        switch (*in.begin()) {
+        auto c = *in.begin();
+        switch (c) {
         case 'n': s.lines.back().parts.emplace_back(ps.at(in), "\n"); break;
         case 'r': s.lines.back().parts.emplace_back(ps.at(in), "\r"); break;
         case 't': s.lines.back().parts.emplace_back(ps.at(in), "\t"); break;
+        // TODO merge with below
+        // `''\'` must escape to itself even though one can just write `'` instead, because of
+        // shit like
+        // `''\'''${` to express the string `'${` (remember that `'''` escapes to `''`)
+        case '\'':
+            s.lines.back().parts.emplace_back(ps.at(in), "'");
+            break;
         case 0:
             if (!ps.featureSettings.isEnabled(Dep::NulBytes)) {
                 ps.nulFound(ps.at(in));
                 break;
             }
             KJ_FALLTHROUGH;
-        default:  s.lines.back().parts.emplace_back(ps.at(in), in.string_view()); break;
+        default:
+            if (!ps.featureSettings.isEnabled(Dep::BrokenStringEscape)) {
+                ps.badEscapeFound(ps.at(in), c, "''\\");
+            }
+
+            s.lines.back().parts.emplace_back(ps.at(in), in.string_view());
+            break;
         }
     }
 };
diff --git a/lix/libexpr/parser/state.hh b/lix/libexpr/parser/state.hh
index b2e2f67e0..ab80a223c 100644
--- a/lix/libexpr/parser/state.hh
+++ b/lix/libexpr/parser/state.hh
@@ -38,6 +38,7 @@ struct State
     void badLineEndingFound(const PosIdx pos, bool warnOnly);
     void badFirstLineIndStringFound(const PosIdx pos);
     void badSingleLineIndStringFound(const PosIdx pos);
+    void badEscapeFound(const PosIdx pos, char found, std::string escape);
     void nulFound(const PosIdx pos);
     void addAttr(ExprAttrs * attrs, AttrPath && attrPath, std::unique_ptr<Expr> e, const PosIdx pos);
     void mergeAttrs(AttrPath & attrPath, ExprSet * source, ExprSet * target);
@@ -122,6 +123,21 @@ inline void State::badFirstLineIndStringFound(const PosIdx pos)
         .pos = positions[pos],
     });
 }
+// Added 2024-12-12, equally used in the wild.
+inline void State::badEscapeFound(const PosIdx pos, char found, std::string escape)
+{
+    logWarning({
+        .msg = HintFmt(
+            "%s is an ill-defined escape. You can drop the %s and simply write %s instead. Use %s "
+            "to silence this warning.",
+            escape + found,
+            escape,
+            found,
+            "--extra-deprecated-features broken-string-escape"
+        ),
+        .pos = positions[pos],
+    });
+}
 // Added 2025-02-05. This is unlikely to ever occur in the wild, given how broken it is
 inline void State::badLineEndingFound(const PosIdx pos, bool warnOnly)
 {
diff --git a/lix/libutil/deprecated-features/broken-string-escape.md b/lix/libutil/deprecated-features/broken-string-escape.md
new file mode 100644
index 000000000..1314ad9d4
--- /dev/null
+++ b/lix/libutil/deprecated-features/broken-string-escape.md
@@ -0,0 +1,17 @@
+---
+name: broken-string-escape
+internalName: BrokenStringEscape
+timeline:
+  - date: 2024-12-12
+    release: 2.95.0
+    cls: [2310]
+    message: Introduced as a warning.
+---
+In Nix, string literals define syntax for escaping special characters like `\n`.
+Only a limited set of escape rules are defined.
+All characters without defined escape sequence escape "as themselves", e.g. `"\f"` becomes simply `f` instead of a form feed character.
+Using these fallback escape sequences is now deprecated, because all usage sites in the wild found so far have been proven to be erroneous,
+where the string did not end up the way the author likely intended.
+For example when writing a regex in Nix, `"\."` will evaluate to the pattern `.` instead of the probably intended `\.` for matching a literal dot character, for which `"\\."` would have been correct instead.
+
+To fix this, carefully evaluate each usage site for its intended usage and either remove the backslash or add a second backslash depending on the context.
diff --git a/lix/libutil/meson.build b/lix/libutil/meson.build
index 2cc007090..1b279592d 100644
--- a/lix/libutil/meson.build
+++ b/lix/libutil/meson.build
@@ -175,6 +175,7 @@ experimental_feature_definitions = files(
 deprecated_feature_definitions = files(
   # keep-sorted start
   'deprecated-features/ancient-let.md',
+  'deprecated-features/broken-string-escape.md',
   'deprecated-features/broken-string-indentation.md',
   'deprecated-features/cr-line-endings.md',
   'deprecated-features/nix-path-shadow.md',
diff --git a/tests/functional2/lang/builtins.fromJSON/eval-okay.err.exp b/tests/functional2/lang/builtins.fromJSON/eval-okay.err.exp
new file mode 100644
index 000000000..e69de29bb
diff --git a/tests/functional2/lang/builtins.fromJSON/in.nix b/tests/functional2/lang/builtins.fromJSON/in.nix
index 4c526b9ae..6a0a21c19 100644
--- a/tests/functional2/lang/builtins.fromJSON/in.nix
+++ b/tests/functional2/lang/builtins.fromJSON/in.nix
@@ -33,7 +33,7 @@ builtins.fromJSON
         };
       Animated = false;
       IDs = [ 116 943 234 38793 true false null (0-100) ];
-      Escapes = "\"\\\/\t\n\r\t";  # supported in JSON but not Nix: \b\f
+      Escapes = "\"\\/\t\n\r\t";  # supported in JSON but not Nix: \b\f
       Subtitle = false;
       Latitude = 37.7668;
       Longitude = -122.3959;
diff --git a/tests/functional2/lang/string-indented/depr-warning-escapes.err.exp b/tests/functional2/lang/string-indented/depr-warning-escapes.err.exp
new file mode 100644
index 000000000..bb2319a97
--- /dev/null
+++ b/tests/functional2/lang/string-indented/depr-warning-escapes.err.exp
@@ -0,0 +1,42 @@
+warning: ''\a is an ill-defined escape. You can drop the ''\ and simply write a instead. Use --extra-deprecated-features broken-string-escape to silence this warning.
+         at /pwd/in.nix:2:4:
+              1| ''
+              2| ''\a''\n
+               |    ^
+              3| ''\f''\{''\$''\"''\'''\r''\v''\\''\t
+warning: ''\f is an ill-defined escape. You can drop the ''\ and simply write f instead. Use --extra-deprecated-features broken-string-escape to silence this warning.
+         at /pwd/in.nix:3:4:
+              2| ''\a''\n
+              3| ''\f''\{''\$''\"''\'''\r''\v''\\''\t
+               |    ^
+              4| ''\'''${"x"}
+warning: ''\{ is an ill-defined escape. You can drop the ''\ and simply write { instead. Use --extra-deprecated-features broken-string-escape to silence this warning.
+         at /pwd/in.nix:3:8:
+              2| ''\a''\n
+              3| ''\f''\{''\$''\"''\'''\r''\v''\\''\t
+               |        ^
+              4| ''\'''${"x"}
+warning: ''\$ is an ill-defined escape. You can drop the ''\ and simply write $ instead. Use --extra-deprecated-features broken-string-escape to silence this warning.
+         at /pwd/in.nix:3:12:
+              2| ''\a''\n
+              3| ''\f''\{''\$''\"''\'''\r''\v''\\''\t
+               |            ^
+              4| ''\'''${"x"}
+warning: ''\" is an ill-defined escape. You can drop the ''\ and simply write " instead. Use --extra-deprecated-features broken-string-escape to silence this warning.
+         at /pwd/in.nix:3:16:
+              2| ''\a''\n
+              3| ''\f''\{''\$''\"''\'''\r''\v''\\''\t
+               |                ^
+              4| ''\'''${"x"}
+warning: ''\v is an ill-defined escape. You can drop the ''\ and simply write v instead. Use --extra-deprecated-features broken-string-escape to silence this warning.
+         at /pwd/in.nix:3:28:
+              2| ''\a''\n
+              3| ''\f''\{''\$''\"''\'''\r''\v''\\''\t
+               |                            ^
+              4| ''\'''${"x"}
+warning: ''\\ is an ill-defined escape. You can drop the ''\ and simply write \ instead. Use --extra-deprecated-features broken-string-escape to silence this warning.
+         at /pwd/in.nix:3:32:
+              2| ''\a''\n
+              3| ''\f''\{''\$''\"''\'''\r''\v''\\''\t
+               |                                ^
+              4| ''\'''${"x"}
diff --git a/tests/functional2/lang/string-indented/depr-warning-escapes.out.exp b/tests/functional2/lang/string-indented/depr-warning-escapes.out.exp
new file mode 100644
index 000000000..c1c79abde
--- /dev/null
+++ b/tests/functional2/lang/string-indented/depr-warning-escapes.out.exp
@@ -0,0 +1 @@
+"a\n\nf{$\"'\rv\\\t\n'\${\"x\"}\n"
diff --git a/tests/functional2/lang/string-indented/eval-okay-escapes.out.exp b/tests/functional2/lang/string-indented/eval-okay-escapes.out.exp
new file mode 100644
index 000000000..c1c79abde
--- /dev/null
+++ b/tests/functional2/lang/string-indented/eval-okay-escapes.out.exp
@@ -0,0 +1 @@
+"a\n\nf{$\"'\rv\\\t\n'\${\"x\"}\n"
diff --git a/tests/functional2/lang/string-indented/in-escapes.nix b/tests/functional2/lang/string-indented/in-escapes.nix
new file mode 100644
index 000000000..c421e07c8
--- /dev/null
+++ b/tests/functional2/lang/string-indented/in-escapes.nix
@@ -0,0 +1,5 @@
+''
+''\a''\n
+''\f''\{''\$''\"''\'''\r''\v''\\''\t
+''\'''${"x"}
+''
diff --git a/tests/functional2/lang/string-indented/test.toml b/tests/functional2/lang/string-indented/test.toml
index 679f0334d..eea26e5da 100644
--- a/tests/functional2/lang/string-indented/test.toml
+++ b/tests/functional2/lang/string-indented/test.toml
@@ -1,12 +1,13 @@
 [[test]]
 runner = "eval-okay"
 matrix = true
-flags = ["--extra-deprecated-features", "broken-string-indentation"]
+flags = ["--extra-deprecated-features", "broken-string-indentation broken-string-escape"]
 
 [[test]]
 name = "depr-warning"
 runner = "eval-okay"
-in = "in.nix"
+matrix = true
+in = ["in.nix", "in-escapes.nix"]
 
 [[test]]
 runner = "parse-okay"
diff --git a/tests/functional2/lang/string/eval-okay-escapes.err.exp b/tests/functional2/lang/string/eval-okay-escapes.err.exp
new file mode 100644
index 000000000..edc70b049
--- /dev/null
+++ b/tests/functional2/lang/string/eval-okay-escapes.err.exp
@@ -0,0 +1,26 @@
+warning: \a is an ill-defined escape. You can drop the \ and simply write a instead. Use --extra-deprecated-features broken-string-escape to silence this warning.
+         at /pwd/in.nix:1:3:
+              1| "\a\n\{\$\"\'\r\v\\\t
+               |   ^
+              2| \f"
+warning: \{ is an ill-defined escape. You can drop the \ and simply write { instead. Use --extra-deprecated-features broken-string-escape to silence this warning.
+         at /pwd/in.nix:1:7:
+              1| "\a\n\{\$\"\'\r\v\\\t
+               |       ^
+              2| \f"
+warning: \' is an ill-defined escape. You can drop the \ and simply write ' instead. Use --extra-deprecated-features broken-string-escape to silence this warning.
+         at /pwd/in.nix:1:13:
+              1| "\a\n\{\$\"\'\r\v\\\t
+               |             ^
+              2| \f"
+warning: \v is an ill-defined escape. You can drop the \ and simply write v instead. Use --extra-deprecated-features broken-string-escape to silence this warning.
+         at /pwd/in.nix:1:17:
+              1| "\a\n\{\$\"\'\r\v\\\t
+               |                 ^
+              2| \f"
+warning: \f is an ill-defined escape. You can drop the \ and simply write f instead. Use --extra-deprecated-features broken-string-escape to silence this warning.
+         at /pwd/in.nix:2:2:
+              1| "\a\n\{\$\"\'\r\v\\\t
+              2| \f"
+               |  ^
+              3|
diff --git a/tests/functional2/lang/string/eval-okay-escapes.out.exp b/tests/functional2/lang/string/eval-okay-escapes.out.exp
new file mode 100644
index 000000000..cd331d686
--- /dev/null
+++ b/tests/functional2/lang/string/eval-okay-escapes.out.exp
@@ -0,0 +1 @@
+"a\n{$\"'\rv\\\t\nf"
diff --git a/tests/functional2/lang/string/in-escapes.nix b/tests/functional2/lang/string/in-escapes.nix
new file mode 100644
index 000000000..07c4d5a8a
--- /dev/null
+++ b/tests/functional2/lang/string/in-escapes.nix
@@ -0,0 +1,2 @@
+"\a\n\{\$\"\'\r\v\\\t
+\f"
diff --git a/tests/functional2/lang/string/test.toml b/tests/functional2/lang/string/test.toml
new file mode 100644
index 000000000..79cf605b5
--- /dev/null
+++ b/tests/functional2/lang/string/test.toml
@@ -0,0 +1,9 @@
+[[test]]
+runner = "eval-okay"
+matrix = true
+flags = ["--extra-deprecated-features", "broken-string-escape"]
+in = ["in.nix", "in-backslash-newline.nix"]
+
+[[test]]
+runner = "eval-okay"
+in = "in-escapes.nix"
diff --git a/tests/nixpkgs/eval.nix b/tests/nixpkgs/eval.nix
index cabd0a40a..b53445d9a 100644
--- a/tests/nixpkgs/eval.nix
+++ b/tests/nixpkgs/eval.nix
@@ -12,6 +12,7 @@ let
   # FIXME: All of these are fixed in Nixpkgs already, so clear the list on the next `nixpkgs-regression` bump
   deprecatedFeatures = [
     "broken-string-indentation"
+    "broken-string-escape"
   ];
 in
 
diff --git a/tests/nixpkgs/lib.nix b/tests/nixpkgs/lib.nix
index 41df3f5b0..332e4e028 100644
--- a/tests/nixpkgs/lib.nix
+++ b/tests/nixpkgs/lib.nix
@@ -21,6 +21,7 @@ let
   # FIXME: All of these are fixed in Nixpkgs already, so clear the list on the next `nixpkgs` bump
   deprecatedFeatures = [
     "broken-string-indentation"
+    "broken-string-escape"
   ];
 
   env.NIX_CONFIG = "extra-deprecated-features = ${concatStringsSep " " deprecatedFeatures}";