Re: [PATCH] Fix ERR_EXIT behavior in function calls and "always" statements

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

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

I have found that there is an error in the implementation of the negation
statement. Zsh currently fails to print "done" for the following code:

set -e
> fn() { true; }
> ! fn
> echo done $?


Fixing the negation statement allows a slightly simpler fix for function
calls. Instead of my original patch, I recommend submitting the 4 patches
attached here.

Philippe



On Wed, Nov 16, 2022 at 3:40 PM Philippe Altherr <philippe.altherr@gmail.com>
wrote:

> The attached patch fixes the ERR_EXIT behavior in function calls and
> "always" statements. The patch does the following:
>
>    - Revert the following patches, which are based on an unfortunate
>    misunderstanding of the expected behavior of ERR_EXIT:
>       -    50929: fix handling of ERR_RETURN bent by 50928.
>       <https://sourceforge.net/p/zsh/code/ci/8839e969bf8f3f129d0efd8ecd51505610a1f01b/>
>       -    50928: fix tests for 50897, mention behavior change in NEWS
>       <https://sourceforge.net/p/zsh/code/ci/1ba8714a7ac665e661c1b3a716ffe2af73d1e443/>
>          -       I kept the localoptions fix in one of the tests.
>       -    50897: nonzero status of complex commands should trigger
>       ERR_EXIT
>       <https://sourceforge.net/p/zsh/code/ci/d873ed6026d7b0c48d6e65ec06df491d015a4d59/>
>    - Add saving and restoring of local_noerrexit in doshfunc in exec.c
>       - This fixes the ERR_EXIT behavior in function calls.
>    - Add "this_noerrexit = 1;" at the very end of exectry in loop.c
>       - This makes "always" statements compliant with the exception 3 of
>       the POSIX specification of "set -e".
>    - Add new tests in C03traps.ztst
>
> Philippe
>
>

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

[-- Attachment #2: patch-01-revert-mistaken-errexit-patches.txt --]
[-- Type: text/plain, Size: 4234 bytes --]

diff --git a/NEWS b/NEWS
index 9c28169bb..cdafd1ff5 100644
--- a/NEWS
+++ b/NEWS
@@ -4,15 +4,6 @@ CHANGES FROM PREVIOUS VERSIONS OF ZSH
 
 Note also the list of incompatibilities in the README file.
 
-Changes since 5.9
------------------
-
-Handling of ERR_EXIT is corrected when the final status of a structured
-command (for, select, while, repeat, if, case, or a list in braces) is
-nonzero.  To be compatible with other shells, "zsh -e" now exits in
-those circumstances, whereas previous versions did not.  This does not
-affect the handling of nonzero status within conditional statements.
-
 Changes since 5.8.1
 -------------------
 
diff --git a/Src/exec.c b/Src/exec.c
index ce0c1f1ad..b0f42ae67 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -451,7 +451,7 @@ execcursh(Estate state, int do_exec)
     cmdpop();
 
     state->pc = end;
-    this_noerrexit = (WC_SUBLIST_TYPE(*end) != WC_SUBLIST_END);
+    this_noerrexit = 1;
 
     return lastval;
 }
@@ -1442,8 +1442,6 @@ execlist(Estate state, int dont_change_job, int exiting)
 		    execsimple(state);
 		else
 		    execpline(state, code, ltype, (ltype & Z_END) && exiting);
-		if (!locallevel || unset(ERRRETURN))
-		    this_noerrexit = noerrexit;
 		state->pc = next;
 		goto sublist_done;
 		break;
diff --git a/Src/loop.c b/Src/loop.c
index be5261369..db5b3e097 100644
--- a/Src/loop.c
+++ b/Src/loop.c
@@ -208,7 +208,7 @@ execfor(Estate state, int do_exec)
     loops--;
     simple_pline = old_simple_pline;
     state->pc = end;
-    this_noerrexit = (WC_SUBLIST_TYPE(*end) != WC_SUBLIST_END);
+    this_noerrexit = 1;
     return lastval;
 }
 
@@ -336,7 +336,7 @@ execselect(Estate state, UNUSED(int do_exec))
     loops--;
     simple_pline = old_simple_pline;
     state->pc = end;
-    this_noerrexit = (WC_SUBLIST_TYPE(*end) != WC_SUBLIST_END);
+    this_noerrexit = 1;
     return lastval;
 }
 
@@ -478,7 +478,7 @@ execwhile(Estate state, UNUSED(int do_exec))
     popheap();
     loops--;
     state->pc = end;
-    this_noerrexit = (WC_SUBLIST_TYPE(*end) != WC_SUBLIST_END);
+    this_noerrexit = 1;
     return lastval;
 }
 
@@ -532,7 +532,7 @@ execrepeat(Estate state, UNUSED(int do_exec))
     loops--;
     simple_pline = old_simple_pline;
     state->pc = end;
-    this_noerrexit = (WC_SUBLIST_TYPE(*end) != WC_SUBLIST_END);
+    this_noerrexit = 1;
     return lastval;
 }
 
@@ -587,7 +587,7 @@ execif(Estate state, int do_exec)
 	    lastval = 0;
     }
     state->pc = end;
-    this_noerrexit = (WC_SUBLIST_TYPE(*end) != WC_SUBLIST_END);
+    this_noerrexit = 1;
 
     return lastval;
 }
@@ -701,7 +701,7 @@ execcase(Estate state, int do_exec)
 
     if (!anypatok)
 	lastval = 0;
-    this_noerrexit = (WC_SUBLIST_TYPE(*end) != WC_SUBLIST_END);
+    this_noerrexit = 1;
 
     return lastval;
 }
diff --git a/Test/C03traps.ztst b/Test/C03traps.ztst
index 5cc45e2de..a7a040d70 100644
--- a/Test/C03traps.ztst
+++ b/Test/C03traps.ztst
@@ -726,7 +726,8 @@ F:Must be tested with a top-level script rather than source or function
   done
   print OK
   )
-1:ERR_EXIT triggered by status 1 at end of for
+0:ERR_EXIT not triggered by status 1 at end of for
+>OK
 
   (setopt err_exit
   integer x=0
@@ -735,7 +736,8 @@ F:Must be tested with a top-level script rather than source or function
   done
   print OK
   )
-1:ERR_EXIT triggered by status 1 at end of while
+0:ERR_EXIT not triggered by status 1 at end of while
+>OK
 
   (setopt err_exit
   repeat 1; do
@@ -743,7 +745,8 @@ F:Must be tested with a top-level script rather than source or function
   done
   print OK
   )
-1:ERR_EXIT triggered by status 1 at end of repeat
+0:ERR_EXIT not triggered by status 1 at end of repeat
+>OK
 
   (setopt err_exit
   if true; then
@@ -751,7 +754,8 @@ F:Must be tested with a top-level script rather than source or function
   fi
   print OK
   )
-1:ERR_EXIT triggered by status 1 at end of if
+0:ERR_EXIT not triggered by status 1 at end of if
+>OK
 
   (setopt err_exit
   {
@@ -759,7 +763,8 @@ F:Must be tested with a top-level script rather than source or function
   }
   print OK
   )
-1:ERR_EXIT triggered by status 1 at end of { }
+0:ERR_EXIT not triggered by status 1 at end of { }
+>OK
 
   unsetopt err_exit err_return
   (setopt err_exit

[-- Attachment #3: patch-02-fix-always-statement.txt --]
[-- Type: text/plain, Size: 1517 bytes --]

diff --git a/Src/loop.c b/Src/loop.c
index db5b3e097..7c3e04b8a 100644
--- a/Src/loop.c
+++ b/Src/loop.c
@@ -793,6 +793,7 @@ exectry(Estate state, int do_exec)
     cmdpop();
     popheap();
     state->pc = end;
+    this_noerrexit = 1;
 
     return endval;
 }
diff --git a/Test/C03traps.ztst b/Test/C03traps.ztst
index a7a040d70..4719dfd57 100644
--- a/Test/C03traps.ztst
+++ b/Test/C03traps.ztst
@@ -721,22 +721,19 @@ F:Must be tested with a top-level script rather than source or function
 >Good
 
   (setopt err_exit
-  for x in y; do
-    false && true
-  done
+  false && true
   print OK
   )
-0:ERR_EXIT not triggered by status 1 at end of for
+0:ERR_EXIT not triggered by "false && true"
 >OK
 
   (setopt err_exit
-  integer x=0
-  while (( ! x++ )); do
+  for x in y; do
     false && true
   done
   print OK
   )
-0:ERR_EXIT not triggered by status 1 at end of while
+0:ERR_EXIT not triggered by status 1 at end of for
 >OK
 
   (setopt err_exit
@@ -755,6 +752,31 @@ F:Must be tested with a top-level script rather than source or function
   print OK
   )
 0:ERR_EXIT not triggered by status 1 at end of if
+>OK
+
+  (setopt err_exit
+  loop=true
+  while print COND; $loop; do
+    loop=false
+    false && true
+  done
+  print OK
+  )
+0:ERR_EXIT not triggered by status 1 at end of while
+>COND
+>COND
+>OK
+
+  (setopt err_exit
+  {
+    false && true
+  } always {
+    print ALWAYS
+  }
+  print OK
+  )
+0:ERR_EXIT not triggered by status 1 at end of always
+>ALWAYS
 >OK
 
   (setopt err_exit

[-- Attachment #4: patch-03-fix-negation-statement.txt --]
[-- Type: text/plain, Size: 1801 bytes --]

diff --git a/Src/exec.c b/Src/exec.c
index b0f42ae67..d8501ca68 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -63,7 +63,10 @@ typedef struct funcsave *Funcsave;
 /**/
 int noerrexit;
 
-/* used to suppress ERREXIT or ERRRETURN for one occurrence: 0 or 1 */
+/*
+ * used to suppress ERREXIT and ERRRETURN for the command under evaluation.
+ * 0 or 1
+ */
 
 /**/
 int this_noerrexit;
@@ -1429,10 +1432,7 @@ execlist(Estate state, int dont_change_job, int exiting)
 	    if (!oldnoerrexit)
 		noerrexit = isend ? 0 : NOERREXIT_EXIT | NOERREXIT_RETURN;
 	    if (WC_SUBLIST_FLAGS(code) & WC_SUBLIST_NOT) {
-		/* suppress errexit for "! this_command" */
-		if (isend)
-		    this_noerrexit = 1;
-		/* suppress errexit for ! <list-of-shell-commands> */
+		/* suppress errexit for the commands in ! <list-of-commands> */
 		noerrexit = NOERREXIT_EXIT | NOERREXIT_RETURN;
 	    }
 	    switch (WC_SUBLIST_TYPE(code)) {
@@ -1443,6 +1443,9 @@ execlist(Estate state, int dont_change_job, int exiting)
 		else
 		    execpline(state, code, ltype, (ltype & Z_END) && exiting);
 		state->pc = next;
+		/* suppress errexit for the command "! ..." */
+		if (WC_SUBLIST_FLAGS(code) & WC_SUBLIST_NOT)
+		  this_noerrexit = 1;
 		goto sublist_done;
 		break;
 	    case WC_SUBLIST_AND:
diff --git a/Test/C03traps.ztst b/Test/C03traps.ztst
index 4719dfd57..08e24a32e 100644
--- a/Test/C03traps.ztst
+++ b/Test/C03traps.ztst
@@ -720,6 +720,21 @@ F:Must be tested with a top-level script rather than source or function
 0:ERR_RETURN in "else" branch in nested function
 >Good
 
+  (setopt err_exit
+  ! true
+  print OK
+  )
+0:ERR_EXIT not triggered by "! true"
+>OK
+
+  (setopt err_exit
+  fn() { true }
+  ! fn
+  print OK
+  )
+0:ERR_EXIT not triggered by "! fn"
+>OK
+
   (setopt err_exit
   false && true
   print OK

[-- Attachment #5: patch-04-fix-function-call.txt --]
[-- Type: text/plain, Size: 3811 bytes --]

diff --git a/Src/exec.c b/Src/exec.c
index d8501ca68..43df8211a 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -5932,15 +5932,6 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int noreturnval)
 	     * This function is forced to return.
 	     */
 	    retflag = 0;
-	    /*
-	     * The calling function isn't necessarily forced to return,
-	     * but it should be made sensitive to ERR_EXIT and
-	     * ERR_RETURN as the assumptions we made at the end of
-	     * constructs within this function no longer apply.  If
-	     * there are cases where this is not true, they need adding
-	     * to C03traps.ztst.
-	     */
-	    this_noerrexit = 0;
 	    breaks = funcsave->breaks;
 	}
 	freearray(pparams);
@@ -6010,6 +6001,7 @@ 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 08e24a32e..a8880673f 100644
--- a/Test/C03traps.ztst
+++ b/Test/C03traps.ztst
@@ -742,6 +742,15 @@ F:Must be tested with a top-level script rather than source or function
 0:ERR_EXIT not triggered by "false && true"
 >OK
 
+  (setopt err_exit
+  fn() {
+    false && true
+  }
+  fn
+  print OK
+  )
+1:ERR_EXIT not triggered by "false && true" but by return from fn
+
   (setopt err_exit
   for x in y; do
     false && true
@@ -751,6 +760,17 @@ F:Must be tested with a top-level script rather than source or function
 0:ERR_EXIT not triggered by status 1 at end of for
 >OK
 
+  (setopt err_exit
+  fn() {
+    for x in y; do
+      false && true
+    done
+  }
+  fn
+  print OK
+  )
+1:ERR_EXIT not triggered by status 1 at end of for but by return from fn
+
   (setopt err_exit
   repeat 1; do
     false && true
@@ -760,6 +780,17 @@ F:Must be tested with a top-level script rather than source or function
 0:ERR_EXIT not triggered by status 1 at end of repeat
 >OK
 
+  (setopt err_exit
+  fn() {
+    repeat 1; do
+      false && true
+    done
+  }
+  fn
+  print OK
+  )
+1:ERR_EXIT not triggered by status 1 at end of repeat but by return from fn
+
   (setopt err_exit
   if true; then
     false && true
@@ -769,6 +800,17 @@ F:Must be tested with a top-level script rather than source or function
 0:ERR_EXIT not triggered by status 1 at end of if
 >OK
 
+  (setopt err_exit
+  fn() {
+    if true; then
+      false && true
+    fi
+  }
+  fn
+  print OK
+  )
+1:ERR_EXIT not triggered by status 1 at end of if but by return from fn
+
   (setopt err_exit
   loop=true
   while print COND; $loop; do
@@ -782,6 +824,21 @@ F:Must be tested with a top-level script rather than source or function
 >COND
 >OK
 
+  (setopt err_exit
+  fn() {
+    loop=true
+    while print COND; $loop; do
+      loop=false
+      false && true
+    done
+  }
+  fn
+  print OK
+  )
+1:ERR_EXIT not triggered by status 1 at end of while but by return from fn
+>COND
+>COND
+
   (setopt err_exit
   {
     false && true
@@ -794,6 +851,20 @@ F:Must be tested with a top-level script rather than source or function
 >ALWAYS
 >OK
 
+  (setopt err_exit
+  fn() {
+    {
+      false && true
+    } always {
+      print ALWAYS
+    }
+  }
+  fn
+  print OK
+  )
+1:ERR_EXIT not triggered by status 1 at end of always but by return from fn
+>ALWAYS
+
   (setopt err_exit
   {
     false && true
@@ -803,6 +874,17 @@ F:Must be tested with a top-level script rather than source or function
 0:ERR_EXIT not triggered by status 1 at end of { }
 >OK
 
+  (setopt err_exit
+  fn() {
+    {
+      false && true
+    }
+  }
+  fn
+  print OK
+  )
+1:ERR_EXIT not triggered by status 1 at end of { } but by return from fn
+
   unsetopt err_exit err_return
   (setopt err_exit
   for x in y; do