Re: 'set -e' with '!' POSIX issue

[Peter Stephenson <p.stephenson@xxxxxxxxxxx>]

On Wed, 5 Oct 2016 12:18:26 +0200
Martijn Dekker <martijn@xxxxxxxx> wrote:
> On Tue, Mar 17, 2009 at 12:46:20PM +0100, Vincent Lefevre wrote:
> > POSIX shells (bash, dash, ksh93, pdksh, posh) return with no output
> > and an exit status equal to 1 on:
> >
> >   sh -c 'set -e; foo() { false && false; }; foo; echo $?'
> >
> > but zsh doesn't, even with "emulate sh":
> >
> > $ zsh -fc 'emulate sh; set -e; foo() { false && false; }; foo; echo $?'
> > 1

I think this is a bug, regardless of the standard.  The "&&" within the
function shouldn't have anything to do with what happens outside, and it
works without that.

The fix appears to be similar to the one for "!".

Please reassure me that this is correct:

% ./zsh -fc 'set -e; foo() { false && false; echo OK; }; foo; echo $?'
OK
0

We hit the first "false" in the &&, which doesn't trigger ERR_EXIT so
the second "false" that would is never executed; therefore we proceed to
"echo OK"; therefore the status of the function is 0.  If there's no
controversy I may add this as a test case for future reference, together
with the "true & false" case that causes the function to be aborted.

pws

diff --git a/Src/exec.c b/Src/exec.c
index 741c80e..c0ed2c4 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -1229,7 +1229,7 @@ execlist(Estate state, int dont_change_job, int exiting)
     }
     while (wc_code(code) == WC_LIST && !breaks && !retflag && !errflag) {
 	int donedebug;
-	int this_noerrexit = 0;
+	int this_noerrexit = 0, this_donetrap = 0;
 
 	ltype = WC_LIST_TYPE(code);
 	csp = cmdsp;
@@ -1353,10 +1353,10 @@ execlist(Estate state, int dont_change_job, int exiting)
 			/* We've skipped to the end of the list, not executing *
 			 * the final pipeline, so don't perform error handling *
 			 * for this sublist.                                   */
-			donetrap = 1;
+			this_donetrap = 1;
 			goto sublist_done;
 		    } else if (WC_SUBLIST_TYPE(code) == WC_SUBLIST_END) {
-			donetrap = 1;
+			this_donetrap = 1;
 			/*
 			 * Treat this in the same way as if we reached
 			 * the end of the sublist normally.
@@ -1386,10 +1386,10 @@ execlist(Estate state, int dont_change_job, int exiting)
 			/* We've skipped to the end of the list, not executing *
 			 * the final pipeline, so don't perform error handling *
 			 * for this sublist.                                   */
-			donetrap = 1;
+			this_donetrap = 1;
 			goto sublist_done;
 		    } else if (WC_SUBLIST_TYPE(code) == WC_SUBLIST_END) {
-			donetrap = 1;
+			this_donetrap = 1;
 			/*
 			 * Treat this in the same way as if we reached
 			 * the end of the sublist normally.
@@ -1439,7 +1439,7 @@ sublist_done:
 	/* Check whether we are suppressing traps/errexit *
 	 * (typically in init scripts) and if we haven't  *
 	 * already performed them for this sublist.       */
-	if (!noerrexit && !this_noerrexit && !donetrap) {
+	if (!noerrexit && !this_noerrexit && !donetrap && !this_donetrap) {
 	    if (sigtrapped[SIGZERR] && lastval) {
 		dotrap(SIGZERR);
 		donetrap = 1;
diff --git a/Test/C03traps.ztst b/Test/C03traps.ztst
index 0faec02..5057dcf 100644
--- a/Test/C03traps.ztst
+++ b/Test/C03traps.ztst
@@ -476,7 +476,7 @@
     fi
   }
   fn
-0:ERRRETURN not triggered in if condition
+0:ERR_RETURN not triggered in if condition
 >Oh, yes
 
   fn() {
@@ -490,7 +490,7 @@
     fi
   }
   fn
-1:ERRRETURN in "if"
+1:ERR_RETURN in "if"
 
   fn() {
     emulate -L zsh
@@ -503,7 +503,7 @@
     fi
   }
   fn
-1:ERRRETURN in "else" branch (regression test)
+1:ERR_RETURN in "else" branch (regression test)
 
   $ZTST_testdir/../Src/zsh -f =(<<<"
   if false; then
@@ -515,7 +515,7 @@
     print Yes
   fi
   ")
-0:ERRRETURN when false "if" is the first statement in an "else" (regression)
+0:ERR_RETURN when false "if" is the first statement in an "else" (regression)
 >Yes
 F:Must be tested with a top-level script rather than source or function
 
@@ -527,7 +527,7 @@ F:Must be tested with a top-level script rather than source or function
       print after
   }
   fn
-1:ERRRETURN, basic case
+1:ERR_RETURN, basic case
 >before
 
   fn() {
@@ -539,7 +539,7 @@ F:Must be tested with a top-level script rather than source or function
       print after
   }
   fn
-0:ERRETURN with "!"
+0:ERR_RETURN with "!"
 >before
 >after
 
@@ -553,7 +553,7 @@ F:Must be tested with a top-level script rather than source or function
       print after
   }
   fn
-1:ERRETURN with "!" and a following false
+1:ERR_RETURN with "!" and a following false
 >before
 
   fn() {
@@ -566,7 +566,7 @@ F:Must be tested with a top-level script rather than source or function
       print after
   }
   fn
-0:ERRETURN with "!" suppressed inside complex structure
+0:ERR_RETURN with "!" suppressed inside complex structure
 >before
 >after
 
@@ -580,9 +580,22 @@ F:Must be tested with a top-level script rather than source or function
       print after
   }
   fn
-1:ERRETURN with no "!" suppression (control case)
+1:ERR_RETURN with no "!" suppression (control case)
 >before
 
+  (setopt err_return
+    fn() {
+      print before-in
+      false && false
+    }
+    print before-out
+    fn
+    print after-out
+  )
+1:ERR_RETURN with "&&" in function (regression test)
+>before-out
+>before-in
+
 %clean
 
   rm -f TRAPEXIT