Correctly report errors for 'and' and 'or' in pipelines with new parser

fish_tests.cpp

@@ -645,6 +645,23 @@ static void test_parser()
         err(L"'break' command inside switch falsely reported as error");
     }
     
+    if (parse_util_detect_errors(L"or cat | cat") || parse_util_detect_errors(L"and cat | cat"))
+    {
+        err(L"boolean command at beginning of pipeline falsely reported as error");
+    }
+    
+    if (! parse_util_detect_errors(L"cat | and cat"))
+    {
+        err(L"'and' command in pipeline not reported as error");
+    }
+    
+    if (! parse_util_detect_errors(L"cat | exec") || ! parse_util_detect_errors(L"exec | cat"))
+    {
+        err(L"'exec' command in pipeline not reported as error");
+    }
+
+
+    
 
     say(L"Testing basic evaluation");
 #if 0

parse_tree.cpp

@@ -1357,13 +1357,15 @@ bool parse_node_tree_t::command_for_plain_statement(const parse_node_t &node, co
     return result;
 }
 
-bool parse_node_tree_t::plain_statement_is_in_pipeline(const parse_node_t &node, bool include_first) const
+bool parse_node_tree_t::statement_is_in_pipeline(const parse_node_t &node, bool include_first) const
 {
     // Moderately nasty hack! Walk up our ancestor chain and see if we are in a job_continuation. This checks if we are in the second or greater element in a pipeline; if we are the first element we treat this as false
+    // This accepts a few statement types
     bool result = false;
     const parse_node_t *ancestor = &node;
     
-    if (ancestor)
+    // If we're given a plain statement, try to get its decorated statement parent
+    if (ancestor && ancestor->type == symbol_plain_statement)
         ancestor = this->get_parent(*ancestor, symbol_decorated_statement);
     if (ancestor)
         ancestor = this->get_parent(*ancestor, symbol_statement);

parse_tree.h

@@ -181,7 +181,7 @@ public:
     bool command_for_plain_statement(const parse_node_t &node, const wcstring &src, wcstring *out_cmd) const;
     
     /* Given a plain statement, return true if the statement is part of a pipeline. If include_first is set, the first command in a pipeline is considered part of it; otherwise only the second or additional commands are */
-    bool plain_statement_is_in_pipeline(const parse_node_t &node, bool include_first) const;
+    bool statement_is_in_pipeline(const parse_node_t &node, bool include_first) const;
     
     /* Given a redirection, get the redirection type (or TOK_NONE) and target (file path, or fd) */
     enum token_type type_for_redirection(const parse_node_t &node, const wcstring &src, int *out_fd, wcstring *out_target) const;

parse_util.cpp

@@ -1016,6 +1016,16 @@ parser_test_error_bits_t parse_util_detect_errors(const wcstring &buff_src, pars
                 // an 'end' without source is an unclosed block
                 has_unclosed_block = true;
             }
+            else if (node.type == symbol_boolean_statement)
+            {
+                // 'or' and 'and' can be in a pipeline, as long as they're first
+                // These numbers 0 and 1 correspond to productions for boolean_statement. This should be cleaned up.
+                bool is_and = (node.production_idx == 0), is_or = (node.production_idx == 1);
+                if ((is_and || is_or) && node_tree.statement_is_in_pipeline(node, false /* don't count first */))
+                {
+                    errored = append_syntax_error(&parse_errors, node, EXEC_ERR_MSG, is_and ? L"and" : L"or");
+                }
+            }
             else if (node.type == symbol_plain_statement)
             {
                 wcstring command;
@@ -1028,12 +1038,10 @@ parser_test_error_bits_t parse_util_detect_errors(const wcstring &buff_src, pars
                     }
                     
                     // Check that pipes are sound
-                    bool is_boolean_command = contains(command, L"or", L"and");
+                    if (! errored && parser_is_pipe_forbidden(command))
-                    bool is_pipe_forbidden = parser_is_pipe_forbidden(command);
-                    if (! errored && (is_boolean_command || is_pipe_forbidden))
                     {
-                        // 'or' and 'and' can be first in the pipeline. forbidden commands cannot be in a pipeline at all
+                        // forbidden commands cannot be in a pipeline at all
-                        if (node_tree.plain_statement_is_in_pipeline(node, is_pipe_forbidden))
+                        if (node_tree.statement_is_in_pipeline(node, true /* count first */))
                         {
                             errored = append_syntax_error(&parse_errors, node, EXEC_ERR_MSG, command.c_str());
                         }
@@ -1062,7 +1070,7 @@ parser_test_error_bits_t parse_util_detect_errors(const wcstring &buff_src, pars
                         }
                     }
                     
-                    // Check that we don't return from outside a function
+                    // Check that we don't break or continue from outside a loop
                     if (! errored && (command == L"break" || command == L"continue"))
                     {
                         // Walk up until we hit a 'for' or 'while' loop. If we hit a function first, stop the search; we can't break an outer loop from inside a function.