From fffcdf8792ecb5ebcf230cc533238ca8dcc5a7f6 Mon Sep 17 00:00:00 2001 From: Johannes Altmanninger Date: Mon, 1 Feb 2021 07:08:05 +0100 Subject: [PATCH] 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 | 1 + src/fish_tests.cpp | 37 +++++++++++++++++++++++++++++++++++++ src/highlight.cpp | 44 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 82 insertions(+) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 3dcfc48d2..7e5296f2e 100644 --- a/CHANGELOG.rst +++ b/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`). diff --git a/src/fish_tests.cpp b/src/fish_tests.cpp index ae887f5fe..9772075ca 100644 --- a/src/fish_tests.cpp +++ b/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}, diff --git a/src/highlight.cpp b/src/highlight.cpp index b35196c56..db07f3111 100644 --- a/src/highlight.cpp +++ b/src/highlight.cpp @@ -784,6 +784,9 @@ class highlighter_t { // The resulting colors. using color_array_t = std::vector; 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 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()) { + 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 &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 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;