* 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ messages in thread
* Tests RE behavior of ERR_EXIT @ 2022-11-09 5:29 Bart Schaefer 2022-11-10 5:22 ` [PATCH] More ERR_EXIT (was Re: Tests RE behavior of ERR_EXIT) Bart Schaefer 0 siblings, 1 reply; 28+ messages in thread From: Bart Schaefer @ 2022-11-09 5:29 UTC (permalink / raw) To: Zsh hackers list [-- Attachment #1: Type: text/plain, Size: 318 bytes --] Changed a set of tests from success to xfail. Anyone have ideas of why these aren't working? If I change the ( subshell ) constructs to use $ZTST_testdir/../Src/zsh -fc then they all correctly exit on nonzero status as expected. Aside, should 'setopt err_return' in the nested function test be using localoptions ? [-- Attachment #2: errexit_xfail.txt --] [-- Type: text/plain, Size: 1901 bytes --] diff --git a/Test/C03traps.ztst b/Test/C03traps.ztst index f120809a7..e3d9ea871 100644 --- a/Test/C03traps.ztst +++ b/Test/C03traps.ztst @@ -720,14 +720,15 @@ F:Must be tested with a top-level script rather than source or function 0:ERR_RETURN in "else" branch in nested function >Good + unsetopt err_return # "leaked" from previous test (setopt err_exit for x in y; do false && true done print OK ) -0:ERR_EXIT not triggered by status 1 at end of for ->OK +1f:ERR_EXIT triggered by status 1 at end of for +F:This fails to exit for unknown reasons and prints OK (setopt err_exit integer x=0 @@ -736,8 +737,8 @@ F:Must be tested with a top-level script rather than source or function done print OK ) -0:ERR_EXIT not triggered by status 1 at end of while ->OK +1f:ERR_EXIT triggered by status 1 at end of while +F:This fails to exit for unknown reasons and prints OK (setopt err_exit repeat 1; do @@ -745,8 +746,8 @@ F:Must be tested with a top-level script rather than source or function done print OK ) -0:ERR_EXIT not triggered by status 1 at end of repeat ->OK +1f:ERR_EXIT triggered by status 1 at end of repeat +F:This fails to exit for unknown reasons and prints OK (setopt err_exit if true; then @@ -754,8 +755,8 @@ F:Must be tested with a top-level script rather than source or function fi print OK ) -0:ERR_EXIT not triggered by status 1 at end of if ->OK +1f:ERR_EXIT triggered by status 1 at end of if +F:This fails to exit for unknown reasons and prints OK (setopt err_exit { @@ -763,8 +764,8 @@ F:Must be tested with a top-level script rather than source or function } print OK ) -0:ERR_EXIT not triggered by status 1 at end of { } ->OK +1f:ERR_EXIT triggered by status 1 at end of { } +F:This fails to exit for unknown reasons and prints OK unsetopt err_exit err_return (setopt err_exit ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH] More ERR_EXIT (was Re: Tests RE behavior of ERR_EXIT) 2022-11-09 5:29 Tests RE behavior of ERR_EXIT Bart Schaefer @ 2022-11-10 5:22 ` Bart Schaefer 2022-11-10 5:47 ` Bart Schaefer 0 siblings, 1 reply; 28+ messages in thread From: Bart Schaefer @ 2022-11-10 5:22 UTC (permalink / raw) To: Zsh hackers list [-- Attachment #1: Type: text/plain, Size: 375 bytes --] On Tue, Nov 8, 2022 at 9:29 PM Bart Schaefer <schaefer@brasslantern.com> wrote: > > Changed a set of tests from success to xfail. Anyone have ideas of > why these aren't working? Found it. Grateful for the thorough regression tests in E01options, too. > Aside, should 'setopt err_return' in the nested function test be using > localoptions ? I'm going to vote "yes" ... [-- Attachment #2: errexit_testspass.txt --] [-- Type: text/plain, Size: 2687 bytes --] diff --git a/NEWS b/NEWS index cdafd1ff5..9c28169bb 100644 --- a/NEWS +++ b/NEWS @@ -4,6 +4,15 @@ 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 d4e681887..eef40232e 100644 --- a/Src/exec.c +++ b/Src/exec.c @@ -1442,6 +1442,8 @@ execlist(Estate state, int dont_change_job, int exiting) execsimple(state); else execpline(state, code, ltype, (ltype & Z_END) && exiting); + if (unset(ERRRETURN)) + this_noerrexit = noerrexit; state->pc = next; goto sublist_done; break; diff --git a/Test/C03traps.ztst b/Test/C03traps.ztst index f120809a7..5cc45e2de 100644 --- a/Test/C03traps.ztst +++ b/Test/C03traps.ztst @@ -713,7 +713,7 @@ F:Must be tested with a top-level script rather than source or function fi } fn() { - setopt err_return + setopt localoptions err_return fn2 || true } fn @@ -726,8 +726,7 @@ F:Must be tested with a top-level script rather than source or function done print OK ) -0:ERR_EXIT not triggered by status 1 at end of for ->OK +1:ERR_EXIT triggered by status 1 at end of for (setopt err_exit integer x=0 @@ -736,8 +735,7 @@ F:Must be tested with a top-level script rather than source or function done print OK ) -0:ERR_EXIT not triggered by status 1 at end of while ->OK +1:ERR_EXIT triggered by status 1 at end of while (setopt err_exit repeat 1; do @@ -745,8 +743,7 @@ F:Must be tested with a top-level script rather than source or function done print OK ) -0:ERR_EXIT not triggered by status 1 at end of repeat ->OK +1:ERR_EXIT triggered by status 1 at end of repeat (setopt err_exit if true; then @@ -754,8 +751,7 @@ F:Must be tested with a top-level script rather than source or function fi print OK ) -0:ERR_EXIT not triggered by status 1 at end of if ->OK +1:ERR_EXIT triggered by status 1 at end of if (setopt err_exit { @@ -763,8 +759,7 @@ F:Must be tested with a top-level script rather than source or function } print OK ) -0:ERR_EXIT not triggered by status 1 at end of { } ->OK +1:ERR_EXIT triggered by status 1 at end of { } unsetopt err_exit err_return (setopt err_exit ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] More ERR_EXIT (was Re: Tests RE behavior of ERR_EXIT) 2022-11-10 5:22 ` [PATCH] More ERR_EXIT (was Re: Tests RE behavior of ERR_EXIT) Bart Schaefer @ 2022-11-10 5:47 ` Bart Schaefer 0 siblings, 0 replies; 28+ messages in thread From: Bart Schaefer @ 2022-11-10 5:47 UTC (permalink / raw) To: Zsh hackers list On Wed, Nov 9, 2022 at 9:22 PM Bart Schaefer <schaefer@brasslantern.com> wrote: > > Grateful for the thorough regression tests in E01options, too. Of course I still missed one detail ... ERR_RETURN only applies inside functions. diff --git a/Src/exec.c b/Src/exec.c index eef40232e..ce0c1f1ad 100644 --- a/Src/exec.c +++ b/Src/exec.c @@ -1442,7 +1442,7 @@ execlist(Estate state, int dont_change_job, int exiting) execsimple(state); else execpline(state, code, ltype, (ltype & Z_END) && exiting); - if (unset(ERRRETURN)) + if (!locallevel || unset(ERRRETURN)) this_noerrexit = noerrexit; state->pc = next; goto sublist_done; ^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2022-11-16 14:22 UTC | newest] Thread overview: 28+ 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 -- strict thread matches above, loose matches on Subject: below -- 2022-11-09 5:29 Tests RE behavior of ERR_EXIT Bart Schaefer 2022-11-10 5:22 ` [PATCH] More ERR_EXIT (was Re: Tests RE behavior of ERR_EXIT) Bart Schaefer 2022-11-10 5:47 ` Bart Schaefer
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).