Overhaul completions, redo NixOS#6693

As I complained in
NixOS#6784 (comment) (a
comment on the wrong PR, sorry again!), NixOS#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 NixOS#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).

src/libcmd/command.hh

@@ -33,21 +33,28 @@ struct NixMultiCommand : virtual MultiCommand, virtual Command
 // For the overloaded run methods
 #pragma GCC diagnostic ignored "-Woverloaded-virtual"
 
-/* A command that requires a Nix store. */
+/**
+ * A command that requires a \ref Store "Nix store".
+ */
 struct StoreCommand : virtual Command
 {
     StoreCommand();
     void run() override;
     ref<Store> getStore();
     virtual ref<Store> createStore();
+    /**
+     * Main entry point, with a Store provided
+     */
     virtual void run(ref<Store>) = 0;
 
 private:
     std::shared_ptr<Store> _store;
 };
 
-/* A command that copies something between `--from` and `--to`
-   stores. */
+/**
+ * A command that copies something between `--from` and `--to`
+ * \ref Store stores.
+ */
 struct CopyCommand : virtual StoreCommand
 {
     std::string srcUri, dstUri;
@@ -59,6 +66,9 @@ struct CopyCommand : virtual StoreCommand
     ref<Store> getDstStore();
 };
 
+/**
+ * A command that needs to evaluate Nix language expressions.
+ */
 struct EvalCommand : virtual StoreCommand, MixEvalArgs
 {
     bool startReplOnEvalErrors = false;
@@ -78,20 +88,26 @@ private:
     std::shared_ptr<EvalState> evalState;
 };
 
+/**
+ * A mixin class for commands that process flakes, adding a few standard flake-related
+ * options/flags.
+ */
 struct MixFlakeOptions : virtual Args, EvalCommand
 {
     flake::LockFlags lockFlags;
 
-    std::optional<std::string> needsFlakeInputCompletion = {};
-
     MixFlakeOptions();
 
-    virtual std::vector<std::string> getFlakesForCompletion()
+    /**
+     * The completion for some of these flags depends on the flake(s) in
+     * question.
+     *
+     * This method should be implemented to gather all flakerefs the command is
+     * operating with (presumably specified via some other arguments) so that
+     * the completions for these flags can use them.
+     */
+    virtual std::vector<FlakeRef> getFlakeRefsForCompletion()
     { return {}; }
-
-    void completeFlakeInput(std::string_view prefix);
-
-    void completionHook() override;
 };
 
 struct SourceExprCommand : virtual Args, MixFlakeOptions
@@ -111,15 +127,34 @@ struct SourceExprCommand : virtual Args, MixFlakeOptions
 
     virtual Strings getDefaultFlakeAttrPathPrefixes();
 
-    void completeInstallable(std::string_view prefix);
+    /**
+     * Complete an installable from the given prefix.
+     */
+    void completeInstallable(AddCompletions & completions, std::string_view prefix);
+
+    /**
+     * Convenience wrapper around the underlying function to make setting the
+     * callback easier.
+     */
+    CompleterClosure getCompleteInstallable();
 };
 
+/**
+ * A mixin class for commands that need a read-only flag.
+ *
+ * What exactly is "read-only" is unspecified, but it will usually be the \ref
+ * Store "Nix store".
+ */
 struct MixReadOnlyOption : virtual Args
 {
     MixReadOnlyOption();
 };
 
-/* Like InstallablesCommand but the installables are not loaded */
+/**
+ * Like InstallablesCommand but the installables are not loaded.
+ *
+ * This is needed by CmdRepl which wants to load (and reload) the installables itself.
+ */
 struct RawInstallablesCommand : virtual Args, SourceExprCommand
 {
     RawInstallablesCommand();
@@ -133,22 +168,27 @@ struct RawInstallablesCommand : virtual Args, SourceExprCommand
 
     bool readFromStdIn = false;
 
-    std::vector<std::string> getFlakesForCompletion() override;
+    std::vector<FlakeRef> getFlakeRefsForCompletion() override;
 
 private:
 
     std::vector<std::string> rawInstallables;
 };
-/* A command that operates on a list of "installables", which can be
-   store paths, attribute paths, Nix expressions, etc. */
+
+/**
+ * A command that operates on a list of "installables", which can be
+ * store paths, attribute paths, Nix expressions, etc.
+ */
 struct InstallablesCommand : RawInstallablesCommand
 {
     virtual void run(ref<Store> store, Installables && installables) = 0;
 
     void run(ref<Store> store, std::vector<std::string> && rawInstallables) override;
 };
 
-/* A command that operates on exactly one "installable" */
+/**
+ * A command that operates on exactly one "installable".
+ */
 struct InstallableCommand : virtual Args, SourceExprCommand
 {
     InstallableCommand();
@@ -157,9 +197,9 @@ struct InstallableCommand : virtual Args, SourceExprCommand
 
     void run(ref<Store> store) override;
 
-    std::vector<std::string> getFlakesForCompletion() override
+    std::vector<FlakeRef> getFlakeRefsForCompletion() override
     {
-        return {_installable};
+        return { parseFlakeRefWithFragment(_installable, absPath(".")).first };
     }
 
 private:
@@ -174,7 +214,12 @@ struct MixOperateOnOptions : virtual Args
     MixOperateOnOptions();
 };
 
-/* A command that operates on zero or more store paths. */
+/**
+ * A command that operates on zero or more extant store paths.
+ *
+ * If the argument the user passes is a some sort of recipe for a path not yet
+ * built, it must be built first.
+ */
 struct BuiltPathsCommand : InstallablesCommand, virtual MixOperateOnOptions
 {
 private:
@@ -206,15 +251,19 @@ struct StorePathsCommand : public BuiltPathsCommand
     void run(ref<Store> store, BuiltPaths && paths) override;
 };
 
-/* A command that operates on exactly one store path. */
+/**
+ * A command that operates on exactly one store path.
+ */
 struct StorePathCommand : public StorePathsCommand
 {
     virtual void run(ref<Store> store, const StorePath & storePath) = 0;
 
     void run(ref<Store> store, StorePaths && storePaths) override;
 };
 
-/* A helper class for registering commands globally. */
+/**
+ * A helper class for registering \ref Command commands globally.
+ */
 struct RegisterCommand
 {
     typedef std::map<std::vector<std::string>, std::function<ref<Command>()>> Commands;
@@ -274,9 +323,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,

src/libcmd/common-eval-args.cc

@@ -129,8 +129,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);
         }}
     });