[PATCH] Fix ERR_EXIT behavior in eval and source statements and better document the noerrexit variables

[Philippe Altherr <philippe.altherr@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2318 bytes --]

ATTENTION: This patch depends on the following patches described here
<https://inbox.vuxu.org/zsh-workers/CAGdYchufLwUgpKVU6b5eOiF15p0njFr59H1q9XizypwNjzqKzw@mail.gmail.com/>
:
- patch-01-revert-mistaken-errexit-patches.txt
- patch-03-fix-negation-statement.txt
- patch-04-fix-function-call.txt

After quite some thinking and code studying, I realized that what is really
missing/wrong is a lack of this_noerrexit resetting in the execlist
function. Adding that fixes the problems for the eval and source
statements. It also makes my previous fix for function calls, in patch
patch-04-fix-function-call.txt, redundant. Therefore, the patch below
reverts that fix. Since that other patch isn't submitted yet, it could be
combined with the one below into a single patch. If you prefer that, let me
know and I will prepare a combined patch.

The patch also significantly expands the description of the variables
noerrexit and this_noerrexit. Since I spent so much time understanding the
role and correct usage of these variables, I figured that writing down my
findings might prove useful to future developers.

For the record, here are examples of eval and source statements that don't
behave correctly in the current Zsh:

set -e
> eval "{ false && true; }"
> echo done


The eval statement should trigger an exit but the current Zsh keeps going.

set -e
> source <(echo '{ false && true; }')
> echo done


The source statement should trigger an exit but the current Zsh keeps going.

While working on the patch I discovered yet another case where the current
Zsh misbehaves:

set -e
> { false && true; } || false;
> echo done


The second "false" should trigger and exit but the current Zsh keeps going.

I was expecting that the current Zsh would also misbehave for the following
case. Interestingly, it doesn't.

set -e
> x=$({ false && true; })
> echo done


The assignment correctly triggers an exit. Why is this working while the
similar examples with eval and source statements don't? The reason is that
in this case the "{ false && true; }" is evaluated in a sub-shell.
Technically, the evaluation in the sub-shell ends with an incorrect value
of this_noerrexit. However, that doesn't impact the evaluation of the
assignment, which happens in the parent shell, where this_noerrexit remains
unchanged.

Philippe

[-- Attachment #1.2: Type: text/html, Size: 3343 bytes --]

[-- Attachment #2: patch-06-fix-eval-and-source-statements.txt --]
[-- Type: text/plain, Size: 5228 bytes --]

diff --git a/Src/exec.c b/Src/exec.c
index 711d8f374..8ff6489ec 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -56,7 +56,17 @@ struct funcsave {
 typedef struct funcsave *Funcsave;
 
 /*
- * used to suppress ERREXIT and trapping of SIGZERR, SIGEXIT.
+ * Used to suppress ERREXIT and trapping of SIGZERR, SIGEXIT in the
+ * evaluation of sub-commands of the command under evaluation. The
+ * variable must be updated before the evaluation of the sub-commands
+ * starts and restored to its previous state right after that
+ * evaluation ends. The variable is read and acted upon in execlist.
+ *
+ * A good usage example can be found in execwhile in loop.c, which
+ * evaluates while statements. The variable is updated to disable
+ * ERREXIT just before evaluating the while's condition and restored
+ * to its previous state right after the evaluation of the condition.
+ *
  * Bits from noerrexit_bits.
  */
 
@@ -64,7 +74,36 @@ typedef struct funcsave *Funcsave;
 int noerrexit;
 
 /*
- * used to suppress ERREXIT and ERRRETURN for the command under evaluation.
+ * Used to suppress ERREXIT and ERRRETURN for the command under
+ * evaluation.  The variable must be enabled (set to 1) at the very
+ * end of the evaluation of the command. It must come after the
+ * evaluation of any sub-commands of the command under evaluation. The
+ * variable is read and acted upon in execlist, which also takes care
+ * of initialising and resetting it to 0.
+ *
+ * Unlike the variable noerrexit, whose state applies to the
+ * evaluation of whole sub-commands (and their direct and indirect
+ * sub-commands), the scope of the variable this_noerrexit is much
+ * more localized. ERREXIT and ERRRETURN are triggered at the end of
+ * the function execlist after the evaluation of some or all of the
+ * list's sub-commands. The role of the variable this_noerrexit is to
+ * give to the functions evaluating the list's sub-commands the
+ * possibility to tell the calling execlist not to trigger ERREXIT and
+ * ERRRETURN. In other words, the variable acts as an additional
+ * return value between the called evaluation functions and the
+ * calling execlist. For that reason the variable must always be set
+ * as late as possible and in particular after any sub-command
+ * evaluation. If the variable is set before the evaluation of a
+ * sub-command, if may affect the wrong execlist, if the sub-command
+ * evaluation involves another execlist call, and/or the variable may
+ * get modified by the sub-command evaluation and thus wouldn't return
+ * the desired value to the calling execlist.
+ *
+ * Good usage examples can be found in the exec functions in loop.c,
+ * which evaluate compound commands. The variable is enabled right
+ * before returning from the functions, after all the sub-commands of
+ * the compound commands have already been evaluated.
+ *
  * 0 or 1
  */
 
@@ -1427,6 +1466,7 @@ execlist(Estate state, int dont_change_job, int exiting)
 	    goto sublist_done;
 	}
 	while (wc_code(code) == WC_SUBLIST) {
+	    this_noerrexit = 0;
 	    int isandor = WC_SUBLIST_TYPE(code) != WC_SUBLIST_END;
 	    int isnot = WC_SUBLIST_FLAGS(code) & WC_SUBLIST_NOT;
 	    next = state->pc + WC_SUBLIST_SKIP(code);
@@ -1582,6 +1622,7 @@ sublist_done:
 	    break;
 	code = *state->pc++;
     }
+    this_noerrexit = 0;
     pline_level = old_pline_level;
     list_pipe = old_list_pipe;
     list_pipe_job = old_list_pipe_job;
@@ -5999,7 +6040,6 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int noreturnval)
 	    trap_return++;
 	ret = lastval;
 	noerrexit = funcsave->noerrexit;
-	this_noerrexit = 0;
 	if (noreturnval) {
 	    lastval = funcsave->lastval;
 	    numpipestats = funcsave->numpipestats;
diff --git a/Test/C03traps.ztst b/Test/C03traps.ztst
index b7132da81..e0b6afb5f 100644
--- a/Test/C03traps.ztst
+++ b/Test/C03traps.ztst
@@ -954,6 +954,47 @@ F:Must be tested with a top-level script rather than source or function
 1:ERR_EXIT triggered by status 1 at end of anon func
 >Still functioning
 
+  (setopt err_exit
+  loop=true; while print loop $? >&2; $loop; do loop=false; false && true; done
+  print done $? >&2
+  )
+0: ERR_EXIT neither triggered inside loop nor triggered by while statement
+?loop 0
+?loop 1
+?done 1
+
+  (setopt err_exit
+  { loop=true; while print loop $? >&2; $loop; do loop=false; false && true; done } || false
+  print done $? >&2
+  )
+1: ERR_EXIT not triggered inside loop but triggered by rhs of ||
+?loop 0
+?loop 1
+
+  (setopt err_exit
+  eval 'loop=true; while print loop $? >&2; $loop; do loop=false; false && true; done'
+  print done $? >&2
+  )
+1: ERR_EXIT not triggered inside loop but triggered by eval
+?loop 0
+?loop 1
+
+  (setopt err_exit
+  source <(echo 'loop=true; while print loop $? >&2; $loop; do loop=false; false && true; done')
+  print done $? >&2
+  )
+1: ERR_EXIT not triggered inside loop but triggered by source
+?loop 0
+?loop 1
+
+  (setopt err_exit
+  v=$(loop=true; while print loop $? >&2; $loop; do loop=false; false && true; done)
+  print done $? >&2
+  )
+1: ERR_EXIT not triggered inside loop but triggered by command substitution
+?loop 0
+?loop 1
+
   if zmodload zsh/system 2>/dev/null; then
   (
     trap 'echo TERM; exit 2' TERM