Highlight redirection target as valid if it contains a to-be-defined variable

If a variable is undefined, but it looks like it will be defined by the
current command line, assume the user knows what they are doing.
This should cover most real-world occurrences.

Closes #6654

CHANGELOG.rst

@@ -268,6 +268,7 @@ Interactive improvements
 -  ``bind`` now shows ``\x7f`` for the del key instead of a literal DEL character (:issue:`7631`)
 -  Paths containing variables or tilde expansion are only suggested when they are still valid (:issue:`7582`).
 -  Syntax highlighting can now color a command as invalid even if executed quickly (:issue:`5912`)
+-  Redirection targets are no longer highlighted as error if they contain variables which will likely be defined by the current commandline (:issue:`6654`).
 -  fish is now more resilient against broken terminal modes (:issue:`7133`, :issue:`4873`).
 -  fish handles being in control of the TTY without owning its own process group better, avoiding some hangs in special configurations (:issue:`7388`).
 -  Keywords can now be colored differently by setting the ``fish_color_keyword`` variable (but ``fish_color_command`` will still be used if it is unset) (:issue:`7678`).

src/fish_tests.cpp

@@ -5085,6 +5085,43 @@ static void test_highlighting() {
         {L"param2", highlight_role_t::param},
     });
 
+    highlight_tests.push_back({
+        {L"for", highlight_role_t::keyword},
+        {L"x", highlight_role_t::param},
+        {L"in", highlight_role_t::keyword},
+        {L"set-by-for-1", highlight_role_t::param},
+        {L"set-by-for-2", highlight_role_t::param},
+        {L";", highlight_role_t::statement_terminator},
+        {L"echo", highlight_role_t::command},
+        {L">", highlight_role_t::redirection},
+        {L"$x", highlight_role_t::redirection},
+        {L";", highlight_role_t::statement_terminator},
+        {L"end", highlight_role_t::keyword},
+    });
+
+    highlight_tests.push_back({
+        {L"set", highlight_role_t::command},
+        {L"x", highlight_role_t::param},
+        {L"set-by-set", highlight_role_t::param},
+        {L";", highlight_role_t::statement_terminator},
+        {L"echo", highlight_role_t::command},
+        {L">", highlight_role_t::redirection},
+        {L"$x", highlight_role_t::redirection},
+        {L"2>", highlight_role_t::redirection},
+        {L"$totally_not_x", highlight_role_t::error},
+        {L"<", highlight_role_t::redirection},
+        {L"$x_but_its_an_impostor", highlight_role_t::error},
+    });
+
+    highlight_tests.push_back({
+        {L"x", highlight_role_t::param, ns},
+        {L"=", highlight_role_t::operat, ns},
+        {L"set-by-variable-override", highlight_role_t::param, ns},
+        {L"echo", highlight_role_t::command},
+        {L">", highlight_role_t::redirection},
+        {L"$x", highlight_role_t::redirection},
+    });
+
     highlight_tests.push_back({
         {L"end", highlight_role_t::error},
         {L";", highlight_role_t::statement_terminator},

src/highlight.cpp

@@ -784,6 +784,9 @@ class highlighter_t {
     // The resulting colors.
     using color_array_t = std::vector<highlight_spec_t>;
     color_array_t color_array;
+    // A stack of variables that the current commandline probably defines.  We mark redirections
+    // as valid if they use one of these variables, to avoid marking valid targets as error.
+    std::vector<wcstring> pending_variables;
 
     // Flags we use for AST parsing.
     static constexpr parse_tree_flags_t ast_flags =
@@ -818,6 +821,7 @@ class highlighter_t {
     void visit(const ast::variable_assignment_t &varas);
     void visit(const ast::semi_nl_t &semi_nl);
     void visit(const ast::decorated_statement_t &stmt);
+    void visit(const ast::block_statement_t &header);
 
     // Visit an argument, perhaps knowing that our command is cd.
     void visit(const ast::argument_t &arg, bool cmd_is_cd = false);
@@ -1022,6 +1026,8 @@ void highlighter_t::visit(const ast::variable_assignment_t &varas) {
     if (auto where = variable_assignment_equals_pos(varas.source(this->buff))) {
         size_t equals_loc = varas.source_range().start + *where;
         this->color_array.at(equals_loc) = highlight_role_t::operat;
+        auto var_name = varas.source(this->buff).substr(0, *where);
+        this->pending_variables.push_back(std::move(var_name));
     }
 }
 
@@ -1061,8 +1067,16 @@ void highlighter_t::visit(const ast::decorated_statement_t &stmt) {
     // Color arguments and redirections.
     // Except if our command is 'cd' we have special logic for how arguments are colored.
     bool is_cd = (expanded_cmd == L"cd");
+    bool is_set = (expanded_cmd == L"set");
     for (const ast::argument_or_redirection_t &v : stmt.args_or_redirs) {
         if (v.is_argument()) {
+            if (is_set) {
+                auto arg = v.argument().source(this->buff);
+                if (valid_var_name(arg)) {
+                    this->pending_variables.push_back(std::move(arg));
+                    is_set = false;
+                }
+            }
             this->visit(v.argument(), is_cd);
         } else {
             this->visit(v.redirection());
@@ -1070,6 +1084,20 @@ void highlighter_t::visit(const ast::decorated_statement_t &stmt) {
     }
 }
 
+void highlighter_t::visit(const ast::block_statement_t &block) {
+    this->visit(*block.header.contents.get());
+    this->visit(block.args_or_redirs);
+    const ast::node_t &bh = *block.header.contents;
+    size_t pending_variables_count = this->pending_variables.size();
+    if (const auto *fh = bh.try_as<ast::for_header_t>()) {
+        auto var_name = fh->var_name.source(this->buff);
+        pending_variables.push_back(std::move(var_name));
+    }
+    this->visit(block.jobs);
+    this->visit(block.end);
+    pending_variables.resize(pending_variables_count);
+}
+
 /// \return whether a string contains a command substitution.
 static bool has_cmdsub(const wcstring &src) {
     size_t cursor = 0;
@@ -1078,6 +1106,20 @@ static bool has_cmdsub(const wcstring &src) {
     return parse_util_locate_cmdsubst_range(src, &cursor, nullptr, &start, &end, true) != 0;
 }
 
+static bool contains_pending_variable(const std::vector<wcstring> &pending_variables,
+                                      const wcstring &haystack) {
+    for (const auto &var_name : pending_variables) {
+        size_t pos = -1;
+        while ((pos = haystack.find(var_name, pos + 1)) != wcstring::npos) {
+            if (pos == 0 || haystack.at(pos - 1) != L'$') continue;
+            size_t end = pos + var_name.size();
+            if (end < haystack.size() && valid_var_name_char(haystack.at(end))) continue;
+            return true;
+        }
+    }
+    return false;
+}
+
 void highlighter_t::visit(const ast::redirection_t &redir) {
     maybe_t<pipe_or_redir_t> oper =
         pipe_or_redir_t::from_string(redir.oper.source(this->buff));  // like 2>
@@ -1110,6 +1152,8 @@ void highlighter_t::visit(const ast::redirection_t &redir) {
             // I/O is disallowed, so we don't have much hope of catching anything but gross
             // errors. Assume it's valid.
             target_is_valid = true;
+        } else if (contains_pending_variable(this->pending_variables, target)) {
+            target_is_valid = true;
         } else if (!expand_one(target, expand_flag::skip_cmdsubst, ctx)) {
             // Could not be expanded.
             target_is_valid = false;