From 6976d0ee7e595bff1035d5766878bb660e402986 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sun, 5 Jul 2020 11:36:13 -0700 Subject: [PATCH] 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 1ca05d32d3ae4929dc61c36e4315997fe2fb6bb8. --- src/ast.cpp | 35 +++++++++++++++++++++-------------- src/fish_tests.cpp | 20 ++++++++++++++++++++ src/parse_tree.cpp | 3 +-- 3 files changed, 42 insertions(+), 16 deletions(-) diff --git a/src/ast.cpp b/src/ast.cpp index 1cb4b0ec6..e69975c2f 100644 --- a/src/ast.cpp +++ b/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 @@ -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(); } diff --git a/src/fish_tests.cpp b/src/fish_tests.cpp index 3d7829bfc..451b3844b 100644 --- a/src/fish_tests.cpp +++ b/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"}); diff --git a/src/parse_tree.cpp b/src/parse_tree.cpp index 28080b292..30006bfd2 100644 --- a/src/parse_tree.cpp +++ b/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,