From 4b079e16e5f208342f9ccc48b38b02653fb7717b Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sat, 31 Mar 2018 14:57:24 -0700 Subject: [PATCH] Execute the conditions of if and while statements outside of their block Variables set in if and while conditions are in the enclosing block, not the if/while statement block. For example: if set -l var (somecommand) ; end echo $var will now work as expected. Fixes #4820. Fixes #1212. --- CHANGELOG.md | 1 + src/parse_execution.cpp | 64 ++++++++++++++++++++--------------------- src/parse_execution.h | 9 ++++-- tests/set.err | 3 ++ tests/set.in | 10 +++++++ tests/set.out | 23 +++++++++++++++ tests/test8.err | 3 ++ tests/test8.in | 10 +++++++ tests/test8.out | 3 ++ 9 files changed, 90 insertions(+), 36 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index eb5b433f2..6c51b7852 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -50,6 +50,7 @@ This section is for changes merged to the `major` branch that are not also merge - fish now supports `&&`, `||`, and `!` (#4620). - The machine hostname, where available, is now exposed as `$hostname` which is now a reserved variable. This drops the dependency on the `hostname` executable (#4422). - `functions --handlers` can be used to show event handlers (#4694). +- Variables set in `if` and `while` conditions are available outside the block (#4820). ## Other significant changes - Command substitution output is now limited to 10 MB by default (#3822). diff --git a/src/parse_execution.cpp b/src/parse_execution.cpp index 17843b679..b728435ec 100644 --- a/src/parse_execution.cpp +++ b/src/parse_execution.cpp @@ -226,10 +226,7 @@ bool parse_execution_context_t::job_is_simple_block(tnode_t job_node) co } parse_execution_result_t parse_execution_context_t::run_if_statement( - tnode_t statement) { - // Push an if block. - if_block_t *ib = parser->push_block(); - + tnode_t statement, const block_t *associated_block) { parse_execution_result_t result = parse_execution_success; // We have a sequence of if clauses, with a final else, resulting in a single job list that we @@ -238,7 +235,7 @@ parse_execution_result_t parse_execution_context_t::run_if_statement( tnode_t if_clause = statement.child<0>(); tnode_t else_clause = statement.child<1>(); for (;;) { - if (should_cancel_execution(ib)) { + if (should_cancel_execution(associated_block)) { result = parse_execution_cancelled; break; } @@ -249,9 +246,9 @@ parse_execution_result_t parse_execution_context_t::run_if_statement( // Check the condition and the tail. We treat parse_execution_errored here as failure, in // accordance with historic behavior. - parse_execution_result_t cond_ret = run_job_conjunction(condition_head, ib); + parse_execution_result_t cond_ret = run_job_conjunction(condition_head, associated_block); if (cond_ret == parse_execution_success) { - cond_ret = run_job_list(condition_boolean_tail, ib); + cond_ret = run_job_list(condition_boolean_tail, associated_block); } const bool take_branch = (cond_ret == parse_execution_success) && proc_get_last_status() == EXIT_SUCCESS; @@ -284,20 +281,22 @@ parse_execution_result_t parse_execution_context_t::run_if_statement( // Execute any job list we got. if (job_list_to_execute) { + if_block_t *ib = parser->push_block(); run_job_list(job_list_to_execute, ib); + if (should_cancel_execution(ib)) { + result = parse_execution_cancelled; + } + parser->pop_block(ib); } else { // No job list means no sucessful conditions, so return 0 (issue #1443). proc_set_last_status(STATUS_CMD_OK); } // It's possible there's a last-minute cancellation (issue #1297). - if (should_cancel_execution(ib)) { + if (should_cancel_execution(associated_block)) { result = parse_execution_cancelled; } - // Done - parser->pop_block(ib); - // Otherwise, take the exit status of the job list. Reversal of issue #1061. return result; } @@ -337,7 +336,7 @@ parse_execution_result_t parse_execution_context_t::run_function_statement( } parse_execution_result_t parse_execution_context_t::run_block_statement( - tnode_t statement) { + tnode_t statement, const block_t *associated_block) { tnode_t bheader = statement.child<0>(); tnode_t contents = statement.child<1>(); @@ -345,7 +344,7 @@ parse_execution_result_t parse_execution_context_t::run_block_statement( if (auto header = bheader.try_get_child()) { ret = run_for_statement(header, contents); } else if (auto header = bheader.try_get_child()) { - ret = run_while_statement(header, contents); + ret = run_while_statement(header, contents, associated_block); } else if (auto header = bheader.try_get_child()) { ret = run_function_statement(header, contents); } else if (auto header = bheader.try_get_child()) { @@ -520,10 +519,8 @@ parse_execution_result_t parse_execution_context_t::run_switch_statement( } parse_execution_result_t parse_execution_context_t::run_while_statement( - tnode_t header, tnode_t contents) { - // Push a while block. - while_block_t *wb = parser->push_block(); - + tnode_t header, tnode_t contents, + const block_t *associated_block) { parse_execution_result_t ret = parse_execution_success; // The conditions of the while loop. @@ -533,9 +530,10 @@ parse_execution_result_t parse_execution_context_t::run_while_statement( // Run while the condition is true. for (;;) { // Check the condition. - parse_execution_result_t cond_ret = this->run_job_conjunction(condition_head, wb); + parse_execution_result_t cond_ret = + this->run_job_conjunction(condition_head, associated_block); if (cond_ret == parse_execution_success) { - cond_ret = run_job_list(condition_boolean_tail, wb); + cond_ret = run_job_list(condition_boolean_tail, associated_block); } // We only continue on successful execution and EXIT_SUCCESS. @@ -544,21 +542,23 @@ parse_execution_result_t parse_execution_context_t::run_while_statement( } // Check cancellation. - if (this->should_cancel_execution(wb)) { + if (this->should_cancel_execution(associated_block)) { ret = parse_execution_cancelled; break; } - // The block ought to go inside the loop (see issue #1212). + // Push a while block and then check its cancellation reason. + while_block_t *wb = parser->push_block(); this->run_job_list(contents, wb); + auto loop_status = wb->loop_status; + auto cancel_reason = this->cancellation_reason(wb); + parser->pop_block(wb); - if (this->cancellation_reason(wb) == execution_cancellation_loop_control) { + if (cancel_reason == execution_cancellation_loop_control) { // Handle break or continue. - if (wb->loop_status == LOOP_CONTINUE) { - // Reset the loop state. - wb->loop_status = LOOP_NORMAL; + if (loop_status == LOOP_CONTINUE) { continue; - } else if (wb->loop_status == LOOP_BREAK) { + } else if (loop_status == LOOP_BREAK) { break; } } @@ -569,9 +569,6 @@ parse_execution_result_t parse_execution_context_t::run_while_statement( break; } } - - // Done - parser->pop_block(wb); return ret; } @@ -1092,11 +1089,12 @@ parse_execution_result_t parse_execution_context_t::run_1_job(tnode_t jo assert(specific_statement_type_is_redirectable_block(specific_statement)); switch (specific_statement.type) { case symbol_block_statement: { - result = this->run_block_statement({&tree(), &specific_statement}); + result = + this->run_block_statement({&tree(), &specific_statement}, associated_block); break; } case symbol_if_statement: { - result = this->run_if_statement({&tree(), &specific_statement}); + result = this->run_if_statement({&tree(), &specific_statement}, associated_block); break; } case symbol_switch_statement: { @@ -1265,9 +1263,9 @@ parse_execution_result_t parse_execution_context_t::eval_node(tnode_t block_io_push(&block_io, io); enum parse_execution_result_t status = parse_execution_success; if (auto block = statement.try_get_child()) { - status = this->run_block_statement(block); + status = this->run_block_statement(block, associated_block); } else if (auto ifstat = statement.try_get_child()) { - status = this->run_if_statement(ifstat); + status = this->run_if_statement(ifstat, associated_block); } else if (auto switchstat = statement.try_get_child()) { status = this->run_switch_statement(switchstat); } else { diff --git a/src/parse_execution.h b/src/parse_execution.h index dceb87235..01a77b3ce 100644 --- a/src/parse_execution.h +++ b/src/parse_execution.h @@ -95,13 +95,16 @@ class parse_execution_context_t { tnode_t specific_statement); // These encapsulate the actual logic of various (block) statements. - parse_execution_result_t run_block_statement(tnode_t statement); + parse_execution_result_t run_block_statement(tnode_t statement, + const block_t *associated_block); parse_execution_result_t run_for_statement(tnode_t header, tnode_t contents); - parse_execution_result_t run_if_statement(tnode_t statement); + parse_execution_result_t run_if_statement(tnode_t statement, + const block_t *associated_block); parse_execution_result_t run_switch_statement(tnode_t statement); parse_execution_result_t run_while_statement(tnode_t statement, - tnode_t contents); + tnode_t contents, + const block_t *associated_block); parse_execution_result_t run_function_statement(tnode_t header, tnode_t body); parse_execution_result_t run_begin_statement(tnode_t contents); diff --git a/tests/set.err b/tests/set.err index 196a395a2..39e8350e7 100644 --- a/tests/set.err +++ b/tests/set.err @@ -20,3 +20,6 @@ $argle bargle: invalid var name #################### # Exporting works + +#################### +# if/for/while scope diff --git a/tests/set.in b/tests/set.in index 0fdcbe2eb..1db01f32b 100644 --- a/tests/set.in +++ b/tests/set.in @@ -60,3 +60,13 @@ set -x TESTVAR0 set -x TESTVAR1 a set -x TESTVAR2 a b env | grep TESTVAR | cat -v + +logmsg if/for/while scope +function test_ifforwhile_scope + if set -l ifvar1 (true && echo val1) ; end + if set -l ifvar2 (echo val2 && false) ; end + if false ; else if set -l ifvar3 (echo val3 && false) ; end + while set -l whilevar1 (echo val3 ; false) ; end + set --show ifvar1 ifvar2 ifvar3 whilevar1 +end +test_ifforwhile_scope diff --git a/tests/set.out b/tests/set.out index 3d4535bab..e429c2e66 100644 --- a/tests/set.out +++ b/tests/set.out @@ -106,3 +106,26 @@ $var6: not set in universal scope TESTVAR0= TESTVAR1=a TESTVAR2=a^^b + +#################### +# if/for/while scope +$ifvar1: set in local scope, unexported, with 1 elements +$ifvar1[1]: length=4 value=|val1| +$ifvar1: not set in global scope +$ifvar1: not set in universal scope + +$ifvar2: set in local scope, unexported, with 1 elements +$ifvar2[1]: length=4 value=|val2| +$ifvar2: not set in global scope +$ifvar2: not set in universal scope + +$ifvar3: set in local scope, unexported, with 1 elements +$ifvar3[1]: length=4 value=|val3| +$ifvar3: not set in global scope +$ifvar3: not set in universal scope + +$whilevar1: set in local scope, unexported, with 1 elements +$whilevar1[1]: length=4 value=|val3| +$whilevar1: not set in global scope +$whilevar1: not set in universal scope + diff --git a/tests/test8.err b/tests/test8.err index 279b7f5aa..46ecfda19 100644 --- a/tests/test8.err +++ b/tests/test8.err @@ -19,3 +19,6 @@ #################### # Catch this corner case, which should produce an error + +#################### +# Loop control in conditions diff --git a/tests/test8.in b/tests/test8.in index 2769765e6..19d645b1a 100644 --- a/tests/test8.in +++ b/tests/test8.in @@ -39,3 +39,13 @@ if false ; or true | false ; echo "failure5" ; end logmsg Catch this corner case, which should produce an error if false ; or --help ; end + +logmsg Loop control in conditions +for i in 1 2 3 + while break; end + echo $i +end +for i in 1 2 3 + while continue; end + echo $i +end diff --git a/tests/test8.out b/tests/test8.out index eb337f273..347669b16 100644 --- a/tests/test8.out +++ b/tests/test8.out @@ -37,3 +37,6 @@ success4 #################### # Catch this corner case, which should produce an error + +#################### +# Loop control in conditions