libcmd: allow setting nested attributes via --arg/--argstr

Closes #496

When running

    nix-build ../nixpkgs --arg config.allowUnfree true -A hello-unfree

the package `hello-unfree` is now built rather than getting an
eval-error rejecting instantiation. This is because `config.allowUnfree`
is now interpreted as nested attribute-set declaration, similar to how
it's done in `nix repl`.

To prevent sudden breakage, this behavior was carefully deprecated with
Nix throwing an error if the identifier for `--arg` is not a pure
identifier, but an expression as above.

Any kind of merging is rejected. I.e. doing

    nix-build ../nixpkgs --arg config '{cudaSupport = true;}' --arg config.allowUnfree true

is prohibited. That way we don't have to think about merge semantics for
cases like this (or even worse `--arg config 'rec { ... }'`). Another
nice side-effect of this is that we don't need to create an EvalState to
force the values and implement merging.

Change-Id: I8b560883a4468a3f32f915764b08f5fdd8fe71bb

doc/manual/rl-next/arg-behavior.md

@@ -0,0 +1,10 @@
+---
+synopsis: "allow setting nested attributes via `--arg`/`--argstr`"
+cls: [5338]
+category: "Features"
+credits: [ma27]
+issues: [fj#496]
+---
+
+Passing `--arg config.allowUnfree true` to e.g. `nix-build` now results in `config` with value
+`{ allowUnfree = true; }` passed to the expression.

doc/manual/src/command-ref/opt-common.md

@@ -177,6 +177,12 @@ Most commands in Lix accept the following command-line options:
   You can override this using `--arg`, e.g., `nix-env --install --attr pkgname --arg system \"i686-freebsd\"`.
   (Note that since the argument is a Nix string literal, you have to escape the quotes.)
 
+  Additionally, dots are interpreted as attribute-path separators.
+  I.e. `nix-instantiate '<nixpkgs>' -A hello-unfree --arg config.allowUnfree true` will result in an argument `config` with value `{ allowUnfree = true; }` being passed to `<nixpkgs>`.
+
+  Please note that merging of different arguments is rejected.
+  I.e. `--arg config '{ cudaSupport = true; }' --arg config.allowUnfree true` will not work whereas `--arg config.cudaSupport true --arg config.allowUnfree true` is accepted.
+
 - <span id="opt-argstr">[`--argstr`](#opt-argstr)</span> *name* *value*
 
   This option is like `--arg`, only the value is not a Nix expression but a string.

lix/libcmd/common-eval-args.cc

@@ -1,3 +1,6 @@
+#include "libexpr/value.hh"
+#include "libutil/strings.hh"
+#include "lix/libexpr/attr-path.hh"
 #include "lix/libexpr/eval-settings.hh"
 #include "lix/libcmd/common-eval-args.hh"
 #include "lix/libmain/shared.hh"
@@ -10,31 +13,10 @@
 #include "lix/libcmd/command.hh"
 #include "lix/libutil/async.hh"
 #include "lix/libutil/error.hh"
-#include "lix/libutil/regex.hh"
-
-#include <regex>
+#include <deque>
 
 namespace nix {
 
-static std::regex const identifierRegex = regex::parse("^[A-Za-z_][A-Za-z0-9_'-]*$");
-static void checkValidNixIdentifier(const std::string & name)
-{
-    std::smatch match;
-    if (!std::regex_match(name, match, identifierRegex)) {
-        throw UsageError(
-            "This invocation specifies a value for argument '%s' "
-            "which isn't a valid Nix identifier. "
-            "The project is dropping support for this so that it's possible to make e.g. "
-            "'%s' evaluating to '%s' in the future. "
-            "If you depend on this behavior, please reach out in "
-            "<https://git.lix.systems/lix-project/lix/issues/496> so we can discuss your use-case.",
-            name,
-            "--arg config.allowUnfree true",
-            "{ config.allowUnfree = true; }"
-        );
-    }
-}
-
 MixEvalArgs::MixEvalArgs()
 {
     addFlag(
@@ -42,10 +24,7 @@ MixEvalArgs::MixEvalArgs()
          .description = "Pass the value *expr* as the argument *name* to Nix functions.",
          .category = category,
          .labels = {"name", "expr"},
-         .handler = {[&](std::string name, std::string expr) {
-             checkValidNixIdentifier(name);
-             autoArgs[name] = ExprArgument(expr);
-         }}}
+         .handler = {[&](std::string name, std::string expr) { autoArgs[name] = ExprArgument(expr); }}}
     );
 
     addFlag({
@@ -53,10 +32,7 @@ MixEvalArgs::MixEvalArgs()
         .description = "Pass the string *string* as the argument *name* to Nix functions.",
         .category = category,
         .labels = {"name", "string"},
-        .handler = {[&](std::string name, std::string s) {
-            checkValidNixIdentifier(name);
-            autoArgs[name] = StringArgument(s);
-        }},
+        .handler = {[&](std::string name, std::string s) { autoArgs[name] = StringArgument(s); }},
     });
 
     addFlag({
@@ -179,9 +155,75 @@ MixEvalArgs::MixEvalArgs()
     });
 }
 
+struct AutoArgsContainer
+{
+    std::map<Symbol, std::variant<Value, AutoArgsContainer>> data;
+
+    Bindings * toBindings(Evaluator & state)
+    {
+        auto bb = state.buildBindings(data.size());
+
+        for (auto & [sym, v] : data) {
+            bb.insert(
+                sym,
+                std::visit(
+                    overloaded{
+                        [&](Value & v) { return v; },
+                        [&](AutoArgsContainer & aac) -> Value {
+                            return {NewValueAs::attrs, aac.toBindings(state)};
+                        }
+                    },
+                    v
+                )
+            );
+        }
+
+        return bb.finish();
+    }
+};
+
+static void addAutoArgRecursive(
+    AutoArgsContainer & container,
+    Evaluator & state,
+    std::vector<std::string> && path,
+    Value & val,
+    const std::string_view pathStr
+)
+{
+    auto * data = &container.data;
+    auto size = path.size();
+    for (auto [i, pathCmp] : enumerate(path)) {
+        auto next = state.symbols.create(pathCmp);
+        auto entry = data->find(next);
+
+        if (entry == data->end()) {
+            if (i == size - 1) {
+                (*data)[next] = val;
+            } else {
+                (*data)[next] = AutoArgsContainer{};
+                data = &std::get<AutoArgsContainer>((*data)[next]).data;
+            }
+        } else {
+            std::visit(
+                overloaded{
+                    [&](Value & v) {
+                        throw Error(
+                            "Cannot set %s via --arg/--argstr when it's the path-extension of another "
+                            "auto-argument!",
+                            pathStr
+                        );
+                    },
+                    [&](AutoArgsContainer & v) { data = &v.data; }
+                },
+                entry->second
+            );
+        }
+    }
+}
+
 Bindings * MixEvalArgs::getAutoArgs(Evaluator & state)
 {
-    auto res = state.buildBindings(autoArgs.size());
+    AutoArgsContainer aac;
     for (auto & [name, value] : autoArgs) {
         Value v = std::visit(
             overloaded{
@@ -193,9 +235,10 @@ Bindings * MixEvalArgs::getAutoArgs(Evaluator & state)
             value
         );
 
-        res.insert(state.symbols.create(name), v);
+        addAutoArgRecursive(aac, state, parseAttrPath(name, false), v, name);
     }
-    return res.finish();
+
+    return aac.toBindings(state);
 }
 
 kj::Promise<Result<EvalPaths::PathResult<SourcePath, ThrownError>>>

lix/libexpr/attr-path.cc

@@ -7,8 +7,7 @@
 
 namespace nix {
 
-
-std::vector<std::string> parseAttrPath(std::string_view const s)
+std::vector<std::string> parseAttrPath(std::string_view const s, bool allowRhsTrailingDot)
 {
     std::vector<std::string> res;
     std::string cur;
@@ -52,7 +51,11 @@ std::vector<std::string> parseAttrPath(std::string_view const s)
         }
         ++i;
     }
-    if (haveData) res.push_back(cur);
+    if (haveData) {
+        res.push_back(cur);
+    } else if (!allowRhsTrailingDot) {
+        throw ParseError("Trailing dot on the right-hand side of path expr '%1%' is not allowed!", s);
+    };
     return res;
 }
 

lix/libexpr/attr-path.hh

@@ -25,7 +25,7 @@ std::pair<SourcePath, uint32_t> findPackageFilename(EvalState & state, Value & v
  * Such an attr path is a dot-separated sequence of attribute names, which are possibly quoted.
  * No escaping is performed; attribute names containing double quotes are unrepresentable.
  */
-std::vector<std::string> parseAttrPath(std::string_view const s);
+std::vector<std::string> parseAttrPath(std::string_view const s, bool allowRhsTrailingDot = true);
 
 /**
  * Converts an attr path from a list of strings into a string once more.

tests/functional2/eval/test_autoargs_cli.py

@@ -0,0 +1,110 @@
+from typing import Any
+
+import pytest
+
+from testlib.fixtures.nix import Nix
+
+
+def do_evaluate(nix: Nix, args: list[str], expect_success: bool = True) -> dict[str, Any] | str:
+    res = (
+        nix.nix_instantiate(
+            [
+                "--eval",
+                "--json",
+                "-E",
+                "{ arg1, arg2 ? null }: { inherit arg1 arg2; }",
+                "--strict",
+                *args,
+            ]
+        )
+        .run()
+        .expect(0 if expect_success else 1)
+    )
+
+    if expect_success:
+        return res.json()
+    return res.stderr_s
+
+
+def test_trivial(nix: Nix):
+    res = do_evaluate(nix, args=["--arg", "arg1", "[ 1 2 3 ]", "--arg", "arg2", "1"])
+
+    assert res["arg1"] == [1, 2, 3]
+    assert res["arg2"] == 1
+
+
+def test_recursive(nix: Nix):
+    res = do_evaluate(
+        nix,
+        args=["--arg", "arg1.foo.bar", "1", "--arg", "arg1.foo.baz", "2", "--arg", "arg1.bar", "3"],
+    )
+
+    assert res["arg1"] == {"foo": {"bar": 1, "baz": 2}, "bar": 3}
+    assert res["arg2"] is None
+
+
+@pytest.mark.parametrize(
+    ("attribute_path", "expected"),
+    [("arg1", 2), ("arg1.foo", {"foo": 2}), ("arg1.foo.bar", {"foo": {"bar": 2}})],
+)
+def test_override(nix: Nix, attribute_path: str, expected: Any):
+    res = do_evaluate(nix, args=["--arg", attribute_path, "1", "--arg", attribute_path, "2"])
+
+    assert res["arg1"] == expected
+
+
+def test_quoting(nix: Nix):
+    res = do_evaluate(
+        nix,
+        args=[
+            "--arg",
+            "arg1.foo.bar",
+            "1",
+            "--arg",
+            'arg1."foo bar baz".baz',
+            "2",
+            "--arg",
+            "arg1.bar",
+            "2",
+        ],
+    )
+
+    assert res["arg1"] == {"foo": {"bar": 1}, "bar": 2, "foo bar baz": {"baz": 2}}
+    assert res["arg2"] is None
+
+
+def test_quoting_error(nix: Nix):
+    res = do_evaluate(nix, ["--arg", 'arg1."foo bar.baz', "1"], expect_success=False)
+    assert "error: missing closing quote in selection path 'arg1.\"foo bar.baz'" in res
+
+
+def test_trailing_dot(nix: Nix):
+    # This is what `parseAttrPath` from `libutil` does and is consistent with the selection path
+    # passed to e.g. `nix-build -A`.
+    res = do_evaluate(nix, args=["--arg", "arg1.bar.", "[ 1 2 3 ]"], expect_success=False)
+
+    assert (
+        "error: Trailing dot on the right-hand side of path expr 'arg1.bar.' is not allowed!" in res
+    )
+
+
+@pytest.mark.parametrize(
+    "args",
+    [
+        ["--arg", "arg1.foo", "1", "--arg", "arg1.foo.bar", "2"],
+        ["--arg", "arg1.foo.bar", "2", "--arg", "arg1.foo", "1"],
+    ],
+)
+def test_conflict(nix: Nix, args: list[str]):
+    res = do_evaluate(nix, args, expect_success=False)
+
+    assert (
+        "error: Cannot set arg1.foo.bar via --arg/--argstr when it's the path-extension of another auto-argument!"
+        in res
+    )
+
+
+@pytest.mark.parametrize("selection", ["foo..bar", "foo.bar.."])
+def test_no_empty_items(nix: Nix, selection: str):
+    res = do_evaluate(nix, ["--arg", selection, "1"], expect_success=False)
+    assert f"error: consecutive dots not allowed in selection path '{selection}'" in res