libexpr: Warn on incorrect string usage
The indentation stripping semantics of strings are fairly bad and have a few gotchas where they behave unintuitively. But the good news is, that these cases are easy to catch and can be avoided. This commit adds a warning in the parser when such strings are detected. Unfortunately Nixpkgs uses this kind of a lot, so we won't be able to actually enable this warning for a while to come. Co-authored-by: Commentator2.0 <lix@crystal-cavern.systems> Change-Id: I3b3b68c2eee4cd70959d3f4ca643cb6caf3a2217
This commit is contained in:
piegames
co-authored by
Commentator2.0
Commentator2.0
parent
4e1c216fd3
commit
f1fbd1d095
@@ -0,0 +1,11 @@
|
||||
---
|
||||
synopsis: 'more deprecated features'
|
||||
issues: []
|
||||
cls: [2092]
|
||||
category: Breaking Changes
|
||||
credits: [piegames, commentator2.0]
|
||||
---
|
||||
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.
|
||||
@@ -247,6 +247,7 @@ struct string : _string, seq<
|
||||
> {};
|
||||
|
||||
struct _ind_string {
|
||||
struct strip_first_line : seq<star<one<' '>>, one<'\n'>> {};
|
||||
struct line_start : semantic, star<one<' '>> {};
|
||||
template<typename... Inner>
|
||||
struct literal : semantic, seq<Inner...> {};
|
||||
@@ -264,7 +265,7 @@ struct _ind_string {
|
||||
struct ind_string : _ind_string, seq<
|
||||
TAO_PEGTL_STRING("''"),
|
||||
// Strip first line completely if empty
|
||||
opt<star<one<' '>>, one<'\n'>>,
|
||||
opt<_ind_string::strip_first_line>,
|
||||
list<
|
||||
seq<
|
||||
// Start a line with some indentation
|
||||
|
||||
@@ -656,9 +656,20 @@ template<> struct BuildAST<grammar::v1::string> : change_head<StringState> {
|
||||
struct IndStringState : SubexprState {
|
||||
using SubexprState::SubexprState;
|
||||
|
||||
// If the first line (after the '') is empty it gets completely removed.
|
||||
// We track that in the grammar because no need to process it any further,
|
||||
// but we still require the information to know the actual number of lines
|
||||
// in the string.
|
||||
bool firstLineStripped = false;
|
||||
std::vector<IndStringLine> lines;
|
||||
};
|
||||
|
||||
template<> struct BuildAST<grammar::v1::ind_string::strip_first_line> {
|
||||
static void apply(const auto & in, IndStringState & s, State & ps) {
|
||||
s.firstLineStripped = true;
|
||||
}
|
||||
};
|
||||
|
||||
template<> struct BuildAST<grammar::v1::ind_string::line_start> {
|
||||
static void apply(const auto & in, IndStringState & s, State & ps) {
|
||||
s.lines.push_back(IndStringLine { in.string_view(), ps.at(in) });
|
||||
@@ -717,6 +728,16 @@ template<> struct BuildAST<grammar::v1::ind_string::nul> {
|
||||
|
||||
template<> struct BuildAST<grammar::v1::ind_string> : change_head<IndStringState> {
|
||||
static void success(const auto & in, IndStringState & s, ExprState & e, State & ps) {
|
||||
if (!ps.featureSettings.isEnabled(Dep::BrokenStringIndent)) {
|
||||
/* Check for semantically incorrect code: Single-line string with indentation */
|
||||
if (s.lines.size() == 1 && !s.firstLineStripped && s.lines.front().indentation.size() > 0) {
|
||||
ps.badSingleLineIndStringFound(ps.at(in));
|
||||
}
|
||||
/* Check for semantically incorrect code: Multi-line string with text on the first line */
|
||||
if (s.lines.size() > 1 && !s.firstLineStripped) {
|
||||
ps.badFirstLineIndStringFound(ps.at(in));
|
||||
}
|
||||
}
|
||||
e.pushExpr(noPos, ps.stripIndentation(ps.at(in), std::move(s.lines)));
|
||||
}
|
||||
};
|
||||
|
||||
@@ -36,6 +36,8 @@ struct State
|
||||
void dupAttr(Symbol attr, const PosIdx pos, const PosIdx prevPos);
|
||||
void overridesFound(const PosIdx pos);
|
||||
void badLineEndingFound(const PosIdx pos, bool warnOnly);
|
||||
void badFirstLineIndStringFound(const PosIdx pos);
|
||||
void badSingleLineIndStringFound(const PosIdx pos);
|
||||
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);
|
||||
@@ -95,6 +97,31 @@ inline void State::overridesFound(const PosIdx pos) {
|
||||
});
|
||||
}
|
||||
|
||||
// Both added 2026-01-30. Probably won't turn this one into an error for a while,
|
||||
|
||||
// as this has quite a lot of use in the wild. But it's clearly wrong code,
|
||||
// so we should warn users about it.
|
||||
// See the documentation on deprecated features for more details.
|
||||
inline void State::badSingleLineIndStringFound(const PosIdx pos)
|
||||
{
|
||||
logWarning({
|
||||
.msg = HintFmt(
|
||||
"Whitespace in a ''-string will be stripped even if the string only has a single line, which is most likely not the intent of the code. To fix this, remove the whitespace or replace the string with \" instead. Use %s to silence this warning.",
|
||||
"--extra-deprecated-features broken-string-indentation"
|
||||
),
|
||||
.pos = positions[pos],
|
||||
});
|
||||
}
|
||||
inline void State::badFirstLineIndStringFound(const PosIdx pos)
|
||||
{
|
||||
logWarning({
|
||||
.msg = HintFmt(
|
||||
"Whitespace calculations for indentation stripping in a multiline ''-string include the first line, so putting text on it will effectively disable all indentation stripping. To fix this, simply break the line right after the string starts. Use %s to silence this warning.",
|
||||
"--extra-deprecated-features broken-string-indentation"
|
||||
),
|
||||
.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)
|
||||
{
|
||||
|
||||
@@ -0,0 +1,23 @@
|
||||
---
|
||||
name: broken-string-indentation
|
||||
internalName: BrokenStringIndent
|
||||
timeline:
|
||||
- date: 2026-01-30
|
||||
release: 2.95.0
|
||||
cls: [2092]
|
||||
message: Introduced as a warning.
|
||||
---
|
||||
Allow indented strings (those starting with `''`) even when the indentation stripping will produce incorrect and probably unintended results.
|
||||
Affected strings are:
|
||||
|
||||
- Single line indented strings that start with whitespace: `'' foo''` will be stripped of its leading space to `foo`.
|
||||
To fix this, convert the string to `"` or manually concatenate in the leading whitespace.
|
||||
- Multi line indented strings with text on the first line:
|
||||
```
|
||||
''foo
|
||||
bar
|
||||
''
|
||||
|
||||
```
|
||||
Having text on the first line here will completely disable the indentation stripping, which is unlikely desired.
|
||||
To fix this, move the contents of the first line down by one line.
|
||||
@@ -175,6 +175,7 @@ experimental_feature_definitions = files(
|
||||
deprecated_feature_definitions = files(
|
||||
# keep-sorted start
|
||||
'deprecated-features/ancient-let.md',
|
||||
'deprecated-features/broken-string-indentation.md',
|
||||
'deprecated-features/cr-line-endings.md',
|
||||
'deprecated-features/nix-path-shadow.md',
|
||||
'deprecated-features/nul-bytes.md',
|
||||
|
||||
@@ -0,0 +1,18 @@
|
||||
warning: Whitespace calculations for indentation stripping in a multiline ''-string include the first line, so putting text on it will effectively disable all indentation stripping. To fix this, simply break the line right after the string starts. Use --extra-deprecated-features broken-string-indentation to silence this warning.
|
||||
at /pwd/in.nix:19:8:
|
||||
18|
|
||||
19| s2 = '' If the string starts with whitespace
|
||||
| ^
|
||||
20| followed by a newline, it's stripped, but
|
||||
warning: Whitespace calculations for indentation stripping in a multiline ''-string include the first line, so putting text on it will effectively disable all indentation stripping. To fix this, simply break the line right after the string starts. Use --extra-deprecated-features broken-string-indentation to silence this warning.
|
||||
at /pwd/in.nix:55:8:
|
||||
54|
|
||||
55| s8 = '' ${""}
|
||||
| ^
|
||||
56| This shows a hacky way to preserve an empty line after the start.
|
||||
warning: Whitespace in a ''-string will be stripped even if the string only has a single line, which is most likely not the intent of the code. To fix this, remove the whitespace or replace the string with " instead. Use --extra-deprecated-features broken-string-indentation to silence this warning.
|
||||
at /pwd/in.nix:102:9:
|
||||
101|
|
||||
102| s12 = '' '';
|
||||
| ^
|
||||
103|
|
||||
@@ -0,0 +1 @@
|
||||
[ "This is an indented multi-line string\nliteral. An amount of whitespace at\nthe start of each line matching the minimum\nindentation of all lines in the string\nliteral together will be removed. Thus,\nin this case four spaces will be\nstripped from each line, even though\n THIS LINE is indented six spaces.\n\nAlso, empty lines don't count in the\ndetermination of the indentation level (the\nprevious empty line has indentation 0, but\nit doesn't matter).\n" "If the string starts with whitespace\n followed by a newline, it's stripped, but\n that's not the case here. Two spaces are\n stripped because of the \" \" at the start. \n" "This line is indented\na bit further.\n" "Anti-quotations, like so, are\nalso allowed.\n" " The \\ is not special here.\n' can be followed by any character except another ', e.g. 'x'.\nLikewise for $, e.g. $$ or $varName.\nBut ' followed by ' is special, as is $ followed by {.\nIf you want them, use anti-quotations: '', \${.\n" " Tabs are not interpreted as whitespace (since we can't guess\n what tab settings are intended), so don't use them.\n\tThis line starts with a space and a tab, so only one\n space will be stripped from each line.\n" "Also note that if the last line (just before the closing ' ')\nconsists only of whitespace, it's ignored. But here there is\nsome non-whitespace stuff, so the line isn't removed. " "\nThis shows a hacky way to preserve an empty line after the start.\nBut there's no reason to do so: you could just repeat the empty\nline.\n" " Similarly you can force an indentation level,\n in this case to 2 spaces. This works because the anti-quote\n is significant (not whitespace).\n" "" "" "" "start on network-interfaces\n\nstart script\n\n rm -f /var/run/opengl-driver\n ln -sf 123 /var/run/opengl-driver\n\n rm -f /var/log/slim.log\n \nend script\n\nenv SLIM_CFGFILE=abc\nenv SLIM_THEMESDIR=def\nenv FONTCONFIG_FILE=/etc/fonts/fonts.conf \t\t\t\t# !!! cleanup\nenv XKB_BINDIR=foo/bin \t\t\t\t# Needed for the Xkb extension.\nenv LD_LIBRARY_PATH=libX11/lib:libXext/lib:/usr/lib/ # related to xorg-sys-opengl - needed to load libglx for (AI)GLX support (for compiz)\n\nenv XORG_DRI_DRIVER_PATH=nvidiaDrivers/X11R6/lib/modules/drivers/ \n\nexec slim/bin/slim\n" "Escaping of ' followed by ': ''\nEscaping of $ followed by {: \${\nAnd finally to interpret \\n etc. as in a string: \n, \r, \t.\n" "foo\n'bla'\nbar\n" "cut -d $'\\t' -f 1\n" "ending dollar $$\n" " Lines without any indentation effectively disable the indentation\n stripping for the entire string:\n\n cat >$out/foo/data <<EOF\n lasjdöaxnasd\nasdom 12398\nä\"§Æẞ¢«»”alsd\nEOF\n" "Empty lines with a bit of whitespace don't affect the indentation calculation:\n\nAnd empty lines with more whitespace will have whitespace in the string:\n \nUnless it's the last line:\n" " Indentation stripping\n must not be impressed by\nthe last line not being empty" "\t Nor by people\n weirdly mixing tabs\n\tand spaces\n\t" ]
|
||||
@@ -0,0 +1,13 @@
|
||||
[[test]]
|
||||
runner = "eval-okay"
|
||||
matrix = true
|
||||
flags = ["--extra-deprecated-features", "broken-string-indentation"]
|
||||
|
||||
[[test]]
|
||||
name = "depr-warning"
|
||||
runner = "eval-okay"
|
||||
in = "in.nix"
|
||||
|
||||
[[test]]
|
||||
runner = "parse-okay"
|
||||
flags = ["--extra-deprecated-features", "broken-string-indentation"]
|
||||
@@ -9,7 +9,10 @@
|
||||
let
|
||||
inherit (lib) concatStringsSep;
|
||||
|
||||
deprecatedFeatures = [ ];
|
||||
# FIXME: All of these are fixed in Nixpkgs already, so clear the list on the next `nixpkgs-regression` bump
|
||||
deprecatedFeatures = [
|
||||
"broken-string-indentation"
|
||||
];
|
||||
in
|
||||
|
||||
runCommand "eval-nixos"
|
||||
|
||||
@@ -18,7 +18,10 @@ let
|
||||
inherit (lib) concatStringsSep optionals;
|
||||
|
||||
# Deprecated features to enable while running the nixpkgs test suite
|
||||
deprecatedFeatures = [ ];
|
||||
# FIXME: All of these are fixed in Nixpkgs already, so clear the list on the next `nixpkgs` bump
|
||||
deprecatedFeatures = [
|
||||
"broken-string-indentation"
|
||||
];
|
||||
|
||||
env.NIX_CONFIG = "extra-deprecated-features = ${concatStringsSep " " deprecatedFeatures}";
|
||||
in
|
||||
|
||||
Reference in New Issue
Block a user