From f69055b5e969b4b85995edcacf0290cdbc4ccf90 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Fri, 12 Jan 2018 11:15:35 -0800 Subject: [PATCH] Adopt tnode_t in parse_util_detect_errors --- src/complete.cpp | 33 ++++++------- src/fish_tests.cpp | 26 +++++----- src/parse_tree.h | 23 +++++++++ src/parse_util.cpp | 121 +++++++++++++++++++++++---------------------- 4 files changed, 113 insertions(+), 90 deletions(-) diff --git a/src/complete.cpp b/src/complete.cpp index 03e2e5a81..3d582db04 100644 --- a/src/complete.cpp +++ b/src/complete.cpp @@ -159,19 +159,19 @@ class completion_entry_t { /// Set of all completion entries. namespace std { - template<> - struct hash { - size_t operator()(const completion_entry_t &c) const { - std::hash hasher; - return hasher((wcstring) c.cmd); - } - }; - template <> - struct equal_to { - bool operator()(const completion_entry_t &c1, const completion_entry_t &c2) const { - return c1.cmd == c2.cmd; - } - }; +template <> +struct hash { + size_t operator()(const completion_entry_t &c) const { + std::hash hasher; + return hasher((wcstring)c.cmd); + } +}; +template <> +struct equal_to { + bool operator()(const completion_entry_t &c1, const completion_entry_t &c2) const { + return c1.cmd == c2.cmd; + } +}; } typedef std::unordered_set completion_entry_set_t; static completion_entry_set_t completion_set; @@ -1281,10 +1281,9 @@ void complete(const wcstring &cmd_with_subcmds, std::vector *out_c if (!done) { parse_node_tree_t tree; - parse_tree_from_string(cmd, - parse_flag_continue_after_error | - parse_flag_accept_incomplete_tokens | - parse_flag_include_comments, + parse_tree_from_string(cmd, parse_flag_continue_after_error | + parse_flag_accept_incomplete_tokens | + parse_flag_include_comments, &tree, NULL); // Find the plain statement to operate on. The cursor may be past it (#1261), so backtrack diff --git a/src/fish_tests.cpp b/src/fish_tests.cpp index 4ec7ea313..8b31a3958 100644 --- a/src/fish_tests.cpp +++ b/src/fish_tests.cpp @@ -2314,8 +2314,8 @@ static void test_completion_insertions() { TEST_1_COMPLETION(L"'foo^", L"bar", COMPLETE_REPLACES_TOKEN, false, L"bar ^"); } -static void perform_one_autosuggestion_cd_test(const wcstring &command, - const wcstring &expected, long line) { +static void perform_one_autosuggestion_cd_test(const wcstring &command, const wcstring &expected, + long line) { std::vector comps; complete(command, &comps, COMPLETION_REQUEST_AUTOSUGGESTION); @@ -2350,8 +2350,8 @@ static void perform_one_autosuggestion_cd_test(const wcstring &command, } } -static void perform_one_completion_cd_test(const wcstring &command, - const wcstring &expected, long line) { +static void perform_one_completion_cd_test(const wcstring &command, const wcstring &expected, + long line) { std::vector comps; complete(command, &comps, COMPLETION_REQUEST_DEFAULT); @@ -2375,10 +2375,10 @@ static void perform_one_completion_cd_test(const wcstring &command, const completion_t &suggestion = comps.at(0); if (suggestion.completion != expected) { - fwprintf( - stderr, - L"line %ld: complete() for cd tab completion returned the wrong expected string for command %ls\n", - line, command.c_str()); + fwprintf(stderr, + L"line %ld: complete() for cd tab completion returned the wrong expected " + L"string for command %ls\n", + line, command.c_str()); fwprintf(stderr, L" actual: %ls\n", suggestion.completion.c_str()); fwprintf(stderr, L"expected: %ls\n", expected.c_str()); do_test_from(suggestion.completion == expected, line); @@ -2660,9 +2660,9 @@ static void test_universal_callbacks() { uvars2.sync(callbacks); // Change uvars1. - uvars1.set(L"alpha", {L"2"}, false); // changes value - uvars1.set(L"beta", {L"1"}, true); // changes export - uvars1.remove(L"delta"); // erases value + uvars1.set(L"alpha", {L"2"}, false); // changes value + uvars1.set(L"beta", {L"1"}, true); // changes export + uvars1.remove(L"delta"); // erases value uvars1.set(L"epsilon", {L"1"}, false); // changes nothing uvars1.sync(callbacks); @@ -4287,7 +4287,7 @@ static void test_illegal_command_exit_code(void) { void test_maybe() { say(L"Testing maybe_t"); - do_test(! bool(maybe_t())); + do_test(!bool(maybe_t())); maybe_t m(3); do_test(m.has_value()); do_test(m.value() == 3); @@ -4306,7 +4306,7 @@ void test_maybe() { do_test(maybe_t() == none()); do_test(!maybe_t(none()).has_value()); m = none(); - do_test(! bool(m)); + do_test(!bool(m)); maybe_t m2("abc"); do_test(!m2.missing_or_empty()); diff --git a/src/parse_tree.h b/src/parse_tree.h index 478b31da7..51b37fc86 100644 --- a/src/parse_tree.h +++ b/src/parse_tree.h @@ -303,6 +303,18 @@ class tnode_t { return tnode_t{tree, child}; } + /// If the child at the given index has the given type, return it; otherwise return an empty + /// child. + /// This is used for e.g. alternations. + /// TODO: check that the type is possible (i.e. sum type). + template + tnode_t try_get_child() const { + const parse_node_t *child = nullptr; + if (nodeptr) child = tree->get_child(*nodeptr, Index); + if (child && child->type == ChildType::token) return {tree, child}; + return {}; + } + /// Type-safe access to a node's parent. /// If the parent exists and has type ParentType, return it. /// Otherwise return a missing tnode. @@ -312,6 +324,17 @@ class tnode_t { return {tree, tree->get_parent(*nodeptr, ParentType::token)}; } + /// Given that we are a list type, \return the next node of some Item in some node list, + /// adjusting 'this' to be the remainder of the list. + /// Returns an empty item on failure. + template + tnode_t next_in_list() { + if (!nodeptr) return {}; + const parse_node_t *next = + tree->next_node_in_node_list(*nodeptr, ItemType::token, &nodeptr); + return {tree, next}; + } + static std::vector find_nodes(const parse_node_tree_t *tree, const parse_node_t *parent, size_t max_count = size_t(-1)) { diff --git a/src/parse_util.cpp b/src/parse_util.cpp index 3ea58463c..931123d14 100644 --- a/src/parse_util.cpp +++ b/src/parse_util.cpp @@ -1040,6 +1040,56 @@ parser_test_error_bits_t parse_util_detect_errors_in_argument(const parse_node_t return err; } +/// Given that the job given by node should be backgrounded, return true if we detect any errors. +static bool detect_errors_in_backgrounded_job(const parse_node_tree_t &node_tree, + tnode_t node, + parse_error_list_t *parse_errors) { + bool errored = false; + // Disallow background in the following cases: + // foo & ; and bar + // foo & ; or bar + // if foo & ; end + // while foo & ; end + const parse_node_t *job_parent = node_tree.get_parent(node); + assert(job_parent != NULL); + switch (job_parent->type) { + case symbol_if_clause: + case symbol_while_header: { + assert(node_tree.get_child(*job_parent, 1) == node); + errored = append_syntax_error(parse_errors, node.source_range()->start, + BACKGROUND_IN_CONDITIONAL_ERROR_MSG); + break; + } + case symbol_job_list: { + // This isn't very complete, e.g. we don't catch 'foo & ; not and bar'. + // Build the job list and then advance it by one. + tnode_t job_list{&node_tree, job_parent}; + auto first_job = job_list.next_in_list(); + assert(first_job.node() == node && "Expected first job to be the node we found"); + (void)first_job; + // Try getting the next job as a boolean statement. + auto next_job = job_list.next_in_list(); + tnode_t next_stmt = next_job.child<0>(); + if (auto bool_stmt = next_stmt.try_get_child()) { + // The next job is indeed a boolean statement. + parse_bool_statement_type_t bool_type = + parse_node_tree_t::statement_boolean_type(*bool_stmt.node()); + if (bool_type == parse_bool_and) { // this is not allowed + errored = append_syntax_error(parse_errors, bool_stmt.source_range()->start, + BOOL_AFTER_BACKGROUND_ERROR_MSG, L"and"); + } else if (bool_type == parse_bool_or) { // this is not allowed + errored = append_syntax_error(parse_errors, bool_stmt.source_range()->start, + BOOL_AFTER_BACKGROUND_ERROR_MSG, L"or"); + } + } + break; + } + default: + break; + } + return errored; +} + parser_test_error_bits_t parse_util_detect_errors(const wcstring &buff_src, parse_error_list_t *out_errors, bool allow_incomplete, @@ -1097,9 +1147,7 @@ parser_test_error_bits_t parse_util_detect_errors(const wcstring &buff_src, // Verify no variable expansions. if (!errored) { - const size_t node_tree_size = node_tree.size(); - for (size_t i = 0; i < node_tree_size; i++) { - const parse_node_t &node = node_tree.at(i); + for (const parse_node_t &node : node_tree) { if (node.type == symbol_end_command && !node.has_source()) { // An 'end' without source is an unclosed block. has_unclosed_block = true; @@ -1115,63 +1163,16 @@ parser_test_error_bits_t parse_util_detect_errors(const wcstring &buff_src, const wcstring arg_src = node.get_source(buff_src); res |= parse_util_detect_errors_in_argument(node, arg_src, &parse_errors); } else if (node.type == symbol_job) { - if (node_tree.job_should_be_backgrounded(node)) { - // Disallow background in the following cases: - // - // foo & ; and bar - // foo & ; or bar - // if foo & ; end - // while foo & ; end - const parse_node_t *job_parent = node_tree.get_parent(node); - assert(job_parent != NULL); - switch (job_parent->type) { - case symbol_if_clause: - case symbol_while_header: { - assert(node_tree.get_child(*job_parent, 1) == &node); - errored = append_syntax_error(&parse_errors, node.source_start, - BACKGROUND_IN_CONDITIONAL_ERROR_MSG); - break; - } - case symbol_job_list: { - // This isn't very complete, e.g. we don't catch 'foo & ; not and bar'. - assert(node_tree.get_child(*job_parent, 0) == &node); - const parse_node_t *next_job_list = - node_tree.get_child(*job_parent, 1, symbol_job_list); - assert(next_job_list != NULL); - const parse_node_t *next_job = - node_tree.next_node_in_node_list(*next_job_list, symbol_job, NULL); - if (next_job == NULL) { - break; - } - - const parse_node_t *next_statement = - node_tree.get_child(*next_job, 0, symbol_statement); - if (next_statement == NULL) { - break; - } - - const parse_node_t *spec_statement = - node_tree.get_child(*next_statement, 0); - if (!spec_statement || - spec_statement->type != symbol_boolean_statement) { - break; - } - - parse_bool_statement_type_t bool_type = - parse_node_tree_t::statement_boolean_type(*spec_statement); - if (bool_type == parse_bool_and) { // this is not allowed - errored = - append_syntax_error(&parse_errors, spec_statement->source_start, - BOOL_AFTER_BACKGROUND_ERROR_MSG, L"and"); - } else if (bool_type == parse_bool_or) { // this is not allowed - errored = - append_syntax_error(&parse_errors, spec_statement->source_start, - BOOL_AFTER_BACKGROUND_ERROR_MSG, L"or"); - } - break; - } - default: { break; } - } + // Disallow background in the following cases: + // + // foo & ; and bar + // foo & ; or bar + // if foo & ; end + // while foo & ; end + // If it's not a background job, nothing to do. + auto job = tnode_t{&node_tree, &node}; + if (node_tree.job_should_be_backgrounded(job)) { + errored |= detect_errors_in_backgrounded_job(node_tree, job, &parse_errors); } } else if (node.type == symbol_plain_statement) { // In a few places below, we want to know if we are in a pipeline.