Skip to content

Commit

Permalink
Overhaul completions, redo #6693
Browse files Browse the repository at this point in the history 
As I complained in
#6784 (comment) (a
comment on the wrong PR, sorry again!), #6693 introduced a second
completions mechanism to fix a bug. Having two completion mechanisms
isn't so nice.

As @thufschmitt also pointed out, it was a bummer to go from `FlakeRef`
to `std::string` when collecting flake refs. Now it is `FlakeRefs`
again.

The underlying issue that sought to work around was that completion of
arguments not at the end can still benefit from the information from
latter arguments.

To fix this better, we rip out that change and simply defer all
completion processing until after all the (regular, already-complete)
arguments have been passed.

In addition, I noticed the original completion logic used some global
variables. I do not like global variables, because even if they save
lines of code, they also obfuscate the architecture of the code.

I got rid of them  moved them to a new `RootArgs` class, which now has
`parseCmdline` instead of `Args`. The idea is that we have many argument
parsers from subcommands and what-not, but only one root args that owns
the other per actual parsing invocation. The state that was global is
now part of the root args instead.

This did, admittedly, add a bunch of new code. And I do feel bad about
that. So I went and added a lot of API docs to try to at least make the
current state of things clear to the next person.

--

This is needed for RFC 134 (tracking issue #7868). It was very hard to
modularize `Installable` parsing when there were two completion
arguments. I wouldn't go as far as to say it is *easy* now, but at least
it is less hard (and the completions test finally passed).

Co-authored-by: Valentin Gagarin <valentin.gagarin@tweag.io>
  • Loading branch information
@Ericson2314 @fricklerhandwerk
Ericson2314 and fricklerhandwerk committed Oct 16, 2023
1 parent 0cc8ff8 commit 5cb0fd9
Show file tree
Hide file tree
Showing 14 changed files with 320 additions and 178 deletions.
18 changes: 5 additions & 13 deletions src/libcmd/command.hh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -97,8 +97,6 @@ struct MixFlakeOptions : virtual Args, EvalCommand
{
flake::LockFlags lockFlags;

std::optional<std::string> needsFlakeInputCompletion = {};

MixFlakeOptions();

/**
Expand All @@ -109,12 +107,8 @@ struct MixFlakeOptions : virtual Args, EvalCommand
* operating with (presumably specified via some other arguments) so that
* the completions for these flags can use them.
*/
virtual std::vector<std::string> getFlakesForCompletion()
virtual std::vector<FlakeRef> getFlakeRefsForCompletion()
{ return {}; }

void completeFlakeInput(std::string_view prefix);

void completionHook() override;
};

struct SourceExprCommand : virtual Args, MixFlakeOptions
Expand Down Expand Up @@ -175,7 +169,7 @@ struct RawInstallablesCommand : virtual Args, SourceExprCommand

bool readFromStdIn = false;

std::vector<std::string> getFlakesForCompletion() override;
std::vector<FlakeRef> getFlakeRefsForCompletion() override;

private:

Expand Down Expand Up @@ -204,10 +198,7 @@ struct InstallableCommand : virtual Args, SourceExprCommand

void run(ref<Store> store) override;

std::vector<std::string> getFlakesForCompletion() override
{
return {_installable};
}
std::vector<FlakeRef> getFlakeRefsForCompletion() override;

private:

Expand Down Expand Up @@ -330,9 +321,10 @@ struct MixEnvironment : virtual Args {
void setEnviron();
};

void completeFlakeRef(ref<Store> store, std::string_view prefix);
void completeFlakeRef(AddCompletions & completions, ref<Store> store, std::string_view prefix);

void completeFlakeRefWithFragment(
AddCompletions & completions,
ref<EvalState> evalState,
flake::LockFlags lockFlags,
Strings attrPathPrefixes,
Expand Down
4 changes: 2 additions & 2 deletions src/libcmd/common-eval-args.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -132,8 +132,8 @@ MixEvalArgs::MixEvalArgs()
if (to.subdir != "") extraAttrs["dir"] = to.subdir;
fetchers::overrideRegistry(from.input, to.input, extraAttrs);
}},
.completer = {[&](size_t, std::string_view prefix) {
completeFlakeRef(openStore(), prefix);
.completer = {[&](AddCompletions & completions, size_t, std::string_view prefix) {
completeFlakeRef(completions, openStore(), prefix);
}}
});

Expand Down
114 changes: 65 additions & 49 deletions src/libcmd/installables.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,20 @@

namespace nix {

static void completeFlakeInputPath(
AddCompletions & completions,
ref<EvalState> evalState,
const std::vector<FlakeRef> & flakeRefs,
std::string_view prefix)
{
for (auto & flakeRef : flakeRefs) {
auto flake = flake::getFlake(*evalState, flakeRef, true);
for (auto & input : flake.inputs)
if (hasPrefix(input.first, prefix))
completions.add(input.first);
}
}

MixFlakeOptions::MixFlakeOptions()
{
auto category = "Common flake-related options";
Expand Down Expand Up @@ -79,8 +93,8 @@ MixFlakeOptions::MixFlakeOptions()
.handler = {[&](std::string s) {
lockFlags.inputUpdates.insert(flake::parseInputPath(s));
}},
.completer = {[&](size_t, std::string_view prefix) {
needsFlakeInputCompletion = {std::string(prefix)};
.completer = {[&](AddCompletions & completions, size_t, std::string_view prefix) {
completeFlakeInputPath(completions, getEvalState(), getFlakeRefsForCompletion(), prefix);
}}
});

Expand All @@ -95,11 +109,12 @@ MixFlakeOptions::MixFlakeOptions()
flake::parseInputPath(inputPath),
parseFlakeRef(flakeRef, absPath("."), true));
}},
.completer = {[&](size_t n, std::string_view prefix) {
if (n == 0)
needsFlakeInputCompletion = {std::string(prefix)};
else if (n == 1)
completeFlakeRef(getEvalState()->store, prefix);
.completer = {[&](AddCompletions & completions, size_t n, std::string_view prefix) {
if (n == 0) {
completeFlakeInputPath(completions, getEvalState(), getFlakeRefsForCompletion(), prefix);
} else if (n == 1) {
completeFlakeRef(completions, getEvalState()->store, prefix);
}
}}
});

Expand Down Expand Up @@ -146,30 +161,12 @@ MixFlakeOptions::MixFlakeOptions()
}
}
}},
.completer = {[&](size_t, std::string_view prefix) {
completeFlakeRef(getEvalState()->store, prefix);
.completer = {[&](AddCompletions & completions, size_t, std::string_view prefix) {
completeFlakeRef(completions, getEvalState()->store, prefix);
}}
});
}

void MixFlakeOptions::completeFlakeInput(std::string_view prefix)
{
auto evalState = getEvalState();
for (auto & flakeRefS : getFlakesForCompletion()) {
auto flakeRef = parseFlakeRefWithFragment(expandTilde(flakeRefS), absPath(".")).first;
auto flake = flake::getFlake(*evalState, flakeRef, true);
for (auto & input : flake.inputs)
if (hasPrefix(input.first, prefix))
completions->add(input.first);
}
}

void MixFlakeOptions::completionHook()
{
if (auto & prefix = needsFlakeInputCompletion)
completeFlakeInput(*prefix);
}

SourceExprCommand::SourceExprCommand()
{
addFlag({
Expand Down Expand Up @@ -226,11 +223,18 @@ Strings SourceExprCommand::getDefaultFlakeAttrPathPrefixes()
};
}

void SourceExprCommand::completeInstallable(std::string_view prefix)
Args::CompleterClosure SourceExprCommand::getCompleteInstallable()
{
return [this](AddCompletions & completions, size_t, std::string_view prefix) {
completeInstallable(completions, prefix);
};
}

void SourceExprCommand::completeInstallable(AddCompletions & completions, std::string_view prefix)
{
try {
if (file) {
completionType = ctAttrs;
completions.setType(AddCompletions::Type::Attrs);

evalSettings.pureEval = false;
auto state = getEvalState();
Expand Down Expand Up @@ -265,14 +269,15 @@ void SourceExprCommand::completeInstallable(std::string_view prefix)
std::string name = state->symbols[i.name];
if (name.find(searchWord) == 0) {
if (prefix_ == "")
completions->add(name);
completions.add(name);
else
completions->add(prefix_ + "." + name);
completions.add(prefix_ + "." + name);
}
}
}
} else {
completeFlakeRefWithFragment(
completions,
getEvalState(),
lockFlags,
getDefaultFlakeAttrPathPrefixes(),
Expand All @@ -285,6 +290,7 @@ void SourceExprCommand::completeInstallable(std::string_view prefix)
}

void completeFlakeRefWithFragment(
AddCompletions & completions,
ref<EvalState> evalState,
flake::LockFlags lockFlags,
Strings attrPathPrefixes,
Expand All @@ -296,9 +302,9 @@ void completeFlakeRefWithFragment(
try {
auto hash = prefix.find('#');
if (hash == std::string::npos) {
completeFlakeRef(evalState->store, prefix);
completeFlakeRef(completions, evalState->store, prefix);
} else {
completionType = ctAttrs;
completions.setType(AddCompletions::Type::Attrs);

auto fragment = prefix.substr(hash + 1);
std::string prefixRoot = "";
Expand Down Expand Up @@ -341,7 +347,7 @@ void completeFlakeRefWithFragment(
auto attrPath2 = (*attr)->getAttrPath(attr2);
/* Strip the attrpath prefix. */
attrPath2.erase(attrPath2.begin(), attrPath2.begin() + attrPathPrefix.size());
completions->add(flakeRefS + "#" + prefixRoot + concatStringsSep(".", evalState->symbols.resolve(attrPath2)));
completions.add(flakeRefS + "#" + prefixRoot + concatStringsSep(".", evalState->symbols.resolve(attrPath2)));
}
}
}
Expand All @@ -352,7 +358,7 @@ void completeFlakeRefWithFragment(
for (auto & attrPath : defaultFlakeAttrPaths) {
auto attr = root->findAlongAttrPath(parseAttrPath(*evalState, attrPath));
if (!attr) continue;
completions->add(flakeRefS + "#" + prefixRoot);
completions.add(flakeRefS + "#" + prefixRoot);
}
}
}
Expand All @@ -361,15 +367,15 @@ void completeFlakeRefWithFragment(
}
}

void completeFlakeRef(ref<Store> store, std::string_view prefix)
void completeFlakeRef(AddCompletions & completions, ref<Store> store, std::string_view prefix)
{
if (!experimentalFeatureSettings.isEnabled(Xp::Flakes))
return;

if (prefix == "")
completions->add(".");
completions.add(".");

completeDir(0, prefix);
Args::completeDir(completions, 0, prefix);

/* Look for registry entries that match the prefix. */
for (auto & registry : fetchers::getRegistries(store)) {
Expand All @@ -378,10 +384,10 @@ void completeFlakeRef(ref<Store> store, std::string_view prefix)
if (!hasPrefix(prefix, "flake:") && hasPrefix(from, "flake:")) {
std::string from2(from, 6);
if (hasPrefix(from2, prefix))
completions->add(from2);
completions.add(from2);
} else {
if (hasPrefix(from, prefix))
completions->add(from);
completions.add(from);
}
}
}
Expand Down Expand Up @@ -747,9 +753,7 @@ RawInstallablesCommand::RawInstallablesCommand()
expectArgs({
.label = "installables",
.handler = {&rawInstallables},
.completer = {[&](size_t, std::string_view prefix) {
completeInstallable(prefix);
}}
.completer = getCompleteInstallable(),
});
}

Expand All @@ -762,6 +766,17 @@ void RawInstallablesCommand::applyDefaultInstallables(std::vector<std::string> &
}
}

std::vector<FlakeRef> RawInstallablesCommand::getFlakeRefsForCompletion()
{
applyDefaultInstallables(rawInstallables);
std::vector<FlakeRef> res;
for (auto i : rawInstallables)
res.push_back(parseFlakeRefWithFragment(
expandTilde(i),
absPath(".")).first);
return res;
}

void RawInstallablesCommand::run(ref<Store> store)
{
if (readFromStdIn && !isatty(STDIN_FILENO)) {
Expand All @@ -775,10 +790,13 @@ void RawInstallablesCommand::run(ref<Store> store)
run(store, std::move(rawInstallables));
}

std::vector<std::string> RawInstallablesCommand::getFlakesForCompletion()
std::vector<FlakeRef> InstallableCommand::getFlakeRefsForCompletion()
{
applyDefaultInstallables(rawInstallables);
return rawInstallables;
return {
parseFlakeRefWithFragment(
expandTilde(_installable),
absPath(".")).first
};
}

void InstallablesCommand::run(ref<Store> store, std::vector<std::string> && rawInstallables)
Expand All @@ -794,9 +812,7 @@ InstallableCommand::InstallableCommand()
.label = "installable",
.optional = true,
.handler = {&_installable},
.completer = {[&](size_t, std::string_view prefix) {
completeInstallable(prefix);
}}
.completer = getCompleteInstallable(),
});
}

Expand Down
9 changes: 5 additions & 4 deletions src/libmain/common-args.cc
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#include "common-args.hh"
#include "args/root.hh"
#include "globals.hh"
#include "loggers.hh"

Expand Down Expand Up @@ -34,21 +35,21 @@ MixCommonArgs::MixCommonArgs(const std::string & programName)
.description = "Set the Nix configuration setting *name* to *value* (overriding `nix.conf`).",
.category = miscCategory,
.labels = {"name", "value"},
.handler = {[](std::string name, std::string value) {
.handler = {[this](std::string name, std::string value) {
try {
globalConfig.set(name, value);
} catch (UsageError & e) {
if (!completions)
if (!getRoot().completions)
warn(e.what());
}
}},
.completer = [](size_t index, std::string_view prefix) {
.completer = [](AddCompletions & completions, size_t index, std::string_view prefix) {
if (index == 0) {
std::map<std::string, Config::SettingInfo> settings;
globalConfig.getSettings(settings);
for (auto & s : settings)
if (hasPrefix(s.first, prefix))
completions->add(s.first, fmt("Set the `%s` setting.", s.first));
completions.add(s.first, fmt("Set the `%s` setting.", s.first));
}
}
});
Expand Down
3 changes: 2 additions & 1 deletion src/libmain/shared.hh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

#include "util.hh"
#include "args.hh"
#include "args/root.hh"
#include "common-args.hh"
#include "path.hh"
#include "derived-path.hh"
Expand Down Expand Up @@ -66,7 +67,7 @@ template<class N> N getIntArg(const std::string & opt,
}


struct LegacyArgs : public MixCommonArgs
struct LegacyArgs : public MixCommonArgs, public RootArgs
{
std::function<bool(Strings::iterator & arg, const Strings::iterator & end)> parseArg;

Expand Down

0 comments on commit 5cb0fd9

Please sign in to comment.