fish cannot detect would-be-valid variable

[mqudsi]

set tables one two three
for table in $tables
    echo foo > $table.sql
end

as $table is not a live variable at the time the loop is being composed, the redirect destination is highlighted in the error color.

This behavior is just a particular illustration of the underlying problem. I'm pretty sure the editor and the parser simply were not designed with any of this in mind, so any fixes would have to either be architectural or hacks. Or I could be wrong and this could be a cinch.

[0ion9]

mmmm... I've definitely encountered this before.. but I'm not sure it should be considered a bug. I like that the variable that is not yet defined is distinguished from a variable that is defined, even though in blocks it gets less reliable.

The ideal solution IMO would be to show variables that are predicted to be defined[1] in a different color than either 'definitely defined now', or 'definitely undefined now' variables.

[1] Probably a very restricted prediction, mainly inspection of set invocations. set foo 1 or set v o; set fo$v 1 should predict that "foo" will be defined. set invocations incorporating a command substitution in the 'variable name' place should not attempt to predict a variable being defined.

A further wrinkle is that scope must be managed (ie. set -l foo should colorize foo as defined only within that block, whereas set foo may colorize foo as defined within the entire subsequent text. (may, because foo might have previously been set -l, and that original scope is the scope that the recolorization should affect)

If fish implemented this kind of prediction then adding prediction of file existence would be the logical follow on (as in echo foo > bar; wc < bar, in which the second bar is currently colored 'missing')

[ridiculousfish]

Yes highlighting occurs with what is valid at the time of parsing. Any sort of symbolic execution would be way too ambitious, but maybe someone can think of something clever and simple to improve this.

[faho]

Tbh I'd probably just assume any redirection that contains an expansion is valid.

[krobelus]

This patch detects variables defined by for, set or variable overrides, and marks any redirection target that contains such a variable as valid. It gives some false results, like allowing set -e foo; echo > $foo.

Marking all redirection targets as valid if they contain undefined variables could be simpler (maybe just check if the expansion produces an empty list).

diff --git a/src/fish_tests.cpp b/src/fish_tests.cpp
index 59883b2e5..c5d21c203 100644
--- a/src/fish_tests.cpp
+++ b/src/fish_tests.cpp
@@ -5081,4 +5081,37 @@ static void test_highlighting() {
     });
 
+    highlight_tests.push_back({
+        {L"for", highlight_role_t::command},
+        {L"i", highlight_role_t::param},
+        {L"in", highlight_role_t::command},
+        {L"file1", highlight_role_t::param},
+        {L"file2", highlight_role_t::param},
+        {L";", highlight_role_t::statement_terminator},
+        {L"echo", highlight_role_t::command},
+        {L">", highlight_role_t::redirection},
+        {L"$i", highlight_role_t::redirection},
+        {L";", highlight_role_t::statement_terminator},
+        {L"end", highlight_role_t::command},
+    });
+
+    highlight_tests.push_back({
+        {L"set", highlight_role_t::command},
+        {L"i", highlight_role_t::param},
+        {L"file", highlight_role_t::param},
+        {L";", highlight_role_t::statement_terminator},
+        {L"echo", highlight_role_t::command},
+        {L">", highlight_role_t::redirection},
+        {L"$i", highlight_role_t::redirection},
+    });
+
+    highlight_tests.push_back({
+        {L"i", highlight_role_t::param, ns},
+        {L"=", highlight_role_t::operat, ns},
+        {L"file", highlight_role_t::param, ns},
+        {L"echo", highlight_role_t::command},
+        {L">", highlight_role_t::redirection},
+        {L"$i", highlight_role_t::redirection},
+    });
+
     highlight_tests.push_back({
         {L"end", highlight_role_t::error},
diff --git a/src/highlight.cpp b/src/highlight.cpp
index b092b04aa..fdb1ac9f2 100644
--- a/src/highlight.cpp
+++ b/src/highlight.cpp
@@ -781,4 +781,7 @@ class highlighter_t {
     using color_array_t = std::vector<highlight_spec_t>;
     color_array_t color_array;
+    // A stack of variables that the current commandline may define.  This is used to mark
+    // redirections that use one of these variables as valid.
+    std::vector<wcstring> pending_variables;
 
     // Flags we use for AST parsing.
@@ -815,4 +818,5 @@ class highlighter_t {
     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.
@@ -1019,4 +1023,6 @@ void highlighter_t::visit(const ast::variable_assignment_t &varas) {
         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));
     }
 }
@@ -1036,5 +1042,5 @@ void highlighter_t::visit(const ast::decorated_statement_t &stmt) {
         // We cannot check if the command is invalid, so just assume it's valid.
         is_valid_cmd = true;
-    } else if (variable_assignment_equals_pos(*cmd)) {
+    } else if (auto equals_pos = variable_assignment_equals_pos(*cmd)) {
         is_valid_cmd = true;
     } else {
@@ -1058,6 +1064,14 @@ void highlighter_t::visit(const ast::decorated_statement_t &stmt) {
     // 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 {
@@ -1067,4 +1081,18 @@ 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) {
@@ -1075,4 +1103,12 @@ static bool has_cmdsub(const wcstring &src) {
 }
 
+static bool contains_pending_variable(const std::vector<wcstring> &pending_variables,
+                                      const wcstring &haystack) {
+    for (const auto &variable : pending_variables) {
+        if (haystack.find(variable) != wcstring::npos) return true;
+    }
+    return false;
+}
+
 void highlighter_t::visit(const ast::redirection_t &redir) {
     maybe_t<pipe_or_redir_t> oper =
@@ -1107,4 +1143,6 @@ void highlighter_t::visit(const ast::redirection_t &redir) {
             // 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.

[ridiculousfish]

Wow, that came out shorter than I thought, nice tests too! The false positives seem livable to me. What's your intention here, did you want to make a PR?

[krobelus]

Yeah, I'll create a PR or merge directly. I wasn't sure if the hack is worth it. The alternative is simpler to implement, but no longer detects an error on echo > $undefined (not sure if we care).

diff --git a/src/highlight.cpp b/src/highlight.cpp
index b092b04aa..27301a4f0 100644
--- a/src/highlight.cpp
+++ b/src/highlight.cpp
@@ -1102,14 +1102,29 @@ void highlighter_t::visit(const ast::redirection_t &redir) {
         // No command substitution, so we can highlight the target file or fd. For example,
         // disallow redirections into a non-existent directory.
         bool target_is_valid = true;
+        bool done = false;
         if (!this->io_ok) {
             // 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 (!expand_one(target, expand_flag::skip_cmdsubst, ctx)) {
-            // Could not be expanded.
-            target_is_valid = false;
-        } else {
+            done = true;
+        }
+        if (!done) {
+            completion_list_t completions;
+            parse_error_list_t errors;
+            if (expand_string(target, &completions, expand_flag::skip_cmdsubst, this->ctx,
+                              &errors) != expand_result_t::ok ||
+                completions.size() > 1) {
+                // Could not be expanded.
+                target_is_valid = false;
+                done = true;
+            } else if (completions.size() == 0) {
+                // Probably contains an undefined variable, assume it will be defined (#6654).
+                target_is_valid = true;
+                done = true;
+            }
+        }
+        if (!done) {
             // Ok, we successfully expanded our target. Now verify that it works with this
             // redirection. We will probably need it as a path (but not in the case of fd
             // redirections). Note that the target is now unescaped.