diff --git a/CHANGELOG.md b/CHANGELOG.md index 922b30a0f..e351b4279 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,7 @@ ### Fixes and improvements - exec now behaves properly inside functions (#5449) +- while loops now evaluate to the last executed command in the loop body (or zero if the body was empty), matching POSIX semantics. # fish 3.0.0 (released December 28, 2018) diff --git a/src/parse_execution.cpp b/src/parse_execution.cpp index 3ac569069..cb2fa3b08 100644 --- a/src/parse_execution.cpp +++ b/src/parse_execution.cpp @@ -524,13 +524,28 @@ parse_execution_result_t parse_execution_context_t::run_while_statement( const block_t *associated_block) { parse_execution_result_t ret = parse_execution_success; + // "The exit status of the while loop shall be the exit status of the last compound-list-2 + // executed, or zero if none was executed." + // Here are more detailed requirements: + // - If we execute the loop body zero times, or the loop body is empty, the status is success. + // - An empty loop body is treated as true, both in the loop condition and after loop exit. + // - The exit status of the last command is visible in the loop condition. (i.e. do not set the + // exit status to true BEFORE executing the loop condition). + // We achieve this by restoring the status if the loop condition fails, plus a special + // affordance for the first condition. + bool first_cond_check = true; + // The conditions of the while loop. tnode_t condition_head = header.child<1>(); tnode_t condition_boolean_tail = header.child<3>(); // Run while the condition is true. - bool loop_executed = false; for (;;) { + // Save off the exit status if it came from the loop body. We'll restore it if the condition + // is false. + int cond_saved_status = first_cond_check ? EXIT_SUCCESS : proc_get_last_status(); + first_cond_check = false; + // Check the condition. parse_execution_result_t cond_ret = this->run_job_conjunction(condition_head, associated_block); @@ -538,8 +553,13 @@ parse_execution_result_t parse_execution_context_t::run_while_statement( cond_ret = run_job_list(condition_boolean_tail, associated_block); } - // We only continue on successful execution and EXIT_SUCCESS. - if (cond_ret != parse_execution_success || proc_get_last_status() != EXIT_SUCCESS) { + // If the loop condition failed to execute, then exit the loop without modifying the exit + // status. If the loop condition executed with a failure status, restore the status and then + // exit the loop. + if (cond_ret != parse_execution_success) { + break; + } else if (proc_get_last_status() != EXIT_SUCCESS) { + proc_set_last_status(cond_saved_status); break; } @@ -549,8 +569,6 @@ parse_execution_result_t parse_execution_context_t::run_while_statement( break; } - loop_executed = true; - // Push a while block and then check its cancellation reason. while_block_t *wb = parser->push_block(); this->run_job_list(contents, wb); @@ -573,16 +591,6 @@ parse_execution_result_t parse_execution_context_t::run_while_statement( break; } } - - // $status after `while` should be 0 if it executed at least once, otherwise the last `$status` - // obtained by executing the condition is preserved. See #4982. - if (loop_executed) { - // Do not override status if exiting due to the presence of an explict `return xxx` (#5513) - if (!associated_block->skip) { - proc_set_last_status(STATUS_CMD_OK); - } - } - return ret; } diff --git a/tests/while.err b/tests/while.err index e69de29bb..2549f3894 100644 --- a/tests/while.err +++ b/tests/while.err @@ -0,0 +1,3 @@ + +#################### +# Loops exit status handling diff --git a/tests/while.in b/tests/while.in index 95cb2584c..d183a54f6 100644 --- a/tests/while.in +++ b/tests/while.in @@ -18,10 +18,56 @@ function runs_once end # this should return 1 -never_runs; echo $status +never_runs; echo "Empty Loop in Function: $status" # this should return 0 -runs_once; echo $status +runs_once; echo "Runs Once: $status" # this should return 2 -early_return; echo $status +early_return; echo "Early Return: $status" + +logmsg Loops exit status handling + +function set_status ; return $argv[1]; end + +# The previous status is visible in the loop condition. +# This includes both the incoming status, and the last command in the +# loop body. +set_status 36 +while begin + set -l saved $status + echo "Condition Status: $status" + set_status $saved + end + true +end + +# The condition status IS visible in the loop body. +set_status 55 +while true + echo "Body Status: $status" + break +end + +# The status of the last command is visible in the loop condition +set_status 13 +while begin + set -l saved $status + echo "Condition 2 Status: $saved" + test $saved -ne 5 + end + set_status 5 +end + +# The status of the last command is visible outside the loop +set rem 5 7 11 +while [ (count $rem) -gt 0 ] + set_status $rem[1] + set rem $rem[2..-1] +end +echo "Loop Exit Status: $status" + +# Empty loops succeed. +false +while false; end +echo "Empty Loop Status: $status" diff --git a/tests/while.out b/tests/while.out index 56f24a18d..f23708aa4 100644 --- a/tests/while.out +++ b/tests/while.out @@ -1,3 +1,12 @@ -1 -0 -2 +Empty Loop in Function: 0 +Runs Once: 0 +Early Return: 2 + +#################### +# Loops exit status handling +Condition Status: 36 +Body Status: 0 +Condition 2 Status: 13 +Condition 2 Status: 5 +Loop Exit Status: 11 +Empty Loop Status: 0