Simplify infinite loop fix when parsing "a="

This reworks the "a=" detection to be simpler.
If we detect a variable assignment that produces an error,
simply consume it.

We also take the opportunity to not highlight it as an error,
and add some tests.

Original commit is 1ca05d32d3.

src/ast.cpp

@@ -663,11 +663,23 @@ class ast_t::populator_t {
     }
 
     bool can_parse(variable_assignment_t *) {
-        // We can parse a variable_assignment if our token is a variable assignment and the next
-        // token is a string. If the next token is not a string, then we have either a bare
-        // assignment like `foo=bar` or perhaps `foo=bar | `, etc. In that case we want to allow
-        // statement to see this assignment so it can produce an error.
-        return peek_token(0).may_be_variable_assignment && peek_type(1) == parse_token_type_string;
+        // Do we have a variable assignment at all?
+        if (!peek_token(0).may_be_variable_assignment) return false;
+
+        // What is the token after it?
+        switch (peek_type(1)) {
+            case parse_token_type_string:
+                // We have `a= cmd` and should treat it as a variable assignment.
+                return true;
+            case parse_token_type_terminate:
+                // We have `a=` which is OK if we are allowing incomplete, an error otherwise.
+                return allow_incomplete();
+            default:
+                // We have e.g. `a= >` which is an error.
+                // Note that we do not produce an error here. Instead we return false so this the
+                // token will be seen by allocate_populate_statement_contents.
+                return false;
+        }
     }
 
     template <parse_token_type_t... Tok>
@@ -749,10 +761,6 @@ class ast_t::populator_t {
             return;
         }
 
-        // HACK We sometimes unwind after not consuming anything, for example on "a=".
-        // To avoid an infinite loop, remember the current token to be able to detect this case.
-        uint32_t first_token = peek_token().range().start;
-
         for (;;) {
             // If we are unwinding, then either we recover or we break the loop, dependent on the
             // loop type.
@@ -760,14 +768,12 @@ class ast_t::populator_t {
                 if (!list_type_stops_unwind(ListType)) {
                     break;
                 }
-                bool consume_first = peek_token().range().start == first_token;
                 // We are going to stop unwinding.
                 // Rather hackish. Just chomp until we get to a string or end node.
                 for (auto type = peek_type();
-                     (consume_first || type != parse_token_type_string) &&
-                     type != parse_token_type_terminate && type != parse_token_type_end;
+                     type != parse_token_type_string && type != parse_token_type_terminate &&
+                     type != parse_token_type_end;
                      type = peek_type()) {
-                    consume_first = false;
                     parse_token_t tok = tokens_.pop();
                     ast_->extras_.errors.push_back(tok.range());
                     FLOGF(ast_construction, L"%*schomping range %u-%u", spaces(), "",
@@ -822,7 +828,8 @@ class ast_t::populator_t {
         } else if (token1.may_be_variable_assignment) {
             // Here we have a variable assignment which we chose to not parse as a variable
             // assignment because there was no string after it.
-            parse_error(token1, parse_error_bare_variable_assignment, L"");
+            // Ensure we consume the token, so we don't get back here again at the same place.
+            parse_error(consume_any_token(), parse_error_bare_variable_assignment, L"");
             return got_error();
         }
 

src/fish_tests.cpp

@@ -4546,6 +4546,20 @@ static void test_new_parser_ad_hoc() {
     if (count != 2) {
         err(L"Expected 2 case item nodes, found %d", count);
     }
+
+    // Ensure that naked variable assignments don't hang.
+    // The bug was that "a=" would produce an error but not be consumed,
+    // leading to an infinite loop.
+
+    // By itself it should produce an error.
+    ast = ast_t::parse(L"a=");
+    do_test(ast.errored());
+
+    // If we are leaving things unterminated, this should not produce an error.
+    // i.e. when typing "a=" at the command line, it should be treated as valid
+    // because we don't want to color it as an error.
+    ast = ast_t::parse(L"a=", parse_flag_leave_unterminated);
+    do_test(!ast.errored());
 }
 
 static void test_new_parser_errors() {
@@ -4568,6 +4582,8 @@ static void test_new_parser_errors() {
         {L"if true ; case ; end", parse_error_generic},
 
         {L"true | and", parse_error_andor_in_pipeline},
+
+        {L"a=", parse_error_bare_variable_assignment},
     };
 
     for (const auto &test : tests) {
@@ -4930,6 +4946,10 @@ static void test_highlighting() {
         {L"# comment", highlight_role_t::comment},
     });
 
+    highlight_tests.push_back({
+        {L"a=", highlight_role_t::param},
+    });
+
     auto &vars = parser_t::principal_parser().vars();
     // Verify variables and wildcards in commands using /bin/cat.
     vars.set(L"VARIABLE_IN_COMMAND", ENV_LOCAL, {L"a"});

src/parse_tree.cpp

@@ -43,11 +43,10 @@ parse_error_code_t parse_error_from_tokenizer_error(tokenizer_error_t err) {
 /// Returns a string description of this parse error.
 wcstring parse_error_t::describe_with_prefix(const wcstring &src, const wcstring &prefix,
                                              bool is_interactive, bool skip_caret) const {
-    if (skip_caret && this->text.empty()) return L"";
-
     wcstring result = prefix;
     switch (code) {
         default:
+            if (skip_caret && this->text.empty()) return L"";
             break;
         case parse_error_andor_in_pipeline:
             append_format(result, EXEC_ERR_MSG,