Merge pull request #14644 from Radvendii/fix-14642

parser.y: properly abstract over to-be-created strings

Author: Radvendii

maintainers/flake-module.nix

@@ -79,6 +79,8 @@
               # Not supported by nixfmt
               ''^tests/functional/lang/eval-okay-deprecate-cursed-or\.nix$''
               ''^tests/functional/lang/eval-okay-attrs5\.nix$''
+              ''^tests/functional/lang/eval-fail-dynamic-attrs-inherit\.nix$''
+              ''^tests/functional/lang/eval-fail-dynamic-attrs-inherit-2\.nix$''
 
               # More syntax tests
               # These tests, or parts of them, should have been parse-* test cases.

src/libexpr/include/nix/expr/parser-state.hh

@@ -47,6 +47,79 @@ struct ParserLocation
     }
 };
 
+/**
+ * This represents a string-like parse that possibly has yet to be constructed.
+ *
+ * Examples:
+ * "foo"
+ * ${"foo" + "bar"}
+ * "foo.bar"
+ * "foo-${a}"
+ *
+ * Using this type allows us to avoid construction altogether in cases where what we actually need is the string
+ * contents. For example in foo."bar.baz", there is no need to construct an AST node for "bar.baz", but we don't know
+ * that until we bubble the value up during parsing and see that it's a node in an AttrPath.
+ */
+class ToBeStringyExpr
+{
+private:
+    using Raw = std::variant<std::monostate, std::string_view, Expr *>;
+    Raw raw;
+
+public:
+    ToBeStringyExpr() = default;
+
+    ToBeStringyExpr(std::string_view v)
+        : raw(v)
+    {
+    }
+
+    ToBeStringyExpr(Expr * expr)
+        : raw(expr)
+    {
+        assert(expr);
+    }
+
+    /**
+     * Visits the expression and invokes an overloaded functor object \ref f.
+     * If the underlying Expr has a dynamic type of ExprString the overload taking std::string_view
+     * is invoked.
+     *
+     * Used to consistently handle simple StringExpr ${"string"} as non-dynamic attributes.
+     * @see https://github.com/NixOS/nix/issues/14642
+     */
+    template<class F>
+    void visit(F && f)
+    {
+        std::visit(
+            overloaded{
+                [&](std::string_view str) { f(str); },
+                [&](Expr * expr) {
+                    ExprString * str = dynamic_cast<ExprString *>(expr);
+                    if (str)
+                        f(str->v.string_view());
+                    else
+                        f(expr);
+                },
+                [](std::monostate) { unreachable(); }},
+            raw);
+    }
+
+    /**
+     * Get or create an Expr from either an existing Expr or from a string.
+     * Delays the allocation or an AST node in case the parser only cares about string contents.
+     */
+    Expr * toExpr(Exprs & exprs)
+    {
+        return std::visit(
+            overloaded{
+                [&](std::string_view str) -> Expr * { return exprs.add<ExprString>(exprs.alloc, str); },
+                [&](Expr * expr) { return expr; },
+                [](std::monostate) -> Expr * { unreachable(); }},
+            raw);
+    }
+};
+
 struct LexerState
 {
     /**

src/libexpr/parser.y

@@ -138,7 +138,7 @@ static Expr * makeCall(Exprs & exprs, PosIdx pos, Expr * fn, Expr * arg) {
 %type <std::vector<std::pair<PosIdx, Expr *>>> string_parts_interpolated
 %type <std::vector<std::pair<PosIdx, std::variant<Expr *, StringToken>>>> ind_string_parts
 %type <Expr *> path_start
-%type <std::variant<Expr *, std::string_view>> string_parts string_attr
+%type <ToBeStringyExpr> string_parts string_attr
 %type <StringToken> attr
 %token <StringToken> ID
 %token <StringToken> STR IND_STR
@@ -297,12 +297,7 @@ expr_simple
   }
   | INT_LIT { $$ = state->exprs.add<ExprInt>($1); }
   | FLOAT_LIT { $$ = state->exprs.add<ExprFloat>($1); }
-  | '"' string_parts '"' {
-      std::visit(overloaded{
-          [&](std::string_view str) { $$ = state->exprs.add<ExprString>(state->exprs.alloc, str); },
-          [&](Expr * expr) { $$ = expr; }},
-      $2);
-  }
+  | '"' string_parts '"' { $$ = $2.toExpr(state->exprs); }
   | IND_STRING_OPEN ind_string_parts IND_STRING_CLOSE {
       $$ = state->stripIndentation(CUR_POS, $2);
   }
@@ -342,9 +337,9 @@ expr_simple
   ;
 
 string_parts
-  : STR { $$ = $1; }
-  | string_parts_interpolated { $$ = state->exprs.add<ExprConcatStrings>(state->exprs.alloc, CUR_POS, true, $1); }
-  | { $$ = std::string_view(); }
+  : STR { $$ = {$1}; }
+  | string_parts_interpolated { $$ = {state->exprs.add<ExprConcatStrings>(state->exprs.alloc, CUR_POS, true, $1)}; }
+  | { $$ = {std::string_view()}; }
   ;
 
 string_parts_interpolated
@@ -447,15 +442,15 @@ attrs
   : attrs attr { $$ = std::move($1); $$.emplace_back(state->symbols.create($2), state->at(@2)); }
   | attrs string_attr
     { $$ = std::move($1);
-      std::visit(overloaded {
+      $2.visit(overloaded{
           [&](std::string_view str) { $$.emplace_back(state->symbols.create(str), state->at(@2)); },
           [&](Expr * expr) {
-              throw ParseError({
-                  .msg = HintFmt("dynamic attributes not allowed in inherit"),
-                  .pos = state->positions[state->at(@2)]
-              });
-          }
-      }, $2);
+                throw ParseError({
+                    .msg = HintFmt("dynamic attributes not allowed in inherit"),
+                    .pos = state->positions[state->at(@2)]
+                });
+          }}
+      );
     }
   | { }
   ;
@@ -464,17 +459,17 @@ attrpath
   : attrpath '.' attr { $$ = std::move($1); $$.emplace_back(state->symbols.create($3)); }
   | attrpath '.' string_attr
     { $$ = std::move($1);
-      std::visit(overloaded {
+      $3.visit(overloaded{
           [&](std::string_view str) { $$.emplace_back(state->symbols.create(str)); },
-          [&](Expr * expr) { $$.emplace_back(expr); }
-      }, std::move($3));
+          [&](Expr * expr) { $$.emplace_back(expr); }}
+      );
     }
   | attr { $$.emplace_back(state->symbols.create($1)); }
   | string_attr
-    { std::visit(overloaded {
+    { $1.visit(overloaded{
           [&](std::string_view str) { $$.emplace_back(state->symbols.create(str)); },
-          [&](Expr * expr) { $$.emplace_back(expr); }
-      }, std::move($1));
+          [&](Expr * expr) { $$.emplace_back(expr); }}
+      );
     }
   ;
 
@@ -485,7 +480,7 @@ attr
 
 string_attr
   : '"' string_parts '"' { $$ = std::move($2); }
-  | DOLLAR_CURLY expr '}' { $$ = $2; }
+  | DOLLAR_CURLY expr '}' { $$ = {$2}; }
   ;
 
 list

tests/functional/lang/eval-fail-dynamic-attrs-inherit-2.err.exp

@@ -0,0 +1,6 @@
+error: dynamic attributes not allowed in inherit
+       at /pwd/lang/eval-fail-dynamic-attrs-inherit-2.nix:5:15:
+            4| {
+            5|   inherit (a) ${"b" + ""};
+             |               ^
+            6| }

tests/functional/lang/eval-fail-dynamic-attrs-inherit-2.nix

@@ -0,0 +1,6 @@
+let
+  a.b = 1;
+in
+{
+  inherit (a) ${"b" + ""};
+}

tests/functional/lang/eval-fail-dynamic-attrs-inherit.err.exp

@@ -0,0 +1,6 @@
+error: dynamic attributes not allowed in inherit
+       at /pwd/lang/eval-fail-dynamic-attrs-inherit.nix:5:11:
+            4| {
+            5|   inherit ${"a" + ""};
+             |           ^
+            6| }

tests/functional/lang/eval-fail-dynamic-attrs-inherit.nix

@@ -0,0 +1,6 @@
+let
+  a = 1;
+in
+{
+  inherit ${"a" + ""};
+}

tests/functional/lang/eval-fail-dynamic-attrs-let-2.err.exp

@@ -0,0 +1,5 @@
+error: dynamic attributes not allowed in let
+       at /pwd/lang/eval-fail-dynamic-attrs-let-2.nix:1:1:
+            1| let
+             | ^
+            2|   ${"${"a"}"} = 1;

tests/functional/lang/eval-fail-dynamic-attrs-let-2.nix

@@ -0,0 +1,4 @@
+let
+  ${"${"a"}"} = 1;
+in
+a

tests/functional/lang/eval-fail-dynamic-attrs-let-3.err.exp

@@ -0,0 +1,5 @@
+error: dynamic attributes not allowed in let
+       at /pwd/lang/eval-fail-dynamic-attrs-let-3.nix:1:1:
+            1| let
+             | ^
+            2|   "${"a"}" = 1;