zsh-workers
 help / color / mirror / code / Atom feed
* Re: [PATCH] More ERR_EXIT (was Re: Tests RE behavior of ERR_EXIT)
@ 2022-11-12 22:16 Philippe Altherr
  2022-11-13  3:59 ` Philippe Altherr
  0 siblings, 1 reply; 26+ messages in thread
From: Philippe Altherr @ 2022-11-12 22:16 UTC (permalink / raw)
  To: Bart Schaefer, zsh-workers

[-- Attachment #1: Type: text/plain, Size: 5634 bytes --]

Hi Bart,

I just noticed the following change:

+Changes since 5.9
> +-----------------
> +
> +Handling of ERR_EXIT is corrected
> *when the final status of a structured+command* (for, select, while,
> repeat, if, case, or a list in braces)
> *is+nonzero*.  To be compatible with other shells, *"zsh -e" now exits* in
> +those circumstances, whereas previous versions did not.  This does not
> +affect the handling of nonzero status within conditional statements.


This looks wrong to me. It's not compatible with the third exception of POSiX
"set -e"
<https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#set>
:

When this option is on, when any command fails (for any of the reasons
> listed in Consequences of Shell Errors or by returning an exit status
> greater than zero), the shell immediately shall exit, as if by executing
> the exit special built-in utility with no arguments, with the following
> exceptions:
>
>    1. The failure of any individual command in a multi-command pipeline
>    shall not cause the shell to exit. Only the failure of the pipeline itself
>    shall be considered.
>    2. The -e setting shall be ignored when executing the compound list
>    following the while, until, if, or elif reserved word, a pipeline beginning
>    with the ! reserved word, or any command of an AND-OR list other than the
>    last.
>    3. *If the exit status of a compound command other than a subshell
>    command was the result of a failure while -e was being ignored, then -e
>    shall not apply to this command.*
>
> My high-level understanding of exception 3 is that if an error was
produced by a condition (note that the last command in a sub-list is NOT a
condition but a regular command), then that error should bubble up the
evaluation stack, without triggering any ERR_EXIT, until it gets ignored
(for example on the left of a ";" in a sequence) or it becomes the result
of an enclosing function or subshell.

Let's consider the following example:

function foo() { if true; then false && true; fi }
> function bar() { foo }
> bar


Exception 2 tells us that "false && true" shouldn't trigger an ERR_EXIT. By
exception 3, the resulting exit status becomes the result, first of the
enclosing if, and then of the enclosing {}, without triggering an ERR_EXIT.
It's only in function "bar" that an "ERR_EXIT" should be triggered because
the call to "foo" returns 1 and function calls aren't compound commands.

Zsh 5.8.1, doesn't trigger any ERR_EXIT for this example. Zsh 5.9.*, with
your latest changes, triggers an ERR_EXIT in "foo". Bash 5.1.16 triggers an
ERR_EXIT in "bar". My understanding is that Bash is right.

Here is the script that I used to test this behavior with Zsh and Bash (for
bash you have to comment out the "foo?3" tests):

# test.zsh
> # Usage: test.zsh A1|...|A4|B1|...|B4|C1|...|C4
> #
> # Example: "test.zsh A1" runs the test for "fooA1".


> set -e


> function err-exit() {
>     local error=$?;
>     # Print where ERR_EXIT was called from.
>     if [[ -n $ZSH_VERSION ]]; then
>         local file=${funcfiletrace[1]%:*}
>         local line=${funcfiletrace[1]#*:}
>     elif [[ -n $BASH_VERSION ]]; then
>         local caller=$(caller 0);
>         local file=${caller##* }
>         local line=${caller%% *}
>     fi
>     echo "Caught error $error at $file:$line: $(cat $file | head -$line |
> tail -1)" >&2
>     return $error;
> }


> trap err-exit ERR
> if [[ -n $ZSH_VERSION ]]; then
>     alias init=''
> elif [[ -n $BASH_VERSION ]]; then
>     # It looks like in Bash the ERR trap must be set in every function
>     shopt -s expand_aliases
>     alias init='trap err-exit ERR'
> fi


> function fooA1() { init;               false        ;
>           }
> function fooA2() { init; if true; then false        ; fi
>          }
> function fooA3() { init;             { false        ; } always { true; }
>          }
> function fooA4() { init;             { false        ; }
>           }


> function fooB1() { init;               false && true;
>           }
> function fooB2() { init; if true; then false && true; fi
>          }
> function fooB3() { init;             { false && true; } always { true; }
>          }
> function fooB4() { init;             { false && true; }
>           }


> function fooC1() { init;               false && true                    ;
> echo foo; }
> function fooC2() { init; if true; then false && true; fi                ;
> echo foo; }
> function fooC3() { init;             { false && true; } always { true; };
> echo foo; }
> function fooC4() { init;             { false && true; }                 ;
> echo foo; }


> function bar() {
>     init
>     echo Start
>     foo$1
>     echo End
> }


> bar "$@"


I included the "foo?3" tests because all the compound commands in loop.c
seem to behave the same except for "exectry", which lacks the following
line:

  this_noerrexit = (WC_SUBLIST_TYPE(*end) != WC_SUBLIST_END);


 I included the "foo?4" tests because sequences are not handled by one of
the functions in loop.c but by some function in exec.c. However, all
"foo?3" and "foo?4" tests nevertheless behave the same as the matching
"foo?2" tests.

Here are the results:

        Zsh 5.8.1      Zsh 5.9.*      Bash 5.1.16



fooA1   Exit in foo*   Exit in foo*   Exit in foo*
> fooA2   Exit in foo*   Exit in foo*   Exit in foo*



fooB1   Exit in bar    Exit in bar    Exit in bar
> fooB2   No exit        Exit in foo*   Exit in bar



fooC1   No exit        No exit        No exit
> fooC2   No exit        Exit in foo*   No exit


My understanding is that Bash's behavior is the correct one.

Philippe

[-- Attachment #2: Type: text/html, Size: 12162 bytes --]

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] More ERR_EXIT (was Re: Tests RE behavior of ERR_EXIT)
  2022-11-12 22:16 [PATCH] More ERR_EXIT (was Re: Tests RE behavior of ERR_EXIT) Philippe Altherr
@ 2022-11-13  3:59 ` Philippe Altherr
  2022-11-13  4:11   ` Bart Schaefer
  0 siblings, 1 reply; 26+ messages in thread
From: Philippe Altherr @ 2022-11-13  3:59 UTC (permalink / raw)
  To: Bart Schaefer, zsh-workers

[-- Attachment #1: Type: text/plain, Size: 4773 bytes --]

>
> My high-level understanding of exception 3 is that if an error was
> produced by a condition (note that the last command in a sub-list is NOT a
> condition but a regular command), then that error should bubble up the
> evaluation stack, without triggering any ERR_EXIT, until it gets ignored
> (for example on the left of a ";" in a sequence) or it becomes the result
> of an enclosing function or subshell.


In other words, an error produced by a condition should never trigger an
ERR_EXIT unless it becomes the result of a function or of a subshell, in
which case an ERR_EXIT should be triggered at the calling point of the
function or subshell.

I assume that command substitutions play the same role as subshells and may
turn errors from conditions into ERR_EXITs. However, I assume that they
only ever do so if their result isn't ignored. So "v=$(...)" may trigger an
ERR_EXIT but never ": $(...)". I added new tests for that and Bash does
indeed behave like that.

Below is the new set of tests .I dropped the one for always blocks and for
sequences, i.e., all the "foo?3" and "foo?4" tests because they don't
differ from the "foo?2" tests. I also reordered the tests to better group
together the ones that behave correctly.

function foo1AX() {           init;               false
>       ; }
> function foo1AY() { init; v=$(init;               false
>        ); }
> function foo1AZ() { init; : $(init;               false
>        ); }



function foo1BX() {           init;               false && true
>       ; }
> function foo1BY() { init; v=$(init;               false && true
>        ); }
> function foo1BZ() { init; : $(init;               false && true
>        ); }



function foo1CX() {           init;               false && true    ; echo
> foo >&2;  }
> function foo1CY() { init; v=$(init;               false && true    ; echo
> foo >&2); }
> function foo1CZ() { init; : $(init;               false && true    ; echo
> foo >&2); }



function foo2AX() {           init; if true; then false        ; fi
>       ; }
> function foo2AY() { init; v=$(init; if true; then false        ; fi
>        ); }
> function foo2AZ() { init; : $(init; if true; then false        ; fi
>        ); }



function foo2BX() {           init; if true; then false && true; fi
>       ; }
> function foo2BY() { init; v=$(init; if true; then false && true; fi
>        ); }
> function foo2BZ() { init; : $(init; if true; then false && true; fi
>        ); }



function foo2CX() {           init; if true; then false && true; fi; echo
> foo >&2;  }
> function foo2CY() { init; v=$(init; if true; then false && true; fi; echo
> foo >&2); }
> function foo2CZ() { init; : $(init; if true; then false && true; fi; echo
> foo >&2); }


And here are the updated results:


         Zsh 5.8.1              Zsh 5.9.0.1-dev        Bash
> 5.1.16(1)-release



foo1AX   Exit in foo*           Exit in foo*           Exit in foo*
> foo1AY   Exit in $() and foo*   Exit in $() and foo*   Exit in $() and foo*
> foo1AZ   Exit in $()            Exit in $()            Exit in $()



foo1BX   Exit in bar            Exit in bar            Exit in bar
> foo1BY   Exit in foo*           Exit in foo*           Exit in foo*
> foo1BZ   No exit                No exit                No exit



foo1CX   No exit                No exit                No exit
> foo1CY   No exit                No exit                No exit
> foo1CZ   No exit                No exit                No exit



foo2AX   Exit in foo*           Exit in foo*           Exit in foo*
> foo2AY   Exit in $() and foo*   Exit in $() and foo*   Exit in $() and foo*
> foo2AZ   Exit in $()            Exit in $()            Exit in $()



foo2BX   No exit                Exit in foo*           Exit in bar
> foo2BY   Exit in foo*           Exit in $() and foo*   Exit in foo*
> foo2BZ   No exit                Exit in $()            No exit



foo2CX   No exit                Exit in foo*           No exit
> foo2CY   No exit                Exit in $() and foo*   No exit
> foo2CZ   No exit                Exit in $()            No exit


- Bash behaves correctly for all tests (from my understanding of the POSIX
spec).
- Zsh 5.8 and 5.9 behave correctly for all "foo1??" and "foo2A?" tests.
- For all "foo2B?" and "foo2C?" tests, either Zsh 5.8 and/or Zsh 5.9 behave
incorrectly.
- Command substitutions don't seem to have any problem (of their own).
   - In other words, only test foo2BX and foo2CX matter. If they get fixed,
I expect the other ones to be fixed too.
- The problem seems to be related to how "false && true" interacts with
other constructs.
   - In Zsh 5.9 the problem seems to be that compound commands now
incorrectly trigger ERR_EXIT.
   - In Zsh 5.8 I can't pinpoint the problem, for now.

Philippe

[-- Attachment #2: Type: text/html, Size: 9673 bytes --]

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] More ERR_EXIT (was Re: Tests RE behavior of ERR_EXIT)
  2022-11-13  3:59 ` Philippe Altherr
@ 2022-11-13  4:11   ` Bart Schaefer
  2022-11-13 13:55     ` Philippe Altherr
  0 siblings, 1 reply; 26+ messages in thread
From: Bart Schaefer @ 2022-11-13  4:11 UTC (permalink / raw)
  To: Philippe Altherr; +Cc: zsh-workers

On Sat, Nov 12, 2022 at 7:59 PM Philippe Altherr
<philippe.altherr@gmail.com> wrote:
>
> - The problem seems to be related to how "false && true" interacts with other constructs.
>    - In Zsh 5.9 the problem seems to be that compound commands now incorrectly trigger ERR_EXIT.
>    - In Zsh 5.8 I can't pinpoint the problem, for now.

You shouldn't even be bothering with 5.8.1, it's been wrong all along;
it blindly never errexits at the end of an if/then/fi.

I think my patches so far have uncovered a different bug that was
already present but was masked by the foregoing, which is, that
noerrexit is unwound in cases where it should not be.  I think this is
happening at lines 1530-1531 of exec.c, right under the comment about
"hairy code near the end of execif()".  That's an area I didn't touch,
but I'm pretty sure it's restoring noerrexit to its state before
entering the "if" (oldnoerrexit) when it should be preserving the
state from the "&&" conditional.  In 5.8.1 this gets reversed again
via this_noerrexit.


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] More ERR_EXIT (was Re: Tests RE behavior of ERR_EXIT)
  2022-11-13  4:11   ` Bart Schaefer
@ 2022-11-13 13:55     ` Philippe Altherr
  2022-11-13 14:24       ` Philippe Altherr
  2022-11-13 16:45       ` Bart Schaefer
  0 siblings, 2 replies; 26+ messages in thread
From: Philippe Altherr @ 2022-11-13 13:55 UTC (permalink / raw)
  To: Bart Schaefer; +Cc: zsh-workers

[-- Attachment #1: Type: text/plain, Size: 5202 bytes --]

>
> You shouldn't even be bothering with 5.8.1, it's been wrong all along;
> it blindly never errexits at the end of an if/then/fi.


I think that this isn't necessarily wrong. My understanding of the code so
far is that the decision to trigger an ERR_EXIT is pushed down the
evaluation stack. In "if cmd1; then cmd2; else cmd3; fi", only the
evaluations of the (word codes representing the) commands "cmd1", "cmd2",
or "cmd3" can ever trigger an ERR_EXIT. The evaluation of the (word codes
representing the) if/then/else itself never triggers an ERR_EXIT. In other
words only (the word codes representing) "basic commands", like function
calls or UNIX commands, can ever trigger an ERR_EXIT. This strategy has the
benefit that ERR_EXIT will be triggered exactly at the point where the
fatal non-zero exit status was produced.

00 if
01   cmd1
02  then
03   cmd2
04. else
05.   cmd3
06 fi

In the example above, with the strategy I described, and with the knowledge
that the if condition never triggers an ERR_EXIT, it's guaranteed that an
ERR_EXIT will only ever be thrown at line 03 or 05. If the triggering of
the ERR_EXIT was sometimes delayed and delegated to the if/then/else, then
ERR_EXIT could also be triggered at line 02 or 04, or worse at line 01 or
06, which wouldn't let you know whether the non-zero status originated from
"cmd2" or from "cmd3". The delayed/delegated triggering looks undesirable
because it gives you less information on the origin of the error. My
understanding is that it's also never needed.

The behavior of ERR_EXIT is controlled by the variables "noerrexit" and
"local_noerrexit". My understanding of these variables is the following:

- noerrexit: This variable is set to indicate that the triggering of
ERR_EXIT must be disabled in the evaluation of any word code from the point
where it's set until it's reset. For example it's set here
<https://github.com/zsh-users/zsh/blob/b1533066ca7d50c88b37ce72093c12cf19807818/Src/loop.c#L550>
in execif before the evaluation of the condition and reset here
<https://github.com/zsh-users/zsh/blob/b1533066ca7d50c88b37ce72093c12cf19807818/Src/loop.c#L576>,
here
<https://github.com/zsh-users/zsh/blob/b1533066ca7d50c88b37ce72093c12cf19807818/Src/loop.c#L578>,
here
<https://github.com/zsh-users/zsh/blob/b1533066ca7d50c88b37ce72093c12cf19807818/Src/loop.c#L580>,
and here
<https://github.com/zsh-users/zsh/blob/b1533066ca7d50c88b37ce72093c12cf19807818/Src/loop.c#L585>
after the evaluation of the condition. I don't really understand why the
reseting is so complicated. It's much more straightforward in execwhile (
here
<https://github.com/zsh-users/zsh/blob/b1533066ca7d50c88b37ce72093c12cf19807818/Src/loop.c#L440>
).

- local_noerrexit: This variable is set to indicate that the triggering of
ERR_EXIT must be disabled in the remainder of the evaluation of the current
word code. For example it's set at the end of each compound command, like
here
<https://github.com/zsh-users/zsh/blob/b1533066ca7d50c88b37ce72093c12cf19807818/Src/loop.c#L481>.
This used to be a plain "this_noerrexit = 1", which I don't think was wrong.

I think my patches so far have uncovered a different bug that was
> already present but was masked by the foregoing, which is, that
> noerrexit is unwound in cases where it should not be.  I think this is
> happening at lines 1530-1531 of exec.c, right under the comment about
> "hairy code near the end of execif()".  That's an area I didn't touch,
> but I'm pretty sure it's restoring noerrexit to its state before
> entering the "if" (oldnoerrexit) when it should be preserving the
> state from the "&&" conditional.  In 5.8.1 this gets reversed again
> via this_noerrexit.


I must admit that I don't understand the NOERREXIT_UNTIL_EXEC logic here
<https://github.com/zsh-users/zsh/blob/b1533066ca7d50c88b37ce72093c12cf19807818/Src/exec.c#L1529>,
nor the complicated resetting logic of noerrexit at the end of execif. I
was about to say that this doesn't seem to be the source of the problem
because if, while, and for statements all behave the same in Zsh.

function fooIf1()    { init; cond=true; if    $cond; then cond=false; false
>        ; fi  ; }
> function fooIf2()    { init; cond=true; if    $cond; then cond=false;
> false && true; fi  ; }



function fooWhile1() { init; cond=true; while $cond; do   cond=false; false
>        ; done; }
> function fooWhile2() { init; cond=true; while $cond; do   cond=false;
> false && true; done; }



function fooFor1()   { init; cond=true; for v in x ; do   cond=false; false
>        ; done; }
> function fooFor2()   { init; cond=true; for v in x ; do   cond=false;
> false && true; done; }


In the examples above fooIf1, fooWhile1, and fooFor1 all work correctly but
fooIf2, fooWhile2, and fooFor2 fail to trigger ERR_EXIT in Zsh 5.8 and
trigger it too early (in foo instead of in bar) in Zsh 5.9.

However, if I comment out the NOERREXIT_UNTIL_EXEC logic in exec.c (or
remove the negation), then fooIf2 and surprisingly also fooFor2 work
correctly in Zsh 5.9 but not fooWhile2!?! fooWhile2 still triggers too
early.

So it looks like this may indeed be the start of the answer. But I'm still
scratching my head on why that is.

Philippe

[-- Attachment #2: Type: text/html, Size: 7053 bytes --]

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] More ERR_EXIT (was Re: Tests RE behavior of ERR_EXIT)
  2022-11-13 13:55     ` Philippe Altherr
@ 2022-11-13 14:24       ` Philippe Altherr
  2022-11-13 15:45         ` Philippe Altherr
  2022-11-13 16:45       ` Bart Schaefer
  1 sibling, 1 reply; 26+ messages in thread
From: Philippe Altherr @ 2022-11-13 14:24 UTC (permalink / raw)
  To: Bart Schaefer; +Cc: zsh-workers

[-- Attachment #1: Type: text/plain, Size: 5875 bytes --]

The commenting out also fixes the problem for case statements and braces
(i.e., for "{ ... }"). It works even if loop.c is reverted to the previous
state with "this_noerrexit = 1" statements, which seem more correct to me.

Apparently execwhile, like execif, needs more complicated
noerrexit resetting logic, even though I still don't understand what it's
doing in execif and why it's needed.

Philippe


On Sun, Nov 13, 2022 at 2:55 PM Philippe Altherr <philippe.altherr@gmail.com>
wrote:

> You shouldn't even be bothering with 5.8.1, it's been wrong all along;
>> it blindly never errexits at the end of an if/then/fi.
>
>
> I think that this isn't necessarily wrong. My understanding of the code so
> far is that the decision to trigger an ERR_EXIT is pushed down the
> evaluation stack. In "if cmd1; then cmd2; else cmd3; fi", only the
> evaluations of the (word codes representing the) commands "cmd1", "cmd2",
> or "cmd3" can ever trigger an ERR_EXIT. The evaluation of the (word codes
> representing the) if/then/else itself never triggers an ERR_EXIT. In other
> words only (the word codes representing) "basic commands", like function
> calls or UNIX commands, can ever trigger an ERR_EXIT. This strategy has the
> benefit that ERR_EXIT will be triggered exactly at the point where the
> fatal non-zero exit status was produced.
>
> 00 if
> 01   cmd1
> 02  then
> 03   cmd2
> 04. else
> 05.   cmd3
> 06 fi
>
> In the example above, with the strategy I described, and with the
> knowledge that the if condition never triggers an ERR_EXIT, it's guaranteed
> that an ERR_EXIT will only ever be thrown at line 03 or 05. If the
> triggering of the ERR_EXIT was sometimes delayed and delegated to the
> if/then/else, then ERR_EXIT could also be triggered at line 02 or 04, or
> worse at line 01 or 06, which wouldn't let you know whether the non-zero
> status originated from "cmd2" or from "cmd3". The delayed/delegated
> triggering looks undesirable because it gives you less information on the
> origin of the error. My understanding is that it's also never needed.
>
> The behavior of ERR_EXIT is controlled by the variables "noerrexit" and
> "local_noerrexit". My understanding of these variables is the following:
>
> - noerrexit: This variable is set to indicate that the triggering of
> ERR_EXIT must be disabled in the evaluation of any word code from the point
> where it's set until it's reset. For example it's set here
> <https://github.com/zsh-users/zsh/blob/b1533066ca7d50c88b37ce72093c12cf19807818/Src/loop.c#L550>
> in execif before the evaluation of the condition and reset here
> <https://github.com/zsh-users/zsh/blob/b1533066ca7d50c88b37ce72093c12cf19807818/Src/loop.c#L576>,
> here
> <https://github.com/zsh-users/zsh/blob/b1533066ca7d50c88b37ce72093c12cf19807818/Src/loop.c#L578>,
> here
> <https://github.com/zsh-users/zsh/blob/b1533066ca7d50c88b37ce72093c12cf19807818/Src/loop.c#L580>,
> and here
> <https://github.com/zsh-users/zsh/blob/b1533066ca7d50c88b37ce72093c12cf19807818/Src/loop.c#L585>
> after the evaluation of the condition. I don't really understand why the
> reseting is so complicated. It's much more straightforward in execwhile (
> here
> <https://github.com/zsh-users/zsh/blob/b1533066ca7d50c88b37ce72093c12cf19807818/Src/loop.c#L440>
> ).
>
> - local_noerrexit: This variable is set to indicate that the triggering of
> ERR_EXIT must be disabled in the remainder of the evaluation of the current
> word code. For example it's set at the end of each compound command, like
> here
> <https://github.com/zsh-users/zsh/blob/b1533066ca7d50c88b37ce72093c12cf19807818/Src/loop.c#L481>.
> This used to be a plain "this_noerrexit = 1", which I don't think was wrong.
>
> I think my patches so far have uncovered a different bug that was
>> already present but was masked by the foregoing, which is, that
>> noerrexit is unwound in cases where it should not be.  I think this is
>> happening at lines 1530-1531 of exec.c, right under the comment about
>> "hairy code near the end of execif()".  That's an area I didn't touch,
>> but I'm pretty sure it's restoring noerrexit to its state before
>> entering the "if" (oldnoerrexit) when it should be preserving the
>> state from the "&&" conditional.  In 5.8.1 this gets reversed again
>> via this_noerrexit.
>
>
> I must admit that I don't understand the NOERREXIT_UNTIL_EXEC logic here
> <https://github.com/zsh-users/zsh/blob/b1533066ca7d50c88b37ce72093c12cf19807818/Src/exec.c#L1529>,
> nor the complicated resetting logic of noerrexit at the end of execif. I
> was about to say that this doesn't seem to be the source of the problem
> because if, while, and for statements all behave the same in Zsh.
>
> function fooIf1()    { init; cond=true; if    $cond; then cond=false;
>> false        ; fi  ; }
>> function fooIf2()    { init; cond=true; if    $cond; then cond=false;
>> false && true; fi  ; }
>
>
>
> function fooWhile1() { init; cond=true; while $cond; do   cond=false;
>> false        ; done; }
>> function fooWhile2() { init; cond=true; while $cond; do   cond=false;
>> false && true; done; }
>
>
>
> function fooFor1()   { init; cond=true; for v in x ; do   cond=false;
>> false        ; done; }
>> function fooFor2()   { init; cond=true; for v in x ; do   cond=false;
>> false && true; done; }
>
>
> In the examples above fooIf1, fooWhile1, and fooFor1 all work correctly
> but fooIf2, fooWhile2, and fooFor2 fail to trigger ERR_EXIT in Zsh 5.8 and
> trigger it too early (in foo instead of in bar) in Zsh 5.9.
>
> However, if I comment out the NOERREXIT_UNTIL_EXEC logic in exec.c (or
> remove the negation), then fooIf2 and surprisingly also fooFor2 work
> correctly in Zsh 5.9 but not fooWhile2!?! fooWhile2 still triggers too
> early.
>
> So it looks like this may indeed be the start of the answer. But I'm still
> scratching my head on why that is.
>
> Philippe
>
>

[-- Attachment #2: Type: text/html, Size: 8067 bytes --]

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] More ERR_EXIT (was Re: Tests RE behavior of ERR_EXIT)
  2022-11-13 14:24       ` Philippe Altherr
@ 2022-11-13 15:45         ` Philippe Altherr
  2022-11-13 16:52           ` Bart Schaefer
  0 siblings, 1 reply; 26+ messages in thread
From: Philippe Altherr @ 2022-11-13 15:45 UTC (permalink / raw)
  To: Bart Schaefer; +Cc: zsh-workers

[-- Attachment #1: Type: text/plain, Size: 7533 bytes --]

Maybe this gives you a clue. In the code below the only difference between
fooLoop1 and fooLoop2 is that the latter uses "false && true" instead of
"false", which should not immediately trigger an ERR_EXIT but instead
bubble up to the calling function and trigger an ERR_EXIT there.

function fooLoop1() {
>     init;
>     v=2;
>     while [[ $v -ne 0 ]]; do
>         echo "Loop with v=$v" >&2;
>         v=$((v-1));
>         false;
>     done
> }
> function fooLoop2() {
>     init;
>     v=2;
>     while [[ $v -ne 0 ]]; do
>         echo "Loop with v=$v" >&2;
>         v=$((v-1));
>         false && true;
>     done
> }


In the latest Zsh, fooLoop1 triggers an ERR_EXIT at the line of the "false"
(expected and correct). fooLoop2 also triggers an ERR_EXIT but after two
loop iterations and at the line of the while.

Now let's remove the following code from exec.c

       if (!(oldnoerrexit & NOERREXIT_UNTIL_EXEC))
>            noerrexit = oldnoerrexit;


With that change the two functions still behave exactly the same but if you
replace the while statement with any other statement (if, case, for, ...)
then no ERR_EXIT is triggered in foo. Instead the non-zero exit status
correctly bubbles up to the caller and triggers an ERR_EXIT in bar.

I still don't understand why the change above fixes the problem for if,
case, for, ... statements. I understand even less why it doesn't fix it for
while statements.

Philippe


On Sun, Nov 13, 2022 at 3:24 PM Philippe Altherr <philippe.altherr@gmail.com>
wrote:

> The commenting out also fixes the problem for case statements and braces
> (i.e., for "{ ... }"). It works even if loop.c is reverted to the previous
> state with "this_noerrexit = 1" statements, which seem more correct to me.
>
> Apparently execwhile, like execif, needs more complicated
> noerrexit resetting logic, even though I still don't understand what it's
> doing in execif and why it's needed.
>
> Philippe
>
>
> On Sun, Nov 13, 2022 at 2:55 PM Philippe Altherr <
> philippe.altherr@gmail.com> wrote:
>
>> You shouldn't even be bothering with 5.8.1, it's been wrong all along;
>>> it blindly never errexits at the end of an if/then/fi.
>>
>>
>> I think that this isn't necessarily wrong. My understanding of the code
>> so far is that the decision to trigger an ERR_EXIT is pushed down the
>> evaluation stack. In "if cmd1; then cmd2; else cmd3; fi", only the
>> evaluations of the (word codes representing the) commands "cmd1", "cmd2",
>> or "cmd3" can ever trigger an ERR_EXIT. The evaluation of the (word codes
>> representing the) if/then/else itself never triggers an ERR_EXIT. In other
>> words only (the word codes representing) "basic commands", like function
>> calls or UNIX commands, can ever trigger an ERR_EXIT. This strategy has the
>> benefit that ERR_EXIT will be triggered exactly at the point where the
>> fatal non-zero exit status was produced.
>>
>> 00 if
>> 01   cmd1
>> 02  then
>> 03   cmd2
>> 04. else
>> 05.   cmd3
>> 06 fi
>>
>> In the example above, with the strategy I described, and with the
>> knowledge that the if condition never triggers an ERR_EXIT, it's guaranteed
>> that an ERR_EXIT will only ever be thrown at line 03 or 05. If the
>> triggering of the ERR_EXIT was sometimes delayed and delegated to the
>> if/then/else, then ERR_EXIT could also be triggered at line 02 or 04, or
>> worse at line 01 or 06, which wouldn't let you know whether the non-zero
>> status originated from "cmd2" or from "cmd3". The delayed/delegated
>> triggering looks undesirable because it gives you less information on the
>> origin of the error. My understanding is that it's also never needed.
>>
>> The behavior of ERR_EXIT is controlled by the variables "noerrexit" and
>> "local_noerrexit". My understanding of these variables is the following:
>>
>> - noerrexit: This variable is set to indicate that the triggering of
>> ERR_EXIT must be disabled in the evaluation of any word code from the point
>> where it's set until it's reset. For example it's set here
>> <https://github.com/zsh-users/zsh/blob/b1533066ca7d50c88b37ce72093c12cf19807818/Src/loop.c#L550>
>> in execif before the evaluation of the condition and reset here
>> <https://github.com/zsh-users/zsh/blob/b1533066ca7d50c88b37ce72093c12cf19807818/Src/loop.c#L576>,
>> here
>> <https://github.com/zsh-users/zsh/blob/b1533066ca7d50c88b37ce72093c12cf19807818/Src/loop.c#L578>,
>> here
>> <https://github.com/zsh-users/zsh/blob/b1533066ca7d50c88b37ce72093c12cf19807818/Src/loop.c#L580>,
>> and here
>> <https://github.com/zsh-users/zsh/blob/b1533066ca7d50c88b37ce72093c12cf19807818/Src/loop.c#L585>
>> after the evaluation of the condition. I don't really understand why the
>> reseting is so complicated. It's much more straightforward in execwhile (
>> here
>> <https://github.com/zsh-users/zsh/blob/b1533066ca7d50c88b37ce72093c12cf19807818/Src/loop.c#L440>
>> ).
>>
>> - local_noerrexit: This variable is set to indicate that the triggering
>> of ERR_EXIT must be disabled in the remainder of the evaluation of the
>> current word code. For example it's set at the end of each compound
>> command, like here
>> <https://github.com/zsh-users/zsh/blob/b1533066ca7d50c88b37ce72093c12cf19807818/Src/loop.c#L481>.
>> This used to be a plain "this_noerrexit = 1", which I don't think was wrong.
>>
>> I think my patches so far have uncovered a different bug that was
>>> already present but was masked by the foregoing, which is, that
>>> noerrexit is unwound in cases where it should not be.  I think this is
>>> happening at lines 1530-1531 of exec.c, right under the comment about
>>> "hairy code near the end of execif()".  That's an area I didn't touch,
>>> but I'm pretty sure it's restoring noerrexit to its state before
>>> entering the "if" (oldnoerrexit) when it should be preserving the
>>> state from the "&&" conditional.  In 5.8.1 this gets reversed again
>>> via this_noerrexit.
>>
>>
>> I must admit that I don't understand the NOERREXIT_UNTIL_EXEC logic here
>> <https://github.com/zsh-users/zsh/blob/b1533066ca7d50c88b37ce72093c12cf19807818/Src/exec.c#L1529>,
>> nor the complicated resetting logic of noerrexit at the end of execif. I
>> was about to say that this doesn't seem to be the source of the problem
>> because if, while, and for statements all behave the same in Zsh.
>>
>> function fooIf1()    { init; cond=true; if    $cond; then cond=false;
>>> false        ; fi  ; }
>>> function fooIf2()    { init; cond=true; if    $cond; then cond=false;
>>> false && true; fi  ; }
>>
>>
>>
>> function fooWhile1() { init; cond=true; while $cond; do   cond=false;
>>> false        ; done; }
>>> function fooWhile2() { init; cond=true; while $cond; do   cond=false;
>>> false && true; done; }
>>
>>
>>
>> function fooFor1()   { init; cond=true; for v in x ; do   cond=false;
>>> false        ; done; }
>>> function fooFor2()   { init; cond=true; for v in x ; do   cond=false;
>>> false && true; done; }
>>
>>
>> In the examples above fooIf1, fooWhile1, and fooFor1 all work correctly
>> but fooIf2, fooWhile2, and fooFor2 fail to trigger ERR_EXIT in Zsh 5.8 and
>> trigger it too early (in foo instead of in bar) in Zsh 5.9.
>>
>> However, if I comment out the NOERREXIT_UNTIL_EXEC logic in exec.c (or
>> remove the negation), then fooIf2 and surprisingly also fooFor2 work
>> correctly in Zsh 5.9 but not fooWhile2!?! fooWhile2 still triggers too
>> early.
>>
>> So it looks like this may indeed be the start of the answer. But I'm
>> still scratching my head on why that is.
>>
>> Philippe
>>
>>

[-- Attachment #2: Type: text/html, Size: 10515 bytes --]

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] More ERR_EXIT (was Re: Tests RE behavior of ERR_EXIT)
  2022-11-13 13:55     ` Philippe Altherr
  2022-11-13 14:24       ` Philippe Altherr
@ 2022-11-13 16:45       ` Bart Schaefer
  2022-11-13 16:53         ` Bart Schaefer
  2022-11-15  1:11         ` Bart Schaefer
  1 sibling, 2 replies; 26+ messages in thread
From: Bart Schaefer @ 2022-11-13 16:45 UTC (permalink / raw)
  To: Philippe Altherr; +Cc: zsh-workers

On Sun, Nov 13, 2022 at 5:55 AM Philippe Altherr
<philippe.altherr@gmail.com> wrote:
>>
>> You shouldn't even be bothering with 5.8.1, it's been wrong all along;
>> it blindly never errexits at the end of an if/then/fi.
>
> I think that this isn't necessarily wrong.

It would have failed all the tests currently in C03traps.  However,
the changes in loop.c from workers/50897 may be redundant with those
at line 1445 of exec.c from workers/50928 and 509289.


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] More ERR_EXIT (was Re: Tests RE behavior of ERR_EXIT)
  2022-11-13 15:45         ` Philippe Altherr
@ 2022-11-13 16:52           ` Bart Schaefer
  0 siblings, 0 replies; 26+ messages in thread
From: Bart Schaefer @ 2022-11-13 16:52 UTC (permalink / raw)
  To: Philippe Altherr; +Cc: zsh-workers

On Sun, Nov 13, 2022 at 7:46 AM Philippe Altherr
<philippe.altherr@gmail.com> wrote:
>
> Now let's remove the following code from exec.c
>
>>        if (!(oldnoerrexit & NOERREXIT_UNTIL_EXEC))
>>            noerrexit = oldnoerrexit;
>
>
> With that change the two functions still behave exactly the same but if you replace the while statement with any other statement (if, case, for, ...) then no ERR_EXIT is triggered in foo. Instead the non-zero exit status correctly bubbles up to the caller and triggers an ERR_EXIT in bar.
>
> I still don't understand why the change above fixes the problem for if, case, for, ... statements. I understand even less why it doesn't fix it for while statements.

NOERREXIT_UNTIL_EXEC is what protects the tests between keywords "if"
and "then" and those between "while" and "do" from triggering
ERR_EXIT.

The problem is that when an && || conditional expression appears
between "then" and "fi" or between "do" and "done" the assignment of
noerrexit = olderrexit still happens, which resets the state too far
into the past.


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] More ERR_EXIT (was Re: Tests RE behavior of ERR_EXIT)
  2022-11-13 16:45       ` Bart Schaefer
@ 2022-11-13 16:53         ` Bart Schaefer
  2022-11-13 18:37           ` Philippe Altherr
  2022-11-15  1:11         ` Bart Schaefer
  1 sibling, 1 reply; 26+ messages in thread
From: Bart Schaefer @ 2022-11-13 16:53 UTC (permalink / raw)
  To: Philippe Altherr; +Cc: zsh-workers

On Sun, Nov 13, 2022 at 8:45 AM Bart Schaefer <schaefer@brasslantern.com> wrote:
>
> at line 1445 of exec.c from workers/50928 and 509289.

50929 obviously.


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] More ERR_EXIT (was Re: Tests RE behavior of ERR_EXIT)
  2022-11-13 16:53         ` Bart Schaefer
@ 2022-11-13 18:37           ` Philippe Altherr
  2022-11-13 20:55             ` Philippe Altherr
  2022-11-13 22:12             ` Bart Schaefer
  0 siblings, 2 replies; 26+ messages in thread
From: Philippe Altherr @ 2022-11-13 18:37 UTC (permalink / raw)
  To: Bart Schaefer; +Cc: zsh-workers

[-- Attachment #1: Type: text/plain, Size: 324 bytes --]

>
> NOERREXIT_UNTIL_EXEC is what protects the tests between keywords "if"
> and "then" and those between "while" and "do" from triggering

ERR_EXIT.


Really? NOERREXIT_UNTIL_EXEC is only ever set in execif. How can it impact
the code of a while statement? There is no trace of it in execwhile. What
am I missing?

Philippe

[-- Attachment #2: Type: text/html, Size: 732 bytes --]

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] More ERR_EXIT (was Re: Tests RE behavior of ERR_EXIT)
  2022-11-13 18:37           ` Philippe Altherr
@ 2022-11-13 20:55             ` Philippe Altherr
  2022-11-13 22:27               ` Bart Schaefer
  2022-11-13 23:10               ` Lawrence Velázquez
  2022-11-13 22:12             ` Bart Schaefer
  1 sibling, 2 replies; 26+ messages in thread
From: Philippe Altherr @ 2022-11-13 20:55 UTC (permalink / raw)
  To: Bart Schaefer; +Cc: zsh-workers

[-- Attachment #1: Type: text/plain, Size: 1609 bytes --]

Here is one thing that could explain why the "where" statement is an
outlier (at least compared to the other compound commands that I have
tested so far). It's the only case where after evaluating "false && true"
you have to first evaluate a whole other command, the loop condition,
before deciding whether you trigger an ERR_EXIT for the "while". For the
other compound commands, you evaluate the "false && true" and then
immediately decide whether you trigger an ERR_EXIT for the compound command.

Here is an observation that could save some complexity. A non-zero exit
status either immediately triggers an ERR_EXIT, like in "false" or it
doesn't like in "false && true" and instead bubbles up to the first
enclosing function or subshell thanks to exception 3. Thus, compound
commands other than subshells never have to trigger an ERR_EXIT. That's a
nice and not necessarily obvious side benefit of exception 3. I wonder
whether the code was designed without that insight or maybe without knowing
about exception 3 altogether. Without exception 3, compound commands
sometimes have to trigger an ERR_EXIT and that could explain some of the
current code and behavior.

Philippe


On Sun, Nov 13, 2022 at 7:37 PM Philippe Altherr <philippe.altherr@gmail.com>
wrote:

> NOERREXIT_UNTIL_EXEC is what protects the tests between keywords "if"
>> and "then" and those between "while" and "do" from triggering
>
> ERR_EXIT.
>
>
> Really? NOERREXIT_UNTIL_EXEC is only ever set in execif. How can it impact
> the code of a while statement? There is no trace of it in execwhile. What
> am I missing?
>
> Philippe
>
>

[-- Attachment #2: Type: text/html, Size: 2475 bytes --]

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] More ERR_EXIT (was Re: Tests RE behavior of ERR_EXIT)
  2022-11-13 18:37           ` Philippe Altherr
  2022-11-13 20:55             ` Philippe Altherr
@ 2022-11-13 22:12             ` Bart Schaefer
  1 sibling, 0 replies; 26+ messages in thread
From: Bart Schaefer @ 2022-11-13 22:12 UTC (permalink / raw)
  To: Philippe Altherr; +Cc: zsh-workers

On Sun, Nov 13, 2022 at 10:37 AM Philippe Altherr
<philippe.altherr@gmail.com> wrote:
>>
> Really? NOERREXIT_UNTIL_EXEC is only ever set in execif. How can it impact the code of a while statement? There is no trace of it in execwhile. What am I missing?

Hmm, I was pretty sure it applied in both of those cases, but
obviously not (or perhaps it's "not any more").  In any case if you
comment out that code near line 1529, a whole slew of tests in
C03traps fail.


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] More ERR_EXIT (was Re: Tests RE behavior of ERR_EXIT)
  2022-11-13 20:55             ` Philippe Altherr
@ 2022-11-13 22:27               ` Bart Schaefer
  2022-11-13 23:10               ` Lawrence Velázquez
  1 sibling, 0 replies; 26+ messages in thread
From: Bart Schaefer @ 2022-11-13 22:27 UTC (permalink / raw)
  To: Philippe Altherr; +Cc: zsh-workers

On Sun, Nov 13, 2022 at 12:56 PM Philippe Altherr
<philippe.altherr@gmail.com> wrote:
>
> Here is one thing that could explain why the "where" [sic] statement is an outlier (at least compared to the other compound commands that I have tested so far). It's the only case where after evaluating "false && true" you have to first evaluate a whole other command, the loop condition, before deciding whether you trigger an ERR_EXIT for the "while".

Yes, that's quite likely, but you're finding that with code that's
already broken (fails multiple C03traps tests).  You can run
  make check TESTNUM=C03
  make check TESTNUM=E01
to determine whether an experimental change is going to regress a
different variation.

> Here is an observation that could save some complexity. A non-zero exit status either immediately triggers an ERR_EXIT, like in "false" or it doesn't like in "false && true" and instead bubbles up to the first enclosing function

It isn't really helping to repeat the observation that the behavior is
not as specified; getting the internal state to reflect "instead
bubbles up" is exactly the complexity.


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] More ERR_EXIT (was Re: Tests RE behavior of ERR_EXIT)
  2022-11-13 20:55             ` Philippe Altherr
  2022-11-13 22:27               ` Bart Schaefer
@ 2022-11-13 23:10               ` Lawrence Velázquez
  1 sibling, 0 replies; 26+ messages in thread
From: Lawrence Velázquez @ 2022-11-13 23:10 UTC (permalink / raw)
  To: Philippe Altherr, Bart Schaefer; +Cc: zsh-workers

On Sun, Nov 13, 2022, at 3:55 PM, Philippe Altherr wrote:
> Thus, compound commands other than subshells never have to trigger
> an ERR_EXIT.

This is not necessarily true, depending on how you attribute blame
for what triggers the early exit.  In the following case, it wouldn't
be unreasonable to associate the failed redirection with the "if"
command.  It's certainly not true that only simple commands can
cause early exits.

	% cat foo.zsh
	if :; then
	    :
	fi >/no/such/path

	printf 'done\n\n'

	% zsh foo.zsh
	foo.zsh:1: no such file or directory: /no/such/path
	done

	% zsh -e foo.zsh
	foo.zsh:1: no such file or directory: /no/such/path

-- 
vq


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] More ERR_EXIT (was Re: Tests RE behavior of ERR_EXIT)
  2022-11-13 16:45       ` Bart Schaefer
  2022-11-13 16:53         ` Bart Schaefer
@ 2022-11-15  1:11         ` Bart Schaefer
  2022-11-15  7:01           ` [PATCH] Even more ERR_EXIT (was Re: More ERR_EXIT " Bart Schaefer
  2022-11-15  7:26           ` [PATCH] More ERR_EXIT (was " Philippe Altherr
  1 sibling, 2 replies; 26+ messages in thread
From: Bart Schaefer @ 2022-11-15  1:11 UTC (permalink / raw)
  To: Philippe Altherr; +Cc: zsh-workers

On Sun, Nov 13, 2022 at 8:45 AM Bart Schaefer <schaefer@brasslantern.com> wrote:
>
> the changes in loop.c from workers/50897 may be redundant with those
> at line 1445 of exec.c from workers/50928 and 50929.

If this is the case (and it appears to be), then there's no point in
bashing through if/case/for/while/repeat/select individually -- the
only case we have to fix is this one:

 (
   setopt ERR_EXIT
   { { { false && true }
        } always { print INSIDE: $? } }
   print OUTSIDE
 )

The above should print "INSIDE: 1" (thus not exit) and then exit
without printing OUTSIDE.


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH] Even more ERR_EXIT (was Re: More ERR_EXIT Re: Tests RE behavior of ERR_EXIT)
  2022-11-15  1:11         ` Bart Schaefer
@ 2022-11-15  7:01           ` Bart Schaefer
  2022-11-15  7:30             ` Philippe Altherr
  2022-11-15  7:26           ` [PATCH] More ERR_EXIT (was " Philippe Altherr
  1 sibling, 1 reply; 26+ messages in thread
From: Bart Schaefer @ 2022-11-15  7:01 UTC (permalink / raw)
  To: Philippe Altherr; +Cc: Zsh hackers list

[-- Attachment #1: Type: text/plain, Size: 1093 bytes --]

On Mon, Nov 14, 2022 at 5:11 PM Bart Schaefer <schaefer@brasslantern.com> wrote:
>
> [if workers/50929 makes 50897 redundant then] there's no point in
> bashing through if/case/for/while/repeat/select individually -- the
> only case we have to fix is this one:

I was half right.  Fixing that case revealed that it was necessary to
go through the others, because the "redundant" fix is one level too
far up the stack.

Attached patch removes 50929 again, and amends this_noerrexit from
50897 in each of the complex command exec* functions.  Finally it adds
tests to E01options for each of the cases that Philippe previously
outlined, with the exception of "select" which is interactive.

The tests from 50928 still pass with this patch.  I spent a ridiculous
amount of time with every change I made flopping back and forth
between C03 working and the new E01 failing and vice-versa, before
noticing what happened when I nested a brace expression inside another
one.

Also a minor fix for TRAPDEBUG that caused me some confusion earlier
in the process.

Are there more cases that need testing?

[-- Attachment #2: errexit_thirdtime.txt --]
[-- Type: text/plain, Size: 5710 bytes --]

diff --git a/Src/exec.c b/Src/exec.c
index ce0c1f1ad..dc5d5dd23 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -451,7 +451,10 @@ execcursh(Estate state, int do_exec)
     cmdpop();
 
     state->pc = end;
-    this_noerrexit = (WC_SUBLIST_TYPE(*end) != WC_SUBLIST_END);
+    if (WC_SUBLIST_TYPE(*end) != WC_SUBLIST_END)
+	this_noerrexit = noerrexit;
+    else
+	this_noerrexit = 1;
 
     return lastval;
 }
@@ -1442,8 +1445,6 @@ execlist(Estate state, int dont_change_job, int exiting)
 		    execsimple(state);
 		else
 		    execpline(state, code, ltype, (ltype & Z_END) && exiting);
-		if (!locallevel || unset(ERRRETURN))
-		    this_noerrexit = noerrexit;
 		state->pc = next;
 		goto sublist_done;
 		break;
@@ -1536,6 +1537,7 @@ sublist_done:
 	     */
 	    int oerrexit_opt = opts[ERREXIT];
 	    opts[ERREXIT] = 0;
+	    oldnoerrexit = noerrexit;
 	    noerrexit = NOERREXIT_EXIT | NOERREXIT_RETURN;
 	    exiting = donetrap;
 	    ret = lastval;
diff --git a/Src/loop.c b/Src/loop.c
index be5261369..cd2f5340b 100644
--- a/Src/loop.c
+++ b/Src/loop.c
@@ -208,7 +208,10 @@ execfor(Estate state, int do_exec)
     loops--;
     simple_pline = old_simple_pline;
     state->pc = end;
-    this_noerrexit = (WC_SUBLIST_TYPE(*end) != WC_SUBLIST_END);
+    if (WC_SUBLIST_TYPE(*end) != WC_SUBLIST_END)
+	this_noerrexit = noerrexit;
+    else
+	this_noerrexit = 1;
     return lastval;
 }
 
@@ -336,7 +339,10 @@ execselect(Estate state, UNUSED(int do_exec))
     loops--;
     simple_pline = old_simple_pline;
     state->pc = end;
-    this_noerrexit = (WC_SUBLIST_TYPE(*end) != WC_SUBLIST_END);
+    if (WC_SUBLIST_TYPE(*end) != WC_SUBLIST_END)
+	this_noerrexit = noerrexit;
+    else
+	this_noerrexit = 1;
     return lastval;
 }
 
@@ -478,7 +484,10 @@ execwhile(Estate state, UNUSED(int do_exec))
     popheap();
     loops--;
     state->pc = end;
-    this_noerrexit = (WC_SUBLIST_TYPE(*end) != WC_SUBLIST_END);
+    if (WC_SUBLIST_TYPE(*end) != WC_SUBLIST_END)
+	this_noerrexit = noerrexit;
+    else
+	this_noerrexit = 1;
     return lastval;
 }
 
@@ -532,7 +541,10 @@ execrepeat(Estate state, UNUSED(int do_exec))
     loops--;
     simple_pline = old_simple_pline;
     state->pc = end;
-    this_noerrexit = (WC_SUBLIST_TYPE(*end) != WC_SUBLIST_END);
+    if (WC_SUBLIST_TYPE(*end) != WC_SUBLIST_END)
+	this_noerrexit = noerrexit;
+    else
+	this_noerrexit = 1;
     return lastval;
 }
 
@@ -587,7 +599,10 @@ execif(Estate state, int do_exec)
 	    lastval = 0;
     }
     state->pc = end;
-    this_noerrexit = (WC_SUBLIST_TYPE(*end) != WC_SUBLIST_END);
+    if (WC_SUBLIST_TYPE(*end) != WC_SUBLIST_END)
+	this_noerrexit = noerrexit;
+    else
+	this_noerrexit = 1;
 
     return lastval;
 }
@@ -701,7 +716,10 @@ execcase(Estate state, int do_exec)
 
     if (!anypatok)
 	lastval = 0;
-    this_noerrexit = (WC_SUBLIST_TYPE(*end) != WC_SUBLIST_END);
+    if (WC_SUBLIST_TYPE(*end) != WC_SUBLIST_END)
+	this_noerrexit = noerrexit;
+    else
+	this_noerrexit = 1;
 
     return lastval;
 }
diff --git a/Test/E01options.ztst b/Test/E01options.ztst
index d38fbed74..939a11a7a 100644
--- a/Test/E01options.ztst
+++ b/Test/E01options.ztst
@@ -385,6 +385,113 @@
 1:ERR_RETURN with additional levels
 >Executed
 
+  (
+    setopt ERR_EXIT
+    { false } always { print INSIDE: $? }
+    print OUTSIDE
+  )
+1:ERR_EXIT from simple brace expression
+
+  (
+    setopt ERR_EXIT
+    { false && true } always { print INSIDE: $? }
+    print OUTSIDE
+  )
+1:ERR_EXIT from conditional brace expression
+>INSIDE: 1
+
+  (
+    setopt ERR_EXIT
+    { if true; then false; true; fi } always { print INSIDE: $? }
+    print OUTSIDE
+  )
+1:ERR_EXIT from "if" with simple body
+
+  (
+    setopt ERR_EXIT
+    { if true; then false && true; fi } always { print INSIDE: $? }
+    print OUTSIDE
+  )
+1:ERR_EXIT from "if" with conditional body
+>INSIDE: 1
+
+  (
+    setopt ERR_EXIT
+    { if false; then :; else false; true; fi } always { print INSIDE: $? }
+    print OUTSIDE
+  )
+1:ERR_EXIT from "else" with simple body
+
+  (
+    setopt ERR_EXIT
+    { if false; then :; else false && true; fi } always { print INSIDE: $? }
+    print OUTSIDE
+  )
+1:ERR_EXIT from "else" with conditional body
+>INSIDE: 1
+
+  (
+    setopt ERR_EXIT
+    { case x in; (*) false;; esac } always { print INSIDE: $? }
+    print OUTSIDE
+  )
+1:ERR_EXIT from "case" with simple body
+
+  (
+    setopt ERR_EXIT
+    { case x in; (*) false && true;; esac } always { print INSIDE: $? }
+    print OUTSIDE
+  )
+1:ERR_EXIT from "case" with conditional body
+>INSIDE: 1
+
+  (
+    setopt ERR_EXIT
+    { for _ in 1; do false; true; done } always { print INSIDE: $? }
+    print OUTSIDE
+  )
+1:ERR_EXIT from "for" with simple body
+
+  (
+    setopt ERR_EXIT
+    { for _ in 1; do false && true; done } always { print INSIDE: $? }
+    print OUTSIDE
+  )
+1:ERR_EXIT from "for" with conditional body
+>INSIDE: 1
+
+  (
+    setopt ERR_EXIT
+    () { { while ((argv[1]--)); do false; true; done
+         } always { print INSIDE: $? } } 1
+    print OUTSIDE
+  )
+1:ERR_EXIT from "while" with simple body
+
+  (
+    setopt ERR_EXIT
+    () { { while ((argv[1]--)); do false && true; done
+         } always { print INSIDE: $? } } 1
+    print OUTSIDE
+  )
+1:ERR_EXIT from "while" with conditional body
+>INSIDE: 1
+
+  (
+    setopt ERR_EXIT
+    { repeat $; do false; done } always { print INSIDE: $? }
+    print OUTSIDE
+  )
+1:ERR_EXIT from "repeat" with simple body
+
+  (
+    setopt ERR_EXIT
+    { repeat 1; do false && true; done } always { print INSIDE: $? }
+    print OUTSIDE
+  )
+1:ERR_EXIT from "repeat" with conditional body
+>INSIDE: 1
+
   (print before; setopt noexec; print after)
 0:NO_EXEC option
 >before

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] More ERR_EXIT (was Re: Tests RE behavior of ERR_EXIT)
  2022-11-15  1:11         ` Bart Schaefer
  2022-11-15  7:01           ` [PATCH] Even more ERR_EXIT (was Re: More ERR_EXIT " Bart Schaefer
@ 2022-11-15  7:26           ` Philippe Altherr
  2022-11-15 19:18             ` Philippe Altherr
  1 sibling, 1 reply; 26+ messages in thread
From: Philippe Altherr @ 2022-11-15  7:26 UTC (permalink / raw)
  To: Bart Schaefer; +Cc: zsh-workers

[-- Attachment #1: Type: text/plain, Size: 1749 bytes --]

I think that I found the problem. Function calls do save
<https://github.com/zsh-users/zsh/blob/b1533066ca7d50c88b37ce72093c12cf19807818/Src/exec.c#L5765>
and restore
<https://github.com/zsh-users/zsh/blob/b1533066ca7d50c88b37ce72093c12cf19807818/Src/exec.c#L6011>
noerrexit.
They should do the same for this_noerrexit. Furthermore exectry
<https://github.com/zsh-users/zsh/blob/b1533066ca7d50c88b37ce72093c12cf19807818/Src/loop.c#L732>
needs
a "this_noerrexit = 1;" at the very end like all the other "exec" functions
in loop.c. If I do these two changes and revert your two patches, then I
think everything works (my tests and the Zsh tests). I will try to confirm
that after some sleep.

 (
>    setopt ERR_EXIT
>    { { { false && true }
>         } always { print INSIDE: $? } }
>    print OUTSIDE
>  )


> The above should print "INSIDE: 1" (thus not exit) and then exit
> without printing OUTSIDE.


No, I don't think so. "{ ... }" and "{ ... } always { ... }" are both
compound commands that should never trigger an ERR_EXIT on a non-zero exit
status produced by a "false && true" that is coming from the inside (i.e.,
that is bubbling up). If you replace the "{ ... } always { ... }" with an
"if true; then { ... } else { ... }" then you can check with Bash that it
prints "OUTSIDE" (but obviously not "INSIDE").

However, the following should trigger an ERR_EXIT when "foo" returns. With
my changes, it does.

set -e
> function foo() {
>    { { { false && true } } always { print INSIDE: $? } }
> }
> foo
> echo OUTSIDE


Interestingly, this example also works in Zsh 5.8. I guess it's thanks to
the missing "this_noerrexit = 1;" in exectry. The version with an
"if/then/else" instead of the "always" only works with my changes.

Philippe

[-- Attachment #2: Type: text/html, Size: 2734 bytes --]

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] Even more ERR_EXIT (was Re: More ERR_EXIT Re: Tests RE behavior of ERR_EXIT)
  2022-11-15  7:01           ` [PATCH] Even more ERR_EXIT (was Re: More ERR_EXIT " Bart Schaefer
@ 2022-11-15  7:30             ` Philippe Altherr
  2022-11-15 19:50               ` Philippe Altherr
  0 siblings, 1 reply; 26+ messages in thread
From: Philippe Altherr @ 2022-11-15  7:30 UTC (permalink / raw)
  To: Bart Schaefer; +Cc: Zsh hackers list

[-- Attachment #1: Type: text/plain, Size: 1349 bytes --]

Oops, I see this only now, just after sending my other reply. I will have a
look later. I first need some sleep.

Philippe


On Tue, Nov 15, 2022 at 8:01 AM Bart Schaefer <schaefer@brasslantern.com>
wrote:

> On Mon, Nov 14, 2022 at 5:11 PM Bart Schaefer <schaefer@brasslantern.com>
> wrote:
> >
> > [if workers/50929 makes 50897 redundant then] there's no point in
> > bashing through if/case/for/while/repeat/select individually -- the
> > only case we have to fix is this one:
>
> I was half right.  Fixing that case revealed that it was necessary to
> go through the others, because the "redundant" fix is one level too
> far up the stack.
>
> Attached patch removes 50929 again, and amends this_noerrexit from
> 50897 in each of the complex command exec* functions.  Finally it adds
> tests to E01options for each of the cases that Philippe previously
> outlined, with the exception of "select" which is interactive.
>
> The tests from 50928 still pass with this patch.  I spent a ridiculous
> amount of time with every change I made flopping back and forth
> between C03 working and the new E01 failing and vice-versa, before
> noticing what happened when I nested a brace expression inside another
> one.
>
> Also a minor fix for TRAPDEBUG that caused me some confusion earlier
> in the process.
>
> Are there more cases that need testing?
>

[-- Attachment #2: Type: text/html, Size: 1856 bytes --]

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] More ERR_EXIT (was Re: Tests RE behavior of ERR_EXIT)
  2022-11-15  7:26           ` [PATCH] More ERR_EXIT (was " Philippe Altherr
@ 2022-11-15 19:18             ` Philippe Altherr
  2022-11-15 21:08               ` Bart Schaefer
                                 ` (2 more replies)
  0 siblings, 3 replies; 26+ messages in thread
From: Philippe Altherr @ 2022-11-15 19:18 UTC (permalink / raw)
  To: Bart Schaefer; +Cc: zsh-workers


[-- Attachment #1.1: Type: text/plain, Size: 1146 bytes --]

Here is a patch with which all tests pass (the Zsh ones, including the new
ones that the patch adds, and my tests mentioned in this thread). The
patch does the following:

- Revert change: 50929: fix handling of ERR_RETURN bent by 50928.
<https://sourceforge.net/p/zsh/code/ci/8839e969bf8f3f129d0efd8ecd51505610a1f01b/>
- Revert change: 50928: fix tests for 50897, mention behavior change in NEWS
<https://sourceforge.net/p/zsh/code/ci/1ba8714a7ac665e661c1b3a716ffe2af73d1e443/>
- Revert change: 50897: nonzero status of complex commands should trigger
ERR_EXIT
<https://sourceforge.net/p/zsh/code/ci/d873ed6026d7b0c48d6e65ec06df491d015a4d59/>
- Add saving and restoring of local_noerrexit in doshfunc in exec.c
- Add "this_noerrexit = 1;" at the very end of exectry in loop.c
- Fix test "ERR_RETURN in "else" branch in nested
function" in C03traps.ztst, to not affect tests coming afterwards
- Add new tests in C03traps.ztst

I'm not sure what to do in Changelog. Should I revert your changes or
instead leave them and comment that the patch reverts them? Also, I have no
clue how you obtain the change numbers (like 50929 and 50928).

Philippe

[-- Attachment #1.2: Type: text/html, Size: 1418 bytes --]

[-- Attachment #2: fix-err-exit.patch --]
[-- Type: application/octet-stream, Size: 7959 bytes --]

diff --git a/ChangeLog b/ChangeLog
index 6478f5480..e12b03330 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,19 +1,9 @@
 2022-11-09  Bart Schaefer  <schaefer@zsh.org>
 
-	* 50929: Src/exec.c: fix handling of ERR_RETURN bent by 50928.
-
-	* 50928: News, Src/exec.c, Test/C03traps.ztst: fix tests for 50897,
-	mention behavior change in NEWS
-
 	* 50922: Src/exec.c, Src/jobs.c: fix additional cases of signals
 	for current shell jobs on the right of a pipeline.  Backs out
 	part of 50874.
 
-2022-11-08  Bart Schaefer  <schaefer@zsh.org>
-
-	* 50897: Src/exec.c, Src/loop.c: nonzero status of complex
-	commands should trigger ERR_EXIT
-
 2022-11-08  Peter Stephenson  <p.stephenson@samsung.com>
 
 	* users/28338: Src/lex.c, Test/D08cmdsubst.ztst: edge case of an
diff --git a/NEWS b/NEWS
index 9c28169bb..cdafd1ff5 100644
--- a/NEWS
+++ b/NEWS
@@ -4,15 +4,6 @@ CHANGES FROM PREVIOUS VERSIONS OF ZSH
 
 Note also the list of incompatibilities in the README file.
 
-Changes since 5.9
------------------
-
-Handling of ERR_EXIT is corrected when the final status of a structured
-command (for, select, while, repeat, if, case, or a list in braces) is
-nonzero.  To be compatible with other shells, "zsh -e" now exits in
-those circumstances, whereas previous versions did not.  This does not
-affect the handling of nonzero status within conditional statements.
-
 Changes since 5.8.1
 -------------------
 
diff --git a/Src/exec.c b/Src/exec.c
index ce0c1f1ad..b9061e3a4 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -49,7 +49,8 @@ struct funcsave {
     int zoptind, lastval, optcind, numpipestats;
     int *pipestats;
     char *scriptname;
-    int breaks, contflag, loops, emulation, noerrexit, oflags, restore_sticky;
+    int breaks, contflag, loops, emulation, noerrexit, this_noerrexit, oflags;
+    int restore_sticky;
     Emulation_options sticky;
     struct funcstack fstack;
 };
@@ -451,7 +452,7 @@ execcursh(Estate state, int do_exec)
     cmdpop();
 
     state->pc = end;
-    this_noerrexit = (WC_SUBLIST_TYPE(*end) != WC_SUBLIST_END);
+    this_noerrexit = 1;
 
     return lastval;
 }
@@ -1442,8 +1443,6 @@ execlist(Estate state, int dont_change_job, int exiting)
 		    execsimple(state);
 		else
 		    execpline(state, code, ltype, (ltype & Z_END) && exiting);
-		if (!locallevel || unset(ERRRETURN))
-		    this_noerrexit = noerrexit;
 		state->pc = next;
 		goto sublist_done;
 		break;
@@ -5763,6 +5762,7 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int noreturnval)
 	funcsave->pipestats = NULL;
 	funcsave->numpipestats = numpipestats;
 	funcsave->noerrexit = noerrexit;
+	funcsave->this_noerrexit = this_noerrexit;
 	if (trap_state == TRAP_STATE_PRIMED)
 	    trap_return--;
 	/*
@@ -6009,6 +6009,7 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int noreturnval)
 	    trap_return++;
 	ret = lastval;
 	noerrexit = funcsave->noerrexit;
+	this_noerrexit = funcsave->this_noerrexit;
 	if (noreturnval) {
 	    lastval = funcsave->lastval;
 	    numpipestats = funcsave->numpipestats;
diff --git a/Src/loop.c b/Src/loop.c
index be5261369..7c3e04b8a 100644
--- a/Src/loop.c
+++ b/Src/loop.c
@@ -208,7 +208,7 @@ execfor(Estate state, int do_exec)
     loops--;
     simple_pline = old_simple_pline;
     state->pc = end;
-    this_noerrexit = (WC_SUBLIST_TYPE(*end) != WC_SUBLIST_END);
+    this_noerrexit = 1;
     return lastval;
 }
 
@@ -336,7 +336,7 @@ execselect(Estate state, UNUSED(int do_exec))
     loops--;
     simple_pline = old_simple_pline;
     state->pc = end;
-    this_noerrexit = (WC_SUBLIST_TYPE(*end) != WC_SUBLIST_END);
+    this_noerrexit = 1;
     return lastval;
 }
 
@@ -478,7 +478,7 @@ execwhile(Estate state, UNUSED(int do_exec))
     popheap();
     loops--;
     state->pc = end;
-    this_noerrexit = (WC_SUBLIST_TYPE(*end) != WC_SUBLIST_END);
+    this_noerrexit = 1;
     return lastval;
 }
 
@@ -532,7 +532,7 @@ execrepeat(Estate state, UNUSED(int do_exec))
     loops--;
     simple_pline = old_simple_pline;
     state->pc = end;
-    this_noerrexit = (WC_SUBLIST_TYPE(*end) != WC_SUBLIST_END);
+    this_noerrexit = 1;
     return lastval;
 }
 
@@ -587,7 +587,7 @@ execif(Estate state, int do_exec)
 	    lastval = 0;
     }
     state->pc = end;
-    this_noerrexit = (WC_SUBLIST_TYPE(*end) != WC_SUBLIST_END);
+    this_noerrexit = 1;
 
     return lastval;
 }
@@ -701,7 +701,7 @@ execcase(Estate state, int do_exec)
 
     if (!anypatok)
 	lastval = 0;
-    this_noerrexit = (WC_SUBLIST_TYPE(*end) != WC_SUBLIST_END);
+    this_noerrexit = 1;
 
     return lastval;
 }
@@ -793,6 +793,7 @@ exectry(Estate state, int do_exec)
     cmdpop();
     popheap();
     state->pc = end;
+    this_noerrexit = 1;
 
     return endval;
 }
diff --git a/Test/C03traps.ztst b/Test/C03traps.ztst
index 5cc45e2de..66d3f098a 100644
--- a/Test/C03traps.ztst
+++ b/Test/C03traps.ztst
@@ -713,29 +713,48 @@ F:Must be tested with a top-level script rather than source or function
     fi
   }
   fn() {
-    setopt localoptions err_return
+    setopt err_return
     fn2 || true
   }
-  fn
+  (fn)
 0:ERR_RETURN in "else" branch in nested function
 >Good
 
   (setopt err_exit
-  for x in y; do
+  false && true
+  print OK
+  )
+0:ERR_EXIT not triggered by "false && true"
+>OK
+
+  (setopt err_exit
+  fn() {
     false && true
-  done
+  }
+  fn
   print OK
   )
-1:ERR_EXIT triggered by status 1 at end of for
+1:ERR_EXIT not triggered by "false && true" but by return from fn
 
   (setopt err_exit
-  integer x=0
-  while (( ! x++ )); do
+  for x in y; do
     false && true
   done
   print OK
   )
-1:ERR_EXIT triggered by status 1 at end of while
+0:ERR_EXIT not triggered by status 1 at end of for
+>OK
+
+  (setopt err_exit
+  fn() {
+    for x in y; do
+      false && true
+    done
+  }
+  fn
+  print OK
+  )
+1:ERR_EXIT not triggered by status 1 at end of for but by return from fn
 
   (setopt err_exit
   repeat 1; do
@@ -743,7 +762,19 @@ F:Must be tested with a top-level script rather than source or function
   done
   print OK
   )
-1:ERR_EXIT triggered by status 1 at end of repeat
+0:ERR_EXIT not triggered by status 1 at end of repeat
+>OK
+
+  (setopt err_exit
+  fn() {
+    repeat 1; do
+      false && true
+    done
+  }
+  fn
+  print OK
+  )
+1:ERR_EXIT not triggered by status 1 at end of repeat but by return from fn
 
   (setopt err_exit
   if true; then
@@ -751,15 +782,93 @@ F:Must be tested with a top-level script rather than source or function
   fi
   print OK
   )
-1:ERR_EXIT triggered by status 1 at end of if
+0:ERR_EXIT not triggered by status 1 at end of if
+>OK
+
+  (setopt err_exit
+  fn() {
+    if true; then
+      false && true
+    fi
+  }
+  fn
+  print OK
+  )
+1:ERR_EXIT not triggered by status 1 at end of if but by return from fn
+
+  (setopt err_exit
+  loop=true
+  while print COND; $loop; do
+    loop=false
+    false && true
+  done
+  print OK
+  )
+0:ERR_EXIT not triggered by status 1 at end of while
+>COND
+>COND
+>OK
+
+  (setopt err_exit
+  fn() {
+    loop=true
+    while print COND; $loop; do
+      loop=false
+      false && true
+    done
+  }
+  fn
+  print OK
+  )
+1:ERR_EXIT not triggered by status 1 at end of while but by return from fn
+>COND
+>COND
 
   (setopt err_exit
   {
     false && true
+  } always {
+    print ALWAYS
   }
   print OK
   )
-1:ERR_EXIT triggered by status 1 at end of { }
+0:ERR_EXIT not triggered by status 1 at end of always
+>ALWAYS
+>OK
+
+  (setopt err_exit
+  fn() {
+    {
+      false && true
+    } always {
+      print ALWAYS
+    }
+  }
+  fn
+  print OK
+  )
+1:ERR_EXIT not triggered by status 1 at end of always but by return from fn
+>ALWAYS
+
+  (setopt err_exit
+  {
+    false && true
+  }
+  print OK
+  )
+0:ERR_EXIT not triggered by status 1 at end of { }
+>OK
+
+  (setopt err_exit
+  fn() {
+    {
+      false && true
+    }
+  }
+  fn
+  print OK
+  )
+1:ERR_EXIT not triggered by status 1 at end of { } but by return from fn
 
   unsetopt err_exit err_return
   (setopt err_exit

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] Even more ERR_EXIT (was Re: More ERR_EXIT Re: Tests RE behavior of ERR_EXIT)
  2022-11-15  7:30             ` Philippe Altherr
@ 2022-11-15 19:50               ` Philippe Altherr
  0 siblings, 0 replies; 26+ messages in thread
From: Philippe Altherr @ 2022-11-15 19:50 UTC (permalink / raw)
  To: Bart Schaefer; +Cc: Zsh hackers list

[-- Attachment #1: Type: text/plain, Size: 2321 bytes --]

As commented in the other thread, code like the following should NOT
trigger an ERR_EXIT (because of exception 3 in the POSIX specification of
"set -e"
<https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#set>
).

setopt err_exit
> if true; then
>   false && true
> fi
> print OK


The non-zero status of the "if" statement only triggers an ERR_EXIT if it
becomes the result of a function call. Then the return of the function
triggers the ERR_EXIT, like in the following code:

setopt err_exit
> fn() {
>   if true; then
>     false && true
>   fi
> }
> fn
> print OK


The test exit status changes in 50928: fix tests for 50897, mention
behavior change in NEWS
<https://sourceforge.net/p/zsh/code/ci/1ba8714a7ac665e661c1b3a716ffe2af73d1e443/>
are
incorrect and several tests in the new patch are incorrect.

Philippe


On Tue, Nov 15, 2022 at 8:30 AM Philippe Altherr <philippe.altherr@gmail.com>
wrote:

> Oops, I see this only now, just after sending my other reply. I will have
> a look later. I first need some sleep.
>
> Philippe
>
>
> On Tue, Nov 15, 2022 at 8:01 AM Bart Schaefer <schaefer@brasslantern.com>
> wrote:
>
>> On Mon, Nov 14, 2022 at 5:11 PM Bart Schaefer <schaefer@brasslantern.com>
>> wrote:
>> >
>> > [if workers/50929 makes 50897 redundant then] there's no point in
>> > bashing through if/case/for/while/repeat/select individually -- the
>> > only case we have to fix is this one:
>>
>> I was half right.  Fixing that case revealed that it was necessary to
>> go through the others, because the "redundant" fix is one level too
>> far up the stack.
>>
>> Attached patch removes 50929 again, and amends this_noerrexit from
>> 50897 in each of the complex command exec* functions.  Finally it adds
>> tests to E01options for each of the cases that Philippe previously
>> outlined, with the exception of "select" which is interactive.
>>
>> The tests from 50928 still pass with this patch.  I spent a ridiculous
>> amount of time with every change I made flopping back and forth
>> between C03 working and the new E01 failing and vice-versa, before
>> noticing what happened when I nested a brace expression inside another
>> one.
>>
>> Also a minor fix for TRAPDEBUG that caused me some confusion earlier
>> in the process.
>>
>> Are there more cases that need testing?
>>
>

[-- Attachment #2: Type: text/html, Size: 3555 bytes --]

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] More ERR_EXIT (was Re: Tests RE behavior of ERR_EXIT)
  2022-11-15 19:18             ` Philippe Altherr
@ 2022-11-15 21:08               ` Bart Schaefer
  2022-11-16  2:41               ` Lawrence Velázquez
  2022-11-16  5:51               ` Bart Schaefer
  2 siblings, 0 replies; 26+ messages in thread
From: Bart Schaefer @ 2022-11-15 21:08 UTC (permalink / raw)
  To: Philippe Altherr; +Cc: zsh-workers

On Tue, Nov 15, 2022 at 11:19 AM Philippe Altherr
<philippe.altherr@gmail.com> wrote:
>
> I'm not sure what to do in Changelog.

Unless you have permission to push to the sourceforge git repository,
you should not edit ChangeLog at all.


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] More ERR_EXIT (was Re: Tests RE behavior of ERR_EXIT)
  2022-11-15 19:18             ` Philippe Altherr
  2022-11-15 21:08               ` Bart Schaefer
@ 2022-11-16  2:41               ` Lawrence Velázquez
  2022-11-16  6:31                 ` Philippe Altherr
  2022-11-16  5:51               ` Bart Schaefer
  2 siblings, 1 reply; 26+ messages in thread
From: Lawrence Velázquez @ 2022-11-16  2:41 UTC (permalink / raw)
  To: Philippe Altherr, Bart Schaefer; +Cc: zsh-workers

On Tue, Nov 15, 2022, at 2:18 PM, Philippe Altherr wrote:
> Here is a patch with which all tests pass (the Zsh ones, including the 
> new ones that the patch adds, and my tests mentioned in this thread). 

Here are two additional cases that your patch does not cover.  They
are not regressions, as stable zsh 5.9 is also affected.

As you've mentioned several times, POSIX states:

	If the exit status of a compound command other than a
	subshell command was the result of a failure while -e was
	being ignored, then -e shall not apply to this command.

This implies that ignored non-zero exit statuses may "bubble up"
until reaching a (...) command or a simple command.  You've already
mentioned function calls, but *any* simple command that derives its
exit status from "the last command executed" should be able to
trigger early exit.  At a minimum, this includes the dot/source and
eval commands, neither of which currently behaves correctly.

-----

	% cat /tmp/dotting.sh; echo
	. /tmp/dotted.sh
	echo done

	% cat /tmp/dotted.sh; echo
	if true; then
	    false && true
	fi

	% bash -e /tmp/dotting.sh; echo $?
	1
	% ksh -e /tmp/dotting.sh; echo $?
	1
	% dash -e /tmp/dotting.sh; echo $?
	1
	% yash -e /tmp/dotting.sh; echo $?
	1
	% zsh -e /tmp/dotting.sh; echo $?
	done
	0
	% Src/zsh -e /tmp/dotting.sh; echo $?
	done
	0

-----

	% cat /tmp/eval.sh; echo
	eval 'if true; then false && true; fi'
	echo done

	% bash -e /tmp/eval.sh; echo $?
	1
	% ksh -e /tmp/eval.sh; echo $?
	1
	% dash -e /tmp/eval.sh; echo $?
	1
	% yash -e /tmp/eval.sh; echo $?
	1
	% zsh -e /tmp/eval.sh; echo $?
	done
	0
	% Src/zsh -e /tmp/eval.sh; echo $?
	done
	0

-- 
vq


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] More ERR_EXIT (was Re: Tests RE behavior of ERR_EXIT)
  2022-11-15 19:18             ` Philippe Altherr
  2022-11-15 21:08               ` Bart Schaefer
  2022-11-16  2:41               ` Lawrence Velázquez
@ 2022-11-16  5:51               ` Bart Schaefer
  2022-11-16  7:56                 ` Philippe Altherr
  2 siblings, 1 reply; 26+ messages in thread
From: Bart Schaefer @ 2022-11-16  5:51 UTC (permalink / raw)
  To: Philippe Altherr; +Cc: zsh-workers

On Tue, Nov 15, 2022 at 11:19 AM Philippe Altherr
<philippe.altherr@gmail.com> wrote:
>
> Here is a patch with which all tests pass (the Zsh ones, including the new ones that the patch adds, and my tests mentioned in this thread).

Thanks.  Please don't name attached files with suffix ".patch",
".diff", ".zsh", etc., as some mail readers won't display them and
they won't be inlined by the zsh-workers list archive server.  Just
use ".txt" unless there is something in the attachment that would be
damaged if interpreted that way (hopefully that won't ever be true of
a patch).

> +++ b/Test/C03traps.ztst
> @@ -713,29 +713,48 @@ F:Must be tested with a top-level script rather than source or function
>      fi
>    }
>    fn() {
> -    setopt localoptions err_return
> +    setopt err_return
>      fn2 || true
>    }
> -  fn
> +  (fn)

Did using localoptions here break something?

Aside from that, I'd like to look at how we got here.  Way back in
workers/50873 (https://www.zsh.org/mla/workers/2022/msg01203.html) you
posted an attachment "bug.zsh" and wrote:

> The output of the script (with Zsh 5.8 on Linux and Zsh 5.8.1 on macOS) is the following:
>
>> aaa: 0
>> bbb: 1
>> ccc: 1
>
> The documentation of ERR_EXIT mentions the special case of conditional expressions. I understand that the command "false" by itself in the expression "false && true" shouldn't trigger an exit since it appears in a position where a condition is expected. However, since the expression "false && true" as a whole evaluates to a non-zero status and doesn't appear in a position where a condition is expected, I would assume that it should trigger an exit. Thus, in my opinion the script should have the following output:
>
>> aaa: 0
>
> That's obviously not the case.

I started making all my changes on the premise that "bbb: 1" was
incorrect output.  After fix-err-exit.patch is applied, the bug.zsh
script outputs

aaa: 0
bbb: 1

So my question is, was the whole premise of the thread that started
there, incorrect?  Because we come to workers/50897 (which
fix-err-exit.patch reverses) where I made this comment:

> There is the question of why ignoring a false status at the end of a
> complex command has so far been considered correct for ERR_EXIT,
> according to C03.  This is a disagreement with e.g. bash.

Your current assertion is that those tests were correct all along,
because you've reverted them back to the 5.8.1/5.9 status, so ...
they're not a disagreement with bash?

Somewhere along the line we pivoted from "errexit isn't exiting in
case X" to "errexit shouldn't exit in case Y" and (given also
Lawrence's remarks RE source and eval) I'm not sure we've yet resolved
that any of the approaches to Y are satisfactory for X.

> I have no clue how you obtain the change numbers (like 50929 and 50928)

Those are zsh-workers article numbers from the X-Seq header in each
message in the archives.


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] More ERR_EXIT (was Re: Tests RE behavior of ERR_EXIT)
  2022-11-16  2:41               ` Lawrence Velázquez
@ 2022-11-16  6:31                 ` Philippe Altherr
  0 siblings, 0 replies; 26+ messages in thread
From: Philippe Altherr @ 2022-11-16  6:31 UTC (permalink / raw)
  To: Lawrence Velázquez; +Cc: Bart Schaefer, zsh-workers

[-- Attachment #1: Type: text/plain, Size: 2885 bytes --]

>
> Here are two additional cases that your patch does not cover.  They
> are not regressions, as stable zsh 5.9 is also affected.


Interesting! And the symptoms are exactly the same as they were with
function calls:

- If you replace "echo done" with "echo done $?" you can see that the
"source" and "eval" statements return the right exit status. They "just"
fail to trigger ERR_EXIT.
- When you drop the "if" statement and only keep "false && true", then
ERR_EXIT is correctly triggered.

It looks like another case where saving and restoring this_noerrexit is
missing. And, indeed, if save and restore this_noerrexit are added just
before and after this execlist
<https://github.com/zsh-users/zsh/blob/b1533066ca7d50c88b37ce72093c12cf19807818/Src/exec.c#L1221>
in execode, then your examples work and all tests still pass. The function
execode
<https://github.com/zsh-users/zsh/blob/b1533066ca7d50c88b37ce72093c12cf19807818/Src/exec.c#L1193>
is
called from builtin.c in the eval
<https://github.com/zsh-users/zsh/blob/b1533066ca7d50c88b37ce72093c12cf19807818/Src/builtin.c#L6067>
function. In that context the saving and restoring looks warranted. But
there are multiple other calls, mainly in exec.c. Although for most it
looks right to save and restore, I can't say confidently that it's indeed
the case for all of them. I also wonder whether other elements of the
execution stack should be saved and restored. For function calls, there are
many more things that are saved and restored.

I also noticed that zsh.h declares a structure execstack
<https://github.com/zsh-users/zsh/blob/b1533066ca7d50c88b37ce72093c12cf19807818/Src/zsh.h#L1132>,
which is used in exec.c to implement execsave
<https://github.com/zsh-users/zsh/blob/b1533066ca7d50c88b37ce72093c12cf19807818/Src/exec.c#L6325>,
which saves the state of the execution stack, and execrestore
<https://github.com/zsh-users/zsh/blob/b1533066ca7d50c88b37ce72093c12cf19807818/Src/exec.c#L6357>,
which restores it. Surprisingly these two methods are only used once in
signals.c (here
<https://github.com/zsh-users/zsh/blob/b1533066ca7d50c88b37ce72093c12cf19807818/Src/signals.c#L1339>
and. here
<https://github.com/zsh-users/zsh/blob/b1533066ca7d50c88b37ce72093c12cf19807818/Src/signals.c#L1391>).
And even more surprisingly, even though plenty of variables are saved,
including this_noerrexit, the variable noerrexit is NOT saved, which looks
very suspicious to me.

How should we proceed? I'm 99% sure that my current patch (without the
changes mentioned above) is correct (i.e., it fixes a number of problems
and doesn't introduce new ones). At the moment, I'm much less confident
about changing execode as described above or about adding noerrexit
to execstack. Would it make sense to submit my pacth (once I have fixed
Changlelog and 1-2 other things) and try to solve the other problems in
follow-up patches?

Philippe

[-- Attachment #2: Type: text/html, Size: 3429 bytes --]

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] More ERR_EXIT (was Re: Tests RE behavior of ERR_EXIT)
  2022-11-16  5:51               ` Bart Schaefer
@ 2022-11-16  7:56                 ` Philippe Altherr
  2022-11-16 14:21                   ` Philippe Altherr
  0 siblings, 1 reply; 26+ messages in thread
From: Philippe Altherr @ 2022-11-16  7:56 UTC (permalink / raw)
  To: Bart Schaefer; +Cc: zsh-workers

[-- Attachment #1: Type: text/plain, Size: 3276 bytes --]

>
> [...] Just use ".txt" [...]


Yep, I noticed that that's what you did and that this was rendered much
better on www.zsh.org/. I will do that.

Did using localoptions here break something?


I was just about to comment on that. This was automatically reverted when I
reverted your 3 patches. And it took me a while to figure out why my new
tests in C03traps were failing. It's only after sending my patch that I
realized that you too had to fix that issue. And that you did it in a nicer
way. I will change my patch to use your fix.

I started making all my changes on the premise that "bbb: 1" was
> incorrect output.


Ah, now I understand where the misunderstanding is coming from :-(

After fix-err-exit.patch is applied, the bug.zsh
> script outputs


> aaa: 0
> bbb: 1


Which is what POSIX mandates and also what Bash does.


> So my question is, was the whole premise of the thread that started
> there, incorrect?


Yes and no :-( In my original post, I noticed two things

A) "false && true" at the top-level doesn't trigger an ERR_EXIT even though
it ends in an error and isn't part of a condition.
B) the call to fun1 doesn't trigger and ERR_EXIT while the call to fun2
does.

At that time I didn't know about POSIX, nor about its exception 3. I also
didn't know what Bash was doing (I hadn't thought about comparing Zsh with
Bash). It was quite clear to me that B was a bug (because in what world
should fun1 and fun2 behave differently?!?). So it was clear that there
should be no "ccc: 1" output. I was much less sure about A. Clearly it
wasn't doing what I wanted but it was much less clear whether it could be
qualified as a bug. And it turns out that POSIX mandates that there is NO
ERR_EXIT. So "bbb: 1" is expected.

Your current assertion is that those tests were correct all along,
> because you've reverted them back to the 5.8.1/5.9 status, so ...
> they're not a disagreement with bash?


Yes, these tests are in agreement with Bash (and POSIX). It's only when you
modify these tests to move the "false && true" and the surrounding
statement into a function that you can observe a disagreement between Zsh
5.8 and Bash.

Somewhere along the line we pivoted from "errexit isn't exiting in
> case X" to "errexit shouldn't exit in case Y"


Did we? All the bugs were always about cases where Zsh did NOT exit while
it should.

In the original thread, I mentioned that I would like to have an option
such that Zsh DOES errexit in a few cases where it currently doesn't, like
for example after the top-level "false && true" or after "false" in "if
false; true; then true fi". You and Lawrence opposed that. That's the only
instance of "errexit shouldn't exit in case Y" that I can remember of. Were
there others?

(given also Lawrence's remarks RE source and eval)


That's again instances of "errexit isn't exiting in case X".

I'm not sure we've yet resolved
> that any of the approaches to Y are satisfactory for X.


If by this you refer to your and Lawrence's opposition to some of the
changes that I wished, then the answer is no. Neither my patch nor the
patching of the newly discovered bugs (source and eval) will give me what I
wish. I will have to come back to that later and try to better explain what
I would like to get and why.

Philippe

[-- Attachment #2: Type: text/html, Size: 5372 bytes --]

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] More ERR_EXIT (was Re: Tests RE behavior of ERR_EXIT)
  2022-11-16  7:56                 ` Philippe Altherr
@ 2022-11-16 14:21                   ` Philippe Altherr
  0 siblings, 0 replies; 26+ messages in thread
From: Philippe Altherr @ 2022-11-16 14:21 UTC (permalink / raw)
  To: Bart Schaefer; +Cc: zsh-workers

[-- Attachment #1: Type: text/plain, Size: 4021 bytes --]

>
> [...] That's the only instance of "errexit shouldn't exit in case Y" that
> I can remember of. Were there others?


Oh and then there was the thread about your patch where I noticed that the
ERR_EXIT was triggered too early. There I commented that in a number of
cases there should be no ERR_EXIT. Unfortunately, I didn't realize that you
had misunderstood the expected behavior. It's only later, when I prepared
my patch, that I realized that you had changed the expected exit status of
some tests.

Philippe


On Wed, Nov 16, 2022 at 8:56 AM Philippe Altherr <philippe.altherr@gmail.com>
wrote:

> [...] Just use ".txt" [...]
>
>
> Yep, I noticed that that's what you did and that this was rendered much
> better on www.zsh.org/. I will do that.
>
> Did using localoptions here break something?
>
>
> I was just about to comment on that. This was automatically reverted when
> I reverted your 3 patches. And it took me a while to figure out why my new
> tests in C03traps were failing. It's only after sending my patch that I
> realized that you too had to fix that issue. And that you did it in a nicer
> way. I will change my patch to use your fix.
>
> I started making all my changes on the premise that "bbb: 1" was
>> incorrect output.
>
>
> Ah, now I understand where the misunderstanding is coming from :-(
>
> After fix-err-exit.patch is applied, the bug.zsh
>> script outputs
>
>
>> aaa: 0
>> bbb: 1
>
>
> Which is what POSIX mandates and also what Bash does.
>
>
>> So my question is, was the whole premise of the thread that started
>> there, incorrect?
>
>
> Yes and no :-( In my original post, I noticed two things
>
> A) "false && true" at the top-level doesn't trigger an ERR_EXIT even
> though it ends in an error and isn't part of a condition.
> B) the call to fun1 doesn't trigger and ERR_EXIT while the call to fun2
> does.
>
> At that time I didn't know about POSIX, nor about its exception 3. I also
> didn't know what Bash was doing (I hadn't thought about comparing Zsh with
> Bash). It was quite clear to me that B was a bug (because in what world
> should fun1 and fun2 behave differently?!?). So it was clear that there
> should be no "ccc: 1" output. I was much less sure about A. Clearly it
> wasn't doing what I wanted but it was much less clear whether it could be
> qualified as a bug. And it turns out that POSIX mandates that there is NO
> ERR_EXIT. So "bbb: 1" is expected.
>
> Your current assertion is that those tests were correct all along,
>> because you've reverted them back to the 5.8.1/5.9 status, so ...
>> they're not a disagreement with bash?
>
>
> Yes, these tests are in agreement with Bash (and POSIX). It's only when
> you modify these tests to move the "false && true" and the surrounding
> statement into a function that you can observe a disagreement between Zsh
> 5.8 and Bash.
>
> Somewhere along the line we pivoted from "errexit isn't exiting in
>> case X" to "errexit shouldn't exit in case Y"
>
>
> Did we? All the bugs were always about cases where Zsh did NOT exit while
> it should.
>
> In the original thread, I mentioned that I would like to have an option
> such that Zsh DOES errexit in a few cases where it currently doesn't, like
> for example after the top-level "false && true" or after "false" in "if
> false; true; then true fi". You and Lawrence opposed that. That's the only
> instance of "errexit shouldn't exit in case Y" that I can remember of. Were
> there others?
>
> (given also Lawrence's remarks RE source and eval)
>
>
> That's again instances of "errexit isn't exiting in case X".
>
> I'm not sure we've yet resolved
>> that any of the approaches to Y are satisfactory for X.
>
>
> If by this you refer to your and Lawrence's opposition to some of the
> changes that I wished, then the answer is no. Neither my patch nor the
> patching of the newly discovered bugs (source and eval) will give me what I
> wish. I will have to come back to that later and try to better explain what
> I would like to get and why.
>
> Philippe
>
>
>

[-- Attachment #2: Type: text/html, Size: 6492 bytes --]

^ permalink raw reply	[flat|nested] 26+ messages in thread

end of thread, other threads:[~2022-11-16 14:22 UTC | newest]

Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-12 22:16 [PATCH] More ERR_EXIT (was Re: Tests RE behavior of ERR_EXIT) Philippe Altherr
2022-11-13  3:59 ` Philippe Altherr
2022-11-13  4:11   ` Bart Schaefer
2022-11-13 13:55     ` Philippe Altherr
2022-11-13 14:24       ` Philippe Altherr
2022-11-13 15:45         ` Philippe Altherr
2022-11-13 16:52           ` Bart Schaefer
2022-11-13 16:45       ` Bart Schaefer
2022-11-13 16:53         ` Bart Schaefer
2022-11-13 18:37           ` Philippe Altherr
2022-11-13 20:55             ` Philippe Altherr
2022-11-13 22:27               ` Bart Schaefer
2022-11-13 23:10               ` Lawrence Velázquez
2022-11-13 22:12             ` Bart Schaefer
2022-11-15  1:11         ` Bart Schaefer
2022-11-15  7:01           ` [PATCH] Even more ERR_EXIT (was Re: More ERR_EXIT " Bart Schaefer
2022-11-15  7:30             ` Philippe Altherr
2022-11-15 19:50               ` Philippe Altherr
2022-11-15  7:26           ` [PATCH] More ERR_EXIT (was " Philippe Altherr
2022-11-15 19:18             ` Philippe Altherr
2022-11-15 21:08               ` Bart Schaefer
2022-11-16  2:41               ` Lawrence Velázquez
2022-11-16  6:31                 ` Philippe Altherr
2022-11-16  5:51               ` Bart Schaefer
2022-11-16  7:56                 ` Philippe Altherr
2022-11-16 14:21                   ` Philippe Altherr

Code repositories for project(s) associated with this public inbox

	https://git.vuxu.org/mirror/zsh/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).