diff --git a/fish_tests.cpp b/fish_tests.cpp index 34d505c96..4df8322ca 100644 --- a/fish_tests.cpp +++ b/fish_tests.cpp @@ -691,7 +691,37 @@ static void test_parser() { err(L"Bad escape in nested command substitution not reported as error"); } + + if (! parse_util_detect_errors(L"false & ; and cat")) + { + err(L"'and' command after background not reported as error"); + } + if (! parse_util_detect_errors(L"true & ; or cat")) + { + err(L"'or' command after background not reported as error"); + } + + if (parse_util_detect_errors(L"true & ; not cat")) + { + err(L"'not' command after background falsely reported as error"); + } + + + if (! parse_util_detect_errors(L"if true & ; end")) + { + err(L"backgrounded 'if' conditional not reported as error"); + } + + if (! parse_util_detect_errors(L"if false; else if true & ; end")) + { + err(L"backgrounded 'else if' conditional not reported as error"); + } + + if (! parse_util_detect_errors(L"while true & ; end")) + { + err(L"backgrounded 'while' conditional not reported as error"); + } say(L"Testing basic evaluation"); #if 0 diff --git a/parse_constants.h b/parse_constants.h index 559aad294..0f882408d 100644 --- a/parse_constants.h +++ b/parse_constants.h @@ -109,6 +109,14 @@ enum parse_statement_decoration_t parse_statement_decoration_exec }; +/* Boolean statement types */ +enum parse_bool_statement_type_t +{ + parse_bool_and, + parse_bool_or, + parse_bool_not +}; + /* Parse error code list */ enum parse_error_code_t { diff --git a/parse_execution.cpp b/parse_execution.cpp index 4bdcf7b6d..749ab805d 100644 --- a/parse_execution.cpp +++ b/parse_execution.cpp @@ -1128,30 +1128,22 @@ parse_execution_result_t parse_execution_context_t::populate_boolean_process(job // Handle a boolean statement bool skip_job = false; assert(bool_statement.type == symbol_boolean_statement); - switch (bool_statement.production_idx) + switch (parse_node_tree_t::statement_boolean_type(bool_statement)) { - // These magic numbers correspond to productions for boolean_statement - case 0: + case parse_bool_and: // AND. Skip if the last job failed. skip_job = (proc_get_last_status() != 0); break; - case 1: + case parse_bool_or: // OR. Skip if the last job succeeded. skip_job = (proc_get_last_status() == 0); break; - case 2: + case parse_bool_not: // NOT. Negate it. job_set_flag(job, JOB_NEGATE, !job_get_flag(job, JOB_NEGATE)); break; - - default: - { - fprintf(stderr, "Unexpected production in boolean statement\n"); - PARSER_DIE(); - break; - } } if (skip_job) diff --git a/parse_tree.cpp b/parse_tree.cpp index c3ef658ed..8c38ef5a5 100644 --- a/parse_tree.cpp +++ b/parse_tree.cpp @@ -1649,6 +1649,30 @@ parse_node_tree_t::parse_node_list_t parse_node_tree_t::specific_statements_for_ return result; } +enum parse_bool_statement_type_t parse_node_tree_t::statement_boolean_type(const parse_node_t &node) +{ + assert(node.type == symbol_boolean_statement); + switch (node.production_idx) + { + // These magic numbers correspond to productions for boolean_statement + case 0: + return parse_bool_and; + + case 1: + return parse_bool_or; + + case 2: + return parse_bool_not; + + default: + { + fprintf(stderr, "Unexpected production in boolean statement\n"); + PARSER_DIE(); + return (enum parse_bool_statement_type_t)(-1); + } + } +} + bool parse_node_tree_t::job_should_be_backgrounded(const parse_node_t &job) const { assert(job.type == symbol_job); @@ -1657,7 +1681,8 @@ bool parse_node_tree_t::job_should_be_backgrounded(const parse_node_t &job) cons const parse_node_t *opt_background = get_child(job, 2, symbol_optional_background); if (opt_background != NULL) { - assert(opt_background->production_idx <= 1); + // We may get the value -1 if the node is not yet materialized (i.e. an incomplete parse tree) + assert(opt_background->production_idx == uint8_t(-1) || opt_background->production_idx <= 1); result = (opt_background->production_idx == 1); } return result; diff --git a/parse_tree.h b/parse_tree.h index 8a86e70b2..59739af48 100644 --- a/parse_tree.h +++ b/parse_tree.h @@ -184,11 +184,13 @@ public: /* Given a job, return all of its statements. These are 'specific statements' (e.g. symbol_decorated_statement, not symbol_statement) */ parse_node_list_t specific_statements_for_job(const parse_node_t &job) const; + /* Returns the boolean type for a boolean node */ + static enum parse_bool_statement_type_t statement_boolean_type(const parse_node_t &node); + /* Given a job, return whether it should be backgrounded, because it has a & specifier */ bool job_should_be_backgrounded(const parse_node_t &job) const; }; - /* The big entry point. Parse a string, attempting to produce a tree for the given goal type */ bool parse_tree_from_string(const wcstring &str, parse_tree_flags_t flags, parse_node_tree_t *output, parse_error_list_t *errors, parse_token_type_t goal = symbol_job_list); diff --git a/parse_util.cpp b/parse_util.cpp index c88b34c5d..e716ce213 100644 --- a/parse_util.cpp +++ b/parse_util.cpp @@ -42,11 +42,16 @@ #include "parser.h" #include "builtin.h" -/** - Error message for improper use of the exec builtin -*/ +/** Error message for improper use of the exec builtin */ #define EXEC_ERR_MSG _(L"The '%ls' command can not be used in a pipeline") +/** Error message for use of backgrounded commands before and/or */ +#define BOOL_AFTER_BACKGROUND_ERROR_MSG _(L"The '%ls' command can not be used immediately after a backgrounded job") + +/** Error message for backgrounded commands as conditionals */ +#define BACKGROUND_IN_CONDITIONAL_ERROR_MSG _(L"Backgrounded commands can not be used as conditionals") + + int parse_util_lineno(const wchar_t *str, size_t offset) { if (! str) @@ -1315,11 +1320,10 @@ parser_test_error_bits_t parse_util_detect_errors(const wcstring &buff_src, pars 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 */)) + parse_bool_statement_type_t type = parse_node_tree_t::statement_boolean_type(node); + if ((type == parse_bool_and || type == parse_bool_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"); + errored = append_syntax_error(&parse_errors, node, EXEC_ERR_MSG, (type == parse_bool_and) ? L"and" : L"or"); } } else if (node.type == symbol_argument) @@ -1327,6 +1331,68 @@ parser_test_error_bits_t parse_util_detect_errors(const wcstring &buff_src, pars 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, 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) + { + const parse_node_t *next_statement = node_tree.get_child(*next_job, 0, symbol_statement); + if (next_statement != NULL) + { + const parse_node_t *spec_statement = node_tree.get_child(*next_statement, 0); + if (spec_statement && spec_statement->type == symbol_boolean_statement) + { + switch (parse_node_tree_t::statement_boolean_type(*spec_statement)) + { + // These are not allowed + case parse_bool_and: + errored = append_syntax_error(&parse_errors, *spec_statement, BOOL_AFTER_BACKGROUND_ERROR_MSG, L"and"); + break; + case parse_bool_or: + errored = append_syntax_error(&parse_errors, *spec_statement, BOOL_AFTER_BACKGROUND_ERROR_MSG, L"or"); + break; + case parse_bool_not: + // This one is OK + break; + } + } + } + } + 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