From 151e75d141616d70e121a3c3ca970a64c8b841bb Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sat, 2 Nov 2019 13:40:31 -0700 Subject: [PATCH] Autosuggestions to validate the first command, not the last command When considering an autosuggestion from history, we attempt to validate the command to ensure that we don't suggest invalid (e.g. path-dependent) commands. Prior to this fix, we would validate the last command in the command line (e.g. in `cd /bin && ./stuff` we would validate "./stuff". This doesn't really make sense; we should be validating the first command because it has the potential to change the PWD. Switch to validating the first command. Also remove some helper functions that became dead through this change. --- src/highlight.cpp | 28 ++++++++++++++++++---------- src/parse_tree.cpp | 17 ----------------- src/parse_tree.h | 10 ---------- src/tnode.h | 5 ----- 4 files changed, 18 insertions(+), 42 deletions(-) diff --git a/src/highlight.cpp b/src/highlight.cpp index 6e0398dc4..88fb88223 100644 --- a/src/highlight.cpp +++ b/src/highlight.cpp @@ -395,23 +395,31 @@ static bool has_expand_reserved(const wcstring &str) { return result; } -// Parse a command line. Return by reference the last command, and the last argument to that command -// (as a string), if any. This is used by autosuggestions. +// Parse a command line. Return by reference the first command, and the first argument to that +// command (as a string), if any. This is used to validate autosuggestions. static bool autosuggest_parse_command(const wcstring &buff, const environment_t &vars, - wcstring *out_expanded_command, wcstring *out_last_arg) { + wcstring *out_expanded_command, wcstring *out_arg) { // Parse the buffer. parse_node_tree_t parse_tree; parse_tree_from_string(buff, parse_flag_continue_after_error | parse_flag_accept_incomplete_tokens, &parse_tree, NULL); - // Find the last statement. - auto last_statement = parse_tree.find_last_node(); - if (last_statement && - plain_statement_get_expanded_command(buff, last_statement, vars, out_expanded_command)) { - // Find the last argument. If we don't get one, return an invalid node. - if (auto last_arg = parse_tree.find_last_node(last_statement)) { - *out_last_arg = last_arg.get_source(buff); + // Find the first statement. + tnode_t first_statement{}; + for (const auto &node : parse_tree) { + if (node.type == symbol_plain_statement) { + first_statement = tnode_t(&parse_tree, &node); + break; + } + } + + if (first_statement && + plain_statement_get_expanded_command(buff, first_statement, vars, out_expanded_command)) { + // Find the first argument. + auto args_and_redirs = first_statement.child<1>(); + if (auto arg = args_and_redirs.next_in_list()) { + *out_arg = arg.get_source(buff); } return true; } diff --git a/src/parse_tree.cpp b/src/parse_tree.cpp index a1070ee49..cef4a826a 100644 --- a/src/parse_tree.cpp +++ b/src/parse_tree.cpp @@ -1212,20 +1212,3 @@ const parse_node_t *parse_node_tree_t::find_node_matching_source_location( return result; } - -const parse_node_t *parse_node_tree_t::find_last_node_of_type(parse_token_type_t type, - const parse_node_t *parent) const { - const parse_node_t *result = NULL; - // Find nodes of the given type in the tree, working backwards. - size_t idx = this->size(); - while (idx--) { - const parse_node_t &node = this->at(idx); - bool expected_type = (node.type == type); - if (expected_type && (parent == NULL || node_has_ancestor(*this, node, *parent))) { - // The types match and it has the right parent. - result = &node; - break; - } - } - return result; -} diff --git a/src/parse_tree.h b/src/parse_tree.h index e0ddf63d2..d813249cb 100644 --- a/src/parse_tree.h +++ b/src/parse_tree.h @@ -181,11 +181,6 @@ class parse_node_tree_t : public std::vector { const parse_node_t *get_parent(const parse_node_t &node, parse_token_type_t expected_type = token_type_invalid) const; - // Finds the last node of a given type, or empty if it could not be found. If parent is NULL, - // this finds the last node in the tree of that type. - template - tnode_t find_last_node(const parse_node_t *parent = NULL) const; - // Finds a node containing the given source location. If 'parent' is not NULL, it must be an // ancestor. const parse_node_t *find_node_matching_source_location(parse_token_type_t type, @@ -205,11 +200,6 @@ class parse_node_tree_t : public std::vector { const parse_node_t *next_node_in_node_list(const parse_node_t &node_list, parse_token_type_t item_type, const parse_node_t **list_tail) const; - - // Finds the last node of a given type underneath a given node, or NULL if it could not be - // found. If parent is NULL, this finds the last node in the tree of that type. - const parse_node_t *find_last_node_of_type(parse_token_type_t type, - const parse_node_t *parent) const; }; /// The big entry point. Parse a string, attempting to produce a tree for the given goal type. diff --git a/src/tnode.h b/src/tnode.h index e65c8b783..e7b2939e9 100644 --- a/src/tnode.h +++ b/src/tnode.h @@ -223,11 +223,6 @@ tnode_t parse_node_tree_t::find_child(const parse_node_t &parent) const { return tnode_t(this, &this->find_child(parent, Type::token)); } -template -tnode_t parse_node_tree_t::find_last_node(const parse_node_t *parent) const { - return tnode_t(this, this->find_last_node_of_type(Type::token, parent)); -} - /// Given a plain statement, get the command from the child node. Returns the command string on /// success, none on failure. maybe_t command_for_plain_statement(tnode_t stmt,