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

@@ -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)

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<g::job_conjunction> condition_head = header.child<1>();
     tnode_t<g::andor_job_list> 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<while_block_t>();
         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;
 }
 

tests/while.err

@@ -0,0 +1,3 @@
+
+####################
+# Loops exit status handling

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"

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