PATCH: test for trap EXIT fix.

PATCH: test for trap EXIT fix.

[Peter Stephenson]

Here's a test that nested EXIT traps work when exiting from within a
function.

The comment notes the following behaviour:

fn() { 
  ( trap 'print This is in the top-level function scope.' EXIT
    exit
  )
}

The subshell exits without ever leaving the scope of fn(), so the EXIT trap
isn't called in this case.

I'm not convinced this is wrong, or at least that it's inconsistent.
Exiting subshells is a bit of a special case.

Index: Test/C03traps.ztst
===================================================================
RCS file: /cvsroot/zsh/zsh/Test/C03traps.ztst,v
retrieving revision 1.1
diff -u -r1.1 C03traps.ztst
--- Test/C03traps.ztst	2001/04/02 12:32:43	1.1
+++ Test/C03traps.ztst	2001/10/01 11:56:41
@@ -45,6 +45,18 @@
 0:Nested `trap - EXIT' on `TRAPEXIT'
 >EXIT1
 
+# We can't test an EXIT trap for the shell as a whole, because
+# we're inside a function scope which we don't leave when the
+# subshell exits.  Not sure if that's the correct behaviour, but
+# it's sort of consistent.
+  ( fn1() { trap 'print Function 1 going' EXIT; exit; print Not reached; }
+    fn2() { trap 'print Function 2 going' EXIT; fn1; print Not reached; }
+    fn2
+  )
+0:EXIT traps on functions when exiting from function
+>Function 1 going
+>Function 2 going
+
   fn1() {
     trap
     trap 'print INT1' INT

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


**********************************************************************
The information transmitted is intended only for the person or
entity to which it is addressed and may contain confidential 
and/or privileged material. 
Any review, retransmission, dissemination or other use of, or
taking of any action in reliance upon, this information by 
persons or entities other than the intended recipient is 
prohibited.  
If you received this in error, please contact the sender and 
delete the material from any computer.
**********************************************************************

Re: PATCH: test for trap EXIT fix.

[Bart Schaefer]

On Oct 1,  1:01pm, Peter Stephenson wrote:
}
} The comment notes the following behaviour:
} 
} fn() { 
}   ( trap 'print This is in the top-level function scope.' EXIT
}     exit
}   )
} }

Wait ... why is that trap at the top-level function scope?  It's inside a
subshell.  Shouldn't the subshell be its own scope?

Anyway, that behavior is not consistent with previous versions of zsh,
even 4.0.x, so we should think about it pretty carefully.

} The subshell exits without ever leaving the scope of fn(), so the EXIT trap
} isn't called in this case.

Hrm.  Try these variants (with `fn' as above):

zsh% trap 'print PS1-level' EXIT; fn 			#1
zsh% (trap 'print PS1-level' EXIT; fn)			#2
zsh% trap 'print PS1-level' EXIT; (exit)		#3
zsh% function TRAPEXIT { print PS1-level }; (exit)	#4
PS1-level
zsh% function TRAPEXIT { print PS1-level }; fn		#5
zsh% 

What's the excuse for the PS1-level trap not running any case but #4?

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

Zsh: http://www.zsh.org | PHPerl Project: http://phperl.sourceforge.net   

Re: PATCH: test for trap EXIT fix.

[Peter Stephenson]

Bart Schaefer wrote:
> On Oct 1,  1:01pm, Peter Stephenson wrote:
> }
> } The comment notes the following behaviour:
> } 
> } fn() { 
> }   ( trap 'print This is in the top-level function scope.' EXIT
> }     exit
> }   )
> } }
> 
> Wait ... why is that trap at the top-level function scope?  It's inside a
> subshell.  Shouldn't the subshell be its own scope?

That's never happened before.  We can introduce scopes for subshells, but
it's probably quite a lot of work.

> zsh% trap 'print PS1-level' EXIT; fn 			#1
> zsh% (trap 'print PS1-level' EXIT; fn)			#2
> zsh% trap 'print PS1-level' EXIT; (exit)		#3
> zsh% function TRAPEXIT { print PS1-level }; (exit)	#4
> PS1-level
> zsh% function TRAPEXIT { print PS1-level }; fn		#5
> zsh% 
> 
> What's the excuse for the PS1-level trap not running any case but #4?

The odd difference between #3 and #4 seems to be deliberate, weirdly
enough.  If you look at the top of entersubsh(), you will see traps are
usually cleared (cl == 2 implies this is not a `real' subshell, for some
definition of `not real' I haven't entirely worked out but which doesn't
apply in this case), but are left when the trap is a function:

    if (cl != 2)
	for (sig = 0; sig < VSIGCOUNT; sig++)
	    if (!(sigtrapped[sig] & ZSIG_FUNC))
		unsettrap(sig);

I can't imagine this is deliberately deliberate?  It must have been
deliberate in some accidental fashion.

I think the behaviour of clearing existing traps on entering subshells is
correct.  But we probably need to handle the case of traps inside the
subshell, when the subshell is in an arbitrary environment, better.
Maybe there's a simple tweak to the code I introduced so this can be done
without a full extra layer of scoping.

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


**********************************************************************
The information transmitted is intended only for the person or
entity to which it is addressed and may contain confidential 
and/or privileged material. 
Any review, retransmission, dissemination or other use of, or
taking of any action in reliance upon, this information by 
persons or entities other than the intended recipient is 
prohibited.  
If you received this in error, please contact the sender and 
delete the material from any computer.
**********************************************************************

Re: PATCH: test for trap EXIT fix.

[Bart Schaefer]

On Oct 2, 11:23am, Peter Stephenson wrote:
}
} Bart Schaefer wrote:
} > On Oct 1,  1:01pm, Peter Stephenson wrote:
} > }
} > } The comment notes the following behaviour:
} > } 
} > } fn() { 
} > }   ( trap 'print This is in the top-level function scope.' EXIT
} > }     exit
} > }   )
} > } }
} > 
} > Wait ... why is that trap at the top-level function scope?  It's inside a
} > subshell.  Shouldn't the subshell be its own scope?
} 
} That's never happened before.  We can introduce scopes for subshells, but
} it's probably quite a lot of work.

I don't really mean a true "scope"; but I'd be less surprised to find
that

	fn() {
	  trap 'print This is in the top-level function scope.' EXIT
	  ( exit )
	}

did not run the trap.  In the first example, what's the point of setting
the trap inside the subshell if you don't mean to trap the exit of the
subshell?

} > zsh% trap 'print PS1-level' EXIT; (exit)		#3
} > zsh% function TRAPEXIT { print PS1-level }; (exit)	#4
} > PS1-level
} 
} The odd difference between #3 and #4 seems to be deliberate, weirdly
} enough.  If you look at the top of entersubsh(), you will see traps are
} usually cleared (cl == 2 implies this is not a `real' subshell, for some
} definition of `not real' I haven't entirely worked out but which doesn't
} apply in this case), but are left when the trap is a function:
} 
}     if (cl != 2)
} 	for (sig = 0; sig < VSIGCOUNT; sig++)
} 	    if (!(sigtrapped[sig] & ZSIG_FUNC))
} 		unsettrap(sig);
} 
} I can't imagine this is deliberately deliberate?  It must have been
} deliberate in some accidental fashion.

Hmm, I'll bet it WAS deliberately deliberate.  Look how bash behaves:

bash2-2.03$ fn() { trap 'echo exiting' 0; (exit); }
bash2-2.03$ fn
bash2-2.03$ fn() { (trap 'echo exiting' 0; exit); }
bash2-2.03$ fn
exiting
bash2-2.03$ 

I think somebody (possibly even PF) decided that exit traps should have
worked all along the way the TRAPEXIT function works (case #4) but made
`trap' work the way the Bourne shell works for compatibility.

At the very least, I imagine that somebody wanted

	TRAPEXIT() { echo 'I am a function' }
	(functions)

to print the definition of the TRAPEXIT function.  QED.

This should probably be documented somewhere.

} Maybe there's a simple tweak to the code I introduced so this can be done
} without a full extra layer of scoping.

Can you explain to me why the subshell exits without ever leaving the
function scope?  Why doesn't it just exit from a forked copy of the
function scope?

Of course in that case we have to clear traps in all scopes on entry to
the subshell, not just traps in the current scope, in order to preserve
the Bourne shell behavior.  Or something like that.

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

Zsh: http://www.zsh.org | PHPerl Project: http://phperl.sourceforge.net   

Re: PATCH: test for trap EXIT fix.

[Peter Stephenson]

Bart Schaefer wrote:
> Can you explain to me why the subshell exits without ever leaving the
> function scope?  Why doesn't it just exit from a forked copy of the
> function scope?

There's special handling in the exec function hierarchy, starting with the
`exiting' flag to execlist.  The key chunk of code is this in execcmd(),
around line 2365 of exec.c

		/* If we're forked (and we should be), no need to return */
		DPUTS(last1 != 1 && !forked, "BUG: not exiting?");
		execlist(state, 0, 1);
                                   ^ `exiting' flag

That means we execute the list of command, and when we get to the end, exit
the shell.  I think this is correct --- I don't think we have any business
leaving the function scope for a subshell contained entirely inside a
function.

At least, that's certainly true if you compare what happens when you drop
off the end of a subshell list with what happens when you drop off the end
of a current shell list.  If you compare calling `exit' in the two, the
behaviour is different, but that's not the typical case for a subshell list.

We may be able to do some special tricks with `exiting' at the end of
execlist(), making sure we don't exit too early from lower down in that
case (because of the optimization for a final command to be exec'd if
there's nothing left to do).  Then we just execute any top level EXIT trap
which has been left to us.

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


**********************************************************************
The information transmitted is intended only for the person or
entity to which it is addressed and may contain confidential 
and/or privileged material. 
Any review, retransmission, dissemination or other use of, or
taking of any action in reliance upon, this information by 
persons or entities other than the intended recipient is 
prohibited.  
If you received this in error, please contact the sender and 
delete the material from any computer.
**********************************************************************

Re: PATCH: test for trap EXIT fix.

[Bart Schaefer]

On Oct 2,  3:39pm, Peter Stephenson wrote:
}
} We may be able to do some special tricks with `exiting' at the end of
} execlist(), making sure we don't exit too early from lower down in that
} case (because of the optimization for a final command to be exec'd if
} there's nothing left to do).  Then we just execute any top level EXIT trap
} which has been left to us.

I'm having a hard time grokking "top level EXIT trap" in that context,
but I think you must be thinking the right thing, because I can't find
any other interpretation that makes sense.

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

Zsh: http://www.zsh.org | PHPerl Project: http://phperl.sourceforge.net   

Re: PATCH: test for trap EXIT fix.

[Peter Stephenson]

Bart Schaefer wrote:
> On Oct 2,  3:39pm, Peter Stephenson wrote:
> } We may be able to do some special tricks with `exiting' at the end of
> } execlist(), making sure we don't exit too early from lower down in that
> } case (because of the optimization for a final command to be exec'd if
> } there's nothing left to do).  Then we just execute any top level EXIT trap
> } which has been left to us.
> 
> I'm having a hard time grokking "top level EXIT trap" in that context,
> but I think you must be thinking the right thing, because I can't find
> any other interpretation that makes sense.

`Top level' means whatever's on the front of the list of EXIT traps.  It's
about the only thing we've got we could possibly execute as a trap.  If I'm
following my own logic properly, this means

fn() {
  function TRAPEXIT { echo foo; }
  ( true )
  ( exit )
}

would execute the trap when the subshells exit (as well as when the
function exits), because the function-type trap is not deleted at the start
of the subshell, and is what we have waiting in the queue on the next exit
from anything.  This would match the behaviour the code were executed
outside the function.

The 4.0 behaviour is that only the explicit exit produces a trap; falling
off the end of the subshell in `( true )' doesn't.  I would guess this is
wrong and they should be equivalent.  That may require more special
handling.

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


**********************************************************************
The information transmitted is intended only for the person or
entity to which it is addressed and may contain confidential 
and/or privileged material. 
Any review, retransmission, dissemination or other use of, or
taking of any action in reliance upon, this information by 
persons or entities other than the intended recipient is 
prohibited.  
If you received this in error, please contact the sender and 
delete the material from any computer.
**********************************************************************

Re: PATCH: test for trap EXIT fix.

[Bart Schaefer]

On Oct 2,  4:32pm, Peter Stephenson wrote:
}
} Bart Schaefer wrote:
} > I'm having a hard time grokking "top level EXIT trap" in that context,
} 
} `Top level' means whatever's on the front of the list of EXIT traps.

Ah.  I'd have said

	trap 'print top-level' EXIT
	fn() {
	  trap 'print fn-level' EXIT
	  ( trap 'print subshell' EXIT )
	}

and in that sense the subshell can't possibly execute the top-level trap.
At the same time, but in another sense, `fn-level' and `subshell' are at
the same "level" there.

} The 4.0 behaviour is that only the explicit exit produces a trap; falling
} off the end of the subshell in `( true )' doesn't.  I would guess this is
} wrong and they should be equivalent.  That may require more special
} handling.

Hrm.

zagzig% echo $ZSH_VERSION
3.0.8
zagzig% TRAPEXIT() { print exiting }
zagzig% (true)
zagzig% (exit)
exiting
zagzig% fn() { ( true ) ; ( exit ); }
zagzig% fn
zagzig% exit
exiting


-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

Zsh: http://www.zsh.org | PHPerl Project: http://phperl.sourceforge.net   

PATCH: (2) test for trap EXIT fix.

[Peter Stephenson]

Bart Schaefer wrote:
> } The 4.0 behaviour is that only the explicit exit produces a trap; falling
> } off the end of the subshell in `( true )' doesn't.  I would guess this is
> } wrong and they should be equivalent.  That may require more special
> } handling.
> 
> Hrm.
> 
> zagzig% echo $ZSH_VERSION
> 3.0.8
> zagzig% TRAPEXIT() { print exiting }
> zagzig% (true)
> zagzig% (exit)
> exiting
> zagzig% fn() { ( true ) ; ( exit ); }
> zagzig% fn
> zagzig% exit
> exiting

Are you saying this is correct and the inconsistency between falling off
the end and exiting is the intended behaviour?  The following might do we
what you want.  Maybe you'd better try it and then complain further.

Index: Src/builtin.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/builtin.c,v
retrieving revision 1.54
diff -u -r1.54 builtin.c
--- Src/builtin.c	2001/09/24 10:12:51	1.54
+++ Src/builtin.c	2001/10/03 15:13:54
@@ -3255,12 +3255,16 @@
 	}
 	/*FALLTHROUGH*/
     case BIN_EXIT:
-	if (locallevel) {
+	if (locallevel > forklevel) {
 	    /*
 	     * We don't exit directly from functions to allow tidying
 	     * up, in particular EXIT traps.  We still need to perform
 	     * the usual interactive tests to see if we can exit at
 	     * all, however.
+	     *
+	     * The forklevel test means we *do* exit from a subshell
+	     * inside a function when we reach the level of the
+	     * function itself.
 	     */
 	    if (stopmsg || (zexit(0,2), !stopmsg)) {
 		retflag = 1;
Index: Src/exec.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/exec.c,v
retrieving revision 1.37
diff -u -r1.37 exec.c
--- Src/exec.c	2001/09/24 10:12:51	1.37
+++ Src/exec.c	2001/10/03 15:13:54
@@ -2490,6 +2490,10 @@
 }
 
 /**/
+int
+forklevel;
+
+/**/
 static void
 entersubsh(int how, int cl, int fake)
 {
@@ -2557,6 +2561,7 @@
     if (cl)
 	clearjobtab();
     times(&shtms);
+    forklevel = locallevel;
 }
 
 /* close internal shell fds */


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


**********************************************************************
The information transmitted is intended only for the person or
entity to which it is addressed and may contain confidential 
and/or privileged material. 
Any review, retransmission, dissemination or other use of, or
taking of any action in reliance upon, this information by 
persons or entities other than the intended recipient is 
prohibited.  
If you received this in error, please contact the sender and 
delete the material from any computer.
**********************************************************************

Re: PATCH: (2) test for trap EXIT fix.

[Bart Schaefer]

On Oct 3,  4:18pm, Peter Stephenson wrote:
}
} Are you saying this is correct and the inconsistency between falling off
} the end and exiting is the intended behaviour?

Not exactly; I'm just providing a historical reference point so we're sure
we've made the right decision.

} The following might do we what you want.  Maybe you'd better try it and
} then complain further.

OK, I'm good at that.

15931 has almost got it, but:

bash2-2.03$ (trap 'echo exiting' 0; true)
exiting
bash2-2.03$ 

zagzig% (trap 'echo exiting' 0; true)
zagzig% 

It matters where the trap was set, as well.

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

Zsh: http://www.zsh.org | PHPerl Project: http://phperl.sourceforge.net   

Re: PATCH: (2) test for trap EXIT fix.

[Peter Stephenson]

Peter Stephenson wrote:
> Bart Schaefer wrote:
> > zagzig% echo $ZSH_VERSION
> > 3.0.8
> > zagzig% TRAPEXIT() { print exiting }
> > zagzig% (true)
> > zagzig% (exit)
> > exiting
> > zagzig% fn() { ( true ) ; ( exit ); }
> > zagzig% fn
> > zagzig% exit
> > exiting
> 
> Are you saying this is correct and the inconsistency between falling off
> the end and exiting is the intended behaviour?  The following might do we
> what you want.

I'n committing this, partly because it seems to make the shell reasonably
compatible with older versions, but mainly because it saves me having to
edit it out when committing the zglob patch.

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


**********************************************************************
The information transmitted is intended only for the person or
entity to which it is addressed and may contain confidential 
and/or privileged material. 
Any review, retransmission, dissemination or other use of, or
taking of any action in reliance upon, this information by 
persons or entities other than the intended recipient is 
prohibited.  
If you received this in error, please contact the sender and 
delete the material from any computer.
**********************************************************************

Re: PATCH: (2) test for trap EXIT fix.

[Bart Schaefer]

On Oct 8,  9:10am, Peter Stephenson wrote:
}
} I'm committing this, partly because it seems to make the shell reasonably
} compatible with older versions

Did you also see my message about how this still doesn't quite act like
bash (and presumably therefore not quite like the standard shell)?

} but mainly because it saves me having to
} edit it out when committing the zglob patch.

Does that mean you're putting the exit-trap patch onto the stable branch?

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

Zsh: http://www.zsh.org | PHPerl Project: http://phperl.sourceforge.net   

Re: PATCH: (2) test for trap EXIT fix.

[Peter Stephenson]

Bart Schaefer wrote:
> On Oct 8,  9:10am, Peter Stephenson wrote:
> }
> } I'm committing this, partly because it seems to make the shell reasonably
> } compatible with older versions
> 
> Did you also see my message about how this still doesn't quite act like
> bash (and presumably therefore not quite like the standard shell)?

Yes, but that's the same issue as whether falling off the end of the list
should trigger the trap.  The position of the `trap' command matters
because of the stuff in entersubsh() we discussed earlier.  I have a
trivial patch to fix this by calling the trap at the end of execcmd() if
the shell is forked and we are exiting (which is implied at that point if
the shell is forked).  This changes the behaviour, however, so I'm not sure
what to do.

> } but mainly because it saves me having to
> } edit it out when committing the zglob patch.
> 
> Does that mean you're putting the exit-trap patch onto the stable branch?

No, I meant to say that.  It's not yet stable enough.  But we should
document the bug in the 4.0 BUGS list.

I've committed the ChangeLog for the previous patches.

Index: Etc/BUGS
===================================================================
RCS file: /cvsroot/zsh/zsh/Etc/BUGS,v
retrieving revision 1.2
diff -u -r1.2 BUGS
--- Etc/BUGS	2000/04/07 15:01:03	1.2
+++ Etc/BUGS	2001/10/08 10:31:42
@@ -2,6 +2,10 @@
 KNOWN BUGS IN ZSH
 -----------------
 
+When a shell is exited from within a function, the only EXIT trap which is
+run is the one for the function itself, if any.  In particular, an EXIT
+trap set for the shell itself will not be executed in this case.
+------------------------------------------------------------------------
 On some terminals, display of lines with exactly 80 characters is
 problematic.  zsh assumes that the terminal does not print an extra
 newline in this case, but some terminals (e.g. aixterm) do.

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


**********************************************************************
The information transmitted is intended only for the person or
entity to which it is addressed and may contain confidential 
and/or privileged material. 
Any review, retransmission, dissemination or other use of, or
taking of any action in reliance upon, this information by 
persons or entities other than the intended recipient is 
prohibited.  
If you received this in error, please contact the sender and 
delete the material from any computer.
**********************************************************************