Fix handling of experimental features mid-parse

If we conditionally "declare" the argument, as we did before, based upon
weather the feature is enabled, commands like

    nix --experimental-features=foo ... --thing-gated-on-foo

won't work, because the experimental feature isn't enabled until *after*
we start parsing.

Instead, allow arguments to also be associated with experimental
features (just as we did for builtins and settings), and then the
command line parser will filter out the experimental ones.

Since the effects of arguments (handler functions) are performed right
away, we get the required behavior: earlier arguments can enable later
arguments enabled!

There is just one catch: we want to keep non-positional
flags...non-positional. So if

    nix --experimental-features=foo ... --thing-gated-on-foo

works, then

    nix --thing-gated-on-foo --experimental-features=foo ...

should also work.

This is not my favorite long-term solution, but for now this is
implemented by delaying the requirement of needed experimental features
until *after* all the arguments have been parsed.
This commit is contained in:
John Ericson 2023-01-16 19:13:31 -05:00
parent aa663b7e89
commit 4607ac7aed
5 changed files with 53 additions and 16 deletions

View file

@ -52,7 +52,7 @@ std::shared_ptr<Completions> completions;
std::string completionMarker = "___COMPLETE___"; std::string completionMarker = "___COMPLETE___";
std::optional<std::string> needsCompletion(std::string_view s) static std::optional<std::string> needsCompletion(std::string_view s)
{ {
if (!completions) return {}; if (!completions) return {};
auto i = s.find(completionMarker); auto i = s.find(completionMarker);
@ -120,6 +120,12 @@ void Args::parseCmdline(const Strings & _cmdline)
if (!argsSeen) if (!argsSeen)
initialFlagsProcessed(); initialFlagsProcessed();
/* Now that we are done parsing, make sure that any experimental
* feature required by the flags is enabled */
for (auto & f : flagExperimentalFeatures)
experimentalFeatureSettings.require(f);
} }
bool Args::processFlag(Strings::iterator & pos, Strings::iterator end) bool Args::processFlag(Strings::iterator & pos, Strings::iterator end)
@ -128,12 +134,18 @@ bool Args::processFlag(Strings::iterator & pos, Strings::iterator end)
auto process = [&](const std::string & name, const Flag & flag) -> bool { auto process = [&](const std::string & name, const Flag & flag) -> bool {
++pos; ++pos;
if (auto & f = flag.experimentalFeature)
flagExperimentalFeatures.insert(*f);
std::vector<std::string> args; std::vector<std::string> args;
bool anyCompleted = false; bool anyCompleted = false;
for (size_t n = 0 ; n < flag.handler.arity; ++n) { for (size_t n = 0 ; n < flag.handler.arity; ++n) {
if (pos == end) { if (pos == end) {
if (flag.handler.arity == ArityAny || anyCompleted) break; if (flag.handler.arity == ArityAny || anyCompleted) break;
throw UsageError("flag '%s' requires %d argument(s)", name, flag.handler.arity); throw UsageError(
"flag '%s' requires %d argument(s), but only %d were given",
name, flag.handler.arity, n);
} }
if (auto prefix = needsCompletion(*pos)) { if (auto prefix = needsCompletion(*pos)) {
anyCompleted = true; anyCompleted = true;
@ -152,8 +164,12 @@ bool Args::processFlag(Strings::iterator & pos, Strings::iterator end)
for (auto & [name, flag] : longFlags) { for (auto & [name, flag] : longFlags) {
if (!hiddenCategories.count(flag->category) if (!hiddenCategories.count(flag->category)
&& hasPrefix(name, std::string(*prefix, 2))) && hasPrefix(name, std::string(*prefix, 2)))
{
if (auto & f = flag->experimentalFeature)
flagExperimentalFeatures.insert(*f);
completions->add("--" + name, flag->description); completions->add("--" + name, flag->description);
} }
}
return false; return false;
} }
auto i = longFlags.find(std::string(*pos, 2)); auto i = longFlags.find(std::string(*pos, 2));
@ -172,6 +188,7 @@ bool Args::processFlag(Strings::iterator & pos, Strings::iterator end)
if (prefix == "-") { if (prefix == "-") {
completions->add("--"); completions->add("--");
for (auto & [flagName, flag] : shortFlags) for (auto & [flagName, flag] : shortFlags)
if (experimentalFeatureSettings.isEnabled(flag->experimentalFeature))
completions->add(std::string("-") + flagName, flag->description); completions->add(std::string("-") + flagName, flag->description);
} }
} }
@ -219,6 +236,8 @@ nlohmann::json Args::toJSON()
auto flags = nlohmann::json::object(); auto flags = nlohmann::json::object();
for (auto & [name, flag] : longFlags) { for (auto & [name, flag] : longFlags) {
/* Skip experimental flags when listing flags. */
if (!experimentalFeatureSettings.isEnabled(flag->experimentalFeature)) continue;
auto j = nlohmann::json::object(); auto j = nlohmann::json::object();
if (flag->aliases.count(name)) continue; if (flag->aliases.count(name)) continue;
if (flag->shortName) if (flag->shortName)

View file

@ -117,6 +117,8 @@ protected:
Handler handler; Handler handler;
std::function<void(size_t, std::string_view)> completer; std::function<void(size_t, std::string_view)> completer;
std::optional<ExperimentalFeature> experimentalFeature;
static Flag mkHashTypeFlag(std::string && longName, HashType * ht); static Flag mkHashTypeFlag(std::string && longName, HashType * ht);
static Flag mkHashTypeOptFlag(std::string && longName, std::optional<HashType> * oht); static Flag mkHashTypeOptFlag(std::string && longName, std::optional<HashType> * oht);
}; };
@ -188,6 +190,16 @@ public:
friend class MultiCommand; friend class MultiCommand;
MultiCommand * parent = nullptr; MultiCommand * parent = nullptr;
private:
/**
* Experimental features needed when parsing args. These are checked
* after flag parsing is completed in order to support enabling
* experimental features coming after the flag that needs the
* experimental feature.
*/
std::set<ExperimentalFeature> flagExperimentalFeatures;
}; };
/* A command is an argument parser that can be executed by calling its /* A command is an argument parser that can be executed by calling its
@ -253,8 +265,6 @@ enum CompletionType {
}; };
extern CompletionType completionType; extern CompletionType completionType;
std::optional<std::string> needsCompletion(std::string_view s);
void completePath(size_t, std::string_view prefix); void completePath(size_t, std::string_view prefix);
void completeDir(size_t, std::string_view prefix); void completeDir(size_t, std::string_view prefix);

View file

@ -170,10 +170,14 @@ std::string Config::toKeyValue()
void Config::convertToArgs(Args & args, const std::string & category) void Config::convertToArgs(Args & args, const std::string & category)
{ {
for (auto & s : _settings) for (auto & s : _settings) {
if (applicable(s.second)) /* We do include args for settings gated on disabled
experimental-features. The args themselves however will also be
gated on any experimental feature the underlying setting is. */
if (!s.second.isAlias)
s.second.setting->convertToArg(args, category); s.second.setting->convertToArg(args, category);
} }
}
AbstractSetting::AbstractSetting( AbstractSetting::AbstractSetting(
const std::string & name, const std::string & name,
@ -219,6 +223,7 @@ void BaseSetting<T>::convertToArg(Args & args, const std::string & category)
.category = category, .category = category,
.labels = {"value"}, .labels = {"value"},
.handler = {[this](std::string s) { overridden = true; set(s); }}, .handler = {[this](std::string s) { overridden = true; set(s); }},
.experimentalFeature = experimentalFeature,
}); });
if (isAppendable()) if (isAppendable())
@ -228,6 +233,7 @@ void BaseSetting<T>::convertToArg(Args & args, const std::string & category)
.category = category, .category = category,
.labels = {"value"}, .labels = {"value"},
.handler = {[this](std::string s) { overridden = true; set(s, true); }}, .handler = {[this](std::string s) { overridden = true; set(s, true); }},
.experimentalFeature = experimentalFeature,
}); });
} }
@ -279,13 +285,15 @@ template<> void BaseSetting<bool>::convertToArg(Args & args, const std::string &
.longName = name, .longName = name,
.description = fmt("Enable the `%s` setting.", name), .description = fmt("Enable the `%s` setting.", name),
.category = category, .category = category,
.handler = {[this]() { override(true); }} .handler = {[this]() { override(true); }},
.experimentalFeature = experimentalFeature,
}); });
args.addFlag({ args.addFlag({
.longName = "no-" + name, .longName = "no-" + name,
.description = fmt("Disable the `%s` setting.", name), .description = fmt("Disable the `%s` setting.", name),
.category = category, .category = category,
.handler = {[this]() { override(false); }} .handler = {[this]() { override(false); }},
.experimentalFeature = experimentalFeature,
}); });
} }

View file

@ -450,7 +450,6 @@ template<class C> Strings quoteStrings(const C & c)
return res; return res;
} }
/* Remove trailing whitespace from a string. FIXME: return /* Remove trailing whitespace from a string. FIXME: return
std::string_view. */ std::string_view. */
std::string chomp(std::string_view s); std::string chomp(std::string_view s);

View file

@ -54,12 +54,11 @@ static bool haveInternet()
std::string programPath; std::string programPath;
struct HelpRequested { };
struct NixArgs : virtual MultiCommand, virtual MixCommonArgs struct NixArgs : virtual MultiCommand, virtual MixCommonArgs
{ {
bool useNet = true; bool useNet = true;
bool refresh = false; bool refresh = false;
bool helpRequested = false;
bool showVersion = false; bool showVersion = false;
NixArgs() : MultiCommand(RegisterCommand::getCommandsFor({})), MixCommonArgs("nix") NixArgs() : MultiCommand(RegisterCommand::getCommandsFor({})), MixCommonArgs("nix")
@ -74,7 +73,7 @@ struct NixArgs : virtual MultiCommand, virtual MixCommonArgs
.longName = "help", .longName = "help",
.description = "Show usage information.", .description = "Show usage information.",
.category = miscCategory, .category = miscCategory,
.handler = {[&]() { throw HelpRequested(); }}, .handler = {[this]() { this->helpRequested = true; }},
}); });
addFlag({ addFlag({
@ -337,7 +336,11 @@ void mainWrapped(int argc, char * * argv)
try { try {
args.parseCmdline(argvToStrings(argc, argv)); args.parseCmdline(argvToStrings(argc, argv));
} catch (HelpRequested &) { } catch (UsageError &) {
if (!args.helpRequested && !completions) throw;
}
if (args.helpRequested) {
std::vector<std::string> subcommand; std::vector<std::string> subcommand;
MultiCommand * command = &args; MultiCommand * command = &args;
while (command) { while (command) {
@ -349,8 +352,6 @@ void mainWrapped(int argc, char * * argv)
} }
showHelp(subcommand, args); showHelp(subcommand, args);
return; return;
} catch (UsageError &) {
if (!completions) throw;
} }
if (completions) { if (completions) {