PATCH: traps, yet again

PATCH: traps, yet again

[Peter Stephenson]

I rethought the problem with exit status from ksh-style traps which was
causing the problem with completers.

The usual case is the one that's not working --- an EXIT trap
should preserve the status.

The unusual case is the one I fixed before --- a `return' from a trap
explicitly forces the enclosing function to return.  So this is the
one that should get the special handling.  I've extended the existing
`trapreturn' mechanism which is used to make TRAPNAL functions force
the enclosing function to behave as if the signal wasn't handled.
The actual use is a bit different, but the basic idea is similar:
if we are in a trap, remember the return status and use it when
we clear up after the trap has finished.

I've put back the original execsave/execrestore to avoid another
headache.

Have a look at the new tests (which pass) and see if you think the
shell is now doing the right thing.

Index: Src/signals.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/signals.c,v
retrieving revision 1.23
diff -u -r1.23 signals.c
--- Src/signals.c	13 Nov 2003 14:34:38 -0000	1.23
+++ Src/signals.c	9 Mar 2004 12:56:50 -0000
@@ -931,6 +931,7 @@
     char *name, num[4];
     int trapret = 0;
     int obreaks = breaks;
+    int isfunc;
  
     /* if signal is being ignored or the trap function		      *
      * is NULL, then return					      *
@@ -948,16 +949,7 @@
     *sigtr |= ZSIG_IGNORED;
 
     lexsave();
-    if (sig != SIGEXIT && sig != SIGDEBUG) {
-	/*
-	 * SIGEXIT and SIGDEBUG are always run synchronously, so we don't
-	 * need to save and restore the state.
-	 *
-	 * Do we actually need this at all now we queue signals
-	 * for handling in places where they won't cause trouble?
-	 */
-	execsave();
-    }
+    execsave();
     breaks = 0;
     runhookdef(BEFORETRAPHOOK, NULL);
     if (*sigtr & ZSIG_FUNC) {
@@ -970,27 +962,52 @@
 	sprintf(num, "%d", sig);
 	zaddlinknode(args, num);
 
-	trapreturn = -1;
+	trapreturn = -1;	/* incremented by doshfunc */
 	sfcontext = SFC_SIGNAL;
 	doshfunc(name, sigfn, args, 0, 1);
 	sfcontext = osc;
 	freelinklist(args, (FreeFunc) NULL);
 	zsfree(name);
-    } else
+
+	isfunc = 1;
+    } else {
+	trapreturn = -2;	/* not incremented, used at current level */
+
 	execode(sigfn, 1, 0);
+
+	isfunc = 0;
+    }
     runhookdef(AFTERTRAPHOOK, NULL);
 
-    if (trapreturn > 0)
+    if (trapreturn > 0 && isfunc) {
+	/*
+	 * Context was its own function.  We propagate the return
+	 * value specially.  Return value zero means don't do
+	 * anything special, so don't handle it.
+	 */
 	trapret = trapreturn;
-    else if (errflag)
+    } else if (trapreturn >= 0 && !isfunc) {
+	/*
+	 * Context was an inline trap.  If an explicit return value
+	 * was used, we need to set `lastval'.  Otherwise we use the
+	 * value restored by execrestore.  In this case, all return
+	 * values indicate an explicit return from the current function,
+	 * so always handle specially.  trapreturn is always restored by
+	 * execrestore.
+	 */
+	trapret = trapreturn + 1;
+    } else if (errflag)
 	trapret = 1;
-    if (sig != SIGEXIT && sig != SIGDEBUG)
-	execrestore();
+    execrestore();
     lexrestore();
 
     if (trapret > 0) {
-	breaks = loops;
-	errflag = 1;
+	if (isfunc) {
+	    breaks = loops;
+	    errflag = 1;
+	} else {
+	    lastval = trapret-1;
+	}
     } else {
 	breaks += obreaks;
 	if (breaks > loops)
Index: Test/C03traps.ztst
===================================================================
RCS file: /cvsroot/zsh/zsh/Test/C03traps.ztst,v
retrieving revision 1.2
diff -u -r1.2 C03traps.ztst
--- Test/C03traps.ztst	1 Oct 2001 12:38:33 -0000	1.2
+++ Test/C03traps.ztst	9 Mar 2004 12:56:50 -0000
@@ -196,3 +196,19 @@
   f
   functions TRAPWINCH
 1:Unsetting ordinary traps with localtraps.
+
+#
+# Returns from within traps are a perennial problem.
+# The following two apply to returns in and around standard
+# ksh-style traps.  The intention is that a return value from
+# within the function is preserved (i.e. statuses set by the trap
+# are ignored) unless the trap explicitly executes `return', which makes
+# it return from the enclosing function.
+#
+  fn() { trap 'true' EXIT; return 1; }
+  fn
+1: ksh-style EXIT traps preserve return value
+
+  inner() { trap 'return 3' EXIT; return 2: }
+  outer() { inner; return 1; }
+3: ksh-style EXIT traps can force return status of enclosing function

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR Ltd., Science Park, Milton Road,
Cambridge, CB4 0WH, UK                          Tel: +44 (0)1223 692070


**********************************************************************
This email and any files transmitted with it are confidential and
intended solely for the use of the individual or entity to whom they
are addressed. If you have received this email in error please notify
the system manager.

This footnote also confirms that this email message has been swept by
MIMEsweeper for the presence of computer viruses.

www.mimesweeper.com
**********************************************************************

Re: PATCH: traps, yet again

[Bart Schaefer]

On Mar 9,  1:11pm, Peter Stephenson wrote:
}
} `trapreturn' mechanism which is used to make TRAPNAL functions force
} the enclosing function to behave as if the signal wasn't handled.
} The actual use is a bit different, but the basic idea is similar:
} if we are in a trap, remember the return status and use it when
} we clear up after the trap has finished.
} 
} I've put back the original execsave/execrestore to avoid another
} headache.

I had been staring at this late last night and was about to suggest
something very close to what you did, but decided to sleep on it.  It
looks like you thought of more contingencies than I had, so it's just
as well.

I think this looks right.