"set -e" handling is broken with zsh 5.3.1 and 5.4.1

"set -e" handling is broken with zsh 5.3.1 and 5.4.1

[Vincent Lefevre]

Consider:

----------------------------------------
#!/usr/bin/env zsh

set -e

f()
{
  [[ -z 1 ]] && false
}

if false; then
  :
else
  f
  echo Fail 1
  echo Fail 2
  f
  echo Fail 3
fi
----------------------------------------

With
  zsh 5.3.1-4+b1 under Debian/stretch
  zsh 5.4.1-1 under Debian/unstable

I get:

% ./cond2-e; echo $?
Fail 1
Fail 2
1

I suppose that cond2-e should die just after f is called, before
outputting anything. At least, the behavior is not consistent.

zsh 5.0.7 was outputting:

Fail 1
Fail 2
Fail 3

but I suppose that this was incorrect (different from what other
shells give on similar POSIX code).

-- 
Vincent Lefèvre <vincent@vinc17.net> - Web: <https://www.vinc17.net/>
100% accessible validated (X)HTML - Blog: <https://www.vinc17.net/blog/>
Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)

Re: "set -e" handling is broken with zsh 5.3.1 and 5.4.1

[Daniel Shahaf]

Vincent Lefevre wrote on Sun, 27 Aug 2017 02:50 +0200:
> ----------------------------------------
> #!/usr/bin/env zsh
> 
> set -e
> 
> f()
> {
>   [[ -z 1 ]] && false
> }
> 
> if false; then
>   :
> else
>   f
>   echo Fail 1
>   echo Fail 2
>   f
>   echo Fail 3
> fi
> ----------------------------------------
> 
> I suppose that cond2-e should die just after f is called, before
> outputting anything. At least, the behavior is not consistent.

That's what happens if [[ is changed to [, or put into a subshell.

Re: "set -e" handling is broken with zsh 5.3.1 and 5.4.1

[Vincent Lefevre]

On 2017-08-27 01:30:34 +0000, Daniel Shahaf wrote:
> Vincent Lefevre wrote on Sun, 27 Aug 2017 02:50 +0200:
> > ----------------------------------------
> > #!/usr/bin/env zsh
> > 
> > set -e
> > 
> > f()
> > {
> >   [[ -z 1 ]] && false
> > }
> > 
> > if false; then
> >   :
> > else
> >   f
> >   echo Fail 1
> >   echo Fail 2
> >   f
> >   echo Fail 3
> > fi
> > ----------------------------------------
> > 
> > I suppose that cond2-e should die just after f is called, before
> > outputting anything. At least, the behavior is not consistent.
> 
> That's what happens if [[ is changed to [, or put into a subshell.

I also get the expected behavior if I put "true" before the line

  [[ -z 1 ]] && false

or if I remove the

if false; then
  :
else

and the "fi".

-- 
Vincent Lefèvre <vincent@vinc17.net> - Web: <https://www.vinc17.net/>
100% accessible validated (X)HTML - Blog: <https://www.vinc17.net/blog/>
Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)

Re: "set -e" handling is broken with zsh 5.3.1 and 5.4.1

[Peter Stephenson]

On Sun, 27 Aug 2017 02:50:40 +0200
Vincent Lefevre <vincent@vinc17.net> wrote:
> Consider:
> 
> ----------------------------------------
> #!/usr/bin/env zsh
> 
> set -e
> 
> f()
> {
>   [[ -z 1 ]] && false
> }
> 
> if false; then
>   :
> else
>   f
>   echo Fail 1
>   echo Fail 2
>   f
>   echo Fail 3
> fi
> ----------------------------------------
> 
> With
>   zsh 5.3.1-4+b1 under Debian/stretch
>   zsh 5.4.1-1 under Debian/unstable
> 
> I get:
> 
> % ./cond2-e; echo $?
> Fail 1
> Fail 2
> 1
> 
> I suppose that cond2-e should die just after f is called, before
> outputting anything.

Yes, I would say so.

This appears to have been deliberate, in that after "if" we usually
restore noerrexit beahaviour on the first thing we execute, but if it's
a function we don't.  However, I can't for the life of me work out why I
made that exeception --- certainly nothing goes wrong in the tests if I
apply the following, and I would expect to be testing whatever it was
made me think we needed the qualification.  It might have been to do
with empty functions, but making f empty, so it runs but doesn't change
the status, doesn't seem to do anything unexpected (we shouldn't
and don't exit).

I think I'll apply the effect of this after the release of 5.4.2, with a
test based on the code above plus some empty functions just in case, and
see what happens.

In the mean time somebody may find a more fiendish variant involving
functions that causes a further problem.

pws

diff --git a/Src/exec.c b/Src/exec.c
index cd99733..82277a3 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -3025,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 & NOERREXIT_UNTIL_EXEC) && !is_shfunc)
+    if ((noerrexit & NOERREXIT_UNTIL_EXEC)/* && !is_shfunc*/)
 	noerrexit = 0;
 
     /* Do prefork substitutions.

Re: "set -e" handling is broken with zsh 5.3.1 and 5.4.1

[Vincent Lefevre]

On 2017-08-27 19:56:48 +0100, Peter Stephenson wrote:
> This appears to have been deliberate, in that after "if" we usually
> restore noerrexit beahaviour on the first thing we execute, but if it's
> a function we don't.  However, I can't for the life of me work out why I
> made that exeception --- certainly nothing goes wrong in the tests if I
> apply the following, and I would expect to be testing whatever it was
> made me think we needed the qualification.  It might have been to do
> with empty functions, but making f empty, so it runs but doesn't change
> the status, doesn't seem to do anything unexpected (we shouldn't
> and don't exit).

Perhaps it was needed in the past, and no longer makes sense now.
Have you looked at the diff that introduced it, the corresponding
commit log and the discussions that occurred at that time?

BTW, if it was added for some purpose, a test should have been added
at the same time to make it fail if this condition was not there.

-- 
Vincent Lefèvre <vincent@vinc17.net> - Web: <https://www.vinc17.net/>
100% accessible validated (X)HTML - Blog: <https://www.vinc17.net/blog/>
Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)

Re: "set -e" handling is broken with zsh 5.3.1 and 5.4.1

[Daniel Shahaf]

Vincent Lefevre wrote on Mon, 28 Aug 2017 00:26 +0200:
> Perhaps it was needed in the past, and no longer makes sense now.
> Have you looked at the diff that introduced it,

It's this one:

commit d6a32ddeed914434f5b56b013c9d03b28781d065
Author: Barton E. Schaefer <schaefer@zsh.org>
Date:   Sat Dec 27 21:55:58 2014 -0800

    34065: following an "if" condition, do not test lastval for ERR_EXIT until a new command is run
    
    Includes unposted regression tests.

Re: "set -e" handling is broken with zsh 5.3.1 and 5.4.1

[Bart Schaefer]

On Sun, Aug 27, 2017 at 6:25 PM, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> Vincent Lefevre wrote on Mon, 28 Aug 2017 00:26 +0200:
>> Perhaps it was needed in the past, and no longer makes sense now.
>> Have you looked at the diff that introduced it,
>
> It's this one:
>
> commit d6a32ddeed914434f5b56b013c9d03b28781d065
> Author: Barton E. Schaefer <schaefer@zsh.org>
> Date:   Sat Dec 27 21:55:58 2014 -0800
>
>     34065: following an "if" condition, do not test lastval for ERR_EXIT until a new command is run


Background is in workers/34055 and 34059, and this looks to have
something to do with noerrexit not propagating into the first sublist
in a function body.  It's entirely possible that the change from
having a few numeric values for the flags to have a set of bit values
has made that test unnecessary, or it may be needed in case that
hasn't been tested yet -- the original report in 34055 involves
calling a function within an if/else block.

Re: "set -e" handling is broken with zsh 5.3.1 and 5.4.1

[Peter Stephenson]

On Sun, 27 Aug 2017 23:45:20 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> Background is in workers/34055 and 34059, and this looks to have
> something to do with noerrexit not propagating into the first sublist
> in a function body.  It's entirely possible that the change from
> having a few numeric values for the flags to have a set of bit values
> has made that test unnecessary, or it may be needed in case that
> hasn't been tested yet -- the original report in 34055 involves
> calling a function within an if/else block.

I committed the chnage.  The regression tests in C03ttraps.ztst are
still passing, and I couldn't see any simple code involving functions
that caused a failure.  If someone finds one we will need to add it to
the tests.

pws