41590 modified as 41595: make ERR_RETURN more logical.

It now operates separately at each function depth.

To keep ERR_EXIT global, make the noerrexit variable usd bit flags.

Extend tests.

9 files changed, 121 insertions, 20 deletions

diff --git a/ChangeLog b/ChangeLog
index 94b435ace..5c5d2a6ab 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,10 @@
+2017-08-24  Peter Stephenson  <p.w.stephenson@ntlworld.com>
+
+	* 41590 (modified as per 41595): README, Doc/Zsh/options.yo,
+	Src/exec.c, Src/init.c, Src/loop.c, Src/signals.c, Src/zsh.h,
+	Test/E01options.ztst: make ERR_RETURN work within each function
+	context, leaving ERR_EXIT global.
+
 2017-08-22  Daniel Shahaf  <d.s@daniel.shahaf.name>
 
 	* 41565: Functions/VCS_Info/VCS_INFO_patch2subject: vcs_info
diff --git a/Doc/Zsh/options.yo b/Doc/Zsh/options.yo
index 70092d681..42571fccd 100644
--- a/Doc/Zsh/options.yo
+++ b/Doc/Zsh/options.yo
@@ -1659,6 +1659,13 @@ as it is by default, and the option tt(ERR_EXIT) is found to have been set
 on exit, then the command for which the tt(DEBUG) trap is being executed is
 skipped.  The option is restored after the trap exits.
 
+Non-zero status in a command list containing tt(&&) or tt(||) is ignored
+for commands not at the end of the list.  Hence
+
+example(false && true)
+
+does not trigger exit.
+
 Exiting due to tt(ERR_EXIT) has certain interactions with asynchronous
 jobs noted in
 ifzman(the section JOBS in zmanref(zshmisc))\
@@ -1672,10 +1679,25 @@ cindex(function return, on error)
 cindex(return from function, on error)
 item(tt(ERR_RETURN))(
 If a command has a non-zero exit status, return immediately from the
-enclosing function.  The logic is identical to that for tt(ERR_EXIT),
+enclosing function.  The logic is similar to that for tt(ERR_EXIT),
 except that an implicit tt(return) statement is executed instead of an
 tt(exit).  This will trigger an exit at the outermost level of a
 non-interactive script.
+
+Normally this option inherits the behaviour of tt(ERR_EXIT) that
+code followed by `tt(&&)' `tt(||)' does not trigger a return.  Hence
+in the following:
+
+example(summit || true)
+
+no return is forced as the combined effect always has a zero return
+status.
+
+Note. however, that if tt(summit) in the above example is itself a
+function, code inside it is considered separately: it may force a return
+from tt(summit) (assuming the option remains set within tt(summit)), but
+not from the enclosing context.  This behaviour is different from
+tt(ERR_EXIT) which is unaffected by function scope.
 )
 pindex(EVAL_LINENO)
 pindex(NO_EVAL_LINENO)
diff --git a/README b/README
index f711b1738..d59ddfebb 100644
--- a/README
+++ b/README
@@ -81,6 +81,19 @@ against user defined widgets inadvertently reading from the tty device,
 and addresses the antisocial behaviour of running a command with its
 stdin closed.
 
+5) [New between 5.4.1 and 5.4.2] In previous versions of the shell, the
+following code:
+
+    () { setopt err_return; false; echo 'oh no' } && true
+
+printed "oh no", as the ERR_RETURN behaviour was suppressed when
+a function was executed on the left hand side of an "&&" list.  This was
+undocumented and inconvenient as it is generally more useful to consider
+execution within a function in isolation from its environment.  The shell
+now returns from the function on executing `false'.  (This is general
+to all functions; an anonymous function is shown here for compactness.)
+
+
 Incompatibilities between 5.0.8 and 5.3
 ----------------------------------------
 
diff --git a/Src/exec.c b/Src/exec.c
index 9996dffed..cd99733f1 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -41,12 +41,15 @@ enum {
     ADDVAR_RESTORE =  1 << 2
 };
 
-/* used to suppress ERREXIT and trapping of SIGZERR, SIGEXIT. */
+/*
+ * used to suppress ERREXIT and trapping of SIGZERR, SIGEXIT.
+ * Bits from noerrexit_bits.
+ */
 
 /**/
 int noerrexit;
 
-/* used to suppress ERREXIT for one occurrence */
+/* used to suppress ERREXIT or ERRRETURN for one occurrence: 0 or 1 */
 
 /**/
 int this_noerrexit;
@@ -1299,7 +1302,7 @@ execlist(Estate state, int dont_change_job, int exiting)
 	    int oerrexit_opt = opts[ERREXIT];
 	    Param pm;
 	    opts[ERREXIT] = 0;
-	    noerrexit = 1;
+	    noerrexit = NOERREXIT_EXIT | NOERREXIT_RETURN;
 	    if (ltype & Z_SIMPLE) /* skip the line number */
 		pc2++;
 	    pm = setsparam("ZSH_DEBUG_CMD", getpermtext(state->prog, pc2, 0));
@@ -1351,13 +1354,13 @@ execlist(Estate state, int dont_change_job, int exiting)
 	    int isend = (WC_SUBLIST_TYPE(code) == WC_SUBLIST_END);
 	    next = state->pc + WC_SUBLIST_SKIP(code);
 	    if (!oldnoerrexit)
-		noerrexit = !isend;
+		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> */
-		noerrexit = 1;
+		noerrexit = NOERREXIT_EXIT | NOERREXIT_RETURN;
 	    }
 	    switch (WC_SUBLIST_TYPE(code)) {
 	    case WC_SUBLIST_END:
@@ -1444,11 +1447,11 @@ sublist_done:
 
 	/*
 	 * See hairy code near the end of execif() for the
-	 * following.  "noerrexit == 2" only applies until
+	 * following.  "noerrexit " only applies until
 	 * we hit execcmd on the way down.  We're now
 	 * on the way back up, so don't restore it.
 	 */
-	if (oldnoerrexit != 2)
+	if (!(oldnoerrexit & NOERREXIT_UNTIL_EXEC))
 	    noerrexit = oldnoerrexit;
 
 	if (sigtrapped[SIGDEBUG] && !isset(DEBUGBEFORECMD) && !donedebug) {
@@ -1458,7 +1461,7 @@ sublist_done:
 	     */
 	    int oerrexit_opt = opts[ERREXIT];
 	    opts[ERREXIT] = 0;
-	    noerrexit = 1;
+	    noerrexit = NOERREXIT_EXIT | NOERREXIT_RETURN;
 	    exiting = donetrap;
 	    ret = lastval;
 	    dotrap(SIGDEBUG);
@@ -1474,16 +1477,19 @@ 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 && !this_donetrap) {
-	    if (sigtrapped[SIGZERR] && lastval) {
+	if (!this_noerrexit && !donetrap && !this_donetrap) {
+	    if (sigtrapped[SIGZERR] && lastval &&
+		!(noerrexit & NOERREXIT_EXIT)) {
 		dotrap(SIGZERR);
 		donetrap = 1;
 	    }
 	    if (lastval) {
 		int errreturn = isset(ERRRETURN) &&
-		    (isset(INTERACTIVE) || locallevel || sourcelevel);
-		int errexit = isset(ERREXIT) ||
-		    (isset(ERRRETURN) && !errreturn);
+		    (isset(INTERACTIVE) || locallevel || sourcelevel) &&
+		    !(noerrexit & NOERREXIT_RETURN);
+		int errexit = (isset(ERREXIT) ||
+			       (isset(ERRRETURN) && !errreturn)) &&
+		    !(noerrexit & NOERREXIT_EXIT);
 		if (errexit) {
 		    if (sigtrapped[SIGEXIT])
 			dotrap(SIGEXIT);
@@ -3019,7 +3025,7 @@ execcmd_exec(Estate state, Execcmd_params eparams,
 	preargs = NULL;
 
     /* if we get this far, it is OK to pay attention to lastval again */
-    if (noerrexit == 2 && !is_shfunc)
+    if ((noerrexit & NOERREXIT_UNTIL_EXEC) && !is_shfunc)
 	noerrexit = 0;
 
     /* Do prefork substitutions.
@@ -5462,6 +5468,7 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int noreturnval)
     char saveopts[OPT_SIZE], *oldscriptname = scriptname;
     char *name = shfunc->node.nam;
     int flags = shfunc->node.flags, ooflags;
+    int savnoerrexit;
     char *fname = dupstring(name);
     int obreaks, ocontflag, oloops, saveemulation, restore_sticky;
     Eprog prog;
@@ -5484,6 +5491,11 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int noreturnval)
 	    trap_return--;
 	oldlastval = lastval;
 	oldnumpipestats = numpipestats;
+	savnoerrexit = noerrexit;
+	/*
+	 * Suppression of ERR_RETURN is turned off in function scope.
+	 */
+	noerrexit &= ~NOERREXIT_RETURN;
 	if (noreturnval) {
 	    /*
 	     * Easiest to use the heap here since we're bracketed
@@ -5702,6 +5714,7 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int noreturnval)
 	if (trap_state == TRAP_STATE_PRIMED)
 	    trap_return++;
 	ret = lastval;
+	noerrexit = savnoerrexit;
 	if (noreturnval) {
 	    lastval = oldlastval;
 	    numpipestats = oldnumpipestats;
diff --git a/Src/init.c b/Src/init.c
index d8c26aca2..87dd2e26d 100644
--- a/Src/init.c
+++ b/Src/init.c
@@ -1070,7 +1070,7 @@ setupvals(char *cmd, char *runscript, char *zsh_name)
     sfcontext = SFC_NONE;
     trap_return = 0;
     trap_state = TRAP_STATE_INACTIVE;
-    noerrexit = -1;
+    noerrexit = NOERREXIT_EXIT | NOERREXIT_RETURN | NOERREXIT_SIGNAL;
     nohistsave = 1;
     dirstack = znewlinklist();
     bufstack = znewlinklist();
@@ -1199,7 +1199,7 @@ init_signals(void)
 void
 run_init_scripts(void)
 {
-    noerrexit = -1;
+    noerrexit = NOERREXIT_EXIT | NOERREXIT_RETURN | NOERREXIT_SIGNAL;
 
     if (EMULATION(EMULATE_KSH|EMULATE_SH)) {
 	if (islogin)
diff --git a/Src/loop.c b/Src/loop.c
index f7eae307b..4859c976b 100644
--- a/Src/loop.c
+++ b/Src/loop.c
@@ -542,7 +542,7 @@ execif(Estate state, int do_exec)
     end = state->pc + WC_IF_SKIP(code);
 
     if (!noerrexit)
-	noerrexit = 1;
+	noerrexit = NOERREXIT_EXIT | NOERREXIT_RETURN;
     while (state->pc < end) {
 	code = *state->pc++;
 	if (wc_code(code) != WC_IF ||
@@ -567,7 +567,12 @@ execif(Estate state, int do_exec)
 
     if (run) {
 	/* we need to ignore lastval until we reach execcmd() */
-	noerrexit = olderrexit ? olderrexit : lastval ? 2 : 0;
+	if (olderrexit)
+	    noerrexit = olderrexit;
+	else if (lastval)
+	    noerrexit = NOERREXIT_EXIT | NOERREXIT_RETURN | NOERREXIT_UNTIL_EXEC;
+	else
+	    noerrexit = 0;
 	cmdpush(run == 2 ? CS_ELSE : (s ? CS_ELIFTHEN : CS_IFTHEN));
 	execlist(state, 1, do_exec);
 	cmdpop();
diff --git a/Src/signals.c b/Src/signals.c
index cad40f4eb..94f379e72 100644
--- a/Src/signals.c
+++ b/Src/signals.c
@@ -652,7 +652,7 @@ zhandler(int sig)
     case SIGINT:
         if (!handletrap(SIGINT)) {
 	    if ((isset(PRIVILEGED) || isset(RESTRICTED)) &&
-		isset(INTERACTIVE) && noerrexit < 0)
+		isset(INTERACTIVE) && (noerrexit & NOERREXIT_SIGNAL))
 		zexit(SIGINT, 1);
             if (list_pipe || chline || simple_pline) {
                 breaks = loops;
diff --git a/Src/zsh.h b/Src/zsh.h
index 91e8d7f8c..abe9a9c82 100644
--- a/Src/zsh.h
+++ b/Src/zsh.h
@@ -2122,6 +2122,17 @@ enum source_return {
     SOURCE_ERROR = 2
 };
 
+enum noerrexit_bits {
+    /* Suppress ERR_EXIT and traps: global */
+    NOERREXIT_EXIT = 1,
+    /* Suppress ERR_RETURN: per function call */
+    NOERREXIT_RETURN = 2,
+    /* NOERREXIT only needed on way down */
+    NOERREXIT_UNTIL_EXEC = 4,
+    /* Force exit on SIGINT */
+    NOERREXIT_SIGNAL = 8
+};
+
 /***********************************/
 /* Definitions for history control */
 /***********************************/
diff --git a/Test/E01options.ztst b/Test/E01options.ztst
index f01d83567..8101ff539 100644
--- a/Test/E01options.ztst
+++ b/Test/E01options.ztst
@@ -345,6 +345,36 @@
 >ZERR trapped
 >off after
 
+  (
+    setopt ERR_EXIT
+    () { false; print is executed; } && true
+    () { false; print is not executed; }
+    print is not executed
+  )
+1:ERR_EXIT is suppressed within a function followed by "&&"
+>is executed
+
+  (
+    setopt ERR_RETURN
+    () { false; print is not executed; } || print is executed
+    print is also executed
+  )
+0:ERR_RETURN is not suppressed within a function followed by "||"
+>is executed
+>is also executed
+
+  (
+    setopt ERR_RETURN
+    () {
+      () { false; print Not executed 1; } || true
+      print Executed
+      () { false; print Not executed 2; }
+      print Not executed 3;
+    } && false
+  )
+1:ERR_RETURN with additional levels
+>Executed
+
   (print before; setopt noexec; print after)
 0:NO_EXEC option
 >before