Adopt tnode_t in parse_util_detect_errors

src/complete.cpp

@@ -159,19 +159,19 @@ class completion_entry_t {
 
 /// Set of all completion entries.
 namespace std {
-    template<>
+template <>
-    struct hash<completion_entry_t> {
+struct hash<completion_entry_t> {
-        size_t operator()(const completion_entry_t &c) const {
+    size_t operator()(const completion_entry_t &c) const {
-            std::hash<wcstring> hasher;
+        std::hash<wcstring> hasher;
-            return hasher((wcstring) c.cmd);
+        return hasher((wcstring)c.cmd);
-        }
+    }
-    };
+};
-    template <>
+template <>
-    struct equal_to<completion_entry_t> {
+struct equal_to<completion_entry_t> {
-        bool operator()(const completion_entry_t &c1, const completion_entry_t &c2) const {
+    bool operator()(const completion_entry_t &c1, const completion_entry_t &c2) const {
-            return c1.cmd == c2.cmd;
+        return c1.cmd == c2.cmd;
-        }
+    }
-    };
+};
 }
 typedef std::unordered_set<completion_entry_t> completion_entry_set_t;
 static completion_entry_set_t completion_set;
@@ -1281,10 +1281,9 @@ void complete(const wcstring &cmd_with_subcmds, std::vector<completion_t> *out_c
 
     if (!done) {
         parse_node_tree_t tree;
-        parse_tree_from_string(cmd,
+        parse_tree_from_string(cmd, parse_flag_continue_after_error |
-                               parse_flag_continue_after_error |
+                                        parse_flag_accept_incomplete_tokens |
-                                   parse_flag_accept_incomplete_tokens |
+                                        parse_flag_include_comments,
-                                   parse_flag_include_comments,
                                &tree, NULL);
 
         // Find the plain statement to operate on. The cursor may be past it (#1261), so backtrack

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,
+static void perform_one_autosuggestion_cd_test(const wcstring &command, const wcstring &expected,
-                                               const wcstring &expected, long line) {
+                                               long line) {
     std::vector<completion_t> 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,
+static void perform_one_completion_cd_test(const wcstring &command, const wcstring &expected,
-                                               const wcstring &expected, long line) {
+                                           long line) {
     std::vector<completion_t> 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(
+            fwprintf(stderr,
-                stderr,
+                     L"line %ld: complete() for cd tab completion returned the wrong expected "
-                L"line %ld: complete() for cd tab completion returned the wrong expected string for command %ls\n",
+                     L"string for command %ls\n",
-                line, command.c_str());
+                     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"alpha", {L"2"}, false);    // changes value
-    uvars1.set(L"beta", {L"1"}, true);    // changes export
+    uvars1.set(L"beta", {L"1"}, true);      // changes export
-    uvars1.remove(L"delta");              // erases value
+    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<int>()));
+    do_test(!bool(maybe_t<int>()));
     maybe_t<int> m(3);
     do_test(m.has_value());
     do_test(m.value() == 3);
@@ -4306,7 +4306,7 @@ void test_maybe() {
     do_test(maybe_t<int>() == none());
     do_test(!maybe_t<int>(none()).has_value());
     m = none();
-    do_test(! bool(m));
+    do_test(!bool(m));
 
     maybe_t<std::string> m2("abc");
     do_test(!m2.missing_or_empty());

src/parse_tree.h

@@ -303,6 +303,18 @@ class tnode_t {
         return tnode_t<child_type>{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 <class ChildType, node_offset_t Index>
+    tnode_t<ChildType> 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 <class ItemType>
+    tnode_t<ItemType> 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<tnode_t> find_nodes(const parse_node_tree_t *tree,
                                            const parse_node_t *parent,
                                            size_t max_count = size_t(-1)) {

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<grammar::job> 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<grammar::job_list> job_list{&node_tree, job_parent};
+            auto first_job = job_list.next_in_list<grammar::job>();
+            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<grammar::job>();
+            tnode_t<grammar::statement> next_stmt = next_job.child<0>();
+            if (auto bool_stmt = next_stmt.try_get_child<grammar::boolean_statement, 0>()) {
+                // 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 (const parse_node_t &node : node_tree) {
-        for (size_t i = 0; i < node_tree_size; i++) {
-            const parse_node_t &node = node_tree.at(i);
             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:
-                    // Disallow background in the following cases:
+                //
-                    //
+                // foo & ; and bar
-                    // foo & ; and bar
+                // foo & ; or bar
-                    // foo & ; or bar
+                // if foo & ; end
-                    // if foo & ; end
+                // while foo & ; end
-                    // while foo & ; end
+                // If it's not a background job, nothing to do.
-                    const parse_node_t *job_parent = node_tree.get_parent(node);
+                auto job = tnode_t<grammar::job>{&node_tree, &node};
-                    assert(job_parent != NULL);
+                if (node_tree.job_should_be_backgrounded(job)) {
-                    switch (job_parent->type) {
+                    errored |= detect_errors_in_backgrounded_job(node_tree, job, &parse_errors);
-                        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; }
-                    }
                 }
             } else if (node.type == symbol_plain_statement) {
                 // In a few places below, we want to know if we are in a pipeline.