Reimplement autosuggestion-triggered completion loading

This concerns what happens if the user types e.g. `grep --i` and grep or
its completions have not yet been loaded. Previously we would "bounce to
the main thread" from within the autosuggestion thread to load grep's
completions. However under concurrent execution, this may deadlock as the
main thread is waiting for something else.

In the new implementation, complete simply records the commands that it
would autoload, and returns them back to the caller, where the caller can
decide how to handle them.

In general iothread_perform_on_main risks deadlock under concurrent
execution and we should try to get rid of it.

There should be no user-visible change from this fix.

src/autoload.cpp

@@ -72,6 +72,9 @@ class autoload_file_cache_t {
     /// If \p allow_stale is true, allow stale entries; otherwise discard them.
     /// This returns an autoloadable file, or none() if there is no such file.
     maybe_t<autoloadable_file_t> check(const wcstring &cmd, bool allow_stale = false);
+
+    /// \return true if a command is cached (either as a hit or miss).
+    bool is_cached(const wcstring &cmd) const;
 };
 
 maybe_t<autoloadable_file_t> autoload_file_cache_t::locate_file(const wcstring &cmd) const {
@@ -142,6 +145,10 @@ maybe_t<autoloadable_file_t> autoload_file_cache_t::check(const wcstring &cmd, b
     return file;
 }
 
+bool autoload_file_cache_t::is_cached(const wcstring &cmd) const {
+    return known_files_.count(cmd) > 0 || misses_cache_.contains(cmd);
+}
+
 autoload_t::autoload_t(wcstring env_var_name)
     : env_var_name_(std::move(env_var_name)), cache_(make_unique<autoload_file_cache_t>()) {}
 
@@ -157,6 +164,8 @@ bool autoload_t::can_autoload(const wcstring &cmd) {
     return cache_->check(cmd, true /* allow stale */).has_value();
 }
 
+bool autoload_t::has_attempted_autoload(const wcstring &cmd) { return cache_->is_cached(cmd); }
+
 wcstring_list_t autoload_t::get_autoloaded_commands() const {
     wcstring_list_t result;
     result.reserve(autoloaded_files_.size());

src/autoload.h

@@ -88,6 +88,9 @@ class autoload_t {
     /// This does not actually mark the command as being autoloaded.
     bool can_autoload(const wcstring &cmd);
 
+    /// \return whether autoloading has been attempted for a command.
+    bool has_attempted_autoload(const wcstring &cmd);
+
     /// \return the names of all commands that have been autoloaded. Note this includes "in-flight"
     /// commands.
     wcstring_list_t get_autoloaded_commands() const;

src/complete.cpp

@@ -332,6 +332,9 @@ class completer_t {
     /// The output completions.
     completion_receiver_t completions;
 
+    /// Commands which we would have tried to load, if we had a parser.
+    wcstring_list_t needs_load;
+
     /// Table of completions conditions that have already been tested and the corresponding test
     /// results.
     using condition_cache_t = std::unordered_map<wcstring, bool>;
@@ -424,6 +427,8 @@ class completer_t {
     void perform_for_commandline(wcstring cmdline);
 
     completion_list_t acquire_completions() { return completions.take(); }
+
+    wcstring_list_t acquire_needs_load() { return std::move(needs_load); }
 };
 
 // Autoloader for completions.
@@ -799,25 +804,6 @@ static size_t short_option_pos(const wcstring &arg, const option_list_t &options
     return arg.size() - 1;
 }
 
-/// Load command-specific completions for the specified command.
-static void complete_load(const wcstring &name) {
-    // We have to load this as a function, since it may define a --wraps or signature.
-    // See issue #2466.
-    auto &parser = parser_t::principal_parser();
-    function_get_props_autoload(name, parser);
-
-    // It's important to NOT hold the lock around completion loading.
-    // We need to take the lock to decide what to load, drop it to perform the load, then reacquire
-    // it.
-    // Note we only look at the global fish_function_path and fish_complete_path.
-    maybe_t<wcstring> path_to_load =
-        completion_autoloader.acquire()->resolve_command(name, env_stack_t::globals());
-    if (path_to_load) {
-        autoload_t::perform_autoload(*path_to_load, parser);
-        completion_autoloader.acquire()->mark_autoload_finished(name);
-    }
-}
-
 /// complete_param: Given a command, find completions for the argument str of command cmd_orig with
 /// previous option popt. If file completions should be disabled, then mark *out_do_file as false.
 ///
@@ -845,8 +831,10 @@ bool completer_t::complete_param_for_command(const wcstring &cmd_orig, const wcs
         // tools that do not exist. Applies to both manual completions ("cm<TAB>", "cmd <TAB>")
         // and automatic completions ("gi" autosuggestion provider -> git)
         FLOG(complete, "Skipping completions for non-existent command");
-    } else {
-        iothread_perform_on_main([&]() { complete_load(cmd); });
+    } else if (ctx.parser) {
+        complete_load(cmd, *ctx.parser);
+    } else if (!completion_autoloader.acquire()->has_attempted_autoload(cmd)) {
+        needs_load.push_back(cmd);
     }
 
     // Make a list of lists of all options that we care about.
@@ -1713,7 +1701,7 @@ void complete_remove_all(const wcstring &cmd, bool cmd_is_path) {
 }
 
 completion_list_t complete(const wcstring &cmd_with_subcmds, completion_request_options_t flags,
-                           const operation_context_t &ctx) {
+                           const operation_context_t &ctx, wcstring_list_t *out_needs_loads) {
     // Determine the innermost subcommand.
     const wchar_t *cmdsubst_begin, *cmdsubst_end;
     parse_util_cmdsubst_extent(cmd_with_subcmds.c_str(), cmd_with_subcmds.size(), &cmdsubst_begin,
@@ -1722,6 +1710,9 @@ completion_list_t complete(const wcstring &cmd_with_subcmds, completion_request_
     wcstring cmd = wcstring(cmdsubst_begin, cmdsubst_end - cmdsubst_begin);
     completer_t completer(ctx, flags);
     completer.perform_for_commandline(std::move(cmd));
+    if (out_needs_loads) {
+        *out_needs_loads = completer.acquire_needs_load();
+    }
     return completer.acquire_completions();
 }
 
@@ -1788,6 +1779,30 @@ static wcstring completion2string(const completion_key_t &key, const complete_en
     return out;
 }
 
+bool complete_load(const wcstring &cmd, parser_t &parser) {
+    bool loaded_new = false;
+
+    // We have to load this as a function, since it may define a --wraps or signature.
+    // See issue #2466.
+    if (function_load(cmd, parser)) {
+        // We autoloaded something; check if we have a --wraps.
+        loaded_new |= complete_get_wrap_targets(cmd).size() > 0;
+    }
+
+    // It's important to NOT hold the lock around completion loading.
+    // We need to take the lock to decide what to load, drop it to perform the load, then reacquire
+    // it.
+    // Note we only look at the global fish_function_path and fish_complete_path.
+    maybe_t<wcstring> path_to_load =
+        completion_autoloader.acquire()->resolve_command(cmd, env_stack_t::globals());
+    if (path_to_load) {
+        autoload_t::perform_autoload(*path_to_load, parser);
+        completion_autoloader.acquire()->mark_autoload_finished(cmd);
+        loaded_new = true;
+    }
+    return loaded_new;
+}
+
 /// Use by the bare `complete`, loaded completions are printed out as commands
 wcstring complete_print(const wcstring &cmd) {
     wcstring out;

src/complete.h

@@ -26,6 +26,7 @@ struct completion_mode_t {
 #define PROG_COMPLETE_SEP L'\t'
 
 class environment_t;
+class parser_t;
 
 enum {
     /// Do not insert space afterwards if this is the only completion. (The default is to try insert
@@ -243,10 +244,18 @@ void complete_remove(const wcstring &cmd, bool cmd_is_path, const wcstring &opti
 /// Removes all completions for a given command.
 void complete_remove_all(const wcstring &cmd, bool cmd_is_path);
 
+/// Load command-specific completions for the specified command.
+/// \return true if something new was loaded, false if not.
+bool complete_load(const wcstring &cmd, parser_t &parser);
+
 /// \return all completions of the command cmd.
+/// If \p ctx contains a parser, this will autoload functions and completions as needed.
+/// If it does not contain a parser, then any completions which need autoloading will be returned in
+/// \p needs_load, if not null.
 class operation_context_t;
 completion_list_t complete(const wcstring &cmd, completion_request_options_t flags,
-                           const operation_context_t &ctx);
+                           const operation_context_t &ctx,
+                           wcstring_list_t *out_needs_load = nullptr);
 
 /// Return a list of all current completions.
 wcstring complete_print(const wcstring &cmd = L"");

src/function.cpp

@@ -85,7 +85,7 @@ static std::shared_ptr<function_properties_t> copy_props(const function_properti
 /// Make sure that if the specified function is a dynamically loaded function, it has been fully
 /// loaded.
 /// Note this executes fish script code.
-static void try_autoload(const wcstring &name, parser_t &parser) {
+bool function_load(const wcstring &name, parser_t &parser) {
     ASSERT_IS_MAIN_THREAD();
     maybe_t<wcstring> path_to_autoload;
     // Note we can't autoload while holding the funcset lock.
@@ -99,10 +99,11 @@ static void try_autoload(const wcstring &name, parser_t &parser) {
 
     // Release the lock and perform any autoload, then reacquire the lock and clean up.
     if (path_to_autoload) {
-        // Crucially, the lock is acquired *after* do_autoload_file_at_path().
+        // Crucially, the lock is acquired after perform_autoload().
         autoload_t::perform_autoload(*path_to_autoload, parser);
         function_set.acquire()->autoloader.mark_autoload_finished(name);
     }
+    return path_to_autoload.has_value();
 }
 
 /// Insert a list of all dynamically loaded functions into the specified list.
@@ -166,7 +167,7 @@ function_properties_ref_t function_get_props(const wcstring &name) {
 function_properties_ref_t function_get_props_autoload(const wcstring &name, parser_t &parser) {
     ASSERT_IS_MAIN_THREAD();
     if (parser_keywords_is_reserved(name)) return nullptr;
-    try_autoload(name, parser);
+    function_load(name, parser);
     return function_get_props(name);
 }
 
@@ -217,7 +218,7 @@ static wcstring get_function_body_source(const function_properties_t &props) {
 
 void function_set_desc(const wcstring &name, const wcstring &desc, parser_t &parser) {
     ASSERT_IS_MAIN_THREAD();
-    try_autoload(name, parser);
+    function_load(name, parser);
     auto funcset = function_set.acquire();
     auto iter = funcset->funcs.find(name);
     if (iter != funcset->funcs.end()) {

src/function.h

@@ -74,6 +74,10 @@ function_properties_ref_t function_get_props(const wcstring &name);
 /// \return the properties for a function, or nullptr if none, perhaps triggering autoloading.
 function_properties_ref_t function_get_props_autoload(const wcstring &name, parser_t &parser);
 
+/// Try autoloading a function.
+/// \return true if something new was autoloaded, false if it was already loaded or did not exist.
+bool function_load(const wcstring &name, parser_t &parser);
+
 /// Sets the description of the function with the name \c name.
 /// This triggers autoloading.
 void function_set_desc(const wcstring &name, const wcstring &desc, parser_t &parser);

src/lru.h

@@ -169,6 +169,11 @@ class lru_cache_t {
         return &where->second.value;
     }
 
+    /// \return true if we contain a value for a key.
+    bool contains(const wcstring &key) const {
+        return this->node_map.find(key) != this->node_map.end();
+    }
+
     // Evicts the node for a given key, returning true if a node was evicted.
     bool evict_node(const wcstring &key) {
         auto where = this->node_map.find(key);

src/reader.cpp

@@ -504,6 +504,9 @@ struct autosuggestion_t {
     // The string which was searched for.
     wcstring search_string{};
 
+    // The list of completions which may need loading.
+    wcstring_list_t needs_load{};
+
     // Whether the autosuggestion should be case insensitive.
     // This is true for file-generated autosuggestions, but not for history.
     bool icase{false};
@@ -1778,18 +1781,21 @@ static std::function<autosuggestion_t(void)> get_autosuggestion_performer(
 
         // Try normal completions.
         completion_request_options_t complete_flags = completion_request_options_t::autosuggest();
-        completion_list_t completions = complete(search_string, complete_flags, ctx);
-        completions_sort_and_prioritize(&completions, complete_flags);
+        wcstring_list_t needs_load;
+        completion_list_t completions = complete(search_string, complete_flags, ctx, &needs_load);
+
+        autosuggestion_t result{};
+        result.search_string = search_string;
+        result.needs_load = std::move(needs_load);
+        result.icase = true;  // normal completions are case-insensitive.
         if (!completions.empty()) {
+            completions_sort_and_prioritize(&completions, complete_flags);
             const completion_t &comp = completions.at(0);
             size_t cursor = cursor_pos;
-            wcstring suggestion = completion_apply_to_command_line(
+            result.text = completion_apply_to_command_line(
                 comp.completion, comp.flags, search_string, &cursor, true /* append only */);
-            // Normal completions are case-insensitive.
-            return autosuggestion_t{std::move(suggestion), search_string, true /* icase */};
         }
-
-        return nothing;
+        return result;
     };
 }
 
@@ -1805,10 +1811,28 @@ bool reader_data_t::can_autosuggest() const {
 // Called after an autosuggestion has been computed on a background thread.
 void reader_data_t::autosuggest_completed(autosuggestion_t result) {
     ASSERT_IS_MAIN_THREAD();
-    if (result.search_string == in_flight_autosuggest_request)
+    if (result.search_string == in_flight_autosuggest_request) {
         in_flight_autosuggest_request.clear();
-    if (!result.empty() && can_autosuggest() && result.search_string == command_line.text() &&
-        string_prefixes_string_case_insensitive(result.search_string, result.text)) {
+    }
+    if (result.search_string != command_line.text()) {
+        // This autosuggestion is stale.
+        return;
+    }
+    // Maybe load completions for commands discovered by this autosuggestion.
+    bool loaded_new = false;
+    for (const wcstring &to_load : result.needs_load) {
+        if (complete_load(to_load, this->parser())) {
+            FLOGF(complete, "Autosuggest found new completions for %ls, restarting",
+                  to_load.c_str());
+            loaded_new = true;
+        }
+    }
+    if (loaded_new) {
+        // We loaded new completions for this command.
+        // Re-do our autosuggestion.
+        this->update_autosuggestion();
+    } else if (!result.empty() && can_autosuggest() &&
+               string_prefixes_string_case_insensitive(result.search_string, result.text)) {
         // Autosuggestion is active and the search term has not changed, so we're good to go.
         autosuggestion = std::move(result);
         if (this->is_repaint_needed()) {