zsh-workers
 help / color / mirror / code / Atom feed
From: Philippe Altherr <philippe.altherr@gmail.com>
To: Zsh hackers list <zsh-workers@zsh.org>
Cc: "Bart Schaefer" <schaefer@brasslantern.com>,
	"Lawrence Velázquez" <larryv@zsh.org>
Subject: [Patch] Fix ERR_RETURN behavior in and/or statements
Date: Fri, 25 Nov 2022 18:44:56 +0100	[thread overview]
Message-ID: <CAGdYchvsHUNnjgW_wHEHcQVYguttj=BpiPq+zWyDHQeo3-KUGg@mail.gmail.com> (raw)


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

ATTENTION: This patch depends on the following patches described here
<https://inbox.vuxu.org/zsh-workers/CAGdYchufLwUgpKVU6b5eOiF15p0njFr59H1q9XizypwNjzqKzw@mail.gmail.com/>
:
- patch-01-revert-mistaken-errexit-patches.txt
- patch-03-fix-negation-statement.txt

I have found another issue in execlist, which affects the behavior of
ERR_RETURN in convoluted cases that involve at least two &&/|| commands and
a function call. The following example exhibits the issue.

set -o err_return
> fn() { { false; echo fn1 } && echo fn2 }
> fn; echo done
> fn && echo done


The execution of "fn; echo done" prints "fn1", "fn2", and "done". The
execution of "fn && echo done" should print the same but currently prints
nothing.

The problem is that this test
<https://github.com/zsh-users/zsh/blob/291940bae6cc0471c35c73498e873bc58dae9a95/Src/exec.c#L1429>
in "execlist" is too conservative. Inside "fn", "oldnoerrexit" is equal to
"NOERREXIT_EXIT". Indeed the "&&" in "fn && echo done"  first sets
"noerrexit" to  "NOERREXIT_EXIT | NOERREXIT_RETURN". The call to "fn" then
removes "NOERREXIT_RETURN". Thus, at the beginning of "execlist" in the
call to "fn", "noerrexit" is equal to "NOERREXIT_EXIT" and "oldnoerrexit"
is initialized to the same value. Since "oldnoerrexit" is non-zero, the test
<https://github.com/zsh-users/zsh/blob/291940bae6cc0471c35c73498e873bc58dae9a95/Src/exec.c#L1429>
fails
to readd "NOERREXIT_RETURN" for the evaluation of "{ false; echo fn1 }".
Therefore the evaluation of "false" triggers an early return of "fn" wîth
exit status 1, which in turn triggers an early return of the toplevel and
thus nothing is printed.

The solution is to unconditionally set "noerrexit" to "NOERREXIT_EXIT |
NOERREXIT_RETURN" for any command on the left of "&&" and "||" (and for any
command on the right of "!"). The patch does exactly that (and adds a
regression test).

Philippe

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

[-- Attachment #2: patch-05-fix-and-or-statement.txt --]
[-- Type: text/plain, Size: 1955 bytes --]

diff --git a/Src/exec.c b/Src/exec.c
index 43df8211a..711d8f374 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -1427,14 +1427,12 @@ execlist(Estate state, int dont_change_job, int exiting)
 	    goto sublist_done;
 	}
 	while (wc_code(code) == WC_SUBLIST) {
-	    int isend = (WC_SUBLIST_TYPE(code) == WC_SUBLIST_END);
+	    int isandor = WC_SUBLIST_TYPE(code) != WC_SUBLIST_END;
+	    int isnot = WC_SUBLIST_FLAGS(code) & WC_SUBLIST_NOT;
 	    next = state->pc + WC_SUBLIST_SKIP(code);
-	    if (!oldnoerrexit)
-		noerrexit = isend ? 0 : NOERREXIT_EXIT | NOERREXIT_RETURN;
-	    if (WC_SUBLIST_FLAGS(code) & WC_SUBLIST_NOT) {
-		/* suppress errexit for the commands in ! <list-of-commands> */
+	    /* suppress errexit for commands before && and || and after ! */
+	    if (isandor || isnot)
 		noerrexit = NOERREXIT_EXIT | NOERREXIT_RETURN;
-	    }
 	    switch (WC_SUBLIST_TYPE(code)) {
 	    case WC_SUBLIST_END:
 		/* End of sublist; just execute, ignoring status. */
@@ -1444,7 +1442,7 @@ execlist(Estate state, int dont_change_job, int exiting)
 		    execpline(state, code, ltype, (ltype & Z_END) && exiting);
 		state->pc = next;
 		/* suppress errexit for the command "! ..." */
-		if (WC_SUBLIST_FLAGS(code) & WC_SUBLIST_NOT)
+		if (isnot)
 		  this_noerrexit = 1;
 		goto sublist_done;
 		break;
diff --git a/Test/C03traps.ztst b/Test/C03traps.ztst
index a8880673f..b7132da81 100644
--- a/Test/C03traps.ztst
+++ b/Test/C03traps.ztst
@@ -670,6 +670,22 @@ F:Must be tested with a top-level script rather than source or function
 >before-out
 >before-in
 
+  (set -o err_return
+    fn() {
+      print before-in
+      { false; true } && true
+      print after-in
+    }
+    print before-out
+    fn && true
+    print after-out
+  )
+0:ERR_RETURN not triggered on LHS of "&&" in function on LHS of "&&" (regression test)
+>before-out
+>before-in
+>after-in
+>after-out
+
   mkdir -p zdotdir
   print >zdotdir/.zshenv '
   setopt norcs errreturn

             reply	other threads:[~2022-11-25 17:45 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-11-25 17:44 Philippe Altherr [this message]
2022-11-25 17:55 ` Bart Schaefer
2022-11-26  3:47   ` Philippe Altherr
2022-12-03  1:31     ` Philippe Altherr
2022-12-03  1:36       ` Philippe Altherr
2022-12-03  5:11         ` Lawrence Velázquez

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAGdYchvsHUNnjgW_wHEHcQVYguttj=BpiPq+zWyDHQeo3-KUGg@mail.gmail.com' \
    --to=philippe.altherr@gmail.com \
    --cc=larryv@zsh.org \
    --cc=schaefer@brasslantern.com \
    --cc=zsh-workers@zsh.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).