Zsh Mailing List Archive
Messages sorted by:
Reverse Date,
Date,
Thread,
Author
Re: [PATCH] Fix ERR_EXIT behavior in function calls and "always" statements
- X-seq: zsh-workers 51001
- From: Philippe Altherr <philippe.altherr@xxxxxxxxx>
- To: Zsh hackers list <zsh-workers@xxxxxxx>
- Cc: Bart Schaefer <schaefer@xxxxxxxxxxxxxxxx>, Lawrence Velázquez <larryv@xxxxxxx>
- Subject: Re: [PATCH] Fix ERR_EXIT behavior in function calls and "always" statements
- Date: Sat, 19 Nov 2022 14:39:29 +0100
- Archived-at: <https://zsh.org/workers/51001>
- In-reply-to: <CAGdYchvfLWmxrCZ3-8ThXdyeuMAUWbV=SqFrXCjSJ8AnWpek9A@mail.gmail.com>
- List-id: <zsh-workers.zsh.org>
- References: <CAGdYchvfLWmxrCZ3-8ThXdyeuMAUWbV=SqFrXCjSJ8AnWpek9A@mail.gmail.com>
I have found that there is an error in the implementation of the negation statement. Zsh currently fails to print "done" for the following code:
set -e
fn() { true; }
! fn
echo done $?
Fixing the negation statement allows a slightly simpler fix for function calls. Instead of my original patch, I recommend submitting the 4 patches attached here.
Philippe
The attached patch fixes the ERR_EXIT behavior in function calls and "always" statements. The patch does the following:
- Revert the following patches, which are based on an unfortunate misunderstanding of the expected behavior of ERR_EXIT:
- Add saving and restoring of local_noerrexit in doshfunc in exec.c
- This fixes the ERR_EXIT behavior in function calls.
- Add "this_noerrexit = 1;" at the very end of exectry in loop.c
- This makes "always" statements compliant with the exception 3 of the POSIX specification of "set -e".
- Add new tests in C03traps.ztst
Philippe
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..b0f42ae67 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -451,7 +451,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 +1442,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;
diff --git a/Src/loop.c b/Src/loop.c
index be5261369..db5b3e097 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;
}
diff --git a/Test/C03traps.ztst b/Test/C03traps.ztst
index 5cc45e2de..a7a040d70 100644
--- a/Test/C03traps.ztst
+++ b/Test/C03traps.ztst
@@ -726,7 +726,8 @@ 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 for
+0:ERR_EXIT not triggered by status 1 at end of for
+>OK
(setopt err_exit
integer x=0
@@ -735,7 +736,8 @@ 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 while
+0:ERR_EXIT not triggered by status 1 at end of while
+>OK
(setopt err_exit
repeat 1; do
@@ -743,7 +745,8 @@ 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
if true; then
@@ -751,7 +754,8 @@ 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
{
@@ -759,7 +763,8 @@ F:Must be tested with a top-level script rather than source or function
}
print OK
)
-1:ERR_EXIT triggered by status 1 at end of { }
+0:ERR_EXIT not triggered by status 1 at end of { }
+>OK
unsetopt err_exit err_return
(setopt err_exit
diff --git a/Src/loop.c b/Src/loop.c
index db5b3e097..7c3e04b8a 100644
--- a/Src/loop.c
+++ b/Src/loop.c
@@ -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 a7a040d70..4719dfd57 100644
--- a/Test/C03traps.ztst
+++ b/Test/C03traps.ztst
@@ -721,22 +721,19 @@ F:Must be tested with a top-level script rather than source or function
>Good
(setopt err_exit
- for x in y; do
- false && true
- done
+ false && true
print OK
)
-0:ERR_EXIT not triggered by status 1 at end of for
+0:ERR_EXIT not triggered by "false && true"
>OK
(setopt err_exit
- integer x=0
- while (( ! x++ )); do
+ for x in y; do
false && true
done
print OK
)
-0:ERR_EXIT not triggered by status 1 at end of while
+0:ERR_EXIT not triggered by status 1 at end of for
>OK
(setopt err_exit
@@ -755,6 +752,31 @@ 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 if
+>OK
+
+ (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
+ {
+ false && true
+ } always {
+ print ALWAYS
+ }
+ print OK
+ )
+0:ERR_EXIT not triggered by status 1 at end of always
+>ALWAYS
>OK
(setopt err_exit
diff --git a/Src/exec.c b/Src/exec.c
index b0f42ae67..d8501ca68 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -63,7 +63,10 @@ typedef struct funcsave *Funcsave;
/**/
int noerrexit;
-/* used to suppress ERREXIT or ERRRETURN for one occurrence: 0 or 1 */
+/*
+ * used to suppress ERREXIT and ERRRETURN for the command under evaluation.
+ * 0 or 1
+ */
/**/
int this_noerrexit;
@@ -1429,10 +1432,7 @@ execlist(Estate state, int dont_change_job, int exiting)
if (!oldnoerrexit)
noerrexit = isend ? 0 : NOERREXIT_EXIT | NOERREXIT_RETURN;
if (WC_SUBLIST_FLAGS(code) & WC_SUBLIST_NOT) {
- /* suppress errexit for "! this_command" */
- if (isend)
- this_noerrexit = 1;
- /* suppress errexit for ! <list-of-shell-commands> */
+ /* suppress errexit for the commands in ! <list-of-commands> */
noerrexit = NOERREXIT_EXIT | NOERREXIT_RETURN;
}
switch (WC_SUBLIST_TYPE(code)) {
@@ -1443,6 +1443,9 @@ execlist(Estate state, int dont_change_job, int exiting)
else
execpline(state, code, ltype, (ltype & Z_END) && exiting);
state->pc = next;
+ /* suppress errexit for the command "! ..." */
+ if (WC_SUBLIST_FLAGS(code) & WC_SUBLIST_NOT)
+ this_noerrexit = 1;
goto sublist_done;
break;
case WC_SUBLIST_AND:
diff --git a/Test/C03traps.ztst b/Test/C03traps.ztst
index 4719dfd57..08e24a32e 100644
--- a/Test/C03traps.ztst
+++ b/Test/C03traps.ztst
@@ -720,6 +720,21 @@ F:Must be tested with a top-level script rather than source or function
0:ERR_RETURN in "else" branch in nested function
>Good
+ (setopt err_exit
+ ! true
+ print OK
+ )
+0:ERR_EXIT not triggered by "! true"
+>OK
+
+ (setopt err_exit
+ fn() { true }
+ ! fn
+ print OK
+ )
+0:ERR_EXIT not triggered by "! fn"
+>OK
+
(setopt err_exit
false && true
print OK
diff --git a/Src/exec.c b/Src/exec.c
index d8501ca68..43df8211a 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -5932,15 +5932,6 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int noreturnval)
* This function is forced to return.
*/
retflag = 0;
- /*
- * The calling function isn't necessarily forced to return,
- * but it should be made sensitive to ERR_EXIT and
- * ERR_RETURN as the assumptions we made at the end of
- * constructs within this function no longer apply. If
- * there are cases where this is not true, they need adding
- * to C03traps.ztst.
- */
- this_noerrexit = 0;
breaks = funcsave->breaks;
}
freearray(pparams);
@@ -6010,6 +6001,7 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int noreturnval)
trap_return++;
ret = lastval;
noerrexit = funcsave->noerrexit;
+ this_noerrexit = 0;
if (noreturnval) {
lastval = funcsave->lastval;
numpipestats = funcsave->numpipestats;
diff --git a/Test/C03traps.ztst b/Test/C03traps.ztst
index 08e24a32e..a8880673f 100644
--- a/Test/C03traps.ztst
+++ b/Test/C03traps.ztst
@@ -742,6 +742,15 @@ F:Must be tested with a top-level script rather than source or function
0:ERR_EXIT not triggered by "false && true"
>OK
+ (setopt err_exit
+ fn() {
+ false && true
+ }
+ fn
+ print OK
+ )
+1:ERR_EXIT not triggered by "false && true" but by return from fn
+
(setopt err_exit
for x in y; do
false && true
@@ -751,6 +760,17 @@ F:Must be tested with a top-level script rather than source or function
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
false && true
@@ -760,6 +780,17 @@ F:Must be tested with a top-level script rather than source or function
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
false && true
@@ -769,6 +800,17 @@ F:Must be tested with a top-level script rather than source or function
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
@@ -782,6 +824,21 @@ F:Must be tested with a top-level script rather than source or function
>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
@@ -794,6 +851,20 @@ F:Must be tested with a top-level script rather than source or function
>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
@@ -803,6 +874,17 @@ F:Must be tested with a top-level script rather than source or function
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
for x in y; do
Messages sorted by:
Reverse Date,
Date,
Thread,
Author