From 212eeaa77c7408893df92aa9b312855bfc9dcd8e Mon Sep 17 00:00:00 2001
From: ridiculousfish <corydoras@ridiculousfish.com>
Date: Mon, 13 Jan 2014 13:14:18 -0800
Subject: [PATCH] Correctly report errors for 'and' and 'or' in pipelines with
 new parser

---
 fish_tests.cpp | 17 +++++++++++++++++
 parse_tree.cpp |  6 ++++--
 parse_tree.h   |  2 +-
 parse_util.cpp | 20 ++++++++++++++------
 4 files changed, 36 insertions(+), 9 deletions(-)

diff --git a/fish_tests.cpp b/fish_tests.cpp
index 319a3872a..fab103351 100644
--- a/fish_tests.cpp
+++ b/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
diff --git a/parse_tree.cpp b/parse_tree.cpp
index 41873bbe9..6bb7a3cd3 100644
--- a/parse_tree.cpp
+++ b/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);
diff --git a/parse_tree.h b/parse_tree.h
index 5cc0a4ccc..f77b87811 100644
--- a/parse_tree.h
+++ b/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;
diff --git a/parse_util.cpp b/parse_util.cpp
index eded7b93f..491e47328 100644
--- a/parse_util.cpp
+++ b/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");
-                    bool is_pipe_forbidden = parser_is_pipe_forbidden(command);
-                    if (! errored && (is_boolean_command || is_pipe_forbidden))
+                    if (! errored && parser_is_pipe_forbidden(command))
                     {
-                        // 'or' and 'and' can be first in the pipeline. forbidden commands cannot be in a pipeline at all
-                        if (node_tree.plain_statement_is_in_pipeline(node, is_pipe_forbidden))
+                        // forbidden commands cannot be in a pipeline at all
+                        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.