zsh-workers
 help / color / mirror / code / Atom feed
* [PATCH] Fix ERR_EXIT behavior in function calls and "always" statements
@ 2022-11-16 14:40 Philippe Altherr
  2022-11-19 13:39 ` Philippe Altherr
  0 siblings, 1 reply; 20+ messages in thread
From: Philippe Altherr @ 2022-11-16 14:40 UTC (permalink / raw)
  To: Zsh hackers list; +Cc: Bart Schaefer, Lawrence Velázquez


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

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:
      -    50929: fix handling of ERR_RETURN bent by 50928.
      <https://sourceforge.net/p/zsh/code/ci/8839e969bf8f3f129d0efd8ecd51505610a1f01b/>
      -    50928: fix tests for 50897, mention behavior change in NEWS
      <https://sourceforge.net/p/zsh/code/ci/1ba8714a7ac665e661c1b3a716ffe2af73d1e443/>
         -       I kept the localoptions fix in one of the tests.
      -    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
      - 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

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

[-- Attachment #2: fix-err-exit-behavior-in-function-calls-and-always-statements.txt --]
[-- Type: text/plain, Size: 7027 bytes --]

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..01d8fa25b 100644
--- a/Test/C03traps.ztst
+++ b/Test/C03traps.ztst
@@ -721,21 +721,40 @@ 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
+  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] 20+ messages in thread

* Re: [PATCH] Fix ERR_EXIT behavior in function calls and "always" statements
  2022-11-16 14:40 [PATCH] Fix ERR_EXIT behavior in function calls and "always" statements Philippe Altherr
@ 2022-11-19 13:39 ` Philippe Altherr
  2022-11-21  0:43   ` Bart Schaefer
  0 siblings, 1 reply; 20+ messages in thread
From: Philippe Altherr @ 2022-11-19 13:39 UTC (permalink / raw)
  To: Zsh hackers list; +Cc: Bart Schaefer, Lawrence Velázquez


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

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



On Wed, Nov 16, 2022 at 3:40 PM Philippe Altherr <philippe.altherr@gmail.com>
wrote:

> 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:
>       -    50929: fix handling of ERR_RETURN bent by 50928.
>       <https://sourceforge.net/p/zsh/code/ci/8839e969bf8f3f129d0efd8ecd51505610a1f01b/>
>       -    50928: fix tests for 50897, mention behavior change in NEWS
>       <https://sourceforge.net/p/zsh/code/ci/1ba8714a7ac665e661c1b3a716ffe2af73d1e443/>
>          -       I kept the localoptions fix in one of the tests.
>       -    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
>       - 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
>
>

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

[-- Attachment #2: patch-01-revert-mistaken-errexit-patches.txt --]
[-- Type: text/plain, Size: 4234 bytes --]

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

[-- Attachment #3: patch-02-fix-always-statement.txt --]
[-- Type: text/plain, Size: 1517 bytes --]

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

[-- Attachment #4: patch-03-fix-negation-statement.txt --]
[-- Type: text/plain, Size: 1801 bytes --]

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

[-- Attachment #5: patch-04-fix-function-call.txt --]
[-- Type: text/plain, Size: 3811 bytes --]

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

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

* Re: [PATCH] Fix ERR_EXIT behavior in function calls and "always" statements
  2022-11-19 13:39 ` Philippe Altherr
@ 2022-11-21  0:43   ` Bart Schaefer
  2022-11-21  7:22     ` Lawrence Velázquez
  0 siblings, 1 reply; 20+ messages in thread
From: Bart Schaefer @ 2022-11-21  0:43 UTC (permalink / raw)
  To: Philippe Altherr; +Cc: Zsh hackers list, Lawrence Velázquez

On Sat, Nov 19, 2022 at 5:39 AM Philippe Altherr
<philippe.altherr@gmail.com> wrote:
>
> I recommend submitting the 4 patches attached here.

Thanks again.  I'm generally more pleased with this set of patches
because the number of places you had to save/restore this_noerrexit in
the previous go-round seemed intuitively wrong to me.

Each time though, you've removed the NEWS item.  Although we've
established that the behavior it describes was not actually
appropriate, there still has been a change in ERR_EXIT behavior that
probably warrants a mention.  What's the best description of that?

Question for the wider list:  With this patch, anonymous functions
behave like functions with respect to ERR_EXIT.  This is reasonable,
but it does mean that

set -e
{ { false && true } } # does not exit
() { { false && true } } # exits

Are we all OK with this, and either way does it need to be called out
somewhere?  For the record (and per my above remark about NEWS) the
previous behavior was that neither of these would exit.  Frankly I'm
still not certain that the extra level of { } should matter in the
function example.


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

* Re: [PATCH] Fix ERR_EXIT behavior in function calls and "always" statements
  2022-11-21  0:43   ` Bart Schaefer
@ 2022-11-21  7:22     ` Lawrence Velázquez
  2022-11-22  2:52       ` Philippe Altherr
  0 siblings, 1 reply; 20+ messages in thread
From: Lawrence Velázquez @ 2022-11-21  7:22 UTC (permalink / raw)
  To: Bart Schaefer, Philippe Altherr; +Cc: zsh-workers

On Sun, Nov 20, 2022, at 7:43 PM, Bart Schaefer wrote:
> Question for the wider list:  With this patch, anonymous functions
> behave like functions with respect to ERR_EXIT.  This is reasonable,
> but it does mean that
>
> set -e
> { { false && true } } # does not exit
> () { { false && true } } # exits
>
> Are we all OK with this

I am not sure.

On the one hand, it's consistent for anonymous functions to behave
like functions here, as they do in other respects.

On the other hand, this behavior violates the general idea that
complex commands shouldn't cause early exits due to ignored nonzero
exit statuses bubbling up from inside.  While anonymous functions
aren't listed as complex commands in the documentation, they certainly
aren't simple commands.  Function calls pop the bubbles (so to
speak) because they are simple commands, not because there's anything
particularly special about functions.  This suggests that anonymous
functions should *not* behave like normal function calls here.

So yeah, I don't know.


> Frankly I'm
> still not certain that the extra level of { } should matter in the
> function example.

Does it?  These seem to behave identically with Philippe's latest
patches; am I overlooking something?

	% Src/zsh -ec '() { { false && true } }; echo done'; echo $?
	1
	% Src/zsh -ec '() { false && true }; echo done'; echo $? 
	1


-- 
vq


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

* Re: [PATCH] Fix ERR_EXIT behavior in function calls and "always" statements
  2022-11-21  7:22     ` Lawrence Velázquez
@ 2022-11-22  2:52       ` Philippe Altherr
  2022-11-22  4:51         ` Bart Schaefer
                           ` (2 more replies)
  0 siblings, 3 replies; 20+ messages in thread
From: Philippe Altherr @ 2022-11-22  2:52 UTC (permalink / raw)
  To: Lawrence Velázquez; +Cc: Bart Schaefer, zsh-workers

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

>
> Each time though, you've removed the NEWS item.  Although we've
> established that the behavior it describes was not actually
> appropriate, there still has been a change in ERR_EXIT behavior that
> probably warrants a mention.  What's the best description of that?


Good point. There is indeed a behavior change but it's more complicated
than what you described. I will try to come up with something. Actually my
3 fix patches address 3 different issues. In my opinion, at least 2 of them
qualify as bugs but it still makes sense to describe what changes.

This suggests that anonymous
> functions should *not* behave like normal function calls here.


That's debatable. If, like me, you see anonymous functions as some kind of
syntactic sugar, then it makes sense that they behave like function calls
and exit. However, I can also understand that others rather see them as
another type of compound command. In that case, they should not exit.

To help decide what to do, here is a table that lists all the different
cases and how they behave in the current Zsh, in a patched Zsh, and in a
patched Zsh where anonymous functions behave as compound commands:

   Code                             Current Zsh     Patched Zsh
 Compound command (and patches)
A)         false                    Exit            Exit            Exit
B)         false && true            No exit         No exit         No exit
C)     {   false && true   }        No exit         No exit         No exit
D)  () {   false           }        Exit            Exit            Exit
E)  () {   false && true   }        Exit            Exit            *No
exit*
F)  () { { false && true } }        No Exit         *Exit*            No
exit
G) f() {   false           }; f     Exit            Exit            Exit
H) f() {   false && true   }; f     Exit            Exit            Exit
I) f() { { false && true } }; f     No Exit         *Exit*            *Exit*

Currently anonymous functions behave like function calls. My patches don't
change that but they change/fix cases F and I to behave as mandated by
POSIX. If anonymous functions are changed to behave like compound commands
then anonymous functions behave as if the code was inlined. This changes
the behavior of case E, which currently exits.

Is the compound command behavior a reasonable one? Certainly yes. Is it
desirable? I guess that's more debatable. Personally, I am leaning towards
the current behavior. I could be convinced otherwise and for sure I could
live with the alternative.

If you care about code complexity (of the Zsh implementation), then that
would be an argument against the compound command option as it requires
additional code. However, in my opinion, languages should in general NOT be
designed to simplify their implementation but rather to simplify their
usage.

Does it?  These seem to behave identically with Philippe's latest
> patches; am I overlooking something?


No, with my patch "false && true" and "{ false && true}" now always behave
the same.

Frankly I'm
> still not certain that the extra level of { } should matter in the
> function example.


With my patches, they no longer do. Note however that the behaviors of the
following examples, which are unaffected by my patches:

set -e
> { false && true } # does not exit
> () { false && true } # exits


As you can see, with my patches, an extra level of { } no longer changes
any behavior. However, already today, anonymous functions don't always
behave the same as the inlined code.

FYI: Here are my next steps
- Write NEWS for my 3 fixes.
- Better document the role and usage of noerrexit and this_noerrexit.
- Try to fix "eval", "source", and possibly a bunch of other related cases.

Unless anyone sees a reason not to, it would be nice to submit my first
pacth, which reverts Bart's changes. For the other patches, I have at the
very least to first add a NEWS item.

Philippe

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

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

* Re: [PATCH] Fix ERR_EXIT behavior in function calls and "always" statements
  2022-11-22  2:52       ` Philippe Altherr
@ 2022-11-22  4:51         ` Bart Schaefer
  2022-11-22 10:17         ` Peter Stephenson
  2022-11-23  6:59         ` Lawrence Velázquez
  2 siblings, 0 replies; 20+ messages in thread
From: Bart Schaefer @ 2022-11-22  4:51 UTC (permalink / raw)
  To: Philippe Altherr; +Cc: Lawrence Velázquez, zsh-workers

On Mon, Nov 21, 2022 at 6:52 PM Philippe Altherr
<philippe.altherr@gmail.com> wrote:
>

For anyone arriving in the middle of the conversation or viewing this
in the archives --

Please note that I wrote this:
>> Each time though, you've removed the NEWS item.  Although we've
>> established that the behavior it describes was not actually
>> appropriate, there still has been a change in ERR_EXIT behavior that
>> probably warrants a mention.  What's the best description of that?

Whereas Lawrence wrote this:
>> This suggests that anonymous
>> functions should *not* behave like normal function calls here.

> That's debatable. If, like me, you see anonymous functions as some kind of syntactic sugar, then it makes sense that they behave like function calls and exit. However, I can also understand that others rather see them as another type of compound command. In that case, they should not exit.

I think it's probably more appropriate to view a function, even an
anonymous one, like a subshell rather than like either a simple or a
complex command.  Subshells also "pop the bubble" to use Lawrence's
analogy, so it's probably fine for anonymous functions to do so.

> FYI: Here are my next steps
> - Write NEWS for my 3 fixes.
> - Better document the role and usage of noerrexit and this_noerrexit.
> - Try to fix "eval", "source", and possibly a bunch of other related cases.
>
> Unless anyone sees a reason not to, it would be nice to submit my first pacth, which reverts Bart's changes. For the other patches, I have at the very least to first add a NEWS item.

I don't think there's any requirement that the NEWS item arrive at the
same time as the other three patches.  We sometimes don't add NEWS
until the time of a release, months (years) after a patch was pushed
to sourceforge git.

I'd also prefer to post all your patches at once because you attached
them all to the same zsh-workers article.  Convention has been that a
series of patches should be sent as a series of articles.

I think I will go ahead and remove (most of) the ChangeLog references
to my patches, unless anyone feels they're important for context (the
git log of course remains).


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

* Re: [PATCH] Fix ERR_EXIT behavior in function calls and "always" statements
  2022-11-22  2:52       ` Philippe Altherr
  2022-11-22  4:51         ` Bart Schaefer
@ 2022-11-22 10:17         ` Peter Stephenson
  2022-11-23  8:11           ` Lawrence Velázquez
  2022-11-23  6:59         ` Lawrence Velázquez
  2 siblings, 1 reply; 20+ messages in thread
From: Peter Stephenson @ 2022-11-22 10:17 UTC (permalink / raw)
  To: Philippe Altherr, zsh-workers

On 22/11/2022 02:52 Philippe Altherr <philippe.altherr@gmail.com> wrote:
> Currently anonymous functions behave like function calls. My patches
> don't change that but they change/fix cases F and I to behave as mandated
> by POSIX. If anonymous functions are changed to behave like compound
> commands then anonymous functions behave as if the code was inlined.
> This changes the behavior of case E, which currently exits.

On the whole I think keeping anonymous functions behaving like other functions
is probably sensible, though we could draw better attention to this unusual
degree of consistency somewhere in the documentation.

I don't think there's a killer argument for this but if you're using an
anonymous function it's because you need some form of function behaviour and
my own inclination would be to continue to provide essentially the whole of it,
hopefully also limiting special cases in the source code.

(One possible comparison is Ruby, where you consistently either get "yield
semantics" or "function semantics", though comparing zsh with Ruby feels
wrong at pretty much every other level so this is a very shallow similarity
indeed.)

pws


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

* Re: [PATCH] Fix ERR_EXIT behavior in function calls and "always" statements
  2022-11-22  2:52       ` Philippe Altherr
  2022-11-22  4:51         ` Bart Schaefer
  2022-11-22 10:17         ` Peter Stephenson
@ 2022-11-23  6:59         ` Lawrence Velázquez
  2022-11-23  9:43           ` Philippe Altherr
  2 siblings, 1 reply; 20+ messages in thread
From: Lawrence Velázquez @ 2022-11-23  6:59 UTC (permalink / raw)
  To: Philippe Altherr; +Cc: zsh-workers

On Mon, Nov 21, 2022, at 9:52 PM, Philippe Altherr wrote:
> To help decide what to do, here is a table that lists all the different 
> cases and how they behave in the current Zsh, in a patched Zsh, and in 
> a patched Zsh where anonymous functions behave as compound commands:
>
>    Code                             Current Zsh     Patched Zsh     
> Compound command (and patches)
> A)         false                    Exit            Exit            Exit
> B)         false && true            No exit         No exit         No 
> exit
> C)     {   false && true   }        No exit         No exit         No 
> exit
> D)  () {   false           }        Exit            Exit            Exit
> E)  () {   false && true   }        Exit            Exit            *No 
> exit*    
> F)  () { { false && true } }        No Exit         *Exit*            
> No exit
> G) f() {   false           }; f     Exit            Exit            Exit
> H) f() {   false && true   }; f     Exit            Exit            
> Exit    
> I) f() { { false && true } }; f     No Exit         *Exit*            
> *Exit*
>
> Currently anonymous functions behave like function calls. My patches 
> don't change that but they change/fix cases F and I to behave as 
> mandated by POSIX.

POSIX does not mandate any behavior for case F unless one treats
anonymous functions as compound commands, in which case the new
behavior actually violates the standard.

-- 
vq


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

* Re: [PATCH] Fix ERR_EXIT behavior in function calls and "always" statements
  2022-11-22 10:17         ` Peter Stephenson
@ 2022-11-23  8:11           ` Lawrence Velázquez
  0 siblings, 0 replies; 20+ messages in thread
From: Lawrence Velázquez @ 2022-11-23  8:11 UTC (permalink / raw)
  To: Peter Stephenson; +Cc: zsh-workers

On Tue, Nov 22, 2022, at 5:17 AM, Peter Stephenson wrote:
> On the whole I think keeping anonymous functions behaving like other functions
> is probably sensible, though we could draw better attention to this unusual
> degree of consistency somewhere in the documentation.
>
> I don't think there's a killer argument for this but if you're using an
> anonymous function it's because you need some form of function behaviour and
> my own inclination would be to continue to provide essentially the whole of it,
> hopefully also limiting special cases in the source code.

There would certainly be value in permitting

    set -e
    foo() { cmd1; cmd2; cmd3 }
    foo
    unset -f foo

to be simplified to

    set -e
    () { cmd1; cmd2; cmd3 }

while maintaining the same early-exit behavior.  That consistency
might be more useful than strict adherence to the syntax-based logic
of POSIX "set -e".  Plus, as Bart noted, the (...) exception has
already set a precedent for special treatment.

-- 
vq


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

* Re: [PATCH] Fix ERR_EXIT behavior in function calls and "always" statements
  2022-11-23  6:59         ` Lawrence Velázquez
@ 2022-11-23  9:43           ` Philippe Altherr
  2022-11-23 16:11             ` Bart Schaefer
  2022-11-24  4:28             ` Lawrence Velázquez
  0 siblings, 2 replies; 20+ messages in thread
From: Philippe Altherr @ 2022-11-23  9:43 UTC (permalink / raw)
  To: Lawrence Velázquez; +Cc: zsh-workers

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

>
> POSIX does not mandate any behavior for case F unless one treats

anonymous functions as compound commands


I think POSIX indirectly mandates the behavior. So far, there are two
proposals on how to specify anonymous functions:

A) a function call (to a function defined on the spot and undefined right
after the call)
B) a compound command

POSIX mandates the behavior for both, function calls and compound commands.
Therefore, if we opt for one of these two specifications, POSIX also
indirectly mandates the behavior of anonymous functions.

In my opinion, the name and the description of anonymous functions strongly
suggests that anonymous functions are a shorthand for defining a function,
calling it with the provided arguments, and undefining the function. I have
always assumed that "() { <body> } <args>" is syntactic sugar for "anon() {
<body> }; { anon <args> } always { unfunction anon }". As far as I can
tell, Zsh effectively implements anonymous functions with function calls.
If this is indeed the case and one agrees with the specification described
here, then everything is consistent; anonymous functions look, feel, and
behave like function calls, including when it comes to ERR_EXIT, and this
with and without my patches.

You propose to specify anonymous functions as a kind of compound command.
This is of course possible but it's not how anonymous functions currently
behave. As far as I can tell, they currently behave in all aspects,
including ERR_EXIT, as function calls. For ERR_EXIT, see case E, which
behaves like the equivalent function call (case H) and not like the
equivalent compound command (case B). Note that this is true with and
without my patch. Beside the new behavior for ERR_EXIT are there other
aspects of anonymous functions that would/should change? If not, is that
change of specification really worth it? I fear that anonymous functions as
compound commands require a more complicated mental model than anonymous
functions as function calls. For example, if anonymous functions are
compound commands then I would expect that the "return" in the code below
exits the function "foo" but that's not what it does.

foo() {
>   echo foo-start;
>   () { echo args: $@; return } 1 2 3
>   echo foo-end;
> }
> foo


My feeling is that changing anonymous functions to behave like compound
commands adds complexity for little benefit. If it's all about the ERR_EXIT
behavior, does that really matter that much? Is there a use case for
anonymous functions where the compound command ERR_EXIT behavior is
significantly better than the function call ERR_EXIT behavior?

I'd also prefer to post all your patches at once because you attached
> them all to the same zsh-workers article.  Convention has been that a
> series of patches should be sent as a series of articles.


OK, then I will resend my patches in separate emails. This way I can better
document what each one is doing. It will also be easier for you (or whoever
submits patches) to sooner submit the less controversial ones while the
more controversial ones remain open for discussion.

I don't think there's any requirement that the NEWS item arrive at the
> same time as the other three patches.  We sometimes don't add NEWS
> until the time of a release, months (years) after a patch was pushed
> to sourceforge git.


I see. In that case I will start another thread with a patch for the NEWS
item where we can discuss the exact wording.

Philippe


On Wed, Nov 23, 2022 at 7:59 AM Lawrence Velázquez <larryv@zsh.org> wrote:

> On Mon, Nov 21, 2022, at 9:52 PM, Philippe Altherr wrote:
> > To help decide what to do, here is a table that lists all the different
> > cases and how they behave in the current Zsh, in a patched Zsh, and in
> > a patched Zsh where anonymous functions behave as compound commands:
> >
> >    Code                             Current Zsh     Patched Zsh
> > Compound command (and patches)
> > A)         false                    Exit            Exit            Exit
> > B)         false && true            No exit         No exit         No
> > exit
> > C)     {   false && true   }        No exit         No exit         No
> > exit
> > D)  () {   false           }        Exit            Exit            Exit
> > E)  () {   false && true   }        Exit            Exit            *No
> > exit*
> > F)  () { { false && true } }        No Exit         *Exit*
> > No exit
> > G) f() {   false           }; f     Exit            Exit            Exit
> > H) f() {   false && true   }; f     Exit            Exit
> > Exit
> > I) f() { { false && true } }; f     No Exit         *Exit*
> > *Exit*
> >
> > Currently anonymous functions behave like function calls. My patches
> > don't change that but they change/fix cases F and I to behave as
> > mandated by POSIX.
>
> POSIX does not mandate any behavior for case F unless one treats
> anonymous functions as compound commands, in which case the new
> behavior actually violates the standard.
>
> --
> vq
>

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

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

* Re: [PATCH] Fix ERR_EXIT behavior in function calls and "always" statements
  2022-11-23  9:43           ` Philippe Altherr
@ 2022-11-23 16:11             ` Bart Schaefer
  2022-11-23 20:57               ` Submitting multiple patches (was: Re: [PATCH] Fix ERR_EXIT behavior in function calls and "always" statements) Daniel Shahaf
  2022-11-24  1:47               ` [PATCH] Fix ERR_EXIT behavior in function calls and "always" statements Philippe Altherr
  2022-11-24  4:28             ` Lawrence Velázquez
  1 sibling, 2 replies; 20+ messages in thread
From: Bart Schaefer @ 2022-11-23 16:11 UTC (permalink / raw)
  To: Philippe Altherr; +Cc: Lawrence Velázquez, zsh-workers

On Wed, Nov 23, 2022 at 1:44 AM Philippe Altherr
<philippe.altherr@gmail.com> wrote:
>
> You propose to specify anonymous functions as a kind of compound command.

I'm not proposing anything.  I asked a question of the group.  Per
latest discussions, my personal feeling is that anonymous functions
should have the same treatment (in this particular respect) as
subshells, so they would not be compound commands.

>> I'd also prefer to post all your patches at once because you attached
>> them all to the same zsh-workers article.  Convention has been that a
>> series of patches should be sent as a series of articles.
>
> OK, then I will resend my patches in separate emails.

That's not necessary either, and I don't think any of the patches so
far are controversial.


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

* Submitting multiple patches (was: Re: [PATCH] Fix ERR_EXIT behavior in function calls and "always" statements)
  2022-11-23 16:11             ` Bart Schaefer
@ 2022-11-23 20:57               ` Daniel Shahaf
  2022-11-23 21:11                 ` Bart Schaefer
  2022-11-24  1:47               ` [PATCH] Fix ERR_EXIT behavior in function calls and "always" statements Philippe Altherr
  1 sibling, 1 reply; 20+ messages in thread
From: Daniel Shahaf @ 2022-11-23 20:57 UTC (permalink / raw)
  To: zsh-workers; +Cc: Philippe Altherr, Lawrence Velázquez

Bart Schaefer wrote on Wed, Nov 23, 2022 at 08:11:02 -0800:
> On Wed, Nov 23, 2022 at 1:44 AM Philippe Altherr
> <philippe.altherr@gmail.com> wrote:
> >> I'd also prefer to post all your patches at once because you attached
> >> them all to the same zsh-workers article.  Convention has been that a
> >> series of patches should be sent as a series of articles.

I don't think we should insist on this.

If someone has git-send-email(1) configured, sure, please send the
patches individually, and let email threading do its thing.  But if not…
I'd rather ask people to send one message with N attachments, numbered
as Philippe did in workers/51001, than ask people to send N separate
mails, in which case they might send nothing at all.

Besides, I'd like to enourage submitters to split their patches if in
doubt, since it's easier to combine patches ("42 + 43: foo") than to
split them ("42 (in part): foo").  So, I'd like to make it easier to
send multiple related diffs.  We can then commit them /en bloc/, or
squashed, or cherry-pick from them, as needed.

I did this myself a couple of times, e.g., in workers/48601.  Looking in
the archives, mhonarc (https://www.zsh.org/mla/workers/2021/msg00826.html)
doesn't render multiple text attachments sanely, but the official mbox
archives and public-inbox both do (https://inbox.vuxu.org/zsh-workers/20210416182141.GA15670@tarpaulin.shahaf.local2/T/#md006c516ad7f04f6d6225f213357839e4de03dcf).

Cheers,

Daniel

> > OK, then I will resend my patches in separate emails.
> 
> That's not necessary either, and I don't think any of the patches so
> far are controversial.
> 


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

* Re: Submitting multiple patches (was: Re: [PATCH] Fix ERR_EXIT behavior in function calls and "always" statements)
  2022-11-23 20:57               ` Submitting multiple patches (was: Re: [PATCH] Fix ERR_EXIT behavior in function calls and "always" statements) Daniel Shahaf
@ 2022-11-23 21:11                 ` Bart Schaefer
  2022-11-23 21:22                   ` Bart Schaefer
  2022-11-23 21:54                   ` Bart Schaefer
  0 siblings, 2 replies; 20+ messages in thread
From: Bart Schaefer @ 2022-11-23 21:11 UTC (permalink / raw)
  To: Daniel Shahaf; +Cc: zsh-workers, Philippe Altherr, Lawrence Velázquez

On Wed, Nov 23, 2022 at 12:58 PM Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
>
> Bart Schaefer wrote on Wed, Nov 23, 2022 at 08:11:02 -0800:
> > On Wed, Nov 23, 2022 at 1:44 AM Philippe Altherr
> > <philippe.altherr@gmail.com> wrote:
> > >> I'd also prefer to post all your patches at once because you attached
> > >> them all to the same zsh-workers article.  Convention has been that a
> > >> series of patches should be sent as a series of articles.
>
> I don't think we should insist on this.

I don't think we should insist on this either, but I also don't think
that, having posted them all in one email in the first place, we need
to have them reposted now, unless something about them is going to
change.

> I'd rather ask people to send one message with N attachments, numbered
> as Philippe did in workers/51001, than ask people to send N separate
> mails, in which case they might send nothing at all.

That's fine.  The ONLY issue with Philippe's message, if there is any
problem with it at all, is that two of the patches overlapped on one
of the files, such that one backed out a change and then another put
that same change back again.  I'd just as soon pretend that was all
one patch consisting only of the independent parts of the two, and
assigned only a single -workers article number.

> Besides, I'd like to enourage submitters to split their patches if in
> doubt, since it's easier to combine patches ("42 + 43: foo") than to
> split them ("42 (in part): foo").  So, I'd like to make it easier to
> send multiple related diffs.

This is perfectly OK.


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

* Re: Submitting multiple patches (was: Re: [PATCH] Fix ERR_EXIT behavior in function calls and "always" statements)
  2022-11-23 21:11                 ` Bart Schaefer
@ 2022-11-23 21:22                   ` Bart Schaefer
  2022-11-23 21:54                   ` Bart Schaefer
  1 sibling, 0 replies; 20+ messages in thread
From: Bart Schaefer @ 2022-11-23 21:22 UTC (permalink / raw)
  To: Daniel Shahaf; +Cc: zsh-workers, Philippe Altherr, Lawrence Velázquez

On Wed, Nov 23, 2022 at 1:11 PM Bart Schaefer <schaefer@brasslantern.com> wrote:
>
> I'd just as soon pretend that was all
> one patch consisting only of the independent parts of the two, and
> assigned only a single -workers article number.

Incidentally, I've already prepared a commit based on this premise, I
just haven't pushed it yet because of this ongoing back-and-forth.


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

* Re: Submitting multiple patches (was: Re: [PATCH] Fix ERR_EXIT behavior in function calls and "always" statements)
  2022-11-23 21:11                 ` Bart Schaefer
  2022-11-23 21:22                   ` Bart Schaefer
@ 2022-11-23 21:54                   ` Bart Schaefer
  2022-11-24  2:21                     ` Philippe Altherr
  1 sibling, 1 reply; 20+ messages in thread
From: Bart Schaefer @ 2022-11-23 21:54 UTC (permalink / raw)
  To: Daniel Shahaf; +Cc: zsh-workers, Philippe Altherr, Lawrence Velázquez

On Wed, Nov 23, 2022 at 1:11 PM Bart Schaefer <schaefer@brasslantern.com> wrote:
>
> The ONLY issue with Philippe's message, if there is any
> problem with it at all, is that two of the patches overlapped on one
> of the files, such that one backed out a change and then another put
> that same change back again.

This also seems to reflect a philosophical point that it might be
worth mentioning in the development guide (which is probably overdue
for an overhaul in general).  Namely, Philippe felt it important to
have one patch that completely backed out two or more of my previous
patches, followed by a patch that incorporated a useful part of one of
those patches.

This isn't generally how we've approached things -- we just move
forward incrementally from the tip of the branch.


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

* Re: [PATCH] Fix ERR_EXIT behavior in function calls and "always" statements
  2022-11-23 16:11             ` Bart Schaefer
  2022-11-23 20:57               ` Submitting multiple patches (was: Re: [PATCH] Fix ERR_EXIT behavior in function calls and "always" statements) Daniel Shahaf
@ 2022-11-24  1:47               ` Philippe Altherr
  1 sibling, 0 replies; 20+ messages in thread
From: Philippe Altherr @ 2022-11-24  1:47 UTC (permalink / raw)
  To: Bart Schaefer; +Cc: Lawrence Velázquez, zsh-workers

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

>
> > You propose to specify anonymous functions as a kind of compound command.
>

> I'm not proposing anything.
>

Sorry for the confusion. I was referring to Lawrence who suggested that
anonymous functions should behave like compound commands.

Philippe

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

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

* Re: Submitting multiple patches (was: Re: [PATCH] Fix ERR_EXIT behavior in function calls and "always" statements)
  2022-11-23 21:54                   ` Bart Schaefer
@ 2022-11-24  2:21                     ` Philippe Altherr
  2022-11-26  2:28                       ` Bart Schaefer
  0 siblings, 1 reply; 20+ messages in thread
From: Philippe Altherr @ 2022-11-24  2:21 UTC (permalink / raw)
  To: Bart Schaefer; +Cc: Daniel Shahaf, zsh-workers, Lawrence Velázquez

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

>
> Namely, Philippe felt it important to
> have one patch that completely backed out two or more of my previous
> patches,
> *followed by a patch that incorporated a useful part of one of**those
> patches*.


Did I do that? My original big/unique patch did indeed remove your fix to
one of the tests in C03traps.ztst and at the same time added an equivalent
but different fix. I realized this only after sending my original patch. My
split patches no longer do that;
patch-01-revert-mistaken-errexit-patches.txt no longer reverts your fix.
I'm not aware of another of your changes that I revert in
patch-01-revert-mistaken-errexit-patches.txt and add back in one of the
later patches. Did I overlook something?

Philippe


On Wed, Nov 23, 2022 at 10:54 PM Bart Schaefer <schaefer@brasslantern.com>
wrote:

> On Wed, Nov 23, 2022 at 1:11 PM Bart Schaefer <schaefer@brasslantern.com>
> wrote:
> >
> > The ONLY issue with Philippe's message, if there is any
> > problem with it at all, is that two of the patches overlapped on one
> > of the files, such that one backed out a change and then another put
> > that same change back again.
>
> This also seems to reflect a philosophical point that it might be
> worth mentioning in the development guide (which is probably overdue
> for an overhaul in general).  Namely, Philippe felt it important to
> have one patch that completely backed out two or more of my previous
> patches, followed by a patch that incorporated a useful part of one of
> those patches.
>
> This isn't generally how we've approached things -- we just move
> forward incrementally from the tip of the branch.
>

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

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

* Re: [PATCH] Fix ERR_EXIT behavior in function calls and "always" statements
  2022-11-23  9:43           ` Philippe Altherr
  2022-11-23 16:11             ` Bart Schaefer
@ 2022-11-24  4:28             ` Lawrence Velázquez
  1 sibling, 0 replies; 20+ messages in thread
From: Lawrence Velázquez @ 2022-11-24  4:28 UTC (permalink / raw)
  To: Philippe Altherr; +Cc: zsh-workers

On Wed, Nov 23, 2022, at 4:43 AM, Philippe Altherr wrote:
> I think POSIX indirectly mandates the behavior. So far, there are two 
> proposals on how to specify anonymous functions:
>
> A) a function call (to a function defined on the spot and undefined 
> right after the call)
> B) a compound command

Anonymous functions are complex commands regardless; they are parsed
much like function definitions.  The question here is only about
how they should interact with ERR_EXIT.


> In my opinion, the name and the description of anonymous functions 
> strongly suggests that anonymous functions are a shorthand for defining 
> a function, calling it with the provided arguments, and undefining the 
> function. I have always assumed that "() { <body> } <args>" is 
> syntactic sugar for "anon() { <body> }; { anon <args> } always { 
> unfunction anon }". As far as I can tell, Zsh effectively implements 
> anonymous functions with function calls. If this is indeed the case and 
> one agrees with the specification described here, then everything is 
> consistent; anonymous functions look, feel, and behave like function 
> calls, including when it comes to ERR_EXIT, and this with and without 
> my patches.

This "syntactic sugar" argument is flawed in its very conception
because the POSIX specification for "set -e" is *all about syntax*.
It addresses the early-exit behavior of various *syntactic* command
forms -- pipelines, compound commands, AND-OR lists.  It does not
know or care about function calls; it does not mention them even
once.  They are just simple commands as far as POSIX "set -e" is
concerned.

To be clear, there is nothing wrong with considering anonymous
functions to be syntactic sugar for defining, calling, and removing
a function, but that does not change their syntax.  They *are*
complex commands.  It doesn't matter how they're implemented or how
you want to think about them -- you cannot reasonably use POSIX to
justify giving them the same early-exit behavior as vanilla function
calls.

Of course, that need not stop us from doing so.  POSIX compliance
is not a requirement for zsh, anonymous functions are already well
outside the POSIX spec, and carving out a (...)-like exception for
them (as Bart originally suggested) has a lot to recommend it (as
I agreed with in my response to Peter).  But at this point POSIX
is more or less out of the picture.  Which is fine.


> You propose to specify anonymous functions as a kind of compound 
> command.

No, I did not propose that.  I presented two possibilities for
anonymous functions' behavior with respect to ERR_EXIT, elaborated
on "like other complex commands" because I thought the argument for
"like regular function calls" was self-evident, and explicitly
expressed ambivalence about which was best.

Plus, they already are complex commands.


> I fear that anonymous functions as compound commands require a more 
> complicated mental model than anonymous functions as function calls.
> For example, if anonymous functions are compound commands then I would 
> expect that the "return" in the code below exits the function "foo" but 
> that's not what it does.
>
>> foo() {
>>   echo foo-start;
>>   () { echo args: $@; return } 1 2 3
>>   echo foo-end;
>> }
>> foo

Why would you expect that?  "Complex command" is a syntactic
classification that doesn't imply any particular behavior.  It
includes {...}, (...), function definitions, the various loops and
conditionals, etc., but these all behave differently.


-- 
vq


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

* Re: Submitting multiple patches (was: Re: [PATCH] Fix ERR_EXIT behavior in function calls and "always" statements)
  2022-11-24  2:21                     ` Philippe Altherr
@ 2022-11-26  2:28                       ` Bart Schaefer
  2022-11-26  4:02                         ` Philippe Altherr
  0 siblings, 1 reply; 20+ messages in thread
From: Bart Schaefer @ 2022-11-26  2:28 UTC (permalink / raw)
  To: Philippe Altherr; +Cc: Daniel Shahaf, zsh-workers, Lawrence Velázquez

On Wed, Nov 23, 2022 at 6:21 PM Philippe Altherr
<philippe.altherr@gmail.com> wrote:
>>
>> Namely, Philippe felt it important to
>> have one patch that completely backed out two or more of my previous
>> patches, followed by a patch that incorporated a useful part of one of
>> those patches.
>
> Did I do that?

Hmm.  When I copy-pasted the entire series of 4 patches from the
zsh-workers archive and put the whole thing through a single run of
"patch -p1", halfway through I got a "Reversed (or previously applied)
patch detected" message, which I told patch to ignore and it happily
finished.  But when I download them and apply them one at a time, that
doesn't happen.

So perhaps I misdiagnosed the reason for the "reversed".


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

* Re: Submitting multiple patches (was: Re: [PATCH] Fix ERR_EXIT behavior in function calls and "always" statements)
  2022-11-26  2:28                       ` Bart Schaefer
@ 2022-11-26  4:02                         ` Philippe Altherr
  0 siblings, 0 replies; 20+ messages in thread
From: Philippe Altherr @ 2022-11-26  4:02 UTC (permalink / raw)
  To: Bart Schaefer; +Cc: Daniel Shahaf, zsh-workers, Lawrence Velázquez

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

>
> But when I download them and apply them one at a time, that
> doesn't happen.


Weird that this leads to a different behavior.

Btw, here are the dependencies for the 4 patches:

- patch-01-revert-mistaken-errexit-patches.txt depends on nothing.

- patch-02-fix-always-statement.txt depends on:
  - patch-01-revert-mistaken-errexit-patches.txt

- patch-03-fix-negation-statement.txt depends on:
  - patch-01-revert-mistaken-errexit-patches.txt

- patch-04-fix-function-call.txt depends on:
  - patch-01-revert-mistaken-errexit-patches.txt
  - patch-03-fix-negation-statement.txt

Since the patches constitute 4 separate logical units, I hope that you can
submit them as 4 different patches rather than a single combined one.

Note also that the 4th patch could be combined with my 6th patch (sent
separately), since the latter reverts parts of the former.

Philippe

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

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

end of thread, other threads:[~2022-11-26  4:03 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-16 14:40 [PATCH] Fix ERR_EXIT behavior in function calls and "always" statements Philippe Altherr
2022-11-19 13:39 ` Philippe Altherr
2022-11-21  0:43   ` Bart Schaefer
2022-11-21  7:22     ` Lawrence Velázquez
2022-11-22  2:52       ` Philippe Altherr
2022-11-22  4:51         ` Bart Schaefer
2022-11-22 10:17         ` Peter Stephenson
2022-11-23  8:11           ` Lawrence Velázquez
2022-11-23  6:59         ` Lawrence Velázquez
2022-11-23  9:43           ` Philippe Altherr
2022-11-23 16:11             ` Bart Schaefer
2022-11-23 20:57               ` Submitting multiple patches (was: Re: [PATCH] Fix ERR_EXIT behavior in function calls and "always" statements) Daniel Shahaf
2022-11-23 21:11                 ` Bart Schaefer
2022-11-23 21:22                   ` Bart Schaefer
2022-11-23 21:54                   ` Bart Schaefer
2022-11-24  2:21                     ` Philippe Altherr
2022-11-26  2:28                       ` Bart Schaefer
2022-11-26  4:02                         ` Philippe Altherr
2022-11-24  1:47               ` [PATCH] Fix ERR_EXIT behavior in function calls and "always" statements Philippe Altherr
2022-11-24  4:28             ` Lawrence Velázquez

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).