Expand more when performing history path detection

When adding a command to history, we first expand its arguments to see
if any arguments are paths which refer to files. If so, we will only
autosuggest that command from history if the files are still valid. For
example, if the user runs `rm ./file.txt` then we will remember that
`./file.txt` referred to a file, and then only autosuggest that if the file
is present again.

Prior to this change we only performed simple expansion relative to the
working directory. This change extends it to variables and tilde
expansion. For example we will now apply the same hinting for
`rm ~/file.txt`

Fixes #7582

src/fish_tests.cpp

@@ -4353,17 +4353,23 @@ void history_tests_t::test_history_path_detection() {
     }
     fclose(f);
 
-    test_environment_t vars;
-    vars.vars[L"PWD"] = tmpdir;
-    vars.vars[L"HOME"] = tmpdir;
+    std::shared_ptr<test_environment_t> vars = std::make_shared<test_environment_t>();
+    vars->vars[L"PWD"] = tmpdir;
+    vars->vars[L"HOME"] = tmpdir;
 
     history_t &history = history_t::history_with_name(L"path_detection");
-    history.add_pending_with_file_detection(L"cmd0 not/a/valid/path", tmpdir);
-    history.add_pending_with_file_detection(L"cmd1 " + filename, tmpdir);
-    history.add_pending_with_file_detection(L"cmd2 " + tmpdir + L"/" + filename, tmpdir);
+    history.add_pending_with_file_detection(L"cmd0 not/a/valid/path", vars);
+    history.add_pending_with_file_detection(L"cmd1 " + filename, vars);
+    history.add_pending_with_file_detection(L"cmd2 " + tmpdir + L"/" + filename, vars);
+    history.add_pending_with_file_detection(L"cmd3  $HOME/" + filename, vars);
+    history.add_pending_with_file_detection(L"cmd4  $HOME/notafile", vars);
+    history.add_pending_with_file_detection(L"cmd5  ~/" + filename, vars);
+    history.add_pending_with_file_detection(L"cmd6  ~/notafile", vars);
+    history.add_pending_with_file_detection(L"cmd7  ~/*f*", vars);
+    history.add_pending_with_file_detection(L"cmd8  ~/*zzz*", vars);
     history.resolve_pending();
 
-    constexpr size_t hist_size = 3;
+    constexpr size_t hist_size = 9;
     if (history.size() != hist_size) {
         err(L"history has wrong size: %lu but expected %lu", (unsigned long)history.size(), (unsigned long)hist_size);
         history.clear();
@@ -4372,9 +4378,9 @@ void history_tests_t::test_history_path_detection() {
 
     // Expected sets of paths.
     wcstring_list_t expected[hist_size] = {
+        {}, {filename}, {tmpdir + L"/" + filename}, {L"$HOME/" + filename}, {}, {L"~/" + filename},
+        {}, {},  // we do not expand globs
         {},
-        {filename},
-        {tmpdir + L"/" + filename},
     };
 
     size_t lap;

src/highlight.cpp

@@ -468,7 +468,7 @@ bool autosuggest_validate_from_history(const history_item_t &item,
     }
 
     // Did the historical command have arguments that look like paths, which aren't paths now?
-    if (!all_paths_are_valid(item.get_required_paths(), working_directory)) {
+    if (!all_paths_are_valid(item.get_required_paths(), ctx)) {
         return false;
     }
 

src/history.cpp

@@ -369,7 +369,7 @@ struct history_impl_t {
     std::unordered_map<long, wcstring> items_at_indexes(const std::vector<long> &idxs);
 
     // Sets the valid file paths for the history item with the given identifier.
-    void set_valid_file_paths(const wcstring_list_t &valid_file_paths, history_identifier_t ident);
+    void set_valid_file_paths(wcstring_list_t &&valid_file_paths, history_identifier_t ident);
 
     // Return the specified history at the specified index. 0 is the index of the current
     // commandline. (So the most recent item is at index 1.)
@@ -459,7 +459,7 @@ void history_impl_t::remove(const wcstring &str_to_remove) {
     assert(first_unwritten_new_item_index <= new_items.size());
 }
 
-void history_impl_t::set_valid_file_paths(const wcstring_list_t &valid_file_paths,
+void history_impl_t::set_valid_file_paths(wcstring_list_t &&valid_file_paths,
                                           history_identifier_t ident) {
     // 0 identifier is used to mean "not necessary".
     if (ident == 0) {
@@ -469,7 +469,7 @@ void history_impl_t::set_valid_file_paths(const wcstring_list_t &valid_file_path
     // Look for an item with the given identifier. It is likely to be at the end of new_items.
     for (auto iter = new_items.rbegin(); iter != new_items.rend(); ++iter) {
         if (iter->identifier == ident) {  // found it
-            iter->required_paths = valid_file_paths;
+            iter->required_paths = std::move(valid_file_paths);
             break;
         }
     }
@@ -1246,21 +1246,42 @@ wcstring history_session_id(const environment_t &vars) {
     return result;
 }
 
-path_list_t valid_paths(const path_list_t &paths, const wcstring &working_directory) {
+path_list_t expand_and_detect_paths(const path_list_t &paths, const environment_t &vars) {
     ASSERT_IS_BACKGROUND_THREAD();
     wcstring_list_t result;
+    wcstring working_directory = vars.get_pwd_slash();
+    operation_context_t ctx(vars, kExpansionLimitBackground);
     for (const wcstring &path : paths) {
-        if (path_is_valid(path, working_directory)) {
-            result.push_back(path);
+        // Suppress cmdsubs since we are on a background thread and don't want to execute fish
+        // script.
+        // Suppress wildcards because we want to suggest e.g. `rm *` even if the directory
+        // is empty (and so rm will fail); this is nevertheless a useful command because it
+        // confirms the directory is empty.
+        wcstring expanded_path = path;
+        if (expand_one(expanded_path, {expand_flag::skip_cmdsubst, expand_flag::skip_wildcards},
+                       ctx)) {
+            if (path_is_valid(expanded_path, working_directory)) {
+                // Note we return the original (unexpanded) path.
+                result.push_back(path);
+            }
         }
     }
     return result;
 }
 
-bool all_paths_are_valid(const path_list_t &paths, const wcstring &working_directory) {
+bool all_paths_are_valid(const path_list_t &paths, const operation_context_t &ctx) {
     ASSERT_IS_BACKGROUND_THREAD();
+    wcstring working_directory = ctx.vars.get_pwd_slash();
     for (const wcstring &path : paths) {
-        if (!path_is_valid(path, working_directory)) {
+        if (ctx.cancel_checker()) {
+            return false;
+        }
+        wcstring expanded_path = path;
+        if (!expand_one(expanded_path, {expand_flag::skip_cmdsubst, expand_flag::skip_wildcards},
+                        ctx)) {
+            return false;
+        }
+        if (!path_is_valid(expanded_path, working_directory)) {
             return false;
         }
     }
@@ -1304,7 +1325,7 @@ void history_t::remove(const wcstring &str) { impl()->remove(str); }
 void history_t::remove_ephemeral_items() { impl()->remove_ephemeral_items(); }
 
 void history_t::add_pending_with_file_detection(const wcstring &str,
-                                                const wcstring &working_dir_slash,
+                                                const std::shared_ptr<environment_t> &vars,
                                                 history_persistence_mode_t persist_mode) {
     // We use empty items as sentinels to indicate the end of history.
     // Do not allow them to be added (#6032).
@@ -1321,9 +1342,8 @@ void history_t::add_pending_with_file_detection(const wcstring &str,
     for (const node_t &node : ast) {
         if (const argument_t *arg = node.try_as<argument_t>()) {
             wcstring potential_path = arg->source(str);
-            bool unescaped = unescape_string_in_place(&potential_path, UNESCAPE_DEFAULT);
-            if (unescaped && string_could_be_path(potential_path)) {
-                potential_paths.push_back(potential_path);
+            if (string_could_be_path(potential_path)) {
+                potential_paths.push_back(std::move(potential_path));
             }
         } else if (const decorated_statement_t *stmt = node.try_as<decorated_statement_t>()) {
             // Hack hack hack - if the command is likely to trigger an exit, then don't do
@@ -1362,9 +1382,10 @@ void history_t::add_pending_with_file_detection(const wcstring &str,
         // Don't hold the lock while we perform this file detection.
         imp->add(std::move(item), true /* pending */);
         iothread_perform([=]() {
-            auto validated_paths = valid_paths(potential_paths, working_dir_slash);
+            // Don't hold the lock while we perform this file detection.
+            auto validated_paths = expand_and_detect_paths(potential_paths, *vars);
             auto imp = this->impl();
-            imp->set_valid_file_paths(validated_paths, identifier);
+            imp->set_valid_file_paths(std::move(validated_paths), identifier);
             imp->enable_automatic_saving();
         });
     } else {

src/history.h

@@ -24,6 +24,7 @@
 struct io_streams_t;
 class env_stack_t;
 class environment_t;
+class operation_context_t;
 
 // Fish supports multiple shells writing to history at once. Here is its strategy:
 //
@@ -178,9 +179,10 @@ class history_t {
     void remove_ephemeral_items();
 
     // Add a new pending history item to the end, and then begin file detection on the items to
-    // determine which arguments are paths. The item has the given \p persist_mode.
+    // determine which arguments are paths. Arguments may be expanded (e.g. with PWD and variables)
+    // using the given \p vars. The item has the given \p persist_mode
     void add_pending_with_file_detection(
-        const wcstring &str, const wcstring &working_dir_slash,
+        const wcstring &str, const std::shared_ptr<environment_t> &vars,
         history_persistence_mode_t persist_mode = history_persistence_mode_t::disk);
 
     // Resolves any pending history items, so that they may be returned in history searches.
@@ -301,14 +303,18 @@ void history_save_all();
 /// Return the prefix for the files to be used for command and read history.
 wcstring history_session_id(const environment_t &vars);
 
-/// Given a list of paths and a working directory, return the paths that are valid
-/// This does disk I/O and may only be called in a background thread
-path_list_t valid_paths(const path_list_t &paths, const wcstring &working_directory);
+/// Given a list of proposed paths and a context, perform variable and home directory expansion,
+/// and detect if the result expands to a value which is also the path to a file.
+/// Wildcard expansions are suppressed - see implementation comments for why.
+/// This is used for autosuggestion hinting. If we add an item to history, and one of its arguments
+/// refers to a file, then we only want to suggest it if there is a valid file there.
+/// This does disk I/O and may only be called in a background thread.
+path_list_t expand_and_detect_paths(const path_list_t &paths, const environment_t &vars);
 
-/// Given a list of paths and a working directory,
-/// return true if all paths in the list are valid
-/// Returns true for if paths is empty
-bool all_paths_are_valid(const path_list_t &paths, const wcstring &working_directory);
+/// Given a list of proposed paths and a context, expand each one and see if it refers to a file.
+/// Wildcard expansions are suppressed.
+/// \return true if \p paths is empty or every path is valid.
+bool all_paths_are_valid(const path_list_t &paths, const operation_context_t &ctx);
 
 /// Sets private mode on. Once in private mode, it cannot be turned off.
 void start_private_mode(env_stack_t &vars);

src/operation_context.h

@@ -60,8 +60,9 @@ class operation_context_t {
                         size_t expansion_limit = kExpansionLimitDefault);
 
     /// Construct from vars alone.
-    explicit operation_context_t(const environment_t &vars)
-        : operation_context_t(nullptr, vars, no_cancel) {}
+    explicit operation_context_t(const environment_t &vars,
+                                 size_t expansion_limit = kExpansionLimitDefault)
+        : operation_context_t(nullptr, vars, no_cancel, expansion_limit) {}
 
     ~operation_context_t();
 };

src/reader.cpp

@@ -3241,7 +3241,7 @@ void reader_data_t::handle_readline_command(readline_cmd_t c, readline_loop_stat
                     } else {
                         mode = history_persistence_mode_t::disk;
                     }
-                    history->add_pending_with_file_detection(text, vars.get_pwd_slash(), mode);
+                    history->add_pending_with_file_detection(text, vars.snapshot(), mode);
                 }
 
                 rls.finished = true;