Fix some issues with experimental config settings

Issues:

1. Features gated on disabled experimental settings should warn and be
   ignored, not silently succeed.

2. Experimental settings in the same config "batch" (file or env var)
   as the enabling of the experimental feature should work.

3. For (2), the order should not matter.

These are analogous to the issues @roberth caught with my changes for
arg handling, but they are instead for config handling.

Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>

7 files changed, 238 insertions, 66 deletions

diff --git a/src/libstore/globals.cc b/src/libstore/globals.cc
index 1b38e32fb..4c66d08ee 100644
--- a/src/libstore/globals.cc
+++ b/src/libstore/globals.cc
@@ -22,6 +22,9 @@
 #include <dlfcn.h>
 #endif
 
+#include "config-impl.hh"
+
+
 namespace nix {
 
 
@@ -192,18 +195,18 @@ NLOHMANN_JSON_SERIALIZE_ENUM(SandboxMode, {
     {SandboxMode::smDisabled, false},
 });
 
-template<> void BaseSetting<SandboxMode>::set(const std::string & str, bool append)
+template<> SandboxMode BaseSetting<SandboxMode>::parse(const std::string & str) const
 {
-    if (str == "true") value = smEnabled;
-    else if (str == "relaxed") value = smRelaxed;
-    else if (str == "false") value = smDisabled;
+    if (str == "true") return smEnabled;
+    else if (str == "relaxed") return smRelaxed;
+    else if (str == "false") return smDisabled;
     else throw UsageError("option '%s' has invalid value '%s'", name, str);
 }
 
-template<> bool BaseSetting<SandboxMode>::isAppendable()
+template<> struct BaseSetting<SandboxMode>::trait
 {
-    return false;
-}
+    static constexpr bool appendable = false;
+};
 
 template<> std::string BaseSetting<SandboxMode>::to_string() const
 {
@@ -235,23 +238,23 @@ template<> void BaseSetting<SandboxMode>::convertToArg(Args & args, const std::s
     });
 }
 
-void MaxBuildJobsSetting::set(const std::string & str, bool append)
+unsigned int MaxBuildJobsSetting::parse(const std::string & str) const
 {
-    if (str == "auto") value = std::max(1U, std::thread::hardware_concurrency());
+    if (str == "auto") return std::max(1U, std::thread::hardware_concurrency());
     else {
         if (auto n = string2Int<decltype(value)>(str))
-            value = *n;
+            return *n;
         else
             throw UsageError("configuration setting '%s' should be 'auto' or an integer", name);
     }
 }
 
 
-void PluginFilesSetting::set(const std::string & str, bool append)
+Paths PluginFilesSetting::parse(const std::string & str) const
 {
     if (pluginsLoaded)
         throw UsageError("plugin-files set after plugins were loaded, you may need to move the flag before the subcommand");
-    BaseSetting<Paths>::set(str, append);
+    return BaseSetting<Paths>::parse(str);
 }
 
 
diff --git a/src/libstore/globals.hh b/src/libstore/globals.hh
index d6c5d437a..609cf53b8 100644
--- a/src/libstore/globals.hh
+++ b/src/libstore/globals.hh
@@ -26,7 +26,7 @@ struct MaxBuildJobsSetting : public BaseSetting<unsigned int>
         options->addSetting(this);
     }
 
-    void set(const std::string & str, bool append = false) override;
+    unsigned int parse(const std::string & str) const override;
 };
 
 struct PluginFilesSetting : public BaseSetting<Paths>
@@ -43,7 +43,7 @@ struct PluginFilesSetting : public BaseSetting<Paths>
         options->addSetting(this);
     }
 
-    void set(const std::string & str, bool append = false) override;
+    Paths parse(const std::string & str) const override;
 };
 
 const uint32_t maxIdsPerBuild =
diff --git a/src/libutil/config-impl.hh b/src/libutil/config-impl.hh
new file mode 100644
index 000000000..b6cae5ec3
--- /dev/null
+++ b/src/libutil/config-impl.hh
@@ -0,0 +1,71 @@
+#pragma once
+/**
+ * @file
+ *
+ * Template implementations (as opposed to mere declarations).
+ *
+ * One only needs to include this when one is declaring a
+ * `BaseClass<CustomType>` setting, or as derived class of such an
+ * instantiation.
+ */
+
+#include "config.hh"
+
+namespace nix {
+
+template<> struct BaseSetting<Strings>::trait
+{
+    static constexpr bool appendable = true;
+};
+template<> struct BaseSetting<StringSet>::trait
+{
+    static constexpr bool appendable = true;
+};
+template<> struct BaseSetting<StringMap>::trait
+{
+    static constexpr bool appendable = true;
+};
+template<> struct BaseSetting<std::set<ExperimentalFeature>>::trait
+{
+    static constexpr bool appendable = true;
+};
+
+template<typename T>
+struct BaseSetting<T>::trait
+{
+    static constexpr bool appendable = false;
+};
+
+template<typename T>
+bool BaseSetting<T>::isAppendable()
+{
+    return trait::appendable;
+}
+
+template<> void BaseSetting<Strings>::appendOrSet(Strings && newValue, bool append);
+template<> void BaseSetting<StringSet>::appendOrSet(StringSet && newValue, bool append);
+template<> void BaseSetting<StringMap>::appendOrSet(StringMap && newValue, bool append);
+template<> void BaseSetting<std::set<ExperimentalFeature>>::appendOrSet(std::set<ExperimentalFeature> && newValue, bool append);
+
+template<typename T>
+void BaseSetting<T>::appendOrSet(T && newValue, bool append)
+{
+    static_assert(!trait::appendable, "using default `appendOrSet` implementation with an appendable type");
+    assert(!append);
+    value = std::move(newValue);
+}
+
+template<typename T>
+void BaseSetting<T>::set(const std::string & str, bool append)
+{
+    if (experimentalFeatureSettings.isEnabled(experimentalFeature))
+        appendOrSet(parse(str), append);
+    else {
+        assert(experimentalFeature);
+        warn("Ignoring setting '%s' because experimental feature '%s' is not enabled",
+            name,
+            showExperimentalFeature(*experimentalFeature));
+    }
+}
+
+}
diff --git a/src/libutil/config.cc b/src/libutil/config.cc
index a42f3a849..085a884dc 100644
--- a/src/libutil/config.cc
+++ b/src/libutil/config.cc
@@ -3,6 +3,8 @@
 #include "abstract-setting-to-json.hh"
 #include "experimental-features.hh"
 
+#include "config-impl.hh"
+
 #include <nlohmann/json.hpp>
 
 namespace nix {
@@ -80,6 +82,8 @@ void Config::getSettings(std::map<std::string, SettingInfo> & res, bool overridd
 void AbstractConfig::applyConfig(const std::string & contents, const std::string & path) {
     unsigned int pos = 0;
 
+    std::vector<std::pair<std::string, std::string>> parsedContents;
+
     while (pos < contents.size()) {
         std::string line;
         while (pos < contents.size() && contents[pos] != '\n')
@@ -125,8 +129,21 @@ void AbstractConfig::applyConfig(const std::string & contents, const std::string
         auto i = tokens.begin();
         advance(i, 2);
 
-        set(name, concatStringsSep(" ", Strings(i, tokens.end()))); // FIXME: slow
+        parsedContents.push_back({
+            name,
+            concatStringsSep(" ", Strings(i, tokens.end())),
+        });
     };
+
+    // First apply experimental-feature related settings
+    for (auto & [name, value] : parsedContents)
+        if (name == "experimental-features" || name == "extra-experimental-features")
+            set(name, value);
+
+    // Then apply other settings
+    for (auto & [name, value] : parsedContents)
+        if (name != "experimental-features" && name != "extra-experimental-features")
+            set(name, value);
 }
 
 void AbstractConfig::applyConfigFile(const Path & path)
@@ -203,12 +220,6 @@ void AbstractSetting::convertToArg(Args & args, const std::string & category)
 }
 
 template<typename T>
-bool BaseSetting<T>::isAppendable()
-{
-    return false;
-}
-
-template<typename T>
 void BaseSetting<T>::convertToArg(Args & args, const std::string & category)
 {
     args.addFlag({
@@ -231,9 +242,9 @@ void BaseSetting<T>::convertToArg(Args & args, const std::string & category)
         });
 }
 
-template<> void BaseSetting<std::string>::set(const std::string & str, bool append)
+template<> std::string BaseSetting<std::string>::parse(const std::string & str) const
 {
-    value = str;
+    return str;
 }
 
 template<> std::string BaseSetting<std::string>::to_string() const
@@ -242,11 +253,11 @@ template<> std::string BaseSetting<std::string>::to_string() const
 }
 
 template<typename T>
-void BaseSetting<T>::set(const std::string & str, bool append)
+T BaseSetting<T>::parse(const std::string & str) const
 {
     static_assert(std::is_integral<T>::value, "Integer required.");
     if (auto n = string2Int<T>(str))
-        value = *n;
+        return *n;
     else
         throw UsageError("setting '%s' has invalid value '%s'", name, str);
 }
@@ -258,12 +269,12 @@ std::string BaseSetting<T>::to_string() const
     return std::to_string(value);
 }
 
-template<> void BaseSetting<bool>::set(const std::string & str, bool append)
+template<> bool BaseSetting<bool>::parse(const std::string & str) const
 {
     if (str == "true" || str == "yes" || str == "1")
-        value = true;
+        return true;
     else if (str == "false" || str == "no" || str == "0")
-        value = false;
+        return false;
     else
         throw UsageError("Boolean setting '%s' has invalid value '%s'", name, str);
 }
@@ -291,16 +302,15 @@ template<> void BaseSetting<bool>::convertToArg(Args & args, const std::string &
     });
 }
 
-template<> void BaseSetting<Strings>::set(const std::string & str, bool append)
+template<> Strings BaseSetting<Strings>::parse(const std::string & str) const
 {
-    auto ss = tokenizeString<Strings>(str);
-    if (!append) value.clear();
-    for (auto & s : ss) value.push_back(std::move(s));
+    return tokenizeString<Strings>(str);
 }
 
-template<> bool BaseSetting<Strings>::isAppendable()
+template<> void BaseSetting<Strings>::appendOrSet(Strings && newValue, bool append)
 {
-    return true;
+    if (!append) value.clear();
+    for (auto && s : std::move(newValue)) value.push_back(std::move(s));
 }
 
 template<> std::string BaseSetting<Strings>::to_string() const
@@ -308,16 +318,16 @@ template<> std::string BaseSetting<Strings>::to_string() const
     return concatStringsSep(" ", value);
 }
 
-template<> void BaseSetting<StringSet>::set(const std::string & str, bool append)
+template<> StringSet BaseSetting<StringSet>::parse(const std::string & str) const
 {
-    if (!append) value.clear();
-    for (auto & s : tokenizeString<StringSet>(str))
-        value.insert(s);
+    return tokenizeString<StringSet>(str);
 }
 
-template<> bool BaseSetting<StringSet>::isAppendable()
+template<> void BaseSetting<StringSet>::appendOrSet(StringSet && newValue, bool append)
 {
-    return true;
+    if (!append) value.clear();
+    for (auto && s : std::move(newValue))
+        value.insert(s);
 }
 
 template<> std::string BaseSetting<StringSet>::to_string() const
@@ -325,21 +335,24 @@ template<> std::string BaseSetting<StringSet>::to_string() const
     return concatStringsSep(" ", value);
 }
 
-template<> void BaseSetting<std::set<ExperimentalFeature>>::set(const std::string & str, bool append)
+template<> std::set<ExperimentalFeature> BaseSetting<std::set<ExperimentalFeature>>::parse(const std::string & str) const
 {
-    if (!append) value.clear();
+    std::set<ExperimentalFeature> res;
     for (auto & s : tokenizeString<StringSet>(str)) {
         auto thisXpFeature = parseExperimentalFeature(s);
         if (thisXpFeature)
-            value.insert(thisXpFeature.value());
+            res.insert(thisXpFeature.value());
         else
             warn("unknown experimental feature '%s'", s);
     }
+    return res;
 }
 
-template<> bool BaseSetting<std::set<ExperimentalFeature>>::isAppendable()
+template<> void BaseSetting<std::set<ExperimentalFeature>>::appendOrSet(std::set<ExperimentalFeature> && newValue, bool append)
 {
-    return true;
+    if (!append) value.clear();
+    for (auto && s : std::move(newValue))
+        value.insert(s);
 }
 
 template<> std::string BaseSetting<std::set<ExperimentalFeature>>::to_string() const
@@ -350,20 +363,23 @@ template<> std::string BaseSetting<std::set<ExperimentalFeature>>::to_string() c
     return concatStringsSep(" ", stringifiedXpFeatures);
 }
 
-template<> void BaseSetting<StringMap>::set(const std::string & str, bool append)
+template<> StringMap BaseSetting<StringMap>::parse(const std::string & str) const
 {
-    if (!append) value.clear();
+    StringMap res;
     for (auto & s : tokenizeString<Strings>(str)) {
         auto eq = s.find_first_of('=');
         if (std::string::npos != eq)
-            value.emplace(std::string(s, 0, eq), std::string(s, eq + 1));
+            res.emplace(std::string(s, 0, eq), std::string(s, eq + 1));
         // else ignored
     }
+    return res;
 }
 
-template<> bool BaseSetting<StringMap>::isAppendable()
+template<> void BaseSetting<StringMap>::appendOrSet(StringMap && newValue, bool append)
 {
-    return true;
+    if (!append) value.clear();
+    for (auto && [k, v] : std::move(newValue))
+        value.emplace(std::move(k), std::move(v));
 }
 
 template<> std::string BaseSetting<StringMap>::to_string() const
@@ -387,15 +403,15 @@ template class BaseSetting<StringSet>;
 template class BaseSetting<StringMap>;
 template class BaseSetting<std::set<ExperimentalFeature>>;
 
-void PathSetting::set(const std::string & str, bool append)
+Path PathSetting::parse(const std::string & str) const
 {
     if (str == "") {
         if (allowEmpty)
-            value = "";
+            return "";
         else
             throw UsageError("setting '%s' cannot be empty", name);
     } else
-        value = canonPath(str);
+        return canonPath(str);
 }
 
 bool GlobalConfig::set(const std::string & name, const std::string & value)
diff --git a/src/libutil/config.hh b/src/libutil/config.hh
index 162626791..2675baed7 100644
--- a/src/libutil/config.hh
+++ b/src/libutil/config.hh
@@ -215,8 +215,11 @@ protected:
 
     virtual void set(const std::string & value, bool append = false) = 0;
 
-    virtual bool isAppendable()
-    { return false; }
+    /**
+     * Whether the type is appendable; i.e. whether the `append`
+     * parameter to `set()` is allowed to be `true`.
+     */
+    virtual bool isAppendable() = 0;
 
     virtual std::string to_string() const = 0;
 
@@ -241,6 +244,23 @@ protected:
     const T defaultValue;
     const bool documentDefault;
 
+    /**
+     * Parse the string into a `T`.
+     *
+     * Used by `set()`.
+     */
+    virtual T parse(const std::string & str) const;
+
+    /**
+     * Append or overwrite `value` with `newValue`.
+     *
+     * Some types to do not support appending in which case `append`
+     * should never be passed. The default handles this case.
+     *
+     * @param append Whether to append or overwrite.
+     */
+    virtual void appendOrSet(T && newValue, bool append);
+
 public:
 
     BaseSetting(const T & def,
@@ -268,9 +288,25 @@ public:
     template<typename U>
     void setDefault(const U & v) { if (!overridden) value = v; }
 
-    void set(const std::string & str, bool append = false) override;
+    /**
+     * Require any experimental feature the setting depends on
+     *
+     * Uses `parse()` to get the value from `str`, and `appendOrSet()`
+     * to set it.
+     */
+    void set(const std::string & str, bool append = false) override final;
 
-    bool isAppendable() override;
+    /**
+     * C++ trick; This is template-specialized to compile-time indicate whether
+     * the type is appendable.
+     */
+    struct trait;
+
+    /**
+     * Always defined based on the C++ magic
+     * with `trait` above.
+     */
+    bool isAppendable() override final;
 
     virtual void override(const T & v)
     {
@@ -336,7 +372,7 @@ public:
         options->addSetting(this);
     }
 
-    void set(const std::string & str, bool append = false) override;
+    Path parse(const std::string & str) const override;
 
     Path operator +(const char * p) const { return value + p; }
 
diff --git a/src/libutil/tests/config.cc b/src/libutil/tests/config.cc
index f250e934e..886e70da5 100644
--- a/src/libutil/tests/config.cc
+++ b/src/libutil/tests/config.cc
@@ -82,6 +82,7 @@ namespace nix {
             TestSetting() : AbstractSetting("test", "test", {}) {}
             void set(const std::string & value, bool append) override {}
             std::string to_string() const override { return {}; }
+            bool isAppendable() override { return false; }
         };
 
         Config config;
@@ -90,6 +91,7 @@ namespace nix {
         ASSERT_FALSE(config.set("test", "value"));
         config.addSetting(&setting);
         ASSERT_TRUE(config.set("test", "value"));
+        ASSERT_FALSE(config.set("extra-test", "value"));
     }
 
     TEST(Config, withInitialValue) {
diff --git a/tests/experimental-features.sh b/tests/experimental-features.sh
index 73554da8c..607bf0a8e 100644
--- a/tests/experimental-features.sh
+++ b/tests/experimental-features.sh
@@ -23,20 +23,64 @@ source common.sh
 # # Medium case, the configuration effects --help
 # grep_both_ways store gc --help
 
-expect 1 nix --experimental-features 'nix-command' show-config --flake-registry 'https://no'
-nix --experimental-features 'nix-command flakes' show-config --flake-registry 'https://no'
+# Test settings that are gated on experimental features; the setting is ignored
+# with a warning if the experimental feature is not enabled. The order of the
+# `setting = value` lines in the configuration should not matter.
+
+# 'flakes' experimental-feature is disabled before, ignore and warn
+NIX_CONFIG='
+  experimental-features = nix-command
+  accept-flake-config = true
+' nix show-config accept-flake-config 1>$TEST_ROOT/stdout 2>$TEST_ROOT/stderr
+grepQuiet "false" $TEST_ROOT/stdout
+grepQuiet "Ignoring setting 'accept-flake-config' because experimental feature 'flakes' is not enabled" $TEST_ROOT/stderr
+
+# 'flakes' experimental-feature is disabled after, ignore and warn
+NIX_CONFIG='
+  accept-flake-config = true
+  experimental-features = nix-command
+' nix show-config accept-flake-config 1>$TEST_ROOT/stdout 2>$TEST_ROOT/stderr
+grepQuiet "false" $TEST_ROOT/stdout
+grepQuiet "Ignoring setting 'accept-flake-config' because experimental feature 'flakes' is not enabled" $TEST_ROOT/stderr
+
+# 'flakes' experimental-feature is enabled before, process
+NIX_CONFIG='
+  experimental-features = nix-command flakes
+  accept-flake-config = true
+' nix show-config accept-flake-config 1>$TEST_ROOT/stdout 2>$TEST_ROOT/stderr
+grepQuiet "true" $TEST_ROOT/stdout
+grepQuietInverse "Ignoring setting 'accept-flake-config'" $TEST_ROOT/stderr
+
+# 'flakes' experimental-feature is enabled after, process
+NIX_CONFIG='
+  accept-flake-config = true
+  experimental-features = nix-command flakes
+' nix show-config accept-flake-config 1>$TEST_ROOT/stdout 2>$TEST_ROOT/stderr
+grepQuiet "true" $TEST_ROOT/stdout
+grepQuietInverse "Ignoring setting 'accept-flake-config'" $TEST_ROOT/stderr
+
+function exit_code_both_ways {
+    expect 1 nix --experimental-features 'nix-command' "$@" 1>/dev/null
+    nix --experimental-features 'nix-command flakes' "$@" 1>/dev/null
+
+    # Also, the order should not matter
+    expect 1 nix "$@" --experimental-features 'nix-command' 1>/dev/null
+    nix "$@" --experimental-features 'nix-command flakes' 1>/dev/null
+}
+
+exit_code_both_ways show-config --flake-registry 'https://no'
 
 # Double check these are stable
-nix --experimental-features '' --help
-nix --experimental-features '' doctor --help
-nix --experimental-features '' repl --help
-nix --experimental-features '' upgrade-nix --help
+nix --experimental-features '' --help 1>/dev/null
+nix --experimental-features '' doctor --help 1>/dev/null
+nix --experimental-features '' repl --help 1>/dev/null
+nix --experimental-features '' upgrade-nix --help 1>/dev/null
 
 # These 3 arguments are currently given to all commands, which is wrong (as not
 # all care). To deal with fixing later, we simply make them require the
 # nix-command experimental features --- it so happens that the commands we wish
 # stabilizing to do not need them anyways.
 for arg in '--print-build-logs' '--offline' '--refresh'; do
-    nix --experimental-features 'nix-command' "$arg" --help
-    ! nix --experimental-features '' "$arg" --help
+    nix --experimental-features 'nix-command' "$arg" --help 1>/dev/null
+    expect 1 nix --experimental-features '' "$arg" --help 1>/dev/null
 done