From 1d21e3f47057f238b4fa3d3b9b8fcd8737d110c8 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sun, 20 Jan 2019 16:37:20 -0800 Subject: [PATCH] Make while loops evaluate to the last executed command status A while loop now evaluates to the last executed command in the body, or zero if the loop body is empty. This matches POSIX semantics. Add a bunch of tricky tests. See #4982 --- CHANGELOG.md | 1 + src/parse_execution.cpp | 33 +++++++++++++------ tests/while.err | 3 ++ tests/while.in | 73 +++++++++++++++++++++++++++++++++++++++++ tests/while.out | 12 +++++++ 5 files changed, 112 insertions(+), 10 deletions(-) create mode 100644 tests/while.err create mode 100644 tests/while.in create mode 100644 tests/while.out diff --git a/CHANGELOG.md b/CHANGELOG.md index d7b6f1aa2..1b27bd483 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,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 68a63c7c1..5d16f66ad 100644 --- a/src/parse_execution.cpp +++ b/src/parse_execution.cpp @@ -523,13 +523,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); @@ -537,8 +552,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; } @@ -548,8 +568,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); @@ -572,11 +590,6 @@ parse_execution_result_t parse_execution_context_t::run_while_statement( break; } } - - if (loop_executed) { - proc_set_last_status(STATUS_CMD_OK); - } - return ret; } diff --git a/tests/while.err b/tests/while.err new file mode 100644 index 000000000..2549f3894 --- /dev/null +++ b/tests/while.err @@ -0,0 +1,3 @@ + +#################### +# Loops exit status handling diff --git a/tests/while.in b/tests/while.in new file mode 100644 index 000000000..d183a54f6 --- /dev/null +++ b/tests/while.in @@ -0,0 +1,73 @@ +# vim: set ft=fish: + +function never_runs + while false + end +end + +function early_return + while true + return 2 + end +end + +function runs_once + set -l i 1 + while test $i -ne 0 && set i (math $i - 1) + end +end + +# this should return 1 +never_runs; echo "Empty Loop in Function: $status" + +# this should return 0 +runs_once; echo "Runs Once: $status" + +# this should return 2 +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 new file mode 100644 index 000000000..f23708aa4 --- /dev/null +++ b/tests/while.out @@ -0,0 +1,12 @@ +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