Re: Unexpected err_return behavior inside if/else block

[Peter Stephenson <p.stephenson@xxxxxxxxxxx>]

On Sun, 8 Jul 2018 20:17:05 -0300
Daniel Santana da Silva <daniel@xxxxxxxxxxxx> wrote:
> The following script outputs "status: 1", where I'd expect the script
> to exit right after the called function failed with the `false`
> statement:
> 
>     #!/usr/bin/env zsh
> 
>     setopt err_return
>     function { if :; then false; fi }
>     echo status: $?

That's a fairly clear bug (so further follow-ups should go to
zsh-workers --- I'm leaving the immediate response in zsh-users
for clarity but it's all internals from here on).

This opened up a can of worms.  It turned out the tests for this
stuff, in C03traps.ztst, weren't running to the end because ERR_EXIT or
ERR_RETURN was set too high up the scope, and this wasn't causing an
error for some reason which may be the retported bug (see below).

Putting back the state revealed another failure: we weren't suppressing
EXIT traps in forked parts of pipelines any more, for which there are a
couple of tests that didn't get executed.  That's probably my fault
due to the pipeline fork rearrangements, though the fix appears to be
straightforward, so that's also fixed here.

With that in place fixing the reported bug was apparently also
straightforward --- but I've left a note in the code as this sort of
thing has a horrible habit of getting more and more complicated.

I *think* this fix has the beneficent(*) side effect that failing to run
the tests in C03traps.ztst is no longer silent, since that's the first
thing I saw after the fix that alerted me to something odd going on.
However, I haven't looked further into this aspect --- and of course it now
does actually run the tests anyway.

The changes are much simpler than the explanation.

pws

(*) I only use words like this when I think it's cromulent to scare
people.


diff --git a/Src/exec.c b/Src/exec.c
index d445278..5864020 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -2728,6 +2728,11 @@ execcmd_fork(Estate state, int how, int type, Wordcode varspc,
 
     if (sigtrapped[SIGINT] & ZSIG_IGNORED)
 	holdintr();
+    /*
+     * EXIT traps shouldn't be called even if we forked to run
+     * shell code as this isn't the main shell.
+     */
+    sigtrapped[SIGEXIT] = 0;
 #ifdef HAVE_NICE
     /* Check if we should run background jobs at a lower priority. */
     if ((how & Z_ASYNC) && isset(BGNICE))
@@ -5792,7 +5797,19 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int noreturnval)
     undoshfunc:
 	--funcdepth;
 	if (retflag) {
+	    /*
+	     * 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);
diff --git a/Test/C03traps.ztst b/Test/C03traps.ztst
index f229625..dce263f 100644
--- a/Test/C03traps.ztst
+++ b/Test/C03traps.ztst
@@ -680,6 +680,22 @@ F:Must be tested with a top-level script rather than source or function
 >Better
 >In .zshenv
 
+  unsetopt errreturn
+  fn2() {
+    if true; then
+      false
+    fi
+  }
+  fn1() {
+    setopt localoptions errreturn
+    fn2
+    print $?
+  }
+  fn1
+  print fn1 done
+0:ERR_RETURN caused by function returning false from within shell construct
+>fn1 done
+
   fn2() {
     if false; then
       print Bad
@@ -741,6 +757,7 @@ 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
 
+  unsetopt err_exit err_return
   (setopt err_exit
   for x in y; do
     false